Skip to content

Comments

feat: event type pbac#22618

Merged
sean-brydon merged 97 commits intomainfrom
feat/event-type-pbac
Sep 17, 2025
Merged

feat: event type pbac#22618
sean-brydon merged 97 commits intomainfrom
feat/event-type-pbac

Conversation

@sean-brydon
Copy link
Member

@sean-brydon sean-brydon commented Jul 18, 2025

Seed your database by "yarn seed-pbac"

Look at the roles created in orgs/teams roles and permissions in settings.

Create new roles/edit old ones to remove all permission from event type - assign it to a user.
Impersonate the user and try to CRUD an event type.
Tick on permissions in the role and keep testing the event type.

Impersonate a team without pbac enabled and ensure everything works as expected

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jul 18, 2025

Walkthrough

This PR integrates PBAC across event types and workflows. It introduces server-side permission checks via PermissionCheckService/getResourcePermissions, a createEventPbacProcedure wrapper for TRPC mutations, and new use cases/utilities (TeamAccessUseCase, EventGroupBuilder, ProfilePermissionProcessor, filter/permission/transform utils). Event type routers (delete/update/duplicate) now use PBAC, and handlers for event types/workflows enforce read/create/update permissions with role fallbacks. Frontend adds an EventPermissionProvider, permission hooks/guards, and gates UI (workflows tab, actions) by permissions. Settings permission UI switches to scope-aware registries. Tests added for new utilities and handlers. No public API shape changes except minor prop additions/defaults and a context type inclusion.

Possibly related PRs

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "feat: event type pbac" concisely captures the primary change — adding permission-based access control for event types. The provided diffs and PR objectives show wide-ranging PBAC work across server, client, tests, and migrations, so the title is accurate and focused. It is clear enough for teammates scanning the commit/PR history.
Description Check ✅ Passed The PR description contains step-by-step test instructions (seed DB, modify roles, impersonate users/teams) that are directly related to the event-type PBAC changes in the diff. Although it emphasizes manual testing rather than a high-level summary of code changes, it remains on-topic and therefore satisfies this lenient description check. It provides useful QA steps for reviewers.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • 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 4f120f6 and a310077.

📒 Files selected for processing (1)
  • apps/web/modules/event-types/views/event-types-listing-view.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/web/modules/event-types/views/event-types-listing-view.tsx
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/event-type-pbac

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

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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 18, 2025

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "[DO NOT MERGE] feat: event type pbac". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@github-actions github-actions bot added the ❗️ migrations contains migration files label Jul 18, 2025
@keithwillcode keithwillcode added consumer core area: core, team members only labels Jul 18, 2025
@delve-auditor
Copy link

delve-auditor bot commented Jul 18, 2025

No security or compliance issues detected. Reviewed everything up to 1bfed57.

Security Overview
  • 🔎 Scanned files: 29 changed file(s)
Detected Code Changes
Change Type Relevant files
Enhancement ► attributes-list-view.tsx
    Add permission checks for organization attributes
► resource-permissions.ts
    Implement PBAC resource permissions logic
► organizations/update.handler.ts
    Update organization permissions handling
Configuration changes ► permission-registry.ts
    Add attribute permissions configuration
► common.json
    Add new permission translations
Refactor ► page.tsx files
    Update organization settings pages with permission checks
► teamAccessUseCase.ts
    Extract team access logic

Reply to this PR with @delve-auditor followed by a description of what change you want and we'll auto-submit a change to this PR to implement it.

@sean-brydon sean-brydon changed the title Feat/event type pbac [DO NOT MERGE] feat: event type pbac Jul 18, 2025
@sean-brydon sean-brydon force-pushed the feat/event-type-pbac branch from a1d62f0 to a4a0da4 Compare July 18, 2025 09:46
Copy link
Contributor

@volnei volnei left a comment

Choose a reason for hiding this comment

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

Nit comments, other than that, looks great!

});

if (!hasMembership) {
console.warn(`User ${userId} is not a member of team ${teamId}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this console?

throw new TRPCError({ code: "UNAUTHORIZED" });
}

const isSystemAdmin = ctx.user.role === "ADMIN";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have constants for roles?

`PBAC check failed for user ${userId} on team ${teamId}, falling back to role check:`,
error
);
hasCreatePermission = ["ADMIN", "OWNER"].includes(hasMembership.role);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have constants for permissions?

userId,
teamId: membership.team.id,
permission: "eventType.read",
fallbackRoles: ["ADMIN", "OWNER", "MEMBER"], // All roles can read event types by default
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use Membership constants here?

@github-actions github-actions bot marked this pull request as draft September 16, 2025 18:55
@sean-brydon
Copy link
Member Author

Nit comments, other than that, looks great!

Will address these in a follow up! Lets get this merged

@sean-brydon sean-brydon marked this pull request as ready for review September 17, 2025 09:25
@sean-brydon sean-brydon merged commit 76ced2d into main Sep 17, 2025
66 of 69 checks passed
@sean-brydon sean-brydon deleted the feat/event-type-pbac branch September 17, 2025 09:25
emrysal added a commit that referenced this pull request Sep 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

consumer core area: core, team members only ❗️ .env changes contains changes to env variables event-types area: event types, event-types ✨ feature New feature or request ❗️ migrations contains migration files ready-for-e2e size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants