diff --git a/cli/server.go b/cli/server.go index c2cd476edfaa4..8336fba770ebc 100644 --- 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 index e04f13d367c6e..fafc0962dce2e 100644 --- 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/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index ed766d48ecd43..d5aa6211a8846 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -8630,7 +8630,7 @@ func (q *FakeQuerier) UpdateUserRoles(_ context.Context, arg database.UpdateUser } // Set new roles - user.RBACRoles = arg.GrantedRoles + user.RBACRoles = slice.Unique(arg.GrantedRoles) // Remove duplicates and sort uniqueRoles := make([]string, 0, len(user.RBACRoles)) exist := make(map[string]struct{}) diff --git a/coderd/idpsync/group_test.go b/coderd/idpsync/group_test.go index 0d64d8abf6059..1275dd4e48503 100644 --- a/coderd/idpsync/group_test.go +++ b/coderd/idpsync/group_test.go @@ -85,7 +85,7 @@ func TestGroupSyncTable(t *testing.T) { testCases := []orgSetupDefinition{ { Name: "SwitchGroups", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", Mapping: map[string][]uuid.UUID{ "foo": {ids.ID("sg-foo"), ids.ID("sg-foo-2")}, @@ -102,16 +102,18 @@ func TestGroupSyncTable(t *testing.T) { ids.ID("sg-bar"): false, ids.ID("sg-baz"): false, }, - ExpectedGroups: []uuid.UUID{ - ids.ID("sg-foo"), - ids.ID("sg-foo-2"), - ids.ID("sg-bar"), - ids.ID("sg-baz"), + assertGroups: &orgGroupAssert{ + ExpectedGroups: []uuid.UUID{ + ids.ID("sg-foo"), + ids.ID("sg-foo-2"), + ids.ID("sg-bar"), + ids.ID("sg-baz"), + }, }, }, { Name: "StayInGroup", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", // Only match foo, so bar does not map RegexFilter: regexp.MustCompile("^foo$"), @@ -125,13 +127,15 @@ func TestGroupSyncTable(t *testing.T) { ids.ID("gg-foo"): true, ids.ID("gg-bar"): false, }, - ExpectedGroups: []uuid.UUID{ - ids.ID("gg-foo"), + assertGroups: &orgGroupAssert{ + ExpectedGroups: []uuid.UUID{ + ids.ID("gg-foo"), + }, }, }, { Name: "UserJoinsGroups", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", Mapping: map[string][]uuid.UUID{ "foo": {ids.ID("ng-foo"), uuid.New()}, @@ -145,29 +149,33 @@ func TestGroupSyncTable(t *testing.T) { ids.ID("ng-bar-2"): false, ids.ID("ng-baz"): false, }, - ExpectedGroups: []uuid.UUID{ - ids.ID("ng-foo"), - ids.ID("ng-bar"), - ids.ID("ng-bar-2"), - ids.ID("ng-baz"), + assertGroups: &orgGroupAssert{ + ExpectedGroups: []uuid.UUID{ + ids.ID("ng-foo"), + ids.ID("ng-bar"), + ids.ID("ng-bar-2"), + ids.ID("ng-baz"), + }, }, }, { Name: "CreateGroups", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", RegexFilter: regexp.MustCompile("^create"), AutoCreateMissing: true, }, Groups: map[uuid.UUID]bool{}, - ExpectedGroupNames: []string{ - "create-bar", - "create-baz", + assertGroups: &orgGroupAssert{ + ExpectedGroupNames: []string{ + "create-bar", + "create-baz", + }, }, }, { Name: "GroupNamesNoMapping", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", RegexFilter: regexp.MustCompile(".*"), AutoCreateMissing: false, @@ -177,14 +185,16 @@ func TestGroupSyncTable(t *testing.T) { "bar": false, "goob": true, }, - ExpectedGroupNames: []string{ - "foo", - "bar", + assertGroups: &orgGroupAssert{ + ExpectedGroupNames: []string{ + "foo", + "bar", + }, }, }, { Name: "NoUser", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", Mapping: map[string][]uuid.UUID{ // Extra ID that does not map to a group @@ -200,13 +210,16 @@ func TestGroupSyncTable(t *testing.T) { }, }, { - Name: "NoSettingsNoUser", - Settings: nil, - Groups: map[uuid.UUID]bool{}, + Name: "NoSettings", + GroupSettings: nil, + Groups: map[uuid.UUID]bool{}, + assertGroups: &orgGroupAssert{ + ExpectedGroups: []uuid.UUID{}, + }, }, { Name: "LegacyMapping", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", RegexFilter: regexp.MustCompile("^legacy"), LegacyNameMapping: map[string]string{ @@ -224,9 +237,11 @@ func TestGroupSyncTable(t *testing.T) { "extra": true, "legacy-bop": true, }, - ExpectedGroupNames: []string{ - "legacy-bar", - "legacy-foo", + assertGroups: &orgGroupAssert{ + ExpectedGroupNames: []string{ + "legacy-bar", + "legacy-foo", + }, }, }, } @@ -311,9 +326,10 @@ func TestGroupSyncTable(t *testing.T) { "random": true, }, // No settings, because they come from the deployment values - Settings: nil, - ExpectedGroups: nil, - ExpectedGroupNames: []string{"legacy-foo", "legacy-baz", "legacy-bar"}, + GroupSettings: nil, + assertGroups: &orgGroupAssert{ + ExpectedGroupNames: []string{"legacy-foo", "legacy-baz", "legacy-bar"}, + }, } //nolint:gocritic // testing @@ -385,16 +401,18 @@ func TestSyncDisabled(t *testing.T) { ids.ID("baz"): false, ids.ID("bop"): false, }, - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", Mapping: map[string][]uuid.UUID{ "foo": {ids.ID("foo")}, "baz": {ids.ID("baz")}, }, }, - ExpectedGroups: []uuid.UUID{ - ids.ID("foo"), - ids.ID("bar"), + assertGroups: &orgGroupAssert{ + ExpectedGroups: []uuid.UUID{ + ids.ID("foo"), + ids.ID("bar"), + }, }, } @@ -728,9 +746,14 @@ func SetupOrganization(t *testing.T, s *idpsync.AGPLIDPSync, db database.Store, } manager := runtimeconfig.NewManager() - if def.Settings != nil { - orgResolver := manager.OrganizationResolver(db, org.ID) - err = s.Group.SetRuntimeValue(context.Background(), orgResolver, (*idpsync.GroupSyncSettings)(def.Settings)) + orgResolver := manager.OrganizationResolver(db, org.ID) + if def.GroupSettings != nil { + err = s.Group.SetRuntimeValue(context.Background(), orgResolver, (*idpsync.GroupSyncSettings)(def.GroupSettings)) + require.NoError(t, err) + } + + if def.RoleSettings != nil { + err = s.Role.SetRuntimeValue(context.Background(), orgResolver, def.RoleSettings) require.NoError(t, err) } @@ -740,6 +763,33 @@ func SetupOrganization(t *testing.T, s *idpsync.AGPLIDPSync, db database.Store, OrganizationID: org.ID, }) } + + if len(def.OrganizationRoles) > 0 { + _, err := db.UpdateMemberRoles(context.Background(), database.UpdateMemberRolesParams{ + GrantedRoles: def.OrganizationRoles, + UserID: user.ID, + OrgID: org.ID, + }) + require.NoError(t, err) + } + + if len(def.CustomRoles) > 0 { + for _, cr := range def.CustomRoles { + _, err := db.InsertCustomRole(context.Background(), database.InsertCustomRoleParams{ + Name: cr, + DisplayName: cr, + OrganizationID: uuid.NullUUID{ + UUID: org.ID, + Valid: true, + }, + SitePermissions: nil, + OrgPermissions: nil, + UserPermissions: nil, + }) + require.NoError(t, err) + } + } + for groupID, in := range def.Groups { dbgen.Group(t, db, database.Group{ ID: groupID, @@ -769,11 +819,25 @@ func SetupOrganization(t *testing.T, s *idpsync.AGPLIDPSync, db database.Store, type orgSetupDefinition struct { Name string // True if the user is a member of the group - Groups map[uuid.UUID]bool - GroupNames map[string]bool - NotMember bool + Groups map[uuid.UUID]bool + GroupNames map[string]bool + OrganizationRoles []string + CustomRoles []string + // NotMember if true will ensure the user is not a member of the organization. + NotMember bool + + GroupSettings *codersdk.GroupSyncSettings + RoleSettings *idpsync.RoleSyncSettings + + assertGroups *orgGroupAssert + assertRoles *orgRoleAssert +} + +type orgRoleAssert struct { + ExpectedOrgRoles []string +} - Settings *codersdk.GroupSyncSettings +type orgGroupAssert struct { ExpectedGroups []uuid.UUID ExpectedGroupNames []string } @@ -794,6 +858,25 @@ func (o orgSetupDefinition) Assert(t *testing.T, orgID uuid.UUID, db database.St require.Len(t, members, 1, "should be a member") } + if o.assertGroups != nil { + o.assertGroups.Assert(t, orgID, db, user) + } + if o.assertRoles != nil { + o.assertRoles.Assert(t, orgID, db, o.NotMember, user) + } + + // If the user is not a member, there is nothing to really assert in the org + if o.assertGroups == nil && o.assertRoles == nil && !o.NotMember { + t.Errorf("no group or role asserts present, must have at least one") + t.FailNow() + } +} + +func (o orgGroupAssert) Assert(t *testing.T, orgID uuid.UUID, db database.Store, user database.User) { + t.Helper() + + ctx := context.Background() + userGroups, err := db.GetGroups(ctx, database.GetGroupsParams{ OrganizationID: orgID, HasMemberID: user.ID, @@ -826,3 +909,23 @@ func (o orgSetupDefinition) Assert(t *testing.T, orgID uuid.UUID, db database.St require.Len(t, o.ExpectedGroupNames, 0, "ExpectedGroupNames should be empty") } } + +//nolint:revive +func (o orgRoleAssert) Assert(t *testing.T, orgID uuid.UUID, db database.Store, notMember bool, user database.User) { + t.Helper() + + ctx := context.Background() + + members, err := db.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: orgID, + UserID: user.ID, + }) + if notMember { + require.ErrorIs(t, err, sql.ErrNoRows) + return + } + require.NoError(t, err) + require.Len(t, members, 1) + member := members[0] + require.ElementsMatch(t, member.OrganizationMember.Roles, o.ExpectedOrgRoles) +} diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 1e8b14956b652..41c0f7d3e8605 100644 --- 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 @@ -76,6 +94,18 @@ type DeploymentSyncSettings struct { GroupAllowList map[string]struct{} // Legacy deployment settings that only apply to the default org. Legacy DefaultOrgLegacySettings + + // 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 } type DefaultOrgLegacySettings struct { @@ -94,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. @@ -112,6 +146,7 @@ type SyncSettings struct { DeploymentSyncSettings Group runtimeconfig.RuntimeEntry[*GroupSyncSettings] + Role runtimeconfig.RuntimeEntry[*RoleSyncSettings] } func NewAGPLSync(logger slog.Logger, manager *runtimeconfig.Manager, settings DeploymentSyncSettings) *AGPLIDPSync { @@ -121,6 +156,7 @@ func NewAGPLSync(logger slog.Logger, manager *runtimeconfig.Manager, settings De SyncSettings: SyncSettings{ DeploymentSyncSettings: settings, Group: runtimeconfig.MustNew[*GroupSyncSettings]("group-sync-settings"), + Role: runtimeconfig.MustNew[*RoleSyncSettings]("role-sync-settings"), }, } } diff --git a/coderd/idpsync/role.go b/coderd/idpsync/role.go new file mode 100644 index 0000000000000..9c63df157a764 --- /dev/null +++ b/coderd/idpsync/role.go @@ -0,0 +1,279 @@ +package idpsync + +import ( + "context" + "encoding/json" + + "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" + "golang.org/x/exp/slices" + "golang.org/x/xerrors" + + "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/rbac/rolestore" + "github.com/coder/coder/v2/coderd/runtimeconfig" + "github.com/coder/coder/v2/coderd/util/slice" +) + +type RoleParams struct { + // 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) 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{ + 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.SyncEntitled { + return nil + } + + // nolint:gocritic // all syncing is done as a system user + ctx = dbauthz.AsSystemRestricted(ctx) + + err := db.InTx(func(tx database.Store) error { + if params.SyncSiteWide { + if err := s.syncSiteWideRoles(ctx, tx, user, params); err != nil { + return err + } + } + + // sync roles per organization + orgMemberships, err := tx.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: uuid.Nil, + UserID: user.ID, + }) + if err != nil { + return xerrors.Errorf("get organizations by user id: %w", err) + } + + // Sync for each organization + // If a key for a given org exists in the map, the user's roles will be + // updated to the value of that key. + expectedRoles := make(map[uuid.UUID][]rbac.RoleIdentifier) + existingRoles := make(map[uuid.UUID][]string) + allExpected := make([]rbac.RoleIdentifier, 0) + for _, member := range orgMemberships { + orgID := member.OrganizationMember.OrganizationID + orgResolver := s.Manager.OrganizationResolver(tx, orgID) + settings, err := s.RoleSyncSettings().Resolve(ctx, orgResolver) + if err != nil { + if !xerrors.Is(err, runtimeconfig.ErrEntryNotFound) { + return xerrors.Errorf("resolve group sync settings: %w", err) + } + // No entry means no role syncing for this organization + continue + } + if settings.Field == "" { + // Explicitly disabled role sync for this organization + continue + } + + existingRoles[orgID] = member.OrganizationMember.Roles + orgRoleClaims, err := s.RolesFromClaim(settings.Field, params.MergedClaims) + if err != nil { + s.Logger.Error(ctx, "failed to parse roles from claim", + slog.F("field", settings.Field), + slog.F("organization_id", orgID), + slog.F("user_id", user.ID), + slog.F("username", user.Username), + slog.Error(err), + ) + + // TODO: If rolesync fails, we might want to reset a user's + // roles to prevent stale roles from existing. + // Eg: `expectedRoles[orgID] = []rbac.RoleIdentifier{}` + // However, implementing this could lock an org admin out + // of fixing their configuration. + // There is also no current method to notify an org admin of + // a configuration issue. + // So until org admins can be notified of configuration issues, + // and they will not be locked out, this code will do nothing to + // the user's roles. + + // Do not return an error, because that would prevent a user + // from logging in. A misconfigured organization should not + // stop a user from logging into the site. + continue + } + + expected := make([]rbac.RoleIdentifier, 0, len(orgRoleClaims)) + for _, role := range orgRoleClaims { + if mappedRoles, ok := settings.Mapping[role]; ok { + for _, mappedRole := range mappedRoles { + expected = append(expected, rbac.RoleIdentifier{OrganizationID: orgID, Name: mappedRole}) + } + continue + } + expected = append(expected, rbac.RoleIdentifier{OrganizationID: orgID, Name: role}) + } + + expectedRoles[orgID] = expected + allExpected = append(allExpected, expected...) + } + + // Now mass sync the user's org membership roles. + validRoles, err := rolestore.Expand(ctx, tx, allExpected) + if err != nil { + return xerrors.Errorf("expand roles: %w", err) + } + validMap := make(map[string]struct{}, len(validRoles)) + for _, validRole := range validRoles { + validMap[validRole.Identifier.UniqueName()] = struct{}{} + } + + // For each org, do the SQL query to update the user's roles. + // TODO: Would be better to batch all these into a single SQL query. + for orgID, roles := range expectedRoles { + validExpected := make([]string, 0, len(roles)) + for _, role := range roles { + if _, ok := validMap[role.UniqueName()]; ok { + validExpected = append(validExpected, role.Name) + } + } + // Ignore the implied member role + validExpected = slices.DeleteFunc(validExpected, func(s string) bool { + return s == rbac.RoleOrgMember() + }) + + existingFound := existingRoles[orgID] + existingFound = slices.DeleteFunc(existingFound, func(s string) bool { + return s == rbac.RoleOrgMember() + }) + + // Only care about unique roles. So remove all duplicates + existingFound = slice.Unique(existingFound) + validExpected = slice.Unique(validExpected) + // A sort is required for the equality check + slices.Sort(existingFound) + slices.Sort(validExpected) + // Is there a difference between the expected roles and the existing roles? + if !slices.Equal(existingFound, validExpected) { + // TODO: Write a unit test to verify we do no db call on no diff + _, err = tx.UpdateMemberRoles(ctx, database.UpdateMemberRolesParams{ + GrantedRoles: validExpected, + UserID: user.ID, + OrgID: orgID, + }) + if err != nil { + return xerrors.Errorf("update member roles(%s): %w", user.ID.String(), err) + } + } + } + return nil + }, nil) + if err != nil { + return xerrors.Errorf("sync user roles(%s): %w", user.ID.String(), err) + } + + return nil +} + +func (s AGPLIDPSync) syncSiteWideRoles(ctx context.Context, tx database.Store, user database.User, params RoleParams) error { + // Apply site wide roles to a user. + // ignored is the list of roles that are not valid Coder roles and will + // be skipped. + ignored := make([]string, 0) + filtered := make([]string, 0, len(params.SiteWideRoles)) + for _, role := range params.SiteWideRoles { + // Because we are only syncing site wide roles, we intentionally will always + // omit 'OrganizationID' from the RoleIdentifier. + // TODO: If custom site wide roles are introduced, this needs to use the + // database to verify the role exists. + if _, err := rbac.RoleByName(rbac.RoleIdentifier{Name: role}); err == nil { + filtered = append(filtered, role) + } else { + ignored = append(ignored, role) + } + } + if len(ignored) > 0 { + s.Logger.Debug(ctx, "OIDC roles ignored in assignment", + slog.F("ignored", ignored), + slog.F("assigned", filtered), + slog.F("user_id", user.ID), + slog.F("username", user.Username), + ) + } + + filtered = slice.Unique(filtered) + slices.Sort(filtered) + + existing := slice.Unique(user.RBACRoles) + slices.Sort(existing) + if !slices.Equal(existing, filtered) { + _, err := tx.UpdateUserRoles(ctx, database.UpdateUserRolesParams{ + GrantedRoles: filtered, + ID: user.ID, + }) + if err != nil { + return xerrors.Errorf("set site wide roles: %w", err) + } + } + return nil +} + +func (AGPLIDPSync) RolesFromClaim(field string, claims jwt.MapClaims) ([]string, error) { + rolesRow, ok := claims[field] + 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 := ParseStringSliceClaim(rolesRow) + if err != nil { + return nil, xerrors.Errorf("failed to parse roles from claim: %w", err) + } + + return parsedRoles, nil +} + +type RoleSyncSettings struct { + // Field selects the claim field to be used as the created user's + // groups. If the group field is the empty string, then no group updates + // will ever come from the OIDC provider. + Field string `json:"field"` + // Mapping maps from an OIDC group --> Coder organization role + Mapping map[string][]string `json:"mapping"` +} + +func (s *RoleSyncSettings) Set(v string) error { + return json.Unmarshal([]byte(v), s) +} + +func (s *RoleSyncSettings) String() string { + return runtimeconfig.JSONString(s) +} diff --git a/coderd/idpsync/role_test.go b/coderd/idpsync/role_test.go new file mode 100644 index 0000000000000..c6ab989881976 --- /dev/null +++ b/coderd/idpsync/role_test.go @@ -0,0 +1,373 @@ +package idpsync_test + +import ( + "context" + "database/sql" + "encoding/json" + "testing" + + "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "golang.org/x/exp/slices" + + "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbmock" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/runtimeconfig" + "github.com/coder/coder/v2/testutil" +) + +func TestRoleSyncTable(t *testing.T) { + t.Parallel() + + if dbtestutil.WillUsePostgres() { + t.Skip("Skipping test because it populates a lot of db entries, which is slow on postgres.") + } + + userClaims := jwt.MapClaims{ + "roles": []string{ + "foo", "bar", "baz", + "create-bar", "create-baz", + "legacy-bar", rbac.RoleOrgAuditor(), + }, + // bad-claim is a number, and will fail any role sync + "bad-claim": 100, + "empty": []string{}, + } + + testCases := []orgSetupDefinition{ + { + Name: "NoSync", + OrganizationRoles: []string{}, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{}, + }, + }, + { + Name: "SyncDisabled", + OrganizationRoles: []string{ + rbac.RoleOrgAdmin(), + }, + RoleSettings: &idpsync.RoleSyncSettings{}, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{ + rbac.RoleOrgAdmin(), + }, + }, + }, + { + // Audit role from claim + Name: "RawAudit", + OrganizationRoles: []string{ + rbac.RoleOrgAdmin(), + }, + RoleSettings: &idpsync.RoleSyncSettings{ + Field: "roles", + Mapping: map[string][]string{}, + }, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{ + rbac.RoleOrgAuditor(), + }, + }, + }, + { + Name: "CustomRole", + OrganizationRoles: []string{ + rbac.RoleOrgAdmin(), + }, + CustomRoles: []string{"foo"}, + RoleSettings: &idpsync.RoleSyncSettings{ + Field: "roles", + Mapping: map[string][]string{}, + }, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{ + rbac.RoleOrgAuditor(), + "foo", + }, + }, + }, + { + Name: "RoleMapping", + OrganizationRoles: []string{ + rbac.RoleOrgAdmin(), + "invalid", // Throw in an extra invalid role that will be removed + }, + CustomRoles: []string{"custom"}, + RoleSettings: &idpsync.RoleSyncSettings{ + Field: "roles", + Mapping: map[string][]string{ + "foo": {"custom", rbac.RoleOrgTemplateAdmin()}, + }, + }, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{ + rbac.RoleOrgAuditor(), + rbac.RoleOrgTemplateAdmin(), + "custom", + }, + }, + }, + { + // InvalidClaims will log an error, but do not block authentication. + // This is to prevent a misconfigured organization from blocking + // a user from authenticating. + Name: "InvalidClaim", + OrganizationRoles: []string{rbac.RoleOrgAdmin()}, + RoleSettings: &idpsync.RoleSyncSettings{ + Field: "bad-claim", + }, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{ + rbac.RoleOrgAdmin(), + }, + }, + }, + { + Name: "NoChange", + OrganizationRoles: []string{rbac.RoleOrgAdmin(), rbac.RoleOrgTemplateAdmin(), rbac.RoleOrgAuditor()}, + RoleSettings: &idpsync.RoleSyncSettings{ + Field: "roles", + Mapping: map[string][]string{ + "foo": {rbac.RoleOrgAuditor(), rbac.RoleOrgTemplateAdmin()}, + "bar": {rbac.RoleOrgAdmin()}, + }, + }, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{ + rbac.RoleOrgAdmin(), rbac.RoleOrgAuditor(), rbac.RoleOrgTemplateAdmin(), + }, + }, + }, + { + // InvalidOriginalRole starts the user with an invalid role. + // In practice, this should not happen, as it means a role was + // inserted into the database that does not exist. + // For the purposes of syncing, it does not matter, and the sync + // should succeed. + Name: "InvalidOriginalRole", + OrganizationRoles: []string{"something-bad"}, + RoleSettings: &idpsync.RoleSyncSettings{ + Field: "roles", + Mapping: map[string][]string{}, + }, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{ + rbac.RoleOrgAuditor(), + }, + }, + }, + { + Name: "NonExistentClaim", + OrganizationRoles: []string{rbac.RoleOrgAuditor()}, + RoleSettings: &idpsync.RoleSyncSettings{ + Field: "not-exists", + Mapping: map[string][]string{}, + }, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{}, + }, + }, + { + Name: "EmptyClaim", + OrganizationRoles: []string{rbac.RoleOrgAuditor()}, + RoleSettings: &idpsync.RoleSyncSettings{ + Field: "empty", + Mapping: map[string][]string{}, + }, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{}, + }, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + manager := runtimeconfig.NewManager() + s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{ + IgnoreErrors: true, + }), + manager, + idpsync.DeploymentSyncSettings{ + SiteRoleField: "roles", + }, + ) + + ctx := testutil.Context(t, testutil.WaitSuperLong) + user := dbgen.User(t, db, database.User{}) + orgID := uuid.New() + SetupOrganization(t, s, db, user, orgID, tc) + + // Do the role sync! + err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{ + SyncEntitled: true, + SyncSiteWide: false, + MergedClaims: userClaims, + }) + require.NoError(t, err) + + tc.Assert(t, orgID, db, user) + }) + } + + // AllTogether runs the entire tabled test as a singular user and + // deployment. This tests all organizations being synced together. + // The reason we do them individually, is that it is much easier to + // debug a single test case. + t.Run("AllTogether", func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + manager := runtimeconfig.NewManager() + s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{ + IgnoreErrors: true, + }), + manager, + // Also sync some site wide roles + idpsync.DeploymentSyncSettings{ + GroupField: "groups", + SiteRoleField: "roles", + // Site sync settings do not matter, + // as we are not testing the site parse here. + // Only the sync, assuming the parse is correct. + }, + ) + + ctx := testutil.Context(t, testutil.WaitSuperLong) + user := dbgen.User(t, db, database.User{}) + + var asserts []func(t *testing.T) + + for _, tc := range testCases { + tc := tc + + orgID := uuid.New() + SetupOrganization(t, s, db, user, orgID, tc) + asserts = append(asserts, func(t *testing.T) { + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + tc.Assert(t, orgID, db, user) + }) + }) + } + + err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{ + SyncEntitled: true, + SyncSiteWide: true, + SiteWideRoles: []string{ + rbac.RoleTemplateAdmin().Name, // Duplicate this value to test deduplication + rbac.RoleTemplateAdmin().Name, rbac.RoleAuditor().Name, + }, + MergedClaims: userClaims, + }) + require.NoError(t, err) + + for _, assert := range asserts { + assert(t) + } + + // Also assert site wide roles + //nolint:gocritic // unit testing assertions + allRoles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID) + require.NoError(t, err) + + allRoleIDs, err := allRoles.RoleNames() + require.NoError(t, err) + + // Remove the org roles + siteRoles := slices.DeleteFunc(allRoleIDs, func(r rbac.RoleIdentifier) bool { + return r.IsOrgRole() + }) + + require.ElementsMatch(t, []rbac.RoleIdentifier{ + rbac.RoleTemplateAdmin(), rbac.RoleAuditor(), rbac.RoleMember(), + }, siteRoles) + }) +} + +// TestNoopNoDiff verifies if no role change occurs, no database call is taken +// per organization. This limits the number of db calls to O(1) if there +// are no changes. Which is the usual case, as user's roles do not change often. +func TestNoopNoDiff(t *testing.T) { + t.Parallel() + + ctx := context.Background() + ctrl := gomock.NewController(t) + mDB := dbmock.NewMockStore(ctrl) + + mgr := runtimeconfig.NewManager() + s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), mgr, idpsync.DeploymentSyncSettings{ + SiteRoleField: "", + SiteRoleMapping: nil, + SiteDefaultRoles: nil, + }) + + userID := uuid.New() + orgID := uuid.New() + siteRoles := []string{rbac.RoleTemplateAdmin().Name, rbac.RoleAuditor().Name} + orgRoles := []string{rbac.RoleOrgAuditor(), rbac.RoleOrgAdmin()} + // The DB mock expects. + // If this test fails, feel free to add more expectations. + // The primary expectations to avoid is 'UpdateUserRoles' + // and 'UpdateMemberRoles'. + mDB.EXPECT().InTx( + gomock.Any(), gomock.Any(), + ).DoAndReturn(func(f func(database.Store) error, _ *sql.TxOptions) error { + err := f(mDB) + return err + }) + + mDB.EXPECT().OrganizationMembers(gomock.Any(), database.OrganizationMembersParams{ + UserID: userID, + }).Return([]database.OrganizationMembersRow{ + { + OrganizationMember: database.OrganizationMember{ + UserID: userID, + OrganizationID: orgID, + Roles: orgRoles, + }, + }, + }, nil) + + mDB.EXPECT().GetRuntimeConfig(gomock.Any(), gomock.Any()).Return( + string(must(json.Marshal(idpsync.RoleSyncSettings{ + Field: "roles", + Mapping: nil, + }))), nil) + + err := s.SyncRoles(ctx, mDB, database.User{ + ID: userID, + Email: "alice@email.com", + Username: "alice", + Status: database.UserStatusActive, + RBACRoles: siteRoles, + LoginType: database.LoginTypePassword, + }, idpsync.RoleParams{ + SyncEntitled: true, + SyncSiteWide: true, + SiteWideRoles: siteRoles, + MergedClaims: jwt.MapClaims{ + "roles": orgRoles, + }, + }) + require.NoError(t, err) +} + +func must[T any](value T, err error) T { + if err != nil { + panic(err) + } + return value +} diff --git a/coderd/members.go b/coderd/members.go index 6f5c0c5864f08..48006221efc59 100644 --- 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,11 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { aReq.Old = member.OrganizationMember.Auditable(member.Username) defer commitAudit() + // Check if changing roles is allowed + if !api.allowChangingMemberRoles(ctx, rw, member, organization) { + return + } + if apiKey.UserID == member.OrganizationMember.UserID { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "You cannot change your own organization roles.", @@ -259,6 +265,35 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, resp[0]) } +func (api *API) allowChangingMemberRoles(ctx context.Context, rw http.ResponseWriter, member httpmw.OrganizationMember, organization database.Organization) bool { + // 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 false + } + + 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 false + } + 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 false + } + } + + return true +} + // convertOrganizationMembers batches the role lookup to make only 1 sql call // We func convertOrganizationMembers(ctx context.Context, db database.Store, mems []database.OrganizationMember) ([]codersdk.OrganizationMember, error) { diff --git a/coderd/runtimeconfig/entry.go b/coderd/runtimeconfig/entry.go index c0260b0268ddb..6a696a88a825c 100644 --- a/coderd/runtimeconfig/entry.go +++ b/coderd/runtimeconfig/entry.go @@ -46,7 +46,7 @@ func MustNew[T EntryValue](name string) RuntimeEntry[T] { } // SetRuntimeValue attempts to update the runtime value of this field in the store via the given Mutator. -func (e *RuntimeEntry[T]) SetRuntimeValue(ctx context.Context, m Resolver, val T) error { +func (e RuntimeEntry[T]) SetRuntimeValue(ctx context.Context, m Resolver, val T) error { name, err := e.name() if err != nil { return xerrors.Errorf("set runtime: %w", err) @@ -56,7 +56,7 @@ func (e *RuntimeEntry[T]) SetRuntimeValue(ctx context.Context, m Resolver, val T } // UnsetRuntimeValue removes the runtime value from the store. -func (e *RuntimeEntry[T]) UnsetRuntimeValue(ctx context.Context, m Resolver) error { +func (e RuntimeEntry[T]) UnsetRuntimeValue(ctx context.Context, m Resolver) error { name, err := e.name() if err != nil { return xerrors.Errorf("unset runtime: %w", err) @@ -66,7 +66,7 @@ func (e *RuntimeEntry[T]) UnsetRuntimeValue(ctx context.Context, m Resolver) err } // Resolve attempts to resolve the runtime value of this field from the store via the given Resolver. -func (e *RuntimeEntry[T]) Resolve(ctx context.Context, r Resolver) (T, error) { +func (e RuntimeEntry[T]) Resolve(ctx context.Context, r Resolver) (T, error) { var zero T name, err := e.name() @@ -87,7 +87,7 @@ func (e *RuntimeEntry[T]) Resolve(ctx context.Context, r Resolver) (T, error) { } // name returns the configured name, or fails with ErrNameNotSet. -func (e *RuntimeEntry[T]) name() (string, error) { +func (e RuntimeEntry[T]) name() (string, error) { if e.n == "" { return "", ErrNameNotSet } diff --git a/coderd/userauth.go b/coderd/userauth.go index 4a63e0155f841..b0ef24ad978cf 100644 --- 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 index aed325c6fe735..48bc3ee15e4c5 100644 --- 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.", diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 5a392360b209a..86e8b0f36c87b 100644 --- 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.SetUserSiteRoles = api.setUserSiteRoles api.AGPL.SiteHandler.RegionsFetcher = func(ctx context.Context) (any, error) { // If the user can read the workspace proxy resource, return that. // If not, always default to the regions. diff --git a/enterprise/coderd/enidpsync/role.go b/enterprise/coderd/enidpsync/role.go new file mode 100644 index 0000000000000..f258e47cf1f78 --- /dev/null +++ b/enterprise/coderd/enidpsync/role.go @@ -0,0 +1,93 @@ +package enidpsync + +import ( + "context" + "fmt" + "net/http" + + "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" + "golang.org/x/xerrors" + + "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/coderd/runtimeconfig" + "github.com/coder/coder/v2/coderd/util/slice" + "github.com/coder/coder/v2/codersdk" +) + +func (e EnterpriseIDPSync) RoleSyncEntitled() bool { + return e.entitlements.Enabled(codersdk.FeatureUserRoleManagement) +} + +func (e EnterpriseIDPSync) OrganizationRoleSyncEnabled(ctx context.Context, db database.Store, orgID uuid.UUID) (bool, error) { + if !e.RoleSyncEntitled() { + return false, nil + } + roleSyncSettings, err := e.Role.Resolve(ctx, e.Manager.OrganizationResolver(db, orgID)) + if err != nil { + if xerrors.Is(err, runtimeconfig.ErrEntryNotFound) { + return false, nil + } + return false, err + } + return roleSyncSettings.Field != "", nil +} + +func (e EnterpriseIDPSync) SiteRoleSyncEnabled() bool { + if !e.RoleSyncEntitled() { + return false + } + return e.AGPLIDPSync.SiteRoleField != "" +} + +func (e EnterpriseIDPSync) ParseRoleClaims(ctx context.Context, mergedClaims jwt.MapClaims) (idpsync.RoleParams, *idpsync.HTTPError) { + if !e.RoleSyncEntitled() { + return e.AGPLIDPSync.ParseRoleClaims(ctx, mergedClaims) + } + + var claimRoles []string + if e.AGPLIDPSync.SiteRoleField != "" { + var err error + // TODO: Smoke test this error for org and site + claimRoles, err = e.AGPLIDPSync.RolesFromClaim(e.AGPLIDPSync.SiteRoleField, mergedClaims) + if err != nil { + rawType := mergedClaims[e.AGPLIDPSync.SiteRoleField] + e.Logger.Error(ctx, "oidc claims user roles field was an unknown type", + slog.F("type", fmt.Sprintf("%T", rawType)), + slog.F("field", e.AGPLIDPSync.SiteRoleField), + slog.F("raw_value", rawType), + slog.Error(err), + ) + // TODO: Determine a static page or not + return idpsync.RoleParams{}, &idpsync.HTTPError{ + Code: http.StatusInternalServerError, + Msg: "Login disabled until site wide 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.", rawType), + RenderStaticPage: false, + } + } + } + + siteRoles := append([]string{}, e.SiteDefaultRoles...) + for _, role := range claimRoles { + if mappedRoles, ok := e.SiteRoleMapping[role]; ok { + if len(mappedRoles) == 0 { + continue + } + // Mapped roles are added to the list of roles + siteRoles = append(siteRoles, mappedRoles...) + continue + } + // Append as is. + siteRoles = append(siteRoles, role) + } + + return idpsync.RoleParams{ + SyncEntitled: e.RoleSyncEntitled(), + SyncSiteWide: e.SiteRoleSyncEnabled(), + SiteWideRoles: slice.Unique(siteRoles), + MergedClaims: mergedClaims, + }, nil +} diff --git a/enterprise/coderd/enidpsync/role_test.go b/enterprise/coderd/enidpsync/role_test.go new file mode 100644 index 0000000000000..9e687684cb018 --- /dev/null +++ b/enterprise/coderd/enidpsync/role_test.go @@ -0,0 +1,144 @@ +package enidpsync_test + +import ( + "context" + "testing" + + "github.com/golang-jwt/jwt/v4" + "github.com/stretchr/testify/require" + + "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/entitlements" + "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/runtimeconfig" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/enterprise/coderd/enidpsync" +) + +func TestEnterpriseParseRoleClaims(t *testing.T) { + t.Parallel() + + entitled := entitlements.New() + entitled.Update(func(en *codersdk.Entitlements) { + en.Features[codersdk.FeatureUserRoleManagement] = codersdk.Feature{ + Entitlement: codersdk.EntitlementEntitled, + Enabled: true, + } + }) + + t.Run("NotEntitled", func(t *testing.T) { + t.Parallel() + + mgr := runtimeconfig.NewManager() + s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitlements.New(), idpsync.DeploymentSyncSettings{}) + + params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{}) + require.Nil(t, err) + require.False(t, params.SyncEntitled) + require.False(t, params.SyncSiteWide) + }) + + t.Run("NotEntitledButEnabled", func(t *testing.T) { + t.Parallel() + // Since it is not entitled, it should not be enabled + + mgr := runtimeconfig.NewManager() + s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitlements.New(), idpsync.DeploymentSyncSettings{ + SiteRoleField: "roles", + }) + + params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{}) + require.Nil(t, err) + require.False(t, params.SyncEntitled) + require.False(t, params.SyncSiteWide) + }) + + t.Run("SiteDisabled", func(t *testing.T) { + t.Parallel() + + mgr := runtimeconfig.NewManager() + s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitled, idpsync.DeploymentSyncSettings{}) + + params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{}) + require.Nil(t, err) + require.True(t, params.SyncEntitled) + require.False(t, params.SyncSiteWide) + }) + + t.Run("SiteEnabled", func(t *testing.T) { + t.Parallel() + + mgr := runtimeconfig.NewManager() + s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitled, idpsync.DeploymentSyncSettings{ + SiteRoleField: "roles", + SiteRoleMapping: map[string][]string{}, + SiteDefaultRoles: []string{rbac.RoleTemplateAdmin().Name}, + }) + + params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{ + "roles": []string{rbac.RoleAuditor().Name}, + }) + require.Nil(t, err) + require.True(t, params.SyncEntitled) + require.True(t, params.SyncSiteWide) + require.ElementsMatch(t, []string{ + rbac.RoleTemplateAdmin().Name, + rbac.RoleAuditor().Name, + }, params.SiteWideRoles) + }) + + t.Run("SiteMapping", func(t *testing.T) { + t.Parallel() + + mgr := runtimeconfig.NewManager() + s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitled, idpsync.DeploymentSyncSettings{ + SiteRoleField: "roles", + SiteRoleMapping: map[string][]string{ + "foo": {rbac.RoleAuditor().Name, rbac.RoleUserAdmin().Name}, + "bar": {rbac.RoleOwner().Name}, + }, + SiteDefaultRoles: []string{rbac.RoleTemplateAdmin().Name}, + }) + + params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{ + "roles": []string{"foo", "bar", "random"}, + }) + require.Nil(t, err) + require.True(t, params.SyncEntitled) + require.True(t, params.SyncSiteWide) + require.ElementsMatch(t, []string{ + rbac.RoleTemplateAdmin().Name, + rbac.RoleAuditor().Name, + rbac.RoleUserAdmin().Name, + rbac.RoleOwner().Name, + // Invalid claims are still passed at this point + "random", + }, params.SiteWideRoles) + }) + + t.Run("DuplicateRoles", func(t *testing.T) { + t.Parallel() + + mgr := runtimeconfig.NewManager() + s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitled, idpsync.DeploymentSyncSettings{ + SiteRoleField: "roles", + SiteRoleMapping: map[string][]string{ + "foo": {rbac.RoleOwner().Name, rbac.RoleAuditor().Name}, + "bar": {rbac.RoleOwner().Name}, + }, + SiteDefaultRoles: []string{rbac.RoleAuditor().Name}, + }) + + params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{ + "roles": []string{"foo", "bar", rbac.RoleAuditor().Name, rbac.RoleOwner().Name}, + }) + require.Nil(t, err) + require.True(t, params.SyncEntitled) + require.True(t, params.SyncSiteWide) + require.ElementsMatch(t, []string{ + rbac.RoleAuditor().Name, + rbac.RoleOwner().Name, + }, params.SiteWideRoles) + }) +} diff --git a/enterprise/coderd/userauth.go b/enterprise/coderd/userauth.go index 60cba28cc37f3..ddb2b8b672186 100644 --- a/enterprise/coderd/userauth.go +++ b/enterprise/coderd/userauth.go @@ -1,34 +1 @@ package coderd - -import ( - "context" - - "github.com/google/uuid" - "golang.org/x/xerrors" - - "cdr.dev/slog" - "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/codersdk" -) - -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", - slog.F("user_id", userID), slog.F("roles", roles), - ) - return nil - } - - // Should this be feature protected? - return db.InTx(func(tx database.Store) error { - _, err := db.UpdateUserRoles(ctx, database.UpdateUserRolesParams{ - GrantedRoles: roles, - ID: userID, - }) - if err != nil { - return xerrors.Errorf("set user roles(%s): %w", userID.String(), err) - } - - return nil - }, nil) -} diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index 3b42dc1aeec5f..f4e774461e406 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -42,7 +42,9 @@ func TestUserOIDC(t *testing.T) { runner := setupOIDCTest(t, oidcTestConfig{ Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true - cfg.UserRoleField = "roles" + }, + DeploymentValues: func(dv *codersdk.DeploymentValues) { + dv.OIDC.UserRoleField = "roles" }, }) @@ -239,7 +241,9 @@ func TestUserOIDC(t *testing.T) { runner := setupOIDCTest(t, oidcTestConfig{ Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true - cfg.UserRoleField = "roles" + }, + DeploymentValues: func(dv *codersdk.DeploymentValues) { + dv.OIDC.UserRoleField = "roles" }, }) @@ -267,9 +271,13 @@ func TestUserOIDC(t *testing.T) { runner := setupOIDCTest(t, oidcTestConfig{ Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true - cfg.UserRoleField = "roles" - cfg.UserRoleMapping = map[string][]string{ - oidcRoleName: {rbac.RoleTemplateAdmin().String()}, + }, + DeploymentValues: func(dv *codersdk.DeploymentValues) { + dv.OIDC.UserRoleField = "roles" + dv.OIDC.UserRoleMapping = serpent.Struct[map[string][]string]{ + Value: map[string][]string{ + oidcRoleName: {rbac.RoleTemplateAdmin().String()}, + }, } }, }) @@ -299,9 +307,13 @@ func TestUserOIDC(t *testing.T) { Userinfo: jwt.MapClaims{oidcRoleName: []string{rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}}, Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true - cfg.UserRoleField = "roles" - cfg.UserRoleMapping = map[string][]string{ - oidcRoleName: {rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}, + }, + DeploymentValues: func(dv *codersdk.DeploymentValues) { + dv.OIDC.UserRoleField = "roles" + dv.OIDC.UserRoleMapping = serpent.Struct[map[string][]string]{ + Value: map[string][]string{ + oidcRoleName: {rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}, + }, } }, }) @@ -334,9 +346,13 @@ func TestUserOIDC(t *testing.T) { Userinfo: jwt.MapClaims{oidcRoleName: []string{rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}}, Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true - cfg.UserRoleField = "roles" - cfg.UserRoleMapping = map[string][]string{ - oidcRoleName: {rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}, + }, + DeploymentValues: func(dv *codersdk.DeploymentValues) { + dv.OIDC.UserRoleField = "roles" + dv.OIDC.UserRoleMapping = serpent.Struct[map[string][]string]{ + Value: map[string][]string{ + oidcRoleName: {rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}, + }, } }, }) @@ -367,7 +383,9 @@ func TestUserOIDC(t *testing.T) { runner := setupOIDCTest(t, oidcTestConfig{ Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true - cfg.UserRoleField = "roles" + }, + DeploymentValues: func(dv *codersdk.DeploymentValues) { + dv.OIDC.UserRoleField = "roles" }, }) @@ -653,7 +671,9 @@ func TestUserOIDC(t *testing.T) { runner := setupOIDCTest(t, oidcTestConfig{ Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true - cfg.UserRoleField = "roles" + }, + DeploymentValues: func(dv *codersdk.DeploymentValues) { + dv.OIDC.UserRoleField = "roles" }, })