fix: allow team managers to create Google Sheets syncs (NES-1489)#8909
Open
fix: allow team managers to create Google Sheets syncs (NES-1489)#8909
Conversation
Replace isIntegrationOwner withAuth guard with isAuthenticated + resolver-level permission check (isIntegrationOwner || isTeamManager), matching the pattern used by googleSheetsSyncDelete and googleSheetsSyncBackfill. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
|
View your CI Pipeline Execution ↗ for commit 3735098
☁️ Nx Cloud last updated this comment at |
Contributor
WalkthroughAuthorization checks for Google Sheets sync creation are refactored from middleware-level constraints to explicit resolver logic. The resolver now permits access if the user is either the integration owner or a team manager, with comprehensive test coverage for each authorization scenario. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apis/api-journeys-modern/src/schema/googleSheetsSync/googleSheetsSyncCreate.mutation.ts (1)
44-57: Consider extracting the manager-or-owner check into a shared helper.This permission block now mirrors the same logic used in the other Google Sheets sync mutations, so centralizing it would make future auth changes less likely to drift between create/delete/backfill.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/schema/googleSheetsSync/googleSheetsSyncCreate.mutation.ts` around lines 44 - 57, Extract the duplicated permission check into a shared helper (e.g., isManagerOrOwner(userId, journey, googleIntegration)) that returns a boolean; replace the inline logic that computes isTeamManager/isIntegrationOwner in googleSheetsSyncCreate (and mirror in other Google Sheets sync mutations) with a call to that helper, and keep the same GraphQLError('Forbidden', { extensions: { code: 'FORBIDDEN' } }) throw when the helper returns false; ensure the helper checks journey.team?.userTeams for a userTeam with matching userId and role === 'manager' and verifies googleIntegration.userId === userId so behavior of the existing variables (isTeamManager, isIntegrationOwner) is preserved.apis/api-journeys-modern/src/schema/googleSheetsSync/googleSheetsSyncCreate.mutation.spec.ts (1)
243-300: Consider adding a manager-without-export-permission case.The new branch introduced here is “team manager but not integration owner,” but the existing export-denied test still only exercises the owner path. Adding the same manager fixture with
mockAbility.mockReturnValue(false)would lock down that the new branch still respects the export gate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/schema/googleSheetsSync/googleSheetsSyncCreate.mutation.spec.ts` around lines 243 - 300, Add a new test that covers the "manager without export permission" path: reuse the same mockJourney and mockIntegration fixtures from the existing case but set mockAbility.mockReturnValue(false) (or mockReturnValueOnce) to simulate export permission denied, invoke authClient with GOOGLE_SHEETS_SYNC_CREATE_MUTATION and the same variables, then assert the mutation is rejected (e.g., data.googleSheetsSyncCreate is null and errors contain the permission/authorization message) and verify prismaMock.googleSheetsSync.create was not called; this ensures the code paths in the googleSheetsSyncCreate resolver enforce the export gate for managers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apis/api-journeys-modern/src/schema/googleSheetsSync/googleSheetsSyncCreate.mutation.spec.ts`:
- Around line 243-300: Add a new test that covers the "manager without export
permission" path: reuse the same mockJourney and mockIntegration fixtures from
the existing case but set mockAbility.mockReturnValue(false) (or
mockReturnValueOnce) to simulate export permission denied, invoke authClient
with GOOGLE_SHEETS_SYNC_CREATE_MUTATION and the same variables, then assert the
mutation is rejected (e.g., data.googleSheetsSyncCreate is null and errors
contain the permission/authorization message) and verify
prismaMock.googleSheetsSync.create was not called; this ensures the code paths
in the googleSheetsSyncCreate resolver enforce the export gate for managers.
In
`@apis/api-journeys-modern/src/schema/googleSheetsSync/googleSheetsSyncCreate.mutation.ts`:
- Around line 44-57: Extract the duplicated permission check into a shared
helper (e.g., isManagerOrOwner(userId, journey, googleIntegration)) that returns
a boolean; replace the inline logic that computes
isTeamManager/isIntegrationOwner in googleSheetsSyncCreate (and mirror in other
Google Sheets sync mutations) with a call to that helper, and keep the same
GraphQLError('Forbidden', { extensions: { code: 'FORBIDDEN' } }) throw when the
helper returns false; ensure the helper checks journey.team?.userTeams for a
userTeam with matching userId and role === 'manager' and verifies
googleIntegration.userId === userId so behavior of the existing variables
(isTeamManager, isIntegrationOwner) is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 250abc16-e81c-4e45-b48b-0245ff2027b4
📒 Files selected for processing (2)
apis/api-journeys-modern/src/schema/googleSheetsSync/googleSheetsSyncCreate.mutation.spec.tsapis/api-journeys-modern/src/schema/googleSheetsSync/googleSheetsSyncCreate.mutation.ts
…-for-team-creator-not-for-other
|
Merge conflict attempting to merge this into stage. Please fix manually. |
…-for-team-creator-not-for-other
stage-branch-merger bot
added a commit
that referenced
this pull request
Mar 30, 2026
…reator-not-for-other' (PR #8909) into stage
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
1 participant
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.
Summary
googleSheetsSyncCreatemutation only allowed the integration creator to create syncs, blocking all other team members including managersisIntegrationOwnerwas used as a hardwithAuthguard, rejecting non-creators before the resolver even ranisAuthenticatedguard + resolver-levelisIntegrationOwner || isTeamManagercheck, matching the pattern already used bygoogleSheetsSyncDeleteandgoogleSheetsSyncBackfillPermission Model After Fix
Test Plan
Post-Deploy Monitoring & Validation
No additional operational monitoring required: this is a permission-model fix that aligns one mutation with the existing pattern used by sibling mutations. No new infrastructure, no new API surface.
Closes NES-1489
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 (1M context) [email protected]
Summary by CodeRabbit