diff --git a/core/role/service.go b/core/role/service.go index 5f1321afe7..6fd1b28515 100644 --- a/core/role/service.go +++ b/core/role/service.go @@ -299,16 +299,16 @@ func (s Service) Delete(ctx context.Context, id string) error { return err } - // 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). + // Mark the row deleted first. The repository refuses with ErrRoleInUse while + // any live policy still references the role, 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:#@...) + // row is marked deleted → remove the role's permission tuples (app/role:#@...) return s.relationService.Delete(ctx, relation.Relation{Object: relation.Object{ ID: roleToDelete.ID, Namespace: schema.RoleNamespace, diff --git a/core/role/service_test.go b/core/role/service_test.go index 58c93a3594..24957204a5 100644 --- a/core/role/service_test.go +++ b/core/role/service_test.go @@ -79,7 +79,7 @@ func Test_Delete(t *testing.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 + // a live policy still references the role -> ErrRoleInUse mockRepository.On("Delete", mock.Anything, "role-1").Return(role.ErrRoleInUse).Once() svc := role.NewService(mockRepository, mockRelationSvc, mockPermissionSvc, mockAuditRecordRepo, nil) diff --git a/internal/store/postgres/role_repository.go b/internal/store/postgres/role_repository.go index eab0a93505..5e4ae14bae 100644 --- a/internal/store/postgres/role_repository.go +++ b/internal/store/postgres/role_repository.go @@ -2,6 +2,7 @@ package postgres import ( "context" + "database/sql" "encoding/json" "errors" "fmt" @@ -14,9 +15,9 @@ import ( "github.com/google/uuid" - "database/sql" - "github.com/doug-martin/goqu/v9" + "github.com/doug-martin/goqu/v9/exp" + "github.com/jmoiron/sqlx" "github.com/raystack/frontier/core/namespace" "github.com/raystack/frontier/core/role" "github.com/raystack/frontier/pkg/db" @@ -259,27 +260,62 @@ func (r RoleRepository) Update(ctx context.Context, rl role.Role) (role.Role, er } func (r RoleRepository) Delete(ctx context.Context, id string) error { - query, params, err := dialect.Delete(TABLE_ROLES).Where( - goqu.Ex{ - "id": id, - }, - ).Returning(&Role{}).ToSQL() + lockQuery, lockParams, err := fromLive(TABLE_ROLES). + Select("id"). + Where(goqu.Ex{"id": id}). + ForUpdate(exp.Wait). + ToSQL() if err != nil { return fmt.Errorf("%w: %s", errQuery, err) } - var roleModel Role - if err = r.dbc.WithTimeout(ctx, TABLE_ROLES, "Delete", func(ctx context.Context) error { - return r.dbc.QueryRowxContext(ctx, query, params...).StructScan(&roleModel) + livePolicy := fromLive(TABLE_POLICIES). + Select(goqu.L("1")). + Where(goqu.I(TABLE_POLICIES + ".role_id").Eq(goqu.I(TABLE_ROLES + ".id"))) + + deleteQuery, deleteParams, err := softDelete(TABLE_ROLES).Where( + goqu.Ex{"id": id}, + goqu.L("NOT EXISTS ?", livePolicy), + ).ToSQL() + if err != nil { + return fmt.Errorf("%w: %s", errQuery, err) + } + + // One transaction, two statements. The first locks the live role row the + // way a hard DELETE would. The second marks it deleted, unless a live + // policy still points at it. + if err = r.dbc.WithTxn(ctx, sql.TxOptions{}, func(tx *sqlx.Tx) error { + return r.dbc.WithTimeout(ctx, TABLE_ROLES, "Delete", func(ctx context.Context) error { + // Lock the role row first. A policy insert holds a lock on the role + // row until it commits, so this waits for it and the check below sees that policy. + var lockedID string + if err := tx.QueryRowContext(ctx, lockQuery, lockParams...).Scan(&lockedID); err != nil { + return err + } + + result, err := tx.ExecContext(ctx, deleteQuery, deleteParams...) + if err != nil { + return err + } + deleted, err := result.RowsAffected() + if err != nil { + return err + } + if deleted == 0 { + return role.ErrRoleInUse + } + return nil + }) }); err != nil { + // WithTxn wraps every error it rolled back on as "rollback: ...". Here + // the rollback is the normal path, so return the plain sentinel. + if errors.Is(err, role.ErrRoleInUse) { + return role.ErrRoleInUse + } err = checkPostgresError(err) 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 } diff --git a/internal/store/postgres/role_repository_test.go b/internal/store/postgres/role_repository_test.go index a30188bcfd..09923f7a1d 100644 --- a/internal/store/postgres/role_repository_test.go +++ b/internal/store/postgres/role_repository_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/raystack/frontier/internal/bootstrap/schema" @@ -422,6 +423,71 @@ func (s *RoleRepositoryTestSuite) TestDelete() { } }) } + + s.Run("should keep the row and mark it deleted", func() { + target := s.roles[1] + s.Require().NoError(s.repository.Delete(s.ctx, target.ID)) + + var deleted bool + err := s.client.QueryRowxContext(s.ctx, "SELECT deleted_at IS NOT NULL FROM roles WHERE id = $1", target.ID).Scan(&deleted) + s.Require().NoError(err) + s.Assert().True(deleted) + }) + + s.Run("should return not found when the role is already deleted", func() { + target := s.roles[2] + _, err := s.client.ExecContext(s.ctx, "UPDATE roles SET deleted_at = now() WHERE id = $1", target.ID) + s.Require().NoError(err) + + s.Assert().ErrorIs(s.repository.Delete(s.ctx, target.ID), role.ErrNotExist) + }) + + s.Run("should return in use and keep the role when a live policy uses it", func() { + target := s.roles[3] + _, err := bootstrapPolicy(s.client, s.orgID, target, uuid.NewString()) + s.Require().NoError(err) + + s.Assert().ErrorIs(s.repository.Delete(s.ctx, target.ID), role.ErrRoleInUse) + _, err = s.repository.Get(s.ctx, target.ID) + s.Assert().NoError(err) + }) + + s.Run("should delete the role when only deleted policies use it", func() { + target := s.roles[4] + _, err := bootstrapPolicy(s.client, s.orgID, target, uuid.NewString()) + s.Require().NoError(err) + _, err = s.client.ExecContext(s.ctx, "UPDATE policies SET deleted_at = now() WHERE role_id = $1", target.ID) + s.Require().NoError(err) + + s.Assert().NoError(s.repository.Delete(s.ctx, target.ID)) + }) + + s.Run("should wait for a policy insert that has not committed and return in use", func() { + target, err := s.repository.Upsert(s.ctx, role.Role{ + Name: "role with a running policy insert", + OrgID: s.orgID, + Metadata: metadata.Metadata{}, + }) + s.Require().NoError(err) + tx, err := s.client.BeginTxx(s.ctx, nil) + s.Require().NoError(err) + defer tx.Rollback() // nolint + _, err = tx.ExecContext(s.ctx, "INSERT INTO policies (role_id, resource_id, resource_type, principal_id, principal_type) VALUES ($1, $2, 'ns1', $3, 'app/user')", + target.ID, s.orgID, uuid.NewString()) + s.Require().NoError(err) + + done := make(chan error, 1) + go func() { done <- s.repository.Delete(s.ctx, target.ID) }() + + s.Require().Eventually(func() bool { + var waiting int + err := s.client.QueryRowxContext(s.ctx, "SELECT count(*) FROM pg_stat_activity WHERE datname = current_database() AND wait_event_type = 'Lock'").Scan(&waiting) + return err == nil && waiting == 1 + }, time.Second, 10*time.Millisecond, "the delete did not wait for the policy insert") + + s.Require().NoError(tx.Commit()) + s.Assert().ErrorIs(<-done, role.ErrRoleInUse) + }) } func (s *RoleRepositoryTestSuite) TestGetByName() { diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index 3f72a656e3..4a0f77aa7f 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -3092,6 +3092,39 @@ func (s *APIRegressionTestSuite) TestOrganizationRoleDeleteInUse() { // after a successful delete, no role->permission tuples should linger s.Assert().False(roleHasPermTuples()) + + // the deleted role is hidden from reads and cannot be deleted twice + _, err = s.testBench.Client.GetOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetOrganizationRoleRequest{ + OrgId: orgID, + Id: roleID, + })) + s.Require().Error(err) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + _, err = s.testBench.Client.DeleteOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRoleRequest{ + OrgId: orgID, + Id: roleID, + })) + s.Require().Error(err) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + + // the name is free again and lands on a new row + recreateResp, 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) + s.Assert().NotEqual(roleID, recreateResp.Msg.GetRole().GetId()) + + // the org now holds a live role and a deleted one; the delete still goes through + _, err = s.testBench.Client.DeleteOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRequest{ + Id: orgID, + })) + s.Require().NoError(err) } func TestEndToEndAPIRegressionTestSuite(t *testing.T) {