Skip to content

Comments

refactor: do a permission check in removeHostsFromEventTypes trpc handler#24176

Merged
hbjORbj merged 2 commits intomainfrom
refactor/more-pbac-replacements-3
Sep 30, 2025
Merged

refactor: do a permission check in removeHostsFromEventTypes trpc handler#24176
hbjORbj merged 2 commits intomainfrom
refactor/more-pbac-replacements-3

Conversation

@hbjORbj
Copy link
Contributor

@hbjORbj hbjORbj commented Sep 30, 2025

What does this PR do?

  • Use a permission check service in removeHostsFromEventTypes trpc handler!

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A - I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Please use the latest Vercel preview and test please 🙏.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 30, 2025

Walkthrough

  • In packages/trpc/server/routers/viewer/organizations/removeHostsFromEventTypes.handler.ts, the legacy admin check is replaced with PermissionCheckService.
  • The handler now checks permission "eventType.update" with fallback roles OWNER and ADMIN.
  • If permission is denied, it responds with HTTP 401 and a specific denial message.
  • Handling for missing organizationId remains unchanged (unauthorized path retained).
  • The Prisma deleteMany operation that removes hosts from event types is unchanged.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the core change by indicating a refactor to add a permission check in the specific removeHostsFromEventTypes tRPC handler, using clear, concise language without unnecessary details.
Description Check ✅ Passed The pull request description directly explains the use of a permission check service in the removeHostsFromEventTypes handler and includes relevant testing instructions and mandatory task confirmations, making it clearly related to the changeset.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/more-pbac-replacements-3

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.

❤️ Share
🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

@graphite-app graphite-app bot requested review from a team September 30, 2025 12:36
@keithwillcode keithwillcode added core area: core, team members only foundation labels Sep 30, 2025
@hbjORbj hbjORbj changed the title refactor: more pbac replacements 3 refactor: do a permission check in removeHostsFromEventTypes trpc han… Sep 30, 2025
@hbjORbj hbjORbj changed the title refactor: do a permission check in removeHostsFromEventTypes trpc han… refactor: do a permission check in removeHostsFromEventTypes trpc handler Sep 30, 2025
@hbjORbj hbjORbj enabled auto-merge (squash) September 30, 2025 12:38
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/trpc/server/routers/viewer/organizations/removeHostsFromEventTypes.handler.ts (1)

38-47: Ensure eventTypeIds belong to the user’s organization before deletion.
While you check the user’s organizationId and update permission, the deleteMany call itself isn’t scoped to the org—any eventTypeId passed could delete hosts on another org’s event type. Pre-validate all input.eventTypeIds against ctx.user.organizationId (e.g. count matching eventTypes with that orgId and compare to eventTypeIds.length, throwing if they differ) before invoking prisma.host.deleteMany.

🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/organizations/removeHostsFromEventTypes.handler.ts (1)

50-50: Prefer named export over default export.

The handler is already exported as a named export (line 17), making the default export redundant. Named exports provide better tree-shaking and clearer imports.

Apply this diff to remove the default export:

-export default removeHostsFromEventTypesHandler;
-
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 474d157 and dcc7a9f.

📒 Files selected for processing (1)
  • packages/trpc/server/routers/viewer/organizations/removeHostsFromEventTypes.handler.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/trpc/server/routers/viewer/organizations/removeHostsFromEventTypes.handler.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • packages/trpc/server/routers/viewer/organizations/removeHostsFromEventTypes.handler.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/trpc/server/routers/viewer/organizations/removeHostsFromEventTypes.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/organizations/removeHostsFromEventTypes.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (2)
  • PermissionCheckService (19-306)
  • hasPermission (183-201)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
packages/trpc/server/routers/viewer/organizations/removeHostsFromEventTypes.handler.ts (2)

1-3: LGTM! Imports are appropriate for PBAC refactor.

The new imports for PermissionCheckService and MembershipRole are correctly added to support the permission-based access control implementation.


20-34: No action required: ‘eventType.update’ is the correct permission. All host-management handlers use this existing permission and no more granular permission is defined in the PBAC model.

@vercel
Copy link

vercel bot commented Sep 30, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
cal Ignored Ignored Sep 30, 2025 1:01pm
cal-eu Ignored Ignored Sep 30, 2025 1:01pm

@hbjORbj hbjORbj merged commit 4118a86 into main Sep 30, 2025
35 of 36 checks passed
@hbjORbj hbjORbj deleted the refactor/more-pbac-replacements-3 branch September 30, 2025 13:30
@github-actions
Copy link
Contributor

E2E results are ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants