fix(role): reject deletion of a role still in use by policies

core/role/errors.go

@@ -7,4 +7,5 @@ var (
 	ErrInvalidID     = errors.New("role id is invalid")
 	ErrConflict      = errors.New("role name already exist")
 	ErrInvalidDetail = errors.New("invalid role detail")
+	ErrRoleInUse     = errors.New("role is in use by one or more policies")
 )

core/role/service.go

@@ -280,11 +280,24 @@ func (s Service) Update(ctx context.Context, toUpdate Role) (Role, error) {
 }
 
 func (s Service) Delete(ctx context.Context, id string) error {
-	if err := s.relationService.Delete(ctx, relation.Relation{Object: relation.Object{
-		ID:        id,
-		Namespace: schema.RoleNamespace,
-	}}); err != nil {
+	// resolve name->ID so the row delete and tuple cleanup target the same role
+	roleToDelete, err := s.Get(ctx, id)
+	if err != nil {
 		return err
 	}
-	return s.repository.Delete(ctx, id)
+
+	// Delete the row first. The policies.role_id foreign key rejects this while
+	// any policy still references the role, surfaced as ErrRoleInUse — so a role that
+	// is still in use is left fully intact (no SpiceDB tuples are touched below).
+	// This is intentionally a guard, not a cascade: removing a role would revoke
+	// access for everyone granted it, so the caller must drop those policies first.
+	if err := s.repository.Delete(ctx, roleToDelete.ID); err != nil {
+		return err
+	}
+
+	// row is gone → remove the role's permission tuples (app/role:<id>#<perm>@...)
+	return s.relationService.Delete(ctx, relation.Relation{Object: relation.Object{
+		ID:        roleToDelete.ID,
+		Namespace: schema.RoleNamespace,
+	}})
 }

core/role/service_test.go

@@ -70,6 +70,46 @@ func Test_Get(t *testing.T) {
 	})
 }
 
+func Test_Delete(t *testing.T) {
+	t.Run("returns ErrRoleInUse and leaves SpiceDB untouched when a policy references the role", func(t *testing.T) {
+		mockRepository := mocks.NewRepository(t)
+		mockRelationSvc := mocks.NewRelationService(t)
+		mockPermissionSvc := mocks.NewPermissionService(t)
+		mockAuditRecordRepo := auditMocks.NewRepository(t)
+
+		mockID := uuid.New().String()
+		mockRepository.On("Get", mock.Anything, mockID).Return(role.Role{ID: "role-1"}, nil).Once()
+		// the policies.role_id FK rejects the row delete -> ErrRoleInUse
+		mockRepository.On("Delete", mock.Anything, "role-1").Return(role.ErrRoleInUse).Once()
+
+		svc := role.NewService(mockRepository, mockRelationSvc, mockPermissionSvc, mockAuditRecordRepo, nil)
+		err := svc.Delete(context.Background(), mockID)
+
+		assert.ErrorIs(t, err, role.ErrRoleInUse)
+		// no permission tuples should be removed when the delete is rejected
+		mockRelationSvc.AssertNotCalled(t, "Delete")
+	})
+
+	t.Run("removes the role's permission tuples after the row is deleted", func(t *testing.T) {
+		mockRepository := mocks.NewRepository(t)
+		mockRelationSvc := mocks.NewRelationService(t)
+		mockPermissionSvc := mocks.NewPermissionService(t)
+		mockAuditRecordRepo := auditMocks.NewRepository(t)
+
+		mockID := uuid.New().String()
+		mockRepository.On("Get", mock.Anything, mockID).Return(role.Role{ID: "role-1"}, nil).Once()
+		mockRepository.On("Delete", mock.Anything, "role-1").Return(nil).Once()
+		mockRelationSvc.On("Delete", mock.Anything, relation.Relation{
+			Object: relation.Object{ID: "role-1", Namespace: schema.RoleNamespace},
+		}).Return(nil).Once()
+
+		svc := role.NewService(mockRepository, mockRelationSvc, mockPermissionSvc, mockAuditRecordRepo, nil)
+		err := svc.Delete(context.Background(), mockID)
+
+		assert.NoError(t, err)
+	})
+}
+
 func Test_List(t *testing.T) {
 	mockRepository := mocks.NewRepository(t)
 	mockRelationSvc := mocks.NewRelationService(t)

internal/api/v1beta1connect/role.go

@@ -416,6 +416,8 @@ func (h *ConnectHandler) DeleteOrganizationRole(ctx context.Context, request *co
 		switch {
 		case errors.Is(err, role.ErrNotExist), errors.Is(err, role.ErrInvalidID):
 			return nil, connect.NewError(connect.CodeNotFound, ErrRoleNotFound)
+		case errors.Is(err, role.ErrRoleInUse):
+			return nil, connect.NewError(connect.CodeFailedPrecondition, role.ErrRoleInUse)
 		default:
 			errorLogger.LogUnexpectedError(ctx, request, "DeleteOrganizationRole", err,
 				"role_id", roleID,
@@ -448,6 +450,8 @@ func (h *ConnectHandler) DeleteRole(ctx context.Context, request *connect.Reques
 		switch {
 		case errors.Is(err, role.ErrNotExist), errors.Is(err, role.ErrInvalidID):
 			return nil, connect.NewError(connect.CodeNotFound, ErrNotFound)
+		case errors.Is(err, role.ErrRoleInUse):
+			return nil, connect.NewError(connect.CodeFailedPrecondition, role.ErrRoleInUse)
 		default:
 			errorLogger.LogUnexpectedError(ctx, request, "DeleteRole", err,
 				"role_id", roleID)

internal/store/postgres/role_repository.go

@@ -274,6 +274,10 @@ func (r RoleRepository) Delete(ctx context.Context, id string) error {
 		switch {
 		case errors.Is(err, sql.ErrNoRows):
 			return role.ErrNotExist
+		case errors.Is(err, ErrForeignKeyViolation):
+			// policies.role_id references roles(id) with no ON DELETE rule, so a
+			// role still bound to any policy cannot be deleted.
+			return role.ErrRoleInUse
 		default:
 			return err
 		}

test/e2e/regression/api_test.go

@@ -2796,6 +2796,104 @@ func (s *APIRegressionTestSuite) TestWebhookAPI() {
 	})
 }
 
+// TestOrganizationRoleDeleteInUse asserts that a role still referenced by a
+// policy cannot be deleted (FailedPrecondition), nothing is torn down while it's
+// in use, and once the policy is removed the role deletes cleanly with no
+// leftover permission tuples (gap #1661.1).
+func (s *APIRegressionTestSuite) TestOrganizationRoleDeleteInUse() {
+	ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie)
+
+	createOrgResp, err := s.testBench.Client.CreateOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{
+		Body: &frontierv1beta1.OrganizationRequestBody{
+			Name: "org-role-delete-in-use",
+		},
+	}))
+	s.Require().NoError(err)
+	orgID := createOrgResp.Msg.GetOrganization().GetId()
+
+	createRoleResp, err := s.testBench.Client.CreateOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRoleRequest{
+		OrgId: orgID,
+		Body: &frontierv1beta1.RoleRequestBody{
+			Title:       "in use role",
+			Name:        "in_use_role",
+			Scopes:      []string{"app/organization"},
+			Permissions: []string{"app.organization.grouplist"},
+		},
+	}))
+	s.Require().NoError(err)
+	roleID := createRoleResp.Msg.GetRole().GetId()
+
+	createUserResp, err := s.testBench.Client.CreateUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateUserRequest{
+		Body: &frontierv1beta1.UserRequestBody{
+			Title: "in use role user",
+			Email: "user-role-delete-in-use@raystack.org",
+			Name:  "user_role_delete_in_use",
+		},
+	}))
+	s.Require().NoError(err)
+
+	createPolicyResp, err := s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{
+		Body: &frontierv1beta1.PolicyRequestBody{
+			RoleId:    roleID,
+			Resource:  schema.JoinNamespaceAndResourceID(schema.OrganizationNamespace, orgID),
+			Principal: schema.JoinNamespaceAndResourceID(schema.UserPrincipal, createUserResp.Msg.GetUser().GetId()),
+		},
+	}))
+	s.Require().NoError(err)
+	policyID := createPolicyResp.Msg.GetPolicy().GetId()
+
+	roleHasPermTuples := func() bool {
+		resp, err := s.testBench.AdminClient.ListRelations(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListRelationsRequest{
+			Object: schema.JoinNamespaceAndResourceID(schema.RoleNamespace, roleID),
+		}))
+		s.Require().NoError(err)
+		return len(resp.Msg.GetRelations()) > 0
+	}
+	rolebindingHasTuples := func() bool {
+		resp, err := s.testBench.AdminClient.ListRelations(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListRelationsRequest{
+			Object: schema.JoinNamespaceAndResourceID(schema.RoleBindingNamespace, policyID),
+		}))
+		s.Require().NoError(err)
+		return len(resp.Msg.GetRelations()) > 0
+	}
+
+	// the policy wrote rolebinding tuples (incl. the #role link to this role)
+	s.Assert().True(rolebindingHasTuples())
+
+	// deleting a role that is still referenced by a policy must be rejected
+	_, err = s.testBench.Client.DeleteOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRoleRequest{
+		OrgId: orgID,
+		Id:    roleID,
+	}))
+	s.Require().Error(err)
+	s.Assert().Equal(connect.CodeFailedPrecondition, connect.CodeOf(err))
+
+	// nothing should have been torn down: role still exists and its perm tuples remain
+	_, err = s.testBench.Client.GetOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetOrganizationRoleRequest{
+		OrgId: orgID,
+		Id:    roleID,
+	}))
+	s.Assert().NoError(err)
+	s.Assert().True(roleHasPermTuples())
+
+	// remove the policy: this clears its rolebinding tuples (incl. the #role link)
+	_, err = s.testBench.Client.DeletePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeletePolicyRequest{
+		Id: policyID,
+	}))
+	s.Require().NoError(err)
+	s.Assert().False(rolebindingHasTuples())
+
+	// with no policy referencing it, the role now deletes cleanly
+	_, err = s.testBench.Client.DeleteOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRoleRequest{
+		OrgId: orgID,
+		Id:    roleID,
+	}))
+	s.Require().NoError(err)
+
+	// after a successful delete, no role->permission tuples should linger
+	s.Assert().False(roleHasPermTuples())
+}
+
 func TestEndToEndAPIRegressionTestSuite(t *testing.T) {
 	suite.Run(t, new(APIRegressionTestSuite))
 }