From 7d16d507ecde00c8121122da2a305d4ced5fbdef Mon Sep 17 00:00:00 2001 From: joseAyudarte91 Date: Thu, 31 Aug 2023 21:59:09 +0200 Subject: [PATCH] Fix experts invitations related permissions handling [Re #1582] Fixed query to get experts results needed to be able to detect an expert user needs to be converted to an approved user. Besides, we have instrumented the code of update stakeholder-related function to unassign unapproved user role from the expert user in case it is detected as such, and be able to assign the approved user role in that case as well. The added functions have their rollbacks as well and we have also added one for the update stakeholder's final function, to have a consistent behaviour. On the other hand the endpoint to accept invitations looks not to be in use and such this looks like the only entrypoint to fix, but we will check. Now we are also catching better the exceptions, using Throwable class. --- backend/src/gpml/db/stakeholder.sql | 1 + backend/src/gpml/handler/stakeholder.clj | 11 +-- .../src/gpml/handler/stakeholder/expert.clj | 58 +++++++++--- backend/src/gpml/service/stakeholder.clj | 89 +++++++++++++++++-- 4 files changed, 134 insertions(+), 25 deletions(-) diff --git a/backend/src/gpml/db/stakeholder.sql b/backend/src/gpml/db/stakeholder.sql index 55bb580b4..f6ee7149f 100644 --- a/backend/src/gpml/db/stakeholder.sql +++ b/backend/src/gpml/db/stakeholder.sql @@ -174,6 +174,7 @@ update stakeholder set --~ (when (contains? params :idp_usernames) "idp_usernames= :idp_usernames::jsonb,") --~ (when (contains? params :picture_id) "picture_id= :picture_id,") --~ (when (contains? params :cv_id) "cv_id= :cv_id, ") +--~ (when (contains? params :review_status) "review_status= :review_status::review_status,") modified = now() where id = :id; diff --git a/backend/src/gpml/handler/stakeholder.clj b/backend/src/gpml/handler/stakeholder.clj index a5f138d12..292f7ae39 100644 --- a/backend/src/gpml/handler/stakeholder.clj +++ b/backend/src/gpml/handler/stakeholder.clj @@ -201,11 +201,12 @@ (fn [{:keys [body-params headers user]}] (try (let [tags (handler.stakeholder.tag/api-stakeholder-tags->stakeholder-tags body-params)] - (update-stakeholder config (assoc body-params - :id (:id user) - :picture {:payload (:picture body-params) - :user-agent (get headers "user-agent")} - :tags tags)) + (update-stakeholder config (-> body-params + (assoc :id (:id user) + :picture {:payload (:picture body-params) + :user-agent (get headers "user-agent")} + :tags tags) + (dissoc :review_status))) (resp/status {:success? true} 204)) (catch Exception e (log logger :error ::failed-to-update-stakeholder {:exception-message (.getMessage e)}) diff --git a/backend/src/gpml/handler/stakeholder/expert.clj b/backend/src/gpml/handler/stakeholder/expert.clj index 67eac4688..4105f54cb 100644 --- a/backend/src/gpml/handler/stakeholder/expert.clj +++ b/backend/src/gpml/handler/stakeholder/expert.clj @@ -8,6 +8,7 @@ [gpml.handler.resource.permission :as h.r.permission] [gpml.handler.responses :as r] [gpml.handler.stakeholder.tag :as handler.stakeholder.tag] + [gpml.service.permissions :as srv.permissions] [gpml.util :as util] [gpml.util.email :as email] [gpml.util.postgresql :as pg-util] @@ -179,6 +180,8 @@ (log logger :error ::send-invitation-emails-failed {:exception-message (.getMessage e) :context-data {:invitations invitations}})))) +;; TODO: Improve how we deal with errors here, since we should rollback invitation processes one by one, as otherwise +;; we might rollback all of them while the notifications or some of them have been sent already. (defn- invite-experts [{:keys [db mailjet-config logger] :as config} {{:keys [body]} :parameters}] @@ -202,21 +205,48 @@ logger mailjet-config {:tags (handler.stakeholder.tag/api-stakeholder-tags->stakeholder-tags {:expertise expertise}) - :stakeholder-id stakeholder-id})) + :stakeholder-id stakeholder-id}) + (let [{:keys [success?]} (srv.permissions/create-resource-context + {:conn conn + :logger logger} + {:context-type :stakeholder + :resource-id stakeholder-id})] + (if-not success? + (throw (ex-info + "Failing creating stakeholder rbac context" + {:reason :failing-creating-stakeholder-rbac-context + :stakeholder-id stakeholder-id})) + (let [role-assignments [{:role-name :unapproved-user + :context-type :application + :resource-id srv.permissions/root-app-resource-id + :user-id stakeholder-id}] + result (first (srv.permissions/assign-roles-to-users + {:conn conn + :logger logger} + role-assignments))] + (when-not (:success? result) + (throw (ex-info + "Failing assigning roles to stakeholder" + {:reason :failing-assigning-roles-to-stakeholder + :role-assignments role-assignments}))))))) (future (send-invitation-emails config invitations)) - (resp/response {:success? true - :invited-experts (map #(update % :id str) invitations)}))) - (catch Exception e - (log logger :error ::invite-experts-error {:exception-message (.getMessage e)}) - (if (instance? SQLException e) - {:status 500 - :body {:success? false - :reason (if (= :unique-constraint-violation (pg-util/get-sql-state e)) - :stakeholder-email-already-exists - (pg-util/get-sql-state e))}} - {:status 500 - :body {:success? false - :reason :could-not-create-expert}})))) + (r/ok {:success? true + :invited-experts (map #(update % :id str) invitations)}))) + (catch Throwable t + (log logger :error ::invite-experts-error {:exception-message (ex-message t) + :exception-data (ex-data t) + :stack-trace (.getStackTrace t) + :context-data body}) + (if (instance? SQLException t) + (r/server-error + {:success? false + :reason (if (= :unique-constraint-violation (pg-util/get-sql-state t)) + :stakeholder-email-already-exists + (pg-util/get-sql-state t))}) + (let [{:keys [reason]} (ex-data t)] + (r/server-error + {:success? false + :reason reason})))))) (defn- generate-admins-expert-suggestion-text [{:keys [email expertise suggested_expertise] :as expert} diff --git a/backend/src/gpml/service/stakeholder.clj b/backend/src/gpml/service/stakeholder.clj index 43c85ef4b..9dc0a4b62 100644 --- a/backend/src/gpml/service/stakeholder.clj +++ b/backend/src/gpml/service/stakeholder.clj @@ -1,5 +1,6 @@ (ns gpml.service.stakeholder - (:require [gpml.db.organisation :as db.organisation] + (:require [duct.logger :refer [log]] + [gpml.db.organisation :as db.organisation] [gpml.db.resource.tag :as db.resource.tag] [gpml.db.stakeholder :as db.stakeholder] [gpml.domain.file :as dom.file] @@ -318,12 +319,89 @@ (fn get-experts [{:keys [old-stakeholder] :as context}] (let [expert? (seq (db.stakeholder/get-experts conn {:filters {:ids [(:id old-stakeholder)]} - :page-size 0 + :page-size 1 :offset 0}))] - (assoc context :expert? expert?)))} + (assoc context :expert? expert?))) + :rollback-fn + (fn rollback-unassign-unapproved-user-role + [{:keys [old-stakeholder invited-expert?] :as context}] + (if invited-expert? + (let [role-assignments [{:role-name :unapproved-user + :context-type :application + :resource-id srv.permissions/root-app-resource-id + :user-id (:id old-stakeholder)}] + result (first (srv.permissions/assign-roles-to-users + {:conn conn + :logger logger} + role-assignments))] + (when-not (:success? result) + (log logger :error ::rollback-unassign-unapproved-user-role {:reason result}))) + (dissoc context :invited-expert?)))} + {:txn-fn + (fn unassign-unapproved-user-role + [{:keys [old-stakeholder expert?] :as context}] + (if (and expert? + (= (:review-status old-stakeholder) "INVITED")) + (let [role-unassignments [{:role-name :unapproved-user + :context-type :application + :resource-id srv.permissions/root-app-resource-id + :user-id (:id old-stakeholder)}] + result (first (srv.permissions/unassign-roles-from-users + {:conn conn + :logger logger} + role-unassignments))] + (if (:success? result) + (assoc context :invited-expert? true) + (assoc context + :success? false + :reason :failed-to-remove-unapproved-user-role + :error-details {:result result}))) + context)) + :rollback-fn + (fn rollback-assign-approved-user-role + [{:keys [old-stakeholder invited-expert?] :as context}] + (if invited-expert? + (let [role-unassignments [{:role-name :approved-user + :context-type :application + :resource-id srv.permissions/root-app-resource-id + :user-id (:id old-stakeholder)}] + result (first (srv.permissions/unassign-roles-from-users + {:conn conn + :logger logger} + role-unassignments))] + (when-not (:success? result) + (log logger :error ::rollback-assign-approved-user-role {:reason result})) + context) + context))} + {:txn-fn + (fn assign-approved-user-role + [{:keys [old-stakeholder invited-expert?] :as context}] + (if invited-expert? + (let [role-assignments [{:role-name :approved-user + :context-type :application + :resource-id srv.permissions/root-app-resource-id + :user-id (:id old-stakeholder)}] + result (first (srv.permissions/assign-roles-to-users + {:conn conn + :logger logger} + role-assignments))] + (if (:success? result) + context + (assoc context + :success? false + :reason :failed-to-add-approved-user-role + :error-details {:result result}))) + context)) + :rollback-fn + (fn rollback-update-stakeholder + [{:keys [old-stakeholder] :as context}] + (let [affected (db.stakeholder/update-stakeholder conn old-stakeholder)] + (when-not (= 1 affected) + (log logger :error ::rollback-update-stakeholder {:id (:id old-stakeholder)}))) + context)} {:txn-fn (fn update-stakeholder - [{:keys [stakeholder old-stakeholder expert? picture-file cv-file] :as context}] + [{:keys [stakeholder old-stakeholder invited-expert? picture-file cv-file] :as context}] (let [idp-usernames (-> (concat (:idp-usernames old-stakeholder) (:idp_usernames stakeholder)) distinct @@ -342,8 +420,7 @@ cv-id (assoc :cv_id cv-id) - (and expert? - (= (:review-status old-stakeholder) "INVITED")) + invited-expert? (assoc :review_status "APPROVED")))] (if (= 1 affected) context