fix: reject empty owner in owner-scoped cascade revocation (closes #275)

internal/service/credential.go

@@ -639,7 +639,30 @@ func (s *CredentialService) RevokeAllActiveForIdentity(ctx context.Context, iden
 // Fires one RevocationNotifier event per affected JTI after the revocation
 // commits, exactly like RevokeAllActiveForIdentity, so Shield's deny-set picks
 // them up within seconds.
+//
+// Both ownerUserID and accountID are REQUIRED and rejected when empty. This is
+// a hard guard, not defensive tidiness: identities.owner_user_id is NOT NULL
+// (001_init_schema), so an ownerless identity — the documented posture for
+// `discovered` ones (identity-lifecycle.md "Ownership: relaxed for discovered
+// only") — stores the empty string. An empty ownerUserID would therefore match
+// EVERY ownerless identity in the account and cascade-revoke each one's whole
+// delegation subtree. The realistic trigger is the intended caller: an IdP
+// offboarding webhook whose user-id field arrives blank, turning "revoke one
+// departing human's agents" into a tenant-wide outage of exactly the workloads
+// nobody watches. Revocation is not reversible, so this fails closed.
+//
+// The identity-scoped sibling is protected only incidentally — its uuid cast
+// rejects "" with a 22P02 — so the guard lives here explicitly rather than
+// relying on that accident holding for a TEXT column. Mirrored in SQL by
+// migration 042 so it survives a caller that bypasses this service.
 func (s *CredentialService) RevokeAllActiveForOwner(ctx context.Context, ownerUserID, accountID, reason string) (int64, error) {
+	if ownerUserID == "" || accountID == "" {
+		return 0, fmt.Errorf(
+			"RevokeAllActiveForOwner requires a non-empty owner_user_id and account_id (got owner=%q account=%q): "+
+				"an empty owner matches every ownerless identity in the account",
+			ownerUserID, accountID)
+	}
+
 	if reason == "" {
 		reason = "owner_deactivated"
 	}

internal/service/credential_revoke_owner_test.go

@@ -0,0 +1,66 @@
+package service
+
+import (
+	"context"
+	"testing"
+)
+
+// TestRevokeAllActiveForOwner_RejectsEmptyOwner pins the guard that keeps a
+// blank owner from cascade-revoking every ownerless identity in an account.
+//
+// Why this matters: identities.owner_user_id is VARCHAR(255) NOT NULL, so an
+// ownerless identity stores the empty string rather than NULL — and ownerless is a documented
+// posture for `discovered` identities, not an anomaly. Migration 041's seed
+// predicate is a plain equality on that column, so an empty owner selects every
+// one of them and walks each delegation subtree. The realistic trigger is an IdP
+// offboarding webhook whose user-id field arrives blank.
+//
+// The receiver is built with a nil repo ON PURPOSE. The guard must short-circuit
+// before any repository call, so a nil-pointer panic here is a real failure
+// signal: it means the guard did not fire and execution reached the DB layer.
+func TestRevokeAllActiveForOwner_RejectsEmptyOwner(t *testing.T) {
+	t.Parallel()
+
+	svc := &CredentialService{} // repo intentionally nil — see doc comment
+	ctx := context.Background()
+
+	tests := []struct {
+		name      string
+		ownerUser string
+		accountID string
+	}{
+		{"empty owner", "", "acct-123"},
+		{"empty account", "user-42", ""},
+		{"both empty", "", ""},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			n, err := svc.RevokeAllActiveForOwner(ctx, tc.ownerUser, tc.accountID, "owner_deactivated")
+
+			if err == nil {
+				t.Fatalf("expected an error for owner=%q account=%q; a blank owner must never "+
+					"reach the cascade — it matches every ownerless identity in the account",
+					tc.ownerUser, tc.accountID)
+			}
+			if n != 0 {
+				t.Errorf("revoked count = %d, want 0 — nothing may be revoked on the reject path", n)
+			}
+		})
+	}
+}
+
+// TestRevokeAllActiveForOwner_GuardPrecedesReasonDefault documents ordering: the
+// argument guard runs before the reason default, so a caller passing an empty
+// reason AND an empty owner still gets the argument error rather than having the
+// blank owner silently carried into the query with a defaulted reason.
+func TestRevokeAllActiveForOwner_GuardPrecedesReasonDefault(t *testing.T) {
+	t.Parallel()
+
+	svc := &CredentialService{} // nil repo: reaching it would panic
+	if _, err := svc.RevokeAllActiveForOwner(context.Background(), "", "", ""); err == nil {
+		t.Fatal("expected an error when owner, account and reason are all empty")
+	}
+}

migrations/042_revoke_by_owner_reject_empty.down.sql

@@ -0,0 +1,54 @@
+-- 042_revoke_by_owner_reject_empty.down.sql
+-- Restores the migration-041 function body (no empty-owner guard). Same
+-- signature and return type, so CREATE OR REPLACE suffices — mirroring how
+-- 031's down restores the 029 bodies.
+--
+-- WARNING: rolling this back reintroduces the hole 042 closed — calling
+-- revoke_credentials_by_owner with an empty p_owner_user_id again matches every
+-- ownerless identity in the account (owner_user_id is NOT NULL, so ownerless
+-- rows store '') and cascade-revokes each one's whole delegation subtree.
+-- Revoked credentials cannot be un-revoked. The service-layer guard in
+-- internal/service/credential.go still applies to callers that go through it;
+-- this rollback only removes the in-database backstop.
+
+CREATE OR REPLACE FUNCTION revoke_credentials_by_owner(
+    p_owner_user_id TEXT,
+    p_account_id    TEXT,
+    p_revoked_at    TIMESTAMPTZ,
+    p_reason        TEXT
+) RETURNS TABLE(
+    jti         VARCHAR(255),
+    identity_id UUID,
+    account_id  VARCHAR(255),
+    project_id  VARCHAR(255),
+    expires_at  TIMESTAMPTZ
+) AS $$
+BEGIN
+    RETURN QUERY
+    WITH RECURSIVE chain(id, jti, depth) AS (
+        SELECT ic.id, ic.jti, 0
+        FROM issued_credentials ic
+        JOIN identities i ON i.id = ic.identity_id
+        WHERE i.owner_user_id = p_owner_user_id
+          AND i.account_id    = p_account_id
+        UNION ALL
+        SELECT ic.id, ic.jti, chain.depth + 1
+        FROM issued_credentials ic
+        JOIN chain ON ic.parent_jti = chain.jti
+        WHERE chain.depth < 50
+    )
+    CYCLE jti SET is_cycle TO TRUE DEFAULT FALSE USING cycle_path
+    , revoked AS (
+        UPDATE issued_credentials ic
+        SET is_revoked    = TRUE,
+            revoked_at    = p_revoked_at,
+            revoke_reason = p_reason
+        WHERE ic.id IN (SELECT c.id FROM chain c WHERE NOT c.is_cycle)
+          AND ic.is_revoked = FALSE
+          AND ic.expires_at > p_revoked_at
+        RETURNING ic.jti, ic.identity_id, ic.account_id, ic.project_id, ic.expires_at
+    )
+    SELECT r.jti, r.identity_id, r.account_id, r.project_id, r.expires_at
+    FROM revoked r;
+END;
+$$ LANGUAGE plpgsql;

migrations/042_revoke_by_owner_reject_empty.up.sql

@@ -0,0 +1,91 @@
+-- 042_revoke_by_owner_reject_empty.up.sql
+-- Guard revoke_credentials_by_owner (migration 041) against an empty owner.
+--
+-- identities.owner_user_id is VARCHAR(255) NOT NULL (001_init_schema), so an
+-- ownerless identity does not store NULL — it stores ''. Ownerless is a real,
+-- documented posture, not an anomaly: discovered identities are created without
+-- an owner on purpose (identity-lifecycle.md "Ownership: relaxed for discovered
+-- only"), and internal/store/postgres/identity.go queries them with an explicit
+-- (owner_user_id IS NULL OR owner_user_id = '') facet.
+--
+-- 041's seed predicate is a plain equality:
+--
+--     WHERE i.owner_user_id = p_owner_user_id
+--       AND i.account_id    = p_account_id
+--
+-- so calling it with p_owner_user_id = '' selects EVERY ownerless identity in
+-- the account and cascade-revokes each one's entire delegation subtree. The
+-- realistic trigger is the intended caller — an IdP offboarding webhook whose
+-- user-id field arrives blank — turning "revoke one departing human's agents"
+-- into a tenant-wide outage of the workloads least likely to be monitored.
+-- Revoked credentials cannot be un-revoked.
+--
+-- internal/service/credential.go rejects the empty owner before reaching the
+-- repo. This mirrors it in SQL so the guarantee survives a caller that goes
+-- straight to the function — a direct psql session, another service, or a repo
+-- method added later. Defense in depth is cheap here and the failure mode is
+-- not recoverable.
+--
+-- RAISE EXCEPTION rather than returning zero rows: a blank owner is a caller
+-- bug, and silently succeeding with "revoked 0 credentials" would let a broken
+-- offboarding integration look healthy indefinitely. Failing loudly surfaces it
+-- on the first malformed event.
+--
+-- Everything else — the recursive walk, CYCLE guard, depth cap 50, liveness
+-- filters on the final UPDATE only, and the RETURNING projection — is carried
+-- over from 041 verbatim. CREATE OR REPLACE keeps the signature stable, so no
+-- caller changes.
+
+CREATE OR REPLACE FUNCTION revoke_credentials_by_owner(
+    p_owner_user_id TEXT,
+    p_account_id    TEXT,
+    p_revoked_at    TIMESTAMPTZ,
+    p_reason        TEXT
+) RETURNS TABLE(
+    jti         VARCHAR(255),
+    identity_id UUID,
+    account_id  VARCHAR(255),
+    project_id  VARCHAR(255),
+    expires_at  TIMESTAMPTZ
+) AS $$
+BEGIN
+    IF p_owner_user_id IS NULL OR p_owner_user_id = '' THEN
+        RAISE EXCEPTION
+            'revoke_credentials_by_owner: p_owner_user_id must be non-empty (an empty owner matches every ownerless identity in the account)'
+            USING ERRCODE = 'invalid_parameter_value';
+    END IF;
+
+    IF p_account_id IS NULL OR p_account_id = '' THEN
+        RAISE EXCEPTION
+            'revoke_credentials_by_owner: p_account_id must be non-empty (tenant scope is required)'
+            USING ERRCODE = 'invalid_parameter_value';
+    END IF;
+
+    RETURN QUERY
+    WITH RECURSIVE chain(id, jti, depth) AS (
+        SELECT ic.id, ic.jti, 0
+        FROM issued_credentials ic
+        JOIN identities i ON i.id = ic.identity_id
+        WHERE i.owner_user_id = p_owner_user_id
+          AND i.account_id    = p_account_id
+        UNION ALL
+        SELECT ic.id, ic.jti, chain.depth + 1
+        FROM issued_credentials ic
+        JOIN chain ON ic.parent_jti = chain.jti
+        WHERE chain.depth < 50
+    )
+    CYCLE jti SET is_cycle TO TRUE DEFAULT FALSE USING cycle_path
+    , revoked AS (
+        UPDATE issued_credentials ic
+        SET is_revoked    = TRUE,
+            revoked_at    = p_revoked_at,
+            revoke_reason = p_reason
+        WHERE ic.id IN (SELECT c.id FROM chain c WHERE NOT c.is_cycle)
+          AND ic.is_revoked = FALSE
+          AND ic.expires_at > p_revoked_at
+        RETURNING ic.jti, ic.identity_id, ic.account_id, ic.project_id, ic.expires_at
+    )
+    SELECT r.jti, r.identity_id, r.account_id, r.project_id, r.expires_at
+    FROM revoked r;
+END;
+$$ LANGUAGE plpgsql;