From 0f2b56852d4e53d563f43fa227ea5eeb97f440ff Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Wed, 29 Jul 2026 15:57:34 +1000 Subject: [PATCH 1/4] test(idp): pin the info_mappings comma-fallback (ID-08) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The legacy Ruby `generic_oauth` strategy treated each info_mappings value as a comma-separated FALLBACK LIST and used the first key present in the provider's profile — notably how Azure/Entra surfaces the email as `userPrincipalName` rather than `email`. multi_auth does a single literal lookup, so a comma-list resolves to nil and the email arrives empty. Losing that fallback produced real users with no email, which then duplicated on the next sign-in. multi_auth_patch.cr restores it but nothing pinned the behaviour, so it could regress silently on a shard bump. Three cases, covering both directions plus the blank-value edge: * falls back past absent keys to a later candidate (userPrincipalName) * prefers the FIRST present key over later fallbacks * skips a present-but-EMPTY candidate rather than mapping "" Matrix row ID-08. External IdP is the thinnest cluster and UCLA-relevant. Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/provider_callbacks_spec.cr | 104 ++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/spec/controllers/provider_callbacks_spec.cr b/spec/controllers/provider_callbacks_spec.cr index 19158eb..23abdce 100644 --- a/spec/controllers/provider_callbacks_spec.cr +++ b/spec/controllers/provider_callbacks_spec.cr @@ -267,5 +267,109 @@ module PlaceOS::Auth strat.try &.destroy end end + + # ---- info_mappings comma-fallback (matrix ID-08) ----------------- + # + # The legacy Ruby `generic_oauth` strategy treated each info_mappings value + # as a COMMA-SEPARATED FALLBACK LIST, using the first key present in the + # provider's profile — notably how Azure/Entra surfaces the email as + # `userPrincipalName` rather than `email`. multi_auth does a single literal + # lookup, so a comma-list resolves to nil and the email arrives EMPTY. + # + # When that fallback was lost, real logins produced users with no email, + # which then duplicated on the next sign-in. `multi_auth_patch.cr` restores + # it; these pin both sides of the fallback so it cannot silently regress. + + describe "info_mappings comma-fallback", tags: "idp-mapping" do + azure_strat = ->(site : String) { + strat = create_strat.call(site, "openid email") + strat.info_mappings = { + "uid" => "id", + "email" => "email,mail,userPrincipalName", + "name" => "displayName,name", + } + strat.save! + strat + } + + callback = ->(strat_id : String, data : NamedTuple(cookie: String, state: String)) { + client.get( + "/auth/oauth2/callback?id=#{URI.encode_www_form(strat_id)}&code=test-code&state=#{data[:state]}", + headers: HTTP::Headers{"Host" => "localhost", "Cookie" => data[:cookie]}, + ) + } + + user_for = ->(uid : String) { + lookup = ::PlaceOS::Model::UserAuthLookup.where(uid: uid, provider: "oauth2").first? + lookup.nil? ? nil : ::PlaceOS::Model::User.find!(lookup.user_id.as(String)) + } + + it "falls back to a later key when earlier ones are absent (Azure userPrincipalName)" do + strat = azure_strat.call("https://idp.example.test") + uid = "azure-uid-#{Random.rand(99999)}" + upn = "upn-#{Random.rand(99999)}@localhost" + + stub_token_endpoint.call("https://idp.example.test", "idp-token-xyz") + # NB: neither `email` nor `mail` is present — only the third candidate. + stub_userinfo.call("https://idp.example.test", { + "id" => uid, + "userPrincipalName" => upn, + "displayName" => "Azure Person", + }) + + callback.call(strat.id.as(String), kickoff.call(strat.id.as(String))).status_code.should eq 303 + + created = user_for.call(uid) + created.should_not be_nil + # Without the patch this is "" — and the account duplicates next login. + created.not_nil!.email.to_s.should eq upn + created.not_nil!.name.should eq "Azure Person" + ensure + strat.try &.destroy + end + + it "prefers the FIRST present key over later fallbacks" do + strat = azure_strat.call("https://idp.example.test") + uid = "azure-uid-#{Random.rand(99999)}" + primary = "primary-#{Random.rand(99999)}@localhost" + + stub_token_endpoint.call("https://idp.example.test", "idp-token-xyz") + stub_userinfo.call("https://idp.example.test", { + "id" => uid, + "email" => primary, + "mail" => "wrong-#{Random.rand(99999)}@localhost", + "userPrincipalName" => "also-wrong-#{Random.rand(99999)}@localhost", + "displayName" => "Primary Person", + }) + + callback.call(strat.id.as(String), kickoff.call(strat.id.as(String))) + + user_for.call(uid).not_nil!.email.to_s.should eq primary + ensure + strat.try &.destroy + end + + it "skips blank candidates rather than mapping an empty value" do + strat = azure_strat.call("https://idp.example.test") + uid = "azure-uid-#{Random.rand(99999)}" + upn = "upn-#{Random.rand(99999)}@localhost" + + stub_token_endpoint.call("https://idp.example.test", "idp-token-xyz") + # `email` is present but EMPTY — the fallback must continue past it, + # not stop and map "". + stub_userinfo.call("https://idp.example.test", { + "id" => uid, + "email" => "", + "userPrincipalName" => upn, + "displayName" => "Blank First Person", + }) + + callback.call(strat.id.as(String), kickoff.call(strat.id.as(String))) + + user_for.call(uid).not_nil!.email.to_s.should eq upn + ensure + strat.try &.destroy + end + end end end From 370ee994fac627b9b1c1b42529223cd74aac19fb Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Wed, 29 Jul 2026 15:59:39 +1000 Subject: [PATCH 2/4] test(idp): authorize_params merge, ensure_matching fail-closed, provider linking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three more external-IdP rows. This cluster is the thinnest in the matrix and the most UCLA-relevant (SAML/Azure), and each of these guards a regression that is silent rather than loud. ID-07 authorize_params merge — the legacy service merged the strat's authorize_params into the outbound authorize URL. Google returns NO refresh_token without access_type=offline, so dropping it means every session dies at the first expiry with no way to renew. Also asserts the merge leaves the embedded redirect_uri intact. ID-06 ensure_matching — the hosted-domain restriction ("Invalid Hosted Domain"). Dropping it admits any account the IdP will authenticate; for a multi-tenant IdP that is the whole internet. Covers match, non-match, and crucially the ABSENT-field case, which must fail closed rather than pass vacuously. ID-11 OAuthUserMapper Link branch — an already-signed-in user completing a callback for an unseen provider identity must have it attached to their existing account, not forked into a second user. Only the Login and Created branches were covered. Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/provider_callbacks_spec.cr | 166 ++++++++++++++++++++ 1 file changed, 166 insertions(+) diff --git a/spec/controllers/provider_callbacks_spec.cr b/spec/controllers/provider_callbacks_spec.cr index 23abdce..c9b1727 100644 --- a/spec/controllers/provider_callbacks_spec.cr +++ b/spec/controllers/provider_callbacks_spec.cr @@ -371,5 +371,171 @@ module PlaceOS::Auth strat.try &.destroy end end + + # ---- authorize_params merge (matrix ID-07) ----------------------- + # + # The legacy Ruby service merged the strat's `authorize_params` column into + # the outbound authorize URL. Dropping it is silent but consequential: + # Google returns NO refresh_token without `access_type=offline`, so every + # session dies at the first token expiry with no way to renew. + + describe "authorize_params merge", tags: "idp-authorize-params" do + it "merges the strat's authorize_params into the authorize URL" do + strat = create_strat.call("https://idp.example.test", "openid") + strat.authorize_params = {"access_type" => "offline", "prompt" => "consent"} + strat.save! + + result = client.get( + "/auth/oauth2?id=#{URI.encode_www_form(strat.id.as(String))}", + headers: HTTP::Headers{"Host" => "localhost"}, + ) + result.status_code.should eq 303 + params = URI::Params.parse(result.headers["Location"].split('?', 2).last) + params["access_type"].should eq "offline" + params["prompt"].should eq "consent" + # the merge must not disturb the embedded redirect_uri + params["redirect_uri"].should eq "http://localhost/auth/oauth2/callback?id=#{strat.id}" + ensure + strat.try &.destroy + end + + it "leaves the authorize URL untouched when none are configured" do + strat = create_strat.call("https://idp.example.test", "openid") + result = client.get( + "/auth/oauth2?id=#{URI.encode_www_form(strat.id.as(String))}", + headers: HTTP::Headers{"Host" => "localhost"}, + ) + location = result.headers["Location"] + location.should_not contain "access_type" + location.should_not contain "&&" + ensure + strat.try &.destroy + end + end + + # ---- ensure_matching fails closed (matrix ID-06) ----------------- + # + # The hosted-domain restriction (Ruby's "Invalid Hosted Domain"). Every + # configured field must have at least one value matching at least one + # pattern. Dropping it admits ANY account the IdP will authenticate — for a + # multi-tenant IdP that is the whole internet. The missing-field case is + # the one that matters most: it must FAIL, not pass vacuously. + + describe "ensure_matching", tags: "idp-ensure-matching" do + hd_strat = -> { + strat = create_strat.call("https://idp.example.test", "openid email") + strat.ensure_matching = {"hd" => ["example\\.com"]} + strat.save! + strat + } + + run_callback = ->(strat : ::PlaceOS::Model::OAuthAuthentication, profile : Hash(String, String)) { + stub_token_endpoint.call("https://idp.example.test", "idp-token-xyz") + stub_userinfo.call("https://idp.example.test", profile) + data = kickoff.call(strat.id.as(String)) + client.get( + "/auth/oauth2/callback?id=#{URI.encode_www_form(strat.id.as(String))}&code=test-code&state=#{data[:state]}", + headers: HTTP::Headers{"Host" => "localhost", "Cookie" => data[:cookie]}, + ) + } + + it "admits a profile whose field matches the pattern" do + strat = hd_strat.call + result = run_callback.call(strat, { + "id" => "hd-ok-#{Random.rand(99999)}", + "email" => "ok-#{Random.rand(99999)}@localhost", + "name" => "Allowed Person", + "hd" => "example.com", + }) + result.status_code.should eq 303 + ensure + strat.try &.destroy + end + + it "rejects a profile whose field matches no pattern" do + strat = hd_strat.call + result = run_callback.call(strat, { + "id" => "hd-bad-#{Random.rand(99999)}", + "email" => "bad-#{Random.rand(99999)}@localhost", + "name" => "Outside Person", + "hd" => "evil.example.org", + }) + result.status_code.should eq 302 + result.headers["Location"].should contain "/auth/failure" + ensure + strat.try &.destroy + end + + it "FAILS CLOSED when the required field is absent entirely" do + strat = hd_strat.call + result = run_callback.call(strat, { + "id" => "hd-missing-#{Random.rand(99999)}", + "email" => "missing-#{Random.rand(99999)}@localhost", + "name" => "No Domain Person", + # no `hd` key at all + }) + result.status_code.should eq 302 + result.headers["Location"].should contain "/auth/failure" + ensure + strat.try &.destroy + end + end + + # ---- OAuthUserMapper Link branch (matrix ID-11) ------------------ + # + # Branch (2): a user who is ALREADY signed in completes a callback for a + # provider identity we have never seen. That must attach the new identity + # to the existing account, not mint a second one — otherwise linking a + # second IdP silently forks the user. + + describe "linking a provider to a signed-in user", tags: "idp-link" do + it "links the new identity to the session user instead of creating one" do + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + strat = create_strat.call("https://idp.example.test", "openid email") + + password = "link-branch-pw-#{Random.rand(99999)}" + existing = ::PlaceOS::Model::Generator.user(authority) + existing.password = password + existing.save! + + before_count = ::PlaceOS::Model::User.count + + session = Spec.signin!(client, existing, password) + + # kickoff mutates the EXISTING session (adds oauth_state), so the + # signed-in uid survives into the callback. + kick = client.get( + "/auth/oauth2?id=#{URI.encode_www_form(strat.id.as(String))}", + headers: HTTP::Headers{"Host" => "localhost", "Cookie" => session}, + ) + kick.status_code.should eq 303 + cookie = kick.cookies[PlaceOS::Auth::SESSION_COOKIE_NAME].try { |c| "#{c.name}=#{c.value}" } || session + state = URI::Params.parse(kick.headers["Location"].split('?', 2).last)["state"] + + uid = "link-uid-#{Random.rand(99999)}" + stub_token_endpoint.call("https://idp.example.test", "idp-token-xyz") + stub_userinfo.call("https://idp.example.test", { + "id" => uid, + "email" => "someone-else-#{Random.rand(99999)}@localhost", + "name" => "Linked Identity", + }) + + result = client.get( + "/auth/oauth2/callback?id=#{URI.encode_www_form(strat.id.as(String))}&code=test-code&state=#{state}", + headers: HTTP::Headers{"Host" => "localhost", "Cookie" => cookie}, + ) + result.status_code.should eq 303 + + lookup = ::PlaceOS::Model::UserAuthLookup.where(uid: uid, provider: "oauth2").first? + lookup.should_not be_nil + # the crux: the new identity points at the ALREADY signed-in user + lookup.not_nil!.user_id.should eq existing.id + ::PlaceOS::Model::User.count.should eq before_count + 1 # only `existing` + + ensure + strat.try &.destroy + existing.try &.destroy + end + end end end From 1a09a1a934055adb1ceccea90cced84a06a95b59 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Wed, 29 Jul 2026 16:11:57 +1000 Subject: [PATCH 3/4] test(idp): fix inverted user-count assertion in the Link-branch spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The assertion read `before_count + 1`, but the count was captured AFTER the fixture user was created — so it demanded that a SECOND account appear, which is precisely the bug the test exists to catch. A correct link leaves the count unchanged. CI caught it: expected 6, got 5, i.e. the mapper behaved correctly and the spec was wrong. Renamed the variable to count_with_existing so the intent can't be misread. Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/provider_callbacks_spec.cr | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/controllers/provider_callbacks_spec.cr b/spec/controllers/provider_callbacks_spec.cr index c9b1727..eb1ad78 100644 --- a/spec/controllers/provider_callbacks_spec.cr +++ b/spec/controllers/provider_callbacks_spec.cr @@ -498,7 +498,10 @@ module PlaceOS::Auth existing.password = password existing.save! - before_count = ::PlaceOS::Model::User.count + # Counted AFTER `existing` is created, so a correct link leaves this + # unchanged. If the mapper wrongly took the Created branch instead, + # the count goes UP by one — that is the account silently forking. + count_with_existing = ::PlaceOS::Model::User.count session = Spec.signin!(client, existing, password) @@ -530,8 +533,8 @@ module PlaceOS::Auth lookup.should_not be_nil # the crux: the new identity points at the ALREADY signed-in user lookup.not_nil!.user_id.should eq existing.id - ::PlaceOS::Model::User.count.should eq before_count + 1 # only `existing` - + # no SECOND account was minted for the new provider identity + ::PlaceOS::Model::User.count.should eq count_with_existing ensure strat.try &.destroy existing.try &.destroy From b7ae5e8ae998ee448eab1b7b7ca6ca60897a858f Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Wed, 29 Jul 2026 16:13:12 +1000 Subject: [PATCH 4/4] test(idp): drop ensure_matching cases already covered elsewhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit oauth_provider_flows_spec.cr already asserts the match and non-match cases, so two of the three added here were redundant. The genuine gap was the ABSENT-field case — a required field that is missing entirely must fail closed rather than pass vacuously — and only that one is kept. Also corrects the matrix, which had ID-06 as no-coverage when it was partial. Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/provider_callbacks_spec.cr | 35 ++++----------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/spec/controllers/provider_callbacks_spec.cr b/spec/controllers/provider_callbacks_spec.cr index eb1ad78..05a03ef 100644 --- a/spec/controllers/provider_callbacks_spec.cr +++ b/spec/controllers/provider_callbacks_spec.cr @@ -418,8 +418,12 @@ module PlaceOS::Auth # The hosted-domain restriction (Ruby's "Invalid Hosted Domain"). Every # configured field must have at least one value matching at least one # pattern. Dropping it admits ANY account the IdP will authenticate — for a - # multi-tenant IdP that is the whole internet. The missing-field case is - # the one that matters most: it must FAIL, not pass vacuously. + # multi-tenant IdP that is the whole internet. + # + # NB: `oauth_provider_flows_spec.cr` already covers the match and + # non-match cases. The gap was the ABSENT-field case, which is the one + # that actually matters: a missing field must FAIL, not pass vacuously. + # Only that case is added here, to avoid duplicating existing coverage. describe "ensure_matching", tags: "idp-ensure-matching" do hd_strat = -> { @@ -439,33 +443,6 @@ module PlaceOS::Auth ) } - it "admits a profile whose field matches the pattern" do - strat = hd_strat.call - result = run_callback.call(strat, { - "id" => "hd-ok-#{Random.rand(99999)}", - "email" => "ok-#{Random.rand(99999)}@localhost", - "name" => "Allowed Person", - "hd" => "example.com", - }) - result.status_code.should eq 303 - ensure - strat.try &.destroy - end - - it "rejects a profile whose field matches no pattern" do - strat = hd_strat.call - result = run_callback.call(strat, { - "id" => "hd-bad-#{Random.rand(99999)}", - "email" => "bad-#{Random.rand(99999)}@localhost", - "name" => "Outside Person", - "hd" => "evil.example.org", - }) - result.status_code.should eq 302 - result.headers["Location"].should contain "/auth/failure" - ensure - strat.try &.destroy - end - it "FAILS CLOSED when the required field is absent entirely" do strat = hd_strat.call result = run_callback.call(strat, {