- Notifications
You must be signed in to change notification settings - Fork928
chore: implement organization sync and create idpsync package#14432
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a8c89a4 to565e8aeCompareUh oh!
There was an error while loading.Please reload this page.
7521495 to14a4c70CompareEmyrk commentedAug 28, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. |
14a4c70 to60e0532Compare60e0532 toaab3940Compare1cc418c to0a49b38Compareaab3940 tofe8d255Compare0a49b38 to369b2dcCompareac21ab3 tobfe6835Compare369b2dc to2d5e40dComparebfe6835 to3ead016Compare2d5e40d to028476dCompare1c9a08c to1714e5bCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pretty much all nits, structurally it all looks good. Some repeated code that could use cleanup, in a later pass.
| This field must be set if using the organization sync feature. Set to | ||
| the claim to be used for organizations. | ||
| --oidc-organization-mapping struct[map[string][]uuid.UUID], $CODER_OIDC_ORGANIZATION_MAPPING (default: {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: Is it possible for this to be justmap[string][]uuid.UUID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you explain why we need this instead of solely relying on the--oidc-organization-field? Orgs are uniquely named right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We have to use the typeserpent.Struct[map[string][]uuid.UUID] to be compatible with all parsing.
Can you explain why we need this instead of solely relying on the --oidc-organization-field? Orgs are uniquely named right?
I could use names, but that assumes the IDP will use Coder organization names in their claims, which we've found with groups that this almost never the case.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| } | ||
| // Keep track of which claims are not mapped for debugging purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| t.Parallel() | ||
| if dbtestutil.WillUsePostgres() { | ||
| t.Skip("Skipping test because it populates a lot of db entries, which is slow on postgres") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this is OK since we aren't introducing new tables or columns and using existing (and well tested) data structures. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yea, I've done this a few times now when I need to populate a lot of data. Essentially, this test is not designed to test the database, but test the logic in the sync method. There is little upside to making this a full SQL db imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Especially since this is in theenidpsync package. I use a real DB in thecoderd/userauth_test.go file.
028476d to6889229Compare1714e5b to6695e64Compare4e3ea79 toa67ab22CompareIDP sync code should be refactored to be contained in it's ownpackage. This will make it easier to maintain and test movingforward.
a67ab22 to3516008Compare10c958b intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
What this does
Adds organization sync as a feature configurable at startup. This means a deployment can now be configured to allow a user to belong to 0 organizations on login if configured to do so. The default behavior is unchanged if no settings are touched.
This features allows auto assigning users into an organization based on their IDP claims. At present, an org mapping is required, so not mapped by organization name. Might be an improvement to allow org names in the future. TBD.
This is MVP and unreleased at this time.
Closes#14350
Refactor
I moved all this code into an
idpsyncpackage. I intend to move role and group sync into this package.Future work