Repository navigation
feat(frontend): member administration page (FND-010C, stacked on #19) - #20
Open
knittingcode wants to merge 62 commits into
Open
knittingcode wants to merge 62 commits into
knittingcode wants to merge 62 commits into
Conversation
ADMINs add existing users by e-mail, change member roles and remove
members (POST, PATCH and DELETE under /organizations/{id}/members) with
the new permissions membership.add, membership.change_role and
membership.remove. MANAGER and REP keep their read permissions.
Every change runs in one transaction that locks the organization row,
checks the actor's membership again under the lock, reads the target
scoped by organization, applies the last-admin rule (BR-03, 409
LAST_ADMIN) and writes the change with its audit event. Existing or
removed memberships are a 409 MEMBERSHIP_EXISTS, unknown users and roles
a 422. The OpenAPI specification describes the three operations and the
new conflict problem.
MembershipAdministrationTest checks the role matrix of the new routes, foreign organizations and membership IDs, the last-admin rule, audit events and their rollback when the audit write fails, CSRF and origin checks, and two lock scenarios held from a separate connection: mutual concurrent demotions leave exactly one ADMIN, and an ADMIN demoted while the request waits for the lock is refused. ApiClient gains PATCH, DELETE and non-blocking requests (FpmServer dispatch/collect); the failing-audit trigger moves to a shared trait. Eight negative controls guard the new protections.
…DR-020 ADR-020 (proposed, awaiting architecture review) records the transaction and lock design, the last-admin rule, the error conventions, the exclusions and the open decisions. The authentication guide lists the new endpoints, permissions, audit actions, log events and tests.
addMember, changeRole and removeMember send the CSRF token, a JSON body and URL-encoded identifiers through apiRequest; isConflict reads the LAST_ADMIN and MEMBERSHIP_EXISTS codes of a 409 conflict. The Permission type gains membership.add, membership.change_role and membership.remove.
/o/:organizationId/members lists the members with the page in the address and shows the add form, the role change and the removal only when the user's own membership has the permission. Every change and every 403, 404 or 409 reloads the organization and the members; nothing changes in the page before the server answers. Conflicts, validation errors (at their inputs, with focus on the first), rate limits and failures with their request ID are reported inline; removing oneself reads the session again and leads to the start page. The organization page links to it.
An ADMIN sees the controls and the refused changes (existing membership, unknown user, last admin) leave the members unchanged; a REP sees no controls and its forged changes are answered 403. The seeded memberships are not modified.
The E2E seed gets a fifth synthetic user, Spare, with no membership. A new members.spec.ts journey has the ADMIN of Alpha add Spare as REP, change the role to MANAGER and remove the membership again, checking each step through the API. The seeded memberships the other specs rely on are left as they were.
…e organization lock The lock holder observes pg_stat_activity inside its own transaction, which keeps the first snapshot until it is cleared, so the waiting requests were never seen. Compare the role change audit payload without relying on jsonb key order.
… key checks The membership lock now uses FOR NO KEY UPDATE: it still serialises the membership changes of one organization, but no longer makes inserts of rows that reference the organization wait for it. The operator's flowcrm:membership:add takes the same lock and checks for an existing membership under it, so it cannot race an ADMIN's add of the same user into a unique constraint violation. OrganizationLockTest covers both against PostgreSQL; two new negative controls (45 in total) prove the tests detect a return to FOR UPDATE and an operator add without the lock.
…embers e2e needs a fresh seed
…t adds and malformed requests The new routes answer with no-store and Vary: Cookie; an ADMIN of another organization who is a REP here gets 403 for every change; two concurrent adds of one user give one 201 and one 409 MEMBERSHIP_EXISTS with a single row and audit event; broken bodies are 400 and malformed membership IDs 404, with nothing changed. The role change audit event is checked for its actor, organization and the absence of e-mail addresses.
Workflows, the role matrix, HTTP examples for adding, changing and removing members with synthetic identities, the error table with the SPA's messages, and the operational limits of ADR-020. Each example names the integration test that verifies it.
…tive controls The per-user API limit (600 requests per minute) is implemented on main, so the README no longer lists it as a limitation; this also matches the README of PR #17. ADR-020 now lists all ten negative controls the branch adds.
…e members page branch
… an open decision Adding by e-mail tells an ADMIN whether an address has an account and returns the user's name and ID; the open decision now names that disclosure and the missing consent, and lists the options without choosing one.
…e outcome is unknown A csrf-invalid answer that persists after the client's single retry was reported as a missing permission. It now says that the session's security token could not be verified and that nothing was changed; the list is not reloaded because the request was not processed. After a network failure or a 2xx answer whose body cannot be read or has an unexpected shape, it is unknown whether the change was saved. The page now says so and reads the organization and the member list again, instead of claiming the change could not be saved and keeping a possibly stale list. Adds MembersPage tests for both cases and updates the error table in docs/membership-administration.md.
# Conflicts: # README.md
…17) into the members page branch
Conflict in docs/architecture/authentication.md: kept this branch's unit test row; the control count is main's 38 plus this branch's 10.
…nto the branch
…nknown outcome A 502 or 504 of the edge (PHP-FPM failed or timed out) can follow a committed membership change. The members page reported it as "The change could not be saved" and kept the old list. It now says that the outcome is unknown and reads the organization and the list again, as it already did when the API could not be reached. The API's own 5xx problems, which state that nothing was changed, are reported as before.
After a change, and while another page is read, the page kept showing the previous rows with enabled controls: a member just removed could be removed again (answered 404), and the caption and pager already named the requested page above the rows of the old one. Until the requested list has arrived the table is now marked aria-busy, the row controls are disabled, and the caption and pager name the page actually shown.
…ted change The answer to POST and PATCH reads the member's profile after the commit, and any request can fail while its response is finished. Only a failed audit write (or an error before the commit) guarantees the rollback; the change and its event are still committed together. The guide's error table and ADR-020 now say so and tell clients to read the list again.
…ers page branch
The API can answer 500 after a membership change has been committed (the answer is prepared after the commit; see the membership guide). The members page reported an API 500 problem as "The change could not be saved" and kept the old list. Every 5xx answer to an add, role change or removal is now an unknown outcome, as 502/504 of the edge already were: the page says it is not known whether the change was saved and reads the organization and the list again. 4xx answers are handled as before.
…e commit Adding a member and changing a role committed the change and its audit event, and only then read the member's profile for the answer. A failure of that read answered 500 for a saved change. ManageMemberships now builds the member view inside the change's transaction, so a failing read rolls the change and its audit event back. The response body, status codes and API contract are unchanged. - integration test: the profile read fails (a test row security policy) for POST and PATCH; 500 and nothing saved, then the request succeeds - unit test: the read is inside the transaction and its failure rolls back - negative control "membership response read after the commit" (50 total); the audit-after-commit control is adapted to the new code - docs: a 500 now means nothing changed, except for a commit without a known result or a failure after the commit (for example the session write)
…t be read again When the list read after a change failed, the load error replaced the whole page: an "outcome unknown" message (or the success status) disappeared and the focus was lost, so the user could no longer tell that a change might have been saved. The load error now shows the answer to the change next to it and receives the focus. The unknown-outcome messages say that the list "is read again" instead of claiming that it "was reloaded", which was wrong when that read failed, and a request without any answer (network failure, abort, timeout) is described as a failed connection rather than as an API that could not be reached. Tests cover aborted and timed-out requests, double clicks and a second Enter while a change runs (one request each), and a failed list read after an unknown outcome and after a success.
A 403 (other than csrf-invalid) or a 404 to a change can mean that the user's own membership was demoted or removed while the page was open. The page already read the organization and the list again, but the session kept the old memberships: the organization switcher still offered a removed organization, and "Go to the start page" led straight back to "Not found". The session is now read again after such an answer, so the switcher and the start page follow the server. Tests cover the removed and the demoted actor, and switching to another organization while a change in the first one is still running.
… keyboard on the members page One journey, with the keyboard only where the ADMIN acts: the ADMIN adds the seeded user Deputy as ADMIN, who opens the members page in a second session. Demoted to MANAGER, Deputy's next change is refused (403) and the controls go. The ADMIN's removal of Deputy reaches the server but its answer is lost (the browser's request is aborted after the same removal was sent with the ADMIN's session): the page reports an unknown outcome and the list read again no longer shows Deputy. Deputy's next change then ends on "Not found", and the start page no longer offers the organization. The seeded memberships are left unchanged. seed_e2e creates the additional user Deputy without a membership.
…ers page branch
… the members page #19 now builds the answer inside the transaction, so an API 500 means that nothing changed except after an ambiguous commit or a failure after it. The members page still treats every 5xx as an unknown outcome, because it cannot tell these cases apart; the code comment and the SPA section now say so and refer to the renamed section.
…hanges of one member Two real-PostgreSQL lock tests that release two waiting requests together: - two ADMINs remove the same member: 204 and 404, the member is removed once with one audit event; - two ADMINs set different roles for the same member: both apply one after the other, the second starting from the first one's role, so the audit events form one chain that ends with the stored role. Both are added to the "membership changes without the organization lock" negative control.
…DR-019 to ADR-020 ADR-020 and the membership guide said that the last enabled ADMIN cannot be demoted when the only other ADMIN is disabled. The last-admin rule counts active memberships whatever the account state, so the enabled ADMIN can demote or remove themselves, leaving only the disabled ADMIN. The behaviour is unchanged (it is an open decision in ADR-020); a real-PostgreSQL test now pins it. ADR-019 now points to ADR-020 for membership administration.
…ers page branch
…bership controls New tests: - an ADMIN of A who is only a REP of B adds a user who is a member of B: 201, and the re-check under A's lock reads the actor's membership in A (closes the untested organization scoping of ofOrganizationAndUser); - a REP promoted to ADMIN has the ADMIN permissions on the next request; - anonymous (401) and rate-limited (429) POST, PATCH and DELETE on the member routes change nothing; - MembershipRefusalLoggingTest: refused lookups are logged on the security channel with reason, actor, organization, route and request ID, never the e-mail address. Nine negative controls: actor membership not scoped to the organization, ADMIN count unscoped or including removed ADMINs, last-admin rule off by one, removal checked like a promotion, removed target changed again, actor checked for a weaker permission, membership change not audited, lock with no row lock. 59 in total.
PostgreSQL text cannot hold a NUL and pdo_pgsql cuts a bound string at the first one, so adding a member with "user@example.test\0junk" found and added the account of "user@example.test" (201). The lookup of UserDirectory::idOfEmail (member add and operator console) now treats such an address as unknown, so the add is a 422 UNKNOWN_USER. No contract change. Regression test and negative control "address with a NUL looked up" (60 controls). The login path is handled separately.
…ers page branch
idOfEmail keeps main's User::canHoldEmail check (NUL byte and invalid UTF-8), which replaces this branch's NUL check; its control is dropped and main's user directory control also runs testAnAddressWithANulIsAnUnknownUser. The documented count is 61 (main's 40 and this branch's 21).
… into the members page branch
Only the documented negative-control count and the test table conflicted: 64 with #26's three controls; the unit-test row keeps this branch's membership entries.
…and #32) into the members page branch
…rship has an unknown outcome Removing oneself or changing one's own role without a usable answer (no answer, an unreadable success answer, or a 5xx) only read the list again. If the removal had been saved, that list was "Not found" while the session still listed the organization: the switcher offered it and the start page led straight back to "Not found". The session is now read again in these cases, as it already was after a 403 or 404.
… session it may still have Two browser cases for the #32 behaviour on the members page: a logout that never reaches the server shows the alert, keeps the page and the session (GET /me 200), and a second sign-out ends it; a logout processed by the server whose answer is lost shows the same alert, and the next change leads to the login with "Your session has ended" and changes nothing.
This was referenced Oct 25, 2026
…n membership changes Membership administration meets the session invalidation of disabled users (ADR-019 decision 4, main #26) in two places, both untested so far: - A disabled ADMIN's session gets 401 on POST, PATCH and DELETE of the member routes and changes nothing. Enabling the user again does not bring that session back, and neither disable nor enable touches the membership: after a new login the ADMIN administers again. - An operator disables an ADMIN whose change is waiting for the organization lock. The request holds its session row (LOCK_TRANSACTIONAL), so the disable's session delete waits for it: the change and its audit event commit first, the disable after, the session row is not brought back, and the next request is a 401. The lock polling helper now takes the statement to look for, so both waits are observed the same way. New negative control: a session row not locked for the request (LOCK_NONE) is detected by the in-flight test.
…ers page branch
This was referenced Oct 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked PR
Depends on #19 (the membership administration API). This branch contains #19's commits (merged in, no rebase) and the frontend commits:
feat(frontend): add the membership administration API clientfeat(frontend): add the member administration pagetest(e2e): cover the member administration through the edgefix(members): treat a server error that is not an API problem as an unknown outcomefix(members): disable the controls while the list shown is out of datefix(members): let the member table fit a narrow screentest(e2e): cover MANAGER, foreign organization, ended session and narrow screen on the members pagefix(frontend): report a CSRF refusal, not an unknown outcome, when the token cannot be refreshedfix(members): keep the focus in the busy confirmation while a removal runsfix(members): treat every server error to a change as an unknown outcomefix(members): keep the answer to a change in view when the list cannot be read againfix(members): read the session again after a 403 or 404 to a changetest(e2e): cover a demoted and a removed ADMIN, a lost answer and the keyboard on the members pagedocs(members): refer to the API's "A 500 after the commit" cases from the members pagefix(members): read the session again when a change of one's own membership has an unknown outcometest(e2e): a sign-out without an answer on the members page keeps the session it may still haveIt targets
mainso CI runs the complete matrix, including end-to-end tests against the real API. Review only the frontend commits here, and merge #19 first. After #19 is merged, this PR's diff reduces to the frontend commits.Summary
The members page of the SPA (FCRM-FND-010C): list, add, change role and remove members of one organization, on top of the membership API of #19.
/o/:organizationId/members, with?page=in the URL, linked from the organization page. It is a member table (caption,scope=col); the e-mail column appears only withmembership.view_email.membership.permissionscontain them:LAST_ADMINandMEMBERSHIP_EXISTShave their own messages;aria-describedby, an error summary and focus on the first error;role=status.src/api/memberships.ts.Motivation
ADMINs need to manage memberships in the browser rather than through the operator console. The page must never claim a change it cannot confirm: the server decides every change, and when the answer is missing or ambiguous the page says so and reads the server's state again.
Implementation
src/api/memberships.ts: POST/PATCH/DELETE through the sharedapiRequest(CSRF header, one retry aftercsrf-invalid).src/features/members/MembersPage.tsx,AddMemberForm.tsx,messages.ts: the page, the form and the mapping of every answer to a message, a reload decision and, after a 403 or 404, a session refresh.docs/membership-administration.md("Members page in the SPA", "What the members page shows"),docs/architecture/authentication.md.Testing performed
Current head
8051d68contains #19 at3534fe8and maine03d151(#26, #32).8051d68: success. The run reported:8051d68(dev web image, read-only mounts):tsc --buildOK,eslint --max-warnings=0OK, Vitest 97/97,vite buildOK.b213459: before the fix, the 4 new Vitest cases ran against the source atbff316c. Result: 3 failed / 49 passed in the members file. The failures were both self-removal cases (connection failure, 502) and the own-role 504 case, each with "expected 1 to be 2" session reads. The fourth case is a control (another member's change does not re-read the session); it passes before and after.Earlier results (before AUTO-004):
Results at
f5f31b8(#19 at5c7567dmerged; the frontend tree andtests/appare identical to20a6610, where Playwright ran; static checks re-run atef5aa2a, frontend identical).frontend/mounted read-only):tsc --buildOK,eslint --max-warnings=0 .OK, Vitest 88 passed / 88 (6 files),vite buildOK.MembersPage.test.tsx("unclear answers, repeated clicks and changed access"): a DELETE ending inAbortErrororTimeoutErroris an unknown outcome and the list is read again; POST, PATCH and DELETE each send one request on a double click (and on a second Enter for the form) while the change runs; a failed list read after an unknown outcome (502) and after a success keeps the answer in view with the focus on the alert; after a 404 and a 403 the session is read again (removed actor: the switcher and the start page no longer offer the organization); switching to another organization while a change runs shows that organization's controls and no stale message.114228c): 4 failed / 44 passed in the members file — both "list cannot be read again" cases and both session cases. The abort/timeout, double-click and cross-organization cases passed before; they are kept as coverage.20a6610: 19 passed / 19, none skipped. New journeykeyboard only: an ADMIN who is demoted or removed while the page is open is told so; a lost answer is an unknown outcome: an ADMIN adds the seeded user Deputy as ADMIN with the keyboard; Deputy (second session) is demoted to MANAGER, their next change gets 403, the alert has the focus and the controls go; the ADMIN removes Deputy with the keyboard while the browser's answer is lost (the request is aborted after the same removal was sent with the ADMIN's session): the page reports an unknown outcome and the list read again no longer shows Deputy; Deputy's next change ends on "Not found" and the start page no longer offers the organization.tests/app/prodlike.sh): not run locally (the shared heavy lock was busy). In CI run 40137104086 onf5f31b8(conclusion success), PL-21 (Chromium over HTTPS, MODE=https) reported 23 passed, 1 skipped (the development-only HMR test), which includes the 9 members journeys and the new one.f5f31b8: success. Guard 36/36 and 73/73; unit 230/230; integration 140/140; backend 18/18; Vitest 88/88 (frontend 5/5); Playwright dev 19 passed; application stack 114/114; production-like 38/38 (PL-11: 4 passed, 15 skipped; PL-21 HTTPS: 23 passed, 1 skipped); container scan 14/14.Limitations
The members e2e spec needs a fresh seed per run: the removed memberships of Spare and Deputy cannot be added again, and a failure between an add and its removal leaves the user active.
stack.shandprodlike.shseed new users for every run (noted in the spec).A removed member cannot be re-added (the backend has no reactivation; see ADR-020).
No client-side request timeout: a request that never answers keeps the change pending until the browser or the edge ends it (then: unknown outcome).
A failed list load has no retry button (reloading the page reads it again); not added, it would be a new feature.
When the outcome of a change of one's own membership is unknown, the session is read again. The members list is still read again too. Elsewhere, the session is only read again after a 403 or 404.
When a load fails after a list was shown, the message for that change stays in view. When the reload leads to "Not found", the message for that change is not shown.
On the members page, a keyboard user who presses "Next" to reach the last page loses the focus, because the button becomes disabled. The organization page on main behaves the same. This is not changed here, because where the focus should go (or whether to use
aria-disabled) is a design decision.Updates
1b6ee8fmerges the feat(organizations): membership administration API (FND-010C) #19 review fixes (no force push);8159c7badds a 429 case with a wait in minutes and the seed note.20c4238,90ccc2b,4884df0: a 502/504 that is not an API problem is now an unknown outcome (message says so, list is reloaded) instead of "could not be saved"; while the shown list is older than the requested one (after a change or a page switch) the table isaria-busyand row controls are disabled, and caption/pager name the page actually shown; docs updated.86020a0: member table cells wrap anywhere so the page fits 320 px (WCAG 1.4.10).f7961c8: new Playwright journeys (MANAGER, foreign organization, ended session and sign-out, self-removal of the last ADMIN, narrow screen).965bb56: merges feat(organizations): membership administration API (FND-010C) #19 at3b5ea69(normal merge). feat(organizations): membership administration API (FND-010C) #19 moved the SPA text out ofdocs/membership-administration.md; this branch keeps feat(organizations): membership administration API (FND-010C) #19's API error table and adds the members page section and a separate "What the members page shows" table.15145fe: when the CSRF token cannot be refreshed after a 403csrf-invalid, the client throws that problem (the refused request was not processed and is not resent); the page says nothing was changed instead of "unknown outcome".fec06c2: during a removal the confirmation stays open (aria-busy, buttons disabled, focus on the group, Escape ignored); afterwards focus goes to the success status or the error message.b78128a: merges feat(organizations): membership administration API (FND-010C) #19 ata218579(docs: a 500 can follow a committed change); no conflict.114228c: every 5xx answer to a change, including the API's own 500 problem, is an unknown outcome: the page says it is not known whether the change was saved and reloads; 4xx handling unchanged; the guide's members-page table updated.324f97e: a failed list read after a change no longer hides the answer to that change; "the list was reloaded" became "the list is read again"; a missing answer is described as a failed connection.ddd0201: after a 403 (notcsrf-invalid) or 404 to a change the session is read again, so the switcher and the start page follow lost access.35e80b5: the keyboard-only demoted/removed-ADMIN and lost-answer journey;seed_e2ecreates the user Deputy.b1822e1,ef5aa2a,f5f31b8: merge feat(organizations): membership administration API (FND-010C) #19 at5e065b6,c0ea699and5c7567d(normal merges, no conflict);20a6610updates the reference to feat(organizations): membership administration API (FND-010C) #19's renamed "A 500 after the commit" section. The page still treats every 5xx as an unknown outcome: it cannot tell a rolled-back 500 from an ambiguous commit.8890bf0,bff316c: normal merges of feat(organizations): membership administration API (FND-010C) #19 (1565f72,8425567) with main fix(auth): match no account for an e-mail address with a NUL byte #28, fix(auth): end sessions when a user is disabled #26 and fix(frontend): keep the user signed in when signing out fails #32. The frontend merged cleanly: Vitest 93/93.b213459: if a change of one's own membership has an unknown outcome (no answer, an unreadable success answer, or a 5xx), the page now reads the session again. Before, a self-removal that was saved despite a lost answer led to "Not found", while the organization switcher still offered the organization and the start page sent the user straight back to "Not found". The docs (membership-administration.mdandarchitecture/authentication.md) were updated.9f1c563: new Playwright journey for fix(frontend): keep the user signed in when signing out fails #32 on the members page:GET /me200), and signing out again ends the session.8051d68: normal merge of feat(organizations): membership administration API (FND-010C) #19 at3534fe8(only backend tests and docs changed; no frontend change).Security considerations