diff --git a/internal/integration_tests/delete_user_cascade_test.go b/internal/integration_tests/delete_user_cascade_test.go new file mode 100644 index 000000000..b0c1ff96b --- /dev/null +++ b/internal/integration_tests/delete_user_cascade_test.go @@ -0,0 +1,111 @@ +package integration_tests + +import ( + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/authorization/engine" + "github.com/authorizerdev/authorizer/internal/graph/model" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// TestDeleteUserCascade proves a hard delete (_delete_user) removes every piece +// of state the account owned: the six user-keyed tables and its FGA grants. +// +// Before this, only sessions were cascaded. The federated-identity orphan was +// the worst of it — it points at a dead user id, jitProvisionFederatedUser fails +// closed on that branch, and the unique (org_id, issuer, subject) triple blocks +// re-provisioning, so the SSO principal is locked out permanently. +func TestDeleteUserCascade(t *testing.T) { + cfg := getTestConfig() + ts, eng := initFGATestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "delete_cascade_" + uuid.New().String() + "@authorizer.dev" + password := "Password@123" + signupRes, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, Password: password, ConfirmPassword: password, + }) + require.NoError(t, err) + require.NotNil(t, signupRes.User) + userID := signupRes.User.ID + + sp := ts.StorageProvider + orgID := "org_" + uuid.New().String() + issuer := "https://idp.example.com" + subject := "upstream_" + uuid.New().String() + + require.NoError(t, sp.AddSession(ctx, &schemas.Session{UserID: userID})) + _, err = sp.AddFederatedIdentity(ctx, &schemas.FederatedIdentity{ + OrgID: orgID, Issuer: issuer, Subject: subject, UserID: userID, + }) + require.NoError(t, err) + _, err = sp.AddOrgMembership(ctx, &schemas.OrgMembership{OrgID: orgID, UserID: userID, Roles: "member"}) + require.NoError(t, err) + _, err = sp.AddAuthenticator(ctx, &schemas.Authenticator{UserID: userID, Method: "totp", Secret: "s3cret"}) + require.NoError(t, err) + _, err = sp.AddWebauthnCredential(ctx, &schemas.WebauthnCredential{ + UserID: userID, CredentialID: "cred_" + uuid.New().String(), PublicKey: "pk", Name: "laptop", + }) + require.NoError(t, err) + require.NoError(t, sp.AddSessionToken(ctx, &schemas.SessionToken{UserID: userID, KeyName: "access", Token: "t"})) + require.NoError(t, sp.AddMFASession(ctx, &schemas.MFASession{UserID: userID, KeyName: "mfa"})) + + // An FGA grant held by this user, which lives outside StorageProvider. + setAdminCookie(t, ts) + _, err = ts.GraphQLProvider.FgaWriteModel(ctx, &model.FgaWriteModelInput{Dsl: fgaTestModel}) + require.NoError(t, err) + require.NoError(t, eng.WriteTuples(ctx, []engine.TupleKey{ + {User: "user:" + userID, Relation: "viewer", Object: "document:secret"}, + })) + clearCookies(ts) + + setAdminCookie(t, ts) + deleteRes, err := ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{Email: email}) + require.NoError(t, err) + require.NotNil(t, deleteRes) + + t.Run("federated identity is gone and the principal can be re-provisioned", func(t *testing.T) { + _, err := sp.GetFederatedIdentity(ctx, orgID, issuer, subject) + require.Error(t, err, "orphaned federated identity is a permanent SSO lockout") + + fresh, err := sp.AddUser(ctx, &schemas.User{ + Email: &[]string{"reprovision_" + uuid.New().String() + "@authorizer.dev"}[0], + SignupMethods: "basic_auth", + }) + require.NoError(t, err) + _, err = sp.AddFederatedIdentity(ctx, &schemas.FederatedIdentity{ + OrgID: orgID, Issuer: issuer, Subject: subject, UserID: fresh.ID, + }) + require.NoError(t, err, "the (org, issuer, subject) triple must be free again") + }) + + t.Run("org membership, authenticator and passkey are gone", func(t *testing.T) { + _, err := sp.GetOrgMembership(ctx, orgID, userID) + assert.Error(t, err) + _, err = sp.GetAuthenticatorDetailsByUserId(ctx, userID, "totp") + assert.Error(t, err) + creds, err := sp.ListWebauthnCredentialsByUserID(ctx, userID) + require.NoError(t, err) + assert.Empty(t, creds) + }) + + t.Run("session tokens and mfa sessions are gone", func(t *testing.T) { + _, err := sp.GetSessionTokenByUserIDAndKey(ctx, userID, "access") + assert.Error(t, err) + sessions, err := sp.GetAllMFASessionsByUserID(ctx, userID) + require.NoError(t, err) + assert.Empty(t, sessions) + }) + + t.Run("fga tuples are gone", func(t *testing.T) { + res, err := eng.ReadTuples(ctx, engine.ReadTuplesFilter{Object: "document:secret"}) + require.NoError(t, err) + for _, tk := range res.Tuples { + assert.NotEqual(t, "user:"+userID, tk.User, "deleted user must not keep holding grants") + } + }) +} diff --git a/internal/service/admin_users.go b/internal/service/admin_users.go index 59c8f11f0..6738cf516 100644 --- a/internal/service/admin_users.go +++ b/internal/service/admin_users.go @@ -132,6 +132,13 @@ func (p *provider) UpdateUser(ctx context.Context, meta RequestMetadata, params params.Roles == nil && params.IsMultiFactorAuthEnabled == nil && params.ResetMfa == nil && + // EmailVerified/PhoneNumberVerified were missing from this gate even + // though both are applied further down, so an admin force-verifying an + // address — the operator's escape hatch when a user cannot receive mail + // — was rejected with "please enter atleast one param to update" unless + // they padded the call with an unrelated field. + params.EmailVerified == nil && + params.PhoneNumberVerified == nil && params.AppData == nil { log.Debug().Msg("please enter atleast one param to update") return nil, nil, InvalidArgument("please enter atleast one param to update") @@ -393,6 +400,15 @@ func (p *provider) DeleteUser(ctx context.Context, meta RequestMetadata, params return nil, nil, err } + // FGA tuples live outside StorageProvider, so the storage cascade cannot + // reach them. Purge synchronously (this is security cleanup, and callers + // must not observe a deleted user still holding grants) but best-effort: a + // tuple-store failure is logged, not returned — the user row is already gone + // and failing here would report a delete that did happen as failed. + if err := p.purgeFgaTuplesForUser(ctx, user.ID); err != nil { + log.Warn().Err(err).Str("user_id", user.ID).Msg("Failed to purge FGA tuples for deleted user; grants may be orphaned") + } + res := &model.Response{ Message: `user deleted successfully`, } diff --git a/internal/service/fga.go b/internal/service/fga.go index 0a98f8225..bcadd85fb 100644 --- a/internal/service/fga.go +++ b/internal/service/fga.go @@ -345,3 +345,55 @@ func (p *provider) enforceRequiredRelations(ctx context.Context, meta RequestMet } return nil } + +// fgaPurgePageSize is the ReadTuples page size used when scanning the store for +// a deleted user's grants. +const fgaPurgePageSize = 100 + +// purgeFgaTuplesForUser removes every relationship tuple naming the given user, +// so a hard-deleted account cannot keep holding grants. A no-op when FGA is not +// enabled. +// +// ponytail: this pages through the WHOLE tuple store and matches client-side, +// because OpenFGA's Read API rejects a user-only filter — it requires at least +// an object type ("the 'tuple_key' field was provided but the object type field +// is required"), and we do not know which types a user appears under. That is +// fine for an admin delete-user call; the upgrade path, if the store grows big +// enough to matter, is to enumerate the model's type definitions via ReadModel +// and issue one filtered Read per type. +func (p *provider) purgeFgaTuplesForUser(ctx context.Context, userID string) error { + if p.AuthzEngine == nil || strings.TrimSpace(userID) == "" { + return nil + } + subject := "user:" + userID + var stale []engine.TupleKey + contToken := "" + for { + res, err := p.AuthzEngine.ReadTuples(ctx, engine.ReadTuplesFilter{ + PageSize: fgaPurgePageSize, + ContinuationToken: contToken, + }) + if err != nil { + return fmt.Errorf("read tuples: %w", err) + } + for _, t := range res.Tuples { + // Match the user as subject and as object: a model may name the + // account on either side, and leaving either behind is a live grant + // pointing at a dead id. + if t.User == subject || t.Object == subject { + stale = append(stale, t) + } + } + if res.ContinuationToken == "" { + break + } + contToken = res.ContinuationToken + } + if len(stale) == 0 { + return nil + } + if err := p.AuthzEngine.DeleteTuples(ctx, stale); err != nil { + return fmt.Errorf("delete %d tuples: %w", len(stale), err) + } + return nil +} diff --git a/internal/storage/db/arangodb/user.go b/internal/storage/db/arangodb/user.go index f818d3f7b..0094ca05e 100644 --- a/internal/storage/db/arangodb/user.go +++ b/internal/storage/db/arangodb/user.go @@ -73,25 +73,27 @@ func (p *provider) UpdateUser(ctx context.Context, user *schemas.User) (*schemas // DeleteUser to delete user information from database func (p *provider) DeleteUser(ctx context.Context, user *schemas.User) error { - collection, _ := p.db.Collection(ctx, schemas.Collections.User) - _, err := collection.RemoveDocument(ctx, user.Key) - if err != nil { - return err - } - query := fmt.Sprintf(`FOR d IN %s FILTER d.user_id == @user_id REMOVE { _key: d._key } IN %s`, schemas.Collections.Session, schemas.Collections.Session) + // Children first, user row last: ArangoDB runs these as separate queries, so + // a partial failure must leave the user row intact and retryable rather than + // stranding orphans that point at a dead id (see + // schemas.UserOwnedCollections). bindVars := map[string]interface{}{ - // Session.UserID is stored as the full document handle (collection/key), - // which is what user.ID holds after Add/Get. Binding user.Key (bare key) - // would match zero session rows. This cascade is the only session-cleanup - // path on user deletion. + // user_id is stored as the full document handle (collection/key), which + // is what user.ID holds after Add/Get. Binding user.Key (bare key) would + // match zero rows. This cascade is the only cleanup path on user deletion. "user_id": user.ID, } - cursor, err := p.db.Query(ctx, query, bindVars) - if err != nil { - return err + for _, collectionName := range schemas.UserOwnedCollections { + query := fmt.Sprintf(`FOR d IN %s FILTER d.user_id == @user_id REMOVE { _key: d._key } IN %s`, collectionName, collectionName) + cursor, err := p.db.Query(ctx, query, bindVars) + if err != nil { + return err + } + _ = cursor.Close() } - defer func() { _ = cursor.Close() }() - return nil + collection, _ := p.db.Collection(ctx, schemas.Collections.User) + _, err := collection.RemoveDocument(ctx, user.Key) + return err } // ListUsers to get list of users from database diff --git a/internal/storage/db/cassandradb/user.go b/internal/storage/db/cassandradb/user.go index 0d047ab02..64faac0b3 100644 --- a/internal/storage/db/cassandradb/user.go +++ b/internal/storage/db/cassandradb/user.go @@ -116,37 +116,40 @@ func (p *provider) UpdateUser(ctx context.Context, user *schemas.User) (*schemas // DeleteUser to delete user information from database func (p *provider) DeleteUser(ctx context.Context, user *schemas.User) error { - query := fmt.Sprintf("DELETE FROM %s WHERE id = ?", KeySpace+"."+schemas.Collections.User) - err := p.db.Query(query, user.ID).Exec() - if err != nil { - return err - } - getSessionsQuery := fmt.Sprintf("SELECT id FROM %s WHERE user_id = ? ALLOW FILTERING", KeySpace+"."+schemas.Collections.Session) - scanner := p.db.Query(getSessionsQuery, user.ID).Iter().Scanner() - var sessionIDList []string - for scanner.Next() { - var wlID string - err = scanner.Scan(&wlID) - if err != nil { + // Children first, user row last: Cassandra has no cross-table transaction, so + // a partial failure must leave the user row intact and retryable rather than + // stranding orphans that point at a dead id (see + // schemas.UserOwnedCollections). Every one of these tables is keyed on `id`. + for _, table := range schemas.UserOwnedCollections { + selectQuery := fmt.Sprintf("SELECT id FROM %s WHERE user_id = ? ALLOW FILTERING", KeySpace+"."+table) + scanner := p.db.Query(selectQuery, user.ID).Iter().Scanner() + var ids []string + for scanner.Next() { + var id string + if err := scanner.Scan(&id); err != nil { + return err + } + ids = append(ids, id) + } + if err := scanner.Err(); err != nil { return err } - sessionIDList = append(sessionIDList, wlID) - } - if len(sessionIDList) > 0 { - placeholders := strings.Repeat("?,", len(sessionIDList)) - placeholders = strings.TrimSuffix(placeholders, ",") - deleteValues := make([]interface{}, len(sessionIDList)) - for i, id := range sessionIDList { + if len(ids) == 0 { + continue + } + placeholders := strings.TrimSuffix(strings.Repeat("?,", len(ids)), ",") + deleteValues := make([]interface{}, len(ids)) + for i, id := range ids { deleteValues[i] = id } - deleteSessionQuery := fmt.Sprintf("DELETE FROM %s WHERE id IN (%s)", KeySpace+"."+schemas.Collections.Session, placeholders) - err = p.db.Query(deleteSessionQuery, deleteValues...).Exec() - if err != nil { + deleteQuery := fmt.Sprintf("DELETE FROM %s WHERE id IN (%s)", KeySpace+"."+table, placeholders) + if err := p.db.Query(deleteQuery, deleteValues...).Exec(); err != nil { return err } } - return nil + query := fmt.Sprintf("DELETE FROM %s WHERE id = ?", KeySpace+"."+schemas.Collections.User) + return p.db.Query(query, user.ID).Exec() } // ListUsers to get list of users from database diff --git a/internal/storage/db/couchbase/user.go b/internal/storage/db/couchbase/user.go index f1a58cd9b..f088fb2fd 100644 --- a/internal/storage/db/couchbase/user.go +++ b/internal/storage/db/couchbase/user.go @@ -75,14 +75,26 @@ func (p *provider) UpdateUser(ctx context.Context, user *schemas.User) (*schemas // DeleteUser to delete user information from database func (p *provider) DeleteUser(ctx context.Context, user *schemas.User) error { + // Children first, user row last: Couchbase runs these as separate statements, + // so a partial failure must leave the user row intact and retryable rather + // than stranding orphans that point at a dead id (see + // schemas.UserOwnedCollections). Sessions were not cleaned up here at all + // before — every other backend did it, this one did not. + for _, collection := range schemas.UserOwnedCollections { + query := fmt.Sprintf("DELETE FROM %s.%s WHERE user_id = $1", p.scopeName, collection) + if _, err := p.db.Query(query, &gocb.QueryOptions{ + ScanConsistency: gocb.QueryScanConsistencyRequestPlus, + Context: ctx, + PositionalParameters: []interface{}{user.ID}, + }); err != nil { + return err + } + } removeOpt := gocb.RemoveOptions{ Context: ctx, } _, err := p.db.Collection(schemas.Collections.User).Remove(user.ID, &removeOpt) - if err != nil { - return err - } - return nil + return err } // ListUsers to get list of users from database diff --git a/internal/storage/db/dynamodb/user.go b/internal/storage/db/dynamodb/user.go index b10c81bb7..890bf930a 100644 --- a/internal/storage/db/dynamodb/user.go +++ b/internal/storage/db/dynamodb/user.go @@ -141,23 +141,28 @@ func (p *provider) DeleteUser(ctx context.Context, user *schemas.User) error { if user.ID == "" { return nil } - if err := p.deleteItemByHash(ctx, schemas.Collections.User, "id", user.ID); err != nil { - return err - } - items, err := p.queryEq(ctx, schemas.Collections.Session, "user_id", "user_id", user.ID, nil) - if err != nil { - return err - } - for _, it := range items { - var s schemas.Session - if err := unmarshalItem(it, &s); err != nil { + // Children first, user row last: DynamoDB has no transaction here, so a + // partial failure must leave the user row intact and retryable rather than + // stranding orphans that point at a dead id (see + // schemas.UserOwnedCollections). Every one of these tables hashes on "id" and + // carries a "user_id" GSI (see tables.go), so one query/delete loop serves + // them all. + for _, table := range schemas.UserOwnedCollections { + items, err := p.queryEq(ctx, table, "user_id", "user_id", user.ID, nil) + if err != nil { return err } - if err := p.deleteItemByHash(ctx, schemas.Collections.Session, "id", s.ID); err != nil { - return err + for _, it := range items { + id, ok := it["id"].(*types.AttributeValueMemberS) + if !ok || id.Value == "" { + continue + } + if err := p.deleteItemByHash(ctx, table, "id", id.Value); err != nil { + return err + } } } - return nil + return p.deleteItemByHash(ctx, schemas.Collections.User, "id", user.ID) } // ListUsers to get list of users from database diff --git a/internal/storage/db/mongodb/user.go b/internal/storage/db/mongodb/user.go index 698e716cc..8f523deb5 100644 --- a/internal/storage/db/mongodb/user.go +++ b/internal/storage/db/mongodb/user.go @@ -68,17 +68,18 @@ func (p *provider) UpdateUser(ctx context.Context, user *schemas.User) (*schemas // DeleteUser to delete user information from database func (p *provider) DeleteUser(ctx context.Context, user *schemas.User) error { + // Children first, user row last: MongoDB has no transaction here, so a + // partial failure must leave the user row intact and retryable rather than + // stranding orphans that point at a dead id (see + // schemas.UserOwnedCollections). + for _, collection := range schemas.UserOwnedCollections { + if _, err := p.db.Collection(collection, options.Collection()).DeleteMany(ctx, bson.M{"user_id": user.ID}, options.Delete()); err != nil { + return err + } + } userCollection := p.db.Collection(schemas.Collections.User, options.Collection()) _, err := userCollection.DeleteOne(ctx, bson.M{"_id": user.ID}, options.Delete()) - if err != nil { - return err - } - sessionCollection := p.db.Collection(schemas.Collections.Session, options.Collection()) - _, err = sessionCollection.DeleteMany(ctx, bson.M{"user_id": user.ID}, options.Delete()) - if err != nil { - return err - } - return nil + return err } // ListUsers to get list of users from database diff --git a/internal/storage/db/provider_template/user.go b/internal/storage/db/provider_template/user.go index 74f622733..af25679bd 100644 --- a/internal/storage/db/provider_template/user.go +++ b/internal/storage/db/provider_template/user.go @@ -41,7 +41,11 @@ func (p *provider) UpdateUser(ctx context.Context, user *schemas.User) (*schemas return user, nil } -// DeleteUser to delete user information from database +// DeleteUser to delete user information from database. +// +// A real implementation MUST cascade to every collection in +// schemas.UserOwnedCollections (children first, user row last) — leaving an +// orphaned federated-identity row behind is a permanent SSO lockout. func (p *provider) DeleteUser(ctx context.Context, user *schemas.User) error { return nil } diff --git a/internal/storage/db/sql/user.go b/internal/storage/db/sql/user.go index cda734c59..d65f7eb3e 100644 --- a/internal/storage/db/sql/user.go +++ b/internal/storage/db/sql/user.go @@ -73,13 +73,30 @@ func (p *provider) UpdateUser(ctx context.Context, user *schemas.User) (*schemas return user, nil } +// userOwnedModels are the GORM models behind schemas.UserOwnedCollections, in +// the same order. TestUserOwnedModelsMatchCollections asserts the two lists +// resolve to the same table names, so adding a collection there without a model +// here fails the build's tests rather than silently skipping a table. +var userOwnedModels = []interface{}{ + &schemas.Session{}, + &schemas.FederatedIdentity{}, + &schemas.OrgMembership{}, + &schemas.Authenticator{}, + &schemas.WebauthnCredential{}, + &schemas.SessionToken{}, + &schemas.MFASession{}, +} + // DeleteUser to delete user information from database func (p *provider) DeleteUser(ctx context.Context, user *schemas.User) error { - // Delete the user and their sessions atomically so a failure cannot leave - // orphaned session rows behind. + // Hard delete: the user row and every row keyed on their id go together, in + // one transaction, so a failure cannot leave orphans behind. See + // schemas.UserOwnedCollections for why an orphan here is a lockout. err := p.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { - if err := tx.Where("user_id = ?", user.ID).Delete(&schemas.Session{}).Error; err != nil { - return err + for _, m := range userOwnedModels { + if err := tx.Where("user_id = ?", user.ID).Delete(m).Error; err != nil { + return err + } } return tx.Delete(&user).Error }) diff --git a/internal/storage/db/sql/user_cascade_test.go b/internal/storage/db/sql/user_cascade_test.go new file mode 100644 index 000000000..2e0118615 --- /dev/null +++ b/internal/storage/db/sql/user_cascade_test.go @@ -0,0 +1,126 @@ +package sql + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + + "github.com/authorizerdev/authorizer/internal/refs" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// TestUserOwnedModelsMatchCollections keeps the GORM model list behind the +// DeleteUser cascade in lockstep with schemas.UserOwnedCollections, which the +// other five backends iterate directly. Without this, adding a user-keyed table +// to the shared list would cascade on five backends and silently skip SQL. +func TestUserOwnedModelsMatchCollections(t *testing.T) { + cfg := sqlMigrationTestConfig(t, "sqlite") + p, err := NewProvider(cfg, sqlTestDeps(t)) + require.NoError(t, err) + defer func() { _ = p.Close() }() + + got := make([]string, 0, len(userOwnedModels)) + for _, m := range userOwnedModels { + stmt := &gorm.Statement{DB: p.db} + require.NoError(t, stmt.Parse(m)) + got = append(got, stmt.Table) + } + assert.ElementsMatch(t, schemas.UserOwnedCollections, got) +} + +// TestDeleteUserCascadesAllUserOwnedRows proves a hard delete removes every row +// keyed on the user id, not just their sessions. The federated-identity row is +// the one that matters most: left behind it points at a dead user id, SSO login +// fails closed on it, and the unique (org_id, issuer, subject) triple blocks +// re-provisioning — a permanent lockout. +func TestDeleteUserCascadesAllUserOwnedRows(t *testing.T) { + for _, dbType := range sqlMigrationTestDBTypes() { + t.Run(dbType, func(t *testing.T) { + cfg := sqlMigrationTestConfig(t, dbType) + p, err := NewProvider(cfg, sqlTestDeps(t)) + require.NoError(t, err) + defer func() { _ = p.Close() }() + + ctx := context.Background() + user, err := p.AddUser(ctx, &schemas.User{ + Email: refs.NewStringRef("cascade-" + dbType + "@example.com"), + SignupMethods: "basic_auth", + }) + require.NoError(t, err) + + const orgID = "org-cascade" + const issuer = "https://idp.example.com" + const subject = "upstream-subject-1" + + require.NoError(t, p.AddSession(ctx, &schemas.Session{UserID: user.ID})) + _, err = p.AddFederatedIdentity(ctx, &schemas.FederatedIdentity{ + OrgID: orgID, Issuer: issuer, Subject: subject, UserID: user.ID, + }) + require.NoError(t, err) + _, err = p.AddOrgMembership(ctx, &schemas.OrgMembership{OrgID: orgID, UserID: user.ID, Roles: "member"}) + require.NoError(t, err) + _, err = p.AddAuthenticator(ctx, &schemas.Authenticator{UserID: user.ID, Method: "totp", Secret: "s3cret"}) + require.NoError(t, err) + _, err = p.AddWebauthnCredential(ctx, &schemas.WebauthnCredential{ + UserID: user.ID, CredentialID: "cred-" + dbType, PublicKey: "pk", Name: "laptop", + }) + require.NoError(t, err) + require.NoError(t, p.AddSessionToken(ctx, &schemas.SessionToken{UserID: user.ID, KeyName: "access", Token: "t"})) + require.NoError(t, p.AddMFASession(ctx, &schemas.MFASession{UserID: user.ID, KeyName: "mfa"})) + + require.NoError(t, p.DeleteUser(ctx, user)) + + // Every user-keyed table must be empty for this id. + for _, m := range userOwnedModels { + var count int64 + require.NoError(t, p.db.WithContext(ctx).Model(m).Where("user_id = ?", user.ID).Count(&count).Error) + stmt := &gorm.Statement{DB: p.db} + require.NoError(t, stmt.Parse(m)) + assert.Zero(t, count, "%s still holds rows for the deleted user", stmt.Table) + } + + // Lockout regression: the triple is free again, so the same upstream + // principal can be re-provisioned instead of failing closed forever. + _, err = p.GetFederatedIdentity(ctx, orgID, issuer, subject) + require.Error(t, err, "federated identity must be gone after the user is deleted") + fresh, err := p.AddUser(ctx, &schemas.User{ + Email: refs.NewStringRef("cascade-reprovision-" + dbType + "@example.com"), + SignupMethods: "basic_auth", + }) + require.NoError(t, err) + _, err = p.AddFederatedIdentity(ctx, &schemas.FederatedIdentity{ + OrgID: orgID, Issuer: issuer, Subject: subject, UserID: fresh.ID, + }) + require.NoError(t, err, "re-provisioning the same (org, issuer, subject) must succeed") + }) + } +} + +// TestDeleteUserAtomicRollback proves the whole cascade is one transaction: when +// a later step fails, the user row is not left deleted on its own. Without it a +// partial cascade would strand exactly the orphans this test suite is about. +func TestDeleteUserAtomicRollback(t *testing.T) { + cfg := sqlMigrationTestConfig(t, "sqlite") + p, err := NewProvider(cfg, sqlTestDeps(t)) + require.NoError(t, err) + defer func() { _ = p.Close() }() + + ctx := context.Background() + user, err := p.AddUser(ctx, &schemas.User{ + Email: refs.NewStringRef("rollback@example.com"), + SignupMethods: "basic_auth", + }) + require.NoError(t, err) + + // Force a cascade step to fail deterministically. + require.NoError(t, p.db.Migrator().DropTable(&schemas.MFASession{})) + + require.Error(t, p.DeleteUser(ctx, user), "delete must fail when a cascade step errors") + + got, err := p.GetUserByID(ctx, user.ID) + require.NoError(t, err, "user must still exist after the cascade rolled back") + assert.Equal(t, user.ID, got.ID) +} diff --git a/internal/storage/provider_test.go b/internal/storage/provider_test.go index 64fc95aa4..3c0774f43 100644 --- a/internal/storage/provider_test.go +++ b/internal/storage/provider_test.go @@ -226,6 +226,10 @@ func TestStorageProvider(t *testing.T) { testUserScimFields(t, ctx, provider) }) + t.Run("Delete User Cascade", func(t *testing.T) { + testDeleteUserCascade(t, ctx, provider) + }) + if isSQLTestDB(dbType) { t.Run("SQL CRUD Correctness Fixes", func(t *testing.T) { testSQLCRUDCorrectnessFixes(t, ctx, provider) @@ -2257,3 +2261,78 @@ func testUserScimFields(t *testing.T, ctx context.Context, provider Provider) { require.NoError(t, err) assert.False(t, afterDeactivate.IsActive, "IsActive=false must persist") } + +// testDeleteUserCascade asserts the hard-delete cascade documented on +// schemas.UserOwnedCollections holds on THIS backend: deleting a user removes +// every row keyed on their id, not just their sessions. +// +// It runs for all six backends because that is the only way this class of bug +// is caught — the cascade used to cover sessions on five backends and nothing +// at all on Couchbase, and CI (SQLite only) was green throughout. The +// federated-identity row is the one that matters: orphaned, it points at a dead +// user id, SSO login fails closed on it, and the unique (org_id, issuer, +// subject) triple blocks re-provisioning, so the principal is locked out for +// good. +func testDeleteUserCascade(t *testing.T, ctx context.Context, provider Provider) { + user, err := provider.AddUser(ctx, &schemas.User{ + ID: uuid.New().String(), + Email: refs.NewStringRef("cascade_" + uuid.New().String() + "@test.com"), + SignupMethods: "basic_auth", + }) + require.NoError(t, err) + + orgID := "org_" + uuid.New().String() + issuer := "https://idp.example.com" + subject := "upstream_" + uuid.New().String() + + require.NoError(t, provider.AddSession(ctx, &schemas.Session{UserID: user.ID})) + _, err = provider.AddFederatedIdentity(ctx, &schemas.FederatedIdentity{ + OrgID: orgID, Issuer: issuer, Subject: subject, UserID: user.ID, + }) + require.NoError(t, err) + _, err = provider.AddOrgMembership(ctx, &schemas.OrgMembership{OrgID: orgID, UserID: user.ID, Roles: "member"}) + require.NoError(t, err) + _, err = provider.AddAuthenticator(ctx, &schemas.Authenticator{UserID: user.ID, Method: "totp", Secret: "s3cret"}) + require.NoError(t, err) + _, err = provider.AddWebauthnCredential(ctx, &schemas.WebauthnCredential{ + UserID: user.ID, CredentialID: "cred_" + uuid.New().String(), PublicKey: "pk", Name: "laptop", + }) + require.NoError(t, err) + require.NoError(t, provider.AddSessionToken(ctx, &schemas.SessionToken{ + UserID: user.ID, KeyName: "access", Token: "t", ExpiresAt: time.Now().Add(time.Hour).Unix(), + })) + require.NoError(t, provider.AddMFASession(ctx, &schemas.MFASession{ + UserID: user.ID, KeyName: "mfa", ExpiresAt: time.Now().Add(time.Hour).Unix(), + })) + + require.NoError(t, provider.DeleteUser(ctx, user)) + + _, err = provider.GetFederatedIdentity(ctx, orgID, issuer, subject) + assert.Error(t, err, "orphaned federated identity is a permanent SSO lockout") + _, err = provider.GetOrgMembership(ctx, orgID, user.ID) + assert.Error(t, err, "org membership must not survive the user") + _, err = provider.GetAuthenticatorDetailsByUserId(ctx, user.ID, "totp") + assert.Error(t, err, "TOTP secret must not survive the user") + _, err = provider.GetSessionTokenByUserIDAndKey(ctx, user.ID, "access") + assert.Error(t, err, "session token must not survive the user") + + creds, err := provider.ListWebauthnCredentialsByUserID(ctx, user.ID) + require.NoError(t, err) + assert.Empty(t, creds, "passkeys must not survive the user") + mfaSessions, err := provider.GetAllMFASessionsByUserID(ctx, user.ID) + require.NoError(t, err) + assert.Empty(t, mfaSessions, "MFA sessions must not survive the user") + + // Lockout regression: the triple is free, so the same upstream principal can + // be re-provisioned onto a fresh account instead of failing closed forever. + fresh, err := provider.AddUser(ctx, &schemas.User{ + ID: uuid.New().String(), + Email: refs.NewStringRef("reprovision_" + uuid.New().String() + "@test.com"), + SignupMethods: "basic_auth", + }) + require.NoError(t, err) + _, err = provider.AddFederatedIdentity(ctx, &schemas.FederatedIdentity{ + OrgID: orgID, Issuer: issuer, Subject: subject, UserID: fresh.ID, + }) + assert.NoError(t, err, "the (org, issuer, subject) triple must be free again") +} diff --git a/internal/storage/schemas/model.go b/internal/storage/schemas/model.go index 02436e71b..c70a737ca 100644 --- a/internal/storage/schemas/model.go +++ b/internal/storage/schemas/model.go @@ -60,4 +60,26 @@ var ( SAMLServiceProvider: Prefix + "saml_service_providers", SAMLIDPKey: Prefix + "saml_idp_keys", } + + // UserOwnedCollections are every collection keyed on `user_id` that a hard + // delete of a user (StorageProvider.DeleteUser) MUST cascade to. This is the + // single source of truth: all six backends iterate it, so a new user-keyed + // table is covered everywhere by adding one line here. + // + // A missed entry is not cosmetic. An orphaned authorizer_federated_identities + // row keeps pointing at a dead user id, jitProvisionFederatedUser fails + // closed on it, and the (org_id, issuer, subject) uniqueness prevents + // re-provisioning — a permanent SSO lockout for that principal. + // + // Soft deletes (DeactivateAccount, revoke access) only stamp the user row and + // must NOT cascade — the account is meant to come back. + UserOwnedCollections = []string{ + Collections.Session, + Collections.FederatedIdentity, + Collections.OrgMembership, + Collections.Authenticators, + Collections.WebauthnCredential, + Collections.SessionToken, + Collections.MFASession, + } ) diff --git a/internal/storage/user_cascade_contract_test.go b/internal/storage/user_cascade_contract_test.go new file mode 100644 index 000000000..85748a960 --- /dev/null +++ b/internal/storage/user_cascade_contract_test.go @@ -0,0 +1,67 @@ +package storage + +import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "testing" +) + +// TestDeleteUserCascadeIsUniform enforces that every backend's DeleteUser +// cascades over the shared schemas.UserOwnedCollections list rather than a +// hand-rolled subset. +// +// It is a static check for the same reason TestNotFoundContractIsUniform is: +// the failure it prevents is backend-specific and silent. CI runs SQLite only, +// so a cascade that covers six tables on SQL and one on Couchbase passes every +// test run and only strands orphans in production — and an orphaned +// federated-identity row is a permanent SSO lockout, not a tidiness problem. +// +// SQL is the one exception: GORM deletes by model, not by table name, so it +// carries a parallel userOwnedModels list. sql.TestUserOwnedModelsMatchCollections +// asserts the two resolve to the same set of tables. +func TestDeleteUserCascadeIsUniform(t *testing.T) { + // backend -> identifier its DeleteUser must reference. + want := map[string]string{ + "sql": "userOwnedModels", + "mongodb": "UserOwnedCollections", + "arangodb": "UserOwnedCollections", + "cassandradb": "UserOwnedCollections", + "dynamodb": "UserOwnedCollections", + "couchbase": "UserOwnedCollections", + } + + for _, backend := range backends { + ident, ok := want[backend] + if !ok { + t.Fatalf("backend %q has no expected cascade identifier — add it here when adding a backend", backend) + } + path := filepath.Join("db", backend, "user.go") + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", path, err) + } + var found, seen bool + for _, decl := range f.Decls { + fn, isFn := decl.(*ast.FuncDecl) + if !isFn || fn.Recv == nil || fn.Body == nil || fn.Name.Name != "DeleteUser" { + continue + } + seen = true + ast.Inspect(fn.Body, func(n ast.Node) bool { + if id, isIdent := n.(*ast.Ident); isIdent && id.Name == ident { + found = true + } + return !found + }) + } + if !seen { + t.Fatalf("%s: no DeleteUser method found — the check would vacuously pass", path) + } + if !found { + t.Errorf("%s: DeleteUser does not cascade over %s — a user-keyed table added to schemas.UserOwnedCollections would be skipped on this backend", path, ident) + } + } +}