Allow to delegate roles on user creation

Manipulating the existing user/register form and user/edit forms was initially left out of the D8 port when I committed it because it added quite a bit of complexity and the code was messy. Having it on a separate tab allowed a standard way to manipulate the roles.

I can see why this feature would be useful though for UX, happy to review patches here if someone wants to put it forward.

Here's a first stab. Effectively, this patch prevents any attempts to adjust roles that you don't have access to and tweaks the user forms to visually align with that.

It might make sense to deny access to the Roles tab if you have access to edit the user (since it's redundant), but I didn't take it that far with this patch.

Attached is a new patch that uses the "assign all roles" permission instead for the check. It also fixes an issue where this doesn't work when creating a user, only when editing existing users.

Hmm, I don't think that would work. The user forms grant access to the roles field only if you have the administer permissions permission.

    $form['account']['roles'] = array(
      '#type' => 'checkboxes',
      '#title' => $this->t('Roles'),
      '#default_value' => (!$register ? $account->getRoles() : array()),
      '#options' => $roles,
      '#access' => $roles && $user->hasPermission('administer permissions'),
    );

So if we don't alter the form for a user with assign all roles, then they will not have access to the field.

Also, just as a performance thing, removing the check for the administer permissions permission means that we're doing unnecessary processing to accumulate the delegated roles. (A user with that permission gets access to all roles out of the box, from core.)

Also just discovered that this patch breaks drush urol. So you cannot assign roles to users from drush because of the hook_user_presave()

Well, sort of. You can still add roles via Drush, but since Drush operates as an anonymous user by default, you would have to specify an appropriate user to run as, i.e.

drush --user=webmaster urol ...

Admittedly, it's not as user friendly because you don't get an error back from that command when it doesn't add a particular role.

The reason I opted to use the presave hook is that we need to be able to limit the roles for more than just the UI (i.e. user form), which is what Role Delegation has traditionally done. If we target UI only, then it leaves security loopholes for users to edit roles they shouldn't via other mechanisms, like web services. So I think the Entity API is still the way to go, but the approach may still need some massaging. The ideal solution would be to somehow actually have proper access control, but access control only really extends to the field level, not "individual options on a field".

The reason for the presave hook makes sense to me, but I think it does cause an API change unrelated to the original request.

Here is a slightly different approach, very similar to how the Role tab works, in fact most of the code was stolen from RoleDelegationSettingsForm. It does not do access checking on submit, only on the form alter. This is just like what happens in RoleDelegationSettingsForm::submitForm().

I also switched back to "administer permissions" like @kevin.dutra mentioned in #10

Yeah, I see where you're coming from. I wonder if there's a way that we could introduce a faux field via the Field API so that it would be available to both the UI and any other mechanism. I'd have to double check to be sure, but I would imagine there's a way to define a field that has no storage or display.

I'm actively working on the field based approach. Hopefully I should have something today, including some test coverage.

Well, it ended up being a bit more involved than I would have hoped, but I think this basically gets us to where we want to be. This patch uses a "computed" base field, in lieu of just a form element, so that a web service would be able to use the field too. For an end user, the UI should be the same. Additional test coverage has been added too. And we should be without the weird side effects of the last attempt at using the Entity API. ;)

Including all the changes might help...

Hmm, looks like there's one scenario where things don't quite work -- when submitting a request via the REST API and omitting the role_change field.

This should cover that last base.

I applied patch #22, now I can assign user to selected roles, which in my configuration exclude "administrator", but I am able to add on the people page the excluded role "administrator" via action links.

The bug from #22 is already reported in https://www.drupal.org/node/2862360 and actually happens regardless of whether this patch is applied or not. That being said, I've applied this patch and while it works, the role field is duplicated and appears both in a separate tab, but below the account fields. This may be specific to my installation, but that's the behavior I'm getting.

1. The roles form element is placed below the "Notify user of new account" checkbox (as @ryanissamson already reported)
2. The 'Roles' label is not translated.

Fixed the untranslated "Roles" label.

Placing the new roles field above the "Notify user of new account" field is tricky since these fields don't have a #weight set, for now I just placed the field inside the "account" group but I'm not sure how to make it appear in the right order without looping over all the fields and setting a #weight for each.

Patch #27 seems to be fixing the issue for "Roles" section, which was not appearing before applying the patch.

I also agree with #26 that "Roles form" is appearing below the "Notify user of new account" checkbox. Ideally it should be above this checkbox.

However great work with the patch, applies cleanly and works really nice..!!!

Marking this as RTBC.

The patch applies cleanly and works well. Not sure how much of a blocker it is to have it in the "wrong place" on the user form. I guess if it's that much of an issue (and the field weights can't be set, because the fields around them don't have weights), people could always modify it themselves?

Otherwise you're into the tricky realm of battling with core field weights in a contrib patch...

Any updates on this still in RTBC ?

It's a great idea, hope to see it committed soon.

Just came across this module, it's exactly what I need! However I'm concerned there aren't going to be any more releases. Have seen a few important bugs fixed in the past year that are now RTBC, however there hasn't been a release or an update to the 8.x development branch in almost 2 years now.

Any news on getting this committed yet?

This has been RTBC for some time. Is there any blocker on committing and release a non-alpha of this module?

Sub'n the old way to bump it up the issue list ;)

Committed and pushed to 8.x-1.x. Thanks!

Thank you