-
Notifications
You must be signed in to change notification settings - Fork 876
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
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.
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 just map[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 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. |
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 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
idpsync
package. I intend to move role and group sync into this package.Future work