fix!: Scoped roles are assignable through admin users page#34852
fix!: Scoped roles are assignable through admin users page#34852gabriellsh wants to merge 6 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: b464b09 The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #34852 +/- ##
========================================
Coverage 59.18% 59.18%
========================================
Files 2821 2821
Lines 67598 67598
Branches 15045 15045
========================================
Hits 40011 40011
Misses 24773 24773
Partials 2814 2814
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
| } | ||
| return { | ||
| ...data, | ||
| user: { ...data.user, roles: data.user.roles.filter((role: string) => isTruthy(availableRoles.find(({ _id }) => _id === role))) }, |
There was a problem hiding this comment.
Considering that availableRoles is an array of objects, couldn't we use availableRoles.some(...) instead of isTruthy?
There was a problem hiding this comment.
Also, I don't know if this is a concern, but if availableRoles array is big enough I'd even consider creating a Set before. E.g const available = new Set(availableRoles.map(({ _id }) => _id) then using available.has(role).
There was a problem hiding this comment.
Yep that does make sense. I don't think there would be a difference between find and some performance wise, but it's way easier to read, thanks for catching this one.
I'm not sure it's necessary to make a set. I don't think roles ever exceed 50, and I think up to 100 this wouldn't be an issue. I'll do some manual perf testing just to make sure and come back to you, but I think that won't be necessary
There was a problem hiding this comment.
@aleksandernsilva too many data transformations between array and Set to make it worthy, I think.
Co-authored-by: Aleksander Nicacio da Silva <aleksander.silva@rocket.chat>
@MarcosSpessatto I didn't do any tests yet because I'm waiting on product to green light this change, since this is a suggestion of my own. I think it'll go through but I decided to no spend effort in tests until I was sure |
There was a problem hiding this comment.
To reinforce, this SHOULD not impact any existing features or "unintended use", since scoped roles are ignored when checking global permissions.
This premise mentioned on the PR description is not true - at least not for the backend side. If a scoped role is assigned to an user globally, its permissions will be granted globally.
It makes sense to block certain roles such as owner from being assigned globally, as they are specifically designed to give permissions to a room's owner. At the same time there are other scoped roles such as moderator that could work both ways, allowing an user to moderate a specific room and other users to moderate an entire workspace.
gabriellsh
changed the title
|
As @pierre-lehnen-rc commented, scoped roles assigned globaly have different behaviors between FE & BE. We should first normalize this and make them work similarly before we can decide if we should move on with this. Flagged this as a breaking change for now. |
6 participants
Proposed changes (including videos or screenshots)
Scoped roles only work when assigned in the context of a room, so there's no reason to show them in the admin page.
Additionally, if the user has scoped roles assigned, they'll be hidden in the select, which in turn will remove them the next time the user's roles are edited.
To reinforce, this SHOULD not impact any existing features or "unintended use", since scoped roles are ignored when checking global permissions.
Issue(s)
CORE-909
Steps to test or reproduce
Further comments