Service-layer auth enforcement: Google sync actions (#419)#223
Service-layer auth enforcement: Google sync actions (#419)#223peterdrier merged 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2834de5007
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try | ||
| { | ||
| var groupResult = await _googleSyncService.EnsureTeamGroupAsync(id); | ||
| var groupResult = await _googleSyncService.EnsureTeamGroupAsync(id, User); |
There was a problem hiding this comment.
Avoid execute-only sync call in TeamsAdmin edit path
This action is still authorized for TeamsAdminBoardOrAdmin, but the new EnsureTeamGroupAsync(id, User) call now requires the Execute requirement (Admin/system only), so TeamsAdmin/Board edits deterministically throw UnauthorizedAccessException before any Google-group logic runs. In EditTeam, that exception is caught after UpdateTeamAsync has already persisted changes, so non-admin editors now get a spurious failure on every edit and prefix changes can be saved without the corresponding group sync being applied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — fixed in 19cabd9.
The handler was too coarse: it had only Preview (read-only) and Execute (everything else, Admin+system only). Per the docs/sections/Teams.md invariant, TeamsAdmin is explicitly authorized to "link/unlink Google resources on all teams" but cannot "execute sync actions" — so the two responsibilities needed separate tiers.
Changes:
- Added a
TeamResourcerequirement that TeamsAdmin + Board + Admin + system can satisfy - Moved
EnsureTeamGroupAsyncandProvisionTeamGroupAsync(called internally by the former) to useTeamResource Executestays Admin/system-only for workspace-wide sync actions (SyncResourcesByTypeAsync,UpdateDriveFolderPathsAsync,RemediateGroupSettingsAsync, etc.)- Added 5 handler tests for the new tier (TeamsAdmin/Board/Admin/system allowed, other admin roles denied)
The EditTeam and CreateTeam paths now satisfy the service-layer auth check for TeamsAdmin/Board editors, so the UpdateTeamAsync-then-catch silent drop you flagged no longer happens.
…lective#419) Push authorization into IGoogleSyncService and ISyncSettingsService so external Google Workspace API calls can never be made without a prior privilege check, regardless of call path. Mirrors the nobodies-collective#418 precedent. - New GoogleSyncOperationRequirement (Preview / Execute) with GoogleSyncAuthorizationHandler — Admin and SystemPrincipal always succeed; TeamsAdmin/Board are allowed Preview-only; everyone else is denied. - Every IGoogleSyncService method with external side effects now accepts ClaimsPrincipal and calls IAuthorizationService.AuthorizeAsync before any Google SDK call. Unauthorized callers raise UnauthorizedAccessException. - Preview actions (SyncResourcesByTypeAsync Preview, CheckGroupSettings, GetEmailMismatches, GetAllDomainGroups) require Preview; mutating actions require Execute. SyncResourcesByTypeAsync and SyncSingleResourceAsync pick the requirement based on SyncAction. - ISyncSettingsService.UpdateModeAsync is similarly guarded since it gates all downstream sync behavior. - Background jobs (SystemTeamSyncJob, GoogleResourceReconciliationJob, GoogleResourceProvisionJob, ProcessGoogleSyncOutboxJob, SuspendNonCompliantMembersJob) now pass SystemPrincipal.Instance. - Controllers (GoogleController, TeamController, TeamAdminController) forward User to the service — [Authorize(Policy = ...)] attributes remain as the first-line check (defense in depth). - GoogleAdminService.LinkGroupToTeamAsync and TeamResourceService.SetRestrictInheritedAccessAsync accept and forward a ClaimsPrincipal so the chain stays typed end-to-end. - Stub implementations updated for the new interface shape. Tests: - New GoogleSyncAuthorizationHandlerTests covering system override, Admin, TeamsAdmin/Board Preview-only, denial matrix. - New GoogleWorkspaceSyncServiceTests asserting every guarded method throws UnauthorizedAccessException for an unprivileged principal before touching the Google API, plus Preview-vs-Execute requirement selection and a null-principal guard. - SyncSettingsServiceTests adds a denied-auth throw case. - Existing job and service tests updated for the new signatures. - Full Humans.Application.Tests suite passes (997 total). Integration test failures are pre-existing Docker/testcontainers infra issues, unrelated to this change. Closes nobodies-collective#419 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2834de5 to
1b6b162
Compare
…ctive#419 review) Codex P1 finding on PR #223: EditTeam and CreateTeam are authorized for TeamsAdminBoardOrAdmin, but EnsureTeamGroupAsync now guards on the Execute requirement which only allows Admin/system. TeamsAdmin and Board editors get UnauthorizedAccessException on every edit, and because the exception is caught after UpdateTeamAsync has already persisted, prefix changes save without the corresponding group sync. The handler was too coarse. docs/sections/Teams.md invariant splits these responsibilities: - TeamsAdmin can "link/unlink Google resources on all teams" - TeamsAdmin cannot "execute sync actions" Add a TeamResource requirement tier that sits between Preview and Execute. The handler allows TeamsAdmin + Board + Admin + system for TeamResource operations. Execute stays Admin + system only for bulk sync and reconciliation. Change EnsureTeamGroupAsync and ProvisionTeamGroupAsync (called by the former) to use TeamResource. All other Execute-gated methods (SyncResourcesByType, UpdateDriveFolderPaths, RemediateGroupSettings, etc.) stay as Execute — those are workspace-wide sync actions. Added handler tests covering TeamsAdmin, Board, Admin, and system can manage team resources, and non-Teams admin roles cannot.
…-collective#419) (#223)" This reverts commit 1626098.
Summary
Phase 3b of the authorization transition plan. Pushes authorization checks into
IGoogleSyncServiceandISyncSettingsServiceso external Google Workspace API calls can never be made without a prior privilege check, regardless of call path. Mirrors the nobodies-collective#418 precedent exactly (resource-based handler +ClaimsPrincipalthreaded through service methods,SystemPrincipalfor jobs).Automated by
/execute-sprintbatch 3.Design
GoogleSyncOperationRequirementwith two operation classes: Preview (read-only Google API — list groups, check drift) and Execute (mutations — add/remove members, provision, remediate).GoogleSyncAuthorizationHandler:SystemPrincipal→ always succeeds (background jobs).Admin→ always succeeds for both Preview and Execute.TeamsAdmin/Board→ Preview-only (they can view the sync dashboard but not mutate).IGoogleSyncServicemethod with external side effects now takes aClaimsPrincipaland callsIAuthorizationService.AuthorizeAsyncat the top of the method, before touching any Google client. Unauthorized callers raiseUnauthorizedAccessException.SyncResourcesByTypeAsyncandSyncSingleResourceAsyncpick their requirement from theSyncActionargument so a Preview action only needs Preview privilege.ISyncSettingsService.UpdateModeAsyncis also guarded, since sync settings gate every downstream sync job.GoogleAdminService.LinkGroupToTeamAsyncandTeamResourceService.SetRestrictInheritedAccessAsyncaccept and forward aClaimsPrincipalso the chain stays typed end-to-end.SystemTeamSyncJob,GoogleResourceReconciliationJob,GoogleResourceProvisionJob,ProcessGoogleSyncOutboxJob,SuspendNonCompliantMembersJob) passSystemPrincipal.Instance.[Authorize(Policy = PolicyNames.AdminOnly)](and analogous) attributes — defense in depth, per.claude/DESIGN_RULES.md.Acceptance criteria
await Get*ServiceAsync()/ HTTP call)SystemPrincipal.Instance)Tests
GoogleSyncAuthorizationHandlerTests— 18 cases covering SystemPrincipal, Admin, TeamsAdmin/Board Preview-only, and denial matrix for every other admin role.GoogleWorkspaceSyncServiceTests— 21 cases: one per guarded public method assertingUnauthorizedAccessExceptionis thrown for an unprivileged principal and thatIAuthorizationService.AuthorizeAsyncis invoked exactly once; Preview-vs-Execute requirement selection; null-principalArgumentNullException.SyncSettingsServiceTests— newThrowsUnauthorized_WhenAuthorizationFailscase.Humans.Application.Testssuite: 997 passed, 0 failed.Test plan
Closes nobodies-collective#419
🤖 Generated with Claude Code