OpenConceptLab/ocl_online#339 | Sign-in: don't map a Keycloak identity onto a deactivated account unless the verified email matches

[paynejd]

Part of OpenConceptLab/ocl_online#339.

What changes

OCLOIDCAuthenticationBackend.filter_users_by_claims matched a Keycloak identity to an OCL account by username alone, including a deactivated account. Now:

• Active account with that username: matched as before.
• Deactivated account: reactivated (undelete()) only when the claims carry email_verified: true and an email equal to the account's (case-insensitive, trimmed). undelete() saves the profile, so propagate_owner_status brings the owner's repos back with it.
• Deactivated account, any other claims (email unverified, missing or different): the sign-in is refused and logged with the user id. The account isn't touched: no profile update, no save, no repo status change.
• Username case: usernames still match exactly as Keycloak sends them. A mixed-case account is never matched, active or not, so that sign-in still gets a new account, as today.

Design note: why a refusal, not a new account

Usernames are unique (user_profiles_username_key). If the filter returned no match for a deactivated account's username, mozilla-django-oidc would call create_user. That raises IntegrityError on the unique index (checked with a probe), which would be an unhandled 500 on every request. So the sign-in is refused instead, never mapped onto the old row:

• A new DeactivatedAccountLoginRefused (a DRF AuthenticationFailed) answers 401 with a message asking the person to contact the OCL team (COMMUNITY_EMAIL).
• RequireAuthenticationMiddleware returns that 401 as-is. Before this change it became the anonymous-access 403. Other authentication failures still get the 403.
• On the session callback path, authenticate() returns None, so that login fails cleanly instead of raising.

Staff can restore an account with the existing PUT /users/<username>/reactivate/.

Tests

core/common/tests.py (OCLOIDCAuthenticationBackendTest):

• Active match: unchanged, including a full get_or_create_user run.
• Deactivated + matching verified email: reactivated, and the owner's source comes back. Email case and whitespace are ignored.
• Deactivated + unverified, non-boolean, missing or different email, or no email on either side: refused. The row and its source are untouched (fields and updated_at).
• Full flow: a refused sign-in creates no user. A later sign-in with the verified email reactivates the account and updates the profile.
• Username case: a mixed-case deactivated account isn't matched. The sign-in gets a new lowercase account, and the old row is untouched.
• Session path: authenticate() returns None on a refusal.

core/middlewares/tests.py:

• A refused sign-in gets a 401 with the reason and WWW-Authenticate.
• Any other AuthenticationFailed still gets the 403.

Local results, in an isolated compose stack with CI's commands:

• The new backend tests fail against master's backends.py (8 failures, 1 error) and pass with this change.
• pylint -j0 core/ rates 10.00/10.
• Full-suite results will be added in a comment.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LuamFpdtPmh4FX8CpVgANp

[paynejd]

Codex adversarial review, pass 1 (codex-cli 0.159.2, commit d878f26)

Reviewed git diff origin/master...HEAD; HEAD is d878f2631e960f845680de3c08e04e5969c41cdd. No files modified. These are source-based findings; Docker access was denied, so tests were not run.

1. High — Account checks and writes are vulnerable to races. backends.py:113, backends.py:87
   An active user can be fetched, concurrently deactivated, then saved by update_user() with stale is_active=True when another claim changes. This reactivates the account and its repos without checking verified email. Likewise, a deactivated account’s email can change after the check; undelete() saves the stale object, restoring the old email and activating it. Deactivation between the two queries can also produce an empty result followed by duplicate-username creation and an uncaught integrity error.
   Fix: Lock the username row inside a transaction spanning lookup, verification, reactivation and update_user(). Coordinate competing account-state writes with that locking strategy.

2. (Moved to the private ticket.) This finding is about existing production data rather than the code in this diff. It is tracked on OpenConceptLab/ocl_online#339.

3. Medium — Refused callbacks retain newly stored credentials and potentially an existing login. backends.py:93
   Upstream auth.py:318 stores tokens before get_or_create_user(). With OIDC_STORE_ACCESS_TOKEN=True, returning None does not remove the refused identity’s access token. Upstream callback login_failure() only redirects; an existing authenticated session also remains. TokenAuthMiddleWare subsequently injects the retained token into requests, creating inconsistent session/header identities and repeated refusals.
   Fix: Store credentials only after successful account resolution, or restore the previous credential state on refusal. Explicitly define whether a failed account-switch attempt preserves or clears the existing login.

4. Medium — Reactivation can partially commit and then return 500. backends.py:119, signals.py:23
   undelete() saves the user; its signal updates sources, synchronously indexes them, then updates/indexes collections. An ES failure can leave the user and sources active while collections remain inactive. BatchIndexingError is not handled by the refusal catches, so REST, middleware, GraphQL and callback paths can return 500 after writes. Large owners also incur full repo indexing during authentication.
   Fix: Make database changes atomic and enqueue indexing after commit with retry support. Keep ES availability outside authentication success.

5. Medium — Non-string email claims cause 500 instead of refusal. backends.py:126
   A truthy JSON number, boolean, list or object in email raises AttributeError at .strip(), even when email_verified=False. Upstream claim verification only checks that the email key exists. None of the relevant callers catches this error. This requires malformed provider claims, rather than an ordinary valid Keycloak email.
   Fix: Require a string email and literal True verification before normalization; otherwise raise the refusal.

6. Low — Every active-user authentication adds a database query. backends.py:113
   The inactive-row .first() query precedes the original user lookup. Protected REST requests can authenticate once in middleware and again in DRF, adding two queries per request relative to the previous code.
   Fix: Fetch the unique username row once, inspect its state, and reuse the resolved object through the authentication flow.

The new tests leave material gaps:

• common/tests.py:2606: unchanged fields and timestamps do not prove no save, no update_user() call, or no signal invocation. Spy on those operations; also check collections and token state.
• common/tests.py:2691: mocking upstream authenticate() proves exception conversion only; it cannot reveal retained session tokens.
• middlewares/tests.py:74: mocking OCLAuthentication proves response formatting, not real backend propagation or absence of writes.
• Missing coverage includes concurrency, malformed claim types, indexing failures, query counts and real callback/DRF/GraphQL integration.

No other significant finding: Literal email_verified is True rejects coercion tricks; empty normalized emails fail. String trimming/lowercasing follows the stated policy, with no additional Unicode bypass identified. Exact username matching and the unique constraint prevent ordinary multiple-row selection; case variants remain separate accounts as before.

The refusal exception itself propagates correctly: REST and TokenExchangeView use DRF’s 401 handling; protected middleware returns 401; GraphQL catches it as AuthenticationFailed and produces anonymous/invalid context; session authentication returns None. Ordinary sequential refusal occurs before undelete() or update_user() and does not write the user or repos.

Successful undelete() deliberately clears verification/deactivation metadata and activates all inactive owned sources and collections, including independently inactive repos. It creates no Django token; token exchange can create one afterward. New log messages contain only user IDs and outcomes—no emails, claims or tokens.

[paynejd]

How the Codex pass 1 findings were handled (fixes in e5a6279):

1. Races: partly fixed. The lookup is now one evaluated query, and that same row is what update_user receives. There's no longer a gap between two queries, and no create_user after a concurrent deactivation. A sign-in that races a staff deactivation or a staff email edit can still save a stale row through update_user's full save. That already happens on master and isn't changed here. The fuller fix, binding identities to the Keycloak subject and locking the row, is a follow-up.
2. Moved to the private ticket.
3. Fixed. A refused sign-in on the session callback path now drops the tokens stored for it. The web apps don't use this path: they exchange the code at /users/oidc/code-exchange/ and send bearer tokens.
4. Not changed. propagate_owner_status updates both sources and collections in the database before indexing either, so an ES failure can't leave collections inactive. The failure can return a 500 on that one request after the reactivation has committed. The next request signs in normally, because the row is now active. The staff PUT /users/<username>/reactivate/ behaves the same way.
5. Fixed. A non-string email is refused.
6. Fixed. Each lookup is one query, as on master, and a test asserts it.

Tests:

• Spies show that a refusal doesn't save, update or create a user.
• Reactivation also brings back the owner's collections.
• A query-count test covers the lookup.
• A bearer request runs end to end through the real middleware, DRF OIDC authentication and backend.
• Concurrency and ES-failure tests weren't added.

Local, at e5a6279: the full suite passes (2,322 tests), and pylint -j0 core/ rates 10.00/10.

[paynejd]

Codex adversarial review, pass 2 (codex-cli 0.159.2, commit e5a6279)

Reviewed git diff origin/master...HEAD; HEAD is e5a6279a0cdcf558b2afdfc0f7f8681004094044. One functional issue remains.

1. Medium — an absent email claim bypasses the refusal and session cleanup.
   Locations: core/common/backends.py:93, core/common/backends.py:102; upstream auth.py:81, :318, :343.

   Scenario: Valid Keycloak userinfo names a deactivated account but omits email entirely. With the configured email scope, upstream verify_claims raises SuspiciousOperation before filter_users_by_claims executes.

   • On a protected bearer request, this becomes generic AuthenticationFailed, which the middleware suppresses into anonymous 403, rather than the intended refusal 401.
   • During the session callback, tokens have already been stored. Upstream catches SuspiciousOperation and returns None, so the new exception handler never removes those tokens.

   The account and repositories remain untouched; this does not authenticate the refused identity. It does leave failed-login token state and violates the specified refusal behavior.

   Suggested fix: Handle the deactivated-account guard during claim verification, before upstream rejects a missing email, while preserving normal validation for other accounts. Add full-path bearer and callback tests with the email key absent.

2. Low — the callback test does not prove persisted session cleanup.
   Location: core/common/tests.py:2719.

   Scenario: The test replaces upstream authenticate with a function that directly raises the new exception and uses a dictionary session. It proves the catch-and-pop branch, but misses the upstream early-return path above and callback session persistence.

   Suggested fix: Exercise the actual callback with a Django session, mocking provider responses only; reload the session and assert both token keys are absent and no refused user is logged in.

Otherwise, no significant issue found within the agreed scope:

• The evaluated queryset is cached and reused by upstream update_user; the extra lookup is removed.
• Non-string emails, empty values, and non-boolean verification claims fail closed. Exact username matching and trimmed, case-insensitive email comparison match the stated policy.
• Refusal occurs before user updates, group changes, saves, or repository propagation.
• DRF views, TokenExchangeView, and GraphQL handle the exception without a new 500. GraphQL treats it as invalid authentication.
• Active-user behavior remains unchanged. Reactivation retains the acknowledged verification, repository, checksum, and synchronous indexing side effects; undelete() does not recreate a Django token.
• (One line about the production data item from pass 1 moved to the private ticket, OpenConceptLab/ocl_online#339.)
• New logs contain only numeric user IDs and outcome text, with no emails, claims, or tokens.

No files were modified. Tests could not run because Docker socket access was denied.

[paynejd]

Codex pass 2: nothing changed in code. Both findings describe behaviour that is the same on master:

1. Missing email claim (403, not 401). mozilla-django-oidc's verify_claims rejects claims without email before filter_users_by_claims runs. It does that for every account, active or deactivated, exactly as on master, and nothing is written. Keycloak includes email in userinfo for any account that has one. The tokens left in the session on that failure come from upstream too, and happen on master for every failed callback.
2. Callback test. The web apps don't use the session callback path. They exchange the code at /users/oidc/code-exchange/ and send bearer tokens. The bearer path is covered end to end by test_bearer_request_for_deactivated_user.

Pass 2 found no other issues in the change: the single lookup, the claim checks, the refusal before any write, the DRF/GraphQL/middleware handling, unchanged active-user behaviour, and the logging.