chore: implement organization sync and create idpsync package#14432
Conversation
a8c89a4 to
565e8ae
Compare
7521495 to
14a4c70
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
14a4c70 to
60e0532
Compare
60e0532 to
aab3940
Compare
1cc418c to
0a49b38
Compare
aab3940 to
fe8d255
Compare
0a49b38 to
369b2dc
Compare
ac21ab3 to
bfe6835
Compare
369b2dc to
2d5e40d
Compare
bfe6835 to
3ead016
Compare
2d5e40d to
028476d
Compare
1c9a08c to
1714e5b
Compare
f0ssel
left a comment
There was a problem hiding this comment.
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.
nit: Is it possible for this to be just map[string][]uuid.UUID?
There was a problem hiding this comment.
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.
We have to use the type serpent.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.
| } | ||
| } | ||
|
|
||
| // Keep track of which claims are not mapped for debugging purposes. |
| 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.
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.
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.
Especially since this is in the enidpsync package. I use a real DB in the coderd/userauth_test.go file.
028476d to
6889229
Compare
1714e5b to
6695e64
Compare
4e3ea79 to
a67ab22
Compare
6889229 to
6dbfe6f
Compare
IDP sync code should be refactored to be contained in it's own package. This will make it easier to maintain and test moving forward.
a67ab22 to
3516008
Compare
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