github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-11-03 00:20:41 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Harden admin granular tokens per review findings

Browse Source 
Exclude the anonymous share method from the grantable vocabulary,
pin the token id in the audit context with a test, check the deleted
flag explicitly, reject blocked or inactive owners at issuance, list
orphan tokens with LEFT JOIN, and give the panel table its action
column. Anonymous methods keep bypassing the admin branches.

AI-assisted-by: muse-spark-1.3-contributor
This commit is contained in:
Andrey Antukh 2026-11-02 14:24:17 +00:00
parent 7704eef169
commit ce637b080d
8 changed files with 189 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.serena/memories/backend/auth-permissions-product-domains.md
View File

@ -25,7 +25,7 @@
- File/project role rows only count with a live team membership: the file/project permission queries require a matching `team_profile_rel` row, and leaving or removing a member deletes their team-scoped `file`/`project`/`team-project-profile-rel` rows (GHSA-v9r9-h77c-55m2).
- File/project/comment checks are implemented in the owning command namespaces, often via helpers imported from `files`, `teams`, or `projects`; do not bypass those helpers with direct DB lookups unless preserving their not-found semantics.
- Comment permission includes both logged-in state and the file/team comment policy. Shared viewer paths may pass `share-id`; preserve that path when changing comment queries.
- Admin authorization is not predicate-based: the `admin` RPC module derives an implicit `admin:<method>` permission per call in `wrap-authentication` (vocabulary in `app.rpc.admin.perms`, issuance in `app.rpc.admin.tokens`). `superuser` bypasses everything; granular access is token-only with a live owner. Unlike product checks, admin denials use explicit authorization codes (`:superuser-required`, `:missing-perms`, `:profile-blocked`, `:profile-inactive`).
- Admin authorization is not predicate-based: the `admin` RPC module derives an implicit `admin:<method>` permission per call in `wrap-authentication` (vocabulary in `app.rpc.admin.perms`, issuance in `app.rpc.admin.tokens`). `superuser` bypasses everything; granular access is token-only with a live owner. Unlike product checks, admin denials use explicit authorization codes (`:superuser-required`, `:missing-perms`, `:profile-blocked`, `:profile-inactive`). Calls authenticated with a token record `:access-token-id` in the audit context (read from the request by `prepare-context-from-request`), so two tokens of one owner stay distinguishable.
## Teams, projects, and invitations

1
backend/resources/admin/js/components/token-perms.js
View File

@ -23,7 +23,6 @@ const RESOURCE_LABELS = new Map([
["jobs", "Jobs"],
["error", "Error reports"],
["virtual", "System"],
["token", "Tokens"],
]);
const RESOURCE_ORDER = [

4
backend/resources/admin/js/pages/tokens.js
View File

@ -18,6 +18,7 @@ const COLUMNS = [
{ key: "permissions", label: "Permissions", class: "admin-cell-count" },
{ key: "created", label: "Created", class: "admin-cell-date" },
{ key: "expires", label: "Expires", class: "admin-cell-date" },
{ key: "actions", label: "", class: "admin-cell-status" },
];
function labeledInput(labelText, { placeholder = "", type = "text" } = {}) {
@ -215,7 +216,7 @@ export function tokensPage(root) {
}))
);
[...table.querySelector("tbody").children].forEach((rowEl, index) => {
const cell = document.createElement("td");
const cell = rowEl.querySelector('[data-key="actions"]');
const revoke = document.createElement("button");
revoke.className = "admin-button admin-button-ghost";
revoke.textContent = "Revoke";
@ -235,7 +236,6 @@ export function tokensPage(root) {
}
});
cell.appendChild(revoke);
rowEl.appendChild(cell);
});
listBox.appendChild(table);
} catch {

87
backend/src/app/rpc.clj
View File

@ -212,59 +212,54 @@
:hint "access token authentication required for this endpoint")
;; Admin methods without an explicit "superuser" requirement
;; answer to their implicit perm. Superuser bypasses it; a
;; session without superuser keeps the historical rejection;
;; a token needs the implicit perm (plus any declared scope).
;; answer to their implicit perm: superuser bypasses it, a
;; session without superuser keeps the historical rejection,
;; and a token needs the implicit perm (plus any declared
;; scope) behind a live owner.
(and admin-method?
(not superuser-required)
required-auth?
(auth/superuser-allowed? cfg profile-id token-perms))
(f cfg params)
required-auth?)
(cond
(auth/superuser-allowed? cfg profile-id token-perms)
(f cfg params)
(and admin-method?
(not superuser-required)
required-auth?
(not= auth-type :token))
(ex/raise :type :authorization
:code :superuser-required
:hint "superuser required for this endpoint")
(not= auth-type :token)
(ex/raise :type :authorization
:code :superuser-required
:hint "superuser required for this endpoint")
(and admin-method?
(not superuser-required)
required-auth?
(not (set/subset? (conj scope-perms implicit-perm) token-perms)))
(ex/raise :type :authorization
:code :missing-perms
:hint "missing required permissions"
:required (conj scope-perms implicit-perm))
(not (set/subset? (conj scope-perms implicit-perm) token-perms))
(ex/raise :type :authorization
:code :missing-perms
:hint "missing required permissions"
:required (conj scope-perms implicit-perm))
;; The token owner behind an admin call must be able to act:
;; a live, active, unblocked profile. Soft-deleted rows are
;; hidden by the default `db/get*` filter, so a deleted owner
;; reads as nil. Registry superusers bypassed above and never
;; reach this branch.
(and admin-method?
(not superuser-required)
required-auth?
(= auth-type :token))
(let [row (db/get* cfg :profile {:id profile-id}
{:columns [:id :is-active :is-blocked :deleted-at]})]
(cond
(and (some? row)
(:is-active row)
(not (:is-blocked row)))
(f cfg params)
;; The token owner behind an admin call must be able to
;; act: a live, active, unblocked profile. The deleted-at
;; column is read and checked explicitly on top of the
;; default `db/get*` filter, so this branch fails closed
;; even if that default ever changes. Registry superusers
;; bypassed above and never reach this branch.
:else
(let [row (db/get* cfg :profile {:id profile-id}
{:columns [:id :is-active :is-blocked :deleted-at]})]
(cond
(and (some? row)
(nil? (:deleted-at row))
(:is-active row)
(not (:is-blocked row)))
(f cfg params)
(and (some? row)
(:is-blocked row))
(ex/raise :type :authorization
:code :profile-blocked
:hint "token owner profile is marked as blocked")
(and (some? row)
(:is-blocked row))
(ex/raise :type :authorization
:code :profile-blocked
:hint "token owner profile is marked as blocked")
:else
(ex/raise :type :authorization
:code :profile-inactive
:hint "token owner profile is not active")))
:else
(ex/raise :type :authorization
:code :profile-inactive
:hint "token owner profile is not active"))))
(and (seq scope-perms)
(not= auth-type :token))

7
backend/src/app/rpc/admin/perms.clj
View File

@ -38,12 +38,15 @@
(defn grantable-perms
"Every implicit admin permission a superuser may grant on a token.
Root-only methods (they declare `superuser`) are excluded: granting
their implicit perm would be useless, the method never consults it."
Root-only methods (they declare `superuser`) and anonymous methods
(they need no credential at all) are excluded: granting their
implicit perm would be useless, the method never consults it."
[]
(into #{}
(comp (filter (fn [[_ mdata]] (admin-method? mdata)))
(remove (fn [[_ mdata]]
(contains? (set (::rpc/perms mdata)) "superuser")))
(remove (fn [[_ mdata]]
(false? (::rpc/auth mdata true))))
(map (fn [[_ mdata]] (method-perm mdata))))
(apply sv/scan-ns admin-namespaces)))

23
backend/src/app/rpc/admin/tokens.clj
View File

@ -69,10 +69,23 @@
:code :unknown-permission
:hint "unknown admin permission"
:unknown unknown)))
(when-not (db/get* cfg :profile {:id owner-id} {:columns [:id :deleted-at]})
(ex/raise :type :not-found
:code :profile-not-found
:hint "owner profile not found"))
(let [owner (db/get* cfg :profile {:id owner-id}
{:columns [:id :is-active :is-blocked :deleted-at]})]
(cond
(or (nil? owner) (some? (:deleted-at owner)))
(ex/raise :type :not-found
:code :profile-not-found
:hint "owner profile not found")
(:is-blocked owner)
(ex/raise :type :authorization
:code :profile-blocked
:hint "owner profile is marked as blocked")
(not (:is-active owner))
(ex/raise :type :authorization
:code :profile-inactive
:hint "owner profile is not active")))
(quotes/check! cfg {::quotes/id ::quotes/access-tokens-per-profile
::quotes/profile-id owner-id})
(let [row (db/tx-run!
@ -95,7 +108,7 @@
t.created_at, t.updated_at, t.expires_at,
p.email AS owner_email
FROM access_token AS t
JOIN profile AS p ON p.id = t.profile_id
LEFT JOIN profile AS p ON p.id = t.profile_id
WHERE t.type = 'admin'")
(def ^:private schema:get-admin-permissions-result

14
backend/test/backend_tests/rpc_admin_perms_test.clj
View File

@ -22,6 +22,7 @@
[app.rpc.admin.perms :as perms]
[app.util.services :as sv]
[backend-tests.helpers :as th]
[clojure.set :as set]
[clojure.test :as t]))
(t/use-fixtures :once th/state-init)
@ -96,6 +97,19 @@
(t/is (not (contains? vocab "admin:create-admin-token"))
"root-only methods are out of the grantable set")))
(t/deftest grantable-vocabulary-excludes-anonymous-methods
(let [methods (apply sv/scan-ns perms/admin-namespaces)
anonymous-perms (into #{}
(comp (filter (fn [[_ mdata]]
(false? (::rpc/auth mdata true))))
(map (fn [[_ mdata]]
(perms/method-perm mdata))))
methods)]
(t/is (seq anonymous-perms)
"sanity: the fixture (an anonymous method) exists")
(t/is (empty? (set/intersection anonymous-perms (perms/grantable-perms)))
"granting a perm that changes nothing is a false delegation")))
;; ----------------------------------------------------------------
;; Implicit perm on the wire
;; ----------------------------------------------------------------

108
backend/test/backend_tests/rpc_admin_tokens_test.clj
View File

@ -13,8 +13,10 @@
[app.config :as cf]
[app.db :as db]
[app.http :as-alias http]
[app.loggers.audit :as audit]
[app.rpc :as rpc]
[app.rpc.admin.perms :as perms]
[app.rpc.commands.access-token :as access-token]
[app.util.services :as sv]
[backend-tests.helpers :as th]
[clojure.test :as t]))
@ -131,6 +133,29 @@
:profile-id (random-uuid)
:perms ["admin:get-teams"]}))))))
(defn- set-flags
[profile-id flags]
(db/update! th/*system* :profile flags {:id profile-id}))
(t/deftest issuing-for-blocked-owner-rejected
(let [admin (th/create-profile* 1 {:is-active true})
other (th/create-profile* 2 {:is-active true})
run (as-superuser admin)]
(set-flags (:id other) {:is-blocked true})
(t/is (= :profile-blocked
(caught-code #(run "create-admin-token" {:name "support"
:profile-id (:id other)
:perms ["admin:get-teams"]}))))))
(t/deftest issuing-for-inactive-owner-rejected
(let [admin (th/create-profile* 1 {:is-active true})
other (th/create-profile* 2)
run (as-superuser admin)]
(t/is (= :profile-inactive
(caught-code #(run "create-admin-token" {:name "support"
:profile-id (:id other)
:perms ["admin:get-teams"]}))))))
(t/deftest expiration-is-optional-but-respected
(let [profile (th/create-profile* 1 {:is-active true})
run (as-superuser profile)
@ -142,6 +167,17 @@
(t/is (nil? (:expires-at plain)))
(t/is (ct/is-after? (:expires-at dated) (ct/now)))))
(t/deftest expiration-as-panel-sends-it
;; The panel sends a raw duration string; it must survive params
;; validation and reach the row as an instant.
(let [profile (th/create-profile* 1 {:is-active true})
run (as-superuser profile)
created (run "create-admin-token" {:name "string-expiry"
:expiration "720h"
:perms ["admin:get-teams"]})]
(t/is (ct/is-after? (:expires-at created) (ct/now)))
(t/is (ct/is-after? (:expires-at (token-row (:id created))) (ct/now)))))
;; ----------------------------------------------------------------
;; Listing and revocation
;; ----------------------------------------------------------------
@ -157,6 +193,19 @@
(t/is (not (contains? entry :token)) "the secret never leaves creation")
(t/is (= #{"admin:get-teams"} (set (:perms entry))))))
(t/deftest listing-filters-by-owner
(let [admin (th/create-profile* 1 {:is-active true})
other (th/create-profile* 2 {:is-active true})
run (as-superuser admin)
mine (run "create-admin-token" {:name "mine"
:perms ["admin:get-teams"]})
theirs (run "create-admin-token" {:name "theirs"
:profile-id (:id other)
:perms ["admin:get-teams"]})
listed (run "get-admin-tokens" {:profile-id (:id other)})]
(t/is (= [(:id theirs)] (mapv :id listed)))
(t/is (not (contains? (set (mapv :id listed)) (:id mine))))))
(t/deftest revocation-deletes-the-row
(let [profile (th/create-profile* 1 {:is-active true})
run (as-superuser profile)
@ -167,12 +216,55 @@
(t/is (empty? (filter #(= (:id created) (:id %))
(run "get-admin-tokens" {}))))))
(t/deftest listing-shows-tokens-of-soft-deleted-owners
;; A soft-deleted owner vanishes from the users list, but their
;; tokens stay listed (with email) and revocable: revocation must
;; not depend on the owner being around.
(let [profile (th/create-profile* 1 {:is-active true})
run (as-superuser profile)
created (run "create-admin-token" {:name "support"
:perms ["admin:get-teams"]})]
(set-flags (:id profile) {:deleted-at (ct/now)})
(let [entry (some #(when (= (:id created) (:id %)) %)
(run "get-admin-tokens" {}))]
(t/is (some? entry))
(t/is (= (:email profile) (:owner-email entry))))
(run "revoke-admin-token" {:id (:id created)})
(t/is (nil? (token-row (:id created))))))
(t/deftest hard-deleting-owner-cascades-its-tokens
;; The `profile_id` foreign key carries `ON DELETE CASCADE`, so a
;; hard-deleted owner takes its tokens with it: nothing to list,
;; nothing to revoke. The `LEFT JOIN` in the listing only matters
;; for rows orphaned by manual cleanup outside the FK.
(let [profile (th/create-profile* 1 {:is-active true})
run (as-superuser profile)
created (run "create-admin-token" {:name "support"
:perms ["admin:get-teams"]})]
(th/db-force-delete :profile {:id (:id profile)})
(t/is (nil? (token-row (:id created))))
(t/is (empty? (run "get-admin-tokens" {})))
(t/is (= :access-token-not-found
(caught-code #(run "revoke-admin-token" {:id (:id created)}))))))
(t/deftest revoking-unknown-token-is-not-found
(let [profile (th/create-profile* 1 {:is-active true})
run (as-superuser profile)]
(t/is (= :access-token-not-found
(caught-code #(run "revoke-admin-token" {:id (random-uuid)}))))))
(t/deftest revoking-a-non-admin-token-is-not-found
;; The type scope protects personal tokens (and MCP ones, same
;; clause) from this endpoint: wrong id kind, no deletion.
(let [profile (th/create-profile* 1 {:is-active true})
run (as-superuser profile)
personal (db/tx-run! th/*system*
#(access-token/create-access-token
% (:id profile) "personal" nil nil))]
(t/is (= :access-token-not-found
(caught-code #(run "revoke-admin-token" {:id (:id personal)}))))
(t/is (some? (token-row (:id personal))) "untouched")))
;; ----------------------------------------------------------------
;; The permission vocabulary
;; ----------------------------------------------------------------
@ -196,6 +288,22 @@
(caught-code #(call-as #{} (:id profile) :token #{"admin:get-teams"}
"get-admin-permissions" {}))))))
;; ----------------------------------------------------------------
;; The audit trace carries the acting token
;; ----------------------------------------------------------------
(t/deftest audit-context-carries-the-acting-token-id
;; The review asked for this trace: it lives in the audit context,
;; built from the request that crossed the token middleware, so two
;; tokens of one owner stay distinguishable. No wrapper change.
(let [token-id (random-uuid)
context (audit/prepare-context-from-request
{:headers {}
:app.http.access-token/id token-id
:app.http.access-token/type "admin"})]
(t/is (= (str token-id) (:access-token-id context)))
(t/is (= "admin" (:access-token-type context)))))
;; ----------------------------------------------------------------
;; The issued token works where it should
;; ----------------------------------------------------------------