Merge pull request #4880 from penpot/niwinz-oidc-fixes

✨ Several improvements to OIDC and other related code

backend/src/app/auth/oidc.clj

@@ -19,8 +19,10 @@
    [app.email.blacklist :as email.blacklist]
    [app.email.whitelist :as email.whitelist]
    [app.http.client :as http]
+   [app.http.errors :as errors]
    [app.http.session :as session]
    [app.loggers.audit :as audit]
+   [app.rpc :as rpc]
    [app.rpc.commands.profile :as profile]
    [app.setup :as-alias setup]
    [app.tokens :as tokens]
@@ -130,8 +132,8 @@
           (-> body json/decode :keys process-oidc-jwks)
           (do
             (l/warn :hint "unable to retrieve JWKs (unexpected response status code)"
-                    :http-status status
+                    :response-status status
-                    :http-body  body)
+                    :response-body  body)
             nil)))
       (catch Throwable cause
         (l/warn :hint "unable to retrieve JWKs (unexpected exception)"
@@ -145,7 +147,7 @@
   (when (contains? cf/flags :login-with-oidc)
     (if-let [opts (prepare-oidc-opts cfg)]
       (let [jwks (fetch-oidc-jwks cfg opts)]
-        (l/info :hint "provider initialized"
+        (l/inf :hint "provider initialized"
                :provider "oidc"
                :method (if (:discover? opts) "discover" "manual")
                :client-id (:client-id opts)
@@ -180,7 +182,7 @@
       (if (and (string? (:client-id opts))
                (string? (:client-secret opts)))
         (do
-          (l/info :hint "provider initialized"
+          (l/inf :hint "provider initialized"
                  :provider "google"
                  :client-id (:client-id opts)
                  :client-secret (obfuscate-string (:client-secret opts)))
@@ -208,8 +210,9 @@
           (ex/raise :type :internal
                     :code :unable-to-retrieve-github-emails
                     :hint "unable to retrieve github emails"
-                    :http-status status
+                    :request-uri (:uri params)
-                    :http-body body))
+                    :response-status status
+                    :response-body body))
 
         (->> body json/decode (filter :primary) first :email))))
 
@@ -234,7 +237,7 @@
       (if (and (string? (:client-id opts))
                (string? (:client-secret opts)))
         (do
-          (l/info :hint "provider initialized"
+          (l/inf :hint "provider initialized"
                  :provider "github"
                  :client-id (:client-id opts)
                  :client-secret (obfuscate-string (:client-secret opts)))
@@ -249,7 +252,7 @@
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (defmethod ig/init-key ::providers/gitlab
-  [_ _]
+  [_ cfg]
   (let [base (cf/get :gitlab-base-uri "https://gitlab.com")
         opts {:base-uri      base
               :client-id     (cf/get :gitlab-client-id)
@@ -258,17 +261,18 @@
               :auth-uri      (str base "/oauth/authorize")
               :token-uri     (str base "/oauth/token")
               :user-uri      (str base "/oauth/userinfo")
+              :jwks-uri      (str base "/oauth/discovery/keys")
               :name          "gitlab"}]
     (when (contains? cf/flags :login-with-gitlab)
       (if (and (string? (:client-id opts))
                (string? (:client-secret opts)))
-        (do
+        (let [jwks (fetch-oidc-jwks cfg opts)]
-          (l/info :hint "provider initialized"
+          (l/inf :hint "provider initialized"
                  :provider "gitlab"
                  :base-uri base
                  :client-id (:client-id opts)
                  :client-secret (obfuscate-string (:client-secret opts)))
-          opts)
+          (assoc opts :jwks jwks))
 
         (do
           (l/warn :hint "unable to initialize auth provider, missing configuration" :provider "gitlab")
@@ -324,7 +328,7 @@
                 :uri (:token-uri provider)
                 :body (u/map->query-string params)}]
 
-    (l/trace :hint "request access token"
+    (l/trc :hint "fetch access token"
            :provider (:name provider)
            :client-id (:client-id provider)
            :client-secret (obfuscate-string (:client-secret provider))
@@ -332,18 +336,23 @@
            :redirect-uri (:redirect_uri params))
 
     (let [{:keys [status body]} (http/req! cfg req {:sync? true})]
-      (l/trace :hint "access token response" :status status :body body)
+      (l/trc :hint "access token fetched" :status status :body body)
       (if (= status 200)
-        (let [data (json/decode body)]
+        (let [data (json/decode body)
-          {:token/access (get data :access_token)
+              data {:token/access (get data :access_token)
                     :token/id     (get data :id_token)
-           :token/type   (get data :token_type)})
+                    :token/type   (get data :token_type)}]
-
+          (l/trc :hint "access token fetched"
+                 :token-id (:token/id data)
+                 :token-type (:token/type data)
+                 :token (:token/access data))
+          data)
         (ex/raise :type :internal
-                  :code :unable-to-retrieve-token
+                  :code :unable-to-fetch-access-token
-                  :hint "unable to retrieve token"
+                  :hint "unable to fetch access token"
-                  :http-status status
+                  :request-uri (:uri req)
-                  :http-body body)))))
+                  :response-status status
+                  :response-body body)))))
 
 (defn- process-user-info
   [provider tdata info]
@@ -370,7 +379,7 @@
 
 (defn- fetch-user-info
   [{:keys [::provider] :as cfg} tdata]
-  (l/trace :hint "fetch user info"
+  (l/trc :hint "fetch user info"
          :uri (:user-uri provider)
          :token (obfuscate-string (:token/access tdata)))
 
@@ -380,7 +389,7 @@
                   :method :get}
         response (http/req! cfg params {:sync? true})]
 
-    (l/trace :hint "user info response"
+    (l/trc :hint "user info response"
            :status (:status response)
            :body   (:body response))
 
@@ -432,7 +441,7 @@
 
         info   (process-user-info provider tdata info)]
 
-    (l/trace :hint "user info" :info info)
+    (l/trc :hint "user info" :info info)
 
     (when-not (s/valid? ::info info)
       (l/warn :hint "received incomplete profile info object (please set correct scopes)" :info info)
@@ -586,22 +595,33 @@
         (redirect-to-register cfg info request)
         (redirect-with-error "registration-disabled")))))
 
+(defn- get-external-session-id
+  [request]
+  (let [session-id (rreq/get-header request "x-external-session-id")]
+    (when (string? session-id)
+      (if (or (> (count session-id) 256)
+              (= session-id "null")
+              (str/blank? session-id))
+        nil
+        session-id))))
+
 (defn- auth-handler
   [cfg {:keys [params] :as request}]
   (let [props  (audit/extract-utm-params params)
-        esid  (rreq/get-header request "x-external-session-id")
+        esid   (rpc/get-external-session-id request)
-        state (tokens/generate (::setup/props cfg)
+        params {:iss :oauth
-                               {:iss :oauth
                 :invitation-token (:invitation-token params)
                 :external-session-id esid
                 :props props
-                                :exp (dt/in-future "4h")})
+                :exp (dt/in-future "4h")}
+        state  (tokens/generate (::setup/props cfg)
+                                (d/without-nils params))
         uri    (build-auth-uri cfg state)]
     {::rres/status 200
      ::rres/body {:redirect-uri uri}}))
 
 (defn- callback-handler
-  [cfg request]
+  [{:keys [::provider] :as cfg} request]
   (try
     (if-let [error (dm/get-in request [:params :error])]
       (redirect-with-error "unable-to-auth" error)
@@ -609,7 +629,16 @@
             profile (get-profile cfg info)]
         (process-callback cfg request info profile)))
     (catch Throwable cause
-      (l/err :hint "error on oauth process" :cause cause)
+      (binding [l/*context* (-> (errors/request->context request)
+                                (assoc :auth/provider (:name provider)))]
+        (let [edata (ex-data cause)]
+          (cond
+            (= :validation (:type edata))
+            (l/wrn :hint "invalid token received" :cause cause)
+
+            :else
+            (l/err :hint "error on oauth process" :cause cause))))
+
       (redirect-with-error "unable-to-auth" (ex-message cause)))))
 
 (def provider-lookup

backend/src/app/loggers/audit.clj

@@ -35,9 +35,13 @@
 
 (defn parse-client-ip
   [request]
-  (or (some-> (rreq/get-header request "x-forwarded-for") (str/split ",") first)
+  (let [ip-addr (or (some-> (rreq/get-header request "x-forwarded-for") (str/split ",") first)
                     (rreq/get-header request "x-real-ip")
-      (some-> (rreq/remote-addr request) str)))
+                    (some-> (rreq/remote-addr request) str))
+        ip-addr (-> ip-addr
+                    (str/split ":" 2)
+                    (first))]
+    ip-addr))
 
 (defn extract-utm-params
   "Extracts additional data from params and namespace them under

backend/src/app/main.clj

@@ -254,7 +254,7 @@
    {::http.client/client (ig/ref ::http.client/client)}
 
    ::oidc.providers/gitlab
-   {}
+   {::http.client/client (ig/ref ::http.client/client)}
 
    ::oidc.providers/generic
    {::http.client/client (ig/ref ::http.client/client)}

backend/src/app/rpc.clj

@@ -70,6 +70,20 @@
         (handle-response-transformation request mdata)
         (handle-before-comple-hook mdata))))
 
+(defn get-external-session-id
+  [request]
+  (when-let [session-id (rreq/get-header request "x-external-session-id")]
+    (when-not (or (> (count session-id) 256)
+                  (= session-id "null")
+                  (str/blank? session-id))
+      session-id)))
+
+(defn- get-external-event-origin
+  [request]
+  (when-let [origin (rreq/get-header request "x-event-origin")]
+    (when-not (> (count origin) 256)
+      origin)))
+
 (defn- rpc-handler
   "Ring handler that dispatches cmd requests and convert between
   internal async flow into ring async flow."
@@ -79,8 +93,8 @@
         profile-id   (or (::session/profile-id request)
                          (::actoken/profile-id request))
 
-        session-id   (rreq/get-header request "x-external-session-id")
+        session-id   (get-external-session-id request)
-        event-origin (rreq/get-header request "x-event-origin")
+        event-origin (get-external-event-origin request)
 
         data         (-> params
                          (assoc ::handler-name handler-name)

docker/devenv/files/nginx.conf

@@ -68,7 +68,10 @@ http {
         proxy_set_header X-Forwarded-Proto $scheme;
         proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
 
-        resolver 127.0.0.11;
+        proxy_buffer_size 16k;
+        proxy_busy_buffers_size 24k; # essentially, proxy_buffer_size + 2 small buffers of 4k
+        proxy_buffers 32 4k;
+        resolver 127.0.0.11 ipv6=off;
 
         etag off;