Merge pull request #560 from Opetushallitus/EH-1518

EH-1518: Käsittele 404-virhetilanne Service Ticketiä haettaessa

src/oph/ehoks/common/api.clj

@@ -14,6 +14,11 @@
   [_ __ ___]
   (response/not-found {:reason "Route not found"}))
 
+(defn unauthorized-handler
+  "Käsittelee tapauksen, jossa ei (ole/saada tarkistettua) käyttöoikeuksia."
+  [_ __ ___]
+  (response/unauthorized {:reason "Unable to check access rights"}))
+
 (defn log-exception
   "Logittaa virheen."
   [ex data]
@@ -41,6 +46,7 @@
    ::c-ex/response-validation (c-ex/with-logging
                                 c-ex/http-response-handler :error)
    :not-found not-found-handler
+   :unauthorized unauthorized-handler
    ::c-ex/default exception-handler})
 
 (defn create-app

src/oph/ehoks/external/cas.clj

@@ -64,7 +64,24 @@
               (nil? (:url @grant-ticket))
               (t/after? (t/now) (:expires @grant-ticket)))
       (refresh-grant-ticket!))
-    (let [ticket (get-service-ticket (:url @grant-ticket) service)]
+    (let [ticket (try
+                   (get-service-ticket (:url @grant-ticket) service)
+                   (catch Exception e
+                     (if (= 404 (:status (ex-data e)))
+                       ; Refresh Ticket Granting Ticket and retry,
+                       ; when fetch Service Ticket call returns 404.
+                       ; This potentially happens after CAS service restart.
+                       (do
+                         (refresh-grant-ticket!)
+                         (try
+                           (get-service-ticket (:url @grant-ticket) service)
+                           (catch Exception ex
+                             (throw (ex-info "Error getting Service Ticket"
+                                             {:type :unauthorized}
+                                             ex)))))
+                       (throw (ex-info "Error getting Service Ticket"
+                                       {:type :unauthorized}
+                                       e)))))]
       (-> data
           (assoc-in [:headers "accept"] "*/*")
           (assoc-in [:headers "ticket"] ticket)

test/oph/ehoks/external/cas_test.clj

@@ -1,5 +1,6 @@
 (ns oph.ehoks.external.cas-test
   (:require [clojure.test :refer [deftest testing is use-fixtures]]
+            [clojure.string :as s]
             [oph.ehoks.external.cas :as c]
             [oph.ehoks.config :refer [config]]
             [oph.ehoks.external.http-client :as client]
@@ -27,8 +28,8 @@
       (t/minutes
         (inc (:ext-cache-lifetime-minutes config))))}})
 
-(deftest test-refresh-service-ticket
-  (testing "Refresh service ticket successfully"
+(deftest test-refresh-grant-ticket
+  (testing "Refresh grant ticket successfully"
     (reset! c/grant-ticket {:url nil :expires nil})
     (is (= (deref c/grant-ticket) {:url nil :expires nil}))
     (client/set-post!
@@ -43,10 +44,17 @@
     (is (= (:url (deref c/grant-ticket)) "test-url"))
     (is (some? (:expires (deref c/grant-ticket)))))
 
-  (testing "Refresh service ticket unsuccessfully"
+  (testing "Refresh grant ticket unsuccessfully (404)"
     (reset! c/grant-ticket {:url nil :expires nil})
     (client/set-post! (fn [_ options]
-                        {:status 404}))
+                        (throw (ex-info "HTTP Exception" {:status 404}))))
+    (is (thrown-with-msg? clojure.lang.ExceptionInfo
+                          #"HTTP Exception"
+                          (c/refresh-grant-ticket!))))
+  (testing "Refresh grant ticket unsuccessfully (missing location header)"
+    (reset! c/grant-ticket {:url nil :expires nil})
+    (client/set-post! (fn [_ options]
+                        {:status 201}))
     (is (thrown-with-msg? clojure.lang.ExceptionInfo
                           #"Failed to refresh CAS Service Ticket"
                           (c/refresh-grant-ticket!)))))
@@ -62,14 +70,87 @@
            "test-ticket"))))
 
 (deftest test-add-cas-ticket
-  (testing "Add service ticket"
+  (testing "Add service ticket successfully"
     (client/set-post! (fn [_ options] {:body "test-ticket"}))
 
     (reset! c/grant-ticket {:url "http://ticket.url"
                             :expires (t/plus (t/now) (t/hours 2))})
     (let [data (c/add-cas-ticket {} "http://test-service")]
       (is (= (get-in data [:headers "accept"]) "*/*"))
-      (is (= (get-in data [:query-params :ticket]) "test-ticket")))))
+      (is (= (get-in data [:query-params :ticket]) "test-ticket"))))
+  (testing "Add service ticket retries when get ST returns 404 once"
+    (let [get-st-returns-404 (atom true)
+          get-st-call-count (atom 0)]
+      (client/set-post!
+        (fn [url _]
+          (cond
+            ; refresh ticket granting ticket
+            (.endsWith url "/v1/tickets")
+            {:status 201 :headers {"location" "http://ticket.url"}}
+            ; get service ticket returns 404 once
+            (= url "http://ticket.url")
+            (do
+              (swap! get-st-call-count inc)
+              (if @get-st-returns-404
+                (do
+                  (reset! get-st-returns-404 false)
+                  (throw
+                    (ex-info
+                      "Test HTTP Exception"
+                      {:body
+                       "TGT-nnn could not be found or is considered invalid"
+                       :status 404})))
+                {:status 201 :body "test-ticket"})))))
+      (reset! c/grant-ticket {:url "http://ticket.url"
+                              :expires (t/plus (t/now) (t/hours 2))})
+      (let [data (c/add-cas-ticket {} "http://test-service")]
+        (is (= (get-in data [:headers "accept"]) "*/*"))
+        (is (= (get-in data [:query-params :ticket]) "test-ticket")))
+      (is (= @get-st-call-count 2))))
+  (testing "Add service ticket eventually throws"
+    (let [get-st-call-count (atom 0)]
+      (client/set-post!
+        (fn [url _]
+          (cond
+            ; refresh ticket granting ticket
+            (.endsWith url "/v1/tickets")
+            {:status 201 :headers {"location" "http://ticket.url"}}
+            ; get service ticket returns 404 continuously
+            (= url "http://ticket.url")
+            (do
+              (swap! get-st-call-count inc)
+              (throw
+                (ex-info
+                  "Test HTTP Exception"
+                  {:status 404
+                   :body
+                   "TGT-nnn could not be found or is considered invalid"}))))))
+      (reset! c/grant-ticket {:url "http://ticket.url"
+                              :expires (t/plus (t/now) (t/hours 2))})
+      (let [result (try
+                     (c/add-cas-ticket {} "http://test-service")
+                     nil
+                     (catch Exception e
+                       e))]
+        (is (= :unauthorized (:type (ex-data result)))))
+      (is (= @get-st-call-count 2))))
+  (testing "Add service ticket immediately throws when get ST returns 500"
+    (let [get-st-call-count (atom 0)]
+      (client/set-post! (fn [_ __]
+                          (swap! get-st-call-count inc)
+                          (throw (ex-info "Test HTTP Exception"
+                                          {:status 500
+                                           :body "Infernal server error"}))))
+      (reset! c/grant-ticket {:url "http://ticket.url"
+                              :expires (t/plus (t/now) (t/hours 2))})
+      (let [result (try
+                     (c/add-cas-ticket {} "http://test-service")
+                     nil
+                     (catch Exception e
+                       e))]
+        (is (= :unauthorized (:type (ex-data result)))))
+      ; not trying to retry in this case
+      (is (= @get-st-call-count 1)))))
 
 (deftest test-with-service-ticket
   (testing "Request with API headers"

test/oph/ehoks/hoks/hoks_handler_test.clj

@@ -864,3 +864,44 @@
                    (update-in [:osaamisen-osoittaminen 0] dissoc :module-id)
                    (dissoc :id :module-id))
                hato))))))
+
+(deftest test-handles-unauthorized
+  (testing "Responds with unauthorized when not able to get ST for käyttöoikeus"
+    (client/set-post!
+      (fn [url _]
+        (cond
+          (.endsWith url "/v1/tickets")
+          {:status 201
+           :headers {"location" "http://localhost/TGT-1234"}}
+          (= url "http://localhost/TGT-1234")
+          (throw
+            (ex-info
+              "Test HTTP Exception"
+              {:status 404
+               :body
+               "TGT-1234 could not be found or is considered invalid"})))))
+    (client/set-get!
+      (fn [url options]
+        (cond
+          (.endsWith url "/serviceValidate")
+          {:status 200
+           :body
+           (str "<cas:serviceResponse"
+                "  xmlns:cas='http://www.yale.edu/tp/cas'>"
+                "<cas:authenticationSuccess><cas:user>ehoks</cas:user>"
+                "<cas:attributes>"
+                "<cas:longTermAuthenticationRequestTokenUsed>false"
+                "</cas:longTermAuthenticationRequestTokenUsed>"
+                "<cas:isFromNewLogin>false</cas:isFromNewLogin>"
+                "<cas:authenticationDate>2019-02-20T10:14:24.046+02:00"
+                "</cas:authenticationDate></cas:attributes>"
+                "</cas:authenticationSuccess></cas:serviceResponse>")})))
+    (let [app (hoks-utils/create-app nil)
+          response (app (-> (mock/request
+                              :get
+                              (str hoks-utils/base-url "/1"))
+                            (mock/header "Caller-Id" "test")
+                            (mock/header "ticket" "ST-testitiketti")))]
+      (is (= (:status response) 401))
+      (is (= (utils/parse-body (:body response))
+             {:reason "Unable to check access rights"})))))