From 9a73013458d110bdf5ba03f095c6593a8f978878 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 9 Sep 2024 19:09:24 -0500 Subject: [PATCH 01/13] rolesync start --- coderd/idpsync/idpsync.go | 7 ++++++ coderd/idpsync/role.go | 52 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 coderd/idpsync/role.go diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 1e8b14956b652..bd0b078acfdb1 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -16,6 +16,7 @@ import ( "github.com/coder/coder/v2/coderd/runtimeconfig" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/site" + "github.com/coder/serpent" ) // IDPSync is an interface, so we can implement this as AGPL and as enterprise, @@ -76,6 +77,11 @@ type DeploymentSyncSettings struct { GroupAllowList map[string]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 serpent.Struct[map[string][]string] + SiteDefaultRoles []string } type DefaultOrgLegacySettings struct { @@ -112,6 +118,7 @@ type SyncSettings struct { DeploymentSyncSettings Group runtimeconfig.RuntimeEntry[*GroupSyncSettings] + Role runtimeconfig.RuntimeEntry[*RoleSyncSettings] } func NewAGPLSync(logger slog.Logger, manager *runtimeconfig.Manager, settings DeploymentSyncSettings) *AGPLIDPSync { diff --git a/coderd/idpsync/role.go b/coderd/idpsync/role.go new file mode 100644 index 0000000000000..5c543600d5409 --- /dev/null +++ b/coderd/idpsync/role.go @@ -0,0 +1,52 @@ +package idpsync + +import ( + "context" + + "github.com/golang-jwt/jwt/v4" + + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/runtimeconfig" +) + +type RoleParams struct { + // SyncEnabled if false will skip syncing the user's roles + SyncEnabled bool + MergedClaims jwt.MapClaims +} + +func (AGPLIDPSync) RoleSyncEnabled() bool { + // AGPL does not support syncing groups. + 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(), + }, 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 { + return nil + } + + // nolint:gocritic // all syncing is done as a system user + ctx = dbauthz.AsSystemRestricted(ctx) + + return 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"` +} From c6080b5a64164c24e9b30f02b101c54aac6c3784 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 10 Sep 2024 11:34:07 -0500 Subject: [PATCH 02/13] chore: implement organization and site wide role sync in idpsync --- coderd/idpsync/idpsync.go | 4 +- coderd/idpsync/role.go | 210 +++++++++++++++++++++++++++- coderd/runtimeconfig/entry.go | 8 +- enterprise/coderd/enidpsync/role.go | 67 +++++++++ 4 files changed, 281 insertions(+), 8 deletions(-) create mode 100644 enterprise/coderd/enidpsync/role.go diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index bd0b078acfdb1..3131be7cbc6fe 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -16,7 +16,6 @@ import ( "github.com/coder/coder/v2/coderd/runtimeconfig" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/site" - "github.com/coder/serpent" ) // IDPSync is an interface, so we can implement this as AGPL and as enterprise, @@ -80,7 +79,7 @@ type DeploymentSyncSettings struct { // SiteRoleField syncs a user's site wide roles from an IDP. SiteRoleField string - SiteRoleMapping serpent.Struct[map[string][]string] + SiteRoleMapping map[string][]string SiteDefaultRoles []string } @@ -128,6 +127,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 index 5c543600d5409..3d23db28a861a 100644 --- a/coderd/idpsync/role.go +++ b/coderd/idpsync/role.go @@ -2,17 +2,27 @@ 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" ) type RoleParams struct { // SyncEnabled if false will skip syncing the user's roles - SyncEnabled bool + SyncEnabled bool + SyncSiteWide bool + SiteWideRoles []string + // MergedClaims are passed to the organization level for syncing MergedClaims jwt.MapClaims } @@ -26,7 +36,8 @@ func (s AGPLIDPSync) RoleSyncSettings() runtimeconfig.RuntimeEntry[*RoleSyncSett func (s AGPLIDPSync) ParseRoleClaims(_ context.Context, _ jwt.MapClaims) (RoleParams, *HTTPError) { return RoleParams{ - SyncEnabled: s.RoleSyncEnabled(), + SyncEnabled: s.RoleSyncEnabled(), + SyncSiteWide: false, }, nil } @@ -39,9 +50,196 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data // 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{ + 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), + ) + + // Failing role sync should reset a user's roles. + expectedRoles[orgID] = []rbac.RoleIdentifier{} + + // 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) + } + } + // Always add the member role to the user. + validExpected = append(validExpected, rbac.RoleOrgMember()) + + // Is there a difference between the expected roles and the existing roles? + if !slices.Equal(existingRoles[orgID], validExpected) { + _, 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 } +// resetUserOrgRoles will reset the user's roles for a specific organization. +// It does not remove them as a member from the organization. +func (s AGPLIDPSync) resetUserOrgRoles(ctx context.Context, tx database.Store, member database.OrganizationMembersRow, orgID uuid.UUID) error { + withoutMember := slices.DeleteFunc(member.OrganizationMember.Roles, func(s string) bool { + return s == rbac.RoleOrgMember() + }) + // If the user has no roles, then skip doing any database request. + if len(withoutMember) == 0 { + return nil + } + + _, err := tx.UpdateMemberRoles(ctx, database.UpdateMemberRolesParams{ + GrantedRoles: []string{}, + UserID: member.OrganizationMember.UserID, + OrgID: orgID, + }) + if err != nil { + return xerrors.Errorf("zero out member roles(%s): %w", member.OrganizationMember.UserID.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. + 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), + ) + } + + _, 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 (s 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 @@ -50,3 +248,11 @@ type RoleSyncSettings struct { // 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/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/enterprise/coderd/enidpsync/role.go b/enterprise/coderd/enidpsync/role.go new file mode 100644 index 0000000000000..78c6426ce39c6 --- /dev/null +++ b/enterprise/coderd/enidpsync/role.go @@ -0,0 +1,67 @@ +package enidpsync + +import ( + "context" + "fmt" + "net/http" + + "github.com/golang-jwt/jwt/v4" + + "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/codersdk" +) + +func (e EnterpriseIDPSync) RoleSyncEnabled() bool { + return e.entitlements.Enabled(codersdk.FeatureUserRoleManagement) +} + +func (e EnterpriseIDPSync) ParseRoleClaims(ctx context.Context, mergedClaims jwt.MapClaims) (idpsync.RoleParams, *idpsync.HTTPError) { + if !e.RoleSyncEnabled() { + 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: Deterine 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{ + SyncEnabled: e.RoleSyncEnabled(), + SyncSiteWide: e.AGPLIDPSync.SiteRoleField != "", + SiteWideRoles: siteRoles, + MergedClaims: mergedClaims, + }, nil +} From ca8e9b973cf4476cff0c00180115303132582520 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 10 Sep 2024 16:09:19 -0500 Subject: [PATCH 03/13] work on unit test for role sync --- coderd/idpsync/group_test.go | 149 +++++++++++++++++++------- coderd/idpsync/role.go | 13 ++- coderd/idpsync/role_test.go | 201 +++++++++++++++++++++++++++++++++++ 3 files changed, 323 insertions(+), 40 deletions(-) create mode 100644 coderd/idpsync/role_test.go diff --git a/coderd/idpsync/group_test.go b/coderd/idpsync/group_test.go index 0d64d8abf6059..64b7b87236a06 100644 --- a/coderd/idpsync/group_test.go +++ b/coderd/idpsync/group_test.go @@ -102,11 +102,13 @@ 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"), + }, }, }, { @@ -125,8 +127,10 @@ 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"), + }, }, }, { @@ -145,11 +149,13 @@ 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"), + }, }, }, { @@ -160,9 +166,11 @@ func TestGroupSyncTable(t *testing.T) { AutoCreateMissing: true, }, Groups: map[uuid.UUID]bool{}, - ExpectedGroupNames: []string{ - "create-bar", - "create-baz", + assertGroups: &orgGroupAssert{ + ExpectedGroupNames: []string{ + "create-bar", + "create-baz", + }, }, }, { @@ -177,9 +185,11 @@ func TestGroupSyncTable(t *testing.T) { "bar": false, "goob": true, }, - ExpectedGroupNames: []string{ - "foo", - "bar", + assertGroups: &orgGroupAssert{ + ExpectedGroupNames: []string{ + "foo", + "bar", + }, }, }, { @@ -200,9 +210,9 @@ func TestGroupSyncTable(t *testing.T) { }, }, { - Name: "NoSettingsNoUser", - Settings: nil, - Groups: map[uuid.UUID]bool{}, + Name: "NoSettingsNoUser", + GroupSettings: nil, + Groups: map[uuid.UUID]bool{}, }, { Name: "LegacyMapping", @@ -224,9 +234,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 +323,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 @@ -392,9 +405,11 @@ func TestSyncDisabled(t *testing.T) { "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,11 +743,12 @@ 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)) - require.NoError(t, err) - } + orgResolver := manager.OrganizationResolver(db, org.ID) + err = s.Group.SetRuntimeValue(context.Background(), orgResolver, def.GroupSettings) + require.NoError(t, err) + + err = s.Role.SetRuntimeValue(context.Background(), orgResolver, def.RoleSettings) + require.NoError(t, err) if !def.NotMember { dbgen.OrganizationMember(t, db, database.OrganizationMember{ @@ -740,6 +756,14 @@ 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) + } for groupID, in := range def.Groups { dbgen.Group(t, db, database.Group{ ID: groupID, @@ -771,9 +795,23 @@ type orgSetupDefinition struct { // True if the user is a member of the group Groups map[uuid.UUID]bool GroupNames map[string]bool - NotMember bool - Settings *codersdk.GroupSyncSettings + OrganizationRoles []string + // NotMember if true will ensure the user is not a member of the organization. + NotMember bool + + GroupSettings *idpsync.GroupSyncSettings + RoleSettings *idpsync.RoleSyncSettings + + assertGroups *orgGroupAssert + assertRoles *orgRoleAssert +} + +type orgRoleAssert struct { + ExpectedOrgRoles []string +} + +type orgGroupAssert struct { ExpectedGroups []uuid.UUID ExpectedGroupNames []string } @@ -794,6 +832,24 @@ 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 o.assertGroups == nil && o.assertRoles == nil { + 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 +882,22 @@ func (o orgSetupDefinition) Assert(t *testing.T, orgID uuid.UUID, db database.St require.Len(t, o.ExpectedGroupNames, 0, "ExpectedGroupNames should be empty") } } + +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/role.go b/coderd/idpsync/role.go index 3d23db28a861a..d06eab3e57e0d 100644 --- a/coderd/idpsync/role.go +++ b/coderd/idpsync/role.go @@ -141,11 +141,18 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data validExpected = append(validExpected, role.Name) } } - // Always add the member role to the user. - validExpected = append(validExpected, rbac.RoleOrgMember()) + // 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() + }) // Is there a difference between the expected roles and the existing roles? - if !slices.Equal(existingRoles[orgID], validExpected) { + if !slices.Equal(existingFound, validExpected) { _, err = tx.UpdateMemberRoles(ctx, database.UpdateMemberRolesParams{ GrantedRoles: validExpected, UserID: user.ID, diff --git a/coderd/idpsync/role_test.go b/coderd/idpsync/role_test.go new file mode 100644 index 0000000000000..920f31cc06526 --- /dev/null +++ b/coderd/idpsync/role_test.go @@ -0,0 +1,201 @@ +package idpsync_test + +import ( + "testing" + + "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" + "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() + + // Last checked, takes 30s with postgres on a fast machine. + 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(), + }, + } + + //ids := coderdtest.NewDeterministicUUIDGenerator() + testCases := []orgSetupDefinition{ + { + Name: "NoSync", + OrganizationRoles: []string{}, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{}, + }, + }, + { + Name: "NoSyncNoChange", + OrganizationRoles: []string{ + rbac.RoleOrgAdmin(), + }, + assertRoles: &orgRoleAssert{ + ExpectedOrgRoles: []string{ + rbac.RoleOrgAdmin(), + }, + }, + }, + { + Name: "NoChange", + 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(), + }, + }, + }, + } + + 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{}), + 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 group sync! + err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{ + SyncEnabled: true, + 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{}), + // manager, + // // Also sync the default org! + // idpsync.DeploymentSyncSettings{ + // GroupField: "groups", + // Legacy: idpsync.DefaultOrgLegacySettings{ + // GroupField: "groups", + // GroupMapping: map[string]string{ + // "foo": "legacy-foo", + // "baz": "legacy-baz", + // }, + // GroupFilter: regexp.MustCompile("^legacy"), + // CreateMissingGroups: true, + // }, + // }, + // ) + // + // ctx := testutil.Context(t, testutil.WaitSuperLong) + // user := dbgen.User(t, db, database.User{}) + // + // var asserts []func(t *testing.T) + // // The default org is also going to do something + // def := orgSetupDefinition{ + // Name: "DefaultOrg", + // GroupNames: map[string]bool{ + // "legacy-foo": false, + // "legacy-baz": true, + // "random": true, + // }, + // // No settings, because they come from the deployment values + // GroupSettings: nil, + // assertGroups: &orgGroupAssert{ + // ExpectedGroupNames: []string{"legacy-foo", "legacy-baz", "legacy-bar"}, + // }, + // } + // + // //nolint:gocritic // testing + // defOrg, err := db.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) + // require.NoError(t, err) + // SetupOrganization(t, s, db, user, defOrg.ID, def) + // asserts = append(asserts, func(t *testing.T) { + // t.Run(def.Name, func(t *testing.T) { + // t.Parallel() + // def.Assert(t, defOrg.ID, db, user) + // }) + // }) + // + // 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) + // }) + // }) + // } + // + // asserts = append(asserts, func(t *testing.T) { + // t.Helper() + // def.Assert(t, defOrg.ID, db, user) + // }) + // + // // Do the group sync! + // err = s.SyncGroups(ctx, db, user, idpsync.GroupParams{ + // SyncEnabled: true, + // MergedClaims: userClaims, + // }) + // require.NoError(t, err) + // + // for _, assert := range asserts { + // assert(t) + // } + //}) +} From b1ece73db3379095a4199bb7351e91b4f333872d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 11 Sep 2024 10:56:39 -0500 Subject: [PATCH 04/13] chore: remove resetting user's roles on misconfigured --- coderd/idpsync/group_test.go | 28 ++++++++++++++--- coderd/idpsync/role.go | 34 ++++++-------------- coderd/idpsync/role_test.go | 61 +++++++++++++++++++++++++++++++----- 3 files changed, 88 insertions(+), 35 deletions(-) diff --git a/coderd/idpsync/group_test.go b/coderd/idpsync/group_test.go index 64b7b87236a06..016362c783b0a 100644 --- a/coderd/idpsync/group_test.go +++ b/coderd/idpsync/group_test.go @@ -756,6 +756,7 @@ 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, @@ -764,6 +765,24 @@ func SetupOrganization(t *testing.T, s *idpsync.AGPLIDPSync, db database.Store, }) 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, @@ -793,10 +812,10 @@ 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 - + 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 @@ -839,7 +858,8 @@ func (o orgSetupDefinition) Assert(t *testing.T, orgID uuid.UUID, db database.St o.assertRoles.Assert(t, orgID, db, o.NotMember, user) } - if o.assertGroups == nil && o.assertRoles == nil { + // 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() } diff --git a/coderd/idpsync/role.go b/coderd/idpsync/role.go index d06eab3e57e0d..e0c6f2e4facd5 100644 --- a/coderd/idpsync/role.go +++ b/coderd/idpsync/role.go @@ -98,8 +98,16 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data slog.Error(err), ) - // Failing role sync should reset a user's roles. - expectedRoles[orgID] = []rbac.RoleIdentifier{} + // 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 @@ -172,28 +180,6 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data return nil } -// resetUserOrgRoles will reset the user's roles for a specific organization. -// It does not remove them as a member from the organization. -func (s AGPLIDPSync) resetUserOrgRoles(ctx context.Context, tx database.Store, member database.OrganizationMembersRow, orgID uuid.UUID) error { - withoutMember := slices.DeleteFunc(member.OrganizationMember.Roles, func(s string) bool { - return s == rbac.RoleOrgMember() - }) - // If the user has no roles, then skip doing any database request. - if len(withoutMember) == 0 { - return nil - } - - _, err := tx.UpdateMemberRoles(ctx, database.UpdateMemberRolesParams{ - GrantedRoles: []string{}, - UserID: member.OrganizationMember.UserID, - OrgID: orgID, - }) - if err != nil { - return xerrors.Errorf("zero out member roles(%s): %w", member.OrganizationMember.UserID.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 diff --git a/coderd/idpsync/role_test.go b/coderd/idpsync/role_test.go index 920f31cc06526..1a10533c7f511 100644 --- a/coderd/idpsync/role_test.go +++ b/coderd/idpsync/role_test.go @@ -31,6 +31,8 @@ func TestRoleSyncTable(t *testing.T) { "create-bar", "create-baz", "legacy-bar", rbac.RoleOrgAuditor(), }, + // bad-claim is a number, and will fail any role sync + "bad-claim": 100, } //ids := coderdtest.NewDeterministicUUIDGenerator() @@ -43,10 +45,11 @@ func TestRoleSyncTable(t *testing.T) { }, }, { - Name: "NoSyncNoChange", + Name: "SyncDisabled", OrganizationRoles: []string{ rbac.RoleOrgAdmin(), }, + RoleSettings: &idpsync.RoleSyncSettings{}, assertRoles: &orgRoleAssert{ ExpectedOrgRoles: []string{ rbac.RoleOrgAdmin(), @@ -54,23 +57,27 @@ func TestRoleSyncTable(t *testing.T) { }, }, { - Name: "NoChange", + // Audit role from claim + Name: "RawAudit", OrganizationRoles: []string{ rbac.RoleOrgAdmin(), }, - RoleSettings: &idpsync.RoleSyncSettings{}, + RoleSettings: &idpsync.RoleSyncSettings{ + Field: "roles", + Mapping: map[string][]string{}, + }, assertRoles: &orgRoleAssert{ ExpectedOrgRoles: []string{ - rbac.RoleOrgAdmin(), + rbac.RoleOrgAuditor(), }, }, }, { - // Audit role from claim - Name: "RawAudit", + Name: "CustomRole", OrganizationRoles: []string{ rbac.RoleOrgAdmin(), }, + CustomRoles: []string{"foo"}, RoleSettings: &idpsync.RoleSyncSettings{ Field: "roles", Mapping: map[string][]string{}, @@ -78,6 +85,43 @@ func TestRoleSyncTable(t *testing.T) { 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(), }, }, }, @@ -90,7 +134,9 @@ func TestRoleSyncTable(t *testing.T) { db, _ := dbtestutil.NewDB(t) manager := runtimeconfig.NewManager() - s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), + s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{ + IgnoreErrors: true, + }), manager, idpsync.DeploymentSyncSettings{ SiteRoleField: "roles", @@ -105,6 +151,7 @@ func TestRoleSyncTable(t *testing.T) { // Do the group sync! err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{ SyncEnabled: true, + SyncSiteWide: false, MergedClaims: userClaims, }) require.NoError(t, err) From 5d0f729aae82fb5f2a5e7d0567fe36d14a014db1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 11 Sep 2024 13:46:47 -0500 Subject: [PATCH 05/13] Begin unit testing work --- coderd/database/dbmem/dbmem.go | 2 +- coderd/idpsync/role.go | 10 ++ coderd/idpsync/role_test.go | 212 ++++++++++++++++++++------------- 3 files changed, 139 insertions(+), 85 deletions(-) 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/role.go b/coderd/idpsync/role.go index e0c6f2e4facd5..4e4a57ecf0202 100644 --- a/coderd/idpsync/role.go +++ b/coderd/idpsync/role.go @@ -15,6 +15,7 @@ import ( "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 { @@ -159,8 +160,15 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data 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, @@ -189,6 +197,8 @@ func (s AGPLIDPSync) syncSiteWideRoles(ctx context.Context, tx database.Store, u 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 { diff --git a/coderd/idpsync/role_test.go b/coderd/idpsync/role_test.go index 1a10533c7f511..064761a2701c6 100644 --- a/coderd/idpsync/role_test.go +++ b/coderd/idpsync/role_test.go @@ -6,9 +6,11 @@ import ( "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "github.com/stretchr/testify/require" + "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/dbtestutil" "github.com/coder/coder/v2/coderd/idpsync" @@ -20,7 +22,6 @@ import ( func TestRoleSyncTable(t *testing.T) { t.Parallel() - // Last checked, takes 30s with postgres on a fast machine. if dbtestutil.WillUsePostgres() { t.Skip("Skipping test because it populates a lot of db entries, which is slow on postgres.") } @@ -33,9 +34,9 @@ func TestRoleSyncTable(t *testing.T) { }, // bad-claim is a number, and will fail any role sync "bad-claim": 100, + "empty": []string{}, } - //ids := coderdtest.NewDeterministicUUIDGenerator() testCases := []orgSetupDefinition{ { Name: "NoSync", @@ -125,6 +126,62 @@ func TestRoleSyncTable(t *testing.T) { }, }, }, + { + 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 { @@ -148,7 +205,7 @@ func TestRoleSyncTable(t *testing.T) { orgID := uuid.New() SetupOrganization(t, s, db, user, orgID, tc) - // Do the group sync! + // Do the role sync! err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{ SyncEnabled: true, SyncSiteWide: false, @@ -164,85 +221,72 @@ func TestRoleSyncTable(t *testing.T) { // 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{}), - // manager, - // // Also sync the default org! - // idpsync.DeploymentSyncSettings{ - // GroupField: "groups", - // Legacy: idpsync.DefaultOrgLegacySettings{ - // GroupField: "groups", - // GroupMapping: map[string]string{ - // "foo": "legacy-foo", - // "baz": "legacy-baz", - // }, - // GroupFilter: regexp.MustCompile("^legacy"), - // CreateMissingGroups: true, - // }, - // }, - // ) - // - // ctx := testutil.Context(t, testutil.WaitSuperLong) - // user := dbgen.User(t, db, database.User{}) - // - // var asserts []func(t *testing.T) - // // The default org is also going to do something - // def := orgSetupDefinition{ - // Name: "DefaultOrg", - // GroupNames: map[string]bool{ - // "legacy-foo": false, - // "legacy-baz": true, - // "random": true, - // }, - // // No settings, because they come from the deployment values - // GroupSettings: nil, - // assertGroups: &orgGroupAssert{ - // ExpectedGroupNames: []string{"legacy-foo", "legacy-baz", "legacy-bar"}, - // }, - // } - // - // //nolint:gocritic // testing - // defOrg, err := db.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) - // require.NoError(t, err) - // SetupOrganization(t, s, db, user, defOrg.ID, def) - // asserts = append(asserts, func(t *testing.T) { - // t.Run(def.Name, func(t *testing.T) { - // t.Parallel() - // def.Assert(t, defOrg.ID, db, user) - // }) - // }) - // - // 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) - // }) - // }) - // } - // - // asserts = append(asserts, func(t *testing.T) { - // t.Helper() - // def.Assert(t, defOrg.ID, db, user) - // }) - // - // // Do the group sync! - // err = s.SyncGroups(ctx, db, user, idpsync.GroupParams{ - // SyncEnabled: true, - // MergedClaims: userClaims, - // }) - // require.NoError(t, err) - // - // for _, assert := range asserts { - // assert(t) - // } - //}) + 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{ + SyncEnabled: 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) + + siteRoles := slices.DeleteFunc(allRoleIDs, func(r rbac.RoleIdentifier) bool { + return r.IsOrgRole() + }) + + require.ElementsMatch(t, []rbac.RoleIdentifier{ + rbac.RoleTemplateAdmin(), rbac.RoleAuditor(), rbac.RoleMember(), + }, siteRoles) + }) } From 601652cd88c9ae7db3329ed9c662872b87a5185d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 11 Sep 2024 14:41:29 -0500 Subject: [PATCH 06/13] test enterprise parse --- coderd/idpsync/role.go | 19 ++- coderd/idpsync/role_test.go | 79 +++++++++++++ enterprise/coderd/enidpsync/role.go | 3 +- enterprise/coderd/enidpsync/role_test.go | 144 +++++++++++++++++++++++ 4 files changed, 238 insertions(+), 7 deletions(-) create mode 100644 enterprise/coderd/enidpsync/role_test.go diff --git a/coderd/idpsync/role.go b/coderd/idpsync/role.go index 4e4a57ecf0202..99da53f198af0 100644 --- a/coderd/idpsync/role.go +++ b/coderd/idpsync/role.go @@ -214,12 +214,19 @@ func (s AGPLIDPSync) syncSiteWideRoles(ctx context.Context, tx database.Store, u ) } - _, err := tx.UpdateUserRoles(ctx, database.UpdateUserRolesParams{ - GrantedRoles: filtered, - ID: user.ID, - }) - if err != nil { - return xerrors.Errorf("set site wide roles: %w", err) + 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 } diff --git a/coderd/idpsync/role_test.go b/coderd/idpsync/role_test.go index 064761a2701c6..7e5a130ba85fb 100644 --- a/coderd/idpsync/role_test.go +++ b/coderd/idpsync/role_test.go @@ -1,17 +1,22 @@ 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" @@ -281,6 +286,7 @@ func TestRoleSyncTable(t *testing.T) { allRoleIDs, err := allRoles.RoleNames() require.NoError(t, err) + // Remove the org roles siteRoles := slices.DeleteFunc(allRoleIDs, func(r rbac.RoleIdentifier) bool { return r.IsOrgRole() }) @@ -290,3 +296,76 @@ func TestRoleSyncTable(t *testing.T) { }, 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) { + 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{ + SyncEnabled: 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/enterprise/coderd/enidpsync/role.go b/enterprise/coderd/enidpsync/role.go index 78c6426ce39c6..b95cfaa352730 100644 --- a/enterprise/coderd/enidpsync/role.go +++ b/enterprise/coderd/enidpsync/role.go @@ -9,6 +9,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" ) @@ -61,7 +62,7 @@ func (e EnterpriseIDPSync) ParseRoleClaims(ctx context.Context, mergedClaims jwt return idpsync.RoleParams{ SyncEnabled: e.RoleSyncEnabled(), SyncSiteWide: e.AGPLIDPSync.SiteRoleField != "", - SiteWideRoles: siteRoles, + 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..15edd44976d80 --- /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.SyncEnabled) + 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.SyncEnabled) + 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.SyncEnabled) + 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.SyncEnabled) + 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.SyncEnabled) + 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.SyncEnabled) + require.True(t, params.SyncSiteWide) + require.ElementsMatch(t, []string{ + rbac.RoleAuditor().Name, + rbac.RoleOwner().Name, + }, params.SiteWideRoles) + }) +} From 2a1e2d06201d6bfb184c72cbf38e90b1bd838381 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 11 Sep 2024 15:13:36 -0500 Subject: [PATCH 07/13] chore: remove old role sync, insert new idpsync package --- cli/server.go | 3 - coderd/coderd.go | 9 -- coderd/idpsync/idpsync.go | 35 ++++++- coderd/idpsync/role.go | 22 ++-- coderd/idpsync/role_test.go | 6 +- coderd/members.go | 28 +++++ coderd/userauth.go | 125 +++-------------------- coderd/users.go | 2 +- enterprise/coderd/coderd.go | 1 - enterprise/coderd/enidpsync/role.go | 33 +++++- enterprise/coderd/enidpsync/role_test.go | 12 +-- enterprise/coderd/userauth.go | 33 ------ enterprise/coderd/userauth_test.go | 46 ++++++--- 13 files changed, 160 insertions(+), 195 deletions(-) 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/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 3131be7cbc6fe..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 @@ -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 index 99da53f198af0..8fd7612dde701 100644 --- 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 index 7e5a130ba85fb..89ed4c10b6f79 100644 --- 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 index 6f5c0c5864f08..52bf7284aaeee 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,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 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 index b95cfaa352730..662a04b879717 100644 --- a/enterprise/coderd/enidpsync/role.go +++ b/enterprise/coderd/enidpsync/role.go @@ -6,19 +6,44 @@ import ( "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) RoleSyncEnabled() bool { +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.RoleSyncEnabled() { + if !e.RoleSyncEntitled() { return e.AGPLIDPSync.ParseRoleClaims(ctx, mergedClaims) } @@ -60,8 +85,8 @@ func (e EnterpriseIDPSync) ParseRoleClaims(ctx context.Context, mergedClaims jwt } return idpsync.RoleParams{ - SyncEnabled: e.RoleSyncEnabled(), - SyncSiteWide: e.AGPLIDPSync.SiteRoleField != "", + 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 index 15edd44976d80..9e687684cb018 100644 --- a/enterprise/coderd/enidpsync/role_test.go +++ b/enterprise/coderd/enidpsync/role_test.go @@ -35,7 +35,7 @@ func TestEnterpriseParseRoleClaims(t *testing.T) { params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{}) require.Nil(t, err) - require.False(t, params.SyncEnabled) + require.False(t, params.SyncEntitled) require.False(t, params.SyncSiteWide) }) @@ -50,7 +50,7 @@ func TestEnterpriseParseRoleClaims(t *testing.T) { params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{}) require.Nil(t, err) - require.False(t, params.SyncEnabled) + require.False(t, params.SyncEntitled) require.False(t, params.SyncSiteWide) }) @@ -62,7 +62,7 @@ func TestEnterpriseParseRoleClaims(t *testing.T) { params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{}) require.Nil(t, err) - require.True(t, params.SyncEnabled) + require.True(t, params.SyncEntitled) require.False(t, params.SyncSiteWide) }) @@ -80,7 +80,7 @@ func TestEnterpriseParseRoleClaims(t *testing.T) { "roles": []string{rbac.RoleAuditor().Name}, }) require.Nil(t, err) - require.True(t, params.SyncEnabled) + require.True(t, params.SyncEntitled) require.True(t, params.SyncSiteWide) require.ElementsMatch(t, []string{ rbac.RoleTemplateAdmin().Name, @@ -105,7 +105,7 @@ func TestEnterpriseParseRoleClaims(t *testing.T) { "roles": []string{"foo", "bar", "random"}, }) require.Nil(t, err) - require.True(t, params.SyncEnabled) + require.True(t, params.SyncEntitled) require.True(t, params.SyncSiteWide) require.ElementsMatch(t, []string{ rbac.RoleTemplateAdmin().Name, @@ -134,7 +134,7 @@ func TestEnterpriseParseRoleClaims(t *testing.T) { "roles": []string{"foo", "bar", rbac.RoleAuditor().Name, rbac.RoleOwner().Name}, }) require.Nil(t, err) - require.True(t, params.SyncEnabled) + require.True(t, params.SyncEntitled) require.True(t, params.SyncSiteWide) require.ElementsMatch(t, []string{ rbac.RoleAuditor().Name, 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" }, }) From 8967a42c466db4b23591a5b1e70eb47f46d072d6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 11 Sep 2024 15:24:06 -0500 Subject: [PATCH 08/13] fixup test --- coderd/idpsync/group_test.go | 5 ++++- enterprise/coderd/enidpsync/role.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/coderd/idpsync/group_test.go b/coderd/idpsync/group_test.go index 016362c783b0a..4a0629293853f 100644 --- a/coderd/idpsync/group_test.go +++ b/coderd/idpsync/group_test.go @@ -210,9 +210,12 @@ func TestGroupSyncTable(t *testing.T) { }, }, { - Name: "NoSettingsNoUser", + Name: "NoSettings", GroupSettings: nil, Groups: map[uuid.UUID]bool{}, + assertGroups: &orgGroupAssert{ + ExpectedGroups: []uuid.UUID{}, + }, }, { Name: "LegacyMapping", diff --git a/enterprise/coderd/enidpsync/role.go b/enterprise/coderd/enidpsync/role.go index 662a04b879717..f258e47cf1f78 100644 --- a/enterprise/coderd/enidpsync/role.go +++ b/enterprise/coderd/enidpsync/role.go @@ -60,7 +60,7 @@ func (e EnterpriseIDPSync) ParseRoleClaims(ctx context.Context, mergedClaims jwt slog.F("raw_value", rawType), slog.Error(err), ) - // TODO: Deterine a static page or not + // 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", From f51fae7c99648c9959765c814618b169ba632066 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 11 Sep 2024 16:01:49 -0500 Subject: [PATCH 09/13] linting --- coderd/idpsync/group_test.go | 1 + coderd/idpsync/role.go | 5 +++-- coderd/idpsync/role_test.go | 2 ++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/coderd/idpsync/group_test.go b/coderd/idpsync/group_test.go index 4a0629293853f..dea1241ce80dd 100644 --- a/coderd/idpsync/group_test.go +++ b/coderd/idpsync/group_test.go @@ -906,6 +906,7 @@ func (o orgGroupAssert) Assert(t *testing.T, orgID uuid.UUID, db database.Store, } } +//nolint:revive func (o orgRoleAssert) Assert(t *testing.T, orgID uuid.UUID, db database.Store, notMember bool, user database.User) { t.Helper() diff --git a/coderd/idpsync/role.go b/coderd/idpsync/role.go index 8fd7612dde701..9c63df157a764 100644 --- a/coderd/idpsync/role.go +++ b/coderd/idpsync/role.go @@ -70,7 +70,8 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data // sync roles per organization orgMemberships, err := tx.OrganizationMembers(ctx, database.OrganizationMembersParams{ - UserID: user.ID, + OrganizationID: uuid.Nil, + UserID: user.ID, }) if err != nil { return xerrors.Errorf("get organizations by user id: %w", err) @@ -241,7 +242,7 @@ func (s AGPLIDPSync) syncSiteWideRoles(ctx context.Context, tx database.Store, u return nil } -func (s AGPLIDPSync) RolesFromClaim(field string, claims jwt.MapClaims) ([]string, error) { +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 diff --git a/coderd/idpsync/role_test.go b/coderd/idpsync/role_test.go index 89ed4c10b6f79..c6ab989881976 100644 --- a/coderd/idpsync/role_test.go +++ b/coderd/idpsync/role_test.go @@ -301,6 +301,8 @@ func TestRoleSyncTable(t *testing.T) { // 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) From 12c7af78b6779790a997a4af9c35ba1e4f1018ae Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 Sep 2024 12:33:23 -0500 Subject: [PATCH 10/13] rebase fixes --- coderd/idpsync/group_test.go | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/coderd/idpsync/group_test.go b/coderd/idpsync/group_test.go index dea1241ce80dd..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")}, @@ -113,7 +113,7 @@ func TestGroupSyncTable(t *testing.T) { }, { Name: "StayInGroup", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", // Only match foo, so bar does not map RegexFilter: regexp.MustCompile("^foo$"), @@ -135,7 +135,7 @@ func TestGroupSyncTable(t *testing.T) { }, { Name: "UserJoinsGroups", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", Mapping: map[string][]uuid.UUID{ "foo": {ids.ID("ng-foo"), uuid.New()}, @@ -160,7 +160,7 @@ func TestGroupSyncTable(t *testing.T) { }, { Name: "CreateGroups", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", RegexFilter: regexp.MustCompile("^create"), AutoCreateMissing: true, @@ -175,7 +175,7 @@ func TestGroupSyncTable(t *testing.T) { }, { Name: "GroupNamesNoMapping", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", RegexFilter: regexp.MustCompile(".*"), AutoCreateMissing: false, @@ -194,7 +194,7 @@ func TestGroupSyncTable(t *testing.T) { }, { Name: "NoUser", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", Mapping: map[string][]uuid.UUID{ // Extra ID that does not map to a group @@ -219,7 +219,7 @@ func TestGroupSyncTable(t *testing.T) { }, { Name: "LegacyMapping", - Settings: &codersdk.GroupSyncSettings{ + GroupSettings: &codersdk.GroupSyncSettings{ Field: "groups", RegexFilter: regexp.MustCompile("^legacy"), LegacyNameMapping: map[string]string{ @@ -401,7 +401,7 @@ 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")}, @@ -747,11 +747,15 @@ func SetupOrganization(t *testing.T, s *idpsync.AGPLIDPSync, db database.Store, manager := runtimeconfig.NewManager() orgResolver := manager.OrganizationResolver(db, org.ID) - err = s.Group.SetRuntimeValue(context.Background(), orgResolver, def.GroupSettings) - require.NoError(t, err) + if def.GroupSettings != nil { + err = s.Group.SetRuntimeValue(context.Background(), orgResolver, (*idpsync.GroupSyncSettings)(def.GroupSettings)) + require.NoError(t, err) + } - err = s.Role.SetRuntimeValue(context.Background(), orgResolver, def.RoleSettings) - require.NoError(t, err) + if def.RoleSettings != nil { + err = s.Role.SetRuntimeValue(context.Background(), orgResolver, def.RoleSettings) + require.NoError(t, err) + } if !def.NotMember { dbgen.OrganizationMember(t, db, database.OrganizationMember{ @@ -822,7 +826,7 @@ type orgSetupDefinition struct { // NotMember if true will ensure the user is not a member of the organization. NotMember bool - GroupSettings *idpsync.GroupSyncSettings + GroupSettings *codersdk.GroupSyncSettings RoleSettings *idpsync.RoleSyncSettings assertGroups *orgGroupAssert From 88576606c62f9c0ce4f7301e21b3150119ffd860 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 Sep 2024 15:41:48 -0500 Subject: [PATCH 11/13] extract method into it's own funciton --- coderd/members.go | 57 ++++++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/coderd/members.go b/coderd/members.go index 52bf7284aaeee..59224f7833f82 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -216,31 +216,9 @@ 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 - } - } + // Check if changing roles is allowed + if !api.allowChangingMemberRoles(rw, ctx, member, organization) { + return } if apiKey.UserID == member.OrganizationMember.UserID { @@ -287,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(rw http.ResponseWriter, ctx context.Context, 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) { From 7ff0bb0d455c003f2c2a0e6cd749d16373b134d7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 Sep 2024 15:58:50 -0500 Subject: [PATCH 12/13] linting --- coderd/members.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/members.go b/coderd/members.go index 59224f7833f82..48006221efc59 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -217,7 +217,7 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { defer commitAudit() // Check if changing roles is allowed - if !api.allowChangingMemberRoles(rw, ctx, member, organization) { + if !api.allowChangingMemberRoles(ctx, rw, member, organization) { return } @@ -265,7 +265,7 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, resp[0]) } -func (api *API) allowChangingMemberRoles(rw http.ResponseWriter, ctx context.Context, member httpmw.OrganizationMember, organization database.Organization) bool { +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. From d8b0d45e9ddacafcc5bd7d35010fb7d1b75a87db Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 Sep 2024 16:34:27 -0500 Subject: [PATCH 13/13] Trigger Build