diff --git a/spec/controllers/authorize_flow_spec.cr b/spec/controllers/authorize_flow_spec.cr index 23edd73..aca09dc 100644 --- a/spec/controllers/authorize_flow_spec.cr +++ b/spec/controllers/authorize_flow_spec.cr @@ -84,6 +84,31 @@ module PlaceOS::Auth user.try &.destroy end + it "bounces an unauthenticated caller to login rather than denying (AU-07)" do + # The deny path is gated on the session exactly like the grant path. + # It matters that it BOUNCES rather than emitting `access_denied`: an + # unauthenticated deny that redirected to the client would let anyone + # who can reach the endpoint fabricate a user's refusal, and the + # client would take it as a decision the user made. + user, password, app = make_app.call + query = "redirect_uri=#{URI.encode_www_form(app.redirect_uri.as(String))}" \ + "&client_id=#{URI.encode_www_form(app.uid.as(String))}&response_type=code&state=abc" + + result = client.delete("/auth/oauth/authorize?#{query}", + headers: HTTP::Headers{"Host" => "localhost"}) + + result.status_code.should eq 303 + location = result.headers["Location"] + location.should eq "/auth/login" + # Nothing reached the client — no decision was invented on the + # user's behalf. + location.should_not contain "access_denied" + location.should_not start_with app.redirect_uri.as(String) + ensure + app.try &.destroy + user.try &.destroy + end + it "refuses to redirect to a URI not registered for the client (no open redirect)" do user, password, app = make_app.call cookie = Spec.signin!(client, user, password) diff --git a/spec/controllers/authorize_validation_spec.cr b/spec/controllers/authorize_validation_spec.cr index 10ebe3d..a3a4567 100644 --- a/spec/controllers/authorize_validation_spec.cr +++ b/spec/controllers/authorize_validation_spec.cr @@ -808,5 +808,43 @@ module PlaceOS::Auth user.try &.destroy end end + + # ---- AU-12: the authorization code's lifetime ---------------------- + + describe "code expiry (AU-12)" do + it "mints codes with the 10-minute Doorkeeper lifetime" do + # Doorkeeper's `authorization_code_expires_in` default is 10 minutes + # and the legacy service never overrode it, so a cutover must not + # silently shorten or lengthen the window a half-finished login has + # to complete in. `authly_adapter.cr` sets `config.code_ttl = + # 10.minutes`; this asserts the value REACHES the code rather than + # asserting the constant back to itself. + # + # The rejection side — an expired code refused with 400 + # `invalid_grant` rather than a 500 — lives in + # `token_disclosure_spec.cr`. + ::Authly.config.code_ttl.should eq 10.minutes + + redirect = "https://au12.example/cb-#{Random.rand(999_999)}" + user, password = make_user.call + app = make_app.call(redirect) + cookie = Spec.signin!(client, user, password) + + result = authorize.call(cookie, { + "response_type" => "code", + "client_id" => app.uid.as(String), + "redirect_uri" => redirect, + "scope" => "public", + }) + result.status_code.should eq 302 + code = URI::Params.parse(result.headers["Location"].split('?', 2).last)["code"] + + payload, _ = JWT.decode(code, ::Authly.config.public_key.as(String), JWT::Algorithm::RS256) + (payload["exp"].as_i64 - payload["iat"].as_i64).should eq 600 + ensure + app.try &.destroy + user.try &.destroy + end + end end end diff --git a/spec/controllers/jwks_spec.cr b/spec/controllers/jwks_spec.cr index 5f14517..c994e97 100644 --- a/spec/controllers/jwks_spec.cr +++ b/spec/controllers/jwks_spec.cr @@ -1,4 +1,5 @@ require "../helper" +require "base64" module PlaceOS::Auth # JWKS parity for the Doorkeeper-openid_connect mount (PPT-2536). @@ -32,5 +33,63 @@ module PlaceOS::Auth n.size.should eq 342 n.should_not start_with "A" # a leading zero byte would encode as "A..." end + + # ---- OI-11: does a token point at the key that signed it? ---------- + + it "issues tokens whose header carries no kid (OI-11 — DIVERGENCE)" do + # An RP validating our tokens fetches the JWKS and has to pick a key. + # The standard way is `kid`: OIDC Core §10.1 says the header SHOULD + # carry one when the JWKS publishes more than one key, and most + # libraries look for it unconditionally. + # + # `Authly.jwt_encode` is `JWT.encode(payload, key, alg)`, which emits + # `{"alg":"RS256","typ":"JWT"}` and nothing else, so our tokens name no + # key at all. It works today only because the JWKS publishes exactly + # ONE key and every sane library falls back to "try the only one". + # + # Two things make this worth pinning rather than shrugging at. It is a + # latent blocker on key rotation: the moment the JWKS holds two keys, a + # kid-less token is ambiguous and strict RPs reject it — so rotation + # needs this fixed FIRST, not during. And it is the same class as the + # missing `nonce` (OI-04): fine for PlaceOS's own clients, a surprise + # for anyone integrating a conformant RP. + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "jwks-kid-#{Random.rand(999_999)}" + app.redirect_uri = "https://jwks.example/cb-#{Random.rand(999_999)}" + app.scopes = "public" + app.confidential = true + app.owner_id = "authority-owner" + app.save! + + issued = client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "client_credentials") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("scope", "public") + }) + issued.status_code.should eq 200 + access = JSON.parse(issued.body)["access_token"].as_s + + # Decode the JOSE header without verifying — that is exactly what an + # RP does before it knows which key to verify with. + header = JSON.parse(String.new(Base64.decode(access.split('.').first + "=="))) + header["alg"].as_s.should eq "RS256" + header["typ"].as_s.should eq "JWT" + header.as_h.has_key?("kid").should be_false + + # The key it WOULD have named, and the reason a fallback works today: + # there is exactly one. + keys = JSON.parse( + client.get("/auth/oauth/discovery/keys", headers: HTTP::Headers{"Host" => "localhost"}).body + )["keys"].as_a + keys.size.should eq 1 + keys.first["kid"].as_s.should_not be_empty + ensure + app.try &.destroy + end end end diff --git a/spec/controllers/multi_tenant_spec.cr b/spec/controllers/multi_tenant_spec.cr new file mode 100644 index 0000000..2402a39 --- /dev/null +++ b/spec/controllers/multi_tenant_spec.cr @@ -0,0 +1,264 @@ +require "../helper" +require "jwt" + +module PlaceOS::Auth + # Multi-tenant boundaries — PPT-2536 test-matrix rows MT-03 and IR-04. + # + # One auth.cr process serves every authority (eight on dev), so every + # tenant boundary here is enforced in code rather than by deployment. + # `bearer_credentials_spec.cr` covers the token half (AZ-06/MT-02): a token + # minted for one authority is refused at another. This covers the two + # boundaries either side of it — the *session* that precedes a token, and + # introspection, which reads other people's tokens for a living. + describe OAuth, tags: "multi-tenant" do + decode = ->(token : String) { + payload, _ = JWT.decode(token, ::Authly.config.public_key.as(String), JWT::Algorithm::RS256) + payload + } + + # A second authority, with a user and a password, alongside `localhost`. + other_tenant = -> { + authority = ::PlaceOS::Model::Generator.authority + authority.domain = "tenant-#{Random.rand(999_999)}.example" + authority.save! + password = "bcrypt-please-#{Random.rand(999_999)}" + user = ::PlaceOS::Model::Generator.user(authority) + user.password = password + user.save! + {authority, user, password} + } + + local_user = -> { + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + password = "bcrypt-please-#{Random.rand(999_999)}" + user = ::PlaceOS::Model::Generator.user(authority) + user.password = password + user.save! + {authority, user, password} + } + + # `Spec.signin!` hardcodes `Host: localhost`, so it cannot sign a user + # in at any other authority — which is exactly what these rows need. + signin_at = ->(host : String, user : ::PlaceOS::Model::User, password : String) { + result = client.post("/auth/signin", + headers: HTTP::Headers{"Host" => host, "Content-Type" => "application/json"}, + body: {email: user.email.to_s, password: password}.to_json) + raise "signin at #{host} failed: #{result.status_code} #{result.body}" unless {202, 303}.includes?(result.status_code) + session = result.cookies[PlaceOS::Auth::SESSION_COOKIE_NAME]? + raise "signin at #{host} set no session cookie" if session.nil? + "#{session.name}=#{session.value}" + } + + make_app = ->(owner : ::PlaceOS::Model::User, slug : String) { + redirect = "https://mt.example/cb-#{slug}-#{Random.rand(999_999)}" + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "mt-#{slug}-#{Random.rand(999_999)}" + app.redirect_uri = redirect + app.scopes = "public" + app.owner_id = owner.id.as(String) + app.confidential = true + app.save! + {app, redirect} + } + + # ---- MT-03: the session is not bound to an authority ---------------- + + describe "session / authority binding (MT-03)" do + it "accepts a session cookie issued by one authority at another (PINNED)" do + # `new_session` stores `uid`, `exp` and `iat` — no authority — and + # `session_user` resolves it with a bare `User.find?(uid)`. The + # session cookie is encrypted with ONE process-wide + # `COOKIE_SESSION_SECRET`, so a cookie minted while signed in at + # authority A decrypts and validates at authority B. + # + # Why this is a boundary worth writing down rather than a hole to + # panic about: a browser will never do this by itself — cookies are + # host-scoped, so B never receives A's cookie. It needs an attacker + # who already holds the cookie value, and at that point they hold the + # session regardless. And it does not yield a usable credential: the + # token minted from it carries the USER's authority in `aud` (see the + # next case), and `ensure_matching_domain` then refuses it at B. + # + # So the tenant boundary is enforced on the TOKEN, not on the + # session. Pinned because that is a real design fact — anyone adding + # a session-only surface to auth.cr (an admin page, a form post) + # inherits this and would need their own check. + _, user, password = local_user.call + cookie = Spec.signin!(client, user, password) + foreign_authority, _, _ = other_tenant.call + + result = client.get("/auth/authority", headers: HTTP::Headers{ + "Host" => foreign_authority.domain, "Cookie" => cookie, + }) + + result.status_code.should eq 200 + JSON.parse(result.body)["session"].as_bool.should be_true + ensure + user.try &.destroy + foreign_authority.try &.destroy + end + + it "stamps the token with the USER's authority, not the request Host" do + # The control that makes the case above safe, and the assertion that + # would fail first if it stopped being true. A code minted while + # talking to authority B, with a session belonging to a `localhost` + # user, still produces a token whose `aud` is `localhost` — so + # `ensure_matching_domain` refuses it at B. + _, user, password = local_user.call + foreign_authority, _, _ = other_tenant.call + app, redirect = make_app.call(user, "aud") + cookie = Spec.signin!(client, user, password) + + authorized = client.get( + "/auth/authorize?response_type=code" \ + "&client_id=#{URI.encode_www_form(app.uid.as(String))}" \ + "&redirect_uri=#{URI.encode_www_form(redirect)}&scope=public", + headers: HTTP::Headers{"Host" => foreign_authority.domain, "Cookie" => cookie}, + ) + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => foreign_authority.domain, + "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "authorization_code") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("code", code) + fp.add("redirect_uri", redirect) + }) + token.status_code.should eq 200 + + access = JSON.parse(token.body)["access_token"].as_s + claims = decode.call(access) + # Minted while addressing the foreign authority, but bound to the + # user's own. + claims["aud"].as_s.should eq "localhost" + claims["aud"].as_s.should_not eq foreign_authority.domain + claims["sub"].as_s.should eq user.id.as(String) + + # And therefore unusable at the authority it was requested through. + refused = client.get("/auth/userinfo", headers: HTTP::Headers{ + "Host" => foreign_authority.domain, "Authorization" => "Bearer #{access}", + }) + refused.status_code.should eq 401 + ensure + app.try &.destroy + user.try &.destroy + foreign_authority.try &.destroy + end + end + + # ---- IR-04: introspection across tenants ---------------------------- + + describe "cross-tenant introspection (IR-04)" do + it "reports another tenant's token as inactive rather than describing it" do + # RFC 7662 §2.2: an introspection response for a token the caller is + # not entitled to see must be `{"active": false}` — not an error, and + # certainly not the token's metadata. `introspection_revocation_spec` + # covers the cross-*application* case within one tenant; this is the + # same guard across authorities, where a leak would cross an + # organisational boundary rather than an app one. + _, local, local_password = local_user.call + foreign_authority, foreign_user, foreign_password = other_tenant.call + + victim_app, victim_redirect = make_app.call(foreign_user, "victim") + caller_app, _ = make_app.call(local, "caller") + + # Mint a real token for the foreign tenant's app. + foreign_cookie = signin_at.call(foreign_authority.domain, foreign_user, foreign_password) + authorized = client.get( + "/auth/authorize?response_type=code" \ + "&client_id=#{URI.encode_www_form(victim_app.uid.as(String))}" \ + "&redirect_uri=#{URI.encode_www_form(victim_redirect)}&scope=public", + headers: HTTP::Headers{"Host" => foreign_authority.domain, "Cookie" => foreign_cookie}, + ) + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + issued = client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => foreign_authority.domain, + "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "authorization_code") + fp.add("client_id", victim_app.uid.as(String)) + fp.add("client_secret", victim_app.secret) + fp.add("code", code) + fp.add("redirect_uri", victim_redirect) + }) + issued.status_code.should eq 200 + victim_token = JSON.parse(issued.body)["access_token"].as_s + + # A different tenant's client asks about it, authenticating properly + # as itself. + result = client.post("/auth/introspect", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("token", victim_token) + fp.add("client_id", caller_app.uid.as(String)) + fp.add("client_secret", caller_app.secret) + }) + + result.status_code.should eq 200 + body = JSON.parse(result.body) + body["active"].as_bool.should be_false + # Nothing about the token may leak alongside the `false` — not the + # subject, not the scope, not the owning client. + body.as_h.has_key?("scope").should be_false + body.as_h.has_key?("client_id").should be_false + body.as_h.has_key?("exp").should be_false + result.body.should_not contain foreign_user.id.as(String) + result.body.should_not contain victim_app.uid.as(String) + ensure + victim_app.try &.destroy + caller_app.try &.destroy + local.try &.destroy + foreign_user.try &.destroy + foreign_authority.try &.destroy + end + + it "still describes the token to its own client (the control)" do + # Without this, a build whose introspection always answered + # `{"active": false}` would pass the case above. + _, local, _ = local_user.call + app, redirect = make_app.call(local, "self") + + issued = client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "client_credentials") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("scope", "public") + }) + issued.status_code.should eq 200 + own_token = JSON.parse(issued.body)["access_token"].as_s + + result = client.post("/auth/introspect", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("token", own_token) + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + }) + + result.status_code.should eq 200 + JSON.parse(result.body)["active"].as_bool.should be_true + ensure + app.try &.destroy + local.try &.destroy + end + end + end +end diff --git a/spec/controllers/oauth_alias_spec.cr b/spec/controllers/oauth_alias_spec.cr index 9cfc83b..a7d245b 100644 --- a/spec/controllers/oauth_alias_spec.cr +++ b/spec/controllers/oauth_alias_spec.cr @@ -84,5 +84,76 @@ module PlaceOS::Auth result.status_code.should eq 303 result.headers["Location"].should eq "/auth/login" end + + # ---- TK-09: the two token mounts must not drift apart -------------- + + it "answers the short and legacy token paths identically (TK-09)" do + # `/auth/token` is what ts-client calls; `/auth/oauth/token` is the + # Doorkeeper mount external integrators hardcoded. They are stacked + # route annotations on ONE method today, so they cannot diverge — but + # that is an implementation detail, and the drop-in promise is that + # both behave the same. This is what fails if anyone ever splits them, + # to deprecate one or to hang a filter on just one. + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + user = ::PlaceOS::Model::Generator.user(authority) + user.save! + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "alias-parity-#{Random.rand(999_999)}" + app.redirect_uri = "https://alias.example/cb-#{Random.rand(999_999)}" + app.scopes = "public" + app.confidential = true + app.owner_id = user.id.as(String) + app.save! + + request = ->(path : String) { + client.post(path, + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "client_credentials") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("scope", "public") + }) + } + + short = request.call("/auth/token") + legacy = request.call("/auth/oauth/token") + + short.status_code.should eq 200 + legacy.status_code.should eq 200 + + # Same envelope and the same cache headers. The tokens themselves + # differ — each call mints a new one — so compare shape, not bytes. + JSON.parse(short.body).as_h.keys.sort!.should eq JSON.parse(legacy.body).as_h.keys.sort! + short.headers["Cache-Control"].should eq legacy.headers["Cache-Control"] + short.headers["Pragma"].should eq legacy.headers["Pragma"] + JSON.parse(short.body)["token_type"].as_s.should eq JSON.parse(legacy.body)["token_type"].as_s + JSON.parse(short.body)["expires_in"].as_i.should eq JSON.parse(legacy.body)["expires_in"].as_i + + # Both tokens actually verify — a shape match on two broken tokens + # would otherwise pass. + ::Authly.valid?(JSON.parse(short.body)["access_token"].as_s).should be_true + ::Authly.valid?(JSON.parse(legacy.body)["access_token"].as_s).should be_true + + # Identical REJECTION too, not just identical success. + bad = ->(path : String) { + client.post(path, + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: "grant_type=client_credentials&client_id=ghost-#{Random.rand(999_999)}&client_secret=x") + } + bad_short = bad.call("/auth/token") + bad_legacy = bad.call("/auth/oauth/token") + bad_short.status_code.should eq bad_legacy.status_code + bad_short.status_code.should eq 401 + JSON.parse(bad_short.body)["error"].as_s.should eq JSON.parse(bad_legacy.body)["error"].as_s + bad_short.headers["WWW-Authenticate"].should eq bad_legacy.headers["WWW-Authenticate"] + ensure + app.try &.destroy + user.try &.destroy + end end end diff --git a/spec/controllers/oidc_id_token_spec.cr b/spec/controllers/oidc_id_token_spec.cr index 6d606b4..2ebd7a3 100644 --- a/spec/controllers/oidc_id_token_spec.cr +++ b/spec/controllers/oidc_id_token_spec.cr @@ -460,5 +460,160 @@ module PlaceOS::Auth user.try &.destroy end end + + # ---- OI-04: nonce --------------------------------------------------- + + describe "nonce is not supported (OI-04)" do + it "drops a nonce sent to the authorize endpoint instead of echoing it" do + # OIDC Core §3.1.2.1 makes `nonce` OPTIONAL for the code flow, and + # §3.1.3.7 step 11 says that if the client sent one, it MUST verify + # the same value comes back in the ID token. auth.cr never captures + # it: `/auth/authorize` has no `nonce` parameter, `Authly::Code` + # has no field for it, and `AuthlyAdapter::Owner#id_token` never + # emits one (there is a comment saying as much). + # + # The consequence is a real interop limit, not a cosmetic gap: an RP + # library that sends `nonce` by default — most do, even on the code + # flow — will reject our ID token as tampered. It is a *safe* + # failure (login refused, never wrongly accepted), and no PlaceOS + # client sends one today, which is why it has never been noticed. + # Pinned so that anyone integrating an external RP finds this here + # rather than in a support ticket, and so an implementation lands + # with a test already waiting for it. + user, app, redirect, cookie, password = nil, nil, nil, nil, nil + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + user = ::PlaceOS::Model::Generator.user(authority) + password = "bcrypt-please-#{Random.rand(99999)}" + user.password = password + user.save! + + redirect = "https://oidc.example/cb/#{UUID.random}" + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "oidc-nonce-#{Random.rand(99999)}" + app.redirect_uri = redirect + app.scopes = "openid public" + app.owner_id = user.id.as(String) + app.confidential = true + app.save! + + cookie = Spec.signin!(client, user, password) + the_nonce = "nonce-#{Random.rand(999_999_999)}" + authorized = client.get( + "/auth/oauth/authorize?response_type=code" \ + "&client_id=#{URI.encode_www_form(app.uid.as(String))}" \ + "&redirect_uri=#{URI.encode_www_form(redirect)}" \ + "&scope=#{URI.encode_www_form("openid public")}" \ + "&nonce=#{URI.encode_www_form(the_nonce)}", + headers: HTTP::Headers{"Host" => "localhost", "Cookie" => cookie}, + ) + + # An unknown parameter must not break the flow — the login still + # works, which is exactly why the missing claim is easy to miss. + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = form_post.call("/auth/token", { + "grant_type" => "authorization_code", + "client_id" => app.uid.as(String), + "client_secret" => app.secret, + "code" => code, + "redirect_uri" => redirect, + }) + token.status_code.should eq 200 + + id_token = JSON.parse(token.body)["id_token"].as_s + id_token.should_not be_empty + payload, _ = JWT.decode(id_token, ::Authly.config.public_key.as(String), JWT::Algorithm::RS256) + + # The claim an OIDC-conformant RP would check, and its absence. + payload.as_h.has_key?("nonce").should be_false + # Positive control: the ID token IS otherwise well-formed, so this is + # a missing claim and not a broken flow. + payload["sub"].as_s.should eq user.id.as(String) + payload["aud"].as_s.should eq app.uid.as(String) + ensure + app.try &.destroy + user.try &.destroy + end + end + + # ---- OI-05 / OI-06: the userinfo endpoint --------------------------- + + describe "userinfo (OI-05, OI-06)" do + it "answers GET and POST identically, with a sub matching the id_token" do + # OIDC Core §5.3 requires both verbs, and §5.3.2 requires the + # `sub` returned here to match the `sub` of the ID token issued in + # the same grant. An RP that keys its user records off userinfo + # while validating the ID token separately breaks silently if the + # two ever disagree — `discovery_spec.cr` proves both verbs are + # ROUTED; this proves they agree, and with what. + user, app, _redirect, token = authorize_and_exchange.call("openid public") + access = token["access_token"].as_s + id_payload, _ = JWT.decode(token["id_token"].as_s, + ::Authly.config.public_key.as(String), JWT::Algorithm::RS256) + + headers = HTTP::Headers{"Host" => "localhost", "Authorization" => "Bearer #{access}"} + via_get = client.get("/auth/userinfo", headers: headers) + via_post = client.post("/auth/userinfo", headers: headers) + + via_get.status_code.should eq 200 + via_post.status_code.should eq 200 + JSON.parse(via_get.body).should eq JSON.parse(via_post.body) + + subject = JSON.parse(via_get.body)["sub"].as_s + subject.should eq user.id.as(String) + subject.should eq id_payload["sub"].as_s + ensure + app.try &.destroy + user.try &.destroy + end + + it "404s with an empty error once the user behind the token is gone (OI-05 — DIVERGENCE)" do + # A token can outlive its user: a deletion, or a tenant teardown, + # inside the 2-hour access-token window. What comes back is + # `404 {"error":""}`. + # + # Why 404 and not the `unknown subject` 401 the code appears to + # intend: `authorize!` calls `::PlaceOS::Model::User.find(...)`, + # which RAISES `PgORM::Error::RecordNotFound` rather than returning + # nil. That escapes the `rescue e : JWT::Error` around it and lands + # on the base controller's RecordNotFound handler — so + # `OAuth#userinfo`'s own `raise Error::Unauthorized.new("unknown + # subject")` guard is never reached by this route. (It still covers + # the guest-scope path, which skips the user lookup entirely.) + # + # Two problems, both diagnosability rather than security. RFC 6750 + # §3.1 and OIDC Core §5.3.3 want 401 `invalid_token` here — an RP + # reading 404 concludes the *endpoint* is missing and may disable + # userinfo entirely, rather than refreshing the token. And the body + # carries `{"error":""}`, because `RecordNotFound` is raised with no + # message, so nothing in the response says what was not found. + # + # Pinned rather than fixed: changing it means either making + # `authorize!` tolerate the missing row (which is a real semantic + # decision about whether a userless token is authenticated at all) + # or catching RecordNotFound per-controller. Worth doing + # deliberately, with the guest-scope path considered alongside. + user, app, _redirect, token = authorize_and_exchange.call("openid public") + access = token["access_token"].as_s + headers = HTTP::Headers{"Host" => "localhost", "Authorization" => "Bearer #{access}"} + + # Control: it works while the user exists, so the change below is + # attributable to the deletion and nothing else. + client.get("/auth/userinfo", headers: headers).status_code.should eq 200 + + user.destroy + user = nil + + result = client.get("/auth/userinfo", headers: headers) + result.status_code.should eq 404 + JSON.parse(result.body)["error"].as_s.should be_empty + # Whatever else changes, no claims may leak for a user that is gone. + result.body.should_not contain "\"sub\"" + ensure + app.try &.destroy + user.try &.destroy + end + end end end diff --git a/spec/controllers/pkce_spec.cr b/spec/controllers/pkce_spec.cr index c5ef4b2..1e110e4 100644 --- a/spec/controllers/pkce_spec.cr +++ b/spec/controllers/pkce_spec.cr @@ -251,5 +251,137 @@ module PlaceOS::Auth app.try &.destroy user.try &.destroy end + + # ---- PK-05 / PK-06 / OI-09: which methods are really accepted ------ + # + # Discovery advertises `code_challenge_methods_supported: ["S256"]` + # (`oauth.cr`, Discovery::Response). A capability document is a promise + # about what the server does, so what it *actually* accepts had better + # match — and the challenge method is not a detail: with `plain`, the + # challenge and the verifier are the same string, and the challenge + # travels in the authorize URL. Anything that reads that URL — nginx's + # access log, the browser's history, a `Referer` — holds the verifier. + # That is precisely why RFC 7636 §7.2 tells a server supporting S256 to + # refuse `plain`. + describe "code_challenge_method handling (PK-05, PK-06, OI-09)" do + authorize_with = ->(app : ::PlaceOS::Model::DoorkeeperApplication, redirect : String, cookie : String, challenge : String, method : String) { + query = String.build do |io| + io << "/auth/authorize?response_type=code" + io << "&client_id=" << URI.encode_www_form(app.uid.as(String)) + io << "&redirect_uri=" << URI.encode_www_form(redirect) + io << "&scope=public" + io << "&code_challenge=" << URI.encode_www_form(challenge) + io << "&code_challenge_method=" << URI.encode_www_form(method) + end + client.get(query, headers: HTTP::Headers{"Host" => "localhost", "Cookie" => cookie}) + } + + exchange = ->(app : ::PlaceOS::Model::DoorkeeperApplication, redirect : String, code : String, verifier : String) { + client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "authorization_code") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("code", code) + fp.add("redirect_uri", redirect) + fp.add("code_verifier", verifier) + }) + } + + it "advertises S256 as the only supported method" do + result = client.get("/.well-known/openid-configuration", + headers: HTTP::Headers{"Host" => "localhost"}) + result.status_code.should eq 200 + JSON.parse(result.body)["code_challenge_methods_supported"] + .as_a.map(&.as_s).should eq ["S256"] + end + + it "accepts a `plain` challenge anyway (PK-05 — DIVERGENCE from the advertised set)" do + # The advertised set says S256 only; the implementation takes `plain` + # too. `normalize_code_challenge` deliberately transforms S256 alone + # and passes every other method through untouched, and authly then + # compares the verifier verbatim. + # + # Not currently exploitable across clients: the method is baked into + # the code at authorize time, so an attacker cannot downgrade a + # *legitimate* client's code — the client picks its own method. The + # exposure is that a naive or hostile client can opt itself into a + # scheme where the authorize URL carries its own proof of possession, + # while discovery tells integrators that cannot happen. + redirect = "https://spa.example/cb-plain-#{Random.rand(99999)}" + user, app, password = make_app.call(redirect) + cookie = Spec.signin!(client, user, password) + verifier = "plain-verifier-#{Random.rand(999_999)}" + + authorized = authorize_with.call(app, redirect, cookie, verifier, "plain") + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = exchange.call(app, redirect, code, verifier) + token.status_code.should eq 200 + JSON.parse(token.body)["access_token"].as_s.should_not be_empty + ensure + app.try &.destroy + user.try &.destroy + end + + it "still checks a `plain` challenge against the verifier" do + # The control for the case above: `plain` is accepted, but it is not + # a no-op — a wrong verifier is still refused. Without this, the + # test above would equally describe a build that ignored PKCE. + redirect = "https://spa.example/cb-plainbad-#{Random.rand(99999)}" + user, app, password = make_app.call(redirect) + cookie = Spec.signin!(client, user, password) + + authorized = authorize_with.call(app, redirect, cookie, "the-real-verifier", "plain") + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = exchange.call(app, redirect, code, "not-the-verifier") + token.status_code.should_not eq 200 + JSON.parse(token.body)["error"].as_s.should_not be_empty + ensure + app.try &.destroy + user.try &.destroy + end + + it "mints an unredeemable code for an unknown method (PK-06)" do + # RFC 7636 §4.3 makes `code_challenge_method` a value the server must + # understand, and §4.4.1 says an unsupported one should be rejected + # at the AUTHORIZE endpoint with `invalid_request`. auth.cr passes it + # through instead: `normalize_code_challenge` transforms S256 alone, + # every other method is stored verbatim, and the code is minted. + # + # It fails CLOSED, which is the part that matters — the exchange is + # refused, so an unknown method can never weaken the check. What it + # costs is diagnosability: the client gets a 302 and a code that + # looks fine, then a 401 `unauthorized_client` one round trip later. + # That code says "this client may not use this grant type", which + # sends an integrator to their client registration when the actual + # problem is one query parameter they chose. Same misdirection class + # as the boot-time `oauth_tokens` check (XO-03) and the undecodable + # -grant 500 — pinned here so the cost is visible if anyone decides + # to move the rejection to the authorize endpoint where it belongs. + redirect = "https://spa.example/cb-s512-#{Random.rand(99999)}" + user, app, password = make_app.call(redirect) + cookie = Spec.signin!(client, user, password) + verifier = "s512-verifier-#{Random.rand(999_999)}" + + authorized = authorize_with.call(app, redirect, cookie, verifier, "S512") + # Accepted here — this is the part RFC 7636 says should have failed. + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = exchange.call(app, redirect, code, verifier) + token.status_code.should eq 401 + JSON.parse(token.body)["error"].as_s.should eq "unauthorized_client" + ensure + app.try &.destroy + user.try &.destroy + end + end end end diff --git a/spec/controllers/refresh_semantics_spec.cr b/spec/controllers/refresh_semantics_spec.cr index 2c1e75a..a24d50d 100644 --- a/spec/controllers/refresh_semantics_spec.cr +++ b/spec/controllers/refresh_semantics_spec.cr @@ -359,5 +359,168 @@ module PlaceOS::Auth user.try &.destroy end end + + # ---- RF-10: a refresh may narrow the grant, never widen it --------- + # + # RFC 6749 §6: "The requested scope MUST NOT include any scope not + # originally granted by the resource owner." Doorkeeper enforced exactly + # that — `RefreshTokenRequest#validate_scope` checks the requested scope + # against `refresh_token.scopes`, the scope the grant actually carried. + # + # authly checks the requested scope against the CLIENT REGISTRATION only + # (`Authly.clients.allowed_scopes?`), which is a different question: it + # asks "may this client ever hold this scope", not "was this scope + # granted to this token". A client registered for more than it was + # granted could therefore widen its own token on refresh. + describe "scope narrowing and widening (RF-10)" do + # A client REGISTERED for two scopes, whose user grants only one. + # That gap is the whole point: it is where widening becomes visible. + two_scope_grant = ->(granted : String) { + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + password = "bcrypt-please-#{Random.rand(999_999)}" + user = ::PlaceOS::Model::Generator.user(authority) + user.password = password + user.save! + + redirect = "https://rf10.example/cb-#{Random.rand(999_999)}" + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "rf10-#{Random.rand(999_999)}" + app.redirect_uri = redirect + app.scopes = "public users" + app.confidential = true + app.owner_id = user.id.as(String) + app.save! + + cookie = Spec.signin!(client, user, password) + authorize_path = String.build do |io| + io << "/auth/authorize?response_type=code" + io << "&client_id=" << URI.encode_www_form(app.uid.as(String)) + io << "&redirect_uri=" << URI.encode_www_form(redirect) + io << "&scope=" << URI.encode_www_form(granted) + end + authorized = client.get(authorize_path, headers: HTTP::Headers{ + "Host" => "localhost", "Cookie" => cookie, + }) + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + exchanged = client.post("/auth/token", headers: form_headers, body: URI::Params.build { |fp| + fp.add("grant_type", "authorization_code") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("code", code) + fp.add("redirect_uri", redirect) + }) + exchanged.status_code.should eq 200 + body = JSON.parse(exchanged.body) + # The grant really is narrower than the registration. + scopes_of.call(decode.call(body["access_token"].as_s)).should eq granted.split + {user, app, body["refresh_token"].as_s} + } + + refresh_asking = ->(app : ::PlaceOS::Model::DoorkeeperApplication, token : String, scope : String) { + client.post("/auth/token", headers: form_headers, body: URI::Params.build { |fp| + fp.add("grant_type", "refresh_token") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("refresh_token", token) + fp.add("scope", scope) + }) + } + + it "does NOT widen the grant when the refresh asks for more" do + # The security-relevant half. Granted `public`, registered for + # `public users`, asking for `users` back. + user, app, refresh_token = two_scope_grant.call("public") + + result = refresh_asking.call(app, refresh_token, "public users") + + # Accepted — but the token that comes back carries the GRANTED + # scope, not the requested one. No escalation. + result.status_code.should eq 200 + claims = decode.call(JSON.parse(result.body)["access_token"].as_s) + scopes_of.call(claims).should eq ["public"] + scopes_of.call(claims).should_not contain "users" + claims["sub"].as_s.should eq user.id.as(String) + ensure + app.try &.destroy + user.try &.destroy + end + + it "does NOT widen even when the extra scope is the only one asked for" do + user, app, refresh_token = two_scope_grant.call("public") + + result = refresh_asking.call(app, refresh_token, "users") + + result.status_code.should eq 200 + scopes_of.call(decode.call(JSON.parse(result.body)["access_token"].as_s)).should eq ["public"] + ensure + app.try &.destroy + user.try &.destroy + end + + it "does NOT narrow either — the scope parameter is ignored on refresh (DIVERGENCE)" do + # RFC 6749 §6 lets a client refresh into a NARROWER scope, and + # Doorkeeper honoured it (`RefreshTokenRequest#validate_scope` + # checks the request against `refresh_token.scopes`). auth.cr honours + # neither direction, for one reason: `oauth.cr`'s refresh branch + # calls `Authly.access_token(grant_type:, client_id:, client_secret:, + # refresh_token:)` and never forwards `scope`, so `Grant#@scope` is + # nil and `Grant#scope` falls through to the scope recovered from the + # refresh token. + # + # Pinned rather than fixed, deliberately. The current behaviour errs + # SAFE — a client can never gain a scope it was not granted. Making + # narrowing work means forwarding the parameter, and at that point + # `validate_scope!` only checks the CLIENT REGISTRATION + # (`Authly.clients.allowed_scopes?`), which asks "may this client ever + # hold this scope", not "was it granted to this token". Forwarding it + # without also adding a granted-scope subset check would convert this + # safe divergence into a real privilege escalation. That is a change + # worth making deliberately, not as a side effect. + user, app, refresh_token = two_scope_grant.call("public users") + + result = refresh_asking.call(app, refresh_token, "public") + + result.status_code.should eq 200 + # Asked for `public`, handed back `public users`. + scopes_of.call(decode.call(JSON.parse(result.body)["access_token"].as_s)).should eq ["public", "users"] + ensure + app.try &.destroy + user.try &.destroy + end + + it "returns exactly the granted scope when the refresh asks for it" do + user, app, refresh_token = two_scope_grant.call("public users") + + result = refresh_asking.call(app, refresh_token, "public users") + + result.status_code.should eq 200 + scopes_of.call(decode.call(JSON.parse(result.body)["access_token"].as_s)).should eq ["public", "users"] + ensure + app.try &.destroy + user.try &.destroy + end + + it "still carries the granted scope when the refresh asks for nothing" do + # The existing behaviour the scope-recovery patch exists to protect + # (the 2026-07-25 revert). Asserted here too so a narrowing check + # cannot regress it into an empty scope. + user, app, refresh_token = two_scope_grant.call("public users") + + result = client.post("/auth/token", headers: form_headers, body: URI::Params.build { |fp| + fp.add("grant_type", "refresh_token") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("refresh_token", refresh_token) + }) + + result.status_code.should eq 200 + scopes_of.call(decode.call(JSON.parse(result.body)["access_token"].as_s)).should eq ["public", "users"] + ensure + app.try &.destroy + user.try &.destroy + end + end end end diff --git a/spec/controllers/saml_callbacks_spec.cr b/spec/controllers/saml_callbacks_spec.cr index fe0859b..06a2029 100644 --- a/spec/controllers/saml_callbacks_spec.cr +++ b/spec/controllers/saml_callbacks_spec.cr @@ -326,5 +326,210 @@ module PlaceOS::Auth strat.try &.destroy end end + + # ---- ID-04: what the Conditions actually buy us -------------------- + # + # A SAML assertion is a *bearer* credential: anyone holding the bytes can + # present them. Everything that stops a captured assertion being reused + # lives in `Conditions` and the surrounding envelope, and every one of + # those checks in `crystal-saml` is guarded by a "skip if not configured" + # clause. So which of them are live depends entirely on what + # `external_providers.cr#build_saml` passes — this block pins that, both + # where it holds and where it does not. + # + # Matters directly for UCLA, which is a SAML/Shibboleth deployment. + describe "assertion conditions (ID-04)", tags: "saml-conditions" do + acs = "http://localhost/auth/adfs/callback" + + strat_with_cert = -> { + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + strat = ::PlaceOS::Model::SamlAuthentication.new + strat.name = "cond-saml-#{Random.rand(99999)}" + strat.issuer = "https://sp.example.test/cond-#{Random.rand(99999)}" + strat.idp_sso_target_url = "https://idp.example.test/sso" + strat.assertion_consumer_service_url = acs + strat.uid_attribute = "email" + strat.idp_cert = Spec::SamlFixtures.idp_cert_pem + strat.authority_id = authority.id + strat.save! + strat + } + + post_assertion = ->(strat : ::PlaceOS::Model::SamlAuthentication, saml_response : String) { + client.post( + "/auth/adfs/callback?id=#{URI.encode_www_form(strat.id.as(String))}", + headers: HTTP::Headers{ + "Host" => "localhost", + "Content-Type" => "application/x-www-form-urlencoded", + }, + body: "SAMLResponse=#{URI.encode_www_form(saml_response)}&RelayState=#{URI.encode_www_form("/backoffice/")}", + ) + } + + lookup_for = ->(email : String) { + ::PlaceOS::Model::UserAuthLookup.where(uid: email, provider: "adfs").first? + } + + refused = ->(result : HTTP::Client::Response, email : String) { + lookup = lookup_for.call(email) + location = result.headers["Location"]? + rejected = result.status_code >= 400 || location.try(&.includes?("/auth/failure")) || false + if lookup || !rejected + fail "ASSERTION WAS NOT REJECTED — status=#{result.status_code} " \ + "location=#{location.inspect} lookup_created=#{!lookup.nil?} " \ + "body=#{result.body[0, 200].inspect}" + end + } + + admitted = ->(result : HTTP::Client::Response, email : String) { + lookup = lookup_for.call(email) + if lookup.nil? + fail "ASSERTION WAS NOT ACCEPTED — status=#{result.status_code} " \ + "location=#{result.headers["Location"]?.inspect} body=#{result.body[0, 200].inspect}" + end + lookup.not_nil! + } + + cleanup = ->(email : String) { + if lookup = lookup_for.call(email) + if user_id = lookup.user_id + ::PlaceOS::Model::User.find?(user_id).try &.destroy + end + lookup.destroy + end + } + + # The control for the whole block. Without it, a build that refused + # every assertion would pass all the rejection cases below. + it "admits an assertion inside its validity window" do + strat = strat_with_cert.call + email = "saml-cond-ok-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email, + not_before: 5.minutes.ago, not_on_or_after: 30.minutes.from_now)) + + admitted.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "refuses an assertion whose NotOnOrAfter has passed" do + # This is the ONLY thing bounding replay of a captured assertion — + # see the one-time-use case at the end of this block. + strat = strat_with_cert.call + email = "saml-cond-exp-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email, + not_before: 2.hours.ago, not_on_or_after: 1.hour.ago)) + + refused.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "refuses an assertion that is not valid yet (NotBefore in the future)" do + strat = strat_with_cert.call + email = "saml-cond-early-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email, + not_before: 1.hour.from_now, not_on_or_after: 2.hours.from_now)) + + refused.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "refuses an assertion minted for a different Service Provider" do + # `validate_audience` compares against `settings.sp_entity_id`, which + # `build_saml` sets from `strat.issuer`. If that were ever left blank + # the check short-circuits to `true` and any assertion the IdP issued + # for ANY service protected by the same IdP would log the bearer in + # here. This is the assertion that catches that. + strat = strat_with_cert.call + email = "saml-cond-aud-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: "https://someone-else.example.test/metadata", email: email)) + + refused.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "refuses an assertion addressed to a different Destination" do + # `validate_destination` compares the Response's `Destination` against + # the configured ACS URL, so an assertion captured from another + # deployment's endpoint cannot be posted here. + strat = strat_with_cert.call + email = "saml-cond-dest-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: "https://elsewhere.example.test/auth/adfs/callback", + audience: strat.issuer.as(String), email: email)) + + refused.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + # ---- gaps, pinned so they stay deliberate ---------------------- + + it "does NOT validate the declared Issuer — trust rests on the cert alone" do + # `crystal-saml`'s `validate_issuer` short-circuits to true when + # `settings.idp_entity_id` is nil, and `build_saml` never passes it + # (`external_providers.cr#build_saml` sets sp_entity_id, idp_cert, + # idp_cert_fingerprint … but no idp_entity_id). So `` is + # decorative here. + # + # That is not currently exploitable: the signature must still verify + # against the strat's pinned `idp_cert`, so an attacker cannot mint + # one. It becomes load-bearing the moment a strat trusts more than one + # key, or a cert is reused across IdPs. Pinned rather than fixed + # because adding the check needs an `idp_entity_id` column and a + # value for every existing strat — a migration, not a code change. + strat = strat_with_cert.call + email = "saml-cond-iss-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email, + idp_entity_id: "https://not-the-configured-idp.example.test/metadata")) + + admitted.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "does NOT enforce one-time use — the same assertion replays until it expires" do + # There is no assertion-ID cache, and the SAML callback deliberately + # skips the session-state check (see "callback state check" above), so + # nothing binds a response to a request we issued. An assertion + # captured in transit, from a proxy log or from browser history can be + # POSTed again by anyone until `NotOnOrAfter`. + # + # The legacy Ruby service behaved the same way, and Shibboleth issues + # short windows, so this is the accepted SAML bearer-token model + # rather than a regression. It is pinned here so the exposure is a + # recorded decision — and so that if someone adds a replay cache, this + # spec is what tells them the behaviour changed. + strat = strat_with_cert.call + email = "saml-cond-replay-#{Random.rand(99999)}@localhost" + encoded = Spec::SamlFixtures.encode( + Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email))) + + first = admitted.call(post_assertion.call(strat, encoded), email) + second_result = post_assertion.call(strat, encoded) + + # Same bytes, accepted again, resolving to the same identity. + second = admitted.call(second_result, email) + second.id.should eq first.id + ensure + cleanup.call(email) if email + strat.try &.destroy + end + end end end diff --git a/spec/controllers/sessions_spec.cr b/spec/controllers/sessions_spec.cr index bee7562..f806ea5 100644 --- a/spec/controllers/sessions_spec.cr +++ b/spec/controllers/sessions_spec.cr @@ -275,6 +275,54 @@ module PlaceOS::Auth end end + # ---- SC-09: per-authority session timeout --------------------------- + + describe "per-authority session timeout (SC-09)" do + it "honours internals.session_timeout over the env default" do + # `authority_session_timeout` reads `Authority#internals` + # ["session_timeout"] and falls back to `SESSION_TIMEOUT_MINUTES`. + # A tenant that sets a shorter window is making a security decision, + # so the override actually being read is the whole point of the row. + # + # Driving it with a NEGATIVE window is what makes this observable in + # a spec: the session's `exp` is inside the encrypted cookie and + # cannot be read from outside, but a session that was already expired + # when it was issued shows up immediately as `session: false`, while + # the same signin under the default shows `session: true`. + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + original = authority.internals.dup + password = "ok-password-1234" + user = create_user.call(password) + + # Control first, under the default timeout. + cookie = Spec.signin!(client, user, password) + live = client.get("/auth/authority", headers: HTTP::Headers{ + "Host" => "localhost", "Cookie" => cookie, + }) + JSON.parse(live.body)["session"].as_bool.should be_true + + authority.internals = authority.internals.merge({ + "session_timeout" => JSON::Any.new(-1_i64), + }) + authority.save! + + expired_cookie = Spec.signin!(client, user, password) + result = client.get("/auth/authority", headers: HTTP::Headers{ + "Host" => "localhost", "Cookie" => expired_cookie, + }) + + result.status_code.should eq 200 + # Issued and already past its window — the override was read. + JSON.parse(result.body)["session"].as_bool.should be_false + ensure + if authority + authority.internals = original || {} of String => JSON::Any + authority.save! + end + user.try &.destroy + end + end + describe "GET /auth/login" do it "redirects to the authority's login_url with {{url}} substituted" do authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil!