chore: remove resetting user's roles on misconfigured

coder · Emyrk · Sep 17, 2024 · Sep 10, 2024 · Sep 10, 2024 · Sep 10, 2024
commit b1ece73db3379095a4199bb7351e91b4f333872d
diff --git 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
@@ -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
@@ -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,41 +45,83 @@ func TestRoleSyncTable(t *testing.T) {
 			},
 		},
 		{
-			Name: "NoSyncNoChange",
+			Name: "SyncDisabled",
 			OrganizationRoles: []string{
 				rbac.RoleOrgAdmin(),
 			},
+			RoleSettings: &idpsync.RoleSyncSettings{},
 			assertRoles: &orgRoleAssert{
 				ExpectedOrgRoles: []string{
 					rbac.RoleOrgAdmin(),
 				},
 			},
 		},
 		{
-			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{},
 			},
 			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)