Skip to content

fix: assign new oauth users to default org #12145

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 5 commits into from
Feb 16, 2024
Merged

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Feb 14, 2024

Begins work on #11922

This is not a final solution, as we eventually want to be able to map to different orgs.
This PR just makes it so multi-org does not break oauth/oidc.

Copy link
Member Author

Emyrk commented Feb 14, 2024

@Emyrk Emyrk force-pushed the stevenmasley/post_user branch from 60775a6 to 04c6149 Compare February 14, 2024 18:08
@Emyrk Emyrk force-pushed the stevenmasley/oauth_user branch from f999de5 to 5322f4e Compare February 14, 2024 18:09
@Emyrk Emyrk changed the title fix: new oauth users assigned to default org fix: assign new oauth users to default org Feb 14, 2024
Comment on lines +1346 to +1351
organization, _ := tx.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx))

// Add the user to the default organization.
// Once multi-organization we should check some configuration to see
// if we should add the user to a different organization.
organizationID = organization.ID
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is annoyingly allowed to be uuid.Nil in unit tests. I am not going to change the behavior here, but we should really fix that so we don't have an edge case living for the sake of tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine not to fix here, but can we add a comment for the ignored err above (tx.GetDefaultOrganization) about this? Comment is just a band-aid though, we definitely want to nip this in the bud before too long.

@Emyrk Emyrk requested a review from mafredri February 15, 2024 15:31
@Emyrk Emyrk force-pushed the stevenmasley/post_user branch from 04c6149 to 7059828 Compare February 15, 2024 16:28
@Emyrk Emyrk force-pushed the stevenmasley/oauth_user branch from 5322f4e to ddb828c Compare February 15, 2024 16:28
@Emyrk Emyrk force-pushed the stevenmasley/post_user branch 2 times, most recently from aeb29f2 to 8f61a71 Compare February 15, 2024 17:18
@Emyrk Emyrk force-pushed the stevenmasley/oauth_user branch from ddb828c to fc2fce2 Compare February 15, 2024 17:58
This is not a final solution, as we eventually want to be able
to map to different orgs. This makes it so multi-org does not break oauth/oidc.
@Emyrk Emyrk force-pushed the stevenmasley/oauth_user branch from fc2fce2 to 872f4a2 Compare February 15, 2024 18:46
Comment on lines +1346 to +1351
organization, _ := tx.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx))

// Add the user to the default organization.
// Once multi-organization we should check some configuration to see
// if we should add the user to a different organization.
organizationID = organization.ID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine not to fix here, but can we add a comment for the ignored err above (tx.GetDefaultOrganization) about this? Comment is just a band-aid though, we definitely want to nip this in the bud before too long.

Base automatically changed from stevenmasley/post_user to main February 16, 2024 14:28
@Emyrk Emyrk merged commit 75870c2 into main Feb 16, 2024
@Emyrk Emyrk deleted the stevenmasley/oauth_user branch February 16, 2024 14:47
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 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.

2 participants