chore(auth): role-model leftovers, inline loadUser copies, inert constant-time compares #313

Summary

Four low-severity findings in internal/auth that a maintainer would clear in one pass over the package. Dead symbols left over from the removed role model (LeanerCloud/cloud-commitments-cli#907/#940) and superseded paths widen the surface every mock has to implement and keep the pre-LeanerCloud/cloud-commitments-cli#907 vocabulary alive. Five callers reimplement the lookup-or-not-found path that loadUser already normalises, DeleteUser inlines checkLastAdminConstraint, sessionSuffix is computed twice and redactEmail exists three times with different masking rules; divergent copies are where the API-key-path-checks-Active-but-session-path-does-not asymmetry (LeanerCloud/cloud-commitments-cli#1541) came from. Four post-fetch subtle.ConstantTimeCompare calls compare a value against the SHA-256 the row was just selected by, so they can only fail on a store bug while their comments claim to close a timing oracle they do not close. Finally, permissionCoveredBy's return true is never reached in the suite, so the carry-through direction of the carve-out ceiling is unasserted.

Location

All at 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd.

• A03-022 dead role-model leftovers: RoleAdmin/RoleUser/RoleReadOnly at internal/auth/types.go:321-325 (zero references outside the declaration); Service.UpdateLastUsed at service_apikeys.go:442 and ResolveGCPCredentials at credentials/resolver.go:305 (no non-test caller); EmailSenderInterface.SendWelcomeEmail at interfaces.go:71 (never invoked by the auth service, still carries a role parameter); User.Salt written as "" at service_user.go:744, service_password.go:251, :487 and read by nothing. Delete the constants, the two functions, the interface method and its mocks, and the Salt field once the column is dropped.
• A03-023 repeated helpers: loadUser exists at service_user.go:279-291 while UpdateUserProfile (:660-669), ChangePassword (service_password.go:213-222), CreateAPIKey, ListUserAPIKeys, authorizeAPIKeyAccess and lookupAPIKeyUser (service_apikeys.go:44-53, 140-149, 190-199, 262-271) inline it; DeleteUser (service_user.go:625-633) repeats checkLastAdminConstraint (:568-577); sessionSuffix computed twice (credentials/resolver.go:185-188, :269-272); redactEmail defined in service_password.go:496, api/handler_auth.go:631, email/sender.go:665 plus redactEmailLocal in api/handler_notifications.go:132; MatchesAccount (types.go:199-212) and AccountScope.Allows (account_scope.go:104-117) are the same loop. Route the lookups through loadUser (with an activeOnly variant for the API-key path), call checkLastAdminConstraint from DeleteUser, extract one roleSessionName(accountID), keep one redactEmail in pkg/logging.
• A03-020 inert constant-time compares at service.go:334, service_apikeys.go:303, service_password.go:424 and :458, each comparing the row's key to the same hash it was selected by (store_postgres.go:670, service_apikeys.go:289-292, service_password.go:444-446). Remove the four compares and the two tests that pin them (service_test.go:194-220, service_password_test.go:624-648), or replace the comment with the true statement: the lookup key is a SHA-256 of a 256-bit random token, so equality timing reveals nothing usable. Note chore(auth): medium/low findings from the 2026-07-28 full review #104's "verified sound" section lists these compares as a positive; that assessment should be corrected alongside.
• A16-014 permissionCoveredBy at group_ceiling.go:376 never returns true in the suite. Its single call site is inside the adminCarvedOuts branch (group_ceiling.go:86), so an always-false return would make any edit to a group that holds one of the separation-of-duties reserved pairs unsaveable, and the suite stays green. Add a case to group_ceiling_permissions_test.go where an actor whose own permissions do not reach the ceiling resubmits a group's existing carved-out permission unchanged, and assert the update succeeds. sec(auth): execute:* wildcard bypasses the admin money-verb carve-out at grant and check cloud-commitments-cli#1901 touches the same function, so land the test with or after that fix.

Failure scenario

None of these is a live defect at this commit. The cost is drift: the next contributor copies one of the inline lookups (as lookupAPIKeyUser did), trusts a timing-oracle comment that is false, or breaks the carve-out carry-through without a failing test.

Evidence

// Predefined roles.
const (
    RoleAdmin    = "admin"
    RoleUser     = "user"
    RoleReadOnly = "readonly"
)

// Constant-time comparison after fetch to close the SQL-equality timing oracle.
// GetSession does `WHERE token = $1` which is not constant-time in PostgreSQL;
// align with ValidateUserAPIKey / validateResetToken (issue LeanerCloud/cloud-commitments-cli#392 PR LeanerCloud/cloud-commitments-cli#837).
if subtle.ConstantTimeCompare([]byte(session.Token), []byte(hashedToken)) != 1 {
    return nil, fmt.Errorf("session not found")
}

Suggested fix

Per checkbox above. One PR per checkbox is fine; none needs a new abstraction.

Found by the 2026-09-02 codebase audit, findings A03-022, A03-023, A03-020, A16-014, each reported by one reviewer and independently confirmed by a second. Full report: docs/audits/codebase-audit-2026-09-02.md.