Skip to content

refactor(authorization): accept a minimal user shape (UserWithRoles) in permission checks - #41351

Merged
ggazzo merged 1 commit into
developfrom
chore/authz-userwithroles-partial-migration
Jul 14, 2026
Merged

refactor(authorization): accept a minimal user shape (UserWithRoles) in permission checks#41351
ggazzo merged 1 commit into
developfrom
chore/authz-userwithroles-partial-migration

Conversation

@ggazzo

@ggazzo ggazzo commented Jul 13, 2026

Copy link
Copy Markdown
Member

What

Follow-up to accepting IUser in permission checks. Adds a minimal named type and migrates the sites that hold a partial user projection (which the previous string | IUser signature rejected).

Core change

getRoles only ever reads user._id and user.roles, so requiring a full IUser was overkill. New type in IAuthorization:

export type UserWithRoles = Pick<IUser, '_id' | 'roles'>;

Authorization.hasPermission / hasAllPermission / hasAtLeastOnePermission and the hasPermissionAsync helpers now take string | UserWithRoles. Full IUser stays compatible, so nothing that already passes a user breaks.

Migrated sites

  • Meteor.userAsync() (full doc at runtime, only the static type was narrow): pushConfig, removeChannelLinks, room publications, loadHistory.
  • canAccessRoom: pass the user it already re-fetches to a full document.
  • Widened partial param types to carry roles, then pass the object: canDeleteMessage, eraseRoom (+ the eraseTeamShared generic constraints it flows into), browseChannels.
  • browseChannels REST caller (api/v1/misc.ts) now projects roles next to __rooms.

Each partial site that now passes an object trades 2 DB lookups for 1 (its own fetch + the check's internal findOneById → just its own fetch, widened to include roles).

Left on the string path

livechat/roomAccessValidator.compatibility receives Pick<IUser, '_id'> from its caller — no roles available, and fetching them would add the lookup this change avoids.

Verification

Every touched file typechecks clean (serena diagnostics), no as casts. IUser.roles being required means TS still blocks any rolesless object — the safety gate is intact.

Review in cubic

Task: ARCH-2248

Summary by CodeRabbit

  • Improvements
    • Permission checks now use available user role information for more accurate authorization decisions.
    • Channel browsing and directory filtering apply role-aware access controls, including separate checks for viewing users and rooms.
    • Room, team, message-history, push notification, and integration actions now consistently enforce permissions using the authenticated user context.

@dionisio-bot

dionisio-bot Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot

changeset-bot Bot commented Jul 13, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 615a68e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1f1c73b7-a6ad-4ff2-b407-2b4f2bb1e959

📥 Commits

Reviewing files that changed from the base of the PR and between 460858e and 615a68e.

📒 Files selected for processing (13)
  • apps/meteor/server/api/lib/eraseTeam.ts
  • apps/meteor/server/api/v1/misc.ts
  • apps/meteor/server/bridges/slack/removeChannelLinks.ts
  • apps/meteor/server/lib/authorization/hasPermission.ts
  • apps/meteor/server/lib/eraseRoom.ts
  • apps/meteor/server/lib/pushConfig.ts
  • apps/meteor/server/meteor-methods/messages/loadHistory.ts
  • apps/meteor/server/meteor-methods/rooms/browseChannels.ts
  • apps/meteor/server/publications/room/index.ts
  • apps/meteor/server/services/authorization/canAccessRoom.ts
  • apps/meteor/server/services/authorization/service.ts
  • packages/core-services/src/index.ts
  • packages/core-services/src/types/IAuthorization.ts
📜 Recent 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/server/lib/eraseRoom.ts
  • apps/meteor/server/bridges/slack/removeChannelLinks.ts
  • apps/meteor/server/services/authorization/canAccessRoom.ts
  • apps/meteor/server/api/v1/misc.ts
  • apps/meteor/server/meteor-methods/messages/loadHistory.ts
  • apps/meteor/server/publications/room/index.ts
  • apps/meteor/server/api/lib/eraseTeam.ts
  • apps/meteor/server/lib/pushConfig.ts
  • packages/core-services/src/index.ts
  • packages/core-services/src/types/IAuthorization.ts
  • apps/meteor/server/services/authorization/service.ts
  • apps/meteor/server/lib/authorization/hasPermission.ts
  • apps/meteor/server/meteor-methods/rooms/browseChannels.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/eraseRoom.ts
  • apps/meteor/server/bridges/slack/removeChannelLinks.ts
  • apps/meteor/server/services/authorization/canAccessRoom.ts
  • apps/meteor/server/api/v1/misc.ts
  • apps/meteor/server/meteor-methods/messages/loadHistory.ts
  • apps/meteor/server/publications/room/index.ts
  • apps/meteor/server/api/lib/eraseTeam.ts
  • apps/meteor/server/lib/pushConfig.ts
  • packages/core-services/src/index.ts
  • packages/core-services/src/types/IAuthorization.ts
  • apps/meteor/server/services/authorization/service.ts
  • apps/meteor/server/lib/authorization/hasPermission.ts
  • apps/meteor/server/meteor-methods/rooms/browseChannels.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/eraseRoom.ts
  • apps/meteor/server/bridges/slack/removeChannelLinks.ts
  • apps/meteor/server/services/authorization/canAccessRoom.ts
  • apps/meteor/server/api/v1/misc.ts
  • apps/meteor/server/meteor-methods/messages/loadHistory.ts
  • apps/meteor/server/publications/room/index.ts
  • apps/meteor/server/api/lib/eraseTeam.ts
  • apps/meteor/server/lib/pushConfig.ts
  • packages/core-services/src/index.ts
  • packages/core-services/src/types/IAuthorization.ts
  • apps/meteor/server/services/authorization/service.ts
  • apps/meteor/server/lib/authorization/hasPermission.ts
  • apps/meteor/server/meteor-methods/rooms/browseChannels.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/eraseRoom.ts
  • apps/meteor/server/bridges/slack/removeChannelLinks.ts
  • apps/meteor/server/services/authorization/canAccessRoom.ts
  • apps/meteor/server/api/v1/misc.ts
  • apps/meteor/server/meteor-methods/messages/loadHistory.ts
  • apps/meteor/server/publications/room/index.ts
  • apps/meteor/server/api/lib/eraseTeam.ts
  • apps/meteor/server/lib/pushConfig.ts
  • packages/core-services/src/index.ts
  • packages/core-services/src/types/IAuthorization.ts
  • apps/meteor/server/services/authorization/service.ts
  • apps/meteor/server/lib/authorization/hasPermission.ts
  • apps/meteor/server/meteor-methods/rooms/browseChannels.ts
🔇 Additional comments (13)
packages/core-services/src/types/IAuthorization.ts (1)

3-15: LGTM!

packages/core-services/src/index.ts (1)

7-7: LGTM!

Also applies to: 141-141

apps/meteor/server/services/authorization/service.ts (1)

1-1: LGTM!

Also applies to: 59-73, 163-163, 175-175, 186-186

apps/meteor/server/services/authorization/canAccessRoom.ts (1)

9-15: LGTM!

apps/meteor/server/api/lib/eraseTeam.ts (1)

10-12: LGTM!

apps/meteor/server/api/v1/misc.ts (1)

470-470: LGTM!

apps/meteor/server/lib/authorization/hasPermission.ts (1)

2-16: LGTM!

apps/meteor/server/lib/eraseRoom.ts (1)

11-37: LGTM!

apps/meteor/server/meteor-methods/rooms/browseChannels.ts (1)

49-60: LGTM!

Also applies to: 123-127, 250-264, 303-305

apps/meteor/server/bridges/slack/removeChannelLinks.ts (1)

24-24: LGTM!

apps/meteor/server/lib/pushConfig.ts (1)

49-49: LGTM!

apps/meteor/server/meteor-methods/messages/loadHistory.ts (1)

57-57: LGTM!

apps/meteor/server/publications/room/index.ts (1)

93-93: LGTM!


Walkthrough

Authorization permission APIs now accept a minimal UserWithRoles shape. Server call sites pass full user objects where available, and related room, team, directory, and deletion helpers require roles.

Changes

Authorization user-shape migration

Layer / File(s) Summary
Permission type contracts
packages/core-services/src/types/IAuthorization.ts, packages/core-services/src/index.ts
Defines and re-exports UserWithRoles, and updates authorization interface methods to accept it.
Authorization service and helpers
apps/meteor/server/services/authorization/service.ts, apps/meteor/server/lib/authorization/hasPermission.ts, apps/meteor/server/services/authorization/canAccessRoom.ts
Updates authorization method types and passes role-bearing user objects through permission evaluation.
Authorization-aware call sites
apps/meteor/server/lib/eraseRoom.ts, apps/meteor/server/api/lib/eraseTeam.ts, apps/meteor/server/meteor-methods/rooms/browseChannels.ts, apps/meteor/server/api/v1/misc.ts, apps/meteor/server/bridges/slack/removeChannelLinks.ts, apps/meteor/server/lib/pushConfig.ts, apps/meteor/server/meteor-methods/messages/loadHistory.ts, apps/meteor/server/publications/room/index.ts
Passes full users to permission checks and includes roles in related user projections and type constraints.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested labels: type: chore

Suggested reviewers: sampaiodiego, gabriellsh

🚥 Pre-merge checks | ✅ 4 | ❌ 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 (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: authorization checks now accept a minimal UserWithRoles shape.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • ARCH-2248: Request failed with status code 401

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.

@ggazzo
ggazzo force-pushed the chore/authz-userwithroles-partial-migration branch from 561c458 to f285a4b Compare July 13, 2026 22:35
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.07%. Comparing base (460858e) to head (615a68e).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #41351      +/-   ##
===========================================
- Coverage    69.12%   69.07%   -0.05%     
===========================================
  Files         3762     3762              
  Lines       147936   147936              
  Branches     26428    26453      +25     
===========================================
- Hits        102261   102190      -71     
- Misses       41185    41258      +73     
+ Partials      4490     4488       -2     
Flag Coverage Δ
e2e 59.17% <ø> (-0.05%) ⬇️
e2e-api 49.00% <54.54%> (-0.41%) ⬇️
unit 70.61% <50.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ggazzo
ggazzo force-pushed the chore/authz-userwithroles-partial-migration branch from f285a4b to 9d2d920 Compare July 13, 2026 23:46
…ecks

Add `UserWithRoles = Pick<IUser, '_id' | 'roles'>` to IAuthorization and relax
the permission-check signatures from `string | IUser` to `string | UserWithRoles`.
`getRoles` only reads `_id` and `roles`, so a full `IUser` was never required.

This lets call sites holding a partial user projection pass the object directly —
skipping the check's internal Users.findOneById — as long as that projection
carries `roles` at runtime. Migrated only sites verified to carry roles at
RUNTIME (not just by type, since finders return `IUser` regardless of projection):

- Meteor.userAsync() sites (full doc): pushConfig, removeChannelLinks,
  room publications, loadHistory.
- canAccessRoom: use the already-refetched full user.
- eraseRoom (+ eraseTeam generic constraints): all callers pass REST `this.user`
  (getDefaultUserFields includes roles) or a full user.
- browseChannels: method caller uses userAsync; its REST caller now projects
  `roles` alongside `__rooms`.

Left on the user-id form: livechat roomAccessValidator (caller has only `_id`),
and canDeleteMessage (one caller chain — deleteFileMessage → Upload.canDeleteFile
— loads the user with a `{ username }` projection, so it lacks roles at runtime).
@ggazzo
ggazzo force-pushed the chore/authz-userwithroles-partial-migration branch from 9d2d920 to 615a68e Compare July 14, 2026 12:30
@ggazzo ggazzo added this to the 8.7.0 milestone Jul 14, 2026
@ggazzo
ggazzo marked this pull request as ready for review July 14, 2026 13:51
@ggazzo
ggazzo requested a review from a team as a code owner July 14, 2026 13:51

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 13 files

Re-trigger cubic

@ggazzo ggazzo added the stat: QA assured Means it has been tested and approved by a company insider label Jul 14, 2026
@dionisio-bot dionisio-bot Bot added the stat: ready to merge PR tested and approved waiting for merge label Jul 14, 2026
@ggazzo
ggazzo merged commit c4bede4 into develop Jul 14, 2026
84 of 86 checks passed
@ggazzo
ggazzo deleted the chore/authz-userwithroles-partial-migration branch July 14, 2026 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge type: chore

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants