Skip to content

Commit 6842821

Browse files
committed
fix(saml): drop-in parity for provider name, ACS URL, NameID format
The legacy Ruby service registered SAML as the OmniAuth strategy 'adfs' (generic_adfs). That name is baked into external IdP registrations, the /auth/adfs kickoff+callback paths, the login-event payload, and the UserAuthLookup id (auth-<authority>-adfs-<uid>). auth.cr used 'saml', which would 404 the /auth/adfs callback and \u2014 worse \u2014 fail to resolve every existing SAML-linked account (recreating users instead). It also advertised its own computed callback as the SAML ACS URL rather than the DB-configured assertion_consumer_service_url the IdP has registered, and never sent the strat's name_identifier_format. - Register the SAML factory under 'adfs' (primary) with 'saml' kept as an internal alias; the recorded provider name is always 'adfs'. - Advertise strat.assertion_consumer_service_url (DB) as the ACS URL. - Pass strat.name_identifier_format through to the AuthnRequest. Makes SAML SSO a true drop-in: no IdP re-registration, no orphaned UserAuthLookup rows. Adds SAMLRequest-decoding specs asserting the ACS URL + issuer come from the DB and that /auth/adfs is served.
1 parent 751c39e commit 6842821

2 files changed

Lines changed: 84 additions & 7 deletions

File tree

spec/controllers/saml_callbacks_spec.cr

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
require "base64"
2+
require "compress/deflate"
3+
14
require "../helper"
25

36
module PlaceOS::Auth
@@ -23,6 +26,63 @@ module PlaceOS::Auth
2326
strat
2427
}
2528

29+
# The legacy Ruby service registered SAML as the OmniAuth strategy
30+
# "adfs" (generic_adfs), so external IdP registrations, the
31+
# `/auth/adfs` kickoff/callback paths, and the `UserAuthLookup` id
32+
# (`auth-<authority>-adfs-<uid>`) all use "adfs". It also advertised
33+
# the DB-configured `assertion_consumer_service_url` as the ACS URL.
34+
# For a drop-in, auth.cr must reproduce all of that exactly.
35+
describe "SAML drop-in parity", tags: "saml-parity" do
36+
# A strat whose ACS URL is deliberately NOT what auth.cr would
37+
# compute from the request host, so we can prove it comes from the
38+
# DB column and not from `callback_uri`.
39+
create_adfs_strat = -> {
40+
authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil!
41+
strat = ::PlaceOS::Model::SamlAuthentication.new
42+
strat.name = "test-adfs-#{Random.rand(99999)}"
43+
strat.issuer = "https://sp.example.test/metadata-#{Random.rand(99999)}"
44+
strat.idp_sso_target_url = "https://idp.example.test/sso"
45+
strat.assertion_consumer_service_url = "https://prod.example.com/auth/adfs/callback?id=preconfigured"
46+
strat.name_identifier_format = "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"
47+
strat.uid_attribute = "email"
48+
strat.authority_id = authority.id
49+
strat.save!
50+
strat
51+
}
52+
53+
decode_saml_request = ->(location : String) {
54+
query = location.split('?', 2).last
55+
raw = URI::Params.parse(query)["SAMLRequest"]
56+
deflated = Base64.decode(raw)
57+
Compress::Deflate::Reader.open(IO::Memory.new(deflated), &.gets_to_end)
58+
}
59+
60+
it "serves the SAML kickoff at /auth/adfs (legacy provider name)" do
61+
strat = create_adfs_strat.call
62+
result = client.get(
63+
"/auth/adfs?id=#{URI.encode_www_form(strat.id.as(String))}",
64+
headers: HTTP::Headers{"Host" => "localhost"},
65+
)
66+
result.status_code.should eq 303
67+
result.headers["Location"].should start_with "https://idp.example.test/sso"
68+
ensure
69+
strat.try &.destroy
70+
end
71+
72+
it "advertises the DB assertion_consumer_service_url as the ACS URL" do
73+
strat = create_adfs_strat.call
74+
result = client.get(
75+
"/auth/adfs?id=#{URI.encode_www_form(strat.id.as(String))}",
76+
headers: HTTP::Headers{"Host" => "localhost"},
77+
)
78+
xml = decode_saml_request.call(result.headers["Location"])
79+
xml.should contain %(AssertionConsumerServiceURL="#{strat.assertion_consumer_service_url}")
80+
xml.should contain strat.issuer
81+
ensure
82+
strat.try &.destroy
83+
end
84+
end
85+
2686
describe "GET /auth/saml (kickoff)" do
2787
it "redirects to the IdP's SSO URL with a SAMLRequest" do
2888
strat = create_saml_strat.call

src/placeos-auth/external_providers.cr

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,16 @@ module PlaceOS::Auth::ExternalProviders
1515
# `:generic_oauth` strategy from the Ruby service.
1616
OAUTH2_PROVIDER = "oauth2"
1717

18-
# SAML callbacks come in as `/auth/saml/callback?id=<adfs_strat.id>`.
19-
# Replaces the OmniAuth `:generic_adfs` strategy from the Ruby
20-
# service. We keep the `saml` name (not `adfs`) because that's the
21-
# protocol and the original `:generic_adfs` was just an AD-FS-
22-
# specific wrapper around the same SAML 2.0 dance.
23-
SAML_PROVIDER = "saml"
18+
# SAML callbacks. The legacy Ruby service registered its SAML strategy
19+
# under the OmniAuth name `adfs` (`generic_adfs`), so that name is
20+
# baked into external IdP registrations, the `/auth/adfs` kickoff and
21+
# callback paths, the login-event payload, and — critically — the
22+
# `UserAuthLookup` id (`auth-<authority>-adfs-<uid>`). We must reuse it
23+
# verbatim, or existing SAML-linked accounts won't resolve after the
24+
# swap. `saml` is kept as an internal alias (auth.cr-era callers), but
25+
# the recorded provider name is always `adfs`.
26+
SAML_PROVIDER = "adfs"
27+
SAML_PROVIDER_ALIAS = "saml"
2428

2529
def self.register!
2630
::MultiAuth.config(OAUTH2_PROVIDER) do |redirect_uri, provider_id|
@@ -30,6 +34,10 @@ module PlaceOS::Auth::ExternalProviders
3034
::MultiAuth.config(SAML_PROVIDER) do |redirect_uri, provider_id|
3135
build_saml(redirect_uri, provider_id)
3236
end
37+
38+
::MultiAuth.config(SAML_PROVIDER_ALIAS) do |redirect_uri, provider_id|
39+
build_saml(redirect_uri, provider_id)
40+
end
3341
end
3442

3543
# ---- OAuth2 -------------------------------------------------------
@@ -73,13 +81,22 @@ module PlaceOS::Auth::ExternalProviders
7381
raise ::MultiAuth::Exception.new("unknown saml strategy: #{provider_id.inspect}")
7482
end
7583

84+
# Advertise the DB-configured ACS URL — exactly what the IdP has
85+
# registered and what the legacy service sent
86+
# (`options.assertion_consumer_service_url = strat.assertion_consumer_service_url`).
87+
# `multi_auth_saml` uses `redirect_uri` as the ACS URL, so pass the
88+
# DB value straight through (falling back to the computed callback
89+
# only if the column is somehow blank).
90+
acs_url = strat.assertion_consumer_service_url.presence || redirect_uri
91+
7692
::MultiAuth::Provider::SAML.new(
7793
provider_name: SAML_PROVIDER,
78-
redirect_uri: redirect_uri,
94+
redirect_uri: acs_url,
7995
idp_sso_url: strat.idp_sso_target_url,
8096
idp_cert: strat.idp_cert,
8197
idp_cert_fingerprint: strat.idp_cert_fingerprint,
8298
sp_entity_id: strat.issuer,
99+
name_identifier_format: strat.name_identifier_format,
83100
uid_attribute: strat.uid_attribute,
84101
attribute_statements: saml_attribute_mapping(strat),
85102
want_assertions_signed: strat.idp_cert.presence || strat.idp_cert_fingerprint.presence ? true : false,

0 commit comments

Comments
 (0)