✨ Add same-subscription option to move-teams permission (#11995)

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"

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")

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

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

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)))))