Skip to content

Commit f17149c

Browse files
EmyrkEmyrk
authored
feat: set groupsync to use default org (#12146)
* fix: assign new oauth users to default org 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.
1 parent dbaafc8 commit f17149c

File tree

11 files changed

+136
-123
lines changed

11 files changed

+136
-123
lines changed

‎coderd/coderd.go

+5-3
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ type Options struct {
134134
BaseDERPMap *tailcfg.DERPMap
135135
DERPMapUpdateFrequency time.Duration
136136
SwaggerEndpoint bool
137-
SetUserGroups func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, groupNames []string, createMissingGroups bool) error
137+
SetUserGroups func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error
138138
SetUserSiteRoles func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, roles []string) error
139139
TemplateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore]
140140
UserQuietHoursScheduleStore *atomic.Pointer[schedule.UserQuietHoursScheduleStore]
@@ -301,9 +301,11 @@ func New(options *Options) *API {
301301
options.TracerProvider = trace.NewNoopTracerProvider()
302302
}
303303
if options.SetUserGroups == nil {
304-
options.SetUserGroups = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, groups []string, createMissingGroups bool) error {
304+
options.SetUserGroups = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error {
305305
logger.Warn(ctx, "attempted to assign OIDC groups without enterprise license",
306-
slog.F("user_id", userID), slog.F("groups", groups), slog.F("create_missing_groups", createMissingGroups),
306+
slog.F("user_id", userID),
307+
slog.F("groups", orgGroupNames),
308+
slog.F("create_missing_groups", createMissingGroups),
307309
)
308310
return nil
309311
}

‎coderd/database/dbauthz/dbauthz.go

+8-10
Original file line numberDiff line numberDiff line change
@@ -793,16 +793,6 @@ func (q *querier) DeleteGroupMemberFromGroup(ctx context.Context, arg database.D
793793
return update(q.log, q.auth, fetch, q.db.DeleteGroupMemberFromGroup)(ctx, arg)
794794
}
795795

796-
func (q *querier) DeleteGroupMembersByOrgAndUser(ctx context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) error {
797-
// This will remove the user from all groups in the org. This counts as updating a group.
798-
// NOTE: instead of fetching all groups in the org with arg.UserID as a member, we instead
799-
// check if the caller has permission to update any group in the org.
800-
fetch := func(ctx context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) (rbac.Objecter, error) {
801-
return rbac.ResourceGroup.InOrg(arg.OrganizationID), nil
802-
}
803-
return update(q.log, q.auth, fetch, q.db.DeleteGroupMembersByOrgAndUser)(ctx, arg)
804-
}
805-
806796
func (q *querier) DeleteLicense(ctx context.Context, id int32) (int32, error) {
807797
err := deleteQ(q.log, q.auth, q.db.GetLicenseByID, func(ctx context.Context, id int32) error {
808798
_, err := q.db.DeleteLicense(ctx, id)
@@ -2555,6 +2545,14 @@ func (q *querier) RegisterWorkspaceProxy(ctx context.Context, arg database.Regis
25552545
return updateWithReturn(q.log, q.auth, fetch, q.db.RegisterWorkspaceProxy)(ctx, arg)
25562546
}
25572547

2548+
func (q *querier) RemoveUserFromAllGroups(ctx context.Context, userID uuid.UUID) error {
2549+
// This is a system function to clear user groups in group sync.
2550+
if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil {
2551+
return err
2552+
}
2553+
return q.db.RemoveUserFromAllGroups(ctx, userID)
2554+
}
2555+
25582556
func (q *querier) RevokeDBCryptKey(ctx context.Context, activeKeyDigest string) error {
25592557
if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil {
25602558
return err

‎coderd/database/dbauthz/dbauthz_test.go

+2-5
Original file line numberDiff line numberDiff line change
@@ -344,17 +344,14 @@ func (s *MethodTestSuite) TestGroup() {
344344
GroupNames: slice.New(g1.Name, g2.Name),
345345
}).Asserts(rbac.ResourceGroup.InOrg(o.ID), rbac.ActionUpdate).Returns()
346346
}))
347-
s.Run("DeleteGroupMembersByOrgAndUser", s.Subtest(func(db database.Store, check *expects) {
347+
s.Run("RemoveUserFromAllGroups", s.Subtest(func(db database.Store, check *expects) {
348348
o := dbgen.Organization(s.T(), db, database.Organization{})
349349
u1 := dbgen.User(s.T(), db, database.User{})
350350
g1 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
351351
g2 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
352352
_ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g1.ID, UserID: u1.ID})
353353
_ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g2.ID, UserID: u1.ID})
354-
check.Args(database.DeleteGroupMembersByOrgAndUserParams{
355-
OrganizationID: o.ID,
356-
UserID: u1.ID,
357-
}).Asserts(rbac.ResourceGroup.InOrg(o.ID), rbac.ActionUpdate).Returns()
354+
check.Args(u1.ID).Asserts(rbac.ResourceSystem, rbac.ActionUpdate).Returns()
358355
}))
359356
s.Run("UpdateGroupByID", s.Subtest(func(db database.Store, check *expects) {
360357
g := dbgen.Group(s.T(), db, database.Group{})

‎coderd/database/dbmem/dbmem.go

+16-30
Original file line numberDiff line numberDiff line change
@@ -1135,36 +1135,6 @@ func (q *FakeQuerier) DeleteGroupMemberFromGroup(_ context.Context, arg database
11351135
return nil
11361136
}
11371137

1138-
func (q *FakeQuerier) DeleteGroupMembersByOrgAndUser(_ context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) error {
1139-
q.mutex.Lock()
1140-
defer q.mutex.Unlock()
1141-
1142-
newMembers := q.groupMembers[:0]
1143-
for _, member := range q.groupMembers {
1144-
if member.UserID != arg.UserID {
1145-
// Do not delete the other members
1146-
newMembers = append(newMembers, member)
1147-
} else if member.UserID == arg.UserID {
1148-
// We only want to delete from groups in the organization in the args.
1149-
for _, group := range q.groups {
1150-
// Find the group that the member is apartof.
1151-
if group.ID == member.GroupID {
1152-
// Only add back the member if the organization ID does not match
1153-
// the arg organization ID. Since the arg is saying which
1154-
// org to delete.
1155-
if group.OrganizationID != arg.OrganizationID {
1156-
newMembers = append(newMembers, member)
1157-
}
1158-
break
1159-
}
1160-
}
1161-
}
1162-
}
1163-
q.groupMembers = newMembers
1164-
1165-
return nil
1166-
}
1167-
11681138
func (q *FakeQuerier) DeleteLicense(_ context.Context, id int32) (int32, error) {
11691139
q.mutex.Lock()
11701140
defer q.mutex.Unlock()
@@ -6096,6 +6066,22 @@ func (q *FakeQuerier) RegisterWorkspaceProxy(_ context.Context, arg database.Reg
60966066
return database.WorkspaceProxy{}, sql.ErrNoRows
60976067
}
60986068

6069+
func (q *FakeQuerier) RemoveUserFromAllGroups(_ context.Context, userID uuid.UUID) error {
6070+
q.mutex.Lock()
6071+
defer q.mutex.Unlock()
6072+
6073+
newMembers := q.groupMembers[:0]
6074+
for _, member := range q.groupMembers {
6075+
if member.UserID == userID {
6076+
continue
6077+
}
6078+
newMembers = append(newMembers, member)
6079+
}
6080+
q.groupMembers = newMembers
6081+
6082+
return nil
6083+
}
6084+
60996085
func (q *FakeQuerier) RevokeDBCryptKey(_ context.Context, activeKeyDigest string) error {
61006086
q.mutex.Lock()
61016087
defer q.mutex.Unlock()

‎coderd/database/dbmetrics/dbmetrics.go

+7-7
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

+14-14
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/querier.go

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

+12-18
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/groupmembers.sql

+2-3
Original file line numberDiff line numberDiff line change
@@ -42,12 +42,11 @@ SELECT
4242
FROM
4343
groups;
4444

45-
-- name: DeleteGroupMembersByOrgAndUser :exec
45+
-- name: RemoveUserFromAllGroups :exec
4646
DELETE FROM
4747
group_members
4848
WHERE
49-
group_members.user_id = @user_id
50-
AND group_id = ANY(SELECT id FROM groups WHERE organization_id = @organization_id);
49+
user_id = @user_id;
5150

5251
-- name: InsertGroupMember :exec
5352
INSERT INTO

‎coderd/userauth.go

+33-4
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/google/go-github/v43/github"
2121
"github.com/google/uuid"
2222
"github.com/moby/moby/pkg/namesgenerator"
23+
"golang.org/x/exp/slices"
2324
"golang.org/x/oauth2"
2425
"golang.org/x/xerrors"
2526

@@ -1217,8 +1218,10 @@ type oauthLoginParams struct {
12171218
// to the Groups provided.
12181219
UsingGroups bool
12191220
CreateMissingGroups bool
1220-
Groups []string
1221-
GroupFilter *regexp.Regexp
1221+
// These are the group names from the IDP. Internally, they will map to
1222+
// some organization groups.
1223+
Groups []string
1224+
GroupFilter *regexp.Regexp
12221225
// Is UsingRoles is true, then the user will be assigned
12231226
// the roles provided.
12241227
UsingRoles bool
@@ -1301,7 +1304,6 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
13011304
link database.UserLink
13021305
err error
13031306
)
1304-
13051307
user = params.User
13061308
link = params.Link
13071309

@@ -1460,6 +1462,9 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
14601462
}
14611463

14621464
// Ensure groups are correct.
1465+
// This places all groups into the default organization.
1466+
// To go multi-org, we need to add a mapping feature here to know which
1467+
// groups go to which orgs.
14631468
if params.UsingGroups {
14641469
filtered := params.Groups
14651470
if params.GroupFilter != nil {
@@ -1471,8 +1476,32 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
14711476
}
14721477
}
14731478

1479+
//nolint:gocritic // No user present in the context.
1480+
defaultOrganization, err := tx.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx))
1481+
if err != nil {
1482+
// If there is no default org, then we can't assign groups.
1483+
// By default, we assume all groups belong to the default org.
1484+
return xerrors.Errorf("get default organization: %w", err)
1485+
}
1486+
1487+
//nolint:gocritic // No user present in the context.
1488+
memberships, err := tx.GetOrganizationMembershipsByUserID(dbauthz.AsSystemRestricted(ctx), user.ID)
1489+
if err != nil {
1490+
return xerrors.Errorf("get organization memberships: %w", err)
1491+
}
1492+
1493+
// If the user is not in the default organization, then we can't assign groups.
1494+
// A user cannot be in groups to an org they are not a member of.
1495+
if !slices.ContainsFunc(memberships, func(member database.OrganizationMember) bool {
1496+
return member.OrganizationID == defaultOrganization.ID
1497+
}) {
1498+
return xerrors.Errorf("user %s is not a member of the default organization, cannot assign to groups in the org", user.ID)
1499+
}
1500+
14741501
//nolint:gocritic
1475-
err := api.Options.SetUserGroups(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, filtered, params.CreateMissingGroups)
1502+
err = api.Options.SetUserGroups(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, map[uuid.UUID][]string{
1503+
defaultOrganization.ID: filtered,
1504+
}, params.CreateMissingGroups)
14761505
if err != nil {
14771506
return xerrors.Errorf("set user groups: %w", err)
14781507
}

0 commit comments

Comments
 (0)