diff --git a/.serena/memories/backend/audit-log.md b/.serena/memories/backend/audit-log.md index fe401cef0c..ccd4153655 100644 --- a/.serena/memories/backend/audit-log.md +++ b/.serena/memories/backend/audit-log.md @@ -16,7 +16,10 @@ Penpot records what users do as events in the Postgres `audit_log` table. There - Most backend events need no manual code: `wrap-audit` in `app.rpc` runs after every RPC handler when `:webhooks`, `:audit-log` or `:telemetry` is on (unless the command sets `::audit/skip`) and builds the event via `prepare-rpc-event`. The event name defaults to the command name (prefixed with `-` outside `main`), props default to the request params, and timestamps come from the server request time. - Commands customize through result metadata (`rph/with-meta`): `::audit/replace-props` swaps the props wholesale (auth commands use `profile->props` so a register event carries the profile, not the password), `::audit/props` merges extras, `::audit/context`/`profile-id`/`name`/`type` override the defaults. `clean-props` always strips nils, qualified keys and `:session-id/:password/:old-password/:token/:client-secret` as a last line of defense. +- INVARIANT: the event's `profile-id` is the caller, resolved as `::audit/profile-id` metadata -> `::rpc/profile-id` -> `uuid/zero`. It is NEVER taken from the response. Results carry `:profile-id` keys that belong to someone else (`get-error-report` returns the report with its content merged, so the profile that owned the report; `verify-token` on a team invitation returns the inviter), and honoring them misattributes the action. Commands with `::rpc/auth false` (`login-*`, `register-profile`, `create-demo-profile`, `verify-token`, `prepare-register-profile`) have no caller to fall back on, so they MUST set `::audit/profile-id` in the result metadata or the event lands on `uuid/zero`. +- The `::audit/profile-id` override goes through `coerce-profile-id`: a string is parsed, anything that cannot become a uuid is discarded (with a warning) and the event falls back to the caller. Do not pass the value straight through: `schema:event` requires `::sm/uuid` and `submit*` swallows the validation error, so a non-uuid override silently loses the row instead of failing loudly. Hand-built events built with `event-from-rpc-params` + `submit` (clone-template, accept-team-invitation, `management/nitrate` push-audit-events) do NOT go through that coercion: they must supply a uuid. - `submit` is the normal entry point (fills defaults, validates `schema:event`, runs inside `tx-run!`, logs failures without failing the RPC). `insert` is the low-level one for CLI/helpers and the webhook subsystem: direct write, no webhook/telemetry fan-out, silent unless `:audit-log` is on. Boot emits `trigger/instance-start` from `setup/props` so every restart is visible in the log. +- `accept-organization-invitation` distinguishes its two flows with the props, not with an origin: `:organization-member-add-source` is `"direct-organization-invitation"` for a direct organization invitation and `"team-invitation"` for a team invitation that also adds to the organization, and `:belongs-to-team-on-add` repeats the same distinction as a boolean. Do not add a third prop for it. The event `context` has an `:event-origin`, but it belongs to the browser: `make-data-event` in `app.main.data.event` puts the UI origin there and it is on the frontend allowlist, while `safe-backend-context-keys` lists no `:event-origin`, so a backend event has nowhere to put one. Text props are dropped from the telemetry shadow rows by `filter-telemetry-props`; the full `audit_log` row and the Nexus archive still carry them. ## Consumers I: webhooks (`app.loggers.webhooks`) @@ -48,4 +51,4 @@ Penpot records what users do as events in the Postgres `audit_log` table. There ## Tests - `backend_tests/rpc-audit-test.clj` exercises the whole backend path (full-row insert, telemetry-only and dual-row modes, `submit*`, no-op without flags, `insert` gating, `prepare-rpc-event` resolution) with `with-redefs [cf/flags #{...}]` against real `audit_log` rows. -- Other RPC suites mock `app.loggers.audit/submit` (nil return; `helpers.clj` stubs it globally) and assert on `:call-args-list`; any new command that must (or must not) emit an event needs the same treatment. +- Other RPC suites mock `app.loggers.audit/submit` (nil return; `helpers.clj` stubs it globally) and assert on `:call-args-list`; any new command that must (or must not) emit an event needs the same treatment. `rpc-team-test/accept-organization-invitation-audit-event` is the reference for invitation events: three scenarios (direct org invitation, team invitation that adds to the org, already-a-member) asserting the emitted props, the add source and the team flag that tell the two flows apart, and the exact number of rows per event name. The count matters: asserting props against the first match with `first` passes even when a command emits the same name twice. Props have no schema on either ingest path, so these assertions are the only guard on the event contract. diff --git a/.serena/memories/common/data-model-change-checklist.md b/.serena/memories/common/data-model-change-checklist.md index 91f00d1e6e..f4540f9ea3 100644 --- a/.serena/memories/common/data-model-change-checklist.md +++ b/.serena/memories/common/data-model-change-checklist.md @@ -6,6 +6,7 @@ - Do not treat nil as a distinct persisted state from absence. Import/export and cleanup paths may filter nil attrs away. - Avoid Clojure-special naming in exported object attrs, especially boolean names ending in `?`; exported/imported data must survive JSON/SVG/Transit and external tooling. - Any new shape attr that participates in component sync must be listed in `app.common.types.component/sync-attrs` with the correct touched group. Attrs absent from `sync-attrs` are ignored by component synchronization. +- Shape `:svg-attrs` is the one map whose keys are camelCase React prop names (`fillRule`, not `fill-rule`), because the SVG import path runs them through `app.common.svg/attrs->props`. The `.penpot` reader (`app.common.json/read-kebab-key`) rewrites every json key of every entry to kebab-case, nested maps included, so any code that decodes a binary file must run `:svg-attrs` back through `attrs->props` or the names are lost. `:svg-defs` and the `:content` tree of `svg-raw` shapes keep their source names and round-trip on their own. ## Cross-module update checklist diff --git a/backend/src/app/binfile/cleaner.clj b/backend/src/app/binfile/cleaner.clj index c846a90149..a7fcd456db 100644 --- a/backend/src/app/binfile/cleaner.clj +++ b/backend/src/app/binfile/cleaner.clj @@ -9,6 +9,7 @@ for recently imported shapes." (:require [app.common.data :as d] + [app.common.svg :as csvg] [app.common.types.shape :as cts] [app.common.uuid :as uuid])) @@ -105,6 +106,15 @@ :reverse-column :column-reverse dir)))) +(defn- fix-svg-attrs + "The json reader of the binfile rewrites every key of every entry to + kebab-case, but `:svg-attrs` keys are react prop names and are stored + in camelCase. `attrs->props` is the transform the svg import path + already applies to them, so running it again restores the names; it + is idempotent, so shapes that come in correct are left untouched." + [shape] + (d/update-when shape :svg-attrs csvg/attrs->props)) + (defn clean-shape-post-decode "A shape procesor that expected to be executed after schema decoding process but before validation." @@ -112,7 +122,8 @@ (-> shape (fix-shape-shadow-color) (fix-root-shape) - (fix-legacy-flex-dir))) + (fix-legacy-flex-dir) + (fix-svg-attrs))) (defn- fix-container [container] diff --git a/backend/src/app/loggers/audit.clj b/backend/src/app/loggers/audit.clj index 152e0d7cb4..ab38799865 100644 --- a/backend/src/app/loggers/audit.clj +++ b/backend/src/app/loggers/audit.clj @@ -350,14 +350,44 @@ {})] (assoc params :context context))) +(defn- coerce-profile-id + "Normalize a hand-written `::audit/profile-id` override to a uuid. + + `schema:event` requires a uuid and `submit*` swallows the validation + error, so a value that is not a uuid loses the event instead of + failing loudly. Commands read the override from places that are not + typed by us (token claims, stringly-typed drivers), so a string has + to be accepted. Anything that cannot become a uuid is discarded, and + the event falls back to the caller, which is always a valid uuid." + [v] + (let [coerced (cond + ;; Fast path: the override comes straight from a `profile` + ;; row in almost every command, so it is already a uuid. + (uuid? v) + v + + (string? v) + (uuid/parse* v) + + :else + nil)] + (when (and (nil? coerced) (some? v)) + (l/error :hint "ignoring unusable ::audit/profile-id" + :profile-id v)) + + coerced)) + (defn prepare-rpc-event [cfg mdata params result] (let [resultm (meta result) request (-> params meta ::http/request) - profile-id (or (::profile-id resultm) - (some-> (:profile-id result) - (cond-> (string? (:profile-id result)) - uuid/parse*)) + ;; SECURITY: the event belongs to whoever made the request. The only + ;; sanctioned override is the `::audit/profile-id` metadata, set + ;; explicitly by the command. Never derive it from the response: + ;; results can carry a `:profile-id` that belongs to somebody else + ;; (the owner of an error report, the inviter of an invitation, ...) + ;; and that silently misattributes the action. + profile-id (or (coerce-profile-id (::profile-id resultm)) (::rpc/profile-id params) uuid/zero) diff --git a/backend/src/app/rpc/commands/verify_token.clj b/backend/src/app/rpc/commands/verify_token.clj index ac0875cb56..26746382c8 100644 --- a/backend/src/app/rpc/commands/verify_token.clj +++ b/backend/src/app/rpc/commands/verify_token.clj @@ -6,7 +6,6 @@ (ns app.rpc.commands.verify-token (:require - [app.common.data :as d] [app.common.exceptions :as ex] [app.common.schema :as sm] [app.common.time :as ct] @@ -99,7 +98,11 @@ (profile/strip-private-attrs) (update :props profile/filter-props) (with-nitrate-licence cfg))] - (assoc claims :profile profile))) + ;; The command is anonymous, so the audit event has no caller to fall + ;; back on and the profile must be declared here. The claims also carry + ;; a `:profile-id`, but the audit layer ignores the response on purpose. + (-> (assoc claims :profile profile) + (rph/with-meta {::audit/profile-id (:id profile)})))) ;; --- Team Invitation @@ -240,6 +243,12 @@ (not (:is-member membership))) (:organization-id membership)) + organization-add-source + (when organization-id-on-add + (if organization-id + "direct-organization-invitation" + "team-invitation")) + organization-member-count-before (when organization-id-on-add (count @@ -280,22 +289,34 @@ (-> (audit/event-from-rpc-params params) (assoc :profile-id created-by) (assoc :name "accept-team-invitation-from") - (assoc :props (assoc props - :profile-id (:id profile) - :email (:email profile))))))) + (assoc :props (-> props + (assoc :invited-by (:created-by invitation)) + (assoc :profile-id (:id profile)) + (assoc :profile-email (:email profile)) + (audit/clean-props))))))) (let [accepted-team-id (accept-invitation cfg claims invitation profile)] + ;; NOTE: the browser used to re-submit a copy of this event from + ;; the `:organization-invitation-audit` payload of this response, + ;; which wrote two rows per acceptance with two prop vocabularies + ;; for the same name, and tied the record to the browser finishing + ;; the flow. Everything the copy carried is computed above. (when organization-id-on-add - (audit/submit - cfg - (-> (audit/event-from-rpc-params params) - (assoc :name "accept-organization-invitation") - (assoc :props - (-> props - (assoc :organization-id organization-id-on-add - :user-id (:id profile) - :user-who-send-invitation (:created-by invitation)) - (audit/clean-props)))))) + (audit/submit cfg (-> (audit/event-from-rpc-params params) + (assoc :name "accept-organization-invitation") + (assoc :props + (-> props + (assoc :organization-id organization-id-on-add) + (assoc :invited-by (:created-by invitation)) + (assoc :profile-id (:id profile)) + (assoc :profile-email (:email profile)) + (assoc :organization-member-add-source + organization-add-source) + (assoc :belongs-to-team-on-add + (boolean team-id)) + (assoc :organization-member-count-before + organization-member-count-before) + (audit/clean-props)))))) (cond-> (assoc claims :state :created @@ -316,15 +337,11 @@ ;; accepted-team-id as :organization-team-id (:organization-id claims) (assoc :organization-team-id accepted-team-id) - - organization-id-on-add - (merge (d/without-nils - {:invitation-id (:id invitation) - :organization-member-count-before - organization-member-count-before})) - - (and organization-id-on-add team-id) - (assoc :organization-id organization-id-on-add)))))) + ;; The response carries the inviter's profile-id, so the + ;; audit event has to name the accepting profile explicitly + ;; or the invitation gets logged against the wrong user. + :always + (rph/with-meta {::audit/profile-id (:id profile)})))))) (do ;; If the user is not logged-in and the invitation has been canceled diff --git a/backend/test/backend_tests/binfile_test.clj b/backend/test/backend_tests/binfile_test.clj index 5b3afa8f93..72a17bac6f 100644 --- a/backend/test/backend_tests/binfile_test.clj +++ b/backend/test/backend_tests/binfile_test.clj @@ -29,6 +29,7 @@ [app.storage.tmp :as tmp] [backend-tests.helpers :as th] [backend-tests.storage-test :as stt] + [clojure.java.io :as jio] [clojure.test :as t] [cuerdas.core :as str] [datoteka.fs :as fs] @@ -201,6 +202,57 @@ ;; of failing with :child-not-found on the next update-file. (t/is (nil? (cfv/validate-file imported [])))))) +(defn- import-svg-attrs-asset + "Imports the `svg-attrs-camel-case.penpot` asset, a real penpot export + whose shapes carry `:svg-attrs` keys in camelCase (the format the + binary export writes), and returns the imported file." + [profile] + (let [input (-> "backend_tests/test_files/svg-attrs-camel-case.penpot" + io/resource + jio/file) + result (-> th/*system* + (assoc ::bfc/project-id (:default-project-id profile)) + (assoc ::bfc/profile-id (:id profile)) + (assoc ::bfc/input input) + (v3/import-files!))] + (bfc/get-file th/*system* (first result)))) + +(t/deftest import-binfile-v3-preserves-camel-case-svg-attrs + ;; The json reader used by the v3 import rewrites every key of every + ;; zip entry to kebab-case, and `:svg-attrs` is the one shape map + ;; whose keys are camelCase react prop names. A shape exported with + ;; `fillRule: "evenodd"` must not come back as `:fill-rule`, or the + ;; renderer falls back to the default fill rule and the shape is + ;; painted without its hole. + (let [profile (th/create-profile* 1) + file (import-svg-attrs-asset profile) + shape (get-in file [:data :pages-index + (uuid/uuid "fc80ab5f-1bf2-817c-8008-b5408f039100") + :objects + (uuid/uuid "ce3641bd-48c8-804c-8008-b54d35cc6f80")])] + + (t/is (= {:fillRule "evenodd"} + (:svg-attrs shape))))) + +(t/deftest import-binfile-v3-preserves-camel-case-svg-attrs-on-components + ;; Same guarantee for shapes stored inside a component: the v3 import + ;; cleans those in a different code path than page shapes. + (let [profile (th/create-profile* 1) + file (import-svg-attrs-asset profile) + shape (fn [component-id shape-id] + (-> file + (get-in [:data :components (uuid/uuid component-id) :objects]) + (get (uuid/uuid shape-id)) + :svg-attrs))] + + (t/is (= {:fillRule "evenodd"} + (shape "fae4bc76-0cc2-8057-8008-b540912cdf78" + "fae4bc76-0cc2-8057-8008-b540912774cb"))) + + (t/is (= {:fillRule "nonzero"} + (shape "fae4bc76-0cc2-8057-8008-b540912c917b" + "fae4bc76-0cc2-8057-8008-b540912774c8"))))) + (t/deftest export-binfile-v3 (let [profile (th/create-profile* 1) file (prepare-simple-file profile) diff --git a/backend/test/backend_tests/rpc_audit_test.clj b/backend/test/backend_tests/rpc_audit_test.clj index 51963bcd88..6cd08060af 100644 --- a/backend/test/backend_tests/rpc_audit_test.clj +++ b/backend/test/backend_tests/rpc_audit_test.clj @@ -550,46 +550,83 @@ (t/is (= {} (:props row))) (t/is (= {} (:context row)))))) -;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -;; PREPARE-RPC-EVENT PROFILE-ID CONVERSION -;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; PREPARE-RPC-EVENT PROFILE-ID RESOLUTION +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -(t/deftest prepare-rpc-event-converts-string-profile-id-to-uuid - ;; When result contains a string :profile-id (e.g. from error reports), - ;; prepare-rpc-event must convert it to a UUID for audit schema compliance. - (let [prof (th/create-profile* 1 {:is-active true}) - string-pid "33601240-a00b-11ea-ba1b-c554cc60e361" - expected #uuid "33601240-a00b-11ea-ba1b-c554cc60e361" - mdata {::sv/name "test-cmd"} - params {::rpc/profile-id (:id prof) - ::rpc/request-id (uuid/next) - ::rpc/request-at (ct/now)} - mock-req (reify - yetti.request/IRequest - (get-header [_ _] nil) - (remote-addr [_] "127.0.0.1")) - params (with-meta params {:app.http/request mock-req}) - result {:profile-id string-pid :some-data "value"} - event (audit/prepare-rpc-event th/*system* mdata params result)] - ;; profile-id must be a UUID, not a string - (t/is (uuid? (:profile-id event))) - (t/is (= expected (:profile-id event))))) - -(t/deftest prepare-rpc-event-handles-invalid-string-profile-id - ;; When result contains an invalid string :profile-id, it should fall back - ;; to the RPC params profile-id (which is always a valid UUID). - (let [prof (th/create-profile* 1 {:is-active true}) - mdata {::sv/name "test-cmd"} - params {::rpc/profile-id (:id prof) - ::rpc/request-id (uuid/next) - ::rpc/request-at (ct/now)} +(defn- prepare-event + "Call prepare-rpc-event with a bare request, returning the built event." + [caller result] + (let [mdata {::sv/name "test-cmd"} + params {::rpc/profile-id caller + ::rpc/request-id (uuid/next) + ::rpc/request-at (ct/now)} mock-req (reify yetti.request/IRequest (get-header [_ _] nil) (remote-addr [_] "127.0.0.1")) - params (with-meta params {:app.http/request mock-req}) - result {:profile-id "not-a-valid-uuid"} - event (audit/prepare-rpc-event th/*system* mdata params result)] - ;; profile-id must fall back to the RPC params profile-id - (t/is (uuid? (:profile-id event))) - (t/is (= (:id prof) (:profile-id event))))) + params (with-meta params {:app.http/request mock-req})] + (audit/prepare-rpc-event th/*system* mdata params result))) + +(t/deftest prepare-rpc-event-ignores-profile-id-from-result + ;; An audit event belongs to the caller, never to whatever `:profile-id` + ;; the response carries. `get-error-report` returns the report with its + ;; decoded content merged in, and that content can hold the profile that + ;; owned the report, so honoring it attributed the call to somebody who + ;; never made it. + (let [caller (th/create-profile* 1 {:is-active true})] + (t/is (= (:id caller) + (:profile-id (prepare-event (:id caller) + {:profile-id "33601240-a00b-11ea-ba1b-c554cc60e361" + :some-data "value"})))) + ;; an unparseable one must not break the event either + (t/is (= (:id caller) + (:profile-id (prepare-event (:id caller) + {:profile-id "not-a-valid-uuid"})))))) + +(t/deftest prepare-rpc-event-uses-metadata-profile-id + ;; `::audit/profile-id` metadata is the only sanctioned override: it is + ;; how commands that authenticate somebody else (login, verify-token) + ;; attribute the event to the right profile. + (let [caller (th/create-profile* 1 {:is-active true}) + target (th/create-profile* 2 {:is-active true}) + result (with-meta {:some-data "value"} + {::audit/profile-id (:id target)})] + (t/is (= (:id target) + (:profile-id (prepare-event (:id caller) result)))))) + +(t/deftest prepare-rpc-event-falls-back-to-zero-for-anonymous-callers + ;; With no metadata and no authenticated caller there is nobody to + ;; attribute the event to, so it lands on the zero uuid. + (t/is (= uuid/zero + (:profile-id (prepare-event nil {:some-data "value"}))))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; PREPARE-RPC-EVENT PROFILE-ID COERCION +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(t/deftest prepare-rpc-event-coerces-string-metadata-profile-id + ;; `::audit/profile-id` is set by hand in a dozen commands, and some of + ;; them read the value from token claims or other stringly-typed sources. + ;; The audit schema demands a uuid, and `submit*` swallows the validation + ;; error, so an unconverted string would drop the event on the floor. + (let [caller (th/create-profile* 1 {:is-active true}) + target "33601240-a00b-11ea-ba1b-c554cc60e361" + result (with-meta {:some-data "value"} + {::audit/profile-id target})] + (t/is (= #uuid "33601240-a00b-11ea-ba1b-c554cc60e361" + (:profile-id (prepare-event (:id caller) result)))) + (t/is (uuid? (:profile-id (prepare-event (:id caller) result)))))) + +(t/deftest prepare-rpc-event-discards-unusable-metadata-profile-id + ;; An override that cannot be turned into a uuid is dropped, not honoured + ;; and not propagated: the event falls back to the caller, which is always + ;; a valid uuid, instead of failing the schema check and losing the row. + (let [caller (th/create-profile* 1 {:is-active true})] + (doseq [bad ["not-a-valid-uuid" "" " " 42 {} [] :whatever nil false]] + (let [result (with-meta {:some-data "value"} + (cond-> {::audit/profile-id bad} + (nil? bad) (dissoc ::audit/profile-id)))] + (t/is (= (:id caller) + (:profile-id (prepare-event (:id caller) result))) + (str "override " (pr-str bad) " must fall back to the caller")))))) diff --git a/backend/test/backend_tests/rpc_commands_error_reports_test.clj b/backend/test/backend_tests/rpc_commands_error_reports_test.clj index 868023c79c..2fb74345dc 100644 --- a/backend/test/backend_tests/rpc_commands_error_reports_test.clj +++ b/backend/test/backend_tests/rpc_commands_error_reports_test.clj @@ -256,31 +256,41 @@ ;; --- Audit event tests -(t/deftest get-error-report-audit-event-has-uuid-profile-id - ;; When get-error-report returns a report with string profile-id in content, - ;; the audit event must have a proper UUID profile-id (not a string). - ;; This tests the prepare-rpc-event function directly since the test RPC - ;; flow doesn't include the audit middleware wrapper. - (let [profile (th/create-profile* 1 {:is-active true}) - id (uuid/next) - orig-pid "33601240-a00b-11ea-ba1b-c554cc60e361" - ;; Simulate the result from get-error-report with string profile-id - result {:id id - :source "logging" - :hint "test error" - :profile-id orig-pid} - mdata {::sv/name "get-error-report"} - params {::rpc/profile-id (:id profile) - ::rpc/request-id (uuid/next) - ::rpc/request-at (ct/now)} - mock-req (reify yetti.request/IRequest - (get-header [_ _] nil) - (remote-addr [_] "127.0.0.1")) - params (with-meta params {:app.http/request mock-req}) - event (audit/prepare-rpc-event th/*system* mdata params result)] - ;; profile-id must be a UUID, not a string - (t/is (uuid? (:profile-id event))) - (t/is (= #uuid "33601240-a00b-11ea-ba1b-c554cc60e361" (:profile-id event))))) +(t/deftest get-error-report-audit-event-attributes-the-caller + ;; The report content carries the profile that owned the report and the + ;; handler merges that content into the response, so the response holds a + ;; `:profile-id` that does not belong to the caller. The audit event must + ;; still belong to the caller: deriving it from the response attributed + ;; privileged reads to the users whose crashes were being inspected. + (let [caller (th/create-profile* 1 {:is-active true}) + owner "33601240-a00b-11ea-ba1b-c554cc60e361" + id (uuid/next)] + (insert-report! th/*system* + {:id id + :source 4 + :content {:profile-id owner + :hint "test error"}}) + + (let [out (token-cmd caller {::th/type :get-error-report :id id})] + (t/is (th/success? out)) + + (let [result (:result out) + event (audit/prepare-rpc-event + th/*system* + {::sv/name "get-error-report"} + (with-meta {::rpc/profile-id (:id caller) + ::rpc/request-id (uuid/next) + ::rpc/request-at (ct/now) + :id id} + {:app.http/request (reify + yetti.request/IRequest + (get-header [_ _] nil) + (remote-addr [_] "127.0.0.1"))}) + result)] + ;; the response does expose the report owner's profile... + (t/is (= owner (get result :profile-id))) + ;; ...but the audit event belongs to whoever called the command + (t/is (= (:id caller) (:profile-id event))))))) ;; Note: The integration of access token middleware with audit context is tested ;; via unit tests in rpc_audit_test.clj and http_middleware_test.clj. diff --git a/backend/test/backend_tests/rpc_profile_test.clj b/backend/test/backend_tests/rpc_profile_test.clj index ec54555701..a2750beb12 100644 --- a/backend/test/backend_tests/rpc_profile_test.clj +++ b/backend/test/backend_tests/rpc_profile_test.clj @@ -1594,7 +1594,6 @@ (t/is (th/ex-of-type? (:error out) :validation)) (t/is (th/ex-of-code? (:error out) :weak-password)))) - (t/deftest update-profile-password-sends-notification (with-mocks [mock {:target 'app.email/send! :return nil}] (let [profile (th/create-profile* 1) @@ -1645,3 +1644,56 @@ (t/is (= eml/password-changed factory)) (t/is (= (:email profile) to)) (t/is (= (:fullname profile) name)))))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; VERIFY-TOKEN AUDIT ATTRIBUTION +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(t/deftest verify-token-auth-audit-event-attributes-the-authenticated-profile + ;; `verify-token` is anonymous, so the audit event cannot infer the profile + ;; from the caller: the handler must declare it in the result metadata. + (let [profile (th/create-profile* 1 {:is-active true}) + token (tokens/generate th/*system* + {:iss :auth + :exp (ct/in-future "1h") + :profile-id (:id profile)}) + out (th/command! {::th/type :verify-token + :token token})] + (t/is (th/success? out)) + (t/is (= (:id profile) + (get-in (meta (:result out)) [:app.loggers.audit/profile-id]))))) + +(t/deftest verify-token-invitation-audit-event-attributes-the-accepting-profile + ;; Both the invitation claims and the response carry the inviter's + ;; profile-id, so the event must not end up on the inviter: the member who + ;; clicked the link is the one who accepted the invitation. + (with-redefs [app.config/flags #{:login-with-password}] + (let [owner (th/create-profile* 1 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)}) + member (th/create-profile* 2 {:is-active true + :email "invited@example.com"}) + email (:email member) + token (tokens/generate th/*system* + {:iss :team-invitation + :exp (ct/in-future "48h") + :role :editor + :profile-id (:id owner) + :team-id (:id team) + :member-email email + :member-id (:id member)})] + (th/db-insert! :team-invitation + {:id (uuid/random) + :team-id (:id team) + :email-to email + :created-by (:id owner) + :role "editor" + :valid-until (ct/in-future "48h")}) + + (let [out (th/command! {::th/type :verify-token + :token token + ::rpc/profile-id (:id member) + ::rpc/auth-type :session}) + event-pid (get-in (meta (:result out)) [:app.loggers.audit/profile-id])] + (t/is (th/success? out)) + (t/is (= (:id member) event-pid)) + (t/is (not= (:id owner) event-pid)))))) diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index b8f782080a..5f6a5d6543 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -492,13 +492,14 @@ (th/command! {::th/type :verify-token ::rpc/profile-id (:id invitee) :token token})) - organization-event - (fn [] + emitted-events + (fn [event-name] (->> (:call-args-list @audit-mock) (map second) - (filter #(= "accept-organization-invitation" (:name %))) - first)) - token-result (atom nil)] + (filter #(= event-name (:name %))))) + organization-event + (fn [] + (first (emitted-events "accept-organization-invitation")))] (db/insert! (:app.db/pool th/*system*) :team-invitation @@ -520,29 +521,22 @@ (fn [& _] default-team-id)] (let [out (verify! direct-token)] (t/is (th/success? out)) - (reset! token-result (:result out)))) + (t/is (not (contains? (:result out) :organization-invitation-audit))))) (let [event (organization-event)] (t/is (= organization-id (get-in event [:props :organization-id]))) - (t/is (= (:id invitee) (get-in event [:props :user-id]))) - (t/is (= (:id inviter) - (get-in event [:props :user-who-send-invitation]))) - (t/is (not (contains? (:props event) :organization-member-add-source))) - (t/is (not (contains? (:props event) :belongs-to-team-on-add))) - (t/is (not (contains? (:props event) :organization-member-count-before))) + (t/is (= (:id invitee) (get-in event [:props :profile-id]))) + (t/is (= (:id inviter) (get-in event [:props :invited-by]))) + (t/is (= (:email invitee) (get-in event [:props :profile-email]))) (t/is (= :editor (get-in event [:props :role]))) (t/is (uuid? (get-in event [:props :invitation-id]))) - (t/is (= organization-id (:organization-id @token-result))) - (t/is (= :editor (:role @token-result))) - (t/is (not (contains? @token-result :organization-invitation-audit))) - (t/is (= (get-in event [:props :invitation-id]) - (:invitation-id @token-result))) - (t/is (= (:id invitee) - (:member-id @token-result))) - (t/is (= (:id inviter) - (:profile-id @token-result))) - (t/is (= 3 - (:organization-member-count-before @token-result))) + (t/is (= "direct-organization-invitation" + (get-in event [:props :organization-member-add-source]))) + (t/is (false? (get-in event [:props :belongs-to-team-on-add]))) + (t/is (= 3 (get-in event [:props :organization-member-count-before]))) + (t/is (not (contains? (:props event) :invitation-origin))) + (t/is (not (contains? (:context event) :event-origin))) + (t/is (= 1 (count (emitted-events "accept-organization-invitation")))) (t/is (not-any? #(contains? #{"accept-team-invitation" "accept-team-invitation-from"} (:name (second %))) @@ -569,7 +563,7 @@ teams/add-profile-to-team! (fn [& _] nil)] (let [out (verify! team-token)] (t/is (th/success? out)) - (reset! token-result (:result out)))) + (t/is (not (contains? (:result out) :organization-invitation-audit))))) (let [events (mapv second (:call-args-list @audit-mock)) event (organization-event)] @@ -577,25 +571,27 @@ (t/is (some #(= "accept-team-invitation-from" (:name %)) events)) (t/is (= (:id team) (get-in event [:props :team-id]))) (t/is (= organization-id (get-in event [:props :organization-id]))) - (t/is (= (:id invitee) (get-in event [:props :user-id]))) - (t/is (= (:id inviter) - (get-in event [:props :user-who-send-invitation]))) - (t/is (not (contains? (:props event) :organization-member-add-source))) - (t/is (not (contains? (:props event) :belongs-to-team-on-add))) - (t/is (not (contains? (:props event) :organization-member-count-before))) - (t/is (= organization-id - (:organization-id @token-result))) - (t/is (= (:id team) (:team-id @token-result))) - (t/is (= :editor (:role @token-result))) - (t/is (not (contains? @token-result :organization-invitation-audit))) - (t/is (= (get-in event [:props :invitation-id]) - (:invitation-id @token-result))) - (t/is (= (:id invitee) - (:member-id @token-result))) - (t/is (= (:id inviter) - (:profile-id @token-result))) - (t/is (= 5 - (:organization-member-count-before @token-result)))) + (t/is (= (:id invitee) (get-in event [:props :profile-id]))) + (t/is (= (:id inviter) (get-in event [:props :invited-by]))) + (t/is (= "team-invitation" + (get-in event [:props :organization-member-add-source]))) + (t/is (true? (get-in event [:props :belongs-to-team-on-add]))) + (t/is (= 5 (get-in event [:props :organization-member-count-before]))) + (t/is (not (contains? (:props event) :invitation-origin))) + (t/is (not (contains? (:context event) :event-origin))) + (t/is (= 1 (count (emitted-events "accept-organization-invitation"))))) + + (let [from-event (first (emitted-events "accept-team-invitation-from"))] + (t/is (= (:id team) (get-in from-event [:props :team-id]))) + (t/is (= :editor (get-in from-event [:props :role]))) + (t/is (uuid? (get-in from-event [:props :invitation-id]))) + (t/is (= (:id inviter) (get-in from-event [:props :invited-by]))) + (t/is (= (:id invitee) (get-in from-event [:props :profile-id]))) + (t/is (= (:email invitee) (get-in from-event [:props :profile-email]))) + (t/is (not (contains? (get-in from-event [:props]) :email))) + (t/is (not (contains? (get-in from-event [:props]) :user-id))) + (t/is (not (contains? (get-in from-event [:props]) + :user-who-send-invitation)))) (th/reset-mock! audit-mock) (db/insert! (:app.db/pool th/*system*) @@ -616,57 +612,11 @@ teams/add-profile-to-team! (fn [& _] nil)] (let [out (verify! team-token)] (t/is (th/success? out)) - (reset! token-result (:result out)))) + (t/is (not (contains? (:result out) :organization-invitation-audit))))) (let [events (mapv second (:call-args-list @audit-mock))] (t/is (some #(= "accept-team-invitation" (:name %)) events)) - (t/is (not-any? #(= "accept-organization-invitation" (:name %)) events)) - (t/is (not (contains? @token-result :organization-invitation-audit))) - (t/is (not (contains? @token-result :organization-member-count-before))))))) - -(t/deftest accept-organization-invitation-response-ids-match-database - (with-mocks [audit-mock {:target 'app.loggers.audit/submit :return nil}] - (let [inviter (th/create-profile* 211 {:is-active true}) - invitee (th/create-profile* 212 {:is-active true}) - organization-id (uuid/random) - default-team-id (uuid/random) - ;; Token minted with a stale :profile-id (e.g. a re-sent link - ;; requested by someone else) and no :member-id (invitee was - ;; unregistered when invited); the invitation row says inviter. - stale-token (tokens/generate - th/*system* - {:iss :team-invitation - :exp (ct/in-future "1h") - :profile-id (uuid/random) - :role :editor - :organization-id organization-id - :member-email (:email invitee)})] - (db/insert! (:app.db/pool th/*system*) - :team-invitation - {:org-id organization-id - :email-to (:email invitee) - :created-by (:id inviter) - :role "editor" - :valid-until (ct/in-future "48h")}) - - (with-redefs [cf/flags (conj cf/flags :admin-console) - nitrate/call - (fn [_cfg method _params] - (case method - :get-organization-membership {:organization-id organization-id - :is-member false} - :get-organization-members [(:id inviter)] - nil)) - teams/initialize-user-in-organization - (fn [& _] default-team-id)] - (let [out (th/command! {::th/type :verify-token - ::rpc/profile-id (:id invitee) - :token stale-token})] - (t/is (th/success? out)) - (t/is (= (:id inviter) (:profile-id (:result out)))) - (t/is (= (:id invitee) (:member-id (:result out)))) - (t/is (not (contains? (:result out) :user-who-send-invitation))) - (t/is (not (contains? (:result out) :user-id)))))))) + (t/is (not-any? #(= "accept-organization-invitation" (:name %)) events)))))) (t/deftest create-team-invitations-with-email-verification-disabled (with-mocks [mock {:target 'app.email/send! :return nil}] diff --git a/backend/test/backend_tests/tasks_telemetry_test.clj b/backend/test/backend_tests/tasks_telemetry_test.clj index 6c02cd9b45..c243fb1cb3 100644 --- a/backend/test/backend_tests/tasks_telemetry_test.clj +++ b/backend/test/backend_tests/tasks_telemetry_test.clj @@ -754,6 +754,34 @@ (t/is (not (contains? (:props result) :route))) (t/is (not (contains? (:props result) :label))))) +(t/deftest test-filter-telemetry-props-accept-organization-invitation + ;; The shadow row of accept-organization-invitation keeps the ids, the boolean + ;; and the count, and drops every text prop, the raw email among them. The + ;; full row and the Nexus archive still carry all of them, and there is no + ;; context key to fall back on: `safe-backend-context-keys` has no + ;; `:event-origin`, that one belongs to the browser. + (let [ftp (ns-resolve 'app.loggers.audit 'filter-telemetry-props) + profile-id (uuid/next) + organization-id (uuid/next) + result (ftp {:source "backend" + :name "accept-organization-invitation" + :type "action" + :props {:profile-id profile-id + :invited-by profile-id + :organization-id organization-id + :team-id (uuid/next) + :belongs-to-team-on-add true + :organization-member-count-before 3 + :organization-member-add-source "team-invitation" + :profile-email "invitee@example.com"}})] + (t/is (= profile-id (get-in result [:props :profile-id]))) + (t/is (= profile-id (get-in result [:props :invited-by]))) + (t/is (= organization-id (get-in result [:props :organization-id]))) + (t/is (true? (get-in result [:props :belongs-to-team-on-add]))) + (t/is (= 3 (get-in result [:props :organization-member-count-before]))) + (t/is (not (contains? (:props result) :profile-email))) + (t/is (not (contains? (:props result) :organization-member-add-source))))) + (t/deftest test-filter-telemetry-props-organization-sso-failure-keeps-reason (let [ftp (ns-resolve 'app.loggers.audit 'filter-telemetry-props) organization-id (uuid/next) diff --git a/backend/test/backend_tests/test_files/svg-attrs-camel-case.penpot b/backend/test/backend_tests/test_files/svg-attrs-camel-case.penpot new file mode 100644 index 0000000000..dd9513c343 Binary files /dev/null and b/backend/test/backend_tests/test_files/svg-attrs-camel-case.penpot differ diff --git a/frontend/src/app/main/ui/auth/verify_token.cljs b/frontend/src/app/main/ui/auth/verify_token.cljs index 14a3a5625f..d0f6ac4ea1 100644 --- a/frontend/src/app/main/ui/auth/verify_token.cljs +++ b/frontend/src/app/main/ui/auth/verify_token.cljs @@ -6,11 +6,9 @@ (ns app.main.ui.auth.verify-token (:require - [app.common.data :as d] [app.config :as cf] [app.main.data.auth :as da] [app.main.data.common :as dcm] - [app.main.data.event :as ev] [app.main.data.notifications :as ntf] [app.main.data.profile :as du] [app.main.repo :as rp] @@ -45,33 +43,8 @@ (st/emit! (da/login-from-token tdata))) (defmethod handle-token :team-invitation - [{:keys [state team-id organization-team-id organization-name invitation-token] :as tdata}] - (when (and (= state :created) - (contains? tdata :organization-member-count-before)) - (let [direct-invitation? (some? organization-team-id)] - (st/emit! - (ev/event - (-> (select-keys tdata [:team-id :organization-id :role - :invitation-id :member-id :profile-id - :organization-member-count-before]) - ;; The audit event keeps its own names: :user-id is the - ;; invitee (:member-id, backfilled by the backend with the - ;; accepting profile) and :user-who-send-invitation is the - ;; inviter (:profile-id, backfilled from `created-by`). - (assoc :user-id (:member-id tdata) - :user-who-send-invitation (:profile-id tdata)) - (dissoc :member-id :profile-id) - (d/without-nils) - (assoc :organization-member-add-source - (if direct-invitation? - "direct-organization-invitation" - "team-invitation") - :belongs-to-team-on-add (boolean team-id) - ::ev/name "accept-organization-invitation" - ::ev/origin (if direct-invitation? - "organization-invitation-acceptance" - "team-invitation-acceptance"))))))) - + [{:keys [state team-id organization-team-id organization-name invitation-token + redirect-to]}] (case state :created (if organization-team-id @@ -85,7 +58,7 @@ (ntf/success (tr "auth.notifications.team-invitation-accepted")))) :pending - (let [route-id (:redirect-to tdata :auth-register)] + (let [route-id (or redirect-to :auth-register)] (st/emit! (rt/nav route-id {:invitation-token invitation-token}))))) (defmethod handle-token :default diff --git a/frontend/test/frontend_tests/data/nitrate_test.cljs b/frontend/test/frontend_tests/data/nitrate_test.cljs index 19ed782d0d..365715d2e4 100644 --- a/frontend/test/frontend_tests/data/nitrate_test.cljs +++ b/frontend/test/frontend_tests/data/nitrate_test.cljs @@ -140,74 +140,6 @@ (t/is (= :auth-register (:id event))) (t/is (= invitation-token (get-in event [:params :invitation-token]))))))) -(t/deftest accept-organization-invitation-audit-event-test - (let [emitted (atom [])] - (with-redefs [st/emit! (fn - ([event] - (swap! emitted conj event)) - ([event & events] - (swap! emitted into (cons event events))))] - (t/testing "accepting a team invitation that adds an organization member" - (verify-token/handle-token - {:iss :team-invitation - :state :created - :team-id "team-1" - :organization-id "organization-1" - :role :editor - :invitation-id "invitation-1" - :member-id "invitee-1" - :profile-id "inviter-1" - :organization-member-count-before 4}) - - (t/is (= {::ev/name "accept-organization-invitation" - ::ev/origin "team-invitation-acceptance" - :team-id "team-1" - :organization-id "organization-1" - :role :editor - :invitation-id "invitation-1" - :user-id "invitee-1" - :user-who-send-invitation "inviter-1" - :organization-member-add-source "team-invitation" - :belongs-to-team-on-add true - :organization-member-count-before 4} - @(first @emitted)))) - - (reset! emitted []) - (t/testing "accepting an invitation directly to the organization" - (verify-token/handle-token - {:iss :team-invitation - :state :created - :organization-id "organization-2" - :organization-team-id "team-default" - :role :viewer - :invitation-id "invitation-2" - :member-id "invitee-2" - :profile-id "inviter-2" - :organization-member-count-before 0}) - - (t/is (= {::ev/name "accept-organization-invitation" - ::ev/origin "organization-invitation-acceptance" - :organization-id "organization-2" - :role :viewer - :invitation-id "invitation-2" - :user-id "invitee-2" - :user-who-send-invitation "inviter-2" - :organization-member-add-source "direct-organization-invitation" - :belongs-to-team-on-add false - :organization-member-count-before 0} - @(first @emitted)))) - - (reset! emitted []) - (t/testing "does not audit a team invitation for an existing organization member" - (verify-token/handle-token - {:iss :team-invitation - :state :created - :team-id "team-3" - :organization-id "organization-3" - :role :editor}) - - (t/is (= 3 (count @emitted))))))) - (t/deftest build-admin-console-url-preserves-public-uri-subpath (t/testing "builds admin console routes below the configured Penpot subpath" (let [public-uri (u/uri "https://example.com/penpot/")]