diff --git a/backend/test/backend_tests/rpc_nitrate_test.clj b/backend/test/backend_tests/rpc_nitrate_test.clj index 978a91ca19..1d279f7fda 100644 --- a/backend/test/backend_tests/rpc_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_nitrate_test.clj @@ -1336,6 +1336,90 @@ (t/is (th/success? out)) (t/is (empty? @sent)))))) +;; The move-teams tests below simulate a second session: Nitrate answers +;; with the permissions as they are at the moment of the call, as if the +;; organization owner had changed them after the user opened the modal. + +(defn- move-team-nitrate-mock + [{:keys [owner-id team-id source-organization perms-by-organization-id calls]}] + (fn [_cfg method params] + (swap! calls conj method) + (case method + :get-organization-membership {:is-member true :organization-id (:organization-id params)} + :get-organization-members [owner-id] + :get-team-organization {:organization source-organization} + :get-organization-permissions (get perms-by-organization-id (:organization-id params)) + :set-team-organization {:id team-id} + :get-organization-sso {:active false} + :get-organization-summary {:id (:organization-id params) :teams [{:id team-id}]} + nil))) + +(t/deftest remove-team-from-organization-follows-move-teams + (doseq [[i move-teams allowed?] [[401 "always" true] + [402 "myOrganizations" false] + [403 "never" false]]] + (t/testing move-teams + (let [owner (th/create-profile* i {:is-active true}) + team (th/create-team* i {:profile-id (:id owner)}) + organization-id (uuid/random) + calls (atom [])] + (with-redefs [cf/flags (conj cf/flags :admin-console) + nitrate/call (move-team-nitrate-mock + {:owner-id (:id owner) + :team-id (:id team) + :perms-by-organization-id + {organization-id {:owner-id (uuid/random) + :permissions {:move-teams move-teams}}} + :calls calls})] + (let [out (th/command! {::th/type :remove-team-from-organization + ::rpc/profile-id (:id owner) + :team-id (:id team) + :organization-id organization-id + :organization-name "Org"})] + (if allowed? + (t/is (th/success? out)) + (t/is (= :not-allowed (th/ex-code (:error out))))) + (t/is (= allowed? + (contains? (set @calls) :remove-team-from-organization))))))))) + +(t/deftest add-team-to-organization-move-follows-move-teams + (doseq [[i desc move-teams same-owner? requester-owns-organization? allowed?] + [[411 "myOrganizations, same owner" "myOrganizations" true false true] + [412 "myOrganizations, other owner" "myOrganizations" false false false] + [413 "never" "never" true false false] + [414 "never, organization owner" "never" true true false]]] + (t/testing desc + (let [owner (th/create-profile* i {:is-active true}) + team (th/create-team* i {:profile-id (:id owner)}) + source-id (uuid/random) + target-id (uuid/random) + source-owner-id (if requester-owns-organization? (:id owner) (uuid/random)) + target-owner-id (if same-owner? source-owner-id (uuid/random)) + calls (atom [])] + (with-redefs [cf/flags (conj cf/flags :admin-console) + nitrate/call (move-team-nitrate-mock + {:owner-id (:id owner) + :team-id (:id team) + :source-organization {:id source-id} + :perms-by-organization-id + {source-id {:owner-id source-owner-id + :permissions {:create-teams "any" + :move-teams move-teams}} + target-id {:owner-id target-owner-id + :permissions {:create-teams "any" + :move-teams "always"}}} + :calls calls}) + teams/initialize-user-in-organization (fn [& _] nil)] + (let [out (th/command! {::th/type :add-team-to-organization + ::rpc/profile-id (:id owner) + :team-id (:id team) + :organization-id target-id})] + (if allowed? + (t/is (th/success? out)) + (t/is (= :not-allowed (th/ex-code (:error out))))) + (t/is (= allowed? + (contains? (set @calls) :set-team-organization))))))))) + (t/deftest get-nitrate-activation-code-request (let [profile (th/create-profile* 1 {:is-active true}) nitrate-id "nitrate-instance-1" diff --git a/common/src/app/common/types/organization.cljc b/common/src/app/common/types/organization.cljc index f601cf2971..380832fe00 100644 --- a/common/src/app/common/types/organization.cljc +++ b/common/src/app/common/types/organization.cljc @@ -100,6 +100,16 @@ :else false)) (defn- can-move-team? + "Checks the source organization's `:move-teams` value. The check that + the user owns the team happens elsewhere. + + - \"always\": the team can move to any organization, or be taken out + of the organization. + - \"myOrganizations\": the team can move only to an organization with + the same owner, and cannot be taken out. A person has one Enterprise + subscription, so the same owner means the same subscription. + - \"never\": only the Admin Console can move teams. Nobody moves them + from Penpot, not even the organization owner." [{:keys [permission-value target-organization-same-owner?]}] (cond (= permission-value "never") diff --git a/common/test/common_tests/types/organization_test.cljc b/common/test/common_tests/types/organization_test.cljc index 1d2c48be01..e4907a26d7 100644 --- a/common/test/common_tests/types/organization_test.cljc +++ b/common/test/common_tests/types/organization_test.cljc @@ -73,12 +73,12 @@ (let [always-organization (assoc organization-perms :permissions {:create-teams "any" :delete-teams "onlyOwners" :move-teams "always"})] - ;; Organization owner should always be allowed + ;; Team owners can move the team anywhere, or take it out (t/is (true? (cto/allowed? :move-team {:organization-perms always-organization :profile-id :owner :team-perms {}}))) - ;; Regular member should be allowed when move-teams is "always" + ;; Being the organization owner makes no difference (t/is (true? (cto/allowed? :move-team {:organization-perms always-organization :profile-id :member @@ -88,7 +88,8 @@ (let [my-organizations (assoc organization-perms :permissions {:create-teams "any" :delete-teams "onlyOwners" :move-teams "myOrganizations"})] - ;; Organization owner must also stay within same-owner organizations + ;; Same owner means same subscription: the organization owner is + ;; bound by it too (t/is (false? (cto/allowed? :move-team {:organization-perms my-organizations :profile-id :owner @@ -99,7 +100,7 @@ :profile-id :owner :team-perms {} :target-organization-same-owner? true}))) - ;; Regular member should be allowed only if target has same owner + ;; A team owner who is not the organization owner, same rule (t/is (true? (cto/allowed? :move-team {:organization-perms my-organizations :profile-id :member @@ -111,11 +112,27 @@ :team-perms {} :target-organization-same-owner? false}))))) +(t/deftest move-team-myorganizations-denies-taking-the-team-out + (let [my-organizations (assoc organization-perms :permissions {:create-teams "any" + :delete-teams "onlyOwners" + :move-teams "myOrganizations"})] + ;; Taking a team out has no target organization, so no same owner + (t/is (false? (cto/allowed? :move-team + {:organization-perms my-organizations + :profile-id :owner + :team-perms {}}))) + (t/is (false? (cto/allowed? :move-team + {:organization-perms my-organizations + :profile-id :member + :team-perms {} + :target-organization-same-owner? nil}))))) + (t/deftest move-team-never-denies-all (let [never-organization (assoc organization-perms :permissions {:create-teams "any" :delete-teams "onlyOwners" :move-teams "never"})] - ;; Even organization owner should be denied + ;; Only the Admin Console moves teams: the organization owner is + ;; denied in Penpot too (t/is (false? (cto/allowed? :move-team {:organization-perms never-organization :profile-id :owner diff --git a/frontend/src/app/main/data/nitrate.cljs b/frontend/src/app/main/data/nitrate.cljs index 67ddad852e..7c3efefb40 100644 --- a/frontend/src/app/main/data/nitrate.cljs +++ b/frontend/src/app/main/data/nitrate.cljs @@ -414,7 +414,18 @@ :subscription-status subscription-status}) pending-id (str (uuid/next)) callback-url (dom/append-query-param (rt/get-current-href) - :pending-action-id pending-id)] + :pending-action-id pending-id) + ;; The permissions may have changed in Nitrate since the modal + ;; was opened: refresh them and explain why the action failed. + on-error + (fn [cause] + (if (= :not-allowed (-> cause ex-data :code)) + (rx/of (dt/fetch-teams) + (modal/show :no-permission-modal + {:type (if (= team-previous-organization-status "other-organization") + :no-organizations-change + :no-organizations-create)})) + (rx/throw cause)))] (rx/concat (when-not skip-audit? (rx/of audit-event)) @@ -423,7 +434,8 @@ (fn [{:keys [authorized redirect-uri]}] (if authorized (->> (rp/cmd! ::add-team-to-organization {:team-id team-id :organization-id organization-id}) - (rx/map (fn [_] (modal/hide)))) + (rx/map (fn [_] (modal/hide))) + (rx/catch on-error)) (if redirect-uri (do (ss/save-pending-action! pending-id {:type :add-team-to-organization diff --git a/frontend/test/frontend_tests/data/nitrate_test.cljs b/frontend/test/frontend_tests/data/nitrate_test.cljs index 365715d2e4..2ea4442157 100644 --- a/frontend/test/frontend_tests/data/nitrate_test.cljs +++ b/frontend/test/frontend_tests/data/nitrate_test.cljs @@ -15,10 +15,12 @@ [app.main.data.notifications :as ntf] [app.main.data.team :as dt] [app.main.repo :as rp] + [app.main.router :as rt] [app.main.store :as st] [app.main.ui.auth.verify-token :as verify-token] [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] + [frontend-tests.helpers.async :as async] [frontend-tests.helpers.mock :as mock] [potok.v2.core :as ptk])) @@ -352,3 +354,43 @@ {:callback callback}) parsed (-> href u/uri :query u/query-string->map :callback)] (t/is (= callback parsed))))) + +(defn- ^:async observe-add-team-rejected-by-permissions + "Runs `add-team-to-organization` for `team` while the backend rejects it + with `:not-allowed`, as when another session changed the permissions + after the modal opened. Resolves to the emitted events." + [team] + (let [emitted (atom []) + event (dnt/add-team-to-organization {:team-id (:id team) + :organization-id "org-2" + :skip-audit? true}) + state {:teams {(:id team) team}}] + (await + (mock/with-mocks* + {rt/get-current-href (mock/stub (constantly "http://localhost/#/dashboard")) + rp/cmd! (mock/stub + (fn [cmd _params] + (case cmd + :check-nitrate-sso (rx/of {:authorized true}) + ::dnt/add-team-to-organization + (rx/throw (ex-info "not allowed" {:type :validation :code :not-allowed}))))) + modal/show (mock/stub (fn [& args] {:modal-show args}))} + (await (async/observe (ptk/watch event state nil) + :on-next #(swap! emitted conj %))))) + @emitted)) + +(t/deftest ^:async add-team-to-organization-shows-no-permission-on-stale-move + (let [emitted (await (observe-add-team-rejected-by-permissions + {:id "team-1" :organization {:id "org-1"}}))] + (t/is (= 2 (count emitted))) + (t/is (= ::dt/fetch-teams (ptk/type (first emitted)))) + (t/is (= {:modal-show [:no-permission-modal {:type :no-organizations-change}]} + (second emitted))))) + +(t/deftest ^:async add-team-to-organization-shows-no-permission-on-stale-add + (let [emitted (await (observe-add-team-rejected-by-permissions + {:id "team-1"}))] + (t/is (= 2 (count emitted))) + (t/is (= ::dt/fetch-teams (ptk/type (first emitted)))) + (t/is (= {:modal-show [:no-permission-modal {:type :no-organizations-create}]} + (second emitted)))))