amadan
console: service-principal review round 2 (F1 tests, F12, C18-C21)
Second fix round on 83eab87 per the coordinator's re-review.
F1: the widened stale-token COUNT guards in MoveApp and RenameAppIn had no
test exercising the guard itself (only the follow-along UPDATE). Added
TestRenameAppRefusesOntoNameWithStaleServiceCredential and
TestMoveAppRefusesOntoNameWithStaleServiceCredential, both verified against
a manual revert of the guard back to kind = ? (reverting either one now
makes its test fail, confirmed by temporarily reverting each and observing
the failure, then restoring).
F12: every doubled straight-apostrophe ('') in a // comment gets rewritten
by this Go 1.26.5 gofmt into a single U+201D character on format --
reproduced independently with a minimal file outside this diff, confirmed
byte-level. Rewrote all nine affected comments (five in svcprincipal.go,
one in authstore.go, three in svcprincipal_test.go) as plain prose ("an
empty name", "is always empty") or with a "" (double-quote) form, neither
of which gofmt touches. Re-ran gofmt -w and grepped for U+201D afterward:
zero matches in every touched file.
C18: MoveApp's stale-token guard only counts rows still bound to the app
being moved, so it cannot see an ACCOUNT-scoped credential already live at
the destination under the same name -- a collision the new
cli_tokens_service_name unique index (F9, prior round) would otherwise
surface as a raw SQLite constraint-violation string mid-transaction. Added
a second guard: if any live kind='service' row travelling with the app
would collide by name with a live kind='service' row already at the
destination (any app scope), refuse with a message in the guard's own
voice before the UPDATE runs. Covered by
TestMoveAppRefusesOntoNameCollisionWithAccountScopedCredential, which also
asserts both credentials keep their original scope after the refusal
(transactional, not a partial move).
C19: added a rotated_at column -- created_at is when the credential was
granted and never moves again; rotated_at is when its CURRENT secret began,
stamped equal to created_at at mint (NewServicePrincipal) and advanced at
every RotateServicePrincipal, which finally uses the now parameter it was
carrying dead since the prior round's F7/C15 fix. Additive ALTER in the
same post-ALTER block as the other new columns; added to CLIToken and
scanCLITokenRows. Covered by TestRotateAdvancesRotatedAtLeavesCreatedAt and
TestNewServicePrincipalStampsRotatedAtAtMint.
C20: RevokeAccountToken now refuses accountID == 0 outright, with a comment
explaining why: the same fact that makes its kind-less query safe for every
real caller (a person's row always carries account_id = 0) is exactly what
makes a caller passing zero dangerous rather than merely wrong. Covered by
TestRevokeAccountTokenRefusesZeroAccount, which passes a real person's own
listing id with accountID=0 and asserts their token survives.
C21: added three tests that fail under the PRE-fix behaviour for the three
previously-unpinned resolutions:
- TestRevokeAccountTokenSoftRevokesNotDeletes: queries cli_tokens directly
after a revoke and asserts the row still exists with revoked_at != 0 (a
DELETE would leave zero rows).
- TestRevokedServicePrincipalAbsentFromCLITokens: asserts a revoked
service principal is absent from CLITokens(minterAddress) (would be
present without the revoked_at = 0 filter).
- TestRotateAdvancesRotatedAtLeavesCreatedAt (doubles as C19's test):
asserts created_at is unchanged and rotated_at moved across a rotate.
Spot-verified two of these (the F1 rename guard and the CLITokens filter)
by reverting the underlying code, watching the new test fail, and
restoring -- see the fix report for the exact commands and output.
Two small rewordings folded in: ErrNoServicePrincipal's message is now
"no such credential" (it also serves legacy kind='app' rows, where "no
such service credential" read oddly); the authstore.go doc comment above
scanCLITokenRows now says "fourteen columns" (was still "thirteen" after
rotated_at's addition).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UimU7fAfeAoBMm6HKCPwQ
4 files changed,
+321
−25
internal/console/authstore.go+18 −5internal/console/store.go+27 −0internal/console/svcprincipal.go+36 −17internal/console/svcprincipal_test.go+240 −3
diff --git a/internal/console/authstore.go b/internal/console/authstore.go| index dfb7bac77..0ac7da442 100644 |
| --- a/internal/console/authstore.go |
| +++ b/internal/console/authstore.go |
| @@ -113,6 +113,15 @@ type CLIToken struct { |
| ExpiresAt time.Time |
| LastUsed time.Time |
| RevokedAt time.Time // zero unless the credential has been revoked |
| + // RotatedAt is when this credential's CURRENT secret began: stamped at |
| + // mint and again at every RotateServicePrincipal, left alone by |
| + // everything else. CreatedAt is when the credential was granted and |
| + // never changes; RotatedAt is how a rotation policy answers "how old is |
| + // the secret in use right now" without that question destroying the |
| + // answer to "how long has this credential existed". Zero for every row |
| + // minted before this column existed, including every legacy kind='app' |
| + // row NewAppToken mints — those never rotate through this path. |
| + RotatedAt time.Time |
| } |
| // NeverExpires reports whether this credential has no expiry — true only |
| @@ -184,6 +193,7 @@ func (s *Store) migrateCLIAuth() error { |
| `ALTER TABLE cli_tokens ADD COLUMN role TEXT NOT NULL DEFAULT ''`, |
| `ALTER TABLE cli_tokens ADD COLUMN name TEXT NOT NULL DEFAULT ''`, |
| `ALTER TABLE cli_tokens ADD COLUMN revoked_at INTEGER NOT NULL DEFAULT 0`, |
| + `ALTER TABLE cli_tokens ADD COLUMN rotated_at INTEGER NOT NULL DEFAULT 0`, |
| } { |
| if _, err := s.db.Exec(stmt); err != nil && !strings.Contains(err.Error(), "duplicate column") { |
| return err |
| @@ -554,8 +564,8 @@ func (s *Store) CLITokenAddress(rawToken string, now time.Time) (string, bool) { |
| // scanCLITokenRows drains a `cli_tokens` listing query into CLIToken values. |
| // CLITokens (by address) and ServicePrincipals (by account) select the same |
| -// thirteen columns in the same order and need the same legacy-row |
| -// normalization — a kind='app' row's `role` column is ” in the database but |
| +// fourteen columns in the same order and need the same legacy-row |
| +// normalization — a kind='app' row's `role` column is empty in the database but |
| // IS the instance role (spec §5). This is the one place that normalization |
| // happens, so neither caller, nor any reader downstream of them, has to |
| // re-derive it. |
| @@ -564,9 +574,9 @@ func scanCLITokenRows(rows *sql.Rows) ([]CLIToken, error) { |
| var out []CLIToken |
| for rows.Next() { |
| var t CLIToken |
| - var created, expires, used, revoked int64 |
| + var created, expires, used, revoked, rotated int64 |
| if err := rows.Scan(&t.ID, &t.Address, &t.Label, &t.RemoteAddr, &t.Kind, &t.AccountID, &t.App, |
| - &t.Role, &t.Name, &created, &expires, &used, &revoked); err != nil { |
| + &t.Role, &t.Name, &created, &expires, &used, &revoked, &rotated); err != nil { |
| return nil, err |
| } |
| t.CreatedAt = time.Unix(created, 0).UTC() |
| @@ -577,6 +587,9 @@ func scanCLITokenRows(rows *sql.Rows) ([]CLIToken, error) { |
| if revoked != 0 { |
| t.RevokedAt = time.Unix(revoked, 0).UTC() |
| } |
| + if rotated != 0 { |
| + t.RotatedAt = time.Unix(rotated, 0).UTC() |
| + } |
| if t.Kind == cliKindApp { |
| t.Role = string(svcrole.Instance) |
| } |
| @@ -594,7 +607,7 @@ func scanCLITokenRows(rows *sql.Rows) ([]CLIToken, error) { |
| // keep that promise. |
| func (s *Store) CLITokens(address string) ([]CLIToken, error) { |
| rows, err := s.db.Query(`SELECT token_hash, address, label, remote_addr, kind, account_id, app, |
| -role, name, created_at, expires_at, last_used, revoked_at |
| +role, name, created_at, expires_at, last_used, revoked_at, rotated_at |
| FROM cli_tokens WHERE address = ? AND revoked_at = 0 ORDER BY created_at DESC, token_hash`, address) |
| if err != nil { |
| return nil, err |
diff --git a/internal/console/store.go b/internal/console/store.go| index babaa4f9a..8d950b2e4 100644 |
| --- a/internal/console/store.go |
| +++ b/internal/console/store.go |
| @@ -1373,6 +1373,33 @@ func (s *Store) MoveApp(app string, from, to int64) error { |
| if staleTokens > 0 { |
| return fmt.Errorf("console: the destination account still has app credentials for %q from a previous app — revoke them first", app) |
| } |
| + // A second, narrower collision the guard above cannot see: |
| + // cli_tokens_service_name only requires a live kind='service' row's NAME |
| + // to be unique ACCOUNT-WIDE — it does not care whether the row is bound |
| + // to this app or to none at all (NewServicePrincipal's own uniqueness |
| + // check is the same shape). So a service principal travelling with `app` |
| + // can collide with an ACCOUNT-SCOPED credential already sitting at the |
| + // destination under the same name, and the guard above cannot catch that |
| + // — it only counts rows still bound to `app` there, and the colliding |
| + // row is not one of them. Left uncaught, the UPDATE below would trip the |
| + // unique index inside this transaction — safe (it rolls back) but with |
| + // SQLite's raw constraint-violation text rather than a sentence in this |
| + // guard's own voice — and a generic name like "hub" or "ci" makes it |
| + // reachable in practice. |
| + var collidingName string |
| + err = tx.QueryRow(`SELECT dst.name FROM cli_tokens dst |
| +WHERE dst.kind = ? AND dst.account_id = ? AND dst.revoked_at = 0 |
| +AND dst.name IN ( |
| + SELECT src.name FROM cli_tokens src |
| + WHERE src.kind = ? AND src.account_id = ? AND src.app = ? AND src.revoked_at = 0 |
| +) |
| +LIMIT 1`, cliKindService, to, cliKindService, from, app).Scan(&collidingName) |
| + if err != nil && err != sql.ErrNoRows { |
| + return err |
| + } |
| + if err == nil { |
| + return fmt.Errorf("console: the destination account already holds a live service credential named %q — rename or revoke it first", collidingName) |
| + } |
| if _, err := tx.Exec(`UPDATE apps SET account_id = ? WHERE app = ? AND account_id = ?`, |
| to, app, from); err != nil { |
| return err |
diff --git a/internal/console/svcprincipal.go b/internal/console/svcprincipal.go| index 6fa759179..39b25983a 100644 |
| --- a/internal/console/svcprincipal.go |
| +++ b/internal/console/svcprincipal.go |
| @@ -61,7 +61,7 @@ var ErrServiceNameTaken = errors.New("console: a service credential of that name |
| // zero rows affected, not a database failure. A door needs to tell the two |
| // apart (404 vs 500) without string-matching an error, which an anonymous |
| // errors.New at each call site would force it to do. |
| -var ErrNoServicePrincipal = errors.New("console: no such service credential") |
| +var ErrNoServicePrincipal = errors.New("console: no such credential") |
| // CLIPrincipal resolves a bearer token to whichever kind of caller minted it. |
| // ok is false for unknown, revoked, expired tokens, and — for a kind='service' |
| @@ -124,11 +124,11 @@ FROM cli_tokens WHERE token_hash = ?`, hash). |
| // reason — the credential speaks for a service, but a human stays |
| // answerable for it. |
| // |
| -// name must be non-empty. ” is not a name a caller chose — it is the value |
| +// name must be non-empty. An empty name is not one a caller chose — it is the value |
| // every legacy kind='app' row already carries by default, and both |
| // RotateServicePrincipal and RevokeServicePrincipal refuse to match it for |
| // exactly that reason (a call with an empty name must never reach a legacy |
| -// row it did not name). Minting a kind='service' row with name=” would |
| +// row it did not name). Minting a kind='service' row with an empty name would |
| // create a credential neither of those doors could ever address again except |
| // by RevokeAccountToken's raw id, so the store refuses it at the door |
| // instead of producing an unaddressable row later. Grammar validation for |
| @@ -139,6 +139,10 @@ FROM cli_tokens WHERE token_hash = ?`, hash). |
| // NewAppToken documents: a production path that silently dies on day 91 is a |
| // worse failure than a long-lived credential whose rotation is one command. |
| // Revocation is the control. |
| +// |
| +// rotated_at is stamped equal to created_at at mint: the current secret |
| +// began exactly when the credential did. RotateServicePrincipal advances it |
| +// on every later rotation without touching created_at. |
| func (s *Store) NewServicePrincipal(accountID int64, name, app string, role svcrole.Role, mintedBy string, now time.Time) (string, error) { |
| if name == "" { |
| return "", errors.New("console: a service credential needs a name") |
| @@ -164,9 +168,9 @@ func (s *Store) NewServicePrincipal(accountID int64, name, app string, role svcr |
| return "", err |
| } |
| if _, err := tx.Exec(`INSERT INTO cli_tokens |
| -(token_hash, address, label, remote_addr, created_at, expires_at, kind, account_id, app, role, name) |
| -VALUES (?, ?, '', '', ?, 0, ?, ?, ?, ?, ?)`, |
| - hashToken(raw), mintedBy, now.Unix(), cliKindService, accountID, app, string(role), name); err != nil { |
| +(token_hash, address, label, remote_addr, created_at, rotated_at, expires_at, kind, account_id, app, role, name) |
| +VALUES (?, ?, '', '', ?, ?, 0, ?, ?, ?, ?, ?)`, |
| + hashToken(raw), mintedBy, now.Unix(), now.Unix(), cliKindService, accountID, app, string(role), name); err != nil { |
| return "", err |
| } |
| if err := tx.Commit(); err != nil { |
| @@ -186,7 +190,7 @@ VALUES (?, ?, '', '', ?, 0, ?, ?, ?, ?, ?)`, |
| // stays out of CLITokens. |
| func (s *Store) ServicePrincipals(accountID int64) ([]CLIToken, error) { |
| rows, err := s.db.Query(`SELECT token_hash, address, label, remote_addr, kind, account_id, app, |
| -role, name, created_at, expires_at, last_used, revoked_at |
| +role, name, created_at, expires_at, last_used, revoked_at, rotated_at |
| FROM cli_tokens WHERE account_id = ? AND kind IN (?, ?) AND revoked_at = 0 |
| ORDER BY created_at DESC, token_hash`, accountID, cliKindService, cliKindApp) |
| if err != nil { |
| @@ -197,15 +201,19 @@ ORDER BY created_at DESC, token_hash`, accountID, cliKindService, cliKindApp) |
| // RotateServicePrincipal mints a fresh secret for a live, NAMED credential |
| // and retires the old one in the same statement: same row, same name, same |
| -// role, same age — only the secret moves and last_used resets, because a |
| -// fresh secret has not been used yet. created_at is deliberately left alone: |
| -// overwriting it would make a credential granted a year ago read as newly |
| -// minted on the very page an owner uses to decide what is stale. Zero rows |
| +// role — only the secret moves, rotated_at advances to now, and last_used |
| +// resets, because a fresh secret has not been used yet. created_at is |
| +// deliberately left alone: overwriting it would make a credential granted a |
| +// year ago read as newly minted on the very page an owner uses to decide |
| +// what is stale. rotated_at is the answer to that question instead — "how |
| +// old is the secret in use right now" — which is why now is taken as a |
| +// parameter here rather than read from the clock inline: this write, like |
| +// every other Store method's, is testable without sleeping. Zero rows |
| // affected — including a call with name="" — is ErrNoServicePrincipal, which |
| // the door renders as 404. |
| // |
| -// name != ” guards against a legacy kind='app' row, whose name column is |
| -// always ”: without it, RotateServicePrincipal(acctID, "") would match that |
| +// The name != "" guard excludes a legacy kind='app' row, whose name column |
| +// is always empty: without it, RotateServicePrincipal(acctID, "") would match that |
| // row (or, if the account somehow held more than one, all of them) and |
| // rotate a credential the caller never named. |
| func (s *Store) RotateServicePrincipal(accountID int64, name string, now time.Time) (string, error) { |
| @@ -213,9 +221,9 @@ func (s *Store) RotateServicePrincipal(accountID int64, name string, now time.Ti |
| if err != nil { |
| return "", err |
| } |
| - res, err := s.db.Exec(`UPDATE cli_tokens SET token_hash = ?, last_used = 0 |
| + res, err := s.db.Exec(`UPDATE cli_tokens SET token_hash = ?, rotated_at = ?, last_used = 0 |
| WHERE kind = ? AND account_id = ? AND name = ? AND name != '' AND revoked_at = 0`, |
| - hashToken(raw), cliKindService, accountID, name) |
| + hashToken(raw), now.Unix(), cliKindService, accountID, name) |
| if err != nil { |
| return "", err |
| } |
| @@ -237,7 +245,7 @@ WHERE kind = ? AND account_id = ? AND name = ? AND name != '' AND revoked_at = 0 |
| // resolving it again, and so a later NewServicePrincipal minting the same |
| // name creates a NEW row rather than resurrecting this one's identity — |
| // the unique index (cli_tokens_service_name) is scoped to live rows for |
| -// exactly that reason. name != ” guards against a legacy kind='app' row for |
| +// exactly that reason. The same name != "" guard applies here, for |
| // the same reason RotateServicePrincipal needs it. |
| func (s *Store) RevokeServicePrincipal(accountID int64, name string, now time.Time) error { |
| res, err := s.db.Exec(`UPDATE cli_tokens SET revoked_at = ? |
| @@ -270,8 +278,19 @@ WHERE kind = ? AND account_id = ? AND name = ? AND name != '' AND revoked_at = 0 |
| // The broad query (no kind filter) is not a hazard: a person's cli_tokens |
| // row always carries account_id = 0 (see CLIToken), so an account-scoped |
| // revoke can never reach a human's terminal token — there is no account id |
| -// for it to match. |
| +// for it to match. That is also exactly why accountID == 0 is refused below |
| +// rather than merely producing a query that happens to match nothing: the |
| +// same fact that makes the broad query SAFE for every real caller makes a |
| +// zero-value accountID CATASTROPHIC rather than harmless — a caller that |
| +// reached this door with an unset or mis-looked-up account id, holding any |
| +// person's own token_hash as `id` (a listing id, not a secret — see |
| +// RevokeCLIToken), would revoke that PERSON's terminal token instead of a |
| +// credential the account actually owns. Refusing it here makes that |
| +// precondition structural, not a caller obligation. |
| func (s *Store) RevokeAccountToken(accountID int64, id string, now time.Time) error { |
| + if accountID == 0 { |
| + return ErrNoServicePrincipal |
| + } |
| res, err := s.db.Exec(`UPDATE cli_tokens SET revoked_at = ? |
| WHERE token_hash = ? AND account_id = ? AND revoked_at = 0`, now.Unix(), id, accountID) |
| if err != nil { |
diff --git a/internal/console/svcprincipal_test.go b/internal/console/svcprincipal_test.go| index d619049ce..a61f47105 100644 |
| --- a/internal/console/svcprincipal_test.go |
| +++ b/internal/console/svcprincipal_test.go |
| @@ -1,6 +1,7 @@ |
| package console |
| import ( |
| + "strings" |
| "testing" |
| "time" |
| @@ -236,7 +237,7 @@ func TestServicePrincipalsListsBothKinds(t *testing.T) { |
| } |
| // TestRotateAndRevokeIgnoreLegacyNamelessRows: a legacy kind='app' row's name |
| -// is ”, and RotateServicePrincipal/RevokeServicePrincipal must never treat |
| +// is empty, and RotateServicePrincipal/RevokeServicePrincipal must never treat |
| // that empty string as a name to match — that would let one call rotate or |
| // revoke an unrelated, unnamed credential. |
| func TestRotateAndRevokeIgnoreLegacyNamelessRows(t *testing.T) { |
| @@ -286,10 +287,10 @@ func TestRevokeAccountTokenScopesByAccountNotAddress(t *testing.T) { |
| } |
| } |
| -// TestNewServicePrincipalRejectsEmptyName: ” is not a name a caller chose — |
| +// TestNewServicePrincipalRejectsEmptyName: an empty name is not one a caller chose — |
| // it is the default every legacy kind='app' row already carries — and both |
| // RotateServicePrincipal and RevokeServicePrincipal refuse to match it, so a |
| -// kind='service' row minted with name=” would be unaddressable except by |
| +// kind='service' row minted with an empty name would be unaddressable except by |
| // RevokeAccountToken's raw id. The store refuses it at the door instead. |
| func TestNewServicePrincipalRejectsEmptyName(t *testing.T) { |
| s := openTestStore(t) |
| @@ -425,3 +426,239 @@ func TestMoveAppMovesServicePrincipalToo(t *testing.T) { |
| t.Errorf("AccountID = %d after move; want %d (the destination account)", p.AccountID, to.ID) |
| } |
| } |
| + |
| +// TestRenameAppRefusesOntoNameWithStaleServiceCredential pins the widened |
| +// half of F1 that TestRenameAppMovesServicePrincipalToo does not reach: the |
| +// stale-token COUNT guard itself. Reverting that guard's `kind IN (?, ?)` |
| +// back to `kind = ?` (cliKindApp alone) would leave this test's refusal |
| +// silently absent — the rename would succeed onto a name a live service |
| +// credential still holds. |
| +func TestRenameAppRefusesOntoNameWithStaleServiceCredential(t *testing.T) { |
| + s := openTestStore(t) |
| + now := time.Now() |
| + acct := mustAccount(t, s, "amadan") |
| + if _, err := s.CreateAppIn(acct.ID, "titogo", "paul@example.test", now); err != nil { |
| + t.Fatalf("CreateAppIn: %v", err) |
| + } |
| + if _, err := s.NewServicePrincipal(acct.ID, "hub", "titogo", svcrole.Publish, "paul@example.test", now); err != nil { |
| + t.Fatalf("NewServicePrincipal: %v", err) |
| + } |
| + // The credential above follows the rename (proven separately by |
| + // TestRenameAppMovesServicePrincipalToo), so "titogo" is now clean. A |
| + // SECOND credential, minted directly against the freed name with no live |
| + // app behind it, stands in for what a purged predecessor's app-kind |
| + // token has always left behind — the exact shape the guard's own doc |
| + // comment describes, and the only way to construct it without a purge |
| + // path this test does not otherwise need. |
| + if err := s.RenameAppIn(acct.ID, "titogo", "tito"); err != nil { |
| + t.Fatalf("RenameAppIn (move away): %v", err) |
| + } |
| + if _, err := s.NewServicePrincipal(acct.ID, "leftover", "titogo", svcrole.Operate, "paul@example.test", now); err != nil { |
| + t.Fatalf("NewServicePrincipal (leftover): %v", err) |
| + } |
| + if _, err := s.CreateAppIn(acct.ID, "second", "paul@example.test", now); err != nil { |
| + t.Fatalf("CreateAppIn (second): %v", err) |
| + } |
| + err := s.RenameAppIn(acct.ID, "second", "titogo") |
| + if err == nil { |
| + t.Fatal("RenameAppIn onto a name with a stale service credential succeeded; want a refusal") |
| + } |
| + if !strings.Contains(err.Error(), "app credentials from a previous app of that name") { |
| + t.Errorf("error = %q, want the stale-token guard's message", err.Error()) |
| + } |
| +} |
| + |
| +// TestMoveAppRefusesOntoNameWithStaleServiceCredential is MoveApp's twin of |
| +// the test above: the equivalent COUNT guard, at the destination account. |
| +func TestMoveAppRefusesOntoNameWithStaleServiceCredential(t *testing.T) { |
| + s := openTestStore(t) |
| + now := time.Now() |
| + from := mustAccount(t, s, "amadan") |
| + to := mustAccount(t, s, "other") |
| + if _, err := s.CreateAppIn(from.ID, "titogo", "paul@example.test", now); err != nil { |
| + t.Fatalf("CreateAppIn: %v", err) |
| + } |
| + // A leftover, purged-predecessor-shaped service credential already sits |
| + // at the DESTINATION under the same app name the move targets. |
| + if _, err := s.NewServicePrincipal(to.ID, "leftover", "titogo", svcrole.Operate, "paul@example.test", now); err != nil { |
| + t.Fatalf("NewServicePrincipal (leftover at destination): %v", err) |
| + } |
| + err := s.MoveApp("titogo", from.ID, to.ID) |
| + if err == nil { |
| + t.Fatal("MoveApp onto a destination with a stale service credential succeeded; want a refusal") |
| + } |
| + if !strings.Contains(err.Error(), "app credentials for") { |
| + t.Errorf("error = %q, want the stale-token guard's message", err.Error()) |
| + } |
| +} |
| + |
| +// TestMoveAppRefusesOntoNameCollisionWithAccountScopedCredential is C18: the |
| +// stale-token guard above only counts rows still bound to the app being |
| +// moved, so it cannot see an ACCOUNT-scoped credential already live at the |
| +// destination under the same name — a collision the unique index |
| +// (cli_tokens_service_name) would otherwise catch as a raw constraint |
| +// error. This is MoveApp's second, narrower guard. |
| +func TestMoveAppRefusesOntoNameCollisionWithAccountScopedCredential(t *testing.T) { |
| + s := openTestStore(t) |
| + now := time.Now() |
| + from := mustAccount(t, s, "amadan") |
| + to := mustAccount(t, s, "other") |
| + if _, err := s.CreateAppIn(from.ID, "titogo", "paul@example.test", now); err != nil { |
| + t.Fatalf("CreateAppIn: %v", err) |
| + } |
| + traveler, err := s.NewServicePrincipal(from.ID, "hub", "titogo", svcrole.Publish, "paul@example.test", now) |
| + if err != nil { |
| + t.Fatalf("NewServicePrincipal (source, app-scoped): %v", err) |
| + } |
| + resident, err := s.NewServicePrincipal(to.ID, "hub", "", svcrole.Operate, "paul@example.test", now) |
| + if err != nil { |
| + t.Fatalf("NewServicePrincipal (destination, account-scoped): %v", err) |
| + } |
| + if err := s.MoveApp("titogo", from.ID, to.ID); err == nil { |
| + t.Fatal("MoveApp onto a name-colliding destination succeeded; want a refusal") |
| + } else if !strings.Contains(err.Error(), "hub") || !strings.Contains(err.Error(), "already holds a live service credential") { |
| + t.Errorf("error = %q, want it to name the colliding credential", err.Error()) |
| + } |
| + // The refusal must be transactional: both credentials keep their |
| + // original scope, neither dropped nor merged. |
| + tp, ok := s.CLIPrincipal(traveler, now) |
| + if !ok || tp.AccountID != from.ID || tp.App != "titogo" { |
| + t.Errorf("traveling credential changed scope: %+v ok=%v", tp, ok) |
| + } |
| + rp, ok := s.CLIPrincipal(resident, now) |
| + if !ok || rp.AccountID != to.ID || rp.App != "" { |
| + t.Errorf("resident credential changed scope: %+v ok=%v", rp, ok) |
| + } |
| +} |
| + |
| +// TestRotateAdvancesRotatedAtLeavesCreatedAt is C19: rotated_at is when the |
| +// CURRENT secret began (stamped at mint and at every rotate); created_at is |
| +// when the credential was granted and never moves again. This is also the |
| +// third bullet of C21 read from the other side — reverting the created_at |
| +// preservation this test also covers would fail it. |
| +func TestRotateAdvancesRotatedAtLeavesCreatedAt(t *testing.T) { |
| + s := openTestStore(t) |
| + minted := time.Now() |
| + acct := mustAccount(t, s, "amadan") |
| + if _, err := s.NewServicePrincipal(acct.ID, "hub", "", svcrole.Publish, "paul@example.test", minted); err != nil { |
| + t.Fatalf("NewServicePrincipal: %v", err) |
| + } |
| + rotated := minted.Add(48 * time.Hour) |
| + if _, err := s.RotateServicePrincipal(acct.ID, "hub", rotated); err != nil { |
| + t.Fatalf("RotateServicePrincipal: %v", err) |
| + } |
| + toks, err := s.ServicePrincipals(acct.ID) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(toks) != 1 { |
| + t.Fatalf("ServicePrincipals = %d rows, want 1", len(toks)) |
| + } |
| + tok := toks[0] |
| + if tok.CreatedAt.Unix() != minted.Unix() { |
| + t.Errorf("CreatedAt = %v after rotate, want the original mint time %v", tok.CreatedAt, minted) |
| + } |
| + if tok.RotatedAt.Unix() != rotated.Unix() { |
| + t.Errorf("RotatedAt = %v after rotate, want the rotate time %v", tok.RotatedAt, rotated) |
| + } |
| +} |
| + |
| +// TestNewServicePrincipalStampsRotatedAtAtMint: the current secret begins |
| +// exactly when the credential does, so a freshly minted row's rotated_at |
| +// must equal its created_at, not read as 0 (which would misrender as |
| +// "never rotated" rather than "rotated when granted"). |
| +func TestNewServicePrincipalStampsRotatedAtAtMint(t *testing.T) { |
| + s := openTestStore(t) |
| + now := time.Now() |
| + acct := mustAccount(t, s, "amadan") |
| + if _, err := s.NewServicePrincipal(acct.ID, "hub", "", svcrole.Publish, "paul@example.test", now); err != nil { |
| + t.Fatalf("NewServicePrincipal: %v", err) |
| + } |
| + toks, err := s.ServicePrincipals(acct.ID) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(toks) != 1 { |
| + t.Fatalf("ServicePrincipals = %d rows, want 1", len(toks)) |
| + } |
| + tok := toks[0] |
| + if tok.RotatedAt.Unix() != tok.CreatedAt.Unix() { |
| + t.Errorf("RotatedAt = %v, CreatedAt = %v; want them equal at mint", tok.RotatedAt, tok.CreatedAt) |
| + } |
| + if tok.RotatedAt.Unix() != now.Unix() { |
| + t.Errorf("RotatedAt = %v; want the mint time %v", tok.RotatedAt, now) |
| + } |
| +} |
| + |
| +// TestRevokeAccountTokenRefusesZeroAccount is C20: the same fact that makes |
| +// RevokeAccountToken's kind-less query safe for every real caller — a |
| +// person's row always carries account_id = 0 — makes accountID == 0 |
| +// catastrophic for a caller that reaches this door by mistake. Proven from |
| +// the dangerous side: pass 0 and a real person's own listing id, and assert |
| +// their token survives. |
| +func TestRevokeAccountTokenRefusesZeroAccount(t *testing.T) { |
| + s := openTestStore(t) |
| + now := time.Now() |
| + raw := mintUserToken(t, s, "paul@example.test", now) |
| + id := hashToken(raw) |
| + if err := s.RevokeAccountToken(0, id, now); err == nil { |
| + t.Fatal("RevokeAccountToken(accountID=0) succeeded; want a refusal") |
| + } |
| + if _, ok := s.CLITokenAddress(raw, now); !ok { |
| + t.Error("a person's token was revoked via RevokeAccountToken(0, ...)") |
| + } |
| +} |
| + |
| +// TestRevokeAccountTokenSoftRevokesNotDeletes pins the Risk A resolution |
| +// from the reviewer's side: a DELETE (the pre-fix behaviour) would leave |
| +// zero rows and fail this test. |
| +func TestRevokeAccountTokenSoftRevokesNotDeletes(t *testing.T) { |
| + s := openTestStore(t) |
| + now := time.Now() |
| + acct := mustAccount(t, s, "amadan") |
| + app, err := s.NewAppToken(acct.ID, "titogo", "paul@example.test", "router", now) |
| + if err != nil { |
| + t.Fatalf("NewAppToken: %v", err) |
| + } |
| + id := hashToken(app) |
| + if err := s.RevokeAccountToken(acct.ID, id, now); err != nil { |
| + t.Fatalf("RevokeAccountToken: %v", err) |
| + } |
| + var count int |
| + var revokedAt int64 |
| + if err := s.db.QueryRow(`SELECT COUNT(*), COALESCE(MAX(revoked_at), 0) FROM cli_tokens WHERE token_hash = ?`, id). |
| + Scan(&count, &revokedAt); err != nil { |
| + t.Fatal(err) |
| + } |
| + if count != 1 { |
| + t.Fatalf("row count after RevokeAccountToken = %d, want 1 (a DELETE would leave 0)", count) |
| + } |
| + if revokedAt == 0 { |
| + t.Error("revoked_at is still 0 after RevokeAccountToken") |
| + } |
| +} |
| + |
| +// TestRevokedServicePrincipalAbsentFromCLITokens pins the Risk B resolution: |
| +// dropping CLITokens' revoked_at = 0 filter would leave this test's |
| +// assertion false — the revoked row would still render on the minter's own |
| +// settings page. |
| +func TestRevokedServicePrincipalAbsentFromCLITokens(t *testing.T) { |
| + s := openTestStore(t) |
| + now := time.Now() |
| + acct := mustAccount(t, s, "amadan") |
| + if _, err := s.NewServicePrincipal(acct.ID, "hub", "", svcrole.Publish, "paul@example.test", now); err != nil { |
| + t.Fatalf("NewServicePrincipal: %v", err) |
| + } |
| + if err := s.RevokeServicePrincipal(acct.ID, "hub", now); err != nil { |
| + t.Fatalf("RevokeServicePrincipal: %v", err) |
| + } |
| + toks, err := s.CLITokens("paul@example.test") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, tok := range toks { |
| + if tok.Name == "hub" { |
| + t.Fatalf("revoked service principal %q still present in CLITokens: %+v", tok.Name, tok) |
| + } |
| + } |
| +} |