fix(auth): api credentials survived revocation and creator removal - #246
Merged
Merged
Conversation
Three defects on the API credential authentication path, all of which left a credential live that an operator had every reason to believe was dead. 1. Soft-deleted credentials still authenticated. AuthenticateOnceWithBasicAuth looks the credential up with withoutGlobalScopes(), which strips SoftDeletingScope along with ExpiryScope. Expiry was re-applied in PHP; soft-deletion never was. A credential the console reports as "Deleted" therefore kept authenticating indefinitely -- and Delete is the only revocation most operators ever perform. Now rejected with the generic "not valid" 401, before the OPTIONS shortcut so a revoked key cannot seed api key session context on a preflight either. 2. Authentication was fail-open when the creating user was gone. A credential carries no identity of its own; it acts as the user that created it. When User::find() no longer resolved that user, the is_admin guard was skipped but setSession() still returned true. Authorization degraded safely -- a null user fails every group and admin check -- but authentication did not, so the key kept working on every read endpoint and every ungated write. Off-boarding a person did not revoke the keys they had created. Auth::setSession() now returns false in that case and the middleware answers 401. Note User is pinned to the mysql connection, so this resolves against the authoritative store for sandbox credentials too. 3. "Expire immediately" did not expire the credential. ApiCredential::setExpiresAtAttribute() maps 'immediately' to Carbon::now(), but Expirable::hasExpired() used a strict `<`, so now() < now() was false. ExpiryScope already treats an exactly-now expiry as expired (it keeps a row only while expires_at > now()), so the two disagreed on that boundary. hasExpired() is now inclusive, which makes the trait and the scope agree. Reported downstream against 1.6.35 and verified against main. Private tracker: FliitAU/fliit-extension#2212
…aned The coverage gate went red on AuthenticateOnceWithBasicAuth (57/58 statements): the early `return;` in bindUserResolver() was previously reached only because the sandbox fixture seeded sandbox-user-1 on the sandbox connection alone, so User::find() came back null on mysql. Correcting that fixture to mirror the real system — User is pinned to mysql and sandbox:sync copies mysql to sandbox — left the guard unexercised. bindUserResolver() is protected static, so a downstream middleware subclass can call it with arguments neither in-tree call site produces. Cover the guard directly through a harness subclass rather than reaching for @codeCoverageIgnore, asserting both halves refuse to bind and that the positive case still does.
Open
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev-v1.6.60 #246 +/- ##
===============================================
Coverage 100.00% 100.00%
- Complexity 6730 6732 +2
===============================================
Files 397 397
Lines 22448 22455 +7
===============================================
+ Hits 22448 22455 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three defects on the API credential authentication path in
AuthenticateOnceWithBasicAuth/Auth::setSession()/Expirable. Each one leaves a credential live that an operator has good reason to believe is dead. Taken together, a stock Fleetbase console has no working way to revoke an API credential: Delete doesn't revoke, and "expire immediately" doesn't expire.Reported by a downstream deployment against 1.6.35; every reference below re-verified against
main(1.6.59). Private tracker:FliitAU/fliit-extension#2212.1. Soft-deleted credentials still authenticate
AuthenticateOnceWithBasicAuth.php:63-66looks the credential up withwithoutGlobalScopes(), which strips both ofApiCredential's global scopes —SoftDeletingScope(from the baseFleetbase\Models\Model) andExpiryScope(fromExpirable). Expiry is then re-applied in PHP at:94viahasExpired(). Soft-deletion never was.So a credential the console reports as Deleted keeps authenticating indefinitely, while the console hides the row from the UI. Delete is the only revocation most operators ever perform.
This is not theoretical: a downstream production database held a credential soft-deleted on 2026-06-12 with
expires_atnull, still authenticating 48 days later on stock behaviour. It was created, deleted as a mistake, and recreated under the same name — and credential names are not unique, so the console list gave the operator no way to tell the two rows apart.Fix: reject
trashed()credentials with the genericnot valid401, placed before theOPTIONSshortcut so a revoked key cannot seed api key session context on a preflight either. The message is deliberately not distinct from "no such key" so a caller cannot probe for revoked-but-real keys.2. Authentication is fail-open when the creating user is gone
Auth::setSession()atsrc/Support/Auth.php:63-78. An API credential carries no identity of its own — it acts as the user that created it. WhenUser::find()no longer resolves that user (deleted, or soft-deleted on off-boarding), theif ($user)guard is skipped sois_adminis never set — butsetSession()still returnstrue.Authorization degrades safely: a null user fails every group and admin check. Authentication does not. The key keeps working on every read endpoint and every ungated write. Off-boarding a person therefore does not revoke the authority of the keys they created.
Fix:
setSession()returnsfalsewhen the credential's user does not resolve, and the middleware answers a clean 401 — the same shape an expired credential gets. The session is no longer half-populated on the way out, andtrackLastUsed()is not called for a rejected request.Worth noting for review:
Userdeclaresprotected $connection = 'mysql'(src/Models/User.php:76), andsandbox:syncmirrorsmysql → sandbox, so production is authoritative for users. Sandbox credentials therefore resolve their creator against the same authoritative store and are unaffected.tests/Unit/Http/MiddlewareContractsTest.phppreviously seededsandbox-user-1only on the sandbox connection, which did not reflect that; the fixture now mirrors it tomysqlas the real system does, and the sandbox secret-key test still passes.3. "Expire immediately" does not expire the credential
ApiCredential::setExpiresAtAttribute()maps the console's'immediately'option toCarbon::now()(src/Models/ApiCredential.php:144), butExpirable::hasExpired()used a strict<(src/Traits/Expirable.php:91) —now() < now()is false.ExpiryScopealready disagreed with it: it keeps a row only whileexpires_at > now()(src/Scopes/ExpiryScope.php:26), i.e. it treats an exactly-now expiry as expired. The trait and the scope were on opposite sides of the same boundary.Fix:
hasExpired()is now inclusive (<=), which makes the two agree and makes "expire this key right now" actually take effect.Not addressed here
Auth::setSession()derivesis_adminfrom whoever clicked "Create", so every key an admin creates is a full-admin key regardless of its name. Least privilege is only reachable indirectly, by scoping the creator into a dedicated service user.ApiCredentialalready usesHasPoliciesandHasPermissions, so the plumbing partly exists but nothing on the auth path consults it. That's a feature, not a fix, and wants a design discussion — happy to open a separate issue.null. That one is indev-engine(api-credential.jsdeclares@attr('date') expires_atwhile the form assigns relative strings), not this repo.Testing
php vendor/bin/pest— 1422 passed, 0 failed (9966 assertions).php-cs-fixerclean.New coverage:
last_used_atwriteOPTIONSpreflightlast_used_atwriteAuth::setSession()returnsfalsefor an orphaned credential (unit level,AuthSupportTest)hasExpired()is true at an exactly-now expiry (LifecycleTraitsTest)Compatibility
Auth::setSession()can now returnfalsefor anApiCredentialwhose user is gone. The only in-tree caller that passes anApiCredentialis this middleware; theUserbranch is untouched. Any deployment currently relying on credentials outliving their creator will see those keys start returning 401 — which is the intent.