Skip to content

Conversation

@HouseinIsProgramming
Copy link
Collaborator

@HouseinIsProgramming HouseinIsProgramming commented Jan 9, 2026

Description

The AND/OR logic of the permissions may sometimes be misleading. This PR aims to clarify some of the confusion and adds a new method that uses ALL logic.

Breaking changes

None

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

@vercel
Copy link

vercel bot commented Jan 9, 2026

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

Project Deployment Review Updated (UTC)
docs Ready Ready Preview, Comment Jan 9, 2026 4:41pm
vendure-storybook Ready Ready Preview, Comment Jan 9, 2026 4:41pm

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 9, 2026

📝 Walkthrough

Walkthrough

Added OR-style permission checking behavior clarification and an explicit AND-style check to RequestContext: the existing userHasPermissions now checks if the user has at least one of the specified permissions (OR), and a new userHasAllPermissions method requires all specified permissions (AND). Tests were added to cover both behaviors, including scenarios with no session, missing channel permissions, and mixed permission sets. Documentation comments on the Allow decorator were updated to reflect OR semantics when multiple permissions are provided.

Changes

Cohort / File(s) Summary
Permission Logic Implementation
packages/core/src/api/common/request-context.ts, packages/core/src/api/common/request-context.spec.ts
Reordered imports; clarified userHasPermissions as OR logic; added userHasAllPermissions() implementing AND logic; added tests covering no session, missing channel permissions, mixed permissions, and various match cases; introduced createRequestContextWithPermissions test helper; updated imports to include Permission type.
Documentation
packages/core/src/api/decorators/allow.decorator.ts
Updated documentation comments to state that multiple permissions in the Allow decorator are treated with OR logic and added an example demonstrating multiple-permission usage.

Estimated code review effort

🎯 Medium | ⏱️ ~30 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main changes: expanding userHasPermissions documentation and adding a new userHasAllPermissions method.
Description check ✅ Passed The description covers the main purpose (clarifying AND/OR logic confusion), confirms no breaking changes, includes a completed checklist matching the template sections.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40a3dd6 and 3c22880.

📒 Files selected for processing (3)
  • packages/core/src/api/common/request-context.spec.ts
  • packages/core/src/api/common/request-context.ts
  • packages/core/src/api/decorators/allow.decorator.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/core/src/api/decorators/allow.decorator.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/api/common/request-context.ts (1)
packages/core/src/common/utils.ts (1)
  • idsAreEqual (38-43)
🪛 Gitleaks (8.30.0)
packages/core/src/api/common/request-context.spec.ts

[high] 265-265: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
  • GitHub Check: e2e tests (24.x, mysql)
  • GitHub Check: e2e tests (22.x, mariadb)
  • GitHub Check: codegen / codegen
  • GitHub Check: unit tests (22.x)
  • GitHub Check: build (20.x)
  • GitHub Check: build (22.x)
  • GitHub Check: unit tests (24.x)
  • GitHub Check: publish_install (ubuntu-latest, 24.x)
  • GitHub Check: publish_install (ubuntu-latest, 20.x)
  • GitHub Check: publish_install (macos-latest, 24.x)
  • GitHub Check: publish_install (windows-latest, 22.x)
  • GitHub Check: publish_install (macos-latest, 20.x)
  • GitHub Check: publish_install (windows-latest, 24.x)
  • GitHub Check: publish_install (ubuntu-latest, 22.x)
  • GitHub Check: publish_install (windows-latest, 20.x)
  • GitHub Check: publish_install (macos-latest, 22.x)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/core/src/api/common/request-context.spec.ts (4)

1-1: LGTM! Import addition supports new permission tests.

The Permission import is necessary for the new test suites and follows the existing import conventions.


133-161: LGTM! Comprehensive test coverage for OR logic.

The test suite thoroughly validates userHasPermissions OR semantics across all key scenarios: no session, empty permissions, partial matches, no matches, and single permission checks.


163-199: LGTM! Comprehensive test coverage for AND logic.

The test suite thoroughly validates userHasAllPermissions AND semantics. The empty array test (line 187-190) correctly expects true, which aligns with JavaScript's Array.every() behavior (vacuously true when checking all zero conditions are met).


245-285: LGTM! Well-designed test helper with clear purpose.

The createRequestContextWithPermissions helper cleanly constructs test contexts with specific permission sets. The static analysis warning at line 265 is a false positive—this is clearly a mock token constant used exclusively for testing, not a real credential.

packages/core/src/api/common/request-context.ts (4)

7-7: LGTM! Minor style improvement.

Reordering the TypeORM imports has no functional impact and improves consistency.


257-266: LGTM! Excellent documentation clarity.

The expanded docstring clearly articulates the OR semantics of userHasPermissions, cross-references the new AND method, and provides a concrete example. This directly addresses the confusion mentioned in the PR objectives.


268-278: LGTM! Implementation correctly enforces OR logic.

The method uses arraysIntersect (defined at lines 410-414) which checks if ANY element of the user's permissions appears in the required permissions array—correct OR semantics matching the updated documentation.


280-306: LGTM! New method correctly implements AND logic.

The userHasAllPermissions method is well-documented and correctly implements AND semantics using Array.every(). The implementation properly handles edge cases (no session, empty permissions) and aligns with the comprehensive test coverage. The @since 3.6.0 tag appropriately marks this as a new addition.


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.

@HouseinIsProgramming
Copy link
Collaborator Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 9, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@dlhck dlhck changed the base branch from minor to master January 9, 2026 15:30
@dlhck dlhck changed the base branch from master to minor January 9, 2026 15:30
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2026

@HouseinIsProgramming HouseinIsProgramming merged commit f2d0359 into master Jan 13, 2026
35 of 38 checks passed
@HouseinIsProgramming HouseinIsProgramming deleted the update-userhasperms branch January 13, 2026 16:05
@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants