diff --git a/.serena/memories/backend/audit-log.md b/.serena/memories/backend/audit-log.md index 825d9a2162..ccd4153655 100644 --- a/.serena/memories/backend/audit-log.md +++ b/.serena/memories/backend/audit-log.md @@ -19,6 +19,7 @@ Penpot records what users do as events in the Postgres `audit_log` table. There - 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`) @@ -50,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/backend/src/app/rpc/commands/verify_token.clj b/backend/src/app/rpc/commands/verify_token.clj index 3c893dc6b2..d98c16dc00 100644 --- a/backend/src/app/rpc/commands/verify_token.clj +++ b/backend/src/app/rpc/commands/verify_token.clj @@ -254,12 +254,6 @@ "direct-organization-invitation" "team-invitation")) - organization-event-origin - (when organization-id-on-add - (if organization-id - "organization-invitation-acceptance" - "team-invitation-acceptance")) - organization-member-count-before (when organization-id-on-add (count @@ -300,46 +294,45 @@ (-> (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) - ;; when the invitation is to an organization, instead of a team, add the - ;; accepted-team-id as :organization-team-id - (:organization-id claims) - (assoc :organization-team-id accepted-team-id) - - organization-id-on-add - (assoc :organization-invitation-audit - {:origin organization-event-origin - :props - (-> props - (assoc :organization-id organization-id-on-add - :organization-member-add-source organization-add-source - :belongs-to-team-on-add (boolean team-id) - :user-id (:id profile) - :user-who-send-invitation (:created-by invitation) - :organization-member-count-before - organization-member-count-before) - (audit/clean-props))})) - ;; 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. - (rph/with-meta {::audit/profile-id (:id profile)})))))) + (cond-> (assoc claims :state :created) + ;; when the invitation is to an organization, instead of a team, add the + ;; accepted-team-id as :organization-team-id + (:organization-id claims) + (assoc :organization-team-id accepted-team-id) + ;; 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/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 61acf23cb8..1c30415db2 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)) - frontend-event (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,32 +521,22 @@ (fn [& _] default-team-id)] (let [out (verify! direct-token)] (t/is (th/success? out)) - (reset! frontend-event - (get-in out [:result :organization-invitation-audit])))) + (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-invitation-acceptance" - (:origin @frontend-event))) - (t/is (= organization-id - (get-in @frontend-event [:props :organization-id]))) - (t/is (= (:id invitee) - (get-in @frontend-event [:props :user-id]))) - (t/is (= (:id inviter) - (get-in @frontend-event [:props :user-who-send-invitation]))) (t/is (= "direct-organization-invitation" - (get-in @frontend-event [:props :organization-member-add-source]))) - (t/is (false? (get-in @frontend-event [:props :belongs-to-team-on-add]))) - (t/is (= 3 - (get-in @frontend-event [:props :organization-member-count-before]))) + (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 %))) @@ -572,8 +563,7 @@ teams/add-profile-to-team! (fn [& _] nil)] (let [out (verify! team-token)] (t/is (th/success? out)) - (reset! frontend-event - (get-in out [:result :organization-invitation-audit])))) + (t/is (not (contains? (:result out) :organization-invitation-audit))))) (let [events (mapv second (:call-args-list @audit-mock)) event (organization-event)] @@ -581,26 +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 (= "team-invitation-acceptance" - (:origin @frontend-event))) - (t/is (= (:id team) (get-in @frontend-event [:props :team-id]))) - (t/is (= organization-id - (get-in @frontend-event [:props :organization-id]))) - (t/is (= (:id invitee) - (get-in @frontend-event [:props :user-id]))) - (t/is (= (:id inviter) - (get-in @frontend-event [:props :user-who-send-invitation]))) + (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 @frontend-event [:props :organization-member-add-source]))) - (t/is (true? (get-in @frontend-event [:props :belongs-to-team-on-add]))) - (t/is (= 5 - (get-in @frontend-event [:props :organization-member-count-before])))) + (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*) @@ -621,13 +612,11 @@ teams/add-profile-to-team! (fn [& _] nil)] (let [out (verify! team-token)] (t/is (th/success? out)) - (reset! frontend-event - (get-in out [:result :organization-invitation-audit])))) + (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 (nil? @frontend-event)))))) + (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 01f72977ed..e3a8570832 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/frontend/src/app/main/ui/auth/verify_token.cljs b/frontend/src/app/main/ui/auth/verify_token.cljs index 52a17761c6..3c1e7902e4 100644 --- a/frontend/src/app/main/ui/auth/verify_token.cljs +++ b/frontend/src/app/main/ui/auth/verify_token.cljs @@ -9,7 +9,6 @@ [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] @@ -44,14 +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-let [{:keys [origin props]} (:organization-invitation-audit tdata)] - (st/emit! - (ev/event - (assoc props - ::ev/name "accept-organization-invitation" - ::ev/origin origin)))) - + [{:keys [state team-id organization-team-id organization-name invitation-token + redirect-to]}] (case state :created (if organization-team-id @@ -65,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 2c59912c70..de5c56d92f 100644 --- a/frontend/test/frontend_tests/data/nitrate_test.cljs +++ b/frontend/test/frontend_tests/data/nitrate_test.cljs @@ -11,8 +11,6 @@ [app.main.data.event :as ev] [app.main.data.nitrate :as dnt] [app.main.data.nitrate-audit :as nitrate-audit] - [app.main.store :as st] - [app.main.ui.auth.verify-token :as verify-token] [cljs.test :as t :include-macros true])) (t/deftest account-age-days-test @@ -115,33 +113,6 @@ (t/is (contains? event :days-since-member-added)) (t/is (nil? (:days-since-member-added event))))))) -(t/deftest accept-organization-invitation-audit-event-test - (let [emitted (atom []) - props {:team-id "team-1" - :organization-id "organization-1" - :role :editor - :invitation-id "invitation-1" - :organization-member-add-source "team-invitation" - :belongs-to-team-on-add true - :organization-member-count-before 4}] - (with-redefs [st/emit! (fn - ([event] - (swap! emitted conj event)) - ([event & events] - (swap! emitted into (cons event events))))] - (verify-token/handle-token - {:iss :team-invitation - :state :created - :team-id "team-1" - :organization-invitation-audit - {:origin "team-invitation-acceptance" - :props props}})) - - (let [event @(first @emitted)] - (t/is (= "accept-organization-invitation" (::ev/name event))) - (t/is (= "team-invitation-acceptance" (::ev/origin event))) - (t/is (= props (dissoc event ::ev/name ::ev/origin)))))) - (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/")]