diff --git a/.serena/memories/audit-log.md b/.serena/memories/audit-log.md index a9766ec80b..03cfad7915 100644 --- a/.serena/memories/audit-log.md +++ b/.serena/memories/audit-log.md @@ -182,7 +182,7 @@ Who uses it: Failure policy: every send is best-effort. A failure never rolls back the business action; it is only recorded for debugging. -Backend side (`backend/src/app/rpc/management/nitrate.clj` `::push-audit-events`): `::audit/skip`, `::rpc/auth false` (trust comes from the shared key, not from a user session). It builds context with `audit/prepare-context-from-request` + `request-id`, takes the given `profile-id` as-is (no coercion here, must already be a uuid), sets `tracked-at = request-at`, defaults `type` to `"action"`, and calls `audit/submit` (which defaults `source` to `"backend"`). So these rows land as **`source = backend`**: they are audited like backend events because a trusted service set the author. +Backend side (`backend/src/app/rpc/management/nitrate.clj` `::push-audit-events`): `::audit/skip`, `::rpc/auth false` (trust comes from the shared key, not from a user session; only the `admin-console` key id is allowed). It builds context with `audit/prepare-context-from-request` + `request-id`, takes the given `profile-id` as-is (no coercion here, must already be a uuid), sets `tracked-at = request-at`, defaults `type` to `"action"`, and calls `audit/submit` (which defaults `source` to `"backend"`). So these rows land as **`source = backend`**: they are audited like backend events because a trusted service set the author. ### 7b. Browser channel (best-effort, like frontend events) @@ -301,7 +301,7 @@ So the full journey is: Penpot buffer → Nexus `audit_log` (via either archive Penpot → admin-console archiving lives in its own job (`:audit-log-consumer-admin-console`, consumer `2`, flag `:admin-console`, params closed empty): it drains its own pending queue and ingests allowlisted events through `nitrate/call :ingest-audit-log`, enriched as described above. What also exists (do not confuse with archiving): -- **Nitrate → Penpot**: `POST /api/management/methods/push-audit-events` (`backend/src/app/rpc/management/nitrate.clj`) lets the trusted Nitrate backend inject events into the Penpot audit log (no RPC auth, `::audit/skip`, request context + `submit`). Section 7a is the main user. +- **Nitrate → Penpot**: `POST /api/management/methods/push-audit-events` (`backend/src/app/rpc/management/nitrate.clj`) lets the trusted Nitrate backend inject events into the Penpot audit log (no RPC auth, `::audit/skip`, request context + `submit`; only the `admin-console` shared-key id can call it). Section 7a is the main user. - **Admin-console browser → collector**: usage events from the admin-console UI traveling through the normal public `push-audit-events` (section 7b). - **Penpot frontend Nitrate bits**: membership/subscription events (`frontend/src/app/main/data/nitrate.cljs`, `nitrate-audit/*`) through the same public endpoint. diff --git a/.serena/memories/backend/auth-permissions-product-domains.md b/.serena/memories/backend/auth-permissions-product-domains.md index d9ee64c171..c0667250f1 100644 --- a/.serena/memories/backend/auth-permissions-product-domains.md +++ b/.serena/memories/backend/auth-permissions-product-domains.md @@ -14,6 +14,7 @@ - Invitation tokens are verified through token issuers and only accepted when the token member id/email matches the authenticated profile; otherwise login proceeds without consuming the invitation. - When public registration is disabled, invitation-based password registration also requires a matching, non-expired `team_invitation` row; registration revalidates it under a row lock before creating the profile. - Access tokens (`app.http.access-token`): personal tokens have JWT issuer `access-token` and DB type other than `mcp`; MCP tokens have issuer `urn:penpot:mcp-token` (`actoken/mcp-token-issuer`) and DB type `mcp`. Authentication requires issuer and type to agree, and the claim `uid` to match the row's profile; other issuers and undecryptable tokens are treated as anonymous. +- Management RPC (`/api/management/methods/:name`) is shared-key only. The middleware authenticates ` `; the RPC wrapper `wrap-shared-key-authorization` authorizes the key id against the method's `::rpc/allowed-shared-keys` set. Absent metadata accepts any configured key; all current methods declare one (`exporter`/`media-processor` for `upload-tempfile`, `nexus` for file analysis, `exporter` for jobs, `admin-console` for Nitrate). A valid key outside the set answers 403 `:shared-key-not-allowed`; denied calls do not reach the audit wrapper. - Personal tokens need flag `:access-tokens`. MCP tokens need `:mcp` or `:access-tokens`, and only on routes whose `actoken/authz` gets `::actoken/allow-mcp true` (the RPC routes); elsewhere (e.g. assets) they are anonymous. - On RPC, an MCP token reaches only commands with `::rpc/token-type "mcp"` (`wrap-authentication` in `app.rpc`); every other command answers 403 `:token-auth-required`. A command with that metadata also rejects sessions and personal tokens. Currently only `send-mcp-feedback`. Blocking a profile invalidates sessions, not tokens, so such commands check `:is-blocked`/`:is-active` themselves. - HTTP/session parsing details such as cookie/header precedence, JWT session token versions, and SameSite behavior are in `mem:backend/subtleties`. diff --git a/.serena/memories/backend/subtleties.md b/.serena/memories/backend/subtleties.md index 922c20bcb7..8bfef97b6e 100644 --- a/.serena/memories/backend/subtleties.md +++ b/.serena/memories/backend/subtleties.md @@ -56,7 +56,7 @@ - Request body parsing is mostly POST-oriented and supports Transit JSON plus plain JSON. Plain JSON request keys are kebab-decoded before being merged into `:params`. A body the parser cannot read is a client error, classified at the parse site and never by exception class (`clojure.data.json` signals its syntax errors with a bare `java.lang.Exception`): 400 `:validation`/`:malformed-json`, one `warn` line (`malformed request body`) carrying path, peer address and content-type instead of a stack trace, and `penpot_http_malformed_requests_total`. `RequestTooBigException` keeps its 413. - Response formatting negotiates with `Accept` or `_fmt=json`. Transit is the default for collection/boolean bodies; JSON encoding has special pointer-map handling. - Auth prefers the session cookie token before the `Authorization` header. Headers may be `Token` or `Bearer`. Only `kid=1`/`ver=1` tokens are decoded as session tokens; anything else is left unauthenticated (legacy v1 tokens were removed). -- Shared-key auth requires `x-shared-key` as ` ` and stores the lowercased key id on the request. If no shared keys are configured it always rejects. +- Shared-key auth requires `x-shared-key` as ` ` and stores the lowercased key id on the request. If no shared keys are configured it always rejects. Management methods declare the key ids they accept with `::rpc/allowed-shared-keys`; a valid key outside that set answers 403 `:shared-key-not-allowed`, and a method without the metadata accepts any configured key. - Session management uses DB storage unless the DB pool is read-only, then falls back to the in-memory manager. Sessions use only the v2 UUID model (`http_session_v2`); legacy string ids were removed. - Session cookies are renewed when `modified-at` is older than the 6h renewal interval. SameSite is `none` for CORS, otherwise strict/lax based on config. Session lifetime config and GC: `mem:backend/session-expiration`. diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index 8d922822c7..f9c9d7eabf 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -366,6 +366,28 @@ (wrap-params-validation cfg $ mdata) (wrap-authentication cfg $ mdata))) +(defn- wrap-shared-key-authorization + "Authorizes a shared-key caller against the method's `::allowed-shared-keys`. + + A management method declares the shared-key ids allowed to call it with + `::allowed-shared-keys`. When the metadata is absent any configured shared key is + accepted, preserving the previous behavior. When it is present and empty no + key is accepted." + [_ f mdata] + (if (contains? mdata ::allowed-shared-keys) + (let [allowed-keys (::allowed-shared-keys mdata)] + (fn [cfg params] + (if (contains? allowed-keys (::auth-key-id params)) + (f cfg params) + (do + (l/wrn :hint "shared key is not allowed for this method" + :handler-name (::handler-name params) + :key-id (::auth-key-id params)) + (ex/raise :type :authorization + :code :shared-key-not-allowed + :hint "shared key is not allowed for this method"))))) + f)) + (defn- wrap-management [cfg f mdata] (as-> f $ @@ -376,7 +398,10 @@ (wrap-audit cfg $ mdata) (wrap-nitrate-sso cfg $ mdata) (wrap-params-validation cfg $ mdata) - (wrap-authentication cfg $ mdata))) + (wrap-authentication cfg $ mdata) + ;; Outermost on purpose: a denied key never opens a transaction, + ;; validates params or runs the method body. + (wrap-shared-key-authorization cfg $ mdata))) (defn- process-method [cfg wrap-fn [f mdata]] diff --git a/backend/src/app/rpc/management/exporter.clj b/backend/src/app/rpc/management/exporter.clj index 332fe23984..1e37e5943f 100644 --- a/backend/src/app/rpc/management/exporter.clj +++ b/backend/src/app/rpc/management/exporter.clj @@ -28,11 +28,16 @@ schema:upload-tempfile-result [:map {:title "upload-templfile-result"}]) +(def ^:private allowed-shared-key-ids + "The exporter uploads its artifacts; the media worker uploads its results." + #{:exporter :media-processor}) + (sv/defmethod ::upload-tempfile {::doc/added "2.12" ::sm/params schema:upload-tempfile-params ::rpc/multipart-only-params [:content] - ::sm/result schema:upload-tempfile-result} + ::sm/result schema:upload-tempfile-result + ::rpc/allowed-shared-keys allowed-shared-key-ids} [cfg {:keys [::rpc/profile-id content]}] (media.v/validate-media-type! content cm/tempfile-types) (let [storage (sto/resolve cfg) diff --git a/backend/src/app/rpc/management/files.clj b/backend/src/app/rpc/management/files.clj index d947ec7549..377247b56c 100644 --- a/backend/src/app/rpc/management/files.clj +++ b/backend/src/app/rpc/management/files.clj @@ -28,6 +28,10 @@ (def projection-version 1) +(def ^:private allowed-shared-key-ids + "Only the nexus file-security pipeline reads file projections." + #{:nexus}) + ;; Bounds for the data that crosses to nexus on every sweep. The ;; projection stays small by construction; anything past the caps sets ;; `:truncated?` instead of growing the payload. @@ -220,7 +224,8 @@ {::doc/added "2.20" ::sm/params schema:get-file-for-analysis-params ::sm/result schema:get-file-for-analysis-result - ::rpc/auth false} + ::rpc/auth false + ::rpc/allowed-shared-keys allowed-shared-key-ids} [cfg {:keys [file-id]}] ;; `realize?` loads the whole object tree eagerly: the projection ;; walks every shape, so lazy pointers (no `load-fn` bound here) diff --git a/backend/src/app/rpc/management/jobs.clj b/backend/src/app/rpc/management/jobs.clj index f5fc250c96..3fd79ff3d0 100644 --- a/backend/src/app/rpc/management/jobs.clj +++ b/backend/src/app/rpc/management/jobs.clj @@ -10,11 +10,12 @@ the ledger. The methods are family-agnostic (they operate on any row of the `job` table) and use the plain JSON schemas of `app.jobs`. - Auth: every method declares ::rpc/auth false because these endpoints - do not require a profile. Actual authentication is enforced by the - management route resolver (resolve-management-methods in app.rpc), - which mandates a valid shared-key before dispatching to these methods. - If the route resolver changes, these methods must be updated accordingly." + Auth: these endpoints take no profile, so every method declares + ::rpc/auth false. The route applies mw/shared-key-auth + (app.http.middleware) to authenticate the ` ` header, and + wrap-management applies wrap-shared-key-authorization (app.rpc) to + check the key id against ::rpc/allowed-shared-keys. If the route + middleware or wrap-management changes, revisit these methods." (:require [app.common.exceptions :as ex] [app.common.schema :as sm] @@ -24,6 +25,10 @@ [app.rpc.doc :as doc] [app.util.services :as sv])) +(def ^:private allowed-shared-key-ids + "Only the exporter worker claims and settles durable jobs today." + #{:exporter}) + ;; ---- RPC METHOD: CLAIM-JOB (def ^:private schema:claim-job-params @@ -42,7 +47,8 @@ {::doc/added "2.20" ::sm/params schema:claim-job-params ::sm/result schema:claim-job-result - ::rpc/auth false} ;; shared-key enforced by route resolver + ::rpc/auth false + ::rpc/allowed-shared-keys allowed-shared-key-ids} [cfg {:keys [job-id scheduled-at]}] (let [row (jobs/get-job cfg job-id)] ;; Race condition: if the job is claimed between get-job and claim! by @@ -74,7 +80,8 @@ {::doc/added "2.20" ::sm/params schema:report-job-progress-params ::sm/result schema:report-job-progress-result - ::rpc/auth false} ;; shared-key enforced by route resolver + ::rpc/auth false + ::rpc/allowed-shared-keys allowed-shared-key-ids} [cfg {:keys [job-id progress]}] ;; A lost race (row already terminal) reports :skip so the worker ;; stops retrying a report that can never land, mirroring claim-job. @@ -130,7 +137,8 @@ {::doc/added "2.20" ::sm/params schema:complete-job-params ::sm/result schema:complete-job-result - ::rpc/auth false} ;; shared-key enforced by route resolver + ::rpc/auth false + ::rpc/allowed-shared-keys allowed-shared-key-ids} [cfg {:keys [job-id result resource-id]}] (if (pos? (jobs/complete cfg :job-id job-id @@ -156,7 +164,8 @@ {::doc/added "2.20" ::sm/params schema:fail-job-params ::sm/result schema:fail-job-result - ::rpc/auth false} ;; shared-key enforced by route resolver + ::rpc/auth false + ::rpc/allowed-shared-keys allowed-shared-key-ids} [cfg {:keys [job-id error]}] (if (pos? (jobs/fail cfg job-id error)) {:action :run} diff --git a/backend/src/app/rpc/management/nitrate.clj b/backend/src/app/rpc/management/nitrate.clj index 20cd899ae7..13f8e01817 100644 --- a/backend/src/app/rpc/management/nitrate.clj +++ b/backend/src/app/rpc/management/nitrate.clj @@ -46,6 +46,10 @@ [cuerdas.core :as str])) +(def ^:private allowed-shared-key-ids + "The Nitrate admin-console is the only caller of this API." + #{:admin-console}) + (defn- profile-to-map [profile] {:id (:id profile) :name (:fullname profile) @@ -58,6 +62,7 @@ (sv/defmethod ::authenticate "Authenticate the current user" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map] ::sm/result schema:profile ::nitrate/sso false} @@ -105,6 +110,7 @@ (sv/defmethod ::get-penpot-version "Get the current Penpot version" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map] ::sm/result schema:get-penpot-version-result ::rpc/auth false} @@ -124,6 +130,7 @@ (sv/defmethod ::get-teams "List teams for which current user is owner" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map] ::sm/result schema:get-teams-result ::nitrate/sso false} @@ -135,6 +142,7 @@ (sv/defmethod ::get-member-teams "List all active teams where the current user is a member" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map] ::sm/result schema:get-member-teams-result ::nitrate/sso false} @@ -151,6 +159,7 @@ (sv/defmethod ::update-profile-theme "Update the current user's theme" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:update-profile-theme ::db/transaction true ::nitrate/sso false} @@ -171,6 +180,7 @@ (sv/defmethod ::update-profile-props "Merge writable properties into the current user's profile" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:update-profile-props ::db/transaction true ::nitrate/sso false} @@ -193,6 +203,7 @@ Accepts an optional previous-id to mark the old logo for garbage collection when replacing an existing one." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:upload-organization-logo ::rpc/multipart-only-params [:content] ::sm/result schema:upload-organization-logo-result @@ -225,6 +236,7 @@ (sv/defmethod ::notify-team-change "Notify to Penpot a team change from nitrate" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:notify-team-change ::rpc/auth false} [cfg team] @@ -245,6 +257,7 @@ (sv/defmethod ::notify-user-added-to-organization "Notify to Penpot that an user has joined an organization from nitrate" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:notify-user-added-to-organization ::rpc/auth false} [cfg {:keys [profile-id organization-id]}] @@ -276,6 +289,7 @@ (sv/defmethod ::get-managed-profiles "List profiles that belong to teams for which current user is owner" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map] ::sm/result schema:managed-profile-result ::nitrate/sso false} @@ -316,6 +330,7 @@ (sv/defmethod ::get-teams-summary "Get summary information for a list of teams" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:get-teams-summary-params ::sm/result schema:get-teams-summary-result ::nitrate/sso false} @@ -437,6 +452,7 @@ RETURNING id, deleted_at;") "For a deleted organization, preserve organization teams and only prefix or delete imported Personal Projects before notifying connected users." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:notify-organization-deletion ::rpc/auth false} [cfg {:keys [organization-id]}] @@ -457,6 +473,7 @@ RETURNING id, deleted_at;") "For a given user, find all owned organizations and apply the deleted-organization transfer rules to their imported Personal Projects teams." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:notify-user-organizations-deletion ::nitrate/sso false} [cfg {:keys [profile-id]}] @@ -483,6 +500,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::get-profile-by-email "Get profile by email" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map [:email ::sm/email]] ::sm/result schema:profile ::nitrate/sso false} @@ -507,6 +525,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::get-profile-by-id "Get profile by email" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map [:id ::sm/uuid]] ::sm/result schema:profile ::nitrate/sso false} @@ -542,6 +561,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::get-organization-member-team-counts "Get the number of non-default teams each profile belongs to within a set of teams." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:get-organization-member-team-counts-params ::sm/result schema:get-organization-member-team-counts-result ::rpc/auth false} @@ -594,6 +614,7 @@ RETURNING id, deleted_at;") "Get the non-default teams a profile belongs to within a set of teams, with the profile's role in each one." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:get-organization-member-teams-params ::sm/result schema:get-organization-member-teams-result ::nitrate/sso false} @@ -631,6 +652,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::invite-to-organization "Invite to organization" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map [:email ::sm/email] [:organization cto/schema:organization-with-avatar]] @@ -663,6 +685,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::get-organization-invitations "Get valid invitations for an organization, returning at most one invitation per email." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:get-organization-invitations-params ::sm/result schema:get-organization-invitations-result ::nitrate/sso false} @@ -691,6 +714,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::delete-organization-invitations "Delete all invitations for one email in an organization scope (organization + organization teams)." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:delete-organization-invitations-params ::nitrate/sso false} [cfg {:keys [organization-id email]}] @@ -718,6 +742,7 @@ RETURNING id, deleted_at;") Called from Nitrate when an organization is about to be deleted, so users that click their invitation token hit the existing invalid-token landing page." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:delete-all-organization-invitations-params ::rpc/auth false} [cfg {:keys [organization-id]}] @@ -756,6 +781,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::remove-from-organization "Remove an user from an organization" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map [:profile-id ::sm/uuid] [:organization-id ::sm/uuid] @@ -801,6 +827,7 @@ RETURNING id, deleted_at;") "Get a summary of the teams that would be deleted, transferred, or exited if the user were removed from the organization" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map [:profile-id ::sm/uuid] [:organization-id ::sm/uuid] @@ -836,6 +863,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::send-renewal-email "Send an Enterprise subscription renewal notice email to a user." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:send-renewal-email-params ::rpc/auth false} [cfg {:keys [profile-id user-email user-name renewal-date estimated-amount organizations]}] @@ -912,6 +940,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::exists-organization-team-invitations-for-non-members "Return if there are any team invitations for emails that are not organization members." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:organization-team-invitations-for-non-members-params ::sm/result schema:exists-organization-team-invitations-for-non-members-result ::nitrate/sso false} @@ -922,6 +951,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::delete-organization-team-invitations-for-non-members "Delete team invitations for emails that are not organization members." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:organization-team-invitations-for-non-members-params ::db/transaction true ::nitrate/sso false} @@ -954,6 +984,7 @@ RETURNING id, deleted_at;") events)" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::audit/skip true ::sm/params schema:push-audit-events-params ::rpc/auth false} @@ -1061,6 +1092,7 @@ RETURNING id, deleted_at;") "Get detailed information for all non-deleted teams in an organization, including owner info and project/file/member counts." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:get-teams-detail-params ::sm/result schema:get-teams-detail-result ::nitrate/sso false} @@ -1091,6 +1123,7 @@ RETURNING id, deleted_at;") (customer-configured), so it is checked against the SSRF blocklist before any outbound request is attempted." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params cto/schema:nitrate-sso ::sm/result schema:check-organization-sso-result ::rpc/auth false} @@ -1104,6 +1137,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::notify-organization-sso-change "Nitrate notifies that an organization sso values have changed" {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map [:organization-id ::sm/uuid] [:updated-props ::sm/boolean] @@ -1163,6 +1197,7 @@ RETURNING id, deleted_at;") email allow-list. Requires the `admin-console-bulk-create-profiles` flag, disabled by default so it is only available on test environments." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params schema:bulk-create-profiles-params ::sm/result schema:bulk-create-profiles-result ::rpc/auth false} @@ -1197,6 +1232,7 @@ RETURNING id, deleted_at;") (sv/defmethod ::get-air-gapped "Returns whether this Penpot instance runs in air-gapped mode." {::doc/added "2.18" + ::rpc/allowed-shared-keys allowed-shared-key-ids ::sm/params [:map] ::sm/result schema:get-air-gapped-result ::rpc/auth false} diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 60dd537836..24c433b012 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -547,13 +547,38 @@ (method-fn params)))) (defn management-command! + "Invokes a management RPC method. + + Resolves the shared-key id from an explicit `::rpc/auth-key-id`, from + `:app.http/auth-key-id` on the data or on the supplied request, and falls + back to the first id the method declares. The fallback lets the existing + tests exercise the allowed path without repeating the key; a test that + checks a rejection declares the key explicitly, including `nil`." [{:keys [::type] :as data}] - (let [[_ method-fn] (get-in *system* [:app.rpc/management-methods type])] + (let [[mdata method-fn] (get-in *system* [:app.rpc/management-methods type])] (when-not method-fn (ex/raise :type :assertion :code :rpc-method-not-found :hint (str/ffmt "management rpc method '%' not found" (name type)))) - (let [params (prepare-rpc-params data)] + (let [request (-> data meta :app.http/request) + key-id (cond + (contains? data ::rpc/auth-key-id) + (::rpc/auth-key-id data) + + (contains? data :app.http/auth-key-id) + (:app.http/auth-key-id data) + + (and (map? request) + (contains? request :app.http/auth-key-id)) + (:app.http/auth-key-id request) + + :else + (first (::rpc/allowed-shared-keys mdata))) + data (if (or (contains? data ::rpc/auth-key-id) + (nil? key-id)) + data + (assoc data ::rpc/auth-key-id key-id)) + params (prepare-rpc-params data)] (try-on! (method-fn params))))) (def multipart-headers diff --git a/backend/test/backend_tests/rpc_file_security_test.clj b/backend/test/backend_tests/rpc_file_security_test.clj index 15ac56c53d..1ecaf3de7f 100644 --- a/backend/test/backend_tests/rpc_file_security_test.clj +++ b/backend/test/backend_tests/rpc_file_security_test.clj @@ -218,6 +218,14 @@ (t/is (some? (:error out))) (t/is (= :not-found (th/ex-type (:error out)))))) +(t/deftest get-file-for-analysis-rejects-a-key-it-does-not-declare + (let [out (th/management-command! {::th/type :get-file-for-analysis + ::rpc/auth-key-id :exporter + :file-id (uuid/random)})] + (t/is (some? (:error out))) + (t/is (= :authorization (th/ex-type (:error out)))) + (t/is (= :shared-key-not-allowed (th/ex-code (:error out)))))) + ;; --- Audit prop coverage (through the commands, into real events) ;; ;; NOTE: the test system neutralizes `audit/submit`, so no audit row diff --git a/backend/test/backend_tests/rpc_management_jobs_test.clj b/backend/test/backend_tests/rpc_management_jobs_test.clj index 9ff0882e68..c228289528 100644 --- a/backend/test/backend_tests/rpc_management_jobs_test.clj +++ b/backend/test/backend_tests/rpc_management_jobs_test.clj @@ -13,6 +13,7 @@ [app.db :as db] [app.jobs :as jobs] [app.metrics :as-alias mtx] + [app.rpc :as-alias rpc] [backend-tests.helpers :as th] [clojure.test :as t])) @@ -76,6 +77,12 @@ id]) (mapv #(jobs/decode-progress (:payload %))))) +(defn- count-job-events + [id] + (-> (th/db-exec! ["SELECT count(*) AS count FROM job_event WHERE job_id = ?" id]) + first + :count)) + (defn- mgmt [type params] (th/management-command! (assoc params ::th/type type))) @@ -134,6 +141,26 @@ (let [out (mgmt :claim-job {:job-id (uuid/next)})] (t/is (= :validation (th/ex-type (:error out)))))) +(t/deftest jobs-methods-reject-a-key-they-do-not-declare + (doseq [type [:claim-job :report-job-progress :complete-job :fail-job]] + (let [out (mgmt type {::rpc/auth-key-id :nexus})] + (t/is (= :authorization (th/ex-type (:error out))) (name type)) + (t/is (= :shared-key-not-allowed (th/ex-code (:error out))) (name type))))) + +(t/deftest complete-job-rejects-a-key-it-does-not-declare-without-side-effects + (let [job-id (mk-job {}) + before (get-row job-id) + events-before (count-job-events job-id) + out (mgmt :complete-job {::rpc/auth-key-id :nexus + :job-id job-id + :result {:x 1}}) + after (get-row job-id)] + (t/is (= :authorization (th/ex-type (:error out)))) + (t/is (= :shared-key-not-allowed (th/ex-code (:error out)))) + (t/is (= (:status before) (:status after))) + (t/is (nil? (:completed-at after))) + (t/is (= events-before (count-job-events job-id))))) + (t/deftest report-job-progress-appends-a-progress-event (let [job-id (mk-job {}) _ (jobs/claim (make-cfg) job-id (:scheduled-at (th/db-get :job {:id job-id} :id :scheduled-at))) diff --git a/backend/test/backend_tests/rpc_management_nitrate_test.clj b/backend/test/backend_tests/rpc_management_nitrate_test.clj index 2d411bf816..cdef65b340 100644 --- a/backend/test/backend_tests/rpc_management_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_management_nitrate_test.clj @@ -52,6 +52,13 @@ (t/is (= :authentication (th/ex-type (:error out)))) (t/is (= :authentication-required (th/ex-code (:error out)))))) +(t/deftest authenticate-rejects-a-key-it-does-not-declare + (let [out (th/management-command! {::th/type :authenticate + ::rpc/auth-key-id :exporter})] + (t/is (not (th/success? out))) + (t/is (= :authorization (th/ex-type (:error out)))) + (t/is (= :shared-key-not-allowed (th/ex-code (:error out)))))) + (t/deftest create-and-update-organization-invitations-audit-props (let [owner-id-ref (atom nil)] (with-mocks [email-mock {:target 'app.email/send :return nil} diff --git a/backend/test/backend_tests/rpc_management_shared_keys_test.clj b/backend/test/backend_tests/rpc_management_shared_keys_test.clj new file mode 100644 index 0000000000..ea78696293 --- /dev/null +++ b/backend/test/backend_tests/rpc_management_shared_keys_test.clj @@ -0,0 +1,49 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns backend-tests.rpc-management-shared-keys-test + "Covers the shared-key scope declared by every management RPC method." + (:require + [app.rpc :as-alias rpc] + [backend-tests.helpers :as th] + [clojure.test :as t])) + +(t/use-fixtures :once (t/compose-fixtures + (partial th/init-config [:enable-admin-console]) + th/init-system)) + +(def ^:private expected-shared-keys + {"app.rpc.management.exporter" #{:exporter :media-processor} + "app.rpc.management.files" #{:nexus} + "app.rpc.management.jobs" #{:exporter} + "app.rpc.management.nitrate" #{:admin-console}}) + +(t/deftest every-management-method-declares-its-shared-keys + (let [methods (get th/*system* :app.rpc/management-methods) + seen (into #{} (map (comp :ns first val)) methods)] + (t/is (seq methods)) + (t/is (= (set (keys expected-shared-keys)) seen) + "every management namespace must be scanned") + (doseq [[method-name [mdata _]] methods] + (let [ns-name (:ns mdata) + expected (get expected-shared-keys ns-name)] + (t/is (some? expected) + (str "unexpected management namespace for " method-name ": " ns-name)) + (t/is (= expected (::rpc/allowed-shared-keys mdata)) + (str "wrong ::rpc/allowed-shared-keys for " method-name " in " ns-name)))))) + +(t/deftest management-command-uses-the-key-from-the-request-metadata + (let [out (th/management-command! + (with-meta {::th/type :upload-tempfile} + {:app.http/request {:app.http/auth-key-id :nexus}}))] + (t/is (= :authorization (th/ex-type (:error out)))) + (t/is (= :shared-key-not-allowed (th/ex-code (:error out)))))) + +(t/deftest management-command-keeps-an-explicit-nil-key + (let [out (th/management-command! {::th/type :upload-tempfile + ::rpc/auth-key-id nil})] + (t/is (= :authorization (th/ex-type (:error out)))) + (t/is (= :shared-key-not-allowed (th/ex-code (:error out)))))) diff --git a/backend/test/backend_tests/rpc_management_test.clj b/backend/test/backend_tests/rpc_management_test.clj index a55dcf95a4..eec3aaabb9 100644 --- a/backend/test/backend_tests/rpc_management_test.clj +++ b/backend/test/backend_tests/rpc_management_test.clj @@ -82,6 +82,29 @@ (t/is (= :validation (th/ex-type (:error out)))) (t/is (= :media-type-not-allowed (th/ex-code (:error out)))))) +(t/deftest upload-tempfile-rejects-a-key-it-does-not-declare + (let [out (th/management-command! {::th/type :upload-tempfile + ::rpc/auth-key-id :nexus})] + (t/is (some? (:error out))) + (t/is (= :authorization (th/ex-type (:error out)))) + (t/is (= :shared-key-not-allowed (th/ex-code (:error out)))))) + +(t/deftest upload-tempfile-accepts-both-declared-keys + (let [profile (th/create-profile* 1 {:is-active true})] + (doseq [key-id [:exporter :media-processor]] + (let [path (fs/create-tempfile :dir "/tmp/penpot" + :prefix (str "test-upload-" (name key-id) "-")) + _ (io/write* path "content") + out (th/multipart-management-command! + {::th/type :upload-tempfile + ::rpc/auth-key-id key-id + ::rpc/profile-id (:id profile) + :content {:filename "export.png" + :path path + :mtype "image/png" + :size 7}})] + (t/is (nil? (:error out)) (name key-id)))))) + (t/deftest duplicate-file (let [storage (-> (:app.storage/storage th/*system*) (configure-storage-backend)) diff --git a/backend/test/backend_tests/rpc_test.clj b/backend/test/backend_tests/rpc_test.clj index a6e18d24f3..0be65dd983 100644 --- a/backend/test/backend_tests/rpc_test.clj +++ b/backend/test/backend_tests/rpc_test.clj @@ -7,6 +7,7 @@ (ns backend-tests.rpc-test (:require [app.common.exceptions :as ex] + [app.common.logging :as l] [app.common.schema :as sm] [app.common.time :as ct] [app.common.uuid :as uuid] @@ -923,6 +924,92 @@ (call-body (method {}) {:extras {::actoken/profile-id pid ::actoken/type "personal"}})))))) +;; --- SHARED KEY AUTHORIZATION + +(defn- call-shared-key-wrapper + [mdata params] + (let [wrapped (#'rpc/wrap-shared-key-authorization + nil (fn [_ params] params) mdata)] + (wrapped nil params))) + +(t/deftest shared-key-authorization-allows-declared-keys + (let [mdata {::rpc/allowed-shared-keys #{:exporter :media-processor}}] + (t/is (= :exporter + (::rpc/auth-key-id + (call-shared-key-wrapper mdata {::rpc/auth-key-id :exporter})))) + (t/is (= :media-processor + (::rpc/auth-key-id + (call-shared-key-wrapper mdata {::rpc/auth-key-id :media-processor})))))) + +(t/deftest shared-key-authorization-rejects-other-key + (let [ex (try (call-shared-key-wrapper {::rpc/allowed-shared-keys #{:exporter}} + {::rpc/auth-key-id :nexus}) + (catch Exception e e))] + (t/is (th/ex-of-type? ex :authorization)) + (t/is (th/ex-of-code? ex :shared-key-not-allowed)))) + +(t/deftest shared-key-authorization-rejects-missing-key + (let [ex (try (call-shared-key-wrapper {::rpc/allowed-shared-keys #{:exporter}} {}) + (catch Exception e e))] + (t/is (th/ex-of-type? ex :authorization)) + (t/is (th/ex-of-code? ex :shared-key-not-allowed)))) + +(t/deftest shared-key-authorization-without-metadata-allows-any-key + (t/is (= :nexus + (::rpc/auth-key-id + (call-shared-key-wrapper {} {::rpc/auth-key-id :nexus})))) + (t/is (nil? (::rpc/auth-key-id (call-shared-key-wrapper {} {}))))) + +(t/deftest shared-key-authorization-empty-set-denies-all + (let [ex (try (call-shared-key-wrapper {::rpc/allowed-shared-keys #{}} + {::rpc/auth-key-id :exporter}) + (catch Exception e e))] + (t/is (th/ex-of-type? ex :authorization)) + (t/is (th/ex-of-code? ex :shared-key-not-allowed)))) + +(t/deftest shared-key-authorization-logs-the-denied-key + (let [records (atom []) + watch (fn [_ _ _ record] (swap! records conj record))] + (add-watch l/log-record ::shared-key-denied watch) + (try + (try (call-shared-key-wrapper {::rpc/allowed-shared-keys #{:exporter}} + {::rpc/auth-key-id :nexus + ::rpc/handler-name "test-method"}) + (catch Exception _)) + (let [deadline (+ (System/currentTimeMillis) 2000) + logged? (fn [] + (some #(and (= :warn (::l/level %)) + (= "shared key is not allowed for this method" + (get (::l/props %) :hint)) + (= :nexus (get (::l/props %) :key-id)) + (= "test-method" (get (::l/props %) :handler-name))) + @records))] + (loop [] + (when-not (or (logged?) + (>= (System/currentTimeMillis) deadline)) + (Thread/sleep 10) + (recur))) + (t/is (some? (logged?)))) + (finally + (remove-watch l/log-record ::shared-key-denied))))) + +(t/deftest shared-key-authorization-is-applied-to-rpc-requests + (let [mdata {::rpc/allowed-shared-keys #{:exporter}} + wrapped (partial (#'rpc/wrap-shared-key-authorization + nil (fn [_ params] params) mdata) + nil) + handler (make-handler {:shared-key-test [mdata wrapped]})] + (t/is (= :exporter + (::rpc/auth-key-id + (call-body handler + {:path-params {:method-name "shared-key-test"} + :extras {::http/auth-key-id :exporter}})))) + (t/is (th/ex-of-code? + (call-ex handler + {:path-params {:method-name "shared-key-test"} + :extras {::http/auth-key-id :nexus}}) + :shared-key-not-allowed)))) + ;; --- PARAMS VALIDATION: SERVER AUTH CONTEXT VS CLIENT PARAMS (t/deftest client-qualified-keys-do-not-override-server-auth-context