Group roles and permissions UI

[zerolab]

This PR is a continuation of amitaibu/og#196. It:

• Fixes the clashing routes for adding a role
• Fixes new group roles considered required, rather than standard
• Adds a role deletion form
• Adds a group permissions and a group role permissions form

To-Do:

• [x] clean-up the routing
• [x] use separate permissions for roles (og_role.add, og_role.update, og_role.delete)
• [x] allow permissions groups other that "Group" and "Group content"
• [ ] tests

Also needs decision as to when amitaibu/og#315 comes into force.

[amitaibu]

Thank you for working on this!

[zerolab]

Shall I submit a separate PR for the linting errors? Most of them are outside this PR.

[amitaibu]

No, it can be part of this PR.

[MPParsley]

@zerolab have you tested this with config_translation enabled?

After applying this patch on a Drupal 8.5 and reinstalling the site from config, the following error occurs:

Call to a member function getPath() on null in config_translation/src/ConfigNamesMapper.php:236
Stack trace:
#0 config_translation/src/Routing/RouteSubscriber.php(39): Drupal\config_translation\ConfigNamesMapper->getOverviewRoute()
#1 web/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php(37): Drupal\config_translation\Routing\RouteSubscriber->alterRoutes(Object(Symfony\Component\Routing\RouteCollection))
#2 [internal function]: Drupal\Core\Routing\RouteSubscriberBase->onAlterRoutes(Object(Drupal\Core\Routing\RouteBuildEvent), 'routing.route_a...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#3 web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func(Array, Object(Drupal\Core\Routing in config_translation/src/ConfigNamesMapper.php, line 236

I assume there's still an error in the routing configuration.

[jludwig]

This is fantastic, thanks!

One minor usability comment:

When changing permissions at /admin/config/group/permissions/node/[bundle]/[role], a message should be displayed to the user to indicate success or failure

[zerolab]

@jludwig good suggestion! added in 928d8068 following the core model.

[MPParsley]

@zerolab the OgRole entity is defined in 'og' but it contains links to routes defined by 'og_ui':

 *   links = {
 *     "add-form" = "/admin/config/group/roles/{entity_type_id}/{bundle_id}/add",
 *     "edit-form" = "/admin/config/group/role/{og_role}/edit",
 *     "canonical" = "/admin/config/group/role/{og_role}/edit",
 *     "delete-form" = "/admin/config/group/role/{og_role}/delete",
 *   },

This won't work when reinstalling the site from config. I suppose we'll have to define the routes in a RouteSubscriber in og that checks if og_ui is enabled...

[zerolab]

There is a task to merge og_ui with OG. I could change this PR to do so if @amitaibu @pfrenssen think this is best tackled here.

[amitaibu]

I think merging OG & OG UI makes sense, since the UI isn't just for admin purposes like for example Field UI -- it's for the actual interaction of end users with the group functionality. I think, however it should be done in a separate PR, to keep this one from being too big.

[pfrenssen]

Created a followup to merge OG and OG UI: https://github.com/Gizra/og/issues/396

[pfrenssen]

@zerolab I am currently sprinting on OG at Drupal Dev Days and I would like to work on this. Could you make me a collaborator?

[zerolab]

Added. Glad you are working on this!

[pfrenssen]

Thanks! I'm trying to move everything forward at once now :)

[pfrenssen]

Restored the original commit chain from https://github.com/amitaibu/og/pull/196 and rebased on the latest 8.x-1.x.

[pfrenssen]

Going to do an in-depth review.

[heddn]

I guess this doesn't necessarily apply here. Or maybe it does. But I'd like to disable deleting group content. But that option is grayed out for group admins. I'm using workflow module on this site and delete isn't in use as much as changing the workflow state of a piece of content to an archived state and unpublishing it.

[heddn]

Cross posting, I just opened #421 as a potential security issue.

[MPParsley]

@zerolab this issue could use an update of the remaining tasks.

Also needs decision as to when amitaibu#315 comes into force. => Note that it's decided to remove og_ui (https://github.com/amitaibu/og/issues/315).

[zerolab]

@MPParsley see https://github.com/Gizra/og/pull/243#issuecomment-396493779. Removing og_ui should be a separate PR.

In terms of what is left:

• address the comments on this PR
• add some tests

Unfortunately I no longer have any paid project time to work on Drupal. Happy to give you contributor access to my fork if you want to get this to completion.

[MPParsley]

@zerolab, thanks that might be useful. I could add some fixes for some remarks made here. I probably won't have time to write all the tests though.

PS: my remark about the split of og_ui was related to this line in your opening comment:

Also needs decision as to when amitaibu#315 comes into force.

[MPParsley]

I was reviewing this but didn't manage to complete it before the Drupal Dev Days sprint was closed.

@pfrenssen, it would be awesome if we could use the momentum of the Drupal 8.7 update to merge this feature. Any chance you could finalize your review?

[pfrenssen]

@MPParsley I'm sorry if I gave the impression that I am pushing OG for the upcoming release. I'm in fact working on a task for my day job project to fix a missing cache tag in our groups, and prepare our project for Drupal 8.7.

I can look at small PRs in my spare time, but a big one like this needs more time and I can only really do this at a sprint. The earliest opportunity will be at Drupalcamp Spain in 2 weeks.

[MPParsley]

The test is failing because of #503 which is now merged back to develop.

@zerolab, could you rebase this or merge develop (https://github.com/zerolab/og/pull/4)?

[zerolab]

The test is failing because of #503 which is now merged back to develop.

@zerolab, could you rebase this or merge develop (zerolab#4)?

Hey @MPParsley you are collaborator on my fork, so feel free to rebase/merge

[pfrenssen]

It has been some time since I last looked at this, but I am very impressed with this work. It works beautifully and it seems to be feature complete.

The one thing that is most needed right now is test coverage. I will start on this with an access test. I want to test that all the different routes regarding roles and permissions are only accessible by the right users.

I will test with the following users:

• UID 1: should have access to everything.
• user with 'administer group' permission: access to everything.
• user with 'administer permissions' permission: access to everything.
• anonymous users: should have access to nothing.
• authenticatied user (non-member): no access.
• member: no access.
• group administrator (with "Group manager has full permissions" option enabled): access only within their own group.
• group administrator (with "Group manager has full permissions" option disabled): no access.
• group member with 'manage roles' permission: access role administration page only within their own group.
• group member with 'manage permissions' permission: access permissions pages only within their own group.

I see that in OG we are making the distinction between 'manage permissions' and 'manage roles' but this seems strange, because both are very similar and basically different views on the same data. The permissions page shows the same as the role page, but just for multiple roles at once. I am not sure if we should somehow block access to the permission form unless the user has both the 'manage roles' and the 'manage permissions' permissions.

In core both roles and permissions are handled by the administer permissions permission. I see that this distinction in permissions between roles and permissions also existed in OG for Drupal 7, so we should probably discuss this in a separate issue.

[pfrenssen]

Created issue to discuss unifying the permissions to manage roles and permissions: #526.

[pfrenssen]

I implemented the access check. I was wrong in my analysis in my previous comment. This UI is only for the "global" per-group type roles and permissions, not for per-group roles and permissions. So group owners and group members with permission to manage roles in their group should not have access to these forms.

Also people with the global "administer permissions" permission should not have access to this. I have verified this in the Drupal 7 version. Only people with "administer group" should have access.

[zerolab]

@pfrenssen your assessment of this being the global roles and permissions is correct

So group owners and group members with permission to manage roles in their group should not have access to these forms.

As far as I remember #231 does that. i.e. at the group level you only add/edit/remove member(ships) and relevant roles

[jerrac]

Is this pull request only about the global roles and permissions ui? Not the per-group ui?

Is there an issue/pull request for letting group owners/admins manage permissions on a per group basis? I've been reading through the drupal 8 issues and I don't think I've found one yet.

FYI, the version of https://github.com/zerolab/og/tree/196-redux I checked out today seems to work just fine for my very limited testing. I was able to add a role and edit the global permissions for my group content type.

[MPParsley]

Is this pull request only about the global roles and permissions ui? Not the per-group ui? You may refer to #231 if you refer to the per group UI to manage members.

Is there an issue/pull request for letting group owners/admins manage permissions on a per group basis? I've been reading through the drupal 8 issues and I don't think I've found one yet. Permissions can be managed per group type.

FYI, the version of https://github.com/zerolab/og/tree/196-redux I checked out today seems to work just fine for my very limited testing. I was able to add a role and edit the global permissions for my group content type. This PR comes from zerolab/og/tree/196-redux.