diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index accafbb07d7..867b9c3c4bb 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -23,6 +23,7 @@ [app.common.logging :as l] [app.config :as cf] [app.db :as db] + [app.http :as-alias http] [app.http.access-token :as-alias actoken] [app.http.session :as session] [app.rpc.commands.auth :as auth] @@ -119,63 +120,121 @@ ;; MIDDLEWARE ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; Perf note: this middleware removes the previous fast-path that +;; short-circuited whenever wrap-session had already set +;; ::session/profile-id. With the fix in place, every authenticated +;; SSO request resolves the header email's profile (a transaction +;; with get-profile-by-email + an idempotent auto-join check). The +;; cost is intentional — correctness over throughput on the steady- +;; state path. If profiling shows this is hot, a follow-up can +;; reintroduce a fast-path by pre-loading the session profile's +;; email (so we can compare without get-or-register-profile) or by +;; caching email→profile-id in a short-lived in-memory map. + (defn- wrap-authz [handler cfg] (fn [request] - ;; Skip when a prior middleware (session or access-token) already - ;; resolved a profile — we only act as a fallback. - (if (or (some? (::session/profile-id request)) - (some? (::actoken/profile-id request))) - (handler request) - (let [email-claim (yreq/get-header request "x-auth-request-email")] - (if (str/blank? email-claim) - (handler request) - (let [local-part (first (str/split email-claim #"@")) - email (resolve-email email-claim) - fullname (or (not-empty (yreq/get-header request "x-auth-request-user")) - local-part) - profile (try + (let [atoken-pid (::actoken/profile-id request) + session-pid (::session/profile-id request) + email-claim (yreq/get-header request "x-auth-request-email")] + (cond + ;; Access-token (API key) — programmatic identity issued out-of-band + ;; by the user. Not a browser SSO session, so the header is not + ;; meaningful here. Pass through unconditionally. + (some? atoken-pid) + (handler request) + + ;; No proxy header — trust whatever wrap-session decided. Without a + ;; header we have no upstream identity to compare against. + (str/blank? email-claim) + (handler request) + + :else + (let [local-part (first (str/split email-claim #"@")) + email (resolve-email email-claim) + fullname (or (not-empty (yreq/get-header request "x-auth-request-user")) + local-part) + profile (try (get-or-register-profile cfg email fullname) (catch Throwable cause (l/err :hint "x-auth-request: error resolving profile" :email email :cause cause) nil))] - (cond - (nil? profile) - (do - (l/wrn :hint "x-auth-request: no profile found for email, passing through unauthenticated" - :email email) - (handler request)) - - (:is-blocked profile) - (do - (l/wrn :hint "x-auth-request: profile is blocked, denying access" - :email email - :profile-id (str (:id profile))) - {::yres/status 403}) - - (not (:is-active profile)) - (do - (l/wrn :hint "x-auth-request: profile is not active, denying access" - :email email - :profile-id (str (:id profile))) - {::yres/status 403}) - - :else - (do - (l/dbg :hint "x-auth-request: authenticating via forwarded header" - :email email - :profile-id (str (:id profile))) - (let [create-session! (session/create-fn cfg profile) - ;; Inject profile-id into the request so this very - ;; request is also treated as authenticated downstream. - response (-> request - (assoc ::session/profile-id (:id profile)) - handler)] - ;; Attach a session cookie so the browser is authenticated - ;; for all subsequent requests without needing to log in. - (create-session! request response)))))))))) + (cond + (nil? profile) + ;; Header email doesn't resolve to a profile (and auto-register + ;; is off). No identity to switch *to* — pass through with + ;; whatever wrap-session set. + (do + (l/wrn :hint "x-auth-request: no profile found for email, passing through unauthenticated" + :email email) + (handler request)) + + (:is-blocked profile) + (do + (l/wrn :hint "x-auth-request: profile is blocked, denying access" + :email email + :profile-id (str (:id profile))) + {::yres/status 403}) + + (not (:is-active profile)) + (do + (l/wrn :hint "x-auth-request: profile is not active, denying access" + :email email + :profile-id (str (:id profile))) + {::yres/status 403}) + + ;; Steady state — existing browser session matches the proxy- + ;; asserted identity. No work to do. + (and session-pid (= session-pid (:id profile))) + (handler request) + + ;; Either no existing session, or the session points at a + ;; *different* profile than oauth2-proxy is asserting. Re-key. + ;; + ;; This is the fix for the session-sharing bug: portal "log out + ;; of all apps" clears the shared _oauth2_proxy cookie + Cognito + ;; session but NOT Penpot's auth-token cookie on its subdomain. + ;; Without this branch, wrap-session resolves the previous + ;; user's profile-id from the stale cookie and this middleware + ;; (under the previous always-skip-when-session rule) never + ;; overrode it. + :else + (do + (when session-pid + (l/inf :hint "x-auth-request: proxy identity differs from existing session — re-keying" + :session-profile-id (str session-pid) + :header-profile-id (str (:id profile)))) + (l/dbg :hint "x-auth-request: authenticating via forwarded header" + :email email + :profile-id (str (:id profile))) + (let [create-session! (session/create-fn cfg profile) + response (-> request + (assoc ::session/profile-id (:id profile)) + ;; Drop stale identity-carrying keys + ;; so downstream code does not see the + ;; previous user's data after re-key. + ;; + ;; ::http/auth-data — errors.clj logs + ;; auth-data.claims.uid as + ;; :request/profile-id; rpc/helpers + ;; exposes the map to RPC handlers via + ;; get-auth-data. + ;; + ;; ::session/session — read indirectly + ;; by session/get-session, which is + ;; called in update-profile-password's + ;; invalidate-others path. Leaving + ;; alice's session here means a + ;; password-change RPC made on the + ;; re-keyed request would invalidate + ;; alice's sessions instead of bob's. + (dissoc ::http/auth-data ::session/session) + handler)] + ;; Fresh auth-token cookie; replaces the stale one the + ;; browser still has (if any). + (create-session! request response))))))))) (def authz {:name ::authz diff --git a/backend/src/app/http/session.clj b/backend/src/app/http/session.clj index bcbb2517a02..3c26c372dff 100644 --- a/backend/src/app/http/session.clj +++ b/backend/src/app/http/session.clj @@ -248,12 +248,32 @@ (binding [ct/*clock* (clock/get-clock (:profile-id session))] (handler request))] - (if (and session (renew-session? session)) - (let [session (->> session - (update-session manager) - (assign-token cfg))] - (assign-session-cookie response session)) - response)) + ;; Renewal runs after the inner handler. Two cases where it + ;; MUST step aside: + ;; + ;; 1. The response already carries the auth-token cookie — + ;; e.g. wrap-authz re-keyed the session to a new user. + ;; Renewing alice's cookie on top of bob's freshly-issued + ;; one would silently undo the re-key. + ;; + ;; 2. The response is an error (status >= 400) — e.g. proxy + ;; identity mismatch with a blocked/inactive incoming + ;; profile produced a 403. Renewing alice's cookie on the + ;; denial response would EXTEND her session lifetime even + ;; though the upstream identity has changed. Better to let + ;; her cookie age out naturally (or get cleared on the next + ;; successful flow) than to refresh it on a mismatch. + (let [status (::yres/status response)] + (if (and session + (renew-session? session) + (or (nil? status) (< status 400)) + (not (contains? (::yres/cookies response) + (cf/get :auth-token-cookie-name)))) + (let [session (->> session + (update-session manager) + (assign-token cfg))] + (assign-session-cookie response session)) + response))) (= type :bearer) (let [session (case (:ver metadata) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index f7d4f70d1c7..133fbe37ad5 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -140,6 +140,33 @@ (t/is (= (:id session) (:sid claims))) (t/is (= (:id profile) (:uid claims))))) +(t/deftest session-authz-does-not-renew-on-error-response + ;; The renewal guard MUST also step aside on error responses. + ;; Without this, a 403 produced by wrap-authz (proxy identity + ;; mismatch with a blocked/inactive incoming profile) would still + ;; cause alice's session cookie to be renewed on the way out — + ;; extending her session lifetime on the very denial that signaled + ;; her identity is no longer valid upstream. + (let [cfg th/*system* + manager (session/inmemory-manager) + profile (th/create-profile* 91) + t0 (ct/inst "2025-01-01T00:00:00Z") + t1 (ct/plus t0 (ct/duration {:seconds 2})) + threshold (ct/duration {:seconds 1}) + handler (-> (fn [_req] {::yres/status 403}) + (#'session/wrap-authz {::session/manager manager}) + (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) + :cookie (partial session/decode-token cfg)})) + token (binding [ct/*clock* (ct/fixed-clock t0)] + (->> (session/create-session manager {:profile-id (:id profile) + :user-agent "user agent"}) + (#'session/assign-token cfg))) + response (binding [cf/config (assoc cf/config :auth-token-cookie-renewal-max-age threshold) + ct/*clock* (ct/fixed-clock t1)] + (handler (->DummyRequest {} {"auth-token" (:token token)})))] + (t/is (= 403 (::yres/status response))) + (t/is (not (contains? (::yres/cookies response) "auth-token"))))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; X-Auth-Request middleware tests ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; @@ -156,18 +183,151 @@ (handler (->DummyRequest {} {})) (t/is (nil? (::session/profile-id @captured))))) -(t/deftest x-auth-request-skips-when-session-present +(t/deftest x-auth-request-preserves-session-when-header-email-unresolvable + ;; When wrap-session has resolved alice's profile-id from the local + ;; auth-token cookie AND the proxy header email cannot be resolved to + ;; a profile (unknown user + auto-register off), the existing session + ;; passes through unchanged. The middleware only re-keys when it has + ;; a *real* alternative identity to switch to. (let [profile-id (random-uuid) - called? (volatile! false) handler (#'app.http.auth-request/wrap-authz - (fn [req] (vreset! called? true) req) + (fn [req] req) (make-xauth-cfg)) request (-> (->DummyRequest {"x-auth-request-email" "user@example.com"} {}) (assoc ::session/profile-id profile-id)) result (handler request)] - ;; profile-id must pass through unchanged — middleware must not overwrite it (t/is (= profile-id (::session/profile-id result))))) +(t/deftest x-auth-request-rekeys-when-session-identity-differs + ;; Repro of the QA-reported session-sharing bug: alice's auth-token + ;; cookie persists on Penpot's subdomain after the portal "log out of + ;; all apps"; bob then logs in upstream. wrap-session resolves alice's + ;; profile-id from the old cookie, but oauth2-proxy is forwarding + ;; bob's email. The middleware MUST re-key to bob. + (let [alice (th/create-profile* 1 {:is-active true}) + bob (th/create-profile* 2 {:is-active true}) + captured (volatile! nil) + cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) {::yres/status 200}) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" (:email bob)} {}) + (assoc ::session/profile-id (:id alice))) + response (handler request)] + ;; Downstream handler sees bob's profile-id, not alice's. + (t/is (= (:id bob) (::session/profile-id @captured))) + ;; A fresh auth-token cookie is issued for bob's session. + (t/is (contains? (::yres/cookies response) "auth-token")))) + +(t/deftest x-auth-request-no-rekey-when-session-matches-header + ;; Steady-state guard: the browser session matches the proxy identity. + ;; No re-key, no new cookie. Without this, every authenticated request + ;; would mint a fresh cookie. + (let [profile (th/create-profile* 1 {:is-active true}) + captured (volatile! nil) + cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) {::yres/status 200}) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" (:email profile)} {}) + (assoc ::session/profile-id (:id profile))) + response (handler request)] + (t/is (= (:id profile) (::session/profile-id @captured))) + (t/is (not (contains? (::yres/cookies response) "auth-token"))))) + +(t/deftest x-auth-request-rekey-clears-stale-auth-data + ;; The re-keyed request must have ::http/auth-data removed before the + ;; inner handler runs. errors.clj logs auth-data.claims.uid as + ;; :request/profile-id and rpc/helpers exposes the map to RPC + ;; handlers via get-auth-data — both would otherwise see alice's UID + ;; after we re-keyed to bob, leaking the stale identity across the + ;; boundary the middleware thinks it has crossed. + (let [alice (th/create-profile* 1 {:is-active true}) + bob (th/create-profile* 2 {:is-active true}) + captured (volatile! nil) + cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) {::yres/status 200}) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" (:email bob)} {}) + (assoc ::session/profile-id (:id alice)) + (assoc ::http/auth-data {:type :cookie + :claims {:uid (:id alice) + :sid (random-uuid)}}))] + (handler request) + (t/is (= (:id bob) (::session/profile-id @captured))) + (t/is (nil? (::http/auth-data @captured))))) + +(t/deftest x-auth-request-rekey-clears-stale-session-map + ;; ::session/session is read indirectly by session/get-session, which + ;; the update-profile-password RPC calls via invalidate-others. If + ;; alice's stale session map survives the re-key to bob, a password- + ;; change made on this request would invalidate alice's other + ;; sessions instead of bob's. Surfaced by Copilot review on PR #22. + (let [alice (th/create-profile* 1 {:is-active true}) + bob (th/create-profile* 2 {:is-active true}) + alice-session {:id (random-uuid) + :profile-id (:id alice) + :user-agent "alice's ua"} + captured (volatile! nil) + cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) {::yres/status 200}) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" (:email bob)} {}) + (assoc ::session/profile-id (:id alice)) + (assoc ::session/session alice-session))] + (handler request) + (t/is (= (:id bob) (::session/profile-id @captured))) + (t/is (nil? (::session/session @captured))))) + +(t/deftest session-authz-renewal-does-not-overwrite-rekeyed-cookie + ;; Integration guard: session/wrap-authz runs AFTER wrap-authz on the + ;; way out and would normally renew the incoming session cookie. If + ;; wrap-authz has already issued a fresh auth-token cookie for the + ;; re-keyed identity (bob), the renewal MUST step aside — otherwise + ;; alice's renewed cookie clobbers bob's fresh one and the re-key is + ;; silently undone. + ;; + ;; This locks in the (not (contains? response cookies "auth-token")) + ;; guard added to session.clj. A future refactor that removes it + ;; fails this test. + (let [alice (th/create-profile* 91 {:is-active true}) + bob (th/create-profile* 92 {:is-active true}) + cfg (make-xauth-cfg) + t0 (ct/inst "2025-01-01T00:00:00Z") + t1 (ct/plus t0 (ct/duration {:seconds 2})) + ;; Lower than (t1 - t0) so the seeded cookie is always due for renewal. + renewal-threshold (ct/duration {:seconds 1}) + middleware (-> (fn [req] {::yres/status 200 + :seen-profile-id (::session/profile-id req)}) + (#'app.http.auth-request/wrap-authz cfg) + (#'session/wrap-authz cfg) + (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) + :cookie (partial session/decode-token cfg)})) + seeded-token (binding [ct/*clock* (ct/fixed-clock t0)] + (get-in ((session/create-fn cfg alice) + (->DummyRequest {} {}) + {::yres/status 200}) + [::yres/cookies "auth-token" :value])) + response (binding [cf/config (assoc cf/config + :auth-token-cookie-renewal-max-age + renewal-threshold) + ct/*clock* (ct/fixed-clock t1)] + (middleware (->DummyRequest {"x-auth-request-email" (:email bob)} + {"auth-token" seeded-token}))) + rekeyed-token (get-in response [::yres/cookies "auth-token" :value]) + followup (middleware (->DummyRequest {} {"auth-token" rekeyed-token}))] + ;; Cookie was re-keyed (not just renewed): the new token differs. + (t/is (some? rekeyed-token)) + (t/is (not= seeded-token rekeyed-token)) + ;; This request's inner handler ran as bob, not alice. + (t/is (= (:id bob) (:seen-profile-id response))) + ;; The re-keyed token, decoded on a follow-up request, still resolves to bob. + ;; If renewal had clobbered the response cookie with a renewed alice token, + ;; this would resolve to alice instead. + (t/is (= (:id bob) (:seen-profile-id followup))))) + (t/deftest x-auth-request-skips-when-access-token-present (let [profile-id (random-uuid) handler (#'app.http.auth-request/wrap-authz