Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Move authn into cmd/frontend #63648

Conversation

eseliger
Copy link
Member

@eseliger eseliger commented Jul 4, 2024

They should not be used outside of cmd/frontend, so making it a frontend internal package.

While doing that, I realized that there is a coupling dependency between authz providers and auth (which is authN) providers: GitLab code host connections can do authz mapping via the usernames of another OIDC or SAML auth provider (https://sourcegraph.com/docs/admin/code_hosts/gitlab#administrator-sudo-level-access-token). It turns out this feature does not work anymore, since at least several releases, because we don't actually instantiate auth providers outside of cmd/frontend and thus the mapping will never find anything (auth providers don't explode when queried before init, unlike authz).
This only now became clear as I moved this code, and the dependency graph was broken, so that's a nice property of these cleanups I guess 😬
Since it doesn't seem to work for quite some time, I opted for removing it, and added a changelog entry about it. Not sure if that is sufficient, I raised a thread here: https://sourcegraph.slack.com/archives/C03K05FCRFH/p1721848436473209.
This would've prevented this change and needed more refactoring as unfortunately we cannot map an auth provider by the conf type to a record in the user_external_accounts table and need to actually instantiate it.

Test plan: Compiler doesn't complain, tests still pass.

Changelog

GitLab code host connections were able to sync permissions by mapping Sourcegraph users to GitLab users via the username property of an external OIDC or SAML provider that is shared across Sourcegraph and GitLab. This integration stopped working a long time ago, and it has been removed in this release.

Copy link
Member Author

eseliger commented Jul 4, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @eseliger and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Jul 4, 2024
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore branch from 41f90b8 to b582762 Compare July 4, 2024 20:29
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from 48139d4 to 32505c0 Compare July 4, 2024 20:29
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore branch from b582762 to 3f0f443 Compare July 5, 2024 05:15
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from 32505c0 to 0a62e2a Compare July 5, 2024 05:15
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore branch from 3f0f443 to c7fe056 Compare July 5, 2024 07:36
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from 0a62e2a to 8827da3 Compare July 5, 2024 07:36
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore branch from c7fe056 to 3d8e03d Compare July 5, 2024 11:28
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from 8827da3 to 4563099 Compare July 5, 2024 11:28
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore branch from 3d8e03d to d01bc3b Compare July 9, 2024 22:37
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from 4563099 to 96515fd Compare July 9, 2024 22:37
Copy link
Contributor

github-actions bot commented Jul 9, 2024

Caution

License checking failed, please read: how to deal with third parties licensing.

@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore branch from d01bc3b to 3fb5d88 Compare July 9, 2024 23:25
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from 96515fd to 49c2385 Compare July 9, 2024 23:26
Copy link
Contributor

github-actions bot commented Jul 9, 2024

Caution

License checking failed, please read: how to deal with third parties licensing.

@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore branch from 3fb5d88 to 1cc6bcc Compare July 9, 2024 23:56
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from 49c2385 to 4eafde8 Compare July 9, 2024 23:57
Copy link
Contributor

github-actions bot commented Jul 9, 2024

Caution

License checking failed, please read: how to deal with third parties licensing.

@eseliger eseliger force-pushed the es/07-23-authzcomputeprovidersonthefly branch from e8fbddf to 2f13f50 Compare July 28, 2024 14:44
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from fcb7563 to fe401db Compare July 28, 2024 14:44
@eseliger eseliger changed the title Move authn into cmd/frontend chore: Move authn into cmd/frontend Jul 29, 2024
@eseliger eseliger force-pushed the es/07-23-authzcomputeprovidersonthefly branch from 2f13f50 to bb95aa9 Compare July 30, 2024 00:47
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from fe401db to 6b20598 Compare July 30, 2024 00:47
@eseliger eseliger requested a review from a team July 30, 2024 13:35
@eseliger eseliger force-pushed the es/07-23-authzcomputeprovidersonthefly branch from bb95aa9 to 13b3a9c Compare July 30, 2024 17:21
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from 6b20598 to b947d99 Compare July 30, 2024 17:21
@eseliger eseliger marked this pull request as ready for review July 30, 2024 17:26
@eseliger eseliger force-pushed the es/07-23-authzcomputeprovidersonthefly branch from 13b3a9c to 980aa69 Compare July 30, 2024 23:23
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from b947d99 to ab3a565 Compare July 30, 2024 23:24
@eseliger eseliger force-pushed the es/07-23-authzcomputeprovidersonthefly branch from 980aa69 to 2e603ee Compare July 31, 2024 00:46
@eseliger eseliger changed the base branch from es/07-23-authzcomputeprovidersonthefly to graphite-base/63648 July 31, 2024 00:59
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from ab3a565 to 80b67b9 Compare July 31, 2024 01:00
@graphite-app graphite-app bot changed the base branch from graphite-base/63648 to main July 31, 2024 01:00
They should not be used outside of cmd/frontend, so making it a frontend internal package.

Test plan:

Compiler doesn't complain, tests still pass
@eseliger eseliger force-pushed the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch from 80b67b9 to ce3a944 Compare July 31, 2024 01:15
@eseliger eseliger merged commit c4c375a into main Jul 31, 2024
14 checks passed
@eseliger eseliger deleted the es/07-03-unexportsomeexternallyirrelevantsymbolsfromuploadstore_split branch July 31, 2024 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants