From 9ebd17f31f27f94ae703183c3e14920bb64356bf Mon Sep 17 00:00:00 2001 From: boskodev790 Date: Fri, 24 Apr 2026 05:14:46 -0500 Subject: [PATCH] :bug: Fix PENPOT_OIDC_USER_INFO_SOURCE flag being silently ignored (#9114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #9108. The `case` expression in `get-info` (`backend/src/app/auth/oidc.clj`) dispatched on `:token` and `:userinfo` keywords, but the provider map's `:user-info-source` value is a string — both from config (the malli schema in `app.config` pins it to one of `"token"`, `"userinfo"`, `"auto"`) and from the hard-coded Google / GitHub provider maps (which already write `"userinfo"`). Strings never equal keywords in Clojure `case`, so every call fell through to the auto-fallback that prefers ID-token claims and only hits the UserInfo endpoint when claims are empty. The net effect: setting `PENPOT_OIDC_USER_INFO_SOURCE=userinfo` did nothing, and OIDC flows whose IdP requires the UserInfo endpoint (so claims come back empty/partial) failed with "incomplete user info". - Extract a pure helper `select-user-info-source` that maps the raw config string to a dispatch keyword (`:token`, `:userinfo`, `:auto`), falling back to `:auto` for unknown / missing / accidentally-keyword values - Rewrite `get-info`'s `case` to dispatch on the helper's output so the arms unambiguously match the normalised keyword - Add vitest-style deftests in `auth_oidc_test.clj` pinning the three valid strings, the nil / "auto" / unknown fallback, and the reverse regression (a keyword input must not slip through as if it were the matching string) - Add a CHANGES.md entry under the 2.17.0 Unreleased `:bug: Bugs fixed` section linking back to #9108 Signed-off-by: Andrey Antukh Co-authored-by: Andrey Antukh --- CHANGES.md | 1 + backend/src/app/auth/oidc.clj | 21 +++++++++++++++---- backend/test/backend_tests/auth_oidc_test.clj | 20 ++++++++++++++++++ 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 2d0516328e..15477e736a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -50,6 +50,7 @@ ### :bug: Bugs fixed +- Fix `PENPOT_OIDC_USER_INFO_SOURCE` flag being silently ignored (`userinfo` / `token`) in the OIDC callback, causing "incomplete user info" failures during registration [Github #9108](https://github.com/penpot/penpot/issues/9108) - Fix `get-view-only-bundle` crashing when a share-link viewer encounters a team member whose email lacks `@` (NullPointerException in `obfuscate-email`) or whose domain has no `.` (previously produced a dangling-dot `****@****.`); now the viewer-side obfuscation is nil-safe and omits the trailing dot when the domain has no TLD - Remove `corepack` from the MCP local launcher so it runs on Node.js 25+, where corepack is no longer bundled [Github #8877](https://github.com/penpot/penpot/issues/8877) - Fix Copy as SVG: emit a single valid SVG document when multiple shapes are selected, and publish `image/svg+xml` to the clipboard so the paste target works in Inkscape and other SVG-native tools [Github #838](https://github.com/penpot/penpot/issues/838) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 9c292ff2c2..782dfabca7 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -548,16 +548,29 @@ (def ^:private valid-info? (sm/validator schema:info)) +(defn- select-user-info-source + "Normalise the provider's configured user-info source into a keyword the + dispatch below can match. The raw value comes from config as a string + per the malli schema in `app.config` (`\"token\"`, `\"userinfo\"`, or + `\"auto\"`) and from hard-coded per-provider maps as strings as well; + any unrecognised or missing value falls back to `:auto` (prefer claims, + use userinfo as fallback)." + [source] + (case source + "token" :token + "userinfo" :userinfo + :auto)) + (defn- get-info [cfg provider state code] (let [tdata (fetch-access-token cfg provider code) claims (get-id-token-claims provider tdata) - info (case (get provider :user-info-source) - :token (dissoc claims :exp :iss :iat :aud :sid) + info (case (select-user-info-source (get provider :user-info-source)) + :token (dissoc claims :exp :iss :iat :aud :sid) :userinfo (fetch-user-info cfg provider tdata) - (or (some-> claims (dissoc :exp :iss :iat :aud :sid)) - (fetch-user-info cfg provider tdata))) + :auto (or (some-> claims (dissoc :exp :iss :iat :aud :sid)) + (fetch-user-info cfg provider tdata))) info (process-user-info provider tdata info)] diff --git a/backend/test/backend_tests/auth_oidc_test.clj b/backend/test/backend_tests/auth_oidc_test.clj index 2a451195c5..bdf18e5541 100644 --- a/backend/test/backend_tests/auth_oidc_test.clj +++ b/backend/test/backend_tests/auth_oidc_test.clj @@ -33,3 +33,23 @@ (t/is (= "nextcloud@example.com" (:email info))) (t/is (= "Nextcloud User" (:fullname info))) (t/is (true? (:email-verified info))))) + +;; The provider's `:user-info-source` value arrives as a string (enforced by +;; the malli schema in `app.config` and used as-is by the hard-coded Google / +;; GitHub provider maps), so the dispatch must interpret strings — not +;; keywords — to actually honour `PENPOT_OIDC_USER_INFO_SOURCE=userinfo`. +(t/deftest select-user-info-source-interprets-config-strings + (t/testing "explicit string values map to keyword dispatch tokens" + (t/is (= :token (#'oidc/select-user-info-source "token"))) + (t/is (= :userinfo (#'oidc/select-user-info-source "userinfo")))) + + (t/testing "missing or explicit \"auto\" falls back to auto dispatch" + (t/is (= :auto (#'oidc/select-user-info-source "auto"))) + (t/is (= :auto (#'oidc/select-user-info-source nil)))) + + (t/testing "unknown values fall back to auto dispatch safely" + (t/is (= :auto (#'oidc/select-user-info-source "unknown"))) + ;; Guards against the reverse regression — a stray keyword value must + ;; not silently slip through as if it were the matching string. + (t/is (= :auto (#'oidc/select-user-info-source :token))) + (t/is (= :auto (#'oidc/select-user-info-source :userinfo)))))