diff --git a/.serena/memories/backend/auth-permissions-product-domains.md b/.serena/memories/backend/auth-permissions-product-domains.md index 3cf678acda..c7f2bd6ae3 100644 --- a/.serena/memories/backend/auth-permissions-product-domains.md +++ b/.serena/memories/backend/auth-permissions-product-domains.md @@ -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:` 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:` 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 diff --git a/backend/resources/admin/js/components/token-perms.js b/backend/resources/admin/js/components/token-perms.js index c8d51802d8..4ecc64b135 100644 --- a/backend/resources/admin/js/components/token-perms.js +++ b/backend/resources/admin/js/components/token-perms.js @@ -23,7 +23,6 @@ const RESOURCE_LABELS = new Map([ ["jobs", "Jobs"], ["error", "Error reports"], ["virtual", "System"], - ["token", "Tokens"], ]); const RESOURCE_ORDER = [ diff --git a/backend/resources/admin/js/pages/tokens.js b/backend/resources/admin/js/pages/tokens.js index d0fbc781fd..cd1dfacf92 100644 --- a/backend/resources/admin/js/pages/tokens.js +++ b/backend/resources/admin/js/pages/tokens.js @@ -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 { diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index 509bc925e4..1deaff8044 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -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)) diff --git a/backend/src/app/rpc/admin/perms.clj b/backend/src/app/rpc/admin/perms.clj index 450ce4c9a8..d2c2007cc3 100644 --- a/backend/src/app/rpc/admin/perms.clj +++ b/backend/src/app/rpc/admin/perms.clj @@ -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))) diff --git a/backend/src/app/rpc/admin/tokens.clj b/backend/src/app/rpc/admin/tokens.clj index 33a9ab3d6b..40aa96521f 100644 --- a/backend/src/app/rpc/admin/tokens.clj +++ b/backend/src/app/rpc/admin/tokens.clj @@ -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 diff --git a/backend/test/backend_tests/rpc_admin_perms_test.clj b/backend/test/backend_tests/rpc_admin_perms_test.clj index 2f32d16f3a..c8b3499203 100644 --- a/backend/test/backend_tests/rpc_admin_perms_test.clj +++ b/backend/test/backend_tests/rpc_admin_perms_test.clj @@ -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 ;; ---------------------------------------------------------------- diff --git a/backend/test/backend_tests/rpc_admin_tokens_test.clj b/backend/test/backend_tests/rpc_admin_tokens_test.clj index 9e10846767..9e1cbae06e 100644 --- a/backend/test/backend_tests/rpc_admin_tokens_test.clj +++ b/backend/test/backend_tests/rpc_admin_tokens_test.clj @@ -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 ;; ----------------------------------------------------------------