Implement admin role as a flag on the role storage, simplify permissions page, remove user_modules_installed

Note on b) AFAIK the only JS that we have on that page is to prevent changing the permission for any actual role if it is granted to the authenticated role, becaue it is not possible for any other role to not have it then. I just tried it is possible to remove permissions. And it works, until you enable the next module. Claiming that we are able to do this is kind of a bug :)

I'll give it a try, sounds like fun.

Its not really clear whether getPermissions should now return ALL permissions.

Let's first see whether we have any test failures with that.

Nice, I guess the part that is missing is removing the existing setting/UI and providing a UI for this... how do we do this? should we, for simplicity, keep the existing UI and just use that to set the flag on the role, or should we make it part of the role edit form?

Not sure about the permission part, maybe we should explicitly save an empty permissions list and document that somewhere?

• Added a simple checkbox on the role edit form
• Adapted some of other existing code as well as the test.

Quick question I had while manually testing the patch: Should we allow the anonymous role and authenticated role allow to be an admin role?

Especially the first one leads to odd results, as you still see the login block, but you can do everything.

I'm responsible for the original implementation of the admin role that went into core, but it's never worked with dynamic permissions.

I do think the ability to remove permissions from the admin role is potentially useful, but contrib could do its own implementation for that.

Whatever we do here is probably going to require some usability review, so tagging.

Not sure we want to throw an exception, the no-op seems more in-keeping.

Not sure a checkbox on role edit form is a good idea - won't that mean you can assign multiple admin roles then?

nit - I find array_intersect(array_keys($roles), $account->getRoles())

easier to digest.

Fair point

Either empty array() or an Exception, not sure we can do an Exception without breaking BC.

I went with the empty array approach, I would hate to require breaking the API for just that.
Feel free to disagree.

On top of that I disabled the checkbox for anonymous roles.

Not sure a checkbox on role edit form is a good idea - won't that mean you can assign multiple admin roles then?

Mh that is a fair point. You could argue that you might want to be able to check additional behaviour based upon the role name itself, but I think this
was always a bad idea.

Berdir pointed out that user module is assigning all permissions on the system every time now, not just new permissions added by a module, which breaks the 'remove a permission' aspect of the current approach anyway. So even if we didn't drop that, we'd have to figure out how to bring it back.

Even its a bit annoying work, I think a contrib module could easily alter the permissions form, change the way how Role::hasPermission is implemented by storing explicit the permissions you don't have access to.

Yep, so let's ditch that for core and let contrib handle it.

CNW for the checkbox issue.

Alright, this time the admin_role check is just internally.

This is why we have test coverage.

+++ b/core/modules/user/src/Form/UserPermissionsForm.php
@@ -85,6 +85,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $admin_role = $this->config('user.settings')->get('user_admin_role');

@@ -153,6 +154,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+          if ($rid === $admin_role) {
+            continue;
+          }

This conditional will never happen, because it's ->get('admin_role').
We need tests for that continue; statement.

Yes #19 is what I was thinking of. It's slightly messy having to set and unset from the form submit, but that hardly ever happens anyway.

• Left the storage in the role entity
• Left the previous UI
• Added a test for the point tim made in 20

1. Not if they are stored in config. We have AFAIK one exception in view mode config entities, but everything else that maps to yaml/config properties uses lowercase. There are some discussions/issues around allowing a mapping, but for now, I think this is correct/consistent.

So is the effect of this that we just don't show the admin role at all on the permissions page?

I think that needs some usability review, will ping Bojhan and yoroy.

+++ b/core/modules/filter/src/FilterFormatFormBase.php
@@ -90,11 +91,16 @@ public function form(array $form, FormStateInterface $form_state) {
-    elseif ($admin_role = $this->config('user.settings')->get('admin_role')) {
-      // If adding a new text format and the site has an administrative role,
-      // pre-select that role so as to grant administrators access to the new
-      // text format permission by default.
-      $form['roles']['#default_value'] = array($admin_role);
+    else {
+      foreach (Role::loadMultiple() as $role) {
+        // If adding a new text format and the site has an administrative role,
+        // pre-select that role so as to grant administrators access to the new
+        // text format permission by default.
+        /** @var \Drupal\user\RoleInterface $role */
+        if ($role->isAdmin()) {
+          $form['roles']['#default_value'][] = $role->id();
+        }
+      }

I think we can drop this, because the admin role no longer needs that permission? you can't take it away anyway? possibly extend with a description that says that the administrative role will always have access?

We could also ensure that the admin role is not even listed, I don't know.

Could use a screenshot to save anyone reviewing having to install and get to that page.

Looking at the screenshots, this needs some work... it looks weird because it is missing the lines, and we should probably at least mention it in the help text above and maybe even remove it completely?

I am not sure we should ever display a role, that has no checkboxes. Either we display it somewhere else. Or we display unselectable-enabled checkboxes everywhere.

I thought about disabled checkboxes in irc, but I remember some usability testing where these were a problem? Or is that mis-remembering?

They are awful, but I guess better than having an empty column. That you have no clue that the admin does have all the permissions - because its showing nothing.

We can make an alternative design, but I am not sure if that still fits in our beta guidelines.

it's also a pattern that we already use on the site, when you enable a permission for the authenticated user role, all other roles except anon are disabled like that. So it might be awful, but at least it is consistently awful ;)

Not showing at all has a small performance improvement, because generating all those checkboxes takes a lot of time, but yeah, it is probably better to do it like this.

I think we can drop this, because the admin role no longer needs that permission? you can't take it away anyway? possibly extend with a description that says that the administrative role will always have access?

That is a really good point!

We could also ensure that the admin role is not even listed, I don't know.

Well, maybe someone wants to allow a specific format just for the admin, in which case, they might expected it to find there.

They are awful, but I guess better than having an empty column. That you have no clue that the admin does have all the permissions - because its showing nothing.

We can make an alternative design, but I am not sure if that still fits in our beta guidelines.

I Agree, its at least less ugly than showing nothing, you really can't disagree here. For a while I was thinking about showing nothing for the admin role,
but this is also kinda confusing, as people might think that this role magically disappeared in the meantime.

Permissions page looks fine to me now.

1. +++ b/core/modules/filter/src/FilterFormatFormBase.php
   @@ -12,6 +12,7 @@
   +use Drupal\user\Entity\Role;
   

   Not used.

2. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -102,11 +113,17 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $admin_roles = $this->roleStorage->getQuery()
   +      ->condition('is_admin', TRUE)
   +      ->execute();
   +    $default_value = $admin_roles ? array_shift($admin_roles) : '';
   
   @@ -459,6 +475,20 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // Change the admin role.
   +    $admin_roles = $this->roleStorage->getQuery()
   +      ->condition('is_admin', TRUE)
   +      ->execute();
   +    foreach ($admin_roles as $rid) {
   +      $this->roleStorage->load($rid)->setIsAdmin(FALSE)->save();
   +    }
   +
   +    $new_admin_role = $form_state->getValue('user_admin_role');
   +    if ($new_admin_role) {
   +      $this->roleStorage->load($new_admin_role)->setIsAdmin(TRUE)->save();
   +    }
   

   This interaction is extremely problematic. I think we should only display the user admin_role is there is 0 or 1 selected. Plus with this form it is not possible to not have an admin role.

Fair point.

Afaics you still have to select a role... There isn't the option to not have an admin role - which is a security issue.

Also this logic needs tests

Afaics you still have to select a role... There isn't the option to not have an admin role - which is a security issue.

Mh, really? You can select '- None -' still.

Here is a test which indeed showed some problems.

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -117,7 +117,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-    $default_value = $admin_roles ? array_shift($admin_roles) : '';
+    $default_value = $admin_roles ? array_values($admin_roles)[0] : '';

Ah, this is needed because of the #access check below and shift removed one entry? I guess reset() would work too?

Looks good, the new code now has test coverage.

Yeah the array_shiftbroke stuff for 2 roles.

Bad merge/rebase.

1. +++ b/core/modules/locale/src/Tests/LocaleUpdateCronTest.php
   @@ -102,7 +102,7 @@ public function testUpdateCron() {
   -    $this->cronRun();
   +    $this->drupalGet('admin/reports/status/run-cron');
   

   This reverts #2431283: Cron CSRF vulnerability.

2. +++ b/core/modules/simpletest/src/KernelTestBase.php
   @@ -140,52 +139,23 @@ protected function setUp() {
   -    $directory = DRUPAL_ROOT . '/' . $this->siteDirectory;
   ...
   -    $container_yamls = [];
   ...
   -      $testing_services_file = $directory . '/services.yml';
   +      $testing_services_file = DRUPAL_ROOT . '/' . $this->siteDirectory . '/services.yml';
   ...
   -      $container_yamls[] = $testing_services_file;
   -    }
   -    $settings_testing_file = DRUPAL_ROOT . '/' . $this->originalSite . '/settings.testing.php';
   -    if (file_exists($settings_testing_file)) {
   -      // Copy the testing-specific settings.php overrides in place.
   ...
   -    }
   -
   -    if (file_exists($directory . '/settings.testing.php')) {
   -      // Add the name of the testing class to settings.php and include the
   -      // testing specific overrides
   -      $hash_salt = Settings::getHashSalt();
   -      $test_class = get_class($this);
   -      $container_yamls_export = Variable::export($container_yamls);
   -      $php = <<<EOD
   -<?php
   -
   -\$settings['hash_salt'] = '$hash_salt';
   -\$settings['container_yamls'] = $container_yamls_export;
   -
   -\$test_class = '$test_class';
   -include DRUPAL_ROOT . '/' . \$site_path . '/settings.testing.php';
   -EOD;
   -      file_put_contents($directory . '/settings.php', $php);
   +      $this->settingsSet('container_yamls', [$testing_services_file]);
   ...
   -    // Bootstrap a new kernel.
   +    // Bootstrap a new kernel. Don't use createFromRequest so we don't mess with settings.
   ...
   -    $site_path = DrupalKernel::findSitePath($request);
   -    $this->kernel->setSitePath($site_path);
   -    if (file_exists($directory . '/settings.testing.php')) {
   -      Settings::initialize(DRUPAL_ROOT, $site_path, $class_loader);
   -    }
   +    $this->kernel->setSitePath(DrupalKernel::findSitePath($request));
   
   +++ b/core/modules/simpletest/src/Tests/KernelTestBaseTest.php
   @@ -27,21 +27,6 @@ class KernelTestBaseTest extends KernelTestBase {
   -    $php = <<<'EOS'
   -# Make sure that the $test_class variable is defined when this file is included.
   -if ($test_class) {
   -}
   ...
   -# Define a function to be able to check that this file was loaded with
   -# function_exists().
   -if (!function_exists('simpletest_test_stub_settings_function')) {
   -  function simpletest_test_stub_settings_function() {}
   -}
   -EOS;
   

   This reverts #2395901: Allow the same test-specific overrides in KernelTestBase as in WebTestBase

@jibran Thanks for seeing it!!

A straight reroll worked.

IMO we need a change notice for this.

Yep we should have a change record for this since this we break the existing administrator role on already installed sites.

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -102,13 +113,22 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $default_value = $admin_roles ? array_values($admin_roles)[0] : '';

I think this is cleaner as:

$default_value = reset($admin_roles);

Less logic. Thanks @Berdir in #41 :)

Here is one, with a slightly better match.

https://www.drupal.org/node/2444095

Fixed beta evaluation.

Ha, fair point!

Looks good now. Committed/pushed to 8.0.x, thanks!