Skip to content

fix: enhance Entra identity linking and error handling in authentication - #58

Draft
Dinssa wants to merge 1 commit into
mainfrom
fix/hfurb-2751-unique-user-key-collision
Draft

fix: enhance Entra identity linking and error handling in authentication#58
Dinssa wants to merge 1 commit into
mainfrom
fix/hfurb-2751-unique-user-key-collision

Conversation

@Dinssa

@Dinssa Dinssa commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What

SSO sign-in returned a 500 for some users, with an unhandled IntegrityError.

Authentication.authenticate identified users solely by (entra_tid, entra_oid). Migration 0025 added those columns in June 2025 as nullable with no backfill, so every account created by the Entra flow before that date holds nulls in both while still owning its email and username. Those accounts miss the lookup permanently, so the code treated each sign-in as a new user and tried to insert a row whose username was already taken. An oid changing (account recreated in the tenant, guest converted to member) causes the same failure.

Changes:

  • _find_user now tries three lookups in priority order: Entra identity, then email, then username (both case-insensitive). The first hit wins.
  • A match writes the current entra_oid/entra_tid onto that account, so it is correctly identified from then on and matches on the identity lookup next time. Each affected account costs one extra pair of queries exactly once, which is why no backfill migration is needed.
  • _is_tenant_allowed still runs first, so email matching only ever happens against an identity Microsoft has verified and an approved tenant.
  • Any remaining IntegrityError is caught, reported to Sentry as a warning with the oid/tid in context, and the request is denied rather than 500ing. The message is static so the issue groups into one countable alert.
  • The denial path now renders the existing templates/403.html instead of a bare text response. It links Knowledge Hub and the support desk, and says nothing about an account existing, so it does not leak whether an email is registered.

Ticket link

https://mhclgdigital.atlassian.net/browse/HFURB-2751

Checklist

  • I double-checked that ACs on the ticket are met by this code update
  • I have added tests to new code and updated existing tests where needed
  • I have added testing instructions to the ticket
  • I have added type hints to my new methods

Additional notes

Screenshots

Comment thread accounts/views.py
return HttpResponseRedirect(next_url)

return HttpResponseForbidden("You are not allowed to access this application.")
return render(request, "403.html", status=HTTPStatus.FORBIDDEN)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't HttpResponseForbidden default to using the 403.html template anyway?

@Dinssa
Dinssa marked this pull request as draft July 30, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants