This repository was archived by the owner on Sep 30, 2024. It is now read-only.
authz: Drop requirement for installing authz providers in every service#63743
Merged
Conversation
081686b to
541ac80
Compare
Member
Author
c1fc0b7 to
8788a18
Compare
eseliger
commented
Jul 19, 2024
Member
Author
There was a problem hiding this comment.
this join was missing, the authzquery conds could generate an invalid query here
Contributor
There was a problem hiding this comment.
Is this an unrelated fix that you just noticed?
Member
Author
There was a problem hiding this comment.
no, now that perms are actually enforced in tests by default this showed in a SQL error during tests 🙈
eseliger
commented
Jul 19, 2024
Member
Author
There was a problem hiding this comment.
this matches how this is invoked at runtime
276943f to
56a265c
Compare
56a265c to
70d5438
Compare
This was referenced Jul 24, 2024
70d5438 to
235c970
Compare
This was referenced Jul 24, 2024
This was referenced Jul 25, 2024
ggilmore
reviewed
Jul 26, 2024
ggilmore
reviewed
Jul 26, 2024
ggilmore
left a comment
Contributor
There was a problem hiding this comment.
This looks good overall, but I'm a bit lost on some of the changes (the ones I commented on) - hard for me to tell if those changes are unrelated fixes or of I'm missing something about the impact of the change here
ggilmore
approved these changes
Jul 26, 2024
235c970 to
20e7396
Compare
pjlast
approved these changes
Jul 29, 2024
pjlast
left a comment
Contributor
There was a problem hiding this comment.
Seems good to me. Trusting tests and manual testing here as well
20e7396 to
89f2377
Compare
89f2377 to
e4136ae
Compare
This was referenced Jul 30, 2024
This was referenced Aug 4, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a register call that is easy to forget. When forgotten, all queries against the repo store will block forever.
In addition, this adds a hard-dependency on conf to every services startup, plus a busy loop. With multi-tenant, this will not work great because authz providers would be a global, and we instead want most things to be ephemeral so they're per-provider. This is a step toward that, but doesn't yet remove the providers global variable.
Good news, it turns out that we don't actually need to register the providers in every service! The reason they were required was to check if zero providers are configured, or if authzbypass mode is enabled.
Authz bypass mode is usually ON, except when there are problems with the authz providers, meaning some authz providers might not be able to sync permissions. Bypassing of permissions is only ever happening if there are ALSO zero providers configured.
So this is basically an optimization for the case where an instance has zero authz configured so that the SQL queries are a bit simpler. This also helps in tests because with bypass mode on and no providers configured, authz enforcement is effectively off in the repo store.
This makes it so that in tests we need to do slightly more work, but also makes for a more realistic test vs at runtime setup. Also, it's highly recommended to use mocks for DB wherever possible in more high-level components to keep tests fast.
To never have a scenario where we accidentally mess up here and enable bypass mode erroneously, this PR drops that entirely. Authz is always enforced, but when a code host connection is unrestricted (i.e., will not spawn a provider) the repos are still visible, so this should be no change over before.
Test plan
The stack starts and works, and all CI tests are still passing. Code review should help as well.