Details
I tested this in combination with the next diff, where we add a updateRolesAndPermissionsForAllThreads migration:
- Create a community
- Update Members permission to include managing tags
- Run the migration
- Confirm that the managing tags permissions is still present
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| keyserver/src/updaters/role-updaters.js | ||
|---|---|---|
| 51–58 ↗ | (On Diff #43370) | I'm not sure if I understand this correctly, but it looks like defaultRolePermissions.Members would override what we have in currentRole.permissions. Wouldn't that mean that we override permissions that a user set? |
| 131–142 ↗ | (On Diff #43370) | I know this is based on the existing code, but it looks quite dangerous from the consistency point of view. Ideally, all the updates in this function should be one transaction, but at least we can first update the memberships and then delete the role. The current approach would leave some members with nonexistent roles, if the delete succeeds and update fails. |
| keyserver/src/updaters/role-updaters.js | ||
|---|---|---|
| 51–58 ↗ | (On Diff #43370) | Yes. User-surfaced permissions are represented internally by roles specified at the community root level, and they don't affect the role permissions for any other threads. As such, role permissions for non-root thread types should always be reset to the default. |
| 131–142 ↗ | (On Diff #43370) | Good point – I'll make sure we update the memberships first |