[NEW] Granular permissions for settings #8942

mrsimpson · 2017-11-24T18:01:46Z

@RocketChat/core This PR adds the ability to allow a role to change only dedicated settings.
This is necessary in order to e. g. allow a user to change the colors of his Rocket.Chat without compromising critical settings such as LDAP-connectivity.

Though the actual purpose is already fulfilled, this PR has to be considered WIP due to the limitations mentioned below.

@Developers (particularly frontend-developers): I need assistance to fix the "final" (mostly UI) bugs

What works

Permissions for each and every setting is created automatically
The permissions-UI is extended to visualize and manage all setting-permissions just like normal permissions.
The settings-UI in the admin-overview displays the selected settings for the role
setting-based permissions are respected/validated when changing a setting
permissions are reactive

What could be improved

In general, the permissions-UI is not beautiful. This is due to the huge width of the permissions table. Any help on that is appreciated - though @engelgabriel said you'd redesign the look anyway. Currently, there's a CSS-hack which does not work in all browsers to make the header take the full width. There has to be another option, but I'm not a CSS-native.

Impressions

Configuring the permissions

There is one explicit permission which makes the whole set applicable.
Whether this is a good idea, I am not convinced myself. The permission could be determined.

Effect

…ified yet

…ns' to see and change the affected settings. However, this is not reactive: Once the permissions for a particular setting are changed, the user needs to log off and on again before it becomes effective in the UI. This is most probably a consequence of the CachedCollection. This collection needed to be changed on permission-change. In the backend however, the permissions become effective immediately.

gdelavald · 2017-11-24T18:05:23Z

This is great, thanks for the PR. We'll check if we have people available to help on the needed changes, else I'm sure the community can help improving it

- Do not try to create permissions for hidden settings in higher-level-callbacks - Remove `setting-permissions` collection - fully integrated into `permissions`

mrsimpson

Added a couple of fixes - and reverted some development which was done earlier.
Now, it works fine from my side: reactively without any frontend or backend exceptions ;)
Some explanations why I changed what and where I was looking for better solutions I did not find below.

package.json

packages/rocketchat-authorization/client/stylesheets/permissions.css

mrsimpson · 2017-11-29T12:08:02Z

packages/rocketchat-authorization/client/views/permissions.html

@@ -1,36 +1,76 @@
+<template name="permissionsTable">


Created a template for the table as both permission tables follow the same scheme.

mrsimpson · 2017-11-29T12:09:23Z

packages/rocketchat-authorization/client/views/permissions.js

@@ -1,66 +1,121 @@
 /* globals ChatPermissions */
+import {permissionLevel} from '../../lib/rocketchat';
+
+const whereNotSetting = {


$ne is not supported by Minimongo. Thus, had to fallback to $where. Still, this does not seem to work, see the .filter-comment some lines below

mrsimpson · 2017-11-29T12:09:47Z

packages/rocketchat-authorization/client/views/permissions.js

-			sort: {
-				_id: 1
+	permissions() {
+		return ChatPermissions.find(whereNotSetting, //the $where seems to have no effect - filtered as workaround after fetch()


any idea why the $where has no effect?

mrsimpson · 2017-11-29T12:11:44Z

packages/rocketchat-authorization/lib/rocketchat.js

@@ -1 +1,5 @@
 RocketChat.authz = {};
+
+export const permissionLevel = {
+	SETTING: 'setting'


I can imagine that there are other levels as well, such as the permission to change dedicated roles, thus making it a map

mrsimpson · 2017-11-29T12:12:49Z

packages/rocketchat-authorization/server/publications/permissions.js

@@ -20,4 +27,12 @@ Meteor.methods({

 RocketChat.models.Permissions.on('changed', (type, permission) => {
 	RocketChat.Notifications.notifyLoggedInThisInstance('permissions-changed', type, permission);


Should this not be notifyLogged() it was like that earlier, so I didn't change it...

mrsimpson · 2017-11-29T12:17:39Z

packages/rocketchat-ui-admin/client/SettingsCachedCollection.js

+		});
+	}
+
+	setupListener(eventType, eventName) {


The redefinition is necessary as the change of permissions has to trigger the Settings-collection to be refreshed. We'll have to listen to an event "authorization changed".

mrsimpson · 2017-11-29T12:17:58Z

packages/rocketchat-ui-admin/client/SettingsCachedCollection.js

+		// private settings also need to listen to a change of authorizationsfor the setting-based authorizations
+		RocketChat.Notifications[eventType || this.eventType](eventName || this.eventName, (t, record) => {
+			this.log('record received', t, record);
+			if (t === 'auth') {


If there was an enumeration-constant, I'd be happy to use it ;)

mrsimpson · 2017-11-29T12:19:30Z

packages/rocketchat-ui-admin/client/SettingsCachedCollection.js

+		RocketChat.Notifications[eventType || this.eventType](eventName || this.eventName, (t, record) => {
+			this.log('record received', t, record);
+			if (t === 'auth') {
+				if (! (RocketChat.authz.hasAllPermission([`change-setting-${ record._id }`, 'manage-selected-settings'])


The concatenation change-setting-${ record._id } should be reusable function of the auth-package. However, as rocketchat:lib is lower than rocketchat:auth, this is not possible.
For simplicity (and worse maintainability) I just green-coded that...

mrsimpson · 2017-12-06T14:06:04Z

removed WIP as this is actually done, except beautification

…gs-permissions

…ng` but not `view-privileged-setting`

Wirone · 2018-01-26T12:27:43Z

Is there possibility to merge this feature in nearest future?

About beautification: maybe it could be transposed from table into tokenized rows? Each row would be a role with autocompleted and tokenized select field. So roles with less privileges would take less space and those with more privileges would take more space. Now every role takes exact space (column) and there are every privileges displayed regardless if they're checked or not. In my concept only chosen privileges would be displayed for each row and each privilege could be removed/added.

…' of https://github.com/assistify/Rocket.Chat into core/RocketChat#8771-fine-granular-settings-permissions

-fine-granular-settings-permissions

mrsimpson · 2018-01-28T10:26:05Z

@RocketChat/core I merged the current master in order to allow other users who build their own version to merge this branch. If you could let me know what you need to get it merged (and when!), I'll merge develop. For other devs: c7d7145 is compatible with 0.60.4

…ermissions

…8771-fine-granular-settings-permissions

mrsimpson · 2019-08-08T13:50:33Z

app/lib/server/methods/saveSettings.js

@@ -41,6 +40,14 @@ Meteor.methods({
 			}
 			settings.updateById(_id, value, editor);
 		});
+		// Throw messages for settings that are not allowed to save!
+		if (settingsNotAllowed.length) {
+			throw new Meteor.Error('error-action-not-allowed', 'Editing settings is not allowed', {


This error is not propagated to the user, just like it was not done earlier

mrsimpson · 2019-08-08T13:52:49Z

app/lib/server/methods/saveSettings.js

 		if (Meteor.userId() === null) {
 			throw new Meteor.Error('error-action-not-allowed', 'Editing settings is not allowed', {
 				method: 'saveSetting',
 			});
 		}

-		if (!hasPermission(Meteor.userId(), 'edit-privileged-setting')) {
-			throw new Meteor.Error('error-action-not-allowed', 'Editing settings is not allowed', {


This actually failed the saving of multiple settings on the first permission violation. Since now multiple settings can be saved at once and some may fail while others wont, I moved this error handling into a loop down below.

mrsimpson

@ggazzo I now successfully merged the feature to 1.4-develop.
To me, it feels as if it was good to merge.
Switching between chat permissions and setting permissions is not beautiful, but given the fact this is not a core interaction, I'd consider it "ok".

It was great if we could get this merged, since it's a must for corporate, as per our compliance guys

mrsimpson · 2019-08-08T13:53:18Z

app/ui-admin/client/SettingsCachedCollection.js

@@ -0,0 +1,38 @@
+import _ from 'underscore';


The setting collection was used twice, so I refactored this into an own file.

Since the setting permissions are based upon permissions, the change of permissions needs to be synced first

…8771-fine-granular-settings-permissions

…gs-permissions

…' of github.com:assistify/Rocket.Chat into core/RocketChat#8771-fine-granular-settings-permissions

ggazzo · 2019-08-13T00:18:59Z

…' of github.com:assistify/Rocket.Chat into pr/8942-mrsimpson-core/RocketChat#8771-fine-granular-settings-permissions

ggazzo · 2019-08-15T17:09:32Z

@sampaiodiego could you review this code plz? I'm not worthy anymore (because I change the code a lot) =/ =) ...

ps: last thing I know, we have to move the streamer to emitter.js AND check if its everything ok because we gonna be sending the settings changes for all "manage-settings" users

…' of github.com:assistify/Rocket.Chat into pr/8942-mrsimpson-core/RocketChat#8771-fine-granular-settings-permissions

…t#8942)

mrsimpson added 8 commits November 17, 2017 18:28

Allow maintenance of per-setting permissions - first draft, not beaut…

eed869a

…ified yet

Implicitly assign and revoke setting group permissions

28b769b

Improve Display of setting permissions

8523456

Add path to permission title

c87a30d

Permission to access setting permissions

48b1076

Adapt wording

daccad8

Don't adapt sorting on the client side

9b71b62

mrsimpson added 5 commits November 26, 2017 18:43

Fix: Apply changed setting permissions reactively

293ad73

Move setting-based permissions to own collection

8f59f1c

Unify collections for setting and other permissions again into one

8d923c2

Get rid of frontend exceptions on changing selected settings

a7fdc87

- Sort permissions by group

f007231

- Do not try to create permissions for hidden settings in higher-level-callbacks - Remove `setting-permissions` collection - fully integrated into `permissions`

mrsimpson commented Nov 29, 2017

View reviewed changes

mrsimpson mentioned this pull request Nov 29, 2017

Setting based permissions - downport assistify/Rocket.Chat#158

Merged

mrsimpson changed the title ~~[NEW] #8771 - fine granular settings permissions (WIP)~~ [NEW] #8771 - fine granular settings permissions Dec 6, 2017

mrsimpson and others added 2 commits December 6, 2017 15:06

Merge branch 'develop' into core/RocketChat#8771-fine-granular-settin…

47d8749

…gs-permissions

Allow administration even if user has got only `edit-privileged-setti…

c706dd4

…ng` but not `view-privileged-setting`

mxmo0rhuhn mentioned this pull request Jan 2, 2018

Admins from LDAP group #1196

Closed

mrsimpson mentioned this pull request Jan 2, 2018

[Feature request] More fine grained permission system #8771

Closed

mrsimpson added 3 commits January 26, 2018 16:08

fix css-selectors for permission inputs

f26246b

Merge branch 'core/RocketChat#8771-fine-granular-settings-permissions…

cd336fe

…' of https://github.com/assistify/Rocket.Chat into core/RocketChat#8771-fine-granular-settings-permissions

Merge remote-tracking branch 'upstream/master' into core/RocketChat#8771

c7d7145

-fine-granular-settings-permissions

Merge tag '0.68.4' into core/RocketChat#8771-fine-granular-settings-p…

4108b7a

…ermissions

mrsimpson mentioned this pull request Aug 24, 2018

Port: Setting based permissions assistify/Rocket.Chat#429

Merged

Merge remote-tracking branch 'upstream/develop' into core/RocketChat#…

02c0160

…8771-fine-granular-settings-permissions

mrsimpson commented Aug 8, 2019

View reviewed changes

mrsimpson and others added 8 commits August 8, 2019 16:39

Change order in which client is notified

6e778c3

Since the setting permissions are based upon permissions, the change of permissions needs to be synced first

Merge remote-tracking branch 'upstream/develop' into core/RocketChat#…

ddd5606

…8771-fine-granular-settings-permissions

Merge branch 'develop' into core/RocketChat#8771-fine-granular-settin…

b80e1ec

…gs-permissions

Merge branch 'develop' into core/RocketChat#8771-fine-granular-settin…

34d51a4

…gs-permissions

Adapt automated test for setting based permissions

9a18fcc

Merge branch 'core/RocketChat#8771-fine-granular-settings-permissions…

a55afb4

…' of github.com:assistify/Rocket.Chat into core/RocketChat#8771-fine-granular-settings-permissions

[WIP] reviewing

93a0464

Permission table

59e2aab

mrsimpson and others added 4 commits August 13, 2019 19:29

Adapt setting permissions test to tabbed layout

8419341

fix settings

7870cd6

settings level constant

d238b7a

Merge branch 'core/RocketChat#8771-fine-granular-settings-permissions…

f2e8ada

…' of github.com:assistify/Rocket.Chat into pr/8942-mrsimpson-core/RocketChat#8771-fine-granular-settings-permissions

ggazzo added 4 commits August 15, 2019 14:54

emiiter

0858217

emiiter

b09fd79

Merge branch 'core/RocketChat#8771-fine-granular-settings-permissions…

bb2af49

…' of github.com:assistify/Rocket.Chat into pr/8942-mrsimpson-core/RocketChat#8771-fine-granular-settings-permissions

remove useless comments

3808a30

ggazzo approved these changes Aug 20, 2019

View reviewed changes

ggazzo merged commit d19c721 into RocketChat:develop Aug 20, 2019

rodrigok changed the title ~~[NEW] #8771 - fine granular settings permissions~~ [NEW] Granular permissions for settings Aug 20, 2019

geekgonecrazy deleted the core/#8771-fine-granular-settings-permissions branch August 20, 2019 15:13

mrsimpson added a commit to assistify/Rocket.Chat that referenced this pull request Aug 27, 2019

[NEW] RocketChat#8771 - fine granular settings permissions (RocketCha…

3233371

…t#8942)

sampaiodiego mentioned this pull request Sep 12, 2019

Release 2.0.0 #15350

Merged

ggazzo mentioned this pull request Feb 3, 2020

Permission settings: limited permissions with access-permissions enabled #4825

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEW] Granular permissions for settings #8942

[NEW] Granular permissions for settings #8942

mrsimpson commented Nov 24, 2017 •

edited

Loading

gdelavald commented Nov 24, 2017

mrsimpson left a comment •

edited

Loading

mrsimpson Nov 29, 2017

mrsimpson Nov 29, 2017

mrsimpson Nov 29, 2017

mrsimpson Nov 29, 2017

mrsimpson Nov 29, 2017

mrsimpson Nov 29, 2017

mrsimpson Nov 29, 2017

mrsimpson Nov 29, 2017

mrsimpson commented Dec 6, 2017

Wirone commented Jan 26, 2018 •

edited

Loading

mrsimpson commented Jan 28, 2018 •

edited

Loading

mrsimpson Aug 8, 2019 •

edited

Loading

mrsimpson Aug 8, 2019

mrsimpson left a comment •

edited

Loading

mrsimpson Aug 8, 2019

ggazzo commented Aug 13, 2019

ggazzo commented Aug 15, 2019 •

edited

Loading

		@@ -20,4 +27,12 @@ Meteor.methods({

		RocketChat.models.Permissions.on('changed', (type, permission) => {
		RocketChat.Notifications.notifyLoggedInThisInstance('permissions-changed', type, permission);

[NEW] Granular permissions for settings #8942

[NEW] Granular permissions for settings #8942

Conversation

mrsimpson commented Nov 24, 2017 • edited Loading

What works

What could be improved

Impressions

gdelavald commented Nov 24, 2017

mrsimpson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrsimpson commented Dec 6, 2017

Wirone commented Jan 26, 2018 • edited Loading

mrsimpson commented Jan 28, 2018 • edited Loading

mrsimpson Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrsimpson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggazzo commented Aug 13, 2019

ggazzo commented Aug 15, 2019 • edited Loading

mrsimpson commented Nov 24, 2017 •

edited

Loading

mrsimpson left a comment •

edited

Loading

Wirone commented Jan 26, 2018 •

edited

Loading

mrsimpson commented Jan 28, 2018 •

edited

Loading

mrsimpson Aug 8, 2019 •

edited

Loading

mrsimpson left a comment •

edited

Loading

ggazzo commented Aug 15, 2019 •

edited

Loading