From 2df4d10bd5aa316c7e9f683dd497f0a28d8bbc3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Valderrama?= Date: Fri, 6 Nov 2026 11:40:50 +0100 Subject: [PATCH] :sparkles: Enforce attribute-based organization permissions (#12868) * :sparkles: Enforce attribute-based organization permissions * :paperclip: Code review --- backend/src/app/nitrate.clj | 32 ++- backend/src/app/rpc/commands/nitrate.clj | 22 +- backend/src/app/rpc/commands/teams.clj | 6 +- .../app/rpc/commands/teams_invitations.clj | 22 +- backend/src/app/rpc/management/nitrate.clj | 41 ++++ .../rpc_management_nitrate_test.clj | 42 ++++ .../test/backend_tests/rpc_nitrate_test.clj | 91 ++++++++ backend/test/backend_tests/rpc_team_test.clj | 205 ++++++++++++++++++ common/src/app/common/types/organization.cljc | 49 +++-- .../common_tests/types/organization_test.cljc | 158 ++++++++++++++ frontend/src/app/main/data/common.cljs | 28 ++- frontend/src/app/main/data/nitrate.cljs | 33 ++- frontend/src/app/main/data/team.cljs | 6 +- frontend/src/app/main/ui/dashboard/team.cljs | 8 +- .../frontend_tests/data/dashboard_test.cljs | 36 ++- 15 files changed, 702 insertions(+), 77 deletions(-) diff --git a/backend/src/app/nitrate.clj b/backend/src/app/nitrate.clj index e475c531c1..d0d8c3e219 100644 --- a/backend/src/app/nitrate.clj +++ b/backend/src/app/nitrate.clj @@ -444,7 +444,8 @@ [:map [:organization-id ::sm/uuid] [:owner-id ::sm/uuid] - [:permissions [:map-of :keyword :string]]] + [:permissions [:map-of :keyword :string]] + [:restrictions-met {:optional true} [:maybe [:map-of :keyword :boolean]]]] params)) (defn- get-organization-sso-api @@ -652,6 +653,35 @@ :cause cause) team))))) +(defn add-organization-info-to-team-strict + "Like `add-organization-info-to-team`, but fails closed for authorization + checks: the team is returned unchanged only when Nitrate answers 404 (the + team belongs to no organization). Any other failure, including a 4xx or an + invalid response body, raises instead of looking like a team without an + organization." + [cfg team params] + (let [params (assoc (or params {}) + :team-id (:id team) + :throw-on-error? true) + team-with-organization + (try + (call cfg :get-team-organization params) + (catch Throwable cause + (let [{:keys [type status]} (ex-data cause)] + (if (and (= :nitrate-http-error type) (= 404 status)) + ::no-organization + (throw cause)))))] + (cond + (= ::no-organization team-with-organization) + team + + (nil? team-with-organization) + (ex/raise :type :nitrate-unavailable + :hint "nitrate did not return a valid team organization response") + + :else + (apply-organization-info-to-team team team-with-organization)))) + (defn add-organization-info-to-teams "Enriches teams with organization information using one batched Nitrate request. Teams absent from the Nitrate response are returned unchanged. diff --git a/backend/src/app/rpc/commands/nitrate.clj b/backend/src/app/rpc/commands/nitrate.clj index 0a51654c3a..af52bede6f 100644 --- a/backend/src/app/rpc/commands/nitrate.clj +++ b/backend/src/app/rpc/commands/nitrate.clj @@ -445,7 +445,8 @@ ;; Check moveTeams permission on the source organization (when (contains? cf/flags :admin-console) (let [organization-perms (nitrate/call cfg :get-organization-permissions - {:organization-id organization-id})] + {:organization-id organization-id + ::rpc/profile-id profile-id})] (if (nil? organization-perms) (ex/raise :type :validation :code :not-allowed @@ -480,8 +481,10 @@ "Returns info about external (non-organization-member) invitations pending for a team. External invitations are those sent to users who are not members of the given organization. Returns {:allows-anybody bool :external-emails [...]}" - [{:keys [::db/conn] :as cfg} team-id organization-id] - (let [organization-perms (nitrate/call cfg :get-organization-permissions {:organization-id organization-id}) + [{:keys [::db/conn] :as cfg} team-id organization-id profile-id] + (let [organization-perms (nitrate/call cfg :get-organization-permissions + {:organization-id organization-id + ::rpc/profile-id profile-id}) allows-anybody (cto/allowed? :add-anybody-to-team {:organization-perms organization-perms})] (if allows-anybody {:allows-anybody true :external-emails []} @@ -516,13 +519,16 @@ (when (contains? cf/flags :admin-console) (let [organization-member-ids-before (into #{} (nitrate/call cfg :get-organization-members {:organization-id organization-id})) - team-with-organization (nitrate/call cfg :get-team-organization {:team-id team-id}) + team-with-organization (nitrate/call cfg :get-team-organization {:team-id team-id + ::rpc/profile-id profile-id}) source-organization-id (get-in team-with-organization [:organization :id]) source-organization-perms (when source-organization-id (nitrate/call cfg :get-organization-permissions - {:organization-id source-organization-id})) + {:organization-id source-organization-id + ::rpc/profile-id profile-id})) target-organization-perms (nitrate/call cfg :get-organization-permissions - {:organization-id organization-id}) + {:organization-id organization-id + ::rpc/profile-id profile-id}) target-organization-same-owner? (and (some? source-organization-perms) (some? target-organization-perms) (= (:owner-id source-organization-perms) @@ -571,7 +577,7 @@ (notifications/notify-team-change cfg team "dashboard.team-belong-organization")) ;; Delete pending invitations for users who are not members of the target organization - (let [{:keys [allows-anybody external-emails]} (get-external-invitation-info cfg team-id organization-id)] + (let [{:keys [allows-anybody external-emails]} (get-external-invitation-info cfg team-id organization-id profile-id)] (when (and (not allows-anybody) (seq external-emails)) (let [conn (::db/conn cfg) emails-array (db/create-array conn "text" external-emails)] @@ -686,7 +692,7 @@ (ex/raise :type :validation :code :insufficient-permissions)) (nitrate/assert-membership cfg profile-id organization-id) - (let [{:keys [allows-anybody external-emails]} (get-external-invitation-info cfg team-id organization-id)] + (let [{:keys [allows-anybody external-emails]} (get-external-invitation-info cfg team-id organization-id profile-id)] {:has-external-invitations (boolean (seq external-emails)) :allows-anybody allows-anybody})) {:has-external-invitations false diff --git a/backend/src/app/rpc/commands/teams.clj b/backend/src/app/rpc/commands/teams.clj index c0bd5b52ee..d96c9a9cf1 100644 --- a/backend/src/app/rpc/commands/teams.clj +++ b/backend/src/app/rpc/commands/teams.clj @@ -544,7 +544,8 @@ (nitrate/assert-membership cfg profile-id organization-id) (let [organization-perms (nitrate/call cfg :get-organization-permissions - {:organization-id organization-id})] + {:organization-id organization-id + ::rpc/profile-id profile-id})] (if (nil? organization-perms) (ex/raise :type :validation :code :not-allowed @@ -892,8 +893,7 @@ can-delete? (if in-organization? (cto/allowed? :delete-team - {:organization-perms {:owner-id (dm/get-in team [:organization :owner-id]) - :permissions (dm/get-in team [:organization :permissions])} + {:organization-perms organization :profile-id profile-id :team-perms perms}) (boolean (:is-owner perms)))] diff --git a/backend/src/app/rpc/commands/teams_invitations.clj b/backend/src/app/rpc/commands/teams_invitations.clj index a58d4dfa11..d151f28287 100644 --- a/backend/src/app/rpc/commands/teams_invitations.clj +++ b/backend/src/app/rpc/commands/teams_invitations.clj @@ -382,13 +382,12 @@ "Unified function to handle both create and resend team invitations. Accepts either: - emails (set) + role (single role for all emails) - - invitations (vector of {:email :role} maps)" + - invitations (vector of {:email :role} maps) + + `team` must already carry its organization info when nitrate is + active (see `nitrate/add-organization-info-to-team-strict`)." [{:keys [::db/conn] :as cfg} {:keys [profile team role emails invitations] :as params}] - (let [;; Enrich team with organization info once for all invitations when nitrate is active - team (if (contains? cf/flags :admin-console) - (nitrate/add-organization-info-to-team cfg team {}) - team) - organization (:organization team) + (let [organization (:organization team) organization-id (:id organization) restricted? (and organization-id (not (cto/allowed? :add-anybody-to-team {:organization-perms organization}))) all-organization-member-ids @@ -488,8 +487,10 @@ (let [perms (teams/get-permissions cfg profile-id team-id) profile (db/get-by-id cfg :profile profile-id) team (db/get-by-id cfg :team team-id) + ;; Strict lookup: a failed Nitrate call must not skip the + ;; organization policy below by looking like a team without one. team-with-org (when (contains? cf/flags :admin-console) - (nitrate/add-organization-info-to-team cfg team {})) + (nitrate/add-organization-info-to-team-strict cfg team {::rpc/profile-id profile-id})) organization (:organization team-with-org) ;; Determine which format is being used using-emails-format? (and emails role) @@ -509,8 +510,7 @@ (when (and (contains? cf/flags :admin-console) organization (not (cto/allowed? :send-invitations - {:organization-perms {:owner-id (:owner-id organization) - :permissions (:permissions organization)} + {:organization-perms organization :profile-id profile-id :team-perms perms}))) (ex/raise :type :validation @@ -540,7 +540,9 @@ ;; Check if the current profile is allowed to send emails (teams/check-profile-muted cfg profile) - (let [team (db/get-by-id cfg :team team-id) + (let [;; Reuse the strict organization lookup made for the policy + ;; check above, so every rule sees the same Nitrate answer. + team (or team-with-org team) ;; NOTE: Is important pass RPC method params down to the ;; `create-team-invitations` because it uses the implicit ;; RPC properties from params for fill necessary data on diff --git a/backend/src/app/rpc/management/nitrate.clj b/backend/src/app/rpc/management/nitrate.clj index 5c99d172db..55a409a0a5 100644 --- a/backend/src/app/rpc/management/nitrate.clj +++ b/backend/src/app/rpc/management/nitrate.clj @@ -610,6 +610,47 @@ RETURNING id, deleted_at;") (db/exec! conn [sql:get-organization-member-team-counts ids-array]))))))) +;; ---- API: get-organization-team-roles + +(def ^:private sql:get-organization-team-roles + "SELECT t.id AS team_id, t.name AS team_name, tpr.profile_id, + CASE WHEN tpr.is_owner THEN 'owner' ELSE 'admin' END AS role + FROM team_profile_rel AS tpr + JOIN team AS t ON t.id = tpr.team_id + JOIN profile AS p ON p.id = tpr.profile_id + WHERE t.id = ANY(?) + AND t.deleted_at IS NULL + AND t.is_default IS FALSE + AND p.deleted_at IS NULL + AND (tpr.is_owner OR tpr.is_admin);") + +(def ^:private schema:get-organization-team-roles-params + [:map [:team-ids [:vector ::sm/uuid]]]) + +(def ^:private schema:get-organization-team-roles-result + [:vector [:map + [:team-id ::sm/uuid] + [:team-name ::sm/text] + [:profile-id ::sm/uuid] + [:role [:enum :owner :admin]]]]) + +(sv/defmethod ::get-organization-team-roles + "Get the owner and admin profiles of each non-default team within a set of teams." + {::doc/added "2.20" + ::rpc/allowed-shared-keys allowed-shared-key-ids + ::sm/params schema:get-organization-team-roles-params + ::sm/result schema:get-organization-team-roles-result + ::rpc/auth false} + [cfg {:keys [team-ids]}] + (if (empty? team-ids) + [] + (db/run! cfg (fn [{:keys [::db/conn]}] + (let [ids-array (db/create-array conn "uuid" team-ids)] + (->> (db/exec! conn [sql:get-organization-team-roles ids-array]) + (mapv (fn [{:keys [role] :as row}] + (assoc row :role (keyword role)))))))))) + + ;; ---- API: get-organization-member-teams (def ^:private sql:get-organization-member-teams diff --git a/backend/test/backend_tests/rpc_management_nitrate_test.clj b/backend/test/backend_tests/rpc_management_nitrate_test.clj index 3fd53422c7..2bc29482c0 100644 --- a/backend/test/backend_tests/rpc_management_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_management_nitrate_test.clj @@ -305,6 +305,48 @@ (t/is (th/success? out)) (t/is (= [] (:result out)))))) +(t/deftest get-organization-team-roles-returns-owners-and-admins-only + (with-mocks [nitrate-mock {:target 'app.nitrate/call :return nil}] + (let [owner (th/create-profile* 1 {:is-active true}) + admin (th/create-profile* 2 {:is-active true}) + editor (th/create-profile* 3 {:is-active true}) + viewer (th/create-profile* 4 {:is-active true}) + deleted (th/create-profile* 5 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)}) + deleted-team (th/create-team* 2 {:profile-id (:id owner)}) + _ (th/db-update! :team + {:deleted-at (ct/now)} + {:id (:id deleted-team)}) + _ (doseq [[profile role] [[admin :admin] + [editor :editor] + [viewer :viewer] + [deleted :admin]]] + (th/create-team-role* {:team-id (:id team) + :profile-id (:id profile) + :role role})) + _ (th/db-update! :profile + {:deleted-at (ct/now)} + {:id (:id deleted)}) + team-ids (mapv :id [team deleted-team {:id (:default-team-id owner)}]) + out (th/management-command! {::th/type :get-organization-team-roles + ::rpc/profile-id (:id owner) + :team-ids team-ids})] + (t/is (th/success? out)) + (t/is (= {(:id owner) :owner + (:id admin) :admin} + (->> out :result (map (juxt :profile-id :role)) (into {})))) + (t/is (= #{(:id team)} (into #{} (map :team-id) (:result out)))) + (t/is (= [(:name team)] (->> out :result (map :team-name) distinct)))))) + +(t/deftest get-organization-team-roles-empty-team-ids + (with-mocks [nitrate-mock {:target 'app.nitrate/call :return nil}] + (let [profile (th/create-profile* 1 {:is-active true}) + out (th/management-command! {::th/type :get-organization-team-roles + ::rpc/profile-id (:id profile) + :team-ids []})] + (t/is (th/success? out)) + (t/is (= [] (:result out)))))) + (t/deftest update-profile-theme-updates-only-theme (with-mocks [nitrate-mock {:target 'app.nitrate/call :return nil}] (let [profile (th/create-profile* 1 {:is-active true diff --git a/backend/test/backend_tests/rpc_nitrate_test.clj b/backend/test/backend_tests/rpc_nitrate_test.clj index 09f36a10a6..abbfc679fc 100644 --- a/backend/test/backend_tests/rpc_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_nitrate_test.clj @@ -1420,6 +1420,97 @@ (t/is (= allowed? (contains? (set @calls) :set-team-organization))))))))) +(t/deftest remove-team-from-organization-follows-move-team-restriction + (doseq [[i restrictions-met allowed?] [[431 {:move-teams false} false] + [432 {:move-teams true} true]]] + (t/testing (str "move-teams restriction met=" (:move-teams restrictions-met)) + (let [owner (th/create-profile* i {:is-active true}) + team (th/create-team* i {:profile-id (:id owner)}) + organization-id (uuid/random) + perms-params (atom nil) + calls (atom [])] + (with-redefs [cf/flags (conj cf/flags :admin-console) + nitrate/call + (fn [_cfg method params] + (swap! calls conj method) + (case method + :get-organization-membership {:is-member true :organization-id (:organization-id params)} + :get-organization-members [(:id owner)] + :get-team-organization {:organization nil} + :get-organization-permissions + (do + (reset! perms-params params) + {:owner-id (uuid/random) + :permissions {:move-teams "always"} + :restrictions-met restrictions-met}) + :remove-team-from-organization nil + nil))] + (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))))) + ;; the caller's profile-id travels to the permissions lookup + (t/is (= (:id owner) (::rpc/profile-id @perms-params))) + (t/is (= allowed? + (contains? (set @calls) :remove-team-from-organization))))))))) + +(t/deftest add-team-to-organization-follows-move-and-create-restrictions + (doseq [[i desc source-met target-met allowed?] + [[441 "source move restriction not met" {:move-teams false} {:create-teams true} false] + [442 "target create restriction not met" {:move-teams true} {:create-teams false} false] + [443 "both met" {:move-teams true} {:create-teams true} true]]] + (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) + org-owner-id (uuid/random) + perms-params (atom []) + calls (atom [])] + (with-redefs [cf/flags (conj cf/flags :admin-console) + nitrate/call + (fn [_cfg method params] + (swap! calls conj method) + (case method + :get-organization-membership {:is-member true :organization-id (:organization-id params)} + :get-organization-members [(:id owner)] + :get-team-organization + (do + (swap! perms-params conj params) + {:organization {:id source-id}}) + :get-organization-permissions + (do + (swap! perms-params conj params) + (cond + (= (:organization-id params) source-id) + {:owner-id org-owner-id + :permissions {:create-teams "any" :move-teams "always"} + :restrictions-met source-met} + (= (:organization-id params) target-id) + {:owner-id org-owner-id + :permissions {:create-teams "any" :move-teams "always"} + :restrictions-met target-met})) + :set-team-organization {:id (:id team)} + :get-organization-sso {:active false} + :get-organization-summary {:id (:organization-id params) :teams [{:id (:id team)}]} + nil)) + 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))))) + ;; every gating lookup carries the caller's profile-id + (t/is (every? #(= (:id owner) (::rpc/profile-id %)) @perms-params)) + (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/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 602585c8b3..277228ffcd 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -170,6 +170,120 @@ (t/is (false? (get-in create-plain [:props :adds-invitee-to-organization]))) (t/is (false? (get-in create-plain [:props :invitee-already-organization-member]))))))) +(t/deftest create-team-invitations-follows-send-invitations-restriction + (with-mocks [email-mock {:target 'app.email/send :return nil} + audit-mock {:target 'app.loggers.audit/submit :return nil}] + (let [org-owner (th/create-profile* 420 {:is-active true}) + admin (th/create-profile* 421 {:is-active true}) + invitee (th/create-profile* 422 {:is-active true}) + team (th/create-team* 420 {:profile-id (:id org-owner)}) + _ (th/create-team-role* {:team-id (:id team) + :profile-id (:id admin) + :role :admin}) + organization-id (uuid/random) + team-org-params (atom nil) + make-organization + (fn [restrictions-met] + {:id organization-id + :name "Acme" + :slug "acme" + :owner-id (:id org-owner) + :avatar-bg-url "https://example.com/avatar.svg" + :permissions {:send-invitations "ownersAndAdmins" + :new-team-members "anyone"} + :restrictions-met restrictions-met}) + make-nitrate-call + (fn [restrictions-met] + (fn [_cfg method params] + (case method + :get-team-organization + (do + (reset! team-org-params params) + (if (= (:team-id params) (:id team)) + {:organization (make-organization restrictions-met) :is-your-penpot false} + {:organization nil :is-your-penpot false})) + + :get-organization-members + [(:id invitee)] + + nil))) + invite! + (fn [] + (th/command! {::th/type :create-team-invitations + ::rpc/profile-id (:id admin) + :team-id (:id team) + :role :editor + :emails [(:email invitee)]}))] + + ;; Restriction not met: a team admin (not the org owner) is denied. + (with-redefs [cf/flags (conj cf/flags :admin-console :email-verification) + nitrate/call (make-nitrate-call {:send-invitations false})] + (let [out (invite!)] + (t/is (not (th/success? out))) + (t/is (= :insufficient-permissions (th/ex-code (:error out)))) + ;; the caller's profile-id travels to Nitrate on the team lookup + (t/is (= (:id admin) (::rpc/profile-id @team-org-params))))) + + ;; Restriction met: the same team admin can invite. + (with-redefs [cf/flags (conj cf/flags :admin-console :email-verification) + nitrate/call (make-nitrate-call {:send-invitations true})] + (t/is (th/success? (invite!))))))) + +(t/deftest create-team-invitations-fails-closed-when-organization-lookup-fails + (with-mocks [email-mock {:target 'app.email/send :return nil} + audit-mock {:target 'app.loggers.audit/submit :return nil}] + (let [owner (th/create-profile* 430 {:is-active true}) + admin (th/create-profile* 431 {:is-active true}) + invitee (th/create-profile* 432 {:is-active true}) + team (th/create-team* 430 {:profile-id (:id owner)}) + _ (th/create-team-role* {:team-id (:id team) + :profile-id (:id admin) + :role :admin}) + make-nitrate-call + (fn [get-team-organization] + (fn [_cfg method params] + (case method + :get-team-organization (get-team-organization params) + nil))) + invite! + (fn [] + (th/command! {::th/type :create-team-invitations + ::rpc/profile-id (:id admin) + :team-id (:id team) + :role :editor + :emails [(:email invitee)]}))] + + ;; A 4xx other than 404 must not look like a team without organization. + (with-redefs [cf/flags (conj cf/flags :admin-console :email-verification) + nitrate/call (make-nitrate-call + (fn [_] + (throw (ex-info "forbidden" + {:type :nitrate-http-error + :status 403}))))] + (let [out (invite!)] + (t/is (not (th/success? out))) + (t/is (= :nitrate-http-error (th/ex-type (:error out)))))) + + ;; An invalid response body (the client returns nil) is rejected too. + (with-redefs [cf/flags (conj cf/flags :admin-console :email-verification) + nitrate/call (make-nitrate-call (constantly nil))] + (let [out (invite!)] + (t/is (not (th/success? out))) + (t/is (= :nitrate-unavailable (th/ex-type (:error out)))))) + + ;; A 404 confirms the team has no organization: team-level rules apply. + (let [lookups (atom [])] + (with-redefs [cf/flags (conj cf/flags :admin-console :email-verification) + nitrate/call (make-nitrate-call + (fn [params] + (swap! lookups conj params) + (throw (ex-info "not found" + {:type :nitrate-http-error + :status 404}))))] + (t/is (th/success? (invite!))) + ;; one strict lookup serves every organization rule + (t/is (= [true] (mapv :throw-on-error? @lookups)))))))) + (t/deftest create-team-invitations-blacklisted-domain (with-mocks [mock {:target 'app.email/send-template :return nil}] (let [profile1 (th/create-profile* 1 {:is-active true}) @@ -830,6 +944,41 @@ :organization))))))) +(t/deftest get-teams-propagates-profile-id-and-restrictions-met + (let [profile (th/create-profile* 1 {:is-active true}) + organization-team (th/create-team* 1 {:profile-id (:id profile)}) + organization-id (uuid/random) + calls (atom []) + restrictions-met {:create-teams true :move-teams false :send-invitations true} + organization {:id organization-id + :name "Acme" + :slug "acme" + :owner-id (:id profile) + :avatar-bg-url "https://example.com/avatar.svg" + :restrictions-met restrictions-met} + nitrate-call (fn [_cfg method params] + (swap! calls conj [method params]) + [{:id (:id organization-team) + :is-your-penpot false + :organization organization}]) + params {::th/type :get-teams + ::rpc/profile-id (:id profile)}] + (with-redefs [cf/flags (conj cf/flags :admin-console) + nitrate/call nitrate-call] + (let [out (th/command! params) + teams (:result out) + team (first (filter #(= (:id organization-team) (:id %)) teams))] + (t/is (th/success? out)) + ;; The batched lookup carries the caller's profile-id so Nitrate can + ;; answer :restrictions-met for this profile. + (t/is (= :get-teams-organizations (ffirst @calls))) + (t/is (= (:id profile) (-> @calls first second ::rpc/profile-id))) + ;; :restrictions-met flows through apply-organization into the nested + ;; :organization map (organization->team-keys). + (t/is (= restrictions-met + (get-in team [:organization :restrictions-met]))))))) + + (t/deftest get-teams-rejects-invalid-organization-batch-response (let [profile (th/create-profile* 1 {:is-active true}) calls (atom []) @@ -1306,6 +1455,62 @@ (t/is (uuid? (:id team))) (t/is (= "Test Team" (:name team))))))))) +(t/deftest create-team-in-organization-follows-attribute-restriction + (with-mocks [audit-mock {:target 'app.loggers.audit/submit :return nil}] + (let [org-owner (th/create-profile* 410 {:is-active true}) + member (th/create-profile* 411 {:is-active true}) + organization-id (uuid/random) + perms-params (atom nil) + make-nitrate-call + (fn [restrictions-met] + (fn [_cfg method p] + (case method + :get-organization-membership + {:organization-id organization-id :is-member true} + + :get-organization-permissions + (do + (reset! perms-params p) + {:owner-id (:id org-owner) + :permissions {:create-teams "any"} + :restrictions-met restrictions-met}) + + :set-team-organization + (let [team-id (:team-id p)] + {:id team-id + :name "Test Team" + :organization-id organization-id + :default-project-id (uuid/random)}) + + nil))) + create! + (fn [profile-id] + (th/command! {::th/type :create-team + ::rpc/profile-id profile-id + :name "Test Team" + :organization-id organization-id}))] + + ;; A non-owner member is denied when the create-teams restriction is not met + (with-redefs [cf/flags (conj cf/flags :admin-console) + nitrate/call (make-nitrate-call {:create-teams false})] + (let [out (create! (:id member))] + (t/is (not (th/success? out))) + (t/is (= :not-allowed (th/ex-code (:error out)))) + ;; the caller's profile-id travels to Nitrate so it can evaluate the restriction + (t/is (= (:id member) (::rpc/profile-id @perms-params))))) + + ;; The same member is allowed when the restriction is met + (th/reset-mock! audit-mock) + (with-redefs [cf/flags (conj cf/flags :admin-console) + nitrate/call (make-nitrate-call {:create-teams true})] + (t/is (th/success? (create! (:id member))))) + + ;; The organization owner keeps the create-teams bypass even when not met + (th/reset-mock! audit-mock) + (with-redefs [cf/flags (conj cf/flags :admin-console) + nitrate/call (make-nitrate-call {:create-teams false})] + (t/is (th/success? (create! (:id org-owner)))))))) + ;; --- T7-F-01: Role ceiling in team invitations --- (t/deftest admin-cannot-create-invitation-with-owner-role diff --git a/common/src/app/common/types/organization.cljc b/common/src/app/common/types/organization.cljc index 380832fe00..82428eab16 100644 --- a/common/src/app/common/types/organization.cljc +++ b/common/src/app/common/types/organization.cljc @@ -19,11 +19,13 @@ [:logo-id {:optional true} [:maybe ::sm/uuid]] [:expired-license {:optional true} [:maybe :boolean]] [:sso-active {:optional true} [:maybe :boolean]] + [:restrictions-met {:optional true} [:maybe [:map-of :keyword :boolean]]] [:permissions {:optional true} [:maybe [:map [:create-teams {:optional true} [:maybe [:enum "any" "onlyMe"]]] [:delete-teams {:optional true} [:maybe [:enum "onlyMe" "onlyOwners"]]] [:move-teams {:optional true} [:maybe [:enum "always" "myOrganizations" "never"]]] + [:send-invitations {:optional true} [:maybe [:enum "ownersAndAdmins" "owners"]]] [:new-team-members {:optional true} [:maybe [:enum "anyone" "members"]]]]]]]) @@ -35,7 +37,7 @@ (def organization->team-keys "Organization field keys to include in the nested :organization map." - [:id :name :custom-photo :slug :avatar-bg-url :owner-id :expired-license :permissions :sso-active]) + [:id :name :custom-photo :slug :avatar-bg-url :owner-id :expired-license :permissions :sso-active :restrictions-met]) (defn apply-organization "Updates a team map with organization fields in a nested :organization map. @@ -85,9 +87,9 @@ :new-team-members "anyone"}) (defn- can-create-team? - [{:keys [is-organization-owner? permission-value]}] + [{:keys [is-organization-owner? permission-value restrictions-met?]}] (or is-organization-owner? - (= permission-value "any"))) + (and (= permission-value "any") (boolean restrictions-met?)))) (defn- can-delete-team? [{:keys [is-organization-owner? permission-value team-perms]}] @@ -109,28 +111,33 @@ 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?]}] + from Penpot, not even the organization owner. + + An attribute restriction (when present) narrows \"always\" and + \"myOrganizations\"; it never widens them and never affects \"never\"." + [{:keys [permission-value target-organization-same-owner? restrictions-met?]}] (cond (= permission-value "never") false (= permission-value "always") - true + (boolean restrictions-met?) (= permission-value "myOrganizations") - (true? target-organization-same-owner?) + (and (true? target-organization-same-owner?) (boolean restrictions-met?)) :else false)) (defn- can-invite-to-team? - [{:keys [permission-value team-perms]}] - (cond - (= permission-value "ownersAndAdmins") - (or (boolean (:is-owner team-perms)) - (boolean (:is-admin team-perms))) + [{:keys [permission-value team-perms restrictions-met?]}] + (and + (cond + (= permission-value "ownersAndAdmins") + (or (boolean (:is-owner team-perms)) + (boolean (:is-admin team-perms))) - (= permission-value "owners") - (boolean (:is-owner team-perms)) + (= permission-value "owners") + (boolean (:is-owner team-perms)) - :else false)) + :else false) + (boolean restrictions-met?))) (defn- can-add-anybody-to-team? [{:keys [permission-value]}] @@ -163,13 +170,18 @@ (get action-rules action) permissions (normalize-organization-permissions organization-perms) is-organization-owner? (owner? organization-perms profile-id) - permission-value (get permissions permission-key)] + permission-value (get permissions permission-key) + ;; Missing key means "met": an old Nitrate that does not send + ;; :restrictions-met keeps today's behaviour (fail-open on the key, + ;; the option rules above still fail-closed on the action). + restrictions-met? (get-in organization-perms [:restrictions-met permission-key] true)] (cond (nil? rule) false :else (boolean (check-fn {:is-organization-owner? is-organization-owner? :permission-value permission-value :team-perms team-perms - :target-organization-same-owner? target-organization-same-owner?}))))) + :target-organization-same-owner? target-organization-same-owner? + :restrictions-met? restrictions-met?}))))) (defn can-send-invitations? "Returns true when the user can send invitations to a team. @@ -178,8 +190,7 @@ [{:keys [organization profile-id team-permissions]}] (if (and (contains? flags/*current* :admin-console) organization) (allowed? :send-invitations - {:organization-perms {:owner-id (:owner-id organization) - :permissions (:permissions organization)} + {:organization-perms organization :profile-id profile-id :team-perms team-permissions}) (or (boolean (:is-owner team-permissions)) diff --git a/common/test/common_tests/types/organization_test.cljc b/common/test/common_tests/types/organization_test.cljc index e4907a26d7..1555e23207 100644 --- a/common/test/common_tests/types/organization_test.cljc +++ b/common/test/common_tests/types/organization_test.cljc @@ -6,6 +6,7 @@ (ns common-tests.types.organization-test (:require + [app.common.flags :as flags] [app.common.types.organization :as cto] [clojure.test :as t])) @@ -193,3 +194,160 @@ {:organization-perms invalid-organization :profile-id :member :team-perms {:is-owner true :is-admin true}}))))) + +;; --- Attribute restriction gate (:restrictions-met) --- + +(t/deftest create-team-owner-bypasses-restriction + ;; The organization owner keeps the create-teams bypass even when the + ;; attribute restriction is not met. + (let [restricted (assoc organization-perms :restrictions-met {:create-teams false})] + (t/is (true? (cto/allowed? :create-team + {:organization-perms restricted + :profile-id :owner + :team-perms {}}))))) + +(t/deftest create-team-restriction-narrows-any + (let [denied (assoc organization-perms :restrictions-met {:create-teams false}) + met (assoc organization-perms :restrictions-met {:create-teams true})] + ;; Non-owner with "any" but the restriction not met -> denied + (t/is (false? (cto/allowed? :create-team + {:organization-perms denied + :profile-id :member + :team-perms {}}))) + ;; Restriction met -> allowed + (t/is (true? (cto/allowed? :create-team + {:organization-perms met + :profile-id :member + :team-perms {}}))) + ;; Missing :restrictions-met key -> allowed (old Nitrate keeps behaviour) + (t/is (true? (cto/allowed? :create-team + {:organization-perms organization-perms + :profile-id :member + :team-perms {}}))))) + +(t/deftest create-team-onlyme-ignores-restriction + ;; "onlyMe" is not restrictable: the option itself denies a member even when + ;; a (bogus) restriction says met, and still allows the owner. + (let [only-me (assoc organization-perms + :permissions {:create-teams "onlyMe" :delete-teams "onlyOwners"} + :restrictions-met {:create-teams true})] + (t/is (false? (cto/allowed? :create-team + {:organization-perms only-me + :profile-id :member + :team-perms {}}))) + (t/is (true? (cto/allowed? :create-team + {:organization-perms only-me + :profile-id :owner + :team-perms {}}))))) + +(t/deftest move-team-restriction-narrows-always + (let [denied (assoc organization-perms + :permissions {:create-teams "any" :delete-teams "onlyOwners" :move-teams "always"} + :restrictions-met {:move-teams false}) + met (assoc organization-perms + :permissions {:create-teams "any" :delete-teams "onlyOwners" :move-teams "always"} + :restrictions-met {:move-teams true})] + (t/is (false? (cto/allowed? :move-team + {:organization-perms denied + :profile-id :member + :team-perms {}}))) + (t/is (true? (cto/allowed? :move-team + {:organization-perms met + :profile-id :member + :team-perms {}}))))) + +(t/deftest move-team-restriction-narrows-myorganizations + (let [denied (assoc organization-perms + :permissions {:create-teams "any" :delete-teams "onlyOwners" :move-teams "myOrganizations"} + :restrictions-met {:move-teams false}) + met (assoc organization-perms + :permissions {:create-teams "any" :delete-teams "onlyOwners" :move-teams "myOrganizations"} + :restrictions-met {:move-teams true})] + ;; Same owner but the restriction not met -> denied + (t/is (false? (cto/allowed? :move-team + {:organization-perms denied + :profile-id :member + :team-perms {} + :target-organization-same-owner? true}))) + ;; Same owner and restriction met -> allowed + (t/is (true? (cto/allowed? :move-team + {:organization-perms met + :profile-id :member + :team-perms {} + :target-organization-same-owner? true}))))) + +(t/deftest move-team-never-ignores-restriction + (let [never (assoc organization-perms + :permissions {:create-teams "any" :delete-teams "onlyOwners" :move-teams "never"} + :restrictions-met {:move-teams true})] + (t/is (false? (cto/allowed? :move-team + {:organization-perms never + :profile-id :member + :team-perms {}}))))) + +(t/deftest send-invitations-restriction-narrows + (let [denied (assoc organization-perms :restrictions-met {:send-invitations false}) + met (assoc organization-perms :restrictions-met {:send-invitations true})] + ;; Team admin, ownersAndAdmins, restriction not met -> denied + (t/is (false? (cto/allowed? :send-invitations + {:organization-perms denied + :profile-id :member + :team-perms {:is-owner false :is-admin true}}))) + ;; Restriction met -> allowed + (t/is (true? (cto/allowed? :send-invitations + {:organization-perms met + :profile-id :member + :team-perms {:is-owner false :is-admin true}}))))) + +(t/deftest send-invitations-owners-restriction-narrows + (let [denied (assoc organization-perms + :permissions {:create-teams "any" :delete-teams "onlyOwners" :send-invitations "owners"} + :restrictions-met {:send-invitations false})] + ;; Even a team owner is denied when the restriction is not met + (t/is (false? (cto/allowed? :send-invitations + {:organization-perms denied + :profile-id :member + :team-perms {:is-owner true :is-admin true}}))))) + +(t/deftest delete-team-unaffected-by-restriction + ;; deleteTeams is not restrictable: a stored restriction must not change it. + (let [restricted (assoc organization-perms :restrictions-met {:delete-teams false})] + (t/is (true? (cto/allowed? :delete-team + {:organization-perms restricted + :profile-id :member + :team-perms {:is-owner true :is-admin true}}))))) + +(t/deftest add-anybody-unaffected-by-restriction + (let [restricted (assoc organization-perms + :permissions {:create-teams "any" :delete-teams "onlyOwners" :new-team-members "anyone"} + :restrictions-met {:new-team-members false})] + (t/is (true? (cto/allowed? :add-anybody-to-team + {:organization-perms restricted + :profile-id :member + :team-perms {}}))))) + +(t/deftest can-send-invitations-respects-restriction + (let [denied {:owner-id :owner + :permissions {:send-invitations "ownersAndAdmins"} + :restrictions-met {:send-invitations false}} + met {:owner-id :owner + :permissions {:send-invitations "ownersAndAdmins"} + :restrictions-met {:send-invitations true}}] + (binding [flags/*current* #{:admin-console}] + (t/is (false? (cto/can-send-invitations? {:organization denied + :profile-id :member + :team-permissions {:is-owner false :is-admin true}}))) + (t/is (true? (cto/can-send-invitations? {:organization met + :profile-id :member + :team-permissions {:is-owner false :is-admin true}})))))) + +(t/deftest can-send-invitations-falls-back-without-flag + ;; Without the admin-console flag, team-level permissions decide and the + ;; restriction is never consulted. + (let [organization {:owner-id :owner + :permissions {:send-invitations "ownersAndAdmins"} + :restrictions-met {:send-invitations false}}] + (binding [flags/*current* #{}] + (t/is (true? (cto/can-send-invitations? {:organization organization + :profile-id :member + :team-permissions {:is-owner false :is-admin true}})))))) diff --git a/frontend/src/app/main/data/common.cljs b/frontend/src/app/main/data/common.cljs index 644f001332..c2a7de99af 100644 --- a/frontend/src/app/main/data/common.cljs +++ b/frontend/src/app/main/data/common.cljs @@ -18,6 +18,7 @@ [app.main.data.modal :as modal] [app.main.data.notifications :as ntf] [app.main.data.persistence :as-alias dps] + [app.main.data.team :as dtm] [app.main.repo :as rp] [app.main.router :as rt] [app.main.store :as st] @@ -250,7 +251,11 @@ Updates local team organization data and redirects to SSO when required. `team` and `notification` come from the server-published message (see backend app.rpc.notifications/notify-team-change); `notification` is a translation - key resolved here, dynamic by design." + key resolved here, dynamic by design. + + The message is broadcast to every connected user, so it cannot carry the + per-profile `:restrictions-met`: the last known value is kept and the teams + are fetched again to get the current profile's one." [{:keys [team notification]}] (ptk/reify ::handle-change-team-organization ptk/WatchEvent @@ -259,11 +264,14 @@ organization (:organization team) team-name (or (:name team) (dm/get-in state [:teams (:id team) :name])) - current-team? (= (:id team) current-team-id)] + current-team? (= (:id team) current-team-id) + known-team? (contains? (:teams state) (:id team))] (when (and (contains? cf/flags :admin-console) - current-team?) + (or current-team? known-team?)) (rx/concat - (when notification + (when known-team? + (rx/of (dtm/fetch-teams))) + (when (and current-team? notification) ;; Execution time translation strings (keys sent by the backend): ;; (tr "dashboard.team-belong-organization") ;; (tr "dashboard.team-no-longer-belong-organization") @@ -274,7 +282,7 @@ :type :toast :level :info :timeout nil}))) - (when (:id organization) + (when (and current-team? (:id organization)) (check-team-sso current-team-id)))))) ptk/UpdateEvent (update [_ state] @@ -284,8 +292,14 @@ organization (:organization team)] (d/update-in-when state [:teams team-id] (fn [team] - (cond-> (cto/apply-organization team organization) - team-name (assoc :name team-name))))) + (let [same-organization? (= (:id organization) + (dm/get-in team [:organization :id])) + organization (cond-> organization + same-organization? + (assoc :restrictions-met + (dm/get-in team [:organization :restrictions-met])))] + (cond-> (cto/apply-organization team organization) + team-name (assoc :name team-name)))))) state)))) (defn handle-organization-change-sso diff --git a/frontend/src/app/main/data/nitrate.cljs b/frontend/src/app/main/data/nitrate.cljs index 7c3efefb40..1fbbce503c 100644 --- a/frontend/src/app/main/data/nitrate.cljs +++ b/frontend/src/app/main/data/nitrate.cljs @@ -362,8 +362,7 @@ (let [source-organization (:organization team) can-move? (cto/allowed? :move-team - {:organization-perms {:owner-id (:owner-id source-organization) - :permissions (:permissions source-organization)} + {:organization-perms source-organization :profile-id profile-id :team-perms (:permissions team) :target-organization-same-owner? false})] @@ -503,10 +502,9 @@ (fn [teams] (let [all-organizations (map dt/team->organization (filter #(and (:is-default %) (:organization %)) teams)) - organizations (filter (fn [organization] - (let [perm (dm/get-in organization [:permissions :create-teams]) - is-own? (= profile-id (:owner-id organization))] - (or (= perm "any") is-own?))) all-organizations) + organizations (filter #(cto/allowed? :create-team + {:organization-perms % + :profile-id profile-id}) all-organizations) team (first (filter #(= (:id %) team-id) teams)) current-organization (:organization team) on-confirm (fn [organization-id] @@ -563,22 +561,17 @@ team (first (filter #(= (:id %) team-id) teams)) source-organization (:organization team) current-organization-id (:id source-organization) - move-perm (dm/get-in source-organization [:permissions :move-teams]) source-owner-id (:owner-id source-organization) + can-move? (fn [organization] + (cto/allowed? :move-team + {:organization-perms source-organization + :profile-id profile-id + :target-organization-same-owner? (= source-owner-id (:owner-id organization))})) can-create? (fn [organization] - (let [perm (dm/get-in organization [:permissions :create-teams]) - is-own? (= profile-id (:owner-id organization))] - (or (= perm "any") is-own?))) - organizations-by-move (case move-perm - "never" - [] - - "myOrganizations" - (filter #(= source-owner-id (:owner-id %)) all-organizations) - - ;; Default to always-allowed behavior. - all-organizations) - organizations (filter can-create? organizations-by-move) + (cto/allowed? :create-team + {:organization-perms organization + :profile-id profile-id})) + organizations (filter #(and (can-move? %) (can-create? %)) all-organizations) selectable-organizations (remove #(= current-organization-id (:id %)) organizations) on-confirm (fn [organization-id] (st/emit! (add-team-to-organization {:team-id team-id diff --git a/frontend/src/app/main/data/team.cljs b/frontend/src/app/main/data/team.cljs index 081f41102d..e8afbfc994 100644 --- a/frontend/src/app/main/data/team.cljs +++ b/frontend/src/app/main/data/team.cljs @@ -124,8 +124,7 @@ in-organization? (and (contains? cf/flags :admin-console) organization) can-create? (if in-organization? (cto/allowed? :create-team - {:organization-perms {:owner-id (:owner-id organization) - :permissions (:permissions organization)} + {:organization-perms organization :profile-id profile-id :team-perms (:permissions team)}) true)] @@ -150,8 +149,7 @@ in-organization? (and (contains? cf/flags :admin-console) organization) can-delete? (if in-organization? (cto/allowed? :delete-team - {:organization-perms {:owner-id (:owner-id organization) - :permissions (:permissions organization)} + {:organization-perms organization :profile-id profile-id :team-perms (:permissions team)}) (boolean (dm/get-in team [:permissions :is-owner]))) diff --git a/frontend/src/app/main/ui/dashboard/team.cljs b/frontend/src/app/main/ui/dashboard/team.cljs index eb7b2f9798..5629337572 100644 --- a/frontend/src/app/main/ui/dashboard/team.cljs +++ b/frontend/src/app/main/ui/dashboard/team.cljs @@ -10,6 +10,7 @@ [app.common.data :as d] [app.common.data.macros :as dm] [app.common.schema :as sm] + [app.common.types.organization :as cto] [app.config :as cfg] [app.main.data.common :as dcm] [app.main.data.event :as ev] @@ -1595,10 +1596,9 @@ ;; Filter to organizations where user is allowed to create/add teams organizations (mf/with-memo [all-organizations profile-id] (->> all-organizations - (filter (fn [organization] - (let [perm (get-in organization [:permissions :create-teams]) - is-owner? (= profile-id (:owner-id organization))] - (or (= perm "any") is-owner?)))))) + (filter #(cto/allowed? :create-team + {:organization-perms % + :profile-id profile-id})))) ;; Keep parity with UX requirement: hide only when user belongs to one organization. can-change-organization? (mf/with-memo [all-organizations] diff --git a/frontend/test/frontend_tests/data/dashboard_test.cljs b/frontend/test/frontend_tests/data/dashboard_test.cljs index d2589d21c2..8bd965eca5 100644 --- a/frontend/test/frontend_tests/data/dashboard_test.cljs +++ b/frontend/test/frontend_tests/data/dashboard_test.cljs @@ -12,6 +12,7 @@ [app.main.data.dashboard :as dd] [app.main.data.notifications :as ntf] [app.main.data.persistence :as dps] + [app.main.data.team :as dtm] [app.main.data.websocket :as dws] [app.main.repo :as rp] [app.main.router :as rt] @@ -48,7 +49,8 @@ rt/get-current-href (mock/stub (constantly current-url))} (await (async/observe (ptk/watch event state nil) :on-next (fn [ev] - (t/is (= ::ntf/show (ptk/type ev))))))) + (t/is (contains? #{::ntf/show ::dtm/fetch-teams} + (ptk/type ev))))))) (.then (fn [_] @tr-calls))))) (t/deftest ^:async team-org-change-toast-names-team-when-joining-org (let [tr-calls (await (tr-args-for-team-org-toast "dashboard.team-belong-organization" @@ -63,6 +65,38 @@ (t/is (= [["dashboard.team-no-longer-belong-organization" ["Team-Apple" "OrgA"]]] tr-calls)))) +(t/deftest ^:async team-org-change-keeps-restrictions-met-of-the-same-organization + (let [team-id (uuid/next) + organization-id (uuid/next) + state {:teams {team-id {:id team-id + :organization {:id organization-id + :name "Acme" + :restrictions-met {:send-invitations false}}}}} + event (dcm/handle-change-team-organization + {:team {:id team-id + :organization {:id organization-id + :name "Acme 2" + :permissions {:send-invitations "owners"}}} + :notification nil})] + (await (mock/with-mocks* + {cf/flags (conj cf/flags :admin-console)} + (let [organization (get-in (ptk/update event state) [:teams team-id :organization])] + (t/is (= "Acme 2" (:name organization))) + (t/is (= {:send-invitations false} (:restrictions-met organization)))))))) + +(t/deftest ^:async team-org-change-refetches-teams-for-the-current-profile + (let [team-id (uuid/next) + state {:teams {team-id {:id team-id}}} + event (dcm/handle-change-team-organization + {:team {:id team-id :organization {:id (uuid/next) :name "Acme"}} + :notification nil}) + types (atom [])] + (await (mock/with-mocks* + {cf/flags (conj cf/flags :admin-console)} + (await (async/observe (ptk/watch event state nil) + :on-next #(swap! types conj (ptk/type %)))))) + (t/is (= [::dtm/fetch-teams] @types)))) + (t/deftest ^:async duplicated-project-has-a-file-count-and-is-unpinned (let [id (uuid/next) copy-id (uuid/next)