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) <noreply@anthropic.com>
Contributor
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
|
View your CI Pipeline Execution ↗ for commit 265f92d
☁️ Nx Cloud last updated this comment at |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis pull request adds authorization validation to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
Contributor
|
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
…-for-team-creator-not-for-other
…tation (NES-1489) The frontend uses journeyVisitorExportToGoogleSheet (not googleSheetsSyncCreate), so the original PR targeted the wrong mutation. This commit: - Reverts googleSheetsSyncCreate to main (original isIntegrationOwner guard was correct) - Adds integration ownership + team-scoping checks to journeyVisitorExportToGoogleSheet - Moves integration validation before Google API token fetch for defense-in-depth - Updates plan document with revised root cause analysis Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jianwei1
changed the title
…-for-team-creator-not-for-other
…-for-team-creator-not-for-other
…-for-team-creator-not-for-other
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
journeyVisitorExportToGoogleSheet(notgoogleSheetsSyncCreate). The original PR targeted the wrong mutation.userId) and team-scoping (teamId) checks tojourneyVisitorExportToGoogleSheet— the mutation the frontend actually uses. Previously, any authenticated team member could create syncs using another user's Google account via direct API calls.googleSheetsSyncCreateback tomain— its originalisIntegrationOwnerwithAuth guard was already correct. This mutation is not called by any frontend code.What changed
googleSheetsSyncCreate.mutation.tsmain(original auth was correct)googleSheetsSyncCreate.mutation.spec.tsmainjourneyVisitorExportToGoogleSheet.mutation.tsjourneyVisitorExportToGoogleSheet.mutation.spec.tsPermission model after fix
journeyVisitorExportToGoogleSheet(the mutation the frontend calls):isAuthenticated— must be logged inability(Action.Export, ...)— must be team manager/member or journey ownerintegration.teamId === journey.teamId— integration must belong to the journey's teamintegration.userId === context.user.id— must own the integration (privacy: only use your own Google account)How it works with PR #8958
Together, these PRs enable any team member to create Google syncs:
Test plan
googleSheetsSyncCreatetests pass (7 tests — reverted to main)journeyVisitorExportToGoogleSheettests pass (16 tests — 2 new)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation