refactor(authorization): accept IUser in hasPermissionAsync wrappers#41340
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAuthorization contracts now accept user IDs or full user objects. Selected crowd and reaction permission checks pass full user objects, and ChangesAuthorization input migration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41340 +/- ##
===========================================
+ Coverage 68.98% 69.09% +0.10%
===========================================
Files 3757 3759 +2
Lines 147735 147859 +124
Branches 26381 26477 +96
===========================================
+ Hits 101919 102164 +245
+ Misses 41317 41208 -109
+ Partials 4499 4487 -12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/jira ARCH-2200 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/definition/externals/meteor/meteor.d.ts`:
- Around line 52-55: Update the userAsync declaration with overloads: keep
no-argument calls returning IUser | null, and add a fields/options overload
returning a partial user type that cannot be treated as a complete IUser. Ensure
projected calls preserve nullable typing while preventing access to
complete-user-only properties such as roles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fb26153-1e4b-436e-9a0f-5945b68be5e4
📒 Files selected for processing (5)
apps/meteor/app/crowd/server/methods.tsapps/meteor/app/reactions/server/setReaction.tsapps/meteor/definition/externals/meteor/meteor.d.tsapps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/crowd/server/methods.tsapps/meteor/app/reactions/server/setReaction.tsapps/meteor/definition/externals/meteor/meteor.d.tsapps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/crowd/server/methods.tsapps/meteor/app/reactions/server/setReaction.tsapps/meteor/definition/externals/meteor/meteor.d.tsapps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/crowd/server/methods.tsapps/meteor/app/reactions/server/setReaction.tsapps/meteor/definition/externals/meteor/meteor.d.tsapps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/crowd/server/methods.tsapps/meteor/app/reactions/server/setReaction.tsapps/meteor/definition/externals/meteor/meteor.d.tsapps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
🔇 Additional comments (5)
apps/meteor/definition/externals/meteor/meteor.d.ts (1)
2-2: LGTM!apps/meteor/server/lib/authorization/hasPermission.ts (1)
4-21: LGTM!packages/core-services/src/types/IAuthorization.ts (1)
13-24: LGTM!apps/meteor/app/crowd/server/methods.ts (1)
27-27: LGTM! Both call sites correctly narrowuserto non-null before forwarding it tohasPermissionAsync. Note: correctness here depends onMeteor.userAsync()reliably returning a fullIUserwithroles, which is flagged for verification at its type declaration inapps/meteor/definition/externals/meteor/meteor.d.ts(lines 52-55).Also applies to: 59-59
apps/meteor/app/reactions/server/setReaction.ts (1)
34-43: 🎯 Functional CorrectnessNo action needed for
setReactioncallers
setReactionis only called fromexecuteSetReaction, which loads the user withUsers.findOneById(userId)before passing it through. No other in-repo callers pass a projected user here.> Likely an incorrect or invalid review comment.
ggazzo
force-pushed
the
chore/authz-haspermission-accept-user
branch
from
4dc8580 to
7fa87b7
Compare
|
Thanks for the review @sampaiodiego! Addressed:
The reason for the |
Authorization already resolves roles from a full IUser, so passing one lets it skip an internal Users.findOneById. Widen the hasPermissionAsync / hasAllPermissionAsync / hasAtLeastOnePermissionAsync wrappers to `string | IUser` and start migrating call sites that already hold a complete user document. - IAuthorization: collapse the `IUser` / `@deprecated string` overload pairs into a single `string | IUser` signature so a union argument resolves without per-call narrowing. - Type Meteor.userAsync() as Promise<IUser | null>: RC stores full user documents and every in-repo caller is no-arg, so roles are always present. - Migrate crowd methods and setReaction as the first sites. Remaining string-only call sites are intentionally left for follow-ups. TypeScript rejects rolesless projections, preventing silent zero-permission results.
ggazzo
force-pushed
the
chore/authz-haspermission-accept-user
branch
from
7fa87b7 to
b2cfa5a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core-services/src/types/IAuthorization.ts (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
IUser['_id']instead of barestringfor the user parameter.The wrapper functions in
hasPermission.tstype the same parameter asIUser['_id'] | IUser, while this interface usesstring | IUser. Both resolve to the same structural type, butIUser['_id']is more semantically precise and keeps the interface aligned with its callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core-services/src/types/IAuthorization.ts` around lines 10 - 12, Update the user parameter types in hasAllPermission, hasPermission, and hasAtLeastOnePermission to use IUser['_id'] | IUser instead of string | IUser, keeping the interface aligned with the wrapper functions in hasPermission.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core-services/src/types/IAuthorization.ts`:
- Around line 10-12: Update the user parameter types in hasAllPermission,
hasPermission, and hasAtLeastOnePermission to use IUser['_id'] | IUser instead
of string | IUser, keeping the interface aligned with the wrapper functions in
hasPermission.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5cf7bd3-cd65-42af-a642-424f150fcf09
📒 Files selected for processing (5)
apps/meteor/app/crowd/server/methods.tsapps/meteor/app/reactions/server/setReaction.tsapps/meteor/definition/externals/meteor/meteor.d.tsapps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/app/reactions/server/setReaction.ts
- apps/meteor/definition/externals/meteor/meteor.d.ts
- apps/meteor/app/crowd/server/methods.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (2)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/lib/authorization/hasPermission.tspackages/core-services/src/types/IAuthorization.ts
🔇 Additional comments (1)
apps/meteor/server/lib/authorization/hasPermission.ts (1)
5-18: LGTM!
ggazzo
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
2 participants
Proposal
Authorization.hasPermission/hasAllPermission/hasAtLeastOnePermissionalready acceptstring | IUser. When a full user is passed, the service skips an internalUsers.findOneByIdinsidegetRoles— saving a DB find per permission check on hot paths where the user is already in hand.The
hasPermissionAsyncwrappers only typeduserId: string, blocking that optimization. This widens them and starts migrating call sites one by one.Changes
IAuthorization: add astring | IUseroverload, declared last so the existing@deprecatedstring overload still flags plain-uid callers (keeps the migration nudge).hasPermission.tswrappers:hasPermissionAsync/hasAllPermissionAsync/hasAtLeastOnePermissionAsyncnow acceptIUser['_id'] | IUser, plain passthrough.Meteor.userAsync()typedPromise<IUser | null>: RC stores full user documents and all in-repo callers are no-arg (no field projection), sorolesis always present at runtime.crowd/server/methods.ts,reactions/server/setReaction.ts.Safety
getRolesreadsuser.rolesdirectly.IUser.rolesis required, so TypeScript rejects passing a rolesless projection — preventing a silent zero-permission result. Only sites where the user is provably a completeIUserare migrated; the type checker gates the rest.Follow-ups
X._idsites holding a fullIUser— migrate incrementally.getRolesCachedon_idinstead ofJSON.stringify(user)so object and string callers share cache entries.Task: ARCH-2248
Summary by CodeRabbit
Bug Fixes
Enhancements
Meteor.userAsync()to retrieve the current signed-in user asynchronously (Promiseof the user ornull).