chore: remove old role sync, insert new idpsync package

coder · Emyrk · Sep 17, 2024 · Sep 10, 2024 · Sep 10, 2024 · Sep 10, 2024
commit 2a1e2d06201d6bfb184c72cbf38e90b1bd838381
diff --git a/cli/server.go b/cli/server.go
@@ -187,9 +187,6 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
 		EmailField:          vals.OIDC.EmailField.String(),
 		AuthURLParams:       vals.OIDC.AuthURLParams.Value,
 		IgnoreUserInfo:      vals.OIDC.IgnoreUserInfo.Value(),
-		UserRoleField:       vals.OIDC.UserRoleField.String(),
-		UserRoleMapping:     vals.OIDC.UserRoleMapping.Value,
-		UserRolesDefault:    vals.OIDC.UserRolesDefault.GetSlice(),
 		SignInText:          vals.OIDC.SignInText.String(),
 		SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(),
 		IconURL:             vals.OIDC.IconURL.String(),

diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -181,7 +181,6 @@ type Options struct {
 	NetworkTelemetryBatchFrequency time.Duration
 	NetworkTelemetryBatchMaxSize   int
 	SwaggerEndpoint                bool
-	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]
 	AccessControlStore             *atomic.Pointer[dbauthz.AccessControlStore]
@@ -373,14 +372,6 @@ func New(options *Options) *API {
 	if options.TracerProvider == nil {
 		options.TracerProvider = trace.NewNoopTracerProvider()
 	}
-	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",
-				slog.F("user_id", userID), slog.F("roles", roles),
-			)
-			return nil
-		}
-	}
 	if options.TemplateScheduleStore == nil {
 		options.TemplateScheduleStore = &atomic.Pointer[schedule.TemplateScheduleStore]{}
 	}

diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go
@@ -43,6 +43,24 @@ type IDPSync interface {
 	// accessed concurrently. The settings are stored in the database.
 	GroupSyncSettings(ctx context.Context, orgID uuid.UUID, db database.Store) (*GroupSyncSettings, error)
 	UpdateGroupSettings(ctx context.Context, orgID uuid.UUID, db database.Store, settings GroupSyncSettings) error
+
+	// RoleSyncEntitled returns true if the deployment is entitled to role syncing.
+	RoleSyncEntitled() bool
+	// OrganizationRoleSyncEnabled returns true if the organization has role sync
+	// enabled.
+	OrganizationRoleSyncEnabled(ctx context.Context, db database.Store, org uuid.UUID) (bool, error)
+	// SiteRoleSyncEnabled returns true if the deployment has role sync enabled
+	// at the site level.
+	SiteRoleSyncEnabled() bool
+	// RoleSyncSettings is similar to GroupSyncSettings. See GroupSyncSettings for
+	// rational.
+	RoleSyncSettings() runtimeconfig.RuntimeEntry[*RoleSyncSettings]
+	// ParseRoleClaims takes claims from an OIDC provider, and returns the params
+	// for role syncing. Most of the logic happens in SyncRoles.
+	ParseRoleClaims(ctx context.Context, mergedClaims jwt.MapClaims) (RoleParams, *HTTPError)
+	// SyncRoles assigns and removes users from roles based on the provided params.
+	// Site & org roles are handled in this method.
+	SyncRoles(ctx context.Context, db database.Store, user database.User, params RoleParams) error
 }
 
 // AGPLIDPSync is the configuration for syncing user information from an external
@@ -77,9 +95,16 @@ type DeploymentSyncSettings struct {
 	// Legacy deployment settings that only apply to the default org.
 	Legacy DefaultOrgLegacySettings
 
-	// SiteRoleField syncs a user's site wide roles from an IDP.
-	SiteRoleField    string
-	SiteRoleMapping  map[string][]string
+	// SiteRoleField selects the claim field to be used as the created user's
+	// roles. If the field is the empty string, then no site role updates
+	// will ever come from the OIDC provider.
+	SiteRoleField string
+	// SiteRoleMapping controls how groups returned by the OIDC provider get mapped
+	// to site roles within Coder.
+	// map[oidcRoleName][]coderRoleName
+	SiteRoleMapping map[string][]string
+	// SiteDefaultRoles is the default set of site roles to assign to a user if role sync
+	// is enabled.
 	SiteDefaultRoles []string
 }
 
@@ -99,6 +124,10 @@ func FromDeploymentValues(dv *codersdk.DeploymentValues) DeploymentSyncSettings
 		OrganizationMapping:       dv.OIDC.OrganizationMapping.Value,
 		OrganizationAssignDefault: dv.OIDC.OrganizationAssignDefault.Value(),
 
+		SiteRoleField:    dv.OIDC.UserRoleField.Value(),
+		SiteRoleMapping:  dv.OIDC.UserRoleMapping.Value,
+		SiteDefaultRoles: dv.OIDC.UserRolesDefault.Value(),
+
 		// TODO: Separate group field for allow list from default org.
 		// Right now you cannot disable group sync from the default org and
 		// configure an allow list.

diff --git a/coderd/idpsync/role.go b/coderd/idpsync/role.go
@@ -19,32 +19,42 @@ import (
 )
 
 type RoleParams struct {
-	// SyncEnabled if false will skip syncing the user's roles
-	SyncEnabled   bool
+	// SyncEntitled if false will skip syncing the user's roles at
+	// all levels.
+	SyncEntitled  bool
 	SyncSiteWide  bool
 	SiteWideRoles []string
 	// MergedClaims are passed to the organization level for syncing
 	MergedClaims jwt.MapClaims
 }
 
-func (AGPLIDPSync) RoleSyncEnabled() bool {
+func (AGPLIDPSync) RoleSyncEntitled() bool {
 	// AGPL does not support syncing groups.
 	return false
 }
+
+func (AGPLIDPSync) OrganizationRoleSyncEnabled(_ context.Context, _ database.Store, _ uuid.UUID) (bool, error) {
+	return false, nil
+}
+
+func (AGPLIDPSync) SiteRoleSyncEnabled() bool {
+	return false
+}
+
 func (s AGPLIDPSync) RoleSyncSettings() runtimeconfig.RuntimeEntry[*RoleSyncSettings] {
 	return s.Role
 }
 
 func (s AGPLIDPSync) ParseRoleClaims(_ context.Context, _ jwt.MapClaims) (RoleParams, *HTTPError) {
 	return RoleParams{
-		SyncEnabled:  s.RoleSyncEnabled(),
-		SyncSiteWide: false,
+		SyncEntitled: s.RoleSyncEntitled(),
+		SyncSiteWide: s.SiteRoleSyncEnabled(),
 	}, nil
 }
 
 func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user database.User, params RoleParams) error {
 	// Nothing happens if sync is not enabled
-	if !params.SyncEnabled {
+	if !params.SyncEntitled {
 		return nil
 	}
 

diff --git a/coderd/idpsync/role_test.go b/coderd/idpsync/role_test.go
@@ -212,7 +212,7 @@ func TestRoleSyncTable(t *testing.T) {
 
 			// Do the role sync!
 			err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{
-				SyncEnabled:  true,
+				SyncEntitled: true,
 				SyncSiteWide: false,
 				MergedClaims: userClaims,
 			})
@@ -264,7 +264,7 @@ func TestRoleSyncTable(t *testing.T) {
 		}
 
 		err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{
-			SyncEnabled:  true,
+			SyncEntitled: true,
 			SyncSiteWide: true,
 			SiteWideRoles: []string{
 				rbac.RoleTemplateAdmin().Name, // Duplicate this value to test deduplication
@@ -353,7 +353,7 @@ func TestNoopNoDiff(t *testing.T) {
 		RBACRoles: siteRoles,
 		LoginType: database.LoginTypePassword,
 	}, idpsync.RoleParams{
-		SyncEnabled:   true,
+		SyncEntitled:  true,
 		SyncSiteWide:  true,
 		SiteWideRoles: siteRoles,
 		MergedClaims: jwt.MapClaims{

diff --git a/coderd/members.go b/coderd/members.go
@@ -11,6 +11,7 @@ import (
 	"github.com/coder/coder/v2/coderd/audit"
 	"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/database/dbtime"
 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/coderd/httpmw"
@@ -215,6 +216,33 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
 	aReq.Old = member.OrganizationMember.Auditable(member.Username)
 	defer commitAudit()
 
+	// Keep this block scoping to prevent accidental use of the user variable.
+	{
+		// nolint:gocritic // The caller could be an org admin without this perm.
+		// We need to disable manual role assignment if role sync is enabled for
+		// the given organization.
+		user, err := api.Database.GetUserByID(dbauthz.AsSystemRestricted(ctx), member.UserID)
+		if err != nil {
+			httpapi.InternalServerError(rw, err)
+			return
+		}
+		if user.LoginType == database.LoginTypeOIDC {
+			// nolint:gocritic // fetching settings
+			orgSync, err := api.IDPSync.OrganizationRoleSyncEnabled(dbauthz.AsSystemRestricted(ctx), api.Database, organization.ID)
+			if err != nil {
+				httpapi.InternalServerError(rw, err)
+				return
+			}
+			if orgSync {
+				httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+					Message: "Cannot modify roles for OIDC users when role sync is enabled. This organization member's roles are managed by the identity provider.",
+					Detail:  "'User Role Field' is set in the organization settings. Ask an administrator to adjust or disable these settings.",
+				})
+				return
+			}
+		}
+	}
+
 	if apiKey.UserID == member.OrganizationMember.UserID {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "You cannot change your own organization roles.",

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -740,17 +740,6 @@ type OIDCConfig struct {
 	// support the userinfo endpoint, or if the userinfo endpoint causes
 	// undesirable behavior.
 	IgnoreUserInfo bool
-	// UserRoleField selects the claim field to be used as the created user's
-	// roles. If the field is the empty string, then no role updates
-	// will ever come from the OIDC provider.
-	UserRoleField string
-	// UserRoleMapping controls how groups returned by the OIDC provider get mapped
-	// to roles within Coder.
-	// map[oidcRoleName][]coderRoleName
-	UserRoleMapping map[string][]string
-	// UserRolesDefault is the default set of roles to assign to a user if role sync
-	// is enabled.
-	UserRolesDefault []string
 	// SignInText is the text to display on the OIDC login button
 	SignInText string
 	// IconURL points to the URL of an icon to display on the OIDC login button
@@ -759,10 +748,6 @@ type OIDCConfig struct {
 	SignupsDisabledText string
 }
 
-func (cfg OIDCConfig) RoleSyncEnabled() bool {
-	return cfg.UserRoleField != ""
-}
-
 // @Summary OpenID Connect Callback
 // @ID openid-connect-callback
 // @Security CoderSessionToken
@@ -983,12 +968,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))
 
-	roles, roleErr := api.oidcRoles(ctx, mergedClaims)
-	if roleErr != nil {
-		roleErr.Write(rw, r)
-		return
-	}
-
 	user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), email)
 	if err != nil {
 		logger.Error(ctx, "oauth2: unable to find linked user", slog.F("email", email), slog.Error(err))
@@ -1011,6 +990,12 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	roleSync, roleSyncErr := api.IDPSync.ParseRoleClaims(ctx, mergedClaims)
+	if roleSyncErr != nil {
+		roleSyncErr.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 {
@@ -1028,10 +1013,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		Username:         username,
 		Name:             name,
 		AvatarURL:        picture,
-		UsingRoles:       api.OIDCConfig.RoleSyncEnabled(),
-		Roles:            roles,
 		OrganizationSync: orgSync,
 		GroupSync:        groupSync,
+		RoleSync:         roleSync,
 		DebugContext: OauthDebugContext{
 			IDTokenClaims:  idtokenClaims,
 			UserInfoClaims: userInfoClaims,
@@ -1067,61 +1051,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 	http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
 }
 
-// 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.
-// It would be preferred to just return an error, however this function
-// decorates returned errors with the appropriate HTTP status codes and details
-// that are hard to carry in a standard `error` without more work.
-func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface{}) ([]string, *idpsync.HTTPError) {
-	roles := api.OIDCConfig.UserRolesDefault
-	if !api.OIDCConfig.RoleSyncEnabled() {
-		return roles, nil
-	}
-
-	rolesRow, ok := mergedClaims[api.OIDCConfig.UserRoleField]
-	if !ok {
-		// If no claim is provided than we can assume the user is just
-		// a member. This is because there is no way to tell the difference
-		// between []string{} and nil for OIDC claims. IDPs omit claims
-		// if they are empty ([]string{}).
-		// Use []interface{}{} so the next typecast works.
-		rolesRow = []interface{}{}
-	}
-
-	parsedRoles, err := idpsync.ParseStringSliceClaim(rolesRow)
-	if err != nil {
-		api.Logger.Error(ctx, "oidc claims user roles field was an unknown type",
-			slog.F("type", fmt.Sprintf("%T", rolesRow)),
-			slog.Error(err),
-		)
-		return nil, &idpsync.HTTPError{
-			Code:             http.StatusInternalServerError,
-			Msg:              "Login disabled until OIDC config is fixed",
-			Detail:           fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow),
-			RenderStaticPage: false,
-		}
-	}
-
-	api.Logger.Debug(ctx, "roles returned in oidc claims",
-		slog.F("len", len(parsedRoles)),
-		slog.F("roles", parsedRoles),
-	)
-	for _, role := range parsedRoles {
-		if mappedRoles, ok := api.OIDCConfig.UserRoleMapping[role]; ok {
-			if len(mappedRoles) == 0 {
-				continue
-			}
-			// Mapped roles are added to the list of roles
-			roles = append(roles, mappedRoles...)
-			continue
-		}
-
-		roles = append(roles, role)
-	}
-	return roles, nil
-}
-
 // claimFields returns the sorted list of fields in the claims map.
 func claimFields(claims map[string]interface{}) []string {
 	fields := []string{}
@@ -1182,10 +1111,7 @@ type oauthLoginParams struct {
 	// OrganizationSync has the organizations that the user will be assigned to.
 	OrganizationSync idpsync.OrganizationParams
 	GroupSync        idpsync.GroupParams
-	// Is UsingRoles is true, then the user will be assigned
-	// the roles provided.
-	UsingRoles bool
-	Roles      []string
+	RoleSync         idpsync.RoleParams
 
 	DebugContext OauthDebugContext
 
@@ -1394,37 +1320,10 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
 			return xerrors.Errorf("sync groups: %w", err)
 		}
 
-		// Ensure roles are correct.
-		if params.UsingRoles {
-			ignored := make([]string, 0)
-			filtered := make([]string, 0, len(params.Roles))
-			for _, role := range params.Roles {
-				// TODO: This only supports mapping deployment wide roles. Organization scoped roles
-				// are unsupported.
-				if _, err := rbac.RoleByName(rbac.RoleIdentifier{Name: role}); err == nil {
-					filtered = append(filtered, role)
-				} else {
-					ignored = append(ignored, role)
-				}
-			}
-
-			//nolint:gocritic
-			err := api.Options.SetUserSiteRoles(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, filtered)
-			if err != nil {
-				return &idpsync.HTTPError{
-					Code:             http.StatusBadRequest,
-					Msg:              "Invalid roles through OIDC claims",
-					Detail:           fmt.Sprintf("Error from role assignment attempt: %s", err.Error()),
-					RenderStaticPage: true,
-				}
-			}
-			if len(ignored) > 0 {
-				logger.Debug(ctx, "OIDC roles ignored in assignment",
-					slog.F("ignored", ignored),
-					slog.F("assigned", filtered),
-					slog.F("user_id", user.ID),
-				)
-			}
+		// Role sync needs to occur after org sync.
+		err = api.IDPSync.SyncRoles(ctx, tx, user, params.RoleSync)
+		if err != nil {
+			return xerrors.Errorf("sync roles: %w", err)
 		}
 
 		needsUpdate := false

diff --git a/coderd/users.go b/coderd/users.go
@@ -1162,7 +1162,7 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) {
 	defer commitAudit()
 	aReq.Old = user
 
-	if user.LoginType == database.LoginTypeOIDC && api.OIDCConfig.RoleSyncEnabled() {
+	if user.LoginType == database.LoginTypeOIDC && api.IDPSync.SiteRoleSyncEnabled() {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "Cannot modify roles for OIDC users when role sync is enabled.",
 			Detail:  "'User Role Field' is set in the OIDC configuration. All role changes must come from the oidc identity provider.",