Merge pull request #611 from Swirrl/ignore-malformed-auth-headers

ignore malformed auth headers and let the request through unauthentic…

drafter/resources/drafter-base-config.edn

@@ -60,28 +60,34 @@
  [:drafter.timeouts/timeout-query :drafter/draftset-timeout] {:endpoint-timeout #timeout #env DRAFTER_TIMEOUT_QUERY_ENDPOINT_DRAFTSET
                                                               :jws-signing-key #env DRAFTER_JWS_SIGNING_KEY }
 
- :drafter.routes/jobs-status {}
+ :drafter.routes/jobs-status
+ {:wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.routes.sparql/live-sparql-query-route {:repo #ig/ref :drafter.backend.live/endpoint
                                                  :timeout-fn #ig/ref [:drafter.timeouts/timeout-query :drafter/live-timeout]}
 
  :drafter.feature.draftset.list/get-draftsets-handler
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.draftset.show/handler
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.draftset.create/handler
- {:drafter/manager #ig/ref :drafter/manager}
+ {:drafter/manager #ig/ref :drafter/manager
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.middleware/wrap-as-draftset-owner
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.draftset.delete/handler {:drafter/backend #ig/ref :drafter/backend
                                            :wrap-as-draftset-owner #ig/ref :drafter.feature.middleware/wrap-as-draftset-owner}
 
  :drafter.feature.draftset.options/handler
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.draftset-data.show/handler {:drafter/backend #ig/ref :drafter/backend
                                               :wrap-as-draftset-owner #ig/ref :drafter.feature.middleware/wrap-as-draftset-owner
@@ -130,15 +136,18 @@
                                            :wrap-as-draftset-owner #ig/ref :drafter.feature.middleware/wrap-as-draftset-owner}
 
  :drafter.feature.draftset.claim/handler
- {:drafter/manager #ig/ref :drafter/manager}
+ {:drafter/manager #ig/ref :drafter/manager
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  #_:drafter.routes.draftsets-api/get-users-handler
  :drafter.feature.users.list/get-users-handler
- {:drafter.user/repo #ig/ref :drafter.user/repo}
+ {:drafter.user/repo #ig/ref :drafter.user/repo
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.endpoint.show/handler {:repo #ig/ref :drafter/backend}
  :drafter.feature.endpoint.list/handler
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  [:drafter/routes :draftset/api] {:context "/v1"
                                   :routes [[:get "/users" #ig/ref :drafter.feature.users.list/get-users-handler]

drafter/src/drafter/async/jobs.clj

@@ -73,10 +73,10 @@
                    (= :error (:type @value-p)))
         (assoc :error @value-p))))
 
-(defmethod ig/init-key :drafter.routes/jobs-status [_ _]
+(defmethod ig/init-key :drafter.routes/jobs-status [_ opts]
   (context
    "/v1/status" []
-   (middleware/wrap-authorize :editor
+   (middleware/wrap-authorize (:wrap-authenticate opts) :editor
     (routes
      (GET "/jobs/:id" [id]
           (or (when-let [job (some-> id r/try-parse-uuid get-job)]

drafter/src/drafter/feature/draftset/claim.clj

@@ -35,11 +35,12 @@
       (forbidden-response "User not in role for draftset claim"))
     (ring/not-found "Draftset not found")))
 
-(defn handler [{{:keys [backend] :as manager} :drafter/manager}]
+(defn handler [{{:keys [backend] :as manager} :drafter/manager
+                wrap-authenticate :wrap-authenticate}]
   (let [inner-handler (partial handler* manager)]
     (->> inner-handler
         (feat-middleware/existing-draftset-handler backend)
-        (middleware/wrap-authorize :editor))))
+        (middleware/wrap-authorize wrap-authenticate :editor))))
 
 (defmethod ig/pre-init-spec :drafter.feature.draftset.claim/handler [_]
   (s/keys :req [:drafter/manager]))

drafter/src/drafter/feature/draftset/create.clj

@@ -12,9 +12,10 @@
    [ring.util.response :as ring]))
 
 (defn create-draftsets-handler
-  [{{:keys [backend global-writes-lock clock] :as manager} :drafter/manager}]
+  [{{:keys [backend global-writes-lock clock] :as manager} :drafter/manager
+    wrap-authenticate :wrap-authenticate}]
   (let [version "/v1"]
-    (middleware/wrap-authorize :editor
+    (middleware/wrap-authorize wrap-authenticate :editor
      (fn [{{:keys [display-name description]} :params user :identity :as request}]
        (feat-common/run-sync
         {:backend backend :global-writes-lock global-writes-lock}

drafter/src/drafter/feature/draftset/list.clj

@@ -74,8 +74,8 @@
 
 (defn get-draftsets-handler
   ":get /draftsets"
-  [{backend :drafter/backend}]
-  (middleware/wrap-authorize :editor
+  [{:keys [drafter/backend wrap-authenticate]}]
+  (middleware/wrap-authorize wrap-authenticate :editor
     (middleware/include-endpoints-param
       (parse-union-with-live-handler
         (fn [{user :identity {:keys [include union-with-live]} :params :as request}]

drafter/src/drafter/feature/draftset/options.clj

@@ -7,8 +7,8 @@
             [clojure.spec.alpha :as s]))
 
 (defn handler
-  [{backend :drafter/backend}]
-  (middleware/wrap-authorize :editor
+  [{:keys [drafter/backend wrap-authenticate]}]
+  (middleware/wrap-authorize wrap-authenticate :editor
    (feat-middleware/existing-draftset-handler
     backend
     (fn [{{:keys [draftset-id]} :params user :identity}]

drafter/src/drafter/feature/draftset/show.clj

@@ -22,8 +22,8 @@
     ::not-found))
 
 (defn handler
-  [{backend :drafter/backend}]
-  (middleware/wrap-authorize :editor
+  [{:keys [drafter/backend wrap-authenticate]}]
+  (middleware/wrap-authorize wrap-authenticate :editor
    (feat-middleware/existing-draftset-handler
     backend
     (parse-union-with-live-handler

drafter/src/drafter/feature/endpoint/list.clj

@@ -3,7 +3,7 @@
             [drafter.endpoint :as ep]
             [drafter.feature.endpoint.public :as pub]
             [drafter.feature.draftset.list :as dsl]
-            [drafter.middleware :refer [include-endpoints-param]]
+            [drafter.middleware :as middleware]
             [drafter.routes.draftsets-api :refer [parse-union-with-live-handler]]
             [clojure.spec.alpha :as s]
             [ring.util.response :as ring]))
@@ -29,11 +29,13 @@
 
 (defn list-handler
   ":get /endpoints"
-  [backend]
-  (include-endpoints-param
-   (parse-union-with-live-handler
-    (fn [{user :identity {:keys [include union-with-live]} :params :as request}]
-      (ring/response (get-endpoints backend user include union-with-live))))))
+  [backend wrap-authenticate]
+  (middleware/wrap-optionally-authenticate wrap-authenticate
+    (middleware/include-endpoints-param
+      (parse-union-with-live-handler
+       (fn [{user :identity {:keys [include union-with-live]} :params :as request}]
+         (ring/response (get-endpoints backend user include union-with-live)))))))
 
-(defmethod ig/init-key ::handler [_ {:keys [drafter/backend] :as opts}]
-  (list-handler backend))
+(defmethod ig/init-key ::handler
+  [_ {:keys [drafter/backend wrap-authenticate] :as opts}]
+  (list-handler backend wrap-authenticate))

drafter/src/drafter/feature/middleware.clj

@@ -53,9 +53,9 @@
       (response/forbidden-response "Operation only permitted by draftset owner"))))
 
 (defn wrap-as-draftset-owner
-  [{backend :drafter/backend}]
+  [{:keys [:drafter/backend wrap-authenticate]}]
   (fn [required-role handler]
-    (middleware/wrap-authorize required-role
+    (middleware/wrap-authorize wrap-authenticate required-role
      (existing-draftset-handler
       backend
       (restrict-to-draftset-owner backend handler)))))

drafter/src/drafter/feature/users/list.clj

@@ -8,8 +8,8 @@
 (defn get-users-handler
   "Ring handler that returns a list of user objects representing users
   within the system."
-  [{user-repo ::user/repo}]
-  (middleware/wrap-authorize :editor
+  [{user-repo ::user/repo wrap-authenticate :wrap-authenticate}]
+  (middleware/wrap-authorize wrap-authenticate :editor
    (fn [r]
      (let [users (user/get-all-users user-repo)
            summaries (map user/get-summary users)]

drafter/src/drafter/handler.clj

@@ -61,14 +61,19 @@
                                     ;; env when scheme needs to be passed through from load balancer
                                     (assoc :proxy true))
                  ;; add custom middleware here
-                 :middleware (cond->> [wrap-authenticate
-                                       #(wrap-resource % "swagger-ui")
-                                       wrap-verbs
-                                       wrap-encode-errors
-                                       middleware/wrap-total-requests-counter
-                                       middleware/wrap-request-timer
-                                       #(log-request % {:query "<scrubbed>"})]
-                               global-auth? (cons #(middleware/wrap-authorize :access %)))
+                 :middleware
+                 (let [middleware [#(wrap-resource % "swagger-ui")
+                                   wrap-verbs
+                                   wrap-encode-errors
+                                   middleware/wrap-total-requests-counter
+                                   middleware/wrap-request-timer
+                                   #(log-request % {:query "<scrubbed>"})]]
+                   (if global-auth?
+                     (cons #(middleware/wrap-authorize
+                             wrap-authenticate :access %)
+                           middleware)
+                     middleware))
+
                  ;; add access rules here
                  :access-rules []
                  ;; serialize/deserialize the following data formats

drafter/src/drafter/middleware.clj

@@ -18,18 +18,6 @@
            (org.apache.tika.mime MediaType)
            (java.util.zip GZIPInputStream)))
 
-;; Attempts to authenticate a request if it has an authorization header.
-;; NOTE allows requests without an authorization header through unchanged, so
-;; wrap-authorize is required to actually restrict access to a route.
-(defmethod ig/init-key :drafter.middleware/wrap-authenticate
-  [_ {:keys [middleware] :as opts}]
-  (fn [handler]
-    (let [wrapped ((apply comp middleware) handler)]
-      (fn [request]
-        (if-let [_ (http/-get-header request "authorization")]
-          (wrapped request)
-          (handler request))))))
-
 (defn- whitelisted? [{:keys [request-method uri]}]
   (case request-method
     ;; "a CORS-preflight request never includes credentials"
@@ -39,17 +27,39 @@
     :get (#{"/" "/swagger/swagger.json"} uri)
     false))
 
-(defn wrap-authorize [required-role handler]
-  (fn [request]
-    (if (whitelisted? request)
-      (handler request)
-      (if-let [user (:identity request)]
-        (if (user/has-role? (:identity request) required-role)
+(defmethod ig/init-key :drafter.middleware/wrap-authenticate
+  [_ {:keys [middleware] :as opts}]
+  (fn [handler]
+    (let [wrapped ((apply comp middleware) handler)]
+      (fn [request]
+        (if (or (:identity request) ; already authenticated
+                (whitelisted? request))
           (handler request)
-          (response/forbidden-response (str "You require the "
-                                            (name required-role)
-                                            " role to perform this action")))
-        (response/unauthorized-response "Not authenticated")))))
+          (wrapped request))))))
+
+(defn wrap-optionally-authenticate
+  "Attempt to authenticate the request only if it has an authorization header.
+   For routes which don't require authentication, but can personalise results
+   if a user is logged in."
+  [wrap-authenticate handler]
+  (let [wrapped (wrap-authenticate handler)]
+    (fn [request]
+      (if (http/-get-header request "authorization")
+        (wrapped request)
+        (handler request)))))
+
+(defn wrap-authorize [wrap-authenticate required-role handler]
+  (wrap-authenticate
+   (fn [request]
+     (if (whitelisted? request)
+       (handler request)
+       (let [user (:identity request)]
+         (if (user/has-role? (:identity request) required-role)
+           (handler request)
+           (response/forbidden-response
+            (str "You require the "
+                 (name required-role)
+                 " role to perform this action"))))))))
 
 (defn require-params [required-keys inner-handler]
   (fn [{:keys [params] :as request}]

drafter/test/drafter/handler_test.clj

@@ -1,6 +1,6 @@
 (ns ^:rest-api drafter.handler-test
   (:require
-   [clojure.test :refer [deftest are]]
+   [clojure.test :refer [deftest is are]]
    [drafter.feature.endpoint.public :as public]
    [drafter.routes.sparql-test :refer [live-query]]
    [drafter.test-common :as tc]
@@ -74,3 +74,15 @@
         list-draftsets test-access    403 ;; Forbidden, requires editor
         list-draftsets test-editor    200
         list-draftsets test-publisher 200))))
+
+;; When muttnik is behind basic auth and drafter is running on the same host,
+;; the browser will forward the header to drafter. We should ignore it.
+(deftest malformed-auth-headers
+  (tc/with-system [{handler :drafter.handler/app} "test-system.edn"]
+    (-> {:scheme :http
+         :request-method :get
+         :uri "/v1/sparql/live"
+         :headers {"accept" "text/plain" "authorization" "Basic foo"}
+         :query-string
+         "query=select%20%2A%20where%20%7B%3Fs%20%3Fp%20%3Fo%7D"}
+        handler :status (= 200) is)))

drafter/test/resources/web.edn

@@ -35,29 +35,35 @@
  [:drafter.timeouts/timeout-query :drafter/draftset-timeout] {:endpoint-timeout #timeout #env DRAFTER_TIMEOUT_QUERY_ENDPOINT_DRAFTSET
                                                               :jws-signing-key #env DRAFTER_JWS_SIGNING_KEY }
 
- :drafter.routes/jobs-status {}
+ :drafter.routes/jobs-status
+ {:wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.routes.sparql/live-sparql-query-route {:repo #ig/ref :drafter.backend.live/endpoint
                                                  :timeout-fn #ig/ref [:drafter.timeouts/timeout-query :drafter/live-timeout]}
 
  :drafter.feature.draftset.show/handler
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.draftset.list/get-draftsets-handler
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
 
  :drafter.feature.draftset.create/handler
- {:drafter/manager #ig/ref :drafter/manager}
+ {:drafter/manager #ig/ref :drafter/manager
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.middleware/wrap-as-draftset-owner
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.draftset.delete/handler {:drafter/backend #ig/ref :drafter/backend
                                            :wrap-as-draftset-owner #ig/ref :drafter.feature.middleware/wrap-as-draftset-owner}
 
  :drafter.feature.draftset.options/handler
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.draftset-data.show/handler {:drafter/backend #ig/ref :drafter/backend
                                               :wrap-as-draftset-owner #ig/ref :drafter.feature.middleware/wrap-as-draftset-owner
@@ -106,14 +112,17 @@
                                            :wrap-as-draftset-owner #ig/ref :drafter.feature.middleware/wrap-as-draftset-owner}
 
  :drafter.feature.draftset.claim/handler
- {:drafter/manager #ig/ref :drafter/manager}
+ {:drafter/manager #ig/ref :drafter/manager
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.users.list/get-users-handler
- {:drafter.user/repo #ig/ref :drafter.user/repo}
+ {:drafter.user/repo #ig/ref :drafter.user/repo
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  :drafter.feature.endpoint.show/handler {:repo #ig/ref :drafter/backend}
  :drafter.feature.endpoint.list/handler
- {:drafter/backend #ig/ref :drafter/backend}
+ {:drafter/backend #ig/ref :drafter/backend
+  :wrap-authenticate #ig/ref :drafter.middleware/wrap-authenticate}
 
  [:drafter/routes :draftset/api] {:context "/v1"
                                   :routes [[:get "/users" #ig/ref :drafter.feature.users.list/get-users-handler]