coder · Emyrk · Feb 16, 2024 · Feb 14, 2024 · Feb 15, 2024 · Feb 15, 2024
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -134,7 +134,7 @@ type Options struct {
 	BaseDERPMap                 *tailcfg.DERPMap
 	DERPMapUpdateFrequency      time.Duration
 	SwaggerEndpoint             bool
-	SetUserGroups               func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, groupNames []string, createMissingGroups bool) error
+	SetUserGroups               func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error
 	SetUserSiteRoles            func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, roles []string) error
 	TemplateScheduleStore       *atomic.Pointer[schedule.TemplateScheduleStore]
 	UserQuietHoursScheduleStore *atomic.Pointer[schedule.UserQuietHoursScheduleStore]
@@ -301,9 +301,11 @@ func New(options *Options) *API {
 		options.TracerProvider = trace.NewNoopTracerProvider()
 	}
 	if options.SetUserGroups == nil {
-		options.SetUserGroups = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, groups []string, createMissingGroups bool) error {
+		options.SetUserGroups = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error {
 			logger.Warn(ctx, "attempted to assign OIDC groups without enterprise license",
-				slog.F("user_id", userID), slog.F("groups", groups), slog.F("create_missing_groups", createMissingGroups),
+				slog.F("user_id", userID),
+				slog.F("groups", orgGroupNames),
+				slog.F("create_missing_groups", createMissingGroups),
 			)
 			return nil
 		}

diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -793,16 +793,6 @@ func (q *querier) DeleteGroupMemberFromGroup(ctx context.Context, arg database.D
 	return update(q.log, q.auth, fetch, q.db.DeleteGroupMemberFromGroup)(ctx, arg)
 }
 
-func (q *querier) DeleteGroupMembersByOrgAndUser(ctx context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) error {
-	// This will remove the user from all groups in the org. This counts as updating a group.
-	// NOTE: instead of fetching all groups in the org with arg.UserID as a member, we instead
-	// check if the caller has permission to update any group in the org.
-	fetch := func(ctx context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) (rbac.Objecter, error) {
-		return rbac.ResourceGroup.InOrg(arg.OrganizationID), nil
-	}
-	return update(q.log, q.auth, fetch, q.db.DeleteGroupMembersByOrgAndUser)(ctx, arg)
-}
-
 func (q *querier) DeleteLicense(ctx context.Context, id int32) (int32, error) {
 	err := deleteQ(q.log, q.auth, q.db.GetLicenseByID, func(ctx context.Context, id int32) error {
 		_, err := q.db.DeleteLicense(ctx, id)
@@ -2555,6 +2545,14 @@ func (q *querier) RegisterWorkspaceProxy(ctx context.Context, arg database.Regis
 	return updateWithReturn(q.log, q.auth, fetch, q.db.RegisterWorkspaceProxy)(ctx, arg)
 }
 
+func (q *querier) RemoveUserFromAllGroups(ctx context.Context, userID uuid.UUID) error {
+	// This is a system function to clear user groups in group sync.
+	if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil {
+		return err
+	}
+	return q.db.RemoveUserFromAllGroups(ctx, userID)
+}
+
 func (q *querier) RevokeDBCryptKey(ctx context.Context, activeKeyDigest string) error {
 	if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil {
 		return err

diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -344,17 +344,14 @@ func (s *MethodTestSuite) TestGroup() {
 			GroupNames:     slice.New(g1.Name, g2.Name),
 		}).Asserts(rbac.ResourceGroup.InOrg(o.ID), rbac.ActionUpdate).Returns()
 	}))
-	s.Run("DeleteGroupMembersByOrgAndUser", s.Subtest(func(db database.Store, check *expects) {
+	s.Run("RemoveUserFromAllGroups", s.Subtest(func(db database.Store, check *expects) {
 		o := dbgen.Organization(s.T(), db, database.Organization{})
 		u1 := dbgen.User(s.T(), db, database.User{})
 		g1 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
 		g2 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
 		_ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g1.ID, UserID: u1.ID})
 		_ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g2.ID, UserID: u1.ID})
-		check.Args(database.DeleteGroupMembersByOrgAndUserParams{
-			OrganizationID: o.ID,
-			UserID:         u1.ID,
-		}).Asserts(rbac.ResourceGroup.InOrg(o.ID), rbac.ActionUpdate).Returns()
+		check.Args(u1.ID).Asserts(rbac.ResourceSystem, rbac.ActionUpdate).Returns()
 	}))
 	s.Run("UpdateGroupByID", s.Subtest(func(db database.Store, check *expects) {
 		g := dbgen.Group(s.T(), db, database.Group{})

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -1135,36 +1135,6 @@ func (q *FakeQuerier) DeleteGroupMemberFromGroup(_ context.Context, arg database
 	return nil
 }
 
-func (q *FakeQuerier) DeleteGroupMembersByOrgAndUser(_ context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) error {
-	q.mutex.Lock()
-	defer q.mutex.Unlock()
-
-	newMembers := q.groupMembers[:0]
-	for _, member := range q.groupMembers {
-		if member.UserID != arg.UserID {
-			// Do not delete the other members
-			newMembers = append(newMembers, member)
-		} else if member.UserID == arg.UserID {
-			// We only want to delete from groups in the organization in the args.
-			for _, group := range q.groups {
-				// Find the group that the member is apartof.
-				if group.ID == member.GroupID {
-					// Only add back the member if the organization ID does not match
-					// the arg organization ID. Since the arg is saying which
-					// org to delete.
-					if group.OrganizationID != arg.OrganizationID {
-						newMembers = append(newMembers, member)
-					}
-					break
-				}
-			}
-		}
-	}
-	q.groupMembers = newMembers
-
-	return nil
-}
-
 func (q *FakeQuerier) DeleteLicense(_ context.Context, id int32) (int32, error) {
 	q.mutex.Lock()
 	defer q.mutex.Unlock()
@@ -6096,6 +6066,22 @@ func (q *FakeQuerier) RegisterWorkspaceProxy(_ context.Context, arg database.Reg
 	return database.WorkspaceProxy{}, sql.ErrNoRows
 }
 
+func (q *FakeQuerier) RemoveUserFromAllGroups(_ context.Context, userID uuid.UUID) error {
+	q.mutex.Lock()
+	defer q.mutex.Unlock()
+
+	newMembers := q.groupMembers[:0]
+	for _, member := range q.groupMembers {
+		if member.UserID == userID {
+			continue
+		}
+		newMembers = append(newMembers, member)
+	}
+	q.groupMembers = newMembers
+
+	return nil
+}
+
 func (q *FakeQuerier) RevokeDBCryptKey(_ context.Context, activeKeyDigest string) error {
 	q.mutex.Lock()
 	defer q.mutex.Unlock()

diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go
diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go
diff --git a/coderd/database/querier.go b/coderd/database/querier.go
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql
@@ -42,12 +42,11 @@ SELECT
 FROM
     groups;
 
--- name: DeleteGroupMembersByOrgAndUser :exec
+-- name: RemoveUserFromAllGroups :exec
 DELETE FROM
 	group_members
 WHERE
-	group_members.user_id = @user_id
-	AND group_id = ANY(SELECT id FROM groups WHERE organization_id = @organization_id);
+	user_id = @user_id;
 
 -- name: InsertGroupMember :exec
 INSERT INTO

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -20,6 +20,7 @@ import (
 	"github.com/google/go-github/v43/github"
 	"github.com/google/uuid"
 	"github.com/moby/moby/pkg/namesgenerator"
+	"golang.org/x/exp/slices"
 	"golang.org/x/oauth2"
 	"golang.org/x/xerrors"
 
@@ -1217,8 +1218,10 @@ type oauthLoginParams struct {
 	// to the Groups provided.
 	UsingGroups         bool
 	CreateMissingGroups bool
-	Groups              []string
-	GroupFilter         *regexp.Regexp
+	// These are the group names from the IDP. Internally, they will map to
+	// some organization groups.
+	Groups      []string
+	GroupFilter *regexp.Regexp
 	// Is UsingRoles is true, then the user will be assigned
 	// the roles provided.
 	UsingRoles bool
@@ -1301,7 +1304,6 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
 			link database.UserLink
 			err  error
 		)
-
 		user = params.User
 		link = params.Link
 
@@ -1460,6 +1462,9 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
 		}
 
 		// Ensure groups are correct.
+		// This places all groups into the default organization.
+		// To go multi-org, we need to add a mapping feature here to know which
+		// groups go to which orgs.
 		if params.UsingGroups {
 			filtered := params.Groups
 			if params.GroupFilter != nil {
@@ -1471,8 +1476,32 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
 				}
 			}
 
+			//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)
+			}
+
+			// If the user is not in the default organization, then we can't assign groups.
+			// A user cannot be in groups to an org they are not a member of.
+			if !slices.ContainsFunc(memberships, func(member database.OrganizationMember) bool {
+				return member.OrganizationID == defaultOrganization.ID
+			}) {
+				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)
 			if err != nil {
 				return xerrors.Errorf("set user groups: %w", err)
 			}