From 3b886995badb2237fe47a01d7a54072080eb1af1 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 29 Sep 2026 15:53:24 +0000 Subject: [PATCH] :bug: Emit the accept-organization-invitation audit event once The event was written twice per accepted organization invitation. The backend submitted it, and the browser then re-submitted a copy of the props that the backend had already put in the response under `:organization-invitation-audit` (`handle-token :team-invitation` in `verify-token.cljs`). Both rows carried the same name with different prop vocabularies, and the browser copy only existed when the browser finished the flow. Emit the event from the backend only. It now also carries the three props that lived in the browser copy: the organization member count before the add, the add source, and whether the invitee also joined a team. The origin moves to the event context as `:event-origin`. The response no longer includes `:organization-invitation-audit`, so the browser stops emitting the event and `verify-token.cljs` drops its `app.main.data.event` require. The `accept-*` events of this command now share one prop vocabulary: `:profile-id` for the accepting profile, `:invited-by` for the inviter and `:profile-email` for the email, replacing the mix of `:user-id`/`:user-who-send-invitation` and `:email`. Audit consumers of `accept-organization-invitation` now see one row per acceptance instead of two, and must read the new prop names. AI-assisted-by: space-bunny-free --- .serena/memories/backend/audit-log.md | 3 +- backend/src/app/rpc/commands/verify_token.clj | 77 ++++++++-------- backend/test/backend_tests/rpc_team_test.clj | 89 ++++++++----------- .../backend_tests/tasks_telemetry_test.clj | 28 ++++++ .../src/app/main/ui/auth/verify_token.cljs | 13 +-- .../frontend_tests/data/nitrate_test.cljs | 29 ------ 6 files changed, 107 insertions(+), 132 deletions(-) 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/")]