Repository navigation
Reject ambiguous email matches so SSO cannot link the wrong account - #1258
Conversation
Signed-off-by: Floating-Y <[email protected]>
WaylandYang
left a comment
There was a problem hiding this comment.
Thank you for catching this. You're right that users_email_active_idx compares the stored spelling, so Ann@x and ann@x can both be active. fetch_optional would then pick one of them arbitrarily, and refusing before any write is the right fix. Preferring the exact spelling would just move the guess somewhere else. A few changes before merge:
- What the person sees. The
AppError::Conflictlands in therefuse("unavailable", …)arm ofaccount_of. In the browser that shows "Single sign-on is unavailable right now", and the audit row recordsunavailable. The person retries forever, and the operator sees what looks like an outage. Please returnAppError::CodedConflict { code: "unlinked", … }instead. That matches the deactivated-address case (nothing to sign in to, ask an administrator), and the detail still reaches the log. - ADR 0069. Decision 1 says a verified address maps "to the active account with that address (case-insensitive)", which assumes it is unique. Please add a dated revision note under it, per the README's "Revisions stay in place": the index is case-sensitive, so two active accounts can differ only in case, and such an address is now refused rather than mapped.
- Tests. The store test is the one that matters. In the exchange test, please drop the last check (sign-in through the existing subject). That path never reaches
account_for_sso, so it re-proves behaviour this change leaves alone. - Small one:
// shortcut:isn't a convention here, and it repeats the doc line above it. One sentence on why the snapshot is enough for now is plenty.
A unique index on lower(email), plus normalising registration and admin creation, is the real fix. It needs a migration and a decision about existing duplicates, so it belongs in its own issue rather than in this PR.
Signed-off-by: Floating-Y <[email protected]>
|
@WaylandYang Thank you for the review. Addressed all four points in d3de92a:
CI run 38900751321 is green for this revision. The full workspace tests ran with PostgreSQL and Global case-insensitive email uniqueness, normalization and handling existing duplicates remain outside this PR. No migration or stored email was changed. |
WaylandYang
left a comment
There was a problem hiding this comment.
Thank you, all four points are addressed. Ambiguous matches now come back as CodedConflict { code: "unlinked" }, so the browser and the audit say "not linked" instead of reporting an outage; the 0069 revision note says what was assumed, why it was wrong and what changed; the exchange test no longer re-proves the subject path; and the comment explains its scope. A unique index on lower(email), with normalisation and a decision on existing duplicates, belongs in its own issue. Merging.
Why
Related to #1237. Registration and administrator-created accounts preserve email casing, while the active-email unique index compares the stored value. SSO auto-linking used
fetch_optionalon a case-insensitive query, so multiple active matches could link an arbitrary account or reject login because that arbitrary account already had a binding.What changes
Read at most two active matches in
account_for_ssoand reject ambiguity before any account or identity write. Neither exact casing nor an unlinked candidate takes precedence. ReturnCodedConflictwith codeunlinked, so the browser callback and audit classify this as an account-linking problem rather than an outage; the diagnostic message remains in logs. The exchange keeps its existing generic public failure response.Keep unique-email matching, provisioning, deactivated-account refusal and binding-conflict rollback. The store regression checks both exact spellings, with and without an existing binding, and compares complete account and identity snapshots. The exchange regression checks generic refusal and no new identity write.
Add a dated revision under ADR 0069 Decision 1, preserving the original claim and explaining why its uniqueness assumption was wrong. Update its status line to reference this PR and the remaining uniqueness boundary.
Boundary: the check covers the SELECT snapshot. Concurrent ordinary registration can still insert a differently cased address afterward. A unique index on
lower(email), normalization in registration/admin creation, and handling existing duplicates are separate work. No migration or stored email is changed; existing token-consumption and failure-audit writes remain.How it was checked
Revision
d3de92a097fd0c2b8e84b8239cc68722224c84cdpassed CI run 38900751321: lint, test, build, migrations, web and backend.cargo test --workspaceon Ubuntu with an isolatedpgvector/pgvector:pg16service, migrations applied,UTOPIA_DATABASE_URLset andUTOPIA_TEST_REQUIRE_DB=1.cargo fmt --all --checkandcargo clippy --workspace --all-targets -- -D warnings.cargo fmt --all --check,git diff --check, andcargo clippy -p utopia-store -p utopia-server --all-targets -- -D warnings.Before review
git commit -s)cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warningsandcargo test --workspacepasscrates/utopia-store/was tested withUTOPIA_DATABASE_URLset (those tests skip without it)