Skip to content

Fix organization role controls and add member search - #4409

Open
paprikaf wants to merge 12 commits into
mainfrom
paprikaf/changes-7040
Open

Fix organization role controls and add member search#4409
paprikaf wants to merge 12 commits into
mainfrom
paprikaf/changes-7040

Conversation

@paprikaf

@paprikaf paprikaf commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Hide organization role editing from admins, matching the existing owner-only API policy. Owners can still promote/demote members; admins retain ordinary-member removal.
  • Enforce the shared removal policy in the API, including admin-to-admin removal returning 403 before any mutation.
  • Atomically authorize and claim removal targets in a short database transaction, with role predicates and exact affected-row counts before federation revocation. Guard concurrent role updates in Core and the Dispatch identity authority, preserving pending-removal retries and shared-database behavior.
  • Re-read active caller and target memberships under the same deterministic row locks at both mutation boundaries. Reject callers demoted, removed, or marked for removal after the initial authorization read, before claiming or mutating a target.
  • Carry the caller-observed expected member role in the signed federation assertion and validate it under the authority lock. Return 409 for stale updates instead of overwriting a newer authority role; even requests for the locally current role go through this check.
  • Make authorized authority removals idempotent when the target is already absent, and let owners/admins resume fully pending satellite removals through the original DELETE. Reauthorize each retry, preserve the marker, and reject mixed pending/active target rows.
  • Widen the removal-pending timestamp from INTEGER to BIGINT with a lossless migration so PostgreSQL can store millisecond markers.
  • Add email search beside Invite members, backed by server-side filtering across pages. Reset pagination when searching and keep query caches isolated by organization and search.
  • Apply the debounced search and page reset together so navigation during the debounce cannot skip the first matching page.
  • Show failed member loads with a Retry action instead of reporting empty results. Retry preserves the current query and page.
  • Include a Core patch changeset, corrected permissions documentation, regression coverage, and fictional example.test fixtures.

Verification

  • Latest review follow-up: 58 Dispatch identity tests and 12 Core migration tests pass. Authorized role-change replays return 200 without another write; conflicting desired roles, revoked callers, pending targets, and owner-only policy violations remain rejected. The SSO mock imports the member-lock helper directly from Core source rather than requiring fresh build output. Migration tests now use a scoped 30-second timeout. Dispatch typecheck and both localization guards pass.
  • Latest actor/precondition follow-up: 131 focused tests passed (78 Core handler/concurrency/federation/permissions/lock-helper tests and 53 Dispatch identity-authority tests). New regressions cover actor demotion/removal/pending state, ordered actor/target locks, signed expected roles, stale authority updates returning 409 without local writes, and no-op requests still validating at the authority. Core build/typecheck, Dispatch typecheck, both localization guards, and diff whitespace validation passed.
  • Follow-up for the fresh test review: scope a 30-second per-test timeout to the real-PGlite suite so database startup fits under load. The exact concurrency command without a CLI timeout override passed all 17 tests in 17.46 seconds; Core typecheck and formatting passed. Global test timeouts and runtime code are unchanged.
  • Latest retry follow-up: 98 focused tests passed (54 Core handler/concurrency/federation tests and 44 Dispatch identity-authority tests). Regressions cover lost authority responses, local cleanup failure, repeated removals, preserved pending markers, and authorization failures. The new authority replay tests failed with 404 before the fix and passed afterward.
  • Earlier verification passed 110 focused tests covering organization hooks, member controls, search recovery and pagination, permission helpers, API handlers, and concurrent member mutations.
  • Real PGlite regressions exercise conditional writes, rollback after partial claims, pending-removal guards, shared-authority writes, and timestamp migration preserving existing data while accepting millisecond markers.
  • The pagination regression failed before the fix with offset 25 instead of 0, then passed with the fix; fake timers control the debounce window.
  • Localization catalog, changed-copy, and additive-migration guards passed.
  • git diff --check passed.
  • Core build and Core and Dispatch typechecks passed.
  • Retry follow-up: Core typecheck and both localization guards passed; Dispatch typecheck passed with temporary non-deployed local build configuration.
  • The earlier UI review's React Doctor check exited successfully (84/100); six existing TeamPage warnings remain for component complexity/size, boolean props, and a string conversion outside this follow-up.

This changes shared Core UI consumed by Dispatch and the Core/Dispatch member mutation boundaries. No production roles or databases were modified, and no deployment was performed.

Federated role-update callers and the authority must be upgraded together: legacy assertions without the expected-role precondition fail closed rather than perform an unconditional role change.

builder-io-integration[bot]

This comment was marked as outdated.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Visual recap — not generated

The recap was suppressed because the diff matched a secret/credential pattern. No plan was published.

Reason: high-confidence secret in diff.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes — looks good ✅

Review Details

Incremental Code Review Summary

The latest PR head addresses all three previously reported issues: the Dispatch SSO test mock now exposes lockOrgMembersForMutation, authority role updates handle already-applied desired roles idempotently, and the PGlite migration/concurrency coverage has an explicit 30-second timeout. The updated implementation retains the fail-closed authorization and deterministic locking model, and the broader Core/Dispatch organization, federation, migration, member-search, and UI test coverage passed in focused runs. Four independent reviewers found no new confirmed authorization, concurrency, federation, migration, or search/cache defects.

Risk assessment: High, due to organization authorization, federated identity mutations, and concurrent database writes. The previously open review threads were verified fixed and resolved before this review. No new reportable issues were found.

🧪 Browser testing: Will run after this review (PR touches UI code) — the dev server was healthy, but all 19 planned cases were escalated because the executor sandbox could not discover or call Chrome browser-automation tools after the allowed retry.

@paprikaf
paprikaf requested a review from liamdebeasi September 5, 2026 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant