Simplify Auth0 JWT validation and remove excessive logging

[JSv4]

Summary

This PR significantly simplifies Auth0 JWT token validation logic and removes excessive debug/info logging throughout the authentication stack. The changes improve code maintainability, reduce noise in logs, and make the authentication flow more straightforward.

Key Changes

Auth0 JWT Validation (config/graphql_auth0_auth/utils.py)

• Removed verbose logging: Eliminated debug logs that logged token prefixes, function entry/exit points, and intermediate processing steps
• Simplified jwt_auth0_decode(): Removed try-catch wrapper and extensive debug logging; now directly returns decoded token
• Simplified get_payload(): Consolidated exception handling to catch jwt.InvalidTokenError (which covers DecodeError, InvalidSignatureError, MissingRequiredClaimError, etc.) instead of handling each separately
• Improved error messages: Made log messages more concise and meaningful (e.g., "JWT validation failed" instead of function-prefixed messages)

Boolean Claim Parsing (config/graphql_auth0_auth/utils.py)

• Stricter validation: _parse_boolean_claim() now only accepts JSON booleans and canonical "true"/"false" strings
• Removed permissive parsing: No longer accepts "yes"/"no"/"1"/"0" or numeric values (0/1/0.0/1.0)
• Fail-closed design: Misconfigured Auth0 Actions that emit non-canonical values will now fail loudly rather than silently working with unexpected formats

Admin Claims Sync

• Simplified logic: Removed redundant logging and streamlined sync_admin_claims_from_payload()
• Cleaner code: Removed unnecessary intermediate variables and comments

Auth0 Token Management (opencontractserver/users/tasks.py)

• Added HTTP timeout: All outbound Auth0 requests now have a 10-second timeout to prevent worker hangs
• Improved token refresh logic: Simplified sync_remote_user() and ensure_valid_auth0_token() to use a single live token query instead of deleting all tokens and refreshing
• Better error handling: Added proper exception handling for network failures and malformed responses
• Atomic token persistence: Used transaction.atomic() with row-level locking to prevent race conditions when multiple workers fetch tokens simultaneously
• Cleaner data application: apply_data_to_user() now uses .get() with defaults instead of direct key access

Authentication Backends

• Simplified Auth0RemoteUserJSONWebTokenBackend: Removed excessive logging; now re-raises both JSONWebTokenExpired and JSONWebTokenError for proper GraphQL error handling
• Clarified Auth0AdminBackend: Made authenticate() a no-op (only get_user() is used for session rehydration); added documentation explaining why credential verification happens in the view, not the backend

JWT Utilities (config/jwt_utils.py)

• Removed debug logging: Eliminated verbose logging from token validation functions
• Cleaner error handling: Simplified exception handling paths

Frontend Changes

• Fixed auth header format: Changed from JWT to Bearer token scheme in useAnnotationImages.tsx
• Removed debug logs: Removed console.log statements from frontend/src/index.tsx
• Improved error link: Added comments explaining token expiration detection logic in errorLink.ts
• Better logout flow: Clear "previously logged in" hint from localStorage on logout to optimize next login

Database & Constants

• Removed unused column: Added migration to drop Auth0APIToken.auth0_Response (duplicated data)
• Removed unused constant: Deleted TOKEN_LOG_PREFIX_LENGTH constant that was only used for debug logging

Tests

• Updated test expectations: Modified test_admin_auth.py to reflect stricter boolean parsing (numeric values no longer accepted)
• Simplified test cases: Consolidated tests for non-canonical string values and numeric values
• Updated backend tests: Changed expectations for Auth0AdminBackend.authenticate() to be a no-op

Notable Implementation Details

1. **Fail-

https://claude.ai/code/session_019RStWzVYsvjGNwkBz5vQrh

[claude]

Code Review: PR #1499 — Simplify Auth0 JWT validation and remove excessive logging

Overview

This is a well-motivated cleanup PR that achieves real improvements: tighter security posture, better operational resilience, and meaningfully less noise. The changes are mostly correct and the architecture decisions (fail-closed boolean parsing, no-op authenticate(), Bearer scheme, sessionStorage for redirects) are sound. A few items deserve attention before merge.

---

Issues to Address

🔴 assert for runtime type check — stripped in optimized mode

File: config/graphql_auth0_auth/utils.py — jwt_auth0_decode()

assert isinstance(public_key, RSAPublicKey)

assert is a no-op when Python is run with -O (optimize flag). If RSAAlgorithm.from_jwk ever returns a private-key object (it shouldn't from a JWKS endpoint, but it's not guaranteed by the type stubs), the assertion is silently bypassed in optimized production builds and jwt.decode receives an unexpected type. Replace with an explicit check:

if not isinstance(public_key, RSAPublicKey):
    raise jwt.InvalidTokenError("JWKS returned unexpected key type")

🔴 Privilege changes are no longer audited

File: config/graphql_auth0_auth/utils.py — sync_admin_claims_from_payload()

The old code logged at INFO whenever is_staff or is_superuser was changed by a JWT claim sync:

logger.info("Synced is_staff=%s for user %s", is_staff_claim, user.username)

Both log lines were removed. If a user's privileges are silently elevated or revoked via a misconfigured Auth0 Action, there is now no audit trail in the application logs. These are low-volume events (once per ADMIN_CLAIMS_CACHE_TTL per user) so the noise argument doesn't apply. Restore them.

🟡 JWT → Bearer scheme change needs cross-cutting verification

File: frontend/src/components/annotator/hooks/useAnnotationImages.tsx

-  headers["Authorization"] = `JWT ${token}`;
+  headers["Authorization"] = `Bearer ${token}`;

This is correct for Auth0 (RFC 6750 Bearer). But the same Authorization header is sent to Django REST Framework endpoints (the annotation image endpoint is REST, not GraphQL). Verify that the DRF DEFAULT_AUTHENTICATION_CLASSES in settings accepts Bearer tokens and not just the JWT prefix that graphql-jwt uses. If both auth paths are active, a single header change here may silently break non-Auth0 deployments.

🟡 In-function import adds per-call overhead

File: config/graphql_auth0_auth/utils.py — jwt_auth0_decode()

from cryptography.hazmat.primitives.asymmetric.rsa import RSAPublicKey

This is inside the function body on every token decode. Move it to the module-level import block. The comment explains it's for type-stub narrowing, but that's a static analysis concern — the runtime import is unconditional.

🟡 ensure_valid_auth0_token blocks a Celery worker

File: opencontractserver/users/tasks.py

return get_new_auth0_token.delay().get()

.delay().get() dispatches a task and then blocks the current worker thread waiting for the result. Under a prefork worker pool with all workers busy, this deadlocks. This was present before this PR, but since tasks.py is being substantially refactored, it's worth fixing: call get_new_auth0_token() directly (it's a plain function at that point) or use apply() instead of delay().get().

---

Minor Notes

• CORS_ORIGIN_WHITELIST removal (config/settings/production.py): Correct — django-cors-headers ≥ 3.0 uses CORS_ALLOWED_ORIGINS. The duplicate is dead config.

• GraphQLJWTTokenAuthMiddleware alias removal (config/websocket/middleware.py): Per CLAUDE.md "no dead code" policy, correct. Confirm the alias isn't referenced in any deployment ASGI routing file or external documentation before merging.

• sessionStorage for admin redirect URL (templates/admin/auth0_login.html): Strictly better than localStorage — cleared on tab close, doesn't persist a stale next URL across sessions. Good change.

• Uninitialized user variable (config/graphql_auth0_auth/utils.py — get_auth0_user_from_token()): All branches either assign user or return None early, so this is safe at runtime. But static analysis tools will flag it. A user: Optional[UserModel] = None initializer before the if block costs nothing and silences the warning.

• expiration_Date now timezone-aware (opencontractserver/users/tasks.py): datetime.datetime.now(datetime.timezone.utc) is correct; the original naive comparison against a timezone-aware DB field was a latent bug. Good fix.

• URL-encoding auth0_Id in API path (tasks.py — get_user_details_async()): urllib.parse.quote(auth0_Id, safe="") is the right fix — Auth0 sub values contain | which is not valid unescaped in a URL path segment.

---

What's Working Well

• Fail-closed boolean parsing: Refusing "yes"/"no"/"1"/"0" and numeric coercion is the right call. A misconfigured Action now fails loudly instead of silently granting privileges.
• authenticate() as a documented no-op: The security argument in the docstring is correct and well-explained. Accepting auth0_user_id without proof of token possession would be an authentication bypass.
• HTTP timeouts on all Auth0 calls: Prevents workers from hanging indefinitely on network failures.
• transaction.atomic() with select_for_update(): Handles the common concurrent-token-creation race correctly.
• JSONWebTokenError now propagates from Auth0RemoteUserJSONWebTokenBackend: Returning None for a tampered token silently downgrades the request to anonymous; raising lets the GraphQL layer return a structured error. The test update reflects this correctly.

[codecov]

Codecov Report

❌ Patch coverage is 89.02439% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...pencontractserver/templates/admin/auth0_login.html	0.00%	4 Missing ⚠️
frontend/src/components/layout/useNavMenu.ts	0.00%	2 Missing ⚠️
opencontractserver/users/tasks.py	97.22%	2 Missing ⚠️
...components/annotator/hooks/useAnnotationImages.tsx	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude]

Code Review — PR #1499: Simplify Auth0 JWT validation and remove excessive logging

Overall this is a well-structured cleanup with several genuine security and reliability improvements. The signal-to-noise ratio across the auth stack is meaningfully better, and the deadlock fix in ensure_valid_auth0_token alone is worth the PR. A few points worth discussing before merge:

---

Correctness / Potential Bugs

1. Auth0RemoteUserJSONWebTokenBackend.get_user() — catch-all removed

The old code had except Exception as e: logger.error(...); return None. The new code only catches UserModel.DoesNotExist. Any unexpected database error during session rehydration will now propagate rather than gracefully returning an anonymous user. In practice Django's AuthenticationMiddleware does not catch arbitrary exceptions from get_user(), so this would surface as a 500. This is arguably more correct (hiding real errors behind None is bad), but it's a silent behaviour change worth flagging.

2. _parse_boolean_claim — implicit fall-through for integers

After removing the int/float branch, the function's control flow for an integer value like 42 is:

• Not bool → skip
• Not str → skip
• Not None → skip
• No final return visible in the diff

If there is no catch-all return False, False at the end of the function, Python will implicitly return None, and the tuple-unpack in _normalize_admin_claim will raise TypeError. test_numeric_values_rejected must be green for merge — please confirm CI passes on it. If the final return is there but not visible in the diff context, ignore this.

3. select_for_update() → delete() ordering in get_new_auth0_token

with transaction.atomic():
    Auth0APIToken.objects.select_for_update().all().delete()
    Auth0APIToken.objects.create(...)

SELECT FOR UPDATE then DELETE is the right pattern on PostgreSQL — the lock prevents two concurrent workers from each inserting after the other's delete. Worth noting that if the table is empty when both workers enter, they race to acquire the lock on zero rows (both succeed immediately) and both create a new row. The net result is two rows, both valid, neither is a problem since ensure_valid_auth0_token takes the freshest. This is fine in practice, but it's worth a comment explaining why the double-insert is harmless.

---

Security

4. Auth0AdminBackend.authenticate() is now a no-op — view tests missing

Making authenticate() a no-op is the correct design (the docstring explains why well). But this PR removes all the existing authenticate-path tests and replaces them with a single "returns None for everything" test. That's fine. What's missing is a test of the admin login view path: that login(request, user, backend='config.admin_auth.backends.Auth0AdminBackend') is called correctly after JWT validation, and that the session is properly populated. Without a view-level integration test the no-op backend change carries some risk — if the view was relying on authenticate(request, auth0_user_id=...) (which now returns None) to write the user into the session, admin login silently breaks. Recommend adding at minimum a smoke-test of the happy path.

5. _parse_boolean_claim strictness — breaking change for existing deployments

The fail-closed design is correct for new setups. However, any existing Auth0 Action template that emits numeric 1/0 for is_staff/is_superuser will silently lose its staff/superuser flag on next JWT validation (no exception, just a warning log and False). The CHANGELOG entry covers this, which is good. Consider adding a warning log in _normalize_admin_claim when a valid existing privilege is about to be dropped because the claim value is unrecognised — so operators get an actionable signal instead of a silent demotion.

---

Breaking Changes

6. GraphQLJWTTokenAuthMiddleware alias removed without deprecation cycle

# removed from config/websocket/middleware.py
GraphQLJWTTokenAuthMiddleware = JWTAuthMiddleware

Any settings file, ASGI_APPLICATION routing, or third-party integration that imports GraphQLJWTTokenAuthMiddleware from this module will get an ImportError. This alias was documented as "backwards compatibility". If this is intentionally being broken, the CHANGELOG should call it out as a breaking change and the commit message should mention it. A grep for GraphQLJWTTokenAuthMiddleware across the repo and any deployment configs would confirm nothing else imports it.

7. CORS_ORIGIN_WHITELIST removed from production.py

This is the deprecated django-cors-headers setting name (superseded by CORS_ALLOWED_ORIGINS in v3.0). The removal is correct. Just confirm the version of django-cors-headers in use is ≥ 3.0 (it almost certainly is, but worth a quick check if any staging environment runs an older pin).

---

Minor / Style

8. apply_data_to_user — last_ip default falls back to current DB value

user.last_ip = data.get("last_ip", user.last_ip)

This is cleaner than the old data["last_ip"] but the fallback to the existing DB value means a partial payload will silently not update last_ip. Given Auth0 may not always include last_ip in the Management API response, this is probably intentional. A brief comment to that effect would make the intent obvious to future readers.

9. datetime.datetime.now() → timezone-aware — good fix

The old get_new_auth0_token used a naive datetime.datetime.now() for expiration_Date. The new code uses datetime.datetime.now(datetime.timezone.utc). The test asserts assertIsNotNone(last_synced.tzinfo). This is a genuine correctness fix.

10. get_user_details_async URL encoding — good fix

urllib.parse.quote(auth0_Id, safe="") correctly encodes | and / in sub claims. The test confirms this. Well done.

11. ensure_valid_auth0_token deadlock fix — good fix

Replacing .delay().get() with .run() eliminates the deadlock under a saturated prefork pool. The comment in the code explaining why .delay().get() deadlocks is exactly the right level of documentation here.

---

Test Coverage

The new test_user_auth0_tasks.py is comprehensive and well-structured. The module-reload pattern to activate USE_AUTH0 is a bit fragile but necessary given the if settings.USE_AUTH0: guard. The tearDownClass restore is correct.

One gap: test_sync_remote_user_uses_existing_live_token and test_sync_remote_user_fetches_token_when_none_live only assert chain length, not which tasks are in the chain. Adding self.assertIs(args[0].task, tasks.get_user_details_async) (or equivalent) would make the tests more precise.

---

Summary

Area	Assessment
Deadlock fix (ensure_valid_auth0_token)	✅ Correct and important
HTTP timeouts on all outbound requests	✅ Essential reliability fix
select_for_update() token persistence	✅ Correct
Stricter boolean claim parsing	✅ Better security posture
Auth0AdminBackend no-op authenticate	✅ Correct design, needs view-level test
GraphQLJWTTokenAuthMiddleware removal	⚠️ Verify no external imports
_parse_boolean_claim integer fall-through	⚠️ Verify CI passes test_numeric_values_rejected
Frontend JWT → Bearer header fix	✅ RFC 6750 compliant
localStorage → sessionStorage for admin redirect	✅ Security improvement
Test coverage	✅ Good, minor gaps noted above

[claude]

Code Review — PR #1499: Simplify Auth0 JWT validation and remove excessive logging

Overall this is a high-quality, well-scoped PR. The changes address real production problems (worker hangs, race conditions, silent auth downgrades, log noise), the new test suite is thorough, and the commit message and PR description explain the "why" clearly. A few items deserve attention before merge.

---

Strengths

• Fail-closed boolean parsing (_parse_boolean_claim): narrowing to JSON booleans + canonical "true"/"false" strings is the right security posture. A misconfigured Auth0 Action fails loudly instead of silently granting or retaining privilege.
• HTTP timeouts on all outbound Auth0 calls (AUTH0_HTTP_TIMEOUT = 10): the previous hang-forever behaviour on Auth0 network blips could exhaust the Celery prefork pool under load. This fix is important.
• ensure_valid_auth0_token uses .run() instead of .delay().get(): the comment explains this correctly — delay().get() dispatches to the broker and blocks the calling worker waiting for a second worker to pick it up; under pool saturation this deadlocks. .run() invokes the body synchronously in-process. Good call.
• transaction.atomic() + select_for_update() in get_new_auth0_token: eliminates the obvious write-write race.
• urllib.parse.quote(auth0_Id, safe=""): sub claims like auth0|abc contain pipe characters that must be percent-encoded in a URL path. The old code sent malformed Management API requests. This is a correctness fix with security implications.
• Auth0RemoteUserJSONWebTokenBackend now re-raises JSONWebTokenError: the old code re-raised only JSONWebTokenExpired and returned None for all other JWT errors. Returning None on a tampered/malformed token silently downgraded the request to anonymous access. Re-raising the full set surfaces a structured error to the GraphQL client and closes that gap.
• Auth0AdminBackend.authenticate() explicit no-op with explanation: the docstring correctly explains why this is safe — the view validates the JWT before calling login(request, user, backend=…), so accepting auth0_user_id via authenticate() would let any caller of django.contrib.auth.authenticate(auth0_user_id=…) log in without proving ownership.
• sessionStorage instead of localStorage for admin_auth0_next: the redirect hint is now tab-scoped and ephemeral, reducing the risk of stale values from an old session interfering with a new one.
• CORS_ORIGIN_WHITELIST removal: this was a deprecated alias that duplicated CORS_ALLOWED_ORIGINS; removing it is correct.
• Test coverage: the new test_user_auth0_tasks.py (382 lines) covers all five tasks end-to-end with mocked HTTP; useAnnotationImages.test.tsx, useNavMenu.test.tsx, and the new errorLink.test.ts cases all pin the specific behaviour introduced by this PR.

---

Issues / Requests for Clarification

1. Potential inconsistency in test_sync_numeric_claim_does_not_promote ⚠️

The test verifies two sub-cases for numeric claims:

# Case 1: numeric 1 must NOT promote a non-staff user
payload_promote = {"https://test.example.com/is_staff": 1}
self.assertFalse(self.user.is_staff)   # expects non-staff: passes if update blocked

# Case 2: numeric 0 "demotes" an existing staff user (fail-closed)
self.user.is_staff = True
self.user.save()
payload_demote = {"https://test.example.com/is_staff": 0}
self.assertFalse(self.user.is_staff)   # expects non-staff: passes only if update IS applied

_parse_boolean_claim(0) now returns (False, False) — confirmed by test_numeric_values_rejected. The update guard in sync_admin_claims_from_payload is:

if is_staff_valid and user.is_staff != is_staff_claim:

If _normalize_admin_claim(0, …) propagates is_valid=False, then is_staff_valid=False, the guard short-circuits, and the user stays staff — making case 2's assertFalse(is_staff) fail.

The full body of _normalize_admin_claim after the _parse_boolean_claim call isn't visible in the diff (unchanged lines). Two possible interpretations:

• A) _normalize_admin_claim converts (False, False) → (False, True) — treat invalid as definitively "no privilege"; the test is correct and the behaviour matches the docstring ("invalid claims treated as False to avoid privilege retention").
• B) _normalize_admin_claim returns (False, False) unchanged — invalid claims are ignored; the guard never fires; the user's existing is_staff=True is preserved; case 2's assertion is wrong.

Could you clarify which path the code takes? If interpretation A is correct, please add a one-line comment at the return site so the guard interaction is obvious to future readers. If interpretation B is correct, the test assertion in case 2 should be assertTrue (privilege retained on invalid claim) and the inline comment "demotes (fail-closed)" is misleading.

2. Race window in get_new_auth0_token when the table is empty

with transaction.atomic():
    Auth0APIToken.objects.select_for_update().all().delete()
    new_token = Auth0APIToken.objects.create(…)

select_for_update() on an empty result set acquires no rows and therefore no lock. Two workers racing to the first-ever token fetch will both see zero rows, both no-op on delete(), and both create() — leaving two rows. The next fetch cleans them up, but there is a window.

The worst outcome is duplicate (identical) token rows, which is not a security issue. However, if you want to close this completely, a UNIQUE constraint on the token field or a PostgreSQL advisory lock would do it. Happy to leave this as-is if the team considers two-row transient state acceptable.

3. Backwards compatibility alias removed from WebSocket middleware

GraphQLJWTTokenAuthMiddleware = JWTAuthMiddleware was removed from config/websocket/middleware.py. If any deployment's ASGI_APPLICATION routing or channels configuration references this alias by string (e.g., in Django Channels middleware stack config), it will raise an ImportError at startup. Please confirm that:

• No ASGI routing or channels layer config in config/asgi.py, settings/, or any compose file references GraphQLJWTTokenAuthMiddleware.
• The removal is intentional and not just an oversight during the cleanup.

4. apply_data_to_user default for missing email_verified

email_verified = bool(data.get("email_verified", False))
user.is_active = email_verified

If Auth0 returns a partial user profile (e.g., during a network issue that results in a truncated response), missing email_verified now disables the account silently. The old code raised KeyError, which was caught and logged, leaving the account in its previous state.

The fail-closed choice (disable on missing verification) is defensible, but a debug-level log on the False default path would help diagnose these silently-disabled accounts in production:

if not email_verified and "email_verified" not in data:
    logger.warning("apply_data_to_user: email_verified missing for %s; disabling account", userPk)

---

Minor Notes

• expiredCodeStrings list in errorLink.ts: checking "tokenExpired" alongside "JSONWebTokenExpired" and "TOKEN_EXPIRED" is good defensive coverage, but the camelCase form isn't produced by any backend code in this repo (graphql_jwt emits JSONWebTokenExpired). Worth adding a source comment so a future reader knows where each code originates.
• configure_user() exception not caught: the created=True path in get_auth0_user_from_token() calls configure_user(user) without a try/except. If configure_user raises (e.g., sync_remote_user.delay() fails to enqueue), the exception propagates silently up. The old code had the same gap; just noting it as something to consider.
• Log audit trail for privilege transitions: the added logger.info(…is_staff… %s -> %s…) messages are excellent — exactly the right level for security-sensitive changes.
• Migration 0027_drop_auth0apitoken_response: straightforward and correct. Verify that auth0_Response is not read back anywhere in management commands or analytics queries before this runs in production.