Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow handler to redact handler attachments #3189

Merged
merged 25 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8cb62ef
feat: allow handler to redact handler attachments
aatkin Sep 28, 2023
869b3e8
doc: update changelog
aatkin Sep 29, 2023
5b721ac
feat: add diana decider to redact attachments test
aatkin Sep 29, 2023
1263759
refactor: use more explicit cond returns
aatkin Sep 29, 2023
98c73cc
refactor: more conditional logic tuning
aatkin Sep 29, 2023
655a8cf
feat: show removed attachments in event log
aatkin Oct 2, 2023
d742b9b
fix: use proper attribute for fake idp user
aatkin Oct 2, 2023
8a4b7b7
feat: merge test decider workflows and add second handler remark
aatkin Oct 2, 2023
a675397
feat: change sv translations for redacted attachments
aatkin Oct 2, 2023
d93cd90
feat: render redacted attachments in pdf
aatkin Oct 2, 2023
567299d
feat: add decider user to demo data
aatkin Oct 3, 2023
1073e61
fix: assoc attachment/can-redact only when not redacted
aatkin Oct 3, 2023
871bc61
feat: render redacted attachments for handling users
aatkin Oct 5, 2023
f15c4b4
feat: hide event redacted attachments from applying users
aatkin Oct 5, 2023
86d2eb0
refactor: extract decider workflow redaction test from default test case
aatkin Oct 5, 2023
ab22101
refactor: show redacted attachments to applying users in complicated …
aatkin Oct 5, 2023
8177cd8
fix: missing key in test command
aatkin Oct 5, 2023
2cfec0e
doc: update changelog
aatkin Oct 5, 2023
22fcee5
chore: match extra structural dom text
aatkin Oct 6, 2023
2f4c55c
Merge remote-tracking branch 'origin/master' into handler-redaction-fix
aatkin Oct 12, 2023
cceffca
fix: add glue code to check attachment dom text in browser tests
aatkin Oct 12, 2023
2a17075
Merge remote-tracking branch 'origin/master' into handler-redaction-fix
aatkin Oct 12, 2023
7d0b5b9
fix: use group-by and remove indent from redact event attachments
aatkin Oct 12, 2023
25c09ad
fix: add missing :url to attachment link and update btu helpers
aatkin Oct 12, 2023
994cc6d
fix: change old test id
aatkin Oct 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ Changes since v2.33

### Changes
- "Show related events" has been removed from events. (#3156)
- Handler can now redact other handlers attachments. (#3190)

### Additions
- (Experimental) Workflow can be configured to enable voting for the approval. Currently all handlers can vote (including bots). Use `:enable-voting`. (#3174)
- There is now a Danish language translation (#3176). We are considering supporting a limited set of languages officially, and improving support for community maintained translations (see #3179).
- Added experimental support for named format parameters in translations. (#3183)
- Cache reloading can be configured using the new `:buzy-hours` config. REMS will then try to avoid reloading during the specified time spans. (#3194)
- The handling users can now see whether an event is shown to the applicant from a small eye icon in the event history. (#3156)
- Event now shows which attachments were redacted when viewing as handling user. (#3190)

### Fixes
- Email template parameters for `:application-expiration-notification` event are now documented. The parameters are different from standard event email parameters, which may have caused confusion.
Expand Down
2 changes: 2 additions & 0 deletions resources/translations/en.edn
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,9 @@
:last-activity "Last activity"
:latest-activity "Latest activity"
:my-applications "My Applications"
:redacted-attachments "Redacted attachments"
:redacted-attachments-replaced "New attachments replace the redacted attachments."
:replacing-attachments "New attachments"
:resource "Resource"
:state "State"
:states {:approved "Approved"
Expand Down
2 changes: 2 additions & 0 deletions resources/translations/fi.edn
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@
:last-activity "Muokattu"
:latest-activity "Viimeisin toimenpide"
:my-applications "Omat hakemukset"
:redacted-attachments "Poistetut liitetiedostot"
:redacted-attachments-replaced "Uudet liitetiedostot korvaavat poistetut liitetiedostot."
:replacing-attachments "Uudet liitetiedostot"
:resource "Resurssi"
:state "Tila"
:states {:approved "Hyväksytty"
Expand Down
4 changes: 3 additions & 1 deletion resources/translations/sv.edn
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@
:last-activity "Ändrat"
:latest-activity "Senaste aktivitet"
:my-applications "Mina ansökningar"
:redacted-attachments-replaced "Nya bilagor byts ut gamla bilagor."
:redacted-attachments "Borttagna bilagor"
:redacted-attachments-replaced "Nya bilagor byts ut borttagna bilagor."
:replacing-attachments "Nya bilagor"
:resource "Resurs"
:state "Status"
:states {:approved "Godkänd"
Expand Down
2 changes: 1 addition & 1 deletion src/clj/rems/application/commands.clj
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@
attachments (build-index {:keys [:attachment/id]} (:application/attachments application))
forbidden-ids (->> redacted-ids
(keep #(get attachments %))
(remove #(application-util/can-redact-attachment % roles (:actor cmd)))
(remove #(application-util/can-redact-attachment? % roles (:actor cmd)))
(map :attachment/id))]
(when (seq forbidden-ids)
{:errors [{:type :forbidden-redact-attachments
Expand Down
112 changes: 51 additions & 61 deletions src/clj/rems/application/model.clj
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
(:require [clojure.set :as set]
[clojure.test :refer [deftest is testing]]
[com.rpl.specter :refer [ALL transform select]]
[medley.core :refer [assoc-some distinct-by find-first map-vals update-existing update-existing-in]]
[medley.core :refer [distinct-by find-first map-vals update-existing update-existing-in]]
[rems.application.events :as events]
[rems.application.master-workflow :as master-workflow]
[rems.common.application-util :refer [applicant-and-members can-redact-attachment can-see-everything? is-applying-user?]]
[rems.common.application-util :refer [applicant-and-members can-redact-attachment? is-applying-user?]]
[rems.common.form :as form]
[rems.common.util :refer [assoc-some-in conj-vec getx getx-in into-vec]]
[rems.permissions :as permissions]
Expand Down Expand Up @@ -768,26 +768,26 @@

(permissions/blacklist application (permissions/compile-rules [{:permission :application.command/vote}])))))

(defn- get-attachment-redact-roles [attachment application]
(when (:attachment/event attachment)
(let [attachment-user-roles (->> (getx-in attachment [:attachment/user :userid])
(permissions/user-roles application))]
(cond
(some #{:handler} attachment-user-roles)
#{}
(defn- allowed-redact-roles [application attachment]
(let [event-id (get-in attachment [:attachment/event :event/id])
user (getx-in attachment [:attachment/user :userid])
roles (permissions/user-roles application user)
workflow (:application/workflow application)]
(cond
(not event-id) ; e.g. applicant attachment in form
#{}

(and (= :workflow/decider (get-in application [:application/workflow :workflow/type]))
(some #{:decider :past-decider} attachment-user-roles))
#{}
(and (= :workflow/decider (:workflow/type workflow))
(some #{:decider :past-decider} roles))
#{}

:else
#{:handler}))))
:else
#{:handler})))

(defn- enrich-attachments [application get-user]
(->> application
(transform [:application/attachments ALL] #(update % :attachment/user get-user))
(transform [:application/attachments ALL] #(let [roles (get-attachment-redact-roles % application)]
(assoc-some % :attachment/redact-roles roles)))))
(transform [:application/attachments ALL] #(assoc % :attachment/redact-roles (allowed-redact-roles application %)))))

(defn enrich-with-injections
[application {:keys [blacklisted?
Expand Down Expand Up @@ -896,32 +896,47 @@
(->> application
(transform [:application/forms ALL :form/fields ALL] #(apply-field-privacy % roles))))

(defn- may-see-redacted-filename [roles]
(some #{:handler :reporter}
roles))
(defn- hide-non-accessible-attachments [application]
(let [visible-ids (set (keys (classify-attachments application)))
visible? (comp visible-ids :attachment/id)]
(update application :application/attachments #(filterv visible? %))))

(defn- apply-attachment-privacy [attachment roles userid]
(let [see-filename (or (not (:attachment/redacted attachment))
(= userid (get-in attachment [:attachment/user :userid]))
(may-see-redacted-filename roles))]
(cond-> attachment
(not see-filename) (assoc :attachment/filename :filename/redacted))))
(defn- hide-redacted-filename [attachment permissions userid]
(cond
(not (:attachment/redacted attachment))
attachment

(defn apply-privacy-by-user [application roles userid]
(->> application
(transform [:application/attachments ALL] #(apply-attachment-privacy % roles userid))))
(= userid (get-in attachment [:attachment/user :userid]))
attachment

(contains? permissions :see-everything)
attachment

(defn apply-attachment-permissions [attachment roles userid]
(-> attachment
(assoc-some :attachment/can-redact (can-redact-attachment attachment roles userid))
(dissoc :attachment/redact-roles)))
:else (assoc attachment :attachment/filename :filename/redacted)))

(defn- set-redact-permissions [attachment roles userid]
(if (:attachment/redacted attachment)
attachment
(assoc attachment :attachment/can-redact (can-redact-attachment? attachment roles userid))))

(defn apply-attachments-privacy [application userid]
(let [roles (permissions/user-roles application userid)
permissions (permissions/user-permissions application userid)
see-everything? (contains? permissions :see-everything)]
(-> (if see-everything?
application
;; applying users do not need to see event redacted attachments, only the latest
(update application :application/events (partial mapv #(dissoc % :event/redacted-attachments))))
(update :application/attachments (partial mapv #(hide-redacted-filename % permissions userid)))
(update :application/attachments (partial mapv #(set-redact-permissions % roles userid))))))

(defn hide-non-public-information [application]
(-> application
hide-invitation-tokens
;; these are not used by the UI, so no need to expose them (especially the user IDs)
(dissoc ::latest-review-request-by-user ::latest-decision-request-by-user)
(dissoc :application/past-members)))
(dissoc :application/past-members)
(update :application/attachments (partial mapv #(dissoc % :attachment/redact-roles)))))

(defn- personalize-todo [application userid]
(cond-> application
Expand All @@ -931,30 +946,6 @@
(contains? (::latest-decision-request-by-user application) userid)
(assoc :application/todo :waiting-for-your-decision)))

(defn- visible-attachment-ids [application]
(->> application
classify-attachments
keys
set))

(deftest test-visible-attachment-ids
(let [application {:application/events [{:event/type :application.event/foo
:event/attachments [{:attachment/id 1}
{:attachment/id 3}]}
{:event/type :application.event/bar}]
:application/forms [{:form/fields [{:field/type :attachment
:field/value "5" :field/previous-value "7,15"}]}
{:form/fields [{:field/type :text
:field/value "2" :field/previous-vaule "2"}
{:field/type :attachment
:field/value "9,11,13"}]}]}]
(is (= #{1 3 5 7 9 11 13 15} (visible-attachment-ids application)))))

(defn- hide-attachments [application]
(let [visible-ids (visible-attachment-ids application)
visible? (comp visible-ids :attachment/id)]
(update application :application/attachments #(filterv visible? %))))

(defn see-application? [application userid]
(let [state (:application/state application)
roles (permissions/user-roles application userid)]
Expand All @@ -977,11 +968,10 @@
application
(hide-sensitive-information application))
(personalize-todo userid)
(hide-non-public-information)
(apply-privacy-by-roles roles)
(apply-privacy-by-user roles userid)
(hide-attachments)
(update :application/attachments (partial map #(apply-attachment-permissions % roles userid)))
(hide-non-accessible-attachments)
(apply-attachments-privacy userid)
(assoc :application/permissions permissions)
(assoc :application/roles roles)
(hide-non-public-information)
(permissions/cleanup)))))
15 changes: 12 additions & 3 deletions src/clj/rems/css/styles.clj
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@
[:&:hover {:color (theme-getx :link-hover-color :color4)}]]
[:.pointer {:cursor :pointer}
[:label.form-check-label {:cursor :pointer}]]
[:.disabled (make-important {:opacity 1})] ; XXX: bootstrap default has too low contrast for accessibility
[:html {:position :relative
:min-width (u/px 320)
:height (u/percent 100)}]
Expand Down Expand Up @@ -608,7 +609,6 @@
[:&:hover {:filter "brightness(80%)"}]]

(rems-table-styles)
[:.btn.disabled {:opacity 0.25}]
[:.catalogue-item-link {:color "#fff"
:text-decoration "underline"}]
[:.language-switcher {:padding ".5em 0"}]
Expand Down Expand Up @@ -854,10 +854,19 @@
(actions-float-menu)
[(s/descendant :.document :h3) {:margin-top (u/rem 4)}]

[:.break-newline {:white-space :pre-wrap}]

[:.attachments {:display :flex
:flex-flow "row wrap"
:align-items :center
:flex-direction :column
:flex-wrap :nowrap
:gap (u/rem 0.5)}]
[:.attachment-link {:width (u/percent 100)
:display :inline-flex
:flex-wrap :nowrap
:align-items :center}]

[:.download.disabled {:border-style :dashed}]
[(s/> :.download ":not(:first-child)") {:margin-left (u/rem 0.5)}]

;; print styling
(stylesheet/at-media
Expand Down
2 changes: 1 addition & 1 deletion src/clj/rems/db/test_data_helpers.clj
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@

(defn assert-no-existing-data! []
(assert (empty? (db/get-organizations {}))
"You have existing oranizations, refusing to continue. An empty database is needed.")
"You have existing organizations, refusing to continue. An empty database is needed.")
(assert (empty? (db/get-application-events {}))
"You have existing applications, refusing to continue. An empty database is needed.")
(assert (empty? (db/get-catalogue-items {}))
Expand Down
11 changes: 9 additions & 2 deletions src/clj/rems/db/test_data_users.clj
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
(ns rems.db.test-data-users
(:require [clojure.test :refer :all]
[rems.application.approver-bot :as approver-bot]
(:require [rems.application.approver-bot :as approver-bot]
[rems.application.bona-fide-bot :as bona-fide-bot]
[rems.application.rejecter-bot :as rejecter-bot]
[rems.application.expirer-bot :as expirer-bot]))
Expand All @@ -27,6 +26,7 @@
:owner "owner"
:reporter "reporter"
:reviewer "carl"
:decider "diana"
:roleless1 "elsa"
:roleless2 "frank"
:johnsmith "johnsmith"
Expand All @@ -38,6 +38,7 @@
"malice" {:userid "malice" :email "malice@example.com" :name "Malice Applicant" :twinOf "alice" :other "Attribute Value" :mappings {"alt-id" "malice-alt-id"}}
"handler" {:userid "handler" :email "handler@example.com" :name "Hannah Handler" :mappings {"alt-id" "handler-alt-id"}}
"carl" {:userid "carl" :email "carl@example.com" :name "Carl Reviewer" :mappings {"alt-id" "carl-alt-id"}}
"diana" {:userid "diana" :email "diana@example.com" :name "Diana Decider"}
"elsa" {:userid "elsa" :email "elsa@example.com" :name "Elsa Roleless" :mappings {"alt-id" "elsa-alt-id"}}
"frank" {:userid "frank" :email "frank@example.com" :name "Frank Roleless" :organizations [{:organization/id "frank"}]}
"organization-owner1" {:userid "organization-owner1" :email "organization-owner1@example.com" :name "Organization Owner 1"}
Expand All @@ -53,6 +54,7 @@
"malice" {:sub "malice" :email "malice@example.com" :name "Malice Applicant" :twinOf "alice" :other "Attribute Value"}
"handler" {:sub "handler" :email "handler@example.com" :name "Hannah Handler"}
"carl" {:sub "carl" :email "carl@example.com" :name "Carl Reviewer"}
"diana" {:sub "diana" :email "diana@example.com" :name "Diana Decider"}
"elsa" {:sub "elsa" :email "elsa@example.com" :name "Elsa Roleless"}
"frank" {:sub "frank" :email "frank@example.com" :name "Frank Roleless" :organizations [{:organization/id "frank"}]}
"organization-owner1" {:sub "organization-owner1" :email "organization-owner1@example.com" :name "Organization Owner 1"}
Expand Down Expand Up @@ -87,6 +89,8 @@
:description "Owner is the user who owns all the resources and manages them in the administration. They can create the catalogue of resources, workflows, forms etc. They can also delegate this power to other users and set who handles which applications, but aren't necessarily the person who does this work themselves."}
{:userid "carl"
:description "Carl is an expert, who is sometimes asked to review applications, thus taking a small part in the handling process."}
{:userid "diana"
:description "Diana is an expert, who is sometimes asked to decide whether applications should be approved or rejected, thus taking a small part in the handling process."}
{:userid "reporter"
:description "Reporter is an administrative users, who is only interested in counting beans and applications, having no part in the actual handling."}]}

Expand Down Expand Up @@ -120,6 +124,7 @@
:approver1 "RDapprover1@funet.fi"
:approver2 "RDapprover2@funet.fi"
:reviewer "RDreview@funet.fi"
:decider "RDdecider@funet.fi"
:organization-owner1 "RDorganizationowner1@funet.fi"
:organization-owner2 "RDorganizationowner2@funet.fi"
:owner "RDowner@funet.fi"
Expand All @@ -131,6 +136,7 @@
"RDapprover1@funet.fi" {:userid "RDapprover1@funet.fi" :email "RDapprover1.test@rems_example.org" :name "RDapprover1 REMSDEMO"}
"RDapprover2@funet.fi" {:userid "RDapprover2@funet.fi" :email "RDapprover2.test@rems_example.org" :name "RDapprover2 REMSDEMO"}
"RDreview@funet.fi" {:userid "RDreview@funet.fi" :email "RDreview.test@rems_example.org" :name "RDreview REMSDEMO"}
"RDdecider@funet.fi" {:userid "RDdecider@funet.fi" :email "RDdecider.test@rems_example.org" :name "RDdecider REMSDEMO"}
"RDowner@funet.fi" {:userid "RDowner@funet.fi" :email "RDowner.test@test_example.org" :name "RDowner REMSDEMO"}
"RDorganizationowner1@funet.fi" {:userid "RDorganizationowner1@funet.fi" :email "RDorganizationowner1.test@test_example.org" :name "RDorganizationowner1 REMSDEMO" :organizations [{:organization/id "organization1"}]}
"RDorganizationowner2@funet.fi" {:userid "RDorganizationowner2@funet.fi" :email "RDorganizationowner2.test@test_example.org" :name "RDorganizationowner2 REMSDEMO" :organizations [{:organization/id "organization2"}]}
Expand All @@ -142,6 +148,7 @@
"RDapprover1@funet.fi" {:sub "RDapprover1@funet.fi" :email "RDapprover1.test@rems_example.org" :name "RDapprover1 REMSDEMO"}
"RDapprover2@funet.fi" {:sub "RDapprover2@funet.fi" :email "RDapprover2.test@rems_example.org" :name "RDapprover2 REMSDEMO"}
"RDreview@funet.fi" {:sub "RDreview@funet.fi" :email "RDreview.test@rems_example.org" :name "RDreview REMSDEMO"}
"RDdecider@funet.fi" {:userid "RDdecider@funet.fi" :email "RDdecider.test@rems_example.org" :name "RDdecider REMSDEMO"}
"RDowner@funet.fi" {:sub "RDowner@funet.fi" :email "RDowner.test@test_example.org" :name "RDowner REMSDEMO"}
"RDorganizationowner1@funet.fi" {:sub "RDorganizationowner1@funet.fi" :email "RDorganizationowner1.test@test_example.org" :name "RDorganizationowner1 REMSDEMO" :organizations [{:organization/id "organization1"}]}
"RDorganizationowner2@funet.fi" {:sub "RDorganizationowner2@funet.fi" :email "RDorganizationowner2.test@test_example.org" :name "RDorganizationowner2 REMSDEMO" :organizations [{:organization/id "organization2"}]}
Expand Down
Loading