chore: remove old group sync code

coder · Emyrk · Sep 11, 2024 · Sep 3, 2024 · Sep 3, 2024 · Sep 3, 2024
commit 164aeacebac6f544d85c73d4cd83fe028ce5259a
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -181,7 +181,6 @@ type Options struct {
 	NetworkTelemetryBatchFrequency time.Duration
 	NetworkTelemetryBatchMaxSize   int
 	SwaggerEndpoint                bool
-	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]
@@ -374,16 +373,6 @@ func New(options *Options) *API {
 	if options.TracerProvider == nil {
 		options.TracerProvider = trace.NewNoopTracerProvider()
 	}
-	if options.SetUserGroups == nil {
-		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", orgGroupNames),
-				slog.F("create_missing_groups", createMissingGroups),
-			)
-			return nil
-		}
-	}
 	if options.SetUserSiteRoles == nil {
 		options.SetUserSiteRoles = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, roles []string) error {
 			logger.Warn(ctx, "attempted to assign OIDC user roles without enterprise license",

diff --git a/coderd/idpsync/group.go b/coderd/idpsync/group.go
@@ -14,6 +14,7 @@ import (
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/db2sdk"
 	"github.com/coder/coder/v2/coderd/database/dbauthz"
+	"github.com/coder/coder/v2/coderd/runtimeconfig"
 	"github.com/coder/coder/v2/coderd/util/slice"
 )
 
@@ -76,7 +77,12 @@ func (s AGPLIDPSync) SyncGroups(ctx context.Context, db database.Store, user dat
 			orgResolver := s.Manager.OrganizationResolver(tx, orgID)
 			settings, err := s.SyncSettings.Group.Resolve(ctx, orgResolver)
 			if err != nil {
-				return xerrors.Errorf("resolve group sync settings: %w", err)
+				if xerrors.Is(err, runtimeconfig.EntryNotFound) {
+					// Default to not being configured
+					settings = &GroupSyncSettings{}
+				} else {
+					return xerrors.Errorf("resolve group sync settings: %w", err)
+				}
 			}
 
 			// Legacy deployment settings will override empty settings.

diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go
@@ -27,15 +27,15 @@ type IDPSync interface {
 	OrganizationSyncEnabled() bool
 	// ParseOrganizationClaims takes claims from an OIDC provider, and returns the
 	// organization sync params for assigning users into organizations.
-	ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError)
+	ParseOrganizationClaims(ctx context.Context, mergedClaims jwt.MapClaims) (OrganizationParams, *HTTPError)
 	// SyncOrganizations assigns and removed users from organizations based on the
 	// provided params.
 	SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error
 
 	GroupSyncEnabled() bool
 	// ParseGroupClaims takes claims from an OIDC provider, and returns the params
 	// for group syncing. Most of the logic happens in SyncGroups.
-	ParseGroupClaims(ctx context.Context, _ jwt.MapClaims) (GroupParams, *HTTPError)
+	ParseGroupClaims(ctx context.Context, mergedClaims jwt.MapClaims) (GroupParams, *HTTPError)
 
 	// SyncGroups assigns and removes users from groups based on the provided params.
 	SyncGroups(ctx context.Context, db database.Store, user database.User, params GroupParams) error

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -20,7 +20,6 @@ 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"
 
@@ -659,6 +658,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 		AvatarURL:    ghUser.GetAvatarURL(),
 		Name:         normName,
 		DebugContext: OauthDebugContext{},
+		GroupSync: idpsync.GroupParams{
+			SyncEnabled:    false,
+		},
 		OrganizationSync: idpsync.OrganizationParams{
 			SyncEnabled:    false,
 			IncludeDefault: true,
@@ -1004,11 +1006,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 	}
 
 	ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username), slog.F("name", name))
-	usingGroups, groups, groupErr := api.oidcGroups(ctx, mergedClaims)
-	if groupErr != nil {
-		groupErr.Write(rw, r)
-		return
-	}
 
 	roles, roleErr := api.oidcRoles(ctx, mergedClaims)
 	if roleErr != nil {
@@ -1032,30 +1029,33 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	groupSync, groupSyncErr := api.IDPSync.ParseGroupClaims(ctx, mergedClaims)
+	if groupSyncErr != nil {
+		groupSyncErr.Write(rw, r)
+		return
+	}
+
 	// If a new user is authenticating for the first time
 	// the audit action is 'register', not 'login'
 	if user.ID == uuid.Nil {
 		aReq.Action = database.AuditActionRegister
 	}
 
 	params := (&oauthLoginParams{
-		User:                user,
-		Link:                link,
-		State:               state,
-		LinkedID:            oidcLinkedID(idToken),
-		LoginType:           database.LoginTypeOIDC,
-		AllowSignups:        api.OIDCConfig.AllowSignups,
-		Email:               email,
-		Username:            username,
-		Name:                name,
-		AvatarURL:           picture,
-		UsingRoles:          api.OIDCConfig.RoleSyncEnabled(),
-		Roles:               roles,
-		UsingGroups:         usingGroups,
-		Groups:              groups,
-		OrganizationSync:    orgSync,
-		CreateMissingGroups: api.OIDCConfig.CreateMissingGroups,
-		GroupFilter:         api.OIDCConfig.GroupFilter,
+		User:             user,
+		Link:             link,
+		State:            state,
+		LinkedID:         oidcLinkedID(idToken),
+		LoginType:        database.LoginTypeOIDC,
+		AllowSignups:     api.OIDCConfig.AllowSignups,
+		Email:            email,
+		Username:         username,
+		Name:             name,
+		AvatarURL:        picture,
+		UsingRoles:       api.OIDCConfig.RoleSyncEnabled(),
+		Roles:            roles,
+		OrganizationSync: orgSync,
+		GroupSync:        groupSync,
 		DebugContext: OauthDebugContext{
 			IDTokenClaims:  idtokenClaims,
 			UserInfoClaims: userInfoClaims,
@@ -1091,79 +1091,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 	http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
 }
 
-// oidcGroups returns the groups for the user from the OIDC claims.
-func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interface{}) (bool, []string, *idpsync.HTTPError) {
-	logger := api.Logger.Named(userAuthLoggerName)
-	usingGroups := false
-	var groups []string
-
-	// If the GroupField is the empty string, then groups from OIDC are not used.
-	// This is so we can support manual group assignment.
-	if api.OIDCConfig.GroupField != "" {
-		// If the allow list is empty, then the user is allowed to log in.
-		// Otherwise, they must belong to at least 1 group in the allow list.
-		inAllowList := len(api.OIDCConfig.GroupAllowList) == 0
-
-		usingGroups = true
-		groupsRaw, ok := mergedClaims[api.OIDCConfig.GroupField]
-		if ok {
-			parsedGroups, err := idpsync.ParseStringSliceClaim(groupsRaw)
-			if err != nil {
-				api.Logger.Debug(ctx, "groups field was an unknown type in oidc claims",
-					slog.F("type", fmt.Sprintf("%T", groupsRaw)),
-					slog.Error(err),
-				)
-				return false, nil, &idpsync.HTTPError{
-					Code:             http.StatusBadRequest,
-					Msg:              "Failed to sync groups from OIDC claims",
-					Detail:           err.Error(),
-					RenderStaticPage: false,
-				}
-			}
-
-			api.Logger.Debug(ctx, "groups returned in oidc claims",
-				slog.F("len", len(parsedGroups)),
-				slog.F("groups", parsedGroups),
-			)
-
-			for _, group := range parsedGroups {
-				if mappedGroup, ok := api.OIDCConfig.GroupMapping[group]; ok {
-					group = mappedGroup
-				}
-				if _, ok := api.OIDCConfig.GroupAllowList[group]; ok {
-					inAllowList = true
-				}
-				groups = append(groups, group)
-			}
-		}
-
-		if !inAllowList {
-			logger.Debug(ctx, "oidc group claim not in allow list, rejecting login",
-				slog.F("allow_list_count", len(api.OIDCConfig.GroupAllowList)),
-				slog.F("user_group_count", len(groups)),
-			)
-			detail := "Ask an administrator to add one of your groups to the allow list"
-			if len(groups) == 0 {
-				detail = "You are currently not a member of any groups! Ask an administrator to add you to an authorized group to login."
-			}
-			return usingGroups, groups, &idpsync.HTTPError{
-				Code:             http.StatusForbidden,
-				Msg:              "Not a member of an allowed group",
-				Detail:           detail,
-				RenderStaticPage: true,
-			}
-		}
-	}
-
-	// This conditional is purely to warn the user they might have misconfigured their OIDC
-	// configuration.
-	if _, groupClaimExists := mergedClaims["groups"]; !usingGroups && groupClaimExists {
-		logger.Debug(ctx, "claim 'groups' was returned, but 'oidc-group-field' is not set, check your coder oidc settings")
-	}
-
-	return usingGroups, groups, nil
-}
-
 // oidcRoles returns the roles for the user from the OIDC claims.
 // If the function returns false, then the caller should return early.
 // All writes to the response writer are handled by this function.
@@ -1278,14 +1205,7 @@ type oauthLoginParams struct {
 	AvatarURL    string
 	// OrganizationSync has the organizations that the user will be assigned to.
 	OrganizationSync idpsync.OrganizationParams
-	// Is UsingGroups is true, then the user will be assigned
-	// to the Groups provided.
-	UsingGroups         bool
-	CreateMissingGroups bool
-	// These are the group names from the IDP. Internally, they will map to
-	// some organization groups.
-	Groups      []string
-	GroupFilter *regexp.Regexp
+	GroupSync        idpsync.GroupParams
 	// Is UsingRoles is true, then the user will be assigned
 	// the roles provided.
 	UsingRoles bool
@@ -1491,53 +1411,9 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
 			return xerrors.Errorf("sync organizations: %w", err)
 		}
 
-		// 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 {
-				filtered = make([]string, 0, len(params.Groups))
-				for _, group := range params.Groups {
-					if params.GroupFilter.MatchString(group) {
-						filtered = append(filtered, group)
-					}
-				}
-			}
-
-			//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.OrganizationMembers(dbauthz.AsSystemRestricted(ctx), database.OrganizationMembersParams{
-				UserID:         user.ID,
-				OrganizationID: uuid.Nil,
-			})
-			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.OrganizationMembersRow) bool {
-				return member.OrganizationMember.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, map[uuid.UUID][]string{
-				defaultOrganization.ID: filtered,
-			}, params.CreateMissingGroups)
-			if err != nil {
-				return xerrors.Errorf("set user groups: %w", err)
-			}
+		err = api.IDPSync.SyncGroups(ctx, tx, user, params.GroupSync)
+		if err != nil {
+			return xerrors.Errorf("sync groups: %w", err)
 		}
 
 		// Ensure roles are correct.

diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go
@@ -145,7 +145,6 @@ func New(ctx context.Context, options *Options) (_ *API, err error) {
 		}
 		return c.Subject, c.Trial, nil
 	}
-	api.AGPL.Options.SetUserGroups = api.setUserGroups
 	api.AGPL.Options.SetUserSiteRoles = api.setUserSiteRoles
 	api.AGPL.SiteHandler.RegionsFetcher = func(ctx context.Context) (any, error) {
 		// If the user can read the workspace proxy resource, return that.

diff --git a/enterprise/coderd/userauth.go b/enterprise/coderd/userauth.go
@@ -8,75 +8,9 @@ import (
 
 	"cdr.dev/slog"
 	"github.com/coder/coder/v2/coderd/database"
-	"github.com/coder/coder/v2/coderd/database/dbauthz"
 	"github.com/coder/coder/v2/codersdk"
 )
 
-// nolint: revive
-func (api *API) setUserGroups(ctx context.Context, logger slog.Logger, db database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error {
-	if !api.Entitlements.Enabled(codersdk.FeatureTemplateRBAC) {
-		return nil
-	}
-
-	return db.InTx(func(tx database.Store) error {
-		// When setting the user's groups, it's easier to just clear their groups and re-add them.
-		// This ensures that the user's groups are always in sync with the auth provider.
-		orgs, err := tx.GetOrganizationsByUserID(ctx, userID)
-		if err != nil {
-			return xerrors.Errorf("get user orgs: %w", err)
-		}
-		if len(orgs) != 1 {
-			return xerrors.Errorf("expected 1 org, got %d", len(orgs))
-		}
-
-		// Delete all groups the user belongs to.
-		// nolint:gocritic // Requires system context to remove user from all groups.
-		err = tx.RemoveUserFromAllGroups(dbauthz.AsSystemRestricted(ctx), userID)
-		if err != nil {
-			return xerrors.Errorf("delete user groups: %w", err)
-		}
-
-		// 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.
-		for orgID, groupNames := range orgGroupNames {
-			// Create the missing groups for each organization.
-			if createMissingGroups {
-				// This is the system creating these additional groups, so we use the system restricted context.
-				// nolint:gocritic
-				created, err := tx.InsertMissingGroups(dbauthz.AsSystemRestricted(ctx), database.InsertMissingGroupsParams{
-					OrganizationID: orgID,
-					GroupNames:     groupNames,
-					Source:         database.GroupSourceOidc,
-				})
-				if err != nil {
-					return xerrors.Errorf("insert missing groups: %w", err)
-				}
-				if len(created) > 0 {
-					logger.Debug(ctx, "auto created missing groups",
-						slog.F("org_id", orgID.ID),
-						slog.F("created", created),
-						slog.F("num", len(created)),
-					)
-				}
-			}
-
-			// Re-add the user to all groups returned by the auth provider.
-			err = tx.InsertUserGroupsByName(ctx, database.InsertUserGroupsByNameParams{
-				UserID:         userID,
-				OrganizationID: orgID,
-				GroupNames:     groupNames,
-			})
-			if err != nil {
-				return xerrors.Errorf("insert user groups: %w", err)
-			}
-		}
-
-		return nil
-	}, nil)
-}
-
 func (api *API) setUserSiteRoles(ctx context.Context, logger slog.Logger, db database.Store, userID uuid.UUID, roles []string) error {
 	if !api.Entitlements.Enabled(codersdk.FeatureUserRoleManagement) {
 		logger.Warn(ctx, "attempted to assign OIDC user roles without enterprise entitlement, roles left unchanged",