fix: keep existing privileges when User.Added is processed late #331

2026-09-16T05:04:29Z

argoyle commented

2026-09-16 05:04:29 +00:00

Problem

Setup() registers one transient consumer per routing key, and go-messaging-amqp mints a separate randomly named queue per consumer, each drained by its own goroutine. User.Added and Privilege.Added for the same company therefore arrive in any order. Process(*UserAdded) replaced privileges[email][companyID] with an empty CompanyPrivileges{}, so a Privilege.Added processed first lost its privilege — permanently, since Fetch() only runs at service start.

This surfaced as authz-service #826 acceptance-test failures: a newly created company's Admin grant vanished, company-service's CreateCompany completion callback waited out its full 30s HasCompanyPrivilege timeout, and the company page never rendered. Two runs failed on that same 30s wait, in different suites.

Fix

User.Added creates the company entry only when it is missing. This matches authz-service's own aggregate (domain/aggregates.go creates the user entry only when absent) and its read view (on conflict (email, company_id) do nothing) — before this change the cache disagreed with the authority. It also makes a User.Added redelivery harmless.

Verification

CGO_ENABLED=1 go test -race ./... passes; prek run --all-files clean.
Mutation-checked: restoring the old User.Added body makes TestPrivilegeHandler_Process_UserAdded_Keeps_Existing_Privileges fail, so the test pins the behaviour rather than restating it.
New tests cover both delivery orders, and that a re-add after User.Removed restores membership without the revoked privileges.

Review

Go Backend and Security experts reviewed the diff. Both cleared it with no Critical or High findings against the change; Security confirmed it cannot fail open, because only Privilege.Added ever sets a flag. Pre-existing hazards they found are tracked in Ambix rather than folded in here: the four-queue design still allows a stale Privilege.Added after User.Removed to resurrect a grant, revocations published during startup are lost for the process lifetime (Fetch() runs before the consumers exist), and Fetch() merges into the privilege map instead of replacing it. The CLAUDE.md note records the ordering invariant and is explicit that the converse does not hold yet.

Rollout

No wire-format or signature change, so mixed versions run side by side safely. After release, 11 services on v0.6.0 take a patch bump; accounting-service and supplier-invoice-service are still on v0.5.1 and only get it with their Codeberg migration.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XMvdB7bcwn1CrQKM4dCshM

## Problem `Setup()` registers one transient consumer per routing key, and go-messaging-amqp mints a separate randomly named queue per consumer, each drained by its own goroutine. `User.Added` and `Privilege.Added` for the same company therefore arrive in any order. `Process(*UserAdded)` replaced `privileges[email][companyID]` with an empty `CompanyPrivileges{}`, so a `Privilege.Added` processed first lost its privilege — permanently, since `Fetch()` only runs at service start. This surfaced as authz-service #826 acceptance-test failures: a newly created company's Admin grant vanished, company-service's `CreateCompany` completion callback waited out its full 30s `HasCompanyPrivilege` timeout, and the company page never rendered. Two runs failed on that same 30s wait, in different suites. ## Fix `User.Added` creates the company entry only when it is missing. This matches authz-service's own aggregate (`domain/aggregates.go` creates the user entry only when absent) and its read view (`on conflict (email, company_id) do nothing`) — before this change the cache disagreed with the authority. It also makes a `User.Added` redelivery harmless. ## Verification - `CGO_ENABLED=1 go test -race ./...` passes; `prek run --all-files` clean. - Mutation-checked: restoring the old `User.Added` body makes `TestPrivilegeHandler_Process_UserAdded_Keeps_Existing_Privileges` fail, so the test pins the behaviour rather than restating it. - New tests cover both delivery orders, and that a re-add after `User.Removed` restores membership without the revoked privileges. ## Review Go Backend and Security experts reviewed the diff. Both cleared it with no Critical or High findings against the change; Security confirmed it cannot fail open, because only `Privilege.Added` ever sets a flag. Pre-existing hazards they found are tracked in Ambix rather than folded in here: the four-queue design still allows a stale `Privilege.Added` after `User.Removed` to resurrect a grant, revocations published during startup are lost for the process lifetime (`Fetch()` runs before the consumers exist), and `Fetch()` merges into the privilege map instead of replacing it. The CLAUDE.md note records the ordering invariant and is explicit that the converse does not hold yet. ## Rollout No wire-format or signature change, so mixed versions run side by side safely. After release, 11 services on v0.6.0 take a patch bump; accounting-service and supplier-invoice-service are still on v0.5.1 and only get it with their Codeberg migration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01XMvdB7bcwn1CrQKM4dCshM

argoyle added 1 commit 2026-09-16 05:04:31 +00:00

fix: keep existing privileges when User.Added is processed late

authz_client / test (push) Skipped

authz_client / vulnerabilities (push) Skipped

pre-commit / pre-commit (push) Skipped

authz_client / vulnerabilities (pull_request) Failing after 55s

Details

authz_client / test (pull_request) Successful in 1m4s

Details

pre-commit / pre-commit (pull_request) Successful in 2m16s

Details

92f78d369a

Setup() registers one transient consumer per routing key, and each mints its
own queue drained by its own goroutine, so User.Added and Privilege.Added for the
same company can be processed in either order. Process(*UserAdded) replaced the
company entry with empty privileges, so a Privilege.Added handled first lost its
privilege until the next Fetch(), which only runs at service start.

Create the entry only when it is missing instead, matching authz-service's own
aggregate and read view, which both leave an existing user entry alone. This also
makes a User.Added redelivery harmless.

Seen as authz-service #826 acceptance-test failures: a new company's Admin grant
vanished, so company-service's CreateCompany callback waited out its 30s timeout
and the company page stayed empty.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XMvdB7bcwn1CrQKM4dCshM

gitea-actions Bot commented

2026-09-16 05:08:07 +00:00

Coverage Report

Total coverage: 98%

## Coverage Report Total coverage: **98%**

argoyle added 1 commit 2026-09-16 05:16:14 +00:00

chore(deps): bump go-messaging-amqp to v0.0.5 and amqp091-go to v1.15.0

authz_client / test (push) Skipped

authz_client / vulnerabilities (push) Skipped

pre-commit / pre-commit (push) Skipped

authz_client / vulnerabilities (pull_request) Successful in 48s

Details

authz_client / test (pull_request) Successful in 1m0s

Details

pre-commit / pre-commit (pull_request) Successful in 2m15s

Details

55e16832c1

govulncheck reports GO-2026-6372 against amqp091-go v1.12.0, pulled in
indirectly: a broker-controlled oversized payload can exhaust memory. Fixed in
v1.13.0; take v1.15.0. The advisory fails CI on main too, and this is the first
build since it was published.

go-messaging-amqp goes to v0.0.5 at the same time, which is what every migrated
service already runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XMvdB7bcwn1CrQKM4dCshM

gitea-actions Bot commented

2026-09-16 05:17:25 +00:00