fix(api): reject API key auth for deactivated user accounts#9225
Conversation
The custom API key authentication only verified that the APIToken row was active and unexpired; it never checked the owning user's is_active flag. DRF's IsAuthenticated only checks user.is_authenticated (always True for a real User), so a user whose account was deactivated could keep using a previously issued API key indefinitely. Add user__is_active=True to the validate_api_token() lookup so a token tied to a disabled account is treated as invalid (a generic AuthenticationFailed, avoiding account-state disclosure). Applied to both the external API middleware (plane/api) and the identical, currently unused copy in plane/app to prevent the gap from being reintroduced. Adds unit coverage on validate_api_token and an end-to-end contract test proving GET /api/v1/users/me/ is denied once the account is deactivated.
sriramveeraghanta
requested review from
dheeru0198 and
pablohashescobar
as code owners
|
No React Doctor issues found. 🎉 Reviewed by React Doctor for commit |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR enforces that API tokens can only authenticate users with active accounts. The middleware constraint is added to both API implementations, rejecting tokens from deactivated users. Unit and contract tests verify the authentication fails once an account is deactivated. ChangesAPI Token Active User Check
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR closes a critical authentication bypass in the external API key auth flow by ensuring API keys are rejected when the owning user account has been deactivated (user.is_active = False). The change is implemented as an atomic filter in the token lookup (preventing account-state disclosure) and is covered by both unit- and endpoint-level contract tests.
Changes:
- Add
user__is_active=TruetoAPIKeyAuthentication.validate_api_token()token validation queries (in both middleware copies). - Add unit tests covering
validate_api_token()for active vs deactivated users. - Add contract tests proving end-to-end denial on a live endpoint when the user is deactivated.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/api/plane/api/middleware/api_authentication.py | Reject API key auth if the token’s owning user is deactivated (user__is_active=True). |
| apps/api/plane/app/middleware/api_authentication.py | Apply the same deactivated-user rejection to the mirrored middleware for parity. |
| apps/api/plane/tests/unit/middleware/test_api_authentication.py | Unit coverage: active user authenticates; deactivated user raises AuthenticationFailed. |
| apps/api/plane/tests/contract/api/test_authentication.py | Contract coverage: /api/v1/users/me/ succeeds for active users and is denied after deactivation. |
Description
The external API's custom API key authentication (
APIKeyAuthentication.validate_api_token) only verified that theAPITokenrow itself was active and unexpired — it never checked the owning user'sis_activeflag. DRF'sIsAuthenticatedonly checksuser.is_authenticated, which is alwaysTruefor a realUserinstance regardless ofis_active. As a result, a user whose account was deactivated by an administrator could continue to use a previously issued API key indefinitely, bypassing the deactivation.This was reported via a bug-bounty submission (rated Critical). Deactivation does not revoke existing API tokens (the self-service deactivation flow deletes sessions but leaves tokens active), so the auth layer was the only gate — and it was missing the check.
Fix: add
user__is_active=Trueto thevalidate_api_token()lookup so a token tied to a disabled account fails the query and is rejected with the existing genericAuthenticationFailed("Given API token is not valid"). Folding it into the query keeps the change atomic (single query, matches existing idiom) and avoids disclosing account state to the caller.Applied to both:
plane/api/middleware/api_authentication.py— the external REST API (the reachable vulnerability, wired intoBaseAPIView).plane/app/middleware/api_authentication.py— an identical, currently-unused copy, fixed for parity so the gap can't be silently reintroduced if it gets wired up later.Note: because
APIKeyAuthenticationsets noWWW-Authenticateheader, DRF surfaces the denial as 403 Forbidden rather than 401 — either way, access is denied.Type of Change
Screenshots and Media (if applicable)
Test Scenarios
Added automated coverage (written test-first, both verified to fail before the fix):
plane/tests/unit/middleware/test_api_authentication.py):validate_api_tokenauthenticates a token for an active user (unchanged behavior).validate_api_tokenraisesAuthenticationFailedonce the owning user is deactivated.plane/tests/contract/api/test_authentication.py):GET /api/v1/users/me/with their API key (200).GET /api/v1/users/me/with the same key (403/401) — previously returned 200, demonstrating the bypass is closed end-to-end.Manual scenario to confirm: generate an API key for a user, deactivate the account, then call any
/api/v1/...endpoint withX-Api-Key— the request should be rejected instead of succeeding.References
user.is_activecheck).Nonecache key) was assessed as not exploitable — throttles run after permission checks and django_redis tolerates aNonekey — so no change was made for it.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Security
Tests