Skip to content
Snippets Groups Projects
Unverified Commit 5280d0bb authored by Cam Saul's avatar Cam Saul
Browse files

Require parent perms when moving/archiving Collections :lock::lock::lock::lock:

parent 0ded36a7
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/models/collection.clj
+ 22
− 16
View file @ 5280d0bb
......@@ -202,6 +202,14 @@
:id su/IntGreaterThanZero
s/Keyword s/Any}))
(s/defn ^:private parent :- CollectionWithLocationAndIDOrRoot
"Fetch the parent Collection of `collection`, or the Root Collection special placeholder object if this is a
top-level Collection."
[collection :- CollectionWithLocationOrRoot]
(if-let [new-parent-id (location-path->parent-id (:location collection))]
(Collection new-parent-id)
root-collection))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Nested Collections: "Effective" Location Paths |
;;; +----------------------------------------------------------------------------------------------------------------+
......@@ -406,10 +414,11 @@
A > B > C
To move or archive B, you need write permissions for both B and C, since C would be affected as well. You do *not*
need permissions for A, even though in some sense you are affecting to contents of A; this is allowed because we
want to let people archive Collections they can see (via 'effectively' pulling them up), even if they can't see the
parent Collection."
To move or archive B, you need write permissions for A, B, and C:
* A, because you are taking something out of it (by archiving it)
* B, because you are archiving it
* C, because by archiving its parent, you are archiving it as well"
[collection :- CollectionWithLocationAndIDOrRoot]
;; Make sure we're not trying to archive the Root Collection...
(when (is-root-collection? collection)
......@@ -418,8 +427,11 @@
(when (db/exists? Collection :id (u/get-id collection), :personal_owner_id [:not= nil])
(throw (Exception. (str (tru "You cannot archive a Personal Collection.")))))
(set
(for [collection-or-id (cons collection
(db/select-ids Collection :location [:like (str (children-location collection) "%")]))]
(for [collection-or-id (cons
(parent collection)
(cons
collection
(db/select-ids Collection :location [:like (str (children-location collection) "%")])))]
(perms/collection-readwrite-path collection-or-id))))
(s/defn perms-for-moving :- #{perms/ObjectPath}
......@@ -435,14 +447,12 @@
D D > B > C
To move or archive B, you would need write permissions for A, B, and D:
To move or archive B, you would need write permissions for A, B, C, and D:
* A, because we're moving something out of it
* B, since it's the Collection we're operating on
* C, since it will by definition be affected too
* D, because it's the new parent Collection, and moving something into it requires write perms.
Note that, like archiving, you *do not* need any permissions for A, the original parent; you can think of this
operation as something done on the Collection itself, rather than 'curating' its parent."
* D, because it's the new parent Collection, and moving something into it requires write perms."
[collection :- CollectionWithLocationAndIDOrRoot, new-parent :- CollectionWithLocationAndIDOrRoot]
;; Make sure we're not trying to move the Root Collection...
(when (is-root-collection? collection)
......@@ -655,11 +665,7 @@
bad experience -- we do not want a User to move a Collection that they have read/write perms for (by definition) to
somewhere else and lose all access for it."
[collection :- CollectionInstance, new-location :- LocationPath]
(let [new-parent-id (location-path->parent-id new-location)
new-parent (if new-parent-id
(Collection new-parent-id)
root-collection)]
(copy-collection-permissions! new-parent (cons collection (descendants collection)))))
(copy-collection-permissions! (parent {:location new-location}) (cons collection (descendants collection))))
(s/defn ^:private revoke-perms-when-moving-into-personal-collection!
"When moving a `collection` that is *not* a descendant of a Personal Collection into a Personal Collection or one of
......
test/metabase/models/collection_test.clj
+ 33
− 25
View file @ 5280d0bb
......@@ -848,9 +848,10 @@
;;; ---------------------------------------------- Perms for Archiving -----------------------------------------------
;; To Archive A, you should need *write* perms for A and all of its descendants...
;; To Archive A, you should need *write* perms for A and all of its descendants, and also the Root Collection...
(expect
#{"/collection/A/"
#{"/collection/root/"
"/collection/A/"
"/collection/B/"
"/collection/C/"
"/collection/D/"
......@@ -861,17 +862,19 @@
(->> (collection/perms-for-archiving a)
(perms-path-ids->names collections))))
;; Now let's move down a level. To archive B, you should only need permissions for B itself, since it doesn't
;; Now let's move down a level. To archive B, you should need permissions for A and B, since B doesn't
;; have any descendants
(expect
#{"/collection/B/"}
#{"/collection/A/"
"/collection/B/"}
(with-collection-hierarchy [{:keys [b], :as collections}]
(->> (collection/perms-for-archiving b)
(perms-path-ids->names collections))))
;; but for C, you should need perms for C, D, E, F, and G
;; but for C, you should need perms for A (parent); C; and D, E, F, and G (descendants)
(expect
#{"/collection/C/"
#{"/collection/A/"
"/collection/C/"
"/collection/D/"
"/collection/E/"
"/collection/F/"
......@@ -880,9 +883,10 @@
(->> (collection/perms-for-archiving c)
(perms-path-ids->names collections))))
;; For D you should need D + E
;; For D you should need C (parent), D, and E (descendant)
(expect
#{"/collection/D/"
#{"/collection/C/"
"/collection/D/"
"/collection/E/"}
(with-collection-hierarchy [{:keys [d], :as collections}]
(->> (collection/perms-for-archiving d)
......@@ -909,32 +913,34 @@
;; `*` marks the things that require permissions in charts below!
;; If we want to move B into C, we should need perms for B and C. B because it is being moved; C we are moving
;; something into it. Note that we do NOT require perms for A
;; If we want to move B into C, we should need perms for A, B, and C. B because it is being moved; C we are moving
;; something into it, A because we are moving something out of it
;;
;; +-> B +-> B*
;; | |
;; A -+-> C -+-> D -> E ===> A -> C* -+-> D -> E
;; | |
;; +-> F -> G +-> F -> G
;; +-> B +-> B*
;; | |
;; A -+-> C -+-> D -> E ===> A* -> C* -+-> D -> E
;; | |
;; +-> F -> G +-> F -> G
(expect
#{"/collection/B/"
#{"/collection/A/"
"/collection/B/"
"/collection/C/"}
(with-collection-hierarchy [{:keys [b c], :as collections}]
(->> (collection/perms-for-moving b c)
(perms-path-ids->names collections))))
;; Ok, now let's try moving something with descendants. If we move C into B, we need perms for C and all its
;; descendants, and B, since it's the new parent; we do not need perms for A, the old parent
;; descendants, and B, since it's the new parent; and A, the old parent
;;
;; +-> B
;; |
;; A -+-> C -+-> D -> E ===> A -> B* -> C* -+-> D* -> E*
;; | |
;; +-> F -> G +-> F* -> G*
;; A -+-> C -+-> D -> E ===> A* -> B* -> C* -+-> D* -> E*
;; | |
;; +-> F -> G +-> F* -> G*
(expect
#{"/collection/B/"
#{"/collection/A/"
"/collection/B/"
"/collection/C/"
"/collection/D/"
"/collection/E/"
......@@ -948,11 +954,12 @@
;;
;; +-> B B* [and Root*]
;; |
;; A -+-> C -+-> D -> E ===> A -> C -+-> D -> E
;; | |
;; +-> F -> G +-> F -> G
;; A -+-> C -+-> D -> E ===> A* -> C -+-> D -> E
;; | |
;; +-> F -> G +-> F -> G
(expect
#{"/collection/root/"
"/collection/A/"
"/collection/B/"}
(with-collection-hierarchy [{:keys [b], :as collections}]
(->> (collection/perms-for-moving b collection/root-collection)
......@@ -960,13 +967,14 @@
;; How about moving C into the Root Collection?
;;
;; +-> B A -> B
;; +-> B A* -> B
;; |
;; A -+-> C -+-> D -> E ===> C* -+-> D* -> E* [and Root*]
;; | |
;; +-> F -> G +-> F* -> G*
(expect
#{"/collection/root/"
"/collection/A/"
"/collection/C/"
"/collection/D/"
"/collection/E/"
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment