Setting based permissions - downport #158

mrsimpson · 2017-11-29T12:37:28Z

This is a downport of RocketChat#8942 : The PR towards Rocket.Chat has been implemented based on develop of Rocket.Chat which is 0.60-based.
Thus, I cherry-picked all the commits and verified 0.59.3-compatibility. I didn't do any adaptions to that so far.

Thus, I'd like you (dear reviewer) to comment on RocketChat#8942 for two reasons:

I already explained some aspects here
Other reviewers @RocketChat can benefit from your comments as well.

What to do?

Check the functionality: Log-in with admin and non-admin-user, giving the non-admin the permissions manage-selected-permissions and some setting-based permissions.
Watch the ability of changing dedicated settings change for the non-admin
Revert the permissions
Verify no conflicts to the standard view/change-priviledged-setting-permission
Check the Code ;)

(cherry picked from commit eed869a)

(cherry picked from commit 28b769b)

(cherry picked from commit 8523456)

(cherry picked from commit c87a30d)

(cherry picked from commit 48b1076)

(cherry picked from commit daccad8)

…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. (cherry picked from commit 00e4bb5)

(cherry picked from commit 9b71b62)

(cherry picked from commit 293ad73)

(cherry picked from commit 8f59f1c)

(cherry picked from commit 8d923c2)

(cherry picked from commit a7fdc87)

- Do not try to create permissions for hidden settings in higher-level-callbacks - Remove `setting-permissions` collection - fully integrated into `permissions` (cherry picked from commit f007231)

(cherry picked from commit 5cf5df2)

ruKurz

Tested functionality

By giving a non-admin role manage-selected-settings permission, users that have that role are able to access the admin view.
When opening the admin view the Info option is available.
After activating some setting-based permissions for the non-admin role, everything works like charm: users of that role are permitted to change the selected settings within the admin view.
I tested by changing the layout colors (nothing else)
Changing dedicated settings works as suspected - also in a reactive way, using two browsers (Safari and chrome).
In addition I tested access-setting-permissions for non-admin users, also this works well.
If a user is only permitted to access-setting-permissions but not to access-permissions he is not able to access the permission option at all. When a user has only access-permissions but not access-setting-permissions he has only access to the general permission settings.
Only if the user has both access-permissions and access-setting-permissions he is able to see the permission option within the admin view.
Removing permissions takes effect in place

Not tested functionality

I did not test this via API.
I did not test if the caching for selected setting-based permissions
I did not get how to verify no conflicts to the standard view/change-priviledged-setting-permission

Abnormalities

there are some labeling problems (translation files): Only the keys are displayed, not the translations
travis failed, see my comment above

The code looks very straight forward. I'm able to comprehend. Since most changes are declarative and not logical, I do not have any significant remark on it. It looks good to me.

Well done!

ruKurz · 2017-12-01T16:57:51Z

packages/rocketchat-i18n/i18n/de.i18n.json

@@ -16,6 +16,7 @@
  "access-mailer_description": "Berechtigung, Massen-E-Mails an alle Benutzer zu versenden.",
  "access-permissions": "Zugriff auf die Berechtigungs-Übersicht",
  "access-permissions_description": "Anpassen der Berechtigungen für die unterschiedlichen Rollen.",
+  "access-setting-permissions": "Einstellungsbezogene Berechtigungen ändern",


I think "Einstellungsbezogene" is very complex even for German people.

What's about
"Berechtigungen für Einstellungen ändern"

or
"Zugriff auf Berechtigungen für Einstellungen"

ruKurz · 2017-12-01T16:59:30Z

packages/rocketchat-i18n/i18n/de.i18n.json

@@ -1575,6 +1576,7 @@
  "Set_as_leader": "Zum Diskussionsleiter ernennen",
  "Set_as_moderator": "Zum Moderator ernennen",
  "Set_as_owner": "Zum Besitzer machen",
+  "Setting_permissions": "Einstellungsbezogene Berechtigungen",


same here, how-to avoid "Einstellungsbezogene"?

ruKurz · 2017-12-01T17:04:59Z

package.json

-		"start": "meteor npm i && meteor",
+		"start": "meteor npm i && meteor run",
+		"debug": "meteor run --inspect",
+		"debug-brk": "meteor run --inspect-brk",


Is this related with the subject of this PR? Or just a general extension for easy debugging?

vickyokrm · 2017-12-04T13:03:31Z

Reviewed and tested the function together with @ThomasRoehl
Here are the review comments.

Detailed walk through on the source code implementation.
The configurable permissions settings list are long which is leads to scrolling difficulties.
(Since, this is screen is used not very often, adding a settings search function to the screen is not handled at this point in time but in future)
Also, the permission setting named together with the group name and hierarchies which will serve the user to drill down to a particular setting easily.
The functionality expected is well achieved.
Changing settings is intuitive in reactive way and its blazing.
Tested activating/deactivating the individual settings from admin user - all permissions worked as expected.
Tested the 'manage-selected-settings' enabler option which will enable the administration view for the non-admin user.
As @ruKurz also highlighted, the 'info' option is displayed for the non-admin user in the administration view even though the non-admin user has any setting to view.
No conflicts are identified with the standard permission settings

vickyokrm

The expected functionality is achieved well with robust and understandable coding style.

* Fixed Issues 87, 88 * Feature/#23 title first message to new request (#149) * Request title and first message while creating new request * Make titles of inputs and placeholders more consistent * Fix display issues: - Dropdown overlapped by input field - In English, the width of the creation dialog was not 100%, thus input fields within too narrow * Minor corrections with respect to error handling: - Refactor error display to an own template - Show error if selected expertise on request creation is invalid (not chosen from the dropdown) - Fix positioning of "at" on members selection if invalid * Fixes #151 - Misspelled label "jetzt chaten" (#152) * Corrections to creation dialog (#154) * Corrections to creation dialog - Propagate first message properly (fixes #153) - Change "Name" to "Title" - get rid of flashing error message on auto-complete-confirmation with tab * Corrections to creation dialog - more robust error handling - prevent flashing of validation errors * Fix improper clearing of request title * Invalid Expertise field highlight * #156 Prefill request creation form (#157) * fixes #156 - Pass topic as URL param (URL encoded) as `topic` or `expertise` (same effect) - Title is focused if expertise is passed * Allow pre-filling of title and question as well. This way, fixes #20 : The consumer (e. g. the wiki page) can create a simple form an pass the content as parameter * - Use the copied current() instead of the internal value ``` FlowRouter.current ƒ () { // 248 // We can't trust outside, that's why we clone this … ``` - refactor query-selectors => reuse * use FlowRouter API properly * Setting based permissions - downport (#158) * Allow maintenance of per-setting permissions (cherry picked from commit eed869a) * Implicitly assign and revoke setting group permissions (cherry picked from commit 28b769b) * Improve Display of setting permissions (cherry picked from commit 8523456) * Add path to permission title (cherry picked from commit c87a30d) * Permission to access setting permissions (cherry picked from commit 48b1076) * Adapt wording (cherry picked from commit daccad8) * UI-adaptation: Allow users with permission 'manage-selected-permissions' 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. (cherry picked from commit 00e4bb5) * Don't adapt sorting on the client side (cherry picked from commit 9b71b62) * Fix: Apply changed setting permissions reactively (cherry picked from commit 293ad73) * Move setting-based permissions to own collection (cherry picked from commit 8f59f1c) * Unify collections for setting and other permissions again into one (cherry picked from commit 8d923c2) * Get rid of frontend exceptions on changing selected settings (cherry picked from commit a7fdc87) * - Sort permissions by group - Do not try to create permissions for hidden settings in higher-level-callbacks - Remove `setting-permissions` collection - fully integrated into `permissions` (cherry picked from commit f007231) * Harmonize wording in German (cherry picked from commit 5cf5df2) * German language informalized (#160) * German language informalized - Liebe Deutsche, wir kennen euch nun besser. Wir wollen ab jetzt “Du” zu einander sagen 😉 * Update de.i18n.json * Update de.i18n.json * Update de.i18n.json * Allow administration even if user has got only `edit-privileged-setting` but not `view-privileged-setting` * Revert "Fixed Issues 87, 88 (livechat on mobile devices)" (#164) * Create configuration expert role on startup (#159) * Allow maintenance of per-setting permissions (cherry picked from commit eed869a) * Implicitly assign and revoke setting group permissions (cherry picked from commit 28b769b) * Improve Display of setting permissions (cherry picked from commit 8523456) * Add path to permission title (cherry picked from commit c87a30d) * Permission to access setting permissions (cherry picked from commit 48b1076) * Adapt wording (cherry picked from commit daccad8) * UI-adaptation: Allow users with permission 'manage-selected-permissions' 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. (cherry picked from commit 00e4bb5) * Don't adapt sorting on the client side (cherry picked from commit 9b71b62) * Fix: Apply changed setting permissions reactively (cherry picked from commit 293ad73) * Move setting-based permissions to own collection (cherry picked from commit 8f59f1c) * Unify collections for setting and other permissions again into one (cherry picked from commit 8d923c2) * Get rid of frontend exceptions on changing selected settings (cherry picked from commit a7fdc87) * - Sort permissions by group - Do not try to create permissions for hidden settings in higher-level-callbacks - Remove `setting-permissions` collection - fully integrated into `permissions` (cherry picked from commit f007231) * Harmonize wording in German (cherry picked from commit 5cf5df2) * add configuration package * Add default role configuration on startup * set default system language to DE * Reduce capabilities of config expert and introduce minor admin * Parted the roles for configuration and managing the rest - Manager - well - manages the application, like a minor admin. Target is that this role is capable of doing everything which is necessary while *regularly* running the application - Config-expert is allowed to customize the application (affecting all users' experience) * Informal german language for our custom texts (#165) * German language informalized - Liebe Deutsche, wir kennen euch nun besser. Wir wollen ab jetzt “Du” zu einander sagen 😉 * German texts of custom enhancement informalized * Bump version to 0.5.0 * Update HISTORY.md

mrsimpson added 14 commits November 29, 2017 13:26

Allow maintenance of per-setting permissions

aaf9da1

(cherry picked from commit eed869a)

Implicitly assign and revoke setting group permissions

5b370b7

(cherry picked from commit 28b769b)

Improve Display of setting permissions

93071c6

(cherry picked from commit 8523456)

Add path to permission title

d663dde

(cherry picked from commit c87a30d)

Permission to access setting permissions

2a38293

(cherry picked from commit 48b1076)

Adapt wording

b0da8da

(cherry picked from commit daccad8)

Don't adapt sorting on the client side

0f136b8

(cherry picked from commit 9b71b62)

Fix: Apply changed setting permissions reactively

f257970

(cherry picked from commit 293ad73)

Move setting-based permissions to own collection

1867e7c

(cherry picked from commit 8f59f1c)

Unify collections for setting and other permissions again into one

2e31f55

(cherry picked from commit 8d923c2)

Get rid of frontend exceptions on changing selected settings

af55aa8

(cherry picked from commit a7fdc87)

- Sort permissions by group

41b3f58

- Do not try to create permissions for hidden settings in higher-level-callbacks - Remove `setting-permissions` collection - fully integrated into `permissions` (cherry picked from commit f007231)

Harmonize wording in German

1cf1d52

(cherry picked from commit 5cf5df2)

mrsimpson requested review from ThomasRoehl and vickyokrm November 29, 2017 12:37

mrsimpson self-assigned this Nov 29, 2017

ruKurz self-requested a review December 1, 2017 16:51

ruKurz reviewed Dec 1, 2017

View reviewed changes

vickyokrm approved these changes Dec 4, 2017

View reviewed changes

vickyokrm merged commit 75d1491 into develop Dec 4, 2017

mrsimpson mentioned this pull request Dec 13, 2017

Allow users with a particular role to change the color scheme #148

Closed

vickyokrm mentioned this pull request Sep 4, 2018

Port: Setting based permissions #429

Merged

mrsimpson deleted the feature/setting-based-permissions-downport branch December 7, 2020 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting based permissions - downport #158

Setting based permissions - downport #158

mrsimpson commented Nov 29, 2017

ruKurz left a comment •

edited

Loading

ruKurz Dec 1, 2017

ruKurz Dec 1, 2017

ruKurz Dec 1, 2017

vickyokrm commented Dec 4, 2017 •

edited

Loading

vickyokrm left a comment

Setting based permissions - downport #158

Setting based permissions - downport #158

Conversation

mrsimpson commented Nov 29, 2017

What to do?

ruKurz left a comment • edited Loading

Choose a reason for hiding this comment

ruKurz Dec 1, 2017

Choose a reason for hiding this comment

ruKurz Dec 1, 2017

Choose a reason for hiding this comment

ruKurz Dec 1, 2017

Choose a reason for hiding this comment

vickyokrm commented Dec 4, 2017 • edited Loading

vickyokrm left a comment

Choose a reason for hiding this comment

ruKurz left a comment •

edited

Loading

vickyokrm commented Dec 4, 2017 •

edited

Loading