Skip to content

feat: set groupsync to use default org #12146

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

feat: set groupsync to use default org #12146

merged 6 commits into from
Feb 16, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Feb 14, 2024

Closes #11938

Comment on lines 1475 to 1504
//nolint:gocritic // No user present in the context.
defaultOrganization, err := tx.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx))
if err != nil {
// If there is no default org, then we can't assign groups.
// By default, we assume all groups belong to the default org.
return xerrors.Errorf("get default organization: %w", err)
}

//nolint:gocritic // No user present in the context.
memberships, err := tx.GetOrganizationMembershipsByUserID(dbauthz.AsSystemRestricted(ctx), user.ID)
if err != nil {
return xerrors.Errorf("get organization memberships: %w", err)
}

inDefault := false
for _, membership := range memberships {
if membership.OrganizationID == defaultOrganization.ID {
inDefault = true
break
}
}

if !inDefault {
return xerrors.Errorf("user %s is not a member of the default organization, cannot assign to groups in the org", user.ID)
}

//nolint:gocritic
err := api.Options.SetUserGroups(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, filtered, params.CreateMissingGroups)
err = api.Options.SetUserGroups(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, map[uuid.UUID][]string{
defaultOrganization.ID: filtered,
}, params.CreateMissingGroups)
Copy link
Member Author

Choose a reason for hiding this comment

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

I do not love how much code this ended up being. I can reduce it by adding another query, but this is all temporary. We need to solve group mapping to orgs, this code is static to only work with the default org.

This PR is required to remove the single org assumption.

Comment on lines +44 to +47
// TODO: This could likely be improved by making these single queries.
// Either by batching or some other means. This for loop could be really
// inefficient if there are a lot of organizations. There was deployments
// on v1 with >100 orgs.
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 just phase 1 to getting the codebase to accept multi orgs. I do not want to prematurely optimize by coming up with some batching that might be removed.

I think we should move to pgx as our pg driver in sqlc to get batching support.

@Emyrk Emyrk changed the title feat: groupsync set groups defaults to default org feat: set groupsync to use default org Feb 14, 2024
@Emyrk Emyrk marked this pull request as ready for review February 14, 2024 19:34
@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/set_groups branch from 9e0314c to 723f42d Compare February 15, 2024 16:28
@Emyrk Emyrk force-pushed the stevenmasley/oauth_user branch from ddb828c to fc2fce2 Compare February 15, 2024 17:58
@Emyrk Emyrk force-pushed the stevenmasley/set_groups branch from 723f42d to 6b10251 Compare February 15, 2024 17:59
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
@Emyrk Emyrk force-pushed the stevenmasley/set_groups branch 2 times, most recently from 5bbfa1a to ced4905 Compare February 15, 2024 18:49
@Emyrk Emyrk force-pushed the stevenmasley/set_groups branch from ced4905 to da6c21c Compare February 15, 2024 19:34
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding comments along the way!

inDefault = true
break
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@@ -1301,7 +1303,6 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
link database.UserLink
err error
)

user = params.User
link = params.Link

Copy link
Member

Choose a reason for hiding this comment

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

[Re: line 1347]

Linter is not happy, rebase issue?

See this comment inline on Graphite.

Base automatically changed from stevenmasley/oauth_user to main February 16, 2024 14:47
@Emyrk Emyrk merged commit f17149c into main Feb 16, 2024
@Emyrk Emyrk deleted the stevenmasley/set_groups branch February 16, 2024 17:09
@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.

setUserGroups requires 1 org
2 participants