add trigger to delete role from organization members on delete

Emyrk · Emyrk · commit cff829b5dc5b · 2024-08-07T12:20:32.000-05:00
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -1397,6 +1397,16 @@ func (q *FakeQuerier) DeleteCustomRole(_ context.Context, arg database.DeleteCus
 	if initial == len(q.data.customRoles) {
 		return sql.ErrNoRows
 	}
+
+	// Emulate the trigger 'remove_organization_member_custom_role'
+	for i, mem := range q.organizationMembers {
+		if mem.OrganizationID == arg.OrganizationID.UUID {
+			mem.Roles = slices.DeleteFunc(mem.Roles, func(role string) bool {
+				return role == arg.Name
+			})
+			q.organizationMembers[i] = mem
+		}
+	}
 	return nil
 }
 
diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000240_delete_user_roles.down.sql b/coderd/database/migrations/000240_delete_user_roles.down.sql
@@ -0,0 +1,2 @@
+DROP TRIGGER IF EXISTS remove_organization_member_custom_role ON custom_roles;
+DROP FUNCTION IF EXISTS remove_organization_member_role;
diff --git a/coderd/database/migrations/000240_delete_user_roles.up.sql b/coderd/database/migrations/000240_delete_user_roles.up.sql
@@ -0,0 +1,29 @@
+-- When a custom role is deleted, we need to remove the assigned role
+-- from all organization members that have it.
+-- This action cannot be reverted, so deleting a custom role should be
+-- done with caution.
+CREATE OR REPLACE FUNCTION remove_organization_member_role()
+	RETURNS TRIGGER AS
+$$
+BEGIN
+	-- Delete the role from all organization members that have it.
+	-- TODO: When site wide custom roles are supported, if the
+	--	organization_id is null, we should remove the role from the 'users'
+	--	table instead.
+	IF OLD.organization_id IS NOT NULL THEN
+		UPDATE organization_members
+		-- this is a noop if the role is not assigned to the member
+		SET roles = array_remove(roles, OLD.name)
+		WHERE
+			-- Scope to the correct organization
+			organization_members.organization_id = OLD.organization_id;
+	END IF;
+	RETURN OLD;
+END;
+$$ LANGUAGE plpgsql;
+
+
+-- Attach the function to deleting the custom role
+CREATE TRIGGER remove_organization_member_custom_role
+	BEFORE DELETE ON custom_roles FOR EACH ROW
+	EXECUTE PROCEDURE remove_organization_member_role();
diff --git a/enterprise/coderd/roles_test.go b/enterprise/coderd/roles_test.go
@@ -326,13 +326,20 @@ func TestCustomOrganizationRole(t *testing.T) {
 			},
 		})
 
-		orgAdmin, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.ScopedRoleOrgAdmin(first.OrganizationID))
+		orgAdmin, orgAdminUser := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.ScopedRoleOrgAdmin(first.OrganizationID))
 		ctx := testutil.Context(t, testutil.WaitMedium)
 
-		//nolint:gocritic // owner is required for this
-		createdRole, err := orgAdmin.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(first.OrganizationID))
+		createdRole, err := orgAdmin.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
 		require.NoError(t, err, "upsert role")
 
+		//nolint:gocritic // org_admin cannot assign to themselves
+		_, err = owner.UpdateOrganizationMemberRoles(ctx, first.OrganizationID, orgAdminUser.ID.String(), codersdk.UpdateRoles{
+			// Give the user this custom role, to ensure when it is deleted, the user
+			// is ok to be used.
+			Roles: []string{createdRole.Name, rbac.ScopedRoleOrgAdmin(first.OrganizationID).Name},
+		})
+		require.NoError(t, err, "assign custom role to user")
+
 		existingRoles, err := orgAdmin.ListOrganizationRoles(ctx, first.OrganizationID)
 		require.NoError(t, err)
 
@@ -352,6 +359,75 @@ func TestCustomOrganizationRole(t *testing.T) {
 			return role.Name == createdRole.Name
 		})
 		require.False(t, exists, "custom role should be deleted")
+
+		// Verify you can still assign roles.
+		// There used to be a bug that if a member had a delete role, they
+		// could not be assigned roles anymore.
+		//nolint:gocritic // org_admin cannot assign to themselves
+		_, err = owner.UpdateOrganizationMemberRoles(ctx, first.OrganizationID, orgAdminUser.ID.String(), codersdk.UpdateRoles{
+			Roles: []string{rbac.ScopedRoleOrgAdmin(first.OrganizationID).Name},
+		})
+		require.NoError(t, err)
+	})
+
+	// Verify deleting a custom role cascades to all members
+	t.Run("DeleteRoleCascadeMembers", func(t *testing.T) {
+		t.Parallel()
+		dv := coderdtest.DeploymentValues(t)
+		dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)}
+		owner, first := coderdenttest.New(t, &coderdenttest.Options{
+			Options: &coderdtest.Options{
+				DeploymentValues: dv,
+			},
+			LicenseOptions: &coderdenttest.LicenseOptions{
+				Features: license.Features{
+					codersdk.FeatureCustomRoles: 1,
+				},
+			},
+		})
+
+		orgAdmin, orgAdminUser := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.ScopedRoleOrgAdmin(first.OrganizationID))
+		ctx := testutil.Context(t, testutil.WaitMedium)
+
+		createdRole, err := orgAdmin.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
+		require.NoError(t, err, "upsert role")
+
+		customRoleIdentifier := rbac.RoleIdentifier{
+			Name:           createdRole.Name,
+			OrganizationID: first.OrganizationID,
+		}
+
+		// Create a few members with the role
+		coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, customRoleIdentifier)
+		coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.ScopedRoleOrgAdmin(first.OrganizationID), customRoleIdentifier)
+		coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.ScopedRoleOrgTemplateAdmin(first.OrganizationID), rbac.ScopedRoleOrgAuditor(first.OrganizationID), customRoleIdentifier)
+
+		// Verify members have the custom role
+		members, err := orgAdmin.OrganizationMembers(ctx, first.OrganizationID)
+		require.NoError(t, err)
+		require.Len(t, members, 5) // 3 members + org admin + owner
+		for _, member := range members {
+			if member.UserID == orgAdminUser.ID || member.UserID == first.UserID {
+				continue
+			}
+
+			require.True(t, slices.ContainsFunc(member.Roles, func(role codersdk.SlimRole) bool {
+				return role.Name == customRoleIdentifier.Name
+			}), "member should have custom role")
+		}
+
+		err = orgAdmin.DeleteOrganizationRole(ctx, first.OrganizationID, createdRole.Name)
+		require.NoError(t, err)
+
+		// Verify the role was removed from all members
+		members, err = orgAdmin.OrganizationMembers(ctx, first.OrganizationID)
+		require.NoError(t, err)
+		require.Len(t, members, 5) // 3 members + org admin + owner
+		for _, member := range members {
+			require.False(t, slices.ContainsFunc(member.Roles, func(role codersdk.SlimRole) bool {
+				return role.Name == customRoleIdentifier.Name
+			}), "role should be removed from all users")
+		}
 	})
 }
 

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+DROP TRIGGER IF EXISTS remove_organization_member_custom_role ON custom_roles;`
	`2`	`+DROP FUNCTION IF EXISTS remove_organization_member_role;`