Skip to content

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

Merged
merged 23 commits into from
Aug 30, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Aug 26, 2024

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

  • Allow configuring at runtime instead (chicken and egg as orgs have to exist)
  • The current config is a bit of a stop gap solution.

@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from a8c89a4 to 565e8ae Compare August 26, 2024 20:32
@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from 7521495 to 14a4c70 Compare August 28, 2024 14:36
@Emyrk Emyrk changed the base branch from main to stevenmasley/organizations_query August 28, 2024 14:36
Copy link
Member Author

Emyrk commented Aug 28, 2024

@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from 14a4c70 to 60e0532 Compare August 28, 2024 14:39
@Emyrk Emyrk changed the base branch from stevenmasley/organizations_query to main August 28, 2024 14:41
@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from 60e0532 to aab3940 Compare August 28, 2024 14:41
@Emyrk Emyrk changed the base branch from main to stevenmasley/slice_unique August 28, 2024 14:41
@Emyrk Emyrk force-pushed the stevenmasley/slice_unique branch from 1cc418c to 0a49b38 Compare August 28, 2024 14:44
@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from aab3940 to fe8d255 Compare August 28, 2024 14:44
@Emyrk Emyrk force-pushed the stevenmasley/slice_unique branch from 0a49b38 to 369b2dc Compare August 28, 2024 16:22
@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from ac21ab3 to bfe6835 Compare August 28, 2024 16:22
@Emyrk Emyrk force-pushed the stevenmasley/slice_unique branch from 369b2dc to 2d5e40d Compare August 28, 2024 16:29
@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from bfe6835 to 3ead016 Compare August 28, 2024 16:29
@Emyrk Emyrk marked this pull request as ready for review August 28, 2024 17:10
@Emyrk Emyrk requested a review from f0ssel August 28, 2024 17:11
@Emyrk Emyrk force-pushed the stevenmasley/slice_unique branch from 2d5e40d to 028476d Compare August 28, 2024 18:24
@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from 1c9a08c to 1714e5b Compare August 28, 2024 18:25
Copy link
Contributor

@f0ssel f0ssel left a 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: {})
Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Member Author

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.
Copy link
Contributor

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")
Copy link
Contributor

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. 👍

Copy link
Member Author

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.

Copy link
Member Author

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.

@Emyrk Emyrk force-pushed the stevenmasley/slice_unique branch from 028476d to 6889229 Compare August 28, 2024 19:34
@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from 1714e5b to 6695e64 Compare August 28, 2024 19:34
@Emyrk Emyrk changed the base branch from stevenmasley/slice_unique to graphite-base/14432 August 29, 2024 02:06
@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from 4e3ea79 to a67ab22 Compare August 29, 2024 02:07
@Emyrk Emyrk force-pushed the graphite-base/14432 branch from 6889229 to 6dbfe6f Compare August 29, 2024 02:07
@Emyrk Emyrk changed the base branch from graphite-base/14432 to main August 29, 2024 02:08
@Emyrk Emyrk force-pushed the stevenmasley/organization_sync branch from a67ab22 to 3516008 Compare August 29, 2024 14:07
@Emyrk Emyrk merged commit 10c958b into main Aug 30, 2024
30 of 32 checks passed
@Emyrk Emyrk deleted the stevenmasley/organization_sync branch August 30, 2024 16:19
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Organization Sync to map OIDC claims -> organization members
2 participants