Skip to content

Commit 89b4bb5

Browse files
EmyrkEmyrk
committed
chore: remove resetting user's roles on misconfigured
1 parent f4678c1 commit 89b4bb5

File tree

4 files changed

+89
-36
lines changed

4 files changed

+89
-36
lines changed

‎coderd/idpsync/group.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func (s AGPLIDPSync) SyncGroups(ctx context.Context, db database.Store, user dat
8383
orgSettings := make(map[uuid.UUID]GroupSyncSettings)
8484
for orgID := range userOrgs {
8585
orgResolver := s.Manager.OrganizationResolver(tx, orgID)
86-
settings, err := s.SyncSettings.Group.Resolve(ctx, orgResolver)
86+
settings, err := s.GroupSyncSettings().Resolve(ctx, orgResolver)
8787
if err != nil {
8888
if !xerrors.Is(err, runtimeconfig.ErrEntryNotFound) {
8989
return xerrors.Errorf("resolve group sync settings: %w", err)

‎coderd/idpsync/group_test.go

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -744,6 +744,7 @@ func SetupOrganization(t *testing.T, s *idpsync.AGPLIDPSync, db database.Store,
744744
OrganizationID: org.ID,
745745
})
746746
}
747+
747748
if len(def.OrganizationRoles) > 0 {
748749
_, err := db.UpdateMemberRoles(context.Background(), database.UpdateMemberRolesParams{
749750
GrantedRoles: def.OrganizationRoles,
@@ -752,6 +753,24 @@ func SetupOrganization(t *testing.T, s *idpsync.AGPLIDPSync, db database.Store,
752753
})
753754
require.NoError(t, err)
754755
}
756+
757+
if len(def.CustomRoles) > 0 {
758+
for _, cr := range def.CustomRoles {
759+
_, err := db.InsertCustomRole(context.Background(), database.InsertCustomRoleParams{
760+
Name: cr,
761+
DisplayName: cr,
762+
OrganizationID: uuid.NullUUID{
763+
UUID: org.ID,
764+
Valid: true,
765+
},
766+
SitePermissions: nil,
767+
OrgPermissions: nil,
768+
UserPermissions: nil,
769+
})
770+
require.NoError(t, err)
771+
}
772+
}
773+
755774
for groupID, in := range def.Groups {
756775
dbgen.Group(t, db, database.Group{
757776
ID: groupID,
@@ -781,10 +800,10 @@ func SetupOrganization(t *testing.T, s *idpsync.AGPLIDPSync, db database.Store,
781800
type orgSetupDefinition struct {
782801
Name string
783802
// True if the user is a member of the group
784-
Groups map[uuid.UUID]bool
785-
GroupNames map[string]bool
786-
803+
Groups map[uuid.UUID]bool
804+
GroupNames map[string]bool
787805
OrganizationRoles []string
806+
CustomRoles []string
788807
// NotMember if true will ensure the user is not a member of the organization.
789808
NotMember bool
790809

@@ -827,7 +846,8 @@ func (o orgSetupDefinition) Assert(t *testing.T, orgID uuid.UUID, db database.St
827846
o.assertRoles.Assert(t, orgID, db, o.NotMember, user)
828847
}
829848

830-
if o.assertGroups == nil && o.assertRoles == nil {
849+
// If the user is not a member, there is nothing to really assert in the org
850+
if o.assertGroups == nil && o.assertRoles == nil && !o.NotMember {
831851
t.Errorf("no group or role asserts present, must have at least one")
832852
t.FailNow()
833853
}

‎coderd/idpsync/role.go

Lines changed: 10 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,16 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data
9898
slog.Error(err),
9999
)
100100

101-
// Failing role sync should reset a user's roles.
102-
expectedRoles[orgID] = []rbac.RoleIdentifier{}
101+
// TODO: If rolesync fails, we might want to reset a user's
102+
// roles to prevent stale roles from existing.
103+
// Eg: `expectedRoles[orgID] = []rbac.RoleIdentifier{}`
104+
// However, implementing this could lock an org admin out
105+
// of fixing their configuration.
106+
// There is also no current method to notify an org admin of
107+
// a configuration issue.
108+
// So until org admins can be notified of configuration issues,
109+
// and they will not be locked out, this code will do nothing to
110+
// the user's roles.
103111

104112
// Do not return an error, because that would prevent a user
105113
// from logging in. A misconfigured organization should not
@@ -172,28 +180,6 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data
172180
return nil
173181
}
174182

175-
// resetUserOrgRoles will reset the user's roles for a specific organization.
176-
// It does not remove them as a member from the organization.
177-
func (s AGPLIDPSync) resetUserOrgRoles(ctx context.Context, tx database.Store, member database.OrganizationMembersRow, orgID uuid.UUID) error {
178-
withoutMember := slices.DeleteFunc(member.OrganizationMember.Roles, func(s string) bool {
179-
return s == rbac.RoleOrgMember()
180-
})
181-
// If the user has no roles, then skip doing any database request.
182-
if len(withoutMember) == 0 {
183-
return nil
184-
}
185-
186-
_, err := tx.UpdateMemberRoles(ctx, database.UpdateMemberRolesParams{
187-
GrantedRoles: []string{},
188-
UserID: member.OrganizationMember.UserID,
189-
OrgID: orgID,
190-
})
191-
if err != nil {
192-
return xerrors.Errorf("zero out member roles(%s): %w", member.OrganizationMember.UserID.String(), err)
193-
}
194-
return nil
195-
}
196-
197183
func (s AGPLIDPSync) syncSiteWideRoles(ctx context.Context, tx database.Store, user database.User, params RoleParams) error {
198184
// Apply site wide roles to a user.
199185
// ignored is the list of roles that are not valid Coder roles and will

‎coderd/idpsync/role_test.go

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ func TestRoleSyncTable(t *testing.T) {
3131
"create-bar", "create-baz",
3232
"legacy-bar", rbac.RoleOrgAuditor(),
3333
},
34+
// bad-claim is a number, and will fail any role sync
35+
"bad-claim": 100,
3436
}
3537

3638
//ids := coderdtest.NewDeterministicUUIDGenerator()
@@ -43,41 +45,83 @@ func TestRoleSyncTable(t *testing.T) {
4345
},
4446
},
4547
{
46-
Name: "NoSyncNoChange",
48+
Name: "SyncDisabled",
4749
OrganizationRoles: []string{
4850
rbac.RoleOrgAdmin(),
4951
},
52+
RoleSettings: &idpsync.RoleSyncSettings{},
5053
assertRoles: &orgRoleAssert{
5154
ExpectedOrgRoles: []string{
5255
rbac.RoleOrgAdmin(),
5356
},
5457
},
5558
},
5659
{
57-
Name: "NoChange",
60+
// Audit role from claim
61+
Name: "RawAudit",
5862
OrganizationRoles: []string{
5963
rbac.RoleOrgAdmin(),
6064
},
61-
RoleSettings: &idpsync.RoleSyncSettings{},
65+
RoleSettings: &idpsync.RoleSyncSettings{
66+
Field: "roles",
67+
Mapping: map[string][]string{},
68+
},
6269
assertRoles: &orgRoleAssert{
6370
ExpectedOrgRoles: []string{
64-
rbac.RoleOrgAdmin(),
71+
rbac.RoleOrgAuditor(),
6572
},
6673
},
6774
},
6875
{
69-
// Audit role from claim
70-
Name: "RawAudit",
76+
Name: "CustomRole",
7177
OrganizationRoles: []string{
7278
rbac.RoleOrgAdmin(),
7379
},
80+
CustomRoles: []string{"foo"},
7481
RoleSettings: &idpsync.RoleSyncSettings{
7582
Field: "roles",
7683
Mapping: map[string][]string{},
7784
},
7885
assertRoles: &orgRoleAssert{
7986
ExpectedOrgRoles: []string{
8087
rbac.RoleOrgAuditor(),
88+
"foo",
89+
},
90+
},
91+
},
92+
{
93+
Name: "RoleMapping",
94+
OrganizationRoles: []string{
95+
rbac.RoleOrgAdmin(),
96+
"invalid", // Throw in an extra invalid role that will be removed
97+
},
98+
CustomRoles: []string{"custom"},
99+
RoleSettings: &idpsync.RoleSyncSettings{
100+
Field: "roles",
101+
Mapping: map[string][]string{
102+
"foo": {"custom", rbac.RoleOrgTemplateAdmin()},
103+
},
104+
},
105+
assertRoles: &orgRoleAssert{
106+
ExpectedOrgRoles: []string{
107+
rbac.RoleOrgAuditor(),
108+
rbac.RoleOrgTemplateAdmin(),
109+
"custom",
110+
},
111+
},
112+
},
113+
{
114+
// InvalidClaims will log an error, but do not block authentication.
115+
// This is to prevent a misconfigured organization from blocking
116+
// a user from authenticating.
117+
Name: "InvalidClaim",
118+
OrganizationRoles: []string{rbac.RoleOrgAdmin()},
119+
RoleSettings: &idpsync.RoleSyncSettings{
120+
Field: "bad-claim",
121+
},
122+
assertRoles: &orgRoleAssert{
123+
ExpectedOrgRoles: []string{
124+
rbac.RoleOrgAdmin(),
81125
},
82126
},
83127
},
@@ -90,7 +134,9 @@ func TestRoleSyncTable(t *testing.T) {
90134

91135
db, _ := dbtestutil.NewDB(t)
92136
manager := runtimeconfig.NewManager()
93-
s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}),
137+
s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{
138+
IgnoreErrors: true,
139+
}),
94140
manager,
95141
idpsync.DeploymentSyncSettings{
96142
SiteRoleField: "roles",
@@ -105,6 +151,7 @@ func TestRoleSyncTable(t *testing.T) {
105151
// Do the group sync!
106152
err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{
107153
SyncEnabled: true,
154+
SyncSiteWide: false,
108155
MergedClaims: userClaims,
109156
})
110157
require.NoError(t, err)

0 commit comments

Comments
 (0)