Repository: CSCfi/rems
Pull request #2867 (2022-03-19 11:23:53)

Cache user mappings
Closes #2774.

Users are fetched as before, but mappings are always cached in memory for performance.

# Checklist for author

Remove items that aren't applicable, check items that are done.

## Reviewability
- [x] Link to issue
- [x] Note if PR is on top of other PR

## Backwards compatibility
- [x] API is backwards compatible or completely new

## Documentation
- [ ] Update changelog if necessary (will be part of the new feature set, no need to advertise)

## Testing
- [x] Valuable features are integration / browser / acceptance tested automatically

## Follow-up
- [ ] New tasks are created for pending or remaining tasks (in the cache ticket we can improve more #2777)



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 65f41eaf6..81fe56de6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -21,7 +21,6 @@ NB: The login has changed to allow more configurable user identity and other att
 
 ### Fixes
 - API-key validity is not checked unless it is actually sent. (#2785)
-- API-Keys are cached for a minute for a slight performance improvement. (#2785)
 
 ## v2.25 "Meripuistotie" 2022-02-15
 
diff --git a/docs/architecture/014-userids.md b/docs/architecture/014-userids.md
index a32eb757f..c24a4fe9a 100644
--- a/docs/architecture/014-userids.md
+++ b/docs/architecture/014-userids.md
@@ -205,10 +205,6 @@ We can also use Lucene to implement user searching.
 
 [Issue #2774](https://github.com/CSCfi/rems/issues/2774)
 
-## Push / post etc. outbound APIs
-
-See issue https://github.com/CSCfi/rems/issues/2888.
-
 ## Notes
 
 2022-01-24 We discussed whether we should store or show more attributes of the users. For example, should the handler see the identities of the user. The old attributes are always replaced when the user comes again so changing IdP also may change the email (or other attributes), which could confuse the handler. We decided not to do anything about this yet, since this is existing behavior. Also storing old attributes might mean that we store and show a user's old email or name and there is no way to change them if the authentication method is removed.
diff --git a/docs/using-the-api.md b/docs/using-the-api.md
index ae8f258bd..6577b9be1 100644
--- a/docs/using-the-api.md
+++ b/docs/using-the-api.md
@@ -29,9 +29,7 @@ You can also authenticate by setting these two headers:
 NB: For the user id it can be an internal REMS id or an external id configured in `config.edn` through `oidc-userid-attributes`.
 If the user can't be found, an error will be returned.
 
-API keys can be defined and modified using the `api-key` command line option to REMS. NB: API-Keys are cached for a minute.
-
-Here are some examples:
+API keys can be defined and modified using the `api-key` command line option to REMS. Here are some examples:
 
 ```sh
 # Get all API keys:
diff --git a/resources/migrations/20220120121117-user-mappings.down.sql b/resources/migrations/20220120121117-user-mappings.down.sql
index ee135c9ff..257cd585c 100644
--- a/resources/migrations/20220120121117-user-mappings.down.sql
+++ b/resources/migrations/20220120121117-user-mappings.down.sql
@@ -1,2 +1,1 @@
-DROP TABLE IF EXISTS user_mappings CASCADE;
-
+DROP TABLE IF EXISTS user_mappings;
diff --git a/resources/migrations/20220120121117-user-mappings.up.sql b/resources/migrations/20220120121117-user-mappings.up.sql
index 2df959a49..98ed7032b 100644
--- a/resources/migrations/20220120121117-user-mappings.up.sql
+++ b/resources/migrations/20220120121117-user-mappings.up.sql
@@ -5,6 +5,3 @@ CREATE TABLE user_mappings (
   PRIMARY KEY (userId, extIdAttribute, extIdValue),
   FOREIGN KEY (userId) REFERENCES users
 );
---;;
-CREATE INDEX index_user_mappings ON user_mappings(extIdAttribute, extIdValue);
-
diff --git a/src/clj/rems/api/blacklist.clj b/src/clj/rems/api/blacklist.clj
index 01007350e..68d6bb012 100644
--- a/src/clj/rems/api/blacklist.clj
+++ b/src/clj/rems/api/blacklist.clj
@@ -36,7 +36,7 @@
       :query-params [{user :- schema-base/UserId nil}
                      {resource :- s/Str nil}]
       :return [BlacklistEntryWithDetails]
-      (ok (blacklist/get-blacklist {:userid (user-mappings/find-userid user)
+      (ok (blacklist/get-blacklist {:userid (when user (user-mappings/find-userid user))
                                     :resource/ext-id resource})))
 
     (GET "/users" []
diff --git a/src/clj/rems/application/commands.clj b/src/clj/rems/application/commands.clj
index 7e011477b..324431262 100644
--- a/src/clj/rems/application/commands.clj
+++ b/src/clj/rems/application/commands.clj
@@ -1,8 +1,10 @@
 (ns rems.application.commands
   (:require [clojure.test :refer [deftest is testing]]
+            [com.rpl.specter :refer [ALL transform]]
             [medley.core :refer [assoc-some]]
             [rems.common.application-util :as application-util]
             [rems.common.form :as form]
+            [rems.common.util :refer [build-index]]
             [rems.form-validation :as form-validation]
             [rems.permissions :as permissions]
             [rems.schema-base :as schema-base]
@@ -662,15 +664,11 @@
 (defn- find-users [cmd injections]
   ;; actor is handled already in middleware
   (case (:type cmd)
-    (:application.command/add-member :application.command/remove-member :application.command/change-applicant)
-    (update-in cmd [:member :userid] (getx injections :find-userid))
-
-    :application.command/request-review
-    (update cmd :reviewers #(mapv (getx injections :find-userid) %))
-
-    :application.command/request-decision
-    (update cmd :deciders #(mapv (getx injections :find-userid) %))
-
+    :application.command/add-member (update-in cmd [:member :userid] (getx injections :find-userid))
+    :application.command/remove-member (update-in cmd [:member :userid] (getx injections :find-userid))
+    :application.command/change-applicant (update-in cmd [:member :userid] (getx injections :find-userid))
+    :application.command/request-review (update cmd :reviewers #(mapv (getx injections :find-userid) %))
+    :application.command/request-decision (update cmd :deciders #(mapv (getx injections :find-userid) %))
     cmd))
 
 (defn handle-command [cmd application injections]
diff --git a/src/clj/rems/auth/auth.clj b/src/clj/rems/auth/auth.clj
index 3efc85b6a..6a06fd979 100644
--- a/src/clj/rems/auth/auth.clj
+++ b/src/clj/rems/auth/auth.clj
@@ -15,9 +15,8 @@
   (get-in request [:headers "x-rems-api-key"]))
 
 (defn get-api-user [request]
-  (-> request
-      (get-in [:headers "x-rems-user-id"])
-      user-mappings/find-userid))
+  (when-some [userid (get-in request [:headers "x-rems-user-id"])]
+    (user-mappings/find-userid userid)))
 
 (defn- api-key-backend []
   (reify
diff --git a/src/clj/rems/auth/oidc.clj b/src/clj/rems/auth/oidc.clj
index 864c1bd4e..ea6d8c288 100644
--- a/src/clj/rems/auth/oidc.clj
+++ b/src/clj/rems/auth/oidc.clj
@@ -30,14 +30,14 @@
   (for [{:keys [attribute rename]} (getx env :oidc-userid-attributes)
         :let [value (get id-data (keyword attribute))]
         :when value]
-    [(keyword (or rename attribute)) value]))
+    [(or rename attribute) value]))
 
 (deftest test-get-userid-attributes
   (with-redefs [env {:oidc-userid-attributes [{:attribute "sub" :rename "elixirId"}
                                               {:attribute "old_sub"}]}]
     (is (= [] (get-userid-attributes nil)))
-    (is (= [[:elixirId "elixir-alice"]
-            [:old_sub "alice"]]
+    (is (= [["elixirId" "elixir-alice"]
+            ["old_sub" "alice"]]
            (get-userid-attributes {:old_sub "alice"
                                    :sub "elixir-alice"
                                    :name "Alice Applicant"})))))
diff --git a/src/clj/rems/db/api_key.clj b/src/clj/rems/db/api_key.clj
index dc4274376..cf3beea5a 100644
--- a/src/clj/rems/db/api_key.clj
+++ b/src/clj/rems/db/api_key.clj
@@ -1,30 +1,19 @@
 (ns rems.db.api-key
-  (:require [clojure.core.memoize :as memo]
-            [clojure.test :refer :all]
-            [rems.common.util :refer [index-by update-present]]
+  (:require [clojure.test :refer :all]
             [rems.db.core :as db]
-            [rems.json :as json]))
+            [rems.json :as json]
+            [rems.util :refer [update-present]]))
 
 (defn- format-api-key [key]
   (-> key
       (update-present :users json/parse-string)
       (update-present :paths json/parse-string)))
 
-(defn- load-api-keys []
-  (->> (db/get-api-keys {})
-       (mapv format-api-key)
-       (index-by [:apikey])))
-
-;; API-Keys are cached for a while (60s).
-;; Whenever changed or removed through code, they are reloaded immediately.
-;; There won't be a difference in the timing, if an API-Key itself exists or not.
-(def ^:private api-key-memo (memo/ttl load-api-keys :ttl/threshold 60000))
-
 (defn get-api-key [key]
-  (get (api-key-memo) key))
+  (format-api-key (db/get-api-key {:apikey key})))
 
 (defn get-api-keys []
-  (vals (api-key-memo)))
+  (mapv format-api-key (db/get-api-keys {})))
 
 (defn- method= [method pattern]
   (or (= pattern "any")
@@ -69,13 +58,11 @@
   (db/upsert-api-key! {:apikey key
                        :comment comment
                        :users (when users (json/generate-string users))
-                       :paths (when paths (json/generate-string paths))})
-  (memo/memo-clear! api-key-memo))
+                       :paths (when paths (json/generate-string paths))}))
 
 (defn update-api-key! [key & [opts]]
   (add-api-key! key (merge (get-api-key key)
                            opts)))
 
 (defn delete-api-key! [key]
-  (db/delete-api-key! {:apikey key})
-  (memo/memo-clear! api-key-memo))
+  (db/delete-api-key! {:apikey key}))
diff --git a/src/clj/rems/db/core.clj b/src/clj/rems/db/core.clj
index 083f12a55..97f042a84 100644
--- a/src/clj/rems/db/core.clj
+++ b/src/clj/rems/db/core.clj
@@ -9,10 +9,10 @@
             [clj-time.jdbc] ;; convert db timestamps to joda-time objects
             [clojure.java.jdbc]
             [clojure.set :refer [superset?]]
+            [clojure.string :as str]
             [conman.core :as conman]
             [mount.core :refer [defstate] :as mount]
-            [rems.config :refer [env]]
-            [clojure.string :as str]))
+            [rems.config :refer [env]]))
 
 ;; See docs/architecture/010-transactions.md
 (defn- hikaricp-settings []
diff --git a/src/clj/rems/db/user_mappings.clj b/src/clj/rems/db/user_mappings.clj
index 9a6393df5..120b47cea 100644
--- a/src/clj/rems/db/user_mappings.clj
+++ b/src/clj/rems/db/user_mappings.clj
@@ -1,6 +1,6 @@
 (ns rems.db.user-mappings
   (:require [clojure.string :as str]
-            [medley.core :refer [map-vals]]
+            [rems.common.util :refer [conj-vec]]
             [rems.db.core :as db]
             [schema.core :as s]))
 
@@ -14,23 +14,53 @@
 
 (defn- format-user-mapping [mapping]
   {:userid (:userid mapping)
-   :ext-id-attribute (:extidattribute mapping)
+   :ext-id-attribute (keyword (:extidattribute mapping))
    :ext-id-value (:extidvalue mapping)})
 
-(defn get-user-mappings
-  [params]
-  (->> (db/get-user-mappings (map-vals name params))
+(defn- load-user-mappings []
+  (->> (db/get-user-mappings {})
        (mapv format-user-mapping)
        (mapv validate-user-mapping)
+       (group-by :ext-id-value)))
+
+;; NB: user mappings are always cached
+;; XXX: consider if this should rather be mount state eventually?
+(def ^:private user-mappings-by-value (atom nil))
+
+(defn- ensure-cached! []
+  (when (nil? @user-mappings-by-value)
+    (reset! user-mappings-by-value (load-user-mappings))))
+
+;; TODO: external API or process to reset cache if needed (db updated)
+(defn reset-cache! []
+  (reset! user-mappings-by-value nil))
+
+(defn get-user-mappings [params]
+  (ensure-cached!)
+  (->> (if (:ext-id-value params) ; can use index?
+         (get @user-mappings-by-value (:ext-id-value params))
+         (mapcat val @user-mappings-by-value))
+       (db/apply-filters (dissoc params :ext-id-value))
        not-empty))
 
 (defn create-user-mapping! [user-mapping]
-  (-> user-mapping
-      validate-user-mapping
-      db/create-user-mapping!))
+  (ensure-cached!)
+  (let [mapping (-> user-mapping
+                    validate-user-mapping)]
+    (db/create-user-mapping! mapping)
+    (swap! user-mappings-by-value
+           (fn [mappings]
+             (update mappings (:ext-id-value mapping) (comp vec distinct conj-vec) mapping)))))
 
 (defn delete-user-mapping! [userid]
-  (db/delete-user-mapping! {:userid userid}))
+  (ensure-cached!)
+  (db/delete-user-mapping! {:userid userid})
+  (swap! user-mappings-by-value
+         (fn [mappings]
+           (->> mappings
+                (mapcat val)
+                (remove #(= userid (:userid %)))
+                (group-by :ext-id-value)))))
 
 (defn find-userid
   "Figures out the `userid` of a user reference.
@@ -39,7 +69,7 @@
   Else the string is assumed to be a `userid`."
   [userid-or-ext-id]
   (when-not (str/blank? userid-or-ext-id)
-    (let [mappings (db/get-user-mappings {:ext-id-value userid-or-ext-id})]
+    (let [mappings (get-user-mappings {:ext-id-value userid-or-ext-id})]
       (assert (< (count mappings) 2) (str "Multiple users found with identity " (pr-str mappings)))
       (or (some :userid mappings)
           userid-or-ext-id))))
diff --git a/src/cljc/rems/common/util.cljc b/src/cljc/rems/common/util.cljc
index 2a6172dfc..36d7109f3 100644
--- a/src/cljc/rems/common/util.cljc
+++ b/src/cljc/rems/common/util.cljc
@@ -554,15 +554,3 @@
   (is (= [] (replace-key [] :a :b)))
   (is (= {:b 1} (replace-key {:a 1} :a :b)))
   (is (= {:a 1} (replace-key {:a 1} :b :c))))
-
-(defn update-present
-  "Like clojure.core/update, but does nothing if the key `k` does not exist in `m`."
-  [m k f & args]
-  (if (find m k)
-    (apply update m k f args)
-    m))
-
-(deftest test-update-present
-  (is (= {:a 1} (update-present {:a 1} :b (constantly true))))
-  (is (= {:a 1 :b true} (update-present {:a 1 :b 2} :b (constantly true))))
-  (is (= {:a 1 :b true} (update-present {:a 1 :b nil} :b (constantly true)))))


Test patch:
diff --git a/test/clj/rems/api/test_users.clj b/test/clj/rems/api/test_users.clj
index 6ed6389d5..f403e9194 100644
--- a/test/clj/rems/api/test_users.clj
+++ b/test/clj/rems/api/test_users.clj
@@ -132,6 +132,8 @@
                                                                                 {:attribute "old_sub"}])]
     (with-fake-login-users {"alice" {:sub "alice" :name "Alice Applicant" :email "alice@example.com" :nickname "In Wonderland"}
                             "elixir-alice" {:sub "elixir-alice" :old_sub "alice" :name "Elixir Alice" :email "alice@elixir-europe.org"}}
+      (is (nil? (user-mappings/get-user-mappings {:ext-id-attribute "elixirId" :ext-id-value "elixir-alice"})) "user mapping should not exist")
+
       (testing "log in alice"
         (let [cookie (login-with-cookies "alice")]
           (assert-can-make-a-request! cookie)
@@ -140,27 +142,32 @@
                            +test-api-key+ "owner")))
           (is (= {:userid "alice" :name "Alice Applicant" :email "alice@example.com" :nickname "In Wonderland"}
                  (users/get-user "alice")
-                 (users/format-user (:identity (middleware/get-session cookie)))))))
+                 (users/format-user (:identity (middleware/get-session cookie)))))
+          (is (= #{{:userid "alice" :name "Alice Applicant" :email "alice@example.com" :nickname "In Wonderland"}}
+                 (set (api-call :get "/api/users/active" nil
+                                +test-api-key+ "owner")))
+              "alice shows as active")))
 
       (testing "log in elixir-alice and create user mapping"
         (is (nil? (user-mappings/get-user-mappings {:ext-id-attribute "elixirId" :ext-id-value "elixir-alice"})) "user mapping should not exist")
         (let [cookie (login-with-cookies "elixir-alice")]
           (assert-can-make-a-request! cookie)
-          (is (= #{{:userid "alice" :name "Alice Applicant" :email "alice@example.com" :nickname "In Wonderland"}
-                   {:userid "alice" :name "Elixir Alice" :email "alice@elixir-europe.org"}}
-                 (set (api-call :get "/api/users/active" nil
-                                +test-api-key+ "owner"))))
           (is (= [{:userid "alice"
                    :ext-id-value "elixir-alice"
                    :ext-id-attribute "elixirId"}]
-                 (user-mappings/get-user-mappings {:ext-id-attribute "elixirId" :ext-id-avlue "elixir-alice"})))
+                 (user-mappings/get-user-mappings {:ext-id-attribute "elixirId" :ext-id-value "elixir-alice"})))
           (is (= {:userid "alice" :name "Elixir Alice" :email "alice@elixir-europe.org"}
                  (users/get-user "alice")
                  (users/format-user (:identity (middleware/get-session cookie))))
-              "Attributes should be updated when logging in"))))
+              "Attributes should be updated when logging in")
+          (is (= #{{:userid "alice" :name "Alice Applicant" :email "alice@example.com" :nickname "In Wonderland"}
+                   {:userid "alice" :name "Elixir Alice" :email "alice@elixir-europe.org"}}
+                 (set (api-call :get "/api/users/active" nil
+                                +test-api-key+ "owner")))
+              "both alices show as active"))))
 
     (with-fake-login-users {"elixir-alice" {:sub "elixir-alice" :name "Elixir Alice" :email "alice@elixir-europe.org"}}
-      (testing "log in elixir-alice with user mapping"
+      (testing "log in elixir-alice with user mapping, no old_sub"
         (is (= [{:userid "alice"
                  :ext-id-value "elixir-alice"
                  :ext-id-attribute "elixirId"}]
@@ -170,7 +177,67 @@
           (is (= #{{:userid "alice" :name "Alice Applicant" :email "alice@example.com" :nickname "In Wonderland"}
                    {:userid "alice" :name "Elixir Alice" :email "alice@elixir-europe.org"}}
                  (set (api-call :get "/api/users/active" nil
-                                +test-api-key+ "owner")))))))))
+                                +test-api-key+ "owner")))
+              "both alices show as active"))))
+
+    (testing "mappings create, get, delete"
+      (user-mappings/create-user-mapping! {:userid "alice"
+                                           :ext-id-value "alice-alt-id"
+                                           :ext-id-attribute "alt-id"})
+      (user-mappings/create-user-mapping! {:userid "alice"
+                                           :ext-id-value "alice-alt-id"
+                                           :ext-id-attribute "alt-id2"})
+      (is (= [{:userid "alice"
+               :ext-id-value "elixir-alice"
+               :ext-id-attribute "elixirId"}
+              {:userid "alice"
+               :ext-id-value "alice-alt-id"
+               :ext-id-attribute "alt-id"}
+              {:userid "alice"
+               :ext-id-value "alice-alt-id"
+               :ext-id-attribute "alt-id2"}]
+             (user-mappings/get-user-mappings {:userid "alice"})))
+      (is (= [{:userid "alice"
+               :ext-id-value "elixir-alice"
+               :ext-id-attribute "elixirId"}]
+             (user-mappings/get-user-mappings {:ext-id-value "elixir-alice"})))
+      (is (= [{:userid "alice"
+               :ext-id-value "alice-alt-id"
+               :ext-id-attribute "alt-id"}
+              {:userid "alice"
+               :ext-id-value "alice-alt-id"
+               :ext-id-attribute "alt-id2"}]
+             (user-mappings/get-user-mappings {:ext-id-value "alice-alt-id"})))
+
+      (user-mappings/delete-user-mapping! "unrelated") ; should not affect tested data
+
+      (is (= [{:userid "alice"
+               :ext-id-value "elixir-alice"
+               :ext-id-attribute "elixirId"}
+              {:userid "alice"
+               :ext-id-value "alice-alt-id"
+               :ext-id-attribute "alt-id"}
+              {:userid "alice"
+               :ext-id-value "alice-alt-id"
+               :ext-id-attribute "alt-id2"}]
+             (user-mappings/get-user-mappings {:userid "alice"})))
+      (is (= [{:userid "alice"
+               :ext-id-value "elixir-alice"
+               :ext-id-attribute "elixirId"}]
+             (user-mappings/get-user-mappings {:ext-id-value "elixir-alice"})))
+      (is (= [{:userid "alice"
+               :ext-id-value "alice-alt-id"
+               :ext-id-attribute "alt-id"}
+              {:userid "alice"
+               :ext-id-value "alice-alt-id"
+               :ext-id-attribute "alt-id2"}]
+             (user-mappings/get-user-mappings {:ext-id-value "alice-alt-id"})))
+
+      (user-mappings/delete-user-mapping! "alice")
+
+      (is (= nil (user-mappings/get-user-mappings {:userid "alice"})))
+      (is (= nil (user-mappings/get-user-mappings {:ext-id-value "elixir-alice"})))
+      (is (= nil (user-mappings/get-user-mappings {:ext-id-value "alice-alt-id"}))))))
 
 
 (deftest user-name-test
diff --git a/test/clj/rems/db/testing.clj b/test/clj/rems/db/testing.clj
index 54a90e5d5..deca71f63 100644
--- a/test/clj/rems/db/testing.clj
+++ b/test/clj/rems/db/testing.clj
@@ -1,19 +1,16 @@
 (ns rems.db.testing
-  (:require [clj-time.core :as time]
-            [clojure.java.jdbc :as jdbc]
+  (:require [clojure.java.jdbc :as jdbc]
             [clojure.test :refer :all]
-            [clojure.tools.logging :as log]
             [conman.core :as conman]
             [luminus-migrations.core :as migrations]
             [mount.core :as mount]
             [rems.application.search]
             [rems.config :refer [env]]
-            [rems.db.api-key :as api-key]
             [rems.db.applications]
             [rems.db.category :as category]
             [rems.db.core :as db]
             [rems.db.test-data :as test-data]
-            [rems.db.test-data-helpers :as test-helpers]
+            [rems.db.user-mappings :as user-mappings]
             [rems.locales])
   (:import [org.joda.time Duration ReadableInstant]))
 
@@ -45,7 +42,8 @@
     (mount/start #'rems.db.applications/all-applications-cache)
     (f)
     (finally
-      (category/reset-cache!))))
+      (category/reset-cache!)
+      (user-mappings/reset-cache!))))
 
 (def +test-api-key+ test-data/+test-api-key+) ;; re-exported for convenience