fix: Default SAML User Role setting not working with role names #37713
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 978d54d The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a changeset entry and fixes SAML role mapping by converting provided role names to role IDs before creating or updating users, ensuring role resolution occurs prior to inserting or updating user documents. (48 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (5)📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-11-27T17:56:26.050ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-11-04T16:49:19.107ZApplied to files:
🧬 Code graph analysis (1)apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts (1)
🔇 Additional comments (4)
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 |
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 02:21", "12/09 11:17 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 16 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 09 Dec 2025 11:17:52 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/server/lib/roles/addUserRoles.ts (1)
18-29: Validation occurs before array normalization.The call to
validateRoleList(roles)at line 18 happens before the array normalization at lines 26-29. If a non-array value is somehow passed (despite the type signature),validateRoleListreturnstrueearly for non-arrays, effectively bypassing validation before the value is wrapped in an array.Consider moving the validation after normalization:
+ if (!Array.isArray(roles)) { + roles = [roles]; + process.env.NODE_ENV === 'development' && console.warn('[WARN] RolesRaw.addUserRoles: roles should be an array'); + } + if (!(await validateRoleList(roles))) { throw new MeteorError('error-invalid-role', 'Invalid role'); } if (process.env.NODE_ENV === 'development' && (scope === 'Users' || scope === 'Subscriptions')) { throw new Error('Roles.addUserRoles method received a role scope instead of a scope value.'); } - - if (!Array.isArray(roles)) { - roles = [roles]; - process.env.NODE_ENV === 'development' && console.warn('[WARN] RolesRaw.addUserRoles: roles should be an array'); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/metal-moose-travel.md(1 hunks)apps/meteor/server/lib/roles/addUserRoles.ts(2 hunks)apps/meteor/server/lib/roles/removeUserFromRoles.ts(2 hunks)apps/meteor/server/lib/roles/validateRoleList.ts(1 hunks)
🧰 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/roles/validateRoleList.tsapps/meteor/server/lib/roles/removeUserFromRoles.tsapps/meteor/server/lib/roles/addUserRoles.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/lib/roles/removeUserFromRoles.tsapps/meteor/server/lib/roles/addUserRoles.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/lib/roles/removeUserFromRoles.tsapps/meteor/server/lib/roles/addUserRoles.ts
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
.changeset/metal-moose-travel.md (1)
1-5: LGTM!The changeset accurately describes the fix and correctly categorizes it as a patch release.
apps/meteor/server/lib/roles/validateRoleList.ts (1)
4-18: LGTM!The validation logic correctly handles both role IDs and role names by querying with
findInIdsOrNamesand checking matches against both_idandnamefields. The early return for empty arrays is appropriate since an empty role list is valid.apps/meteor/server/lib/roles/removeUserFromRoles.ts (1)
31-45: LGTM!The role lookup correctly uses
findOneByIdOrNameto support both IDs and names, and appropriately usesrole._idfor the actual database operations. This ensures consistency in the stored data regardless of how the role was specified.apps/meteor/server/lib/roles/addUserRoles.ts (1)
31-48: LGTM!The role lookup correctly uses
findOneByIdOrNameto resolve roles by either ID or name, and consistently usesrole._idfor the actual database operations. The graceful handling of missing roles (warn and continue) allows processing of other valid roles in the list.
pierre-lehnen-rc
left a comment
pierre-lehnen-rc
left a comment
There was a problem hiding this comment.
Roles must always be identified by their ID, and so those functions (addUserRoles, removeUserFromRoles and validateRoleList) are not meant to support role names. Instead of modifying them, you should make sure that no role names are passed to them by converting any potential name to an ID before those functions get called.
Ideally even the setting itself should accept only role IDs as values, but that would require some new UI component to select roles and get their ID (which is not feasible for a simple fix), so converting the name to an ID before using it is the next best approach.
ricardogarim
force-pushed
the
fix/saml-default-role
branch
from
158b5df to
6dd88c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts (1)
32-40: Solid implementation of role name/ID resolution.The validation via length comparison is reasonable. One minor improvement: the error message could identify which specific roles failed to match for easier debugging.
const convertRoleNamesToIds = async (roleNamesOrIds: string[]): Promise<IRole['_id'][]> => { const roles = await Roles.findInIdsOrNames(roleNamesOrIds).toArray(); if (roles.length !== roleNamesOrIds.length) { - throw new Error(`Failed to convert role names to ids: ${roleNamesOrIds.join(', ')}`); + const foundIds = new Set(roles.flatMap((r) => [r._id, r.name])); + const notFound = roleNamesOrIds.filter((r) => !foundIds.has(r)); + throw new Error(`Failed to find roles: ${notFound.join(', ')}`); } return roles.map((role) => role._id); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/metal-moose-travel.md(1 hunks)apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/metal-moose-travel.md
🧰 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/meteor-accounts-saml/server/lib/SAML.ts
🧠 Learnings (1)
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
🧬 Code graph analysis (1)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts (1)
apps/meteor/app/authentication/server/startup/index.js (2)
roles(306-306)Accounts(282-282)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts (2)
177-178: LGTM!Using
skipNewUserRolesSetting: truecorrectly ensures the pre-resolved role IDs inglobalRolesare used directly without modification.
213-215: LGTM!Correctly uses the pre-converted role IDs for existing user updates, ensuring consistency with new user creation.
ricardogarim
force-pushed
the
fix/saml-default-role
branch
from
6dd88c9 to
6a1491d
Compare
ricardogarim
force-pushed
the
fix/saml-default-role
branch
from
6a1491d to
9222e4f
Compare
|
Just one note: when the default flow is triggered, both Since this behavior isn’t directly tied to the scope of this task, I’m flagging it so the team can decide whether it should be handled as a separate fix. |
ricardogarim
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
pierre-lehnen-rc
changed the title
Proposed changes
As described in SUP-839, SAML role assignment fails when the
SAML_Custom_Default_default_user_rolesetting is configured using a role name instead of a role ID. If administrators set this value to a role name (e.g., "custom-admin"), SAML authentication returns "Invalid role [error-invalid-role]". This issue does not occur with default roles like "user" or "admin" because their role IDs match their names. However, it fails for custom roles where the role ID differs from the role name.Solution
Added
convertRoleNamesToIds()helper inSAML.tsthat converts role names to IDs usingRoles.findInIdsOrNames().Issue(s)
Steps to test or reproduce
custom-adminSAML_Default_User_Role=custom-adminFurther comments
Full investigation notes and technical details have been documented in the task SUP-839.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.