Role permissions are not sorted when saving via admin/people/permissions

Setting back the following code solve the issue :

\Drupal\user\Entity\Role::preSave:

    if (!$this->isSyncing()) {
      // Permissions are always ordered alphabetically to avoid conflicts in the
      // exported configuration.
      sort($this->permissions);
    }

Still needs to see why it has been removed. Seems something doesn't work as expected after 🐛 Role permissions not sorted in config export Fixed changes.

Found this similar issue for 10.x to
🐛 user_update_10000 causes exported permissions to be numerically keyed Closed: duplicate

Move it to critical since it will regenerate all permissions roles files of existing projects, and will need a rollback.

After updating another project from 9.5.3 to 9.5.4, i don't reproduce the issue.
Really strange

Forget about this, i restart from previous dump, run the update again and cannot reproduce

I finally reproduce again.

This will occurs only if you remove a permission from a role (and then export configuration with drush).

May be there is a better solution, but set the code from Role::preSave back solve it right now.

Let see later why schema update from 🐛 Role permissions not sorted in config export Fixed does not solve this as expected.

Tried replicating and was not able to.

Exported permissions
Removed a permission from a role
Exported again.
not seeing any issues.

Think this can be lowered to Normal.

As a bug will also need a test case to show the isuse.

I don't think this meets the requirements → for a critical, can you elaborate on why you feel it is, it doesn't seem to cause data loss

The IS states"revoke a permission". Does that mean via the action? I can see how that may lead to unsorted permissions depending on how that action plugin works. I would be surprised if it did though since saving a Role entity should trigger the sort that was added in https://www.drupal.org/project/drupal/issues/3039499 🐛 Role permissions not sorted in config export Fixed

I reproduce in both contexts : after updating from 9.5.3 to 9.5.4, and after installation to 9.5.4 with existing config.

Steps to reproduce :
- Have an installed drupal at 9.5.3
- Be sure configuration is up to date : drush cex -y
- Update to 9.5.4 :

composer update -W drupal/core-recommended:9.5.4
drush updb -y
drush cex -y

Permissions in user.role.authenticated.yml is :

 permissions:
  - 'access administration pages'
  - 'access content overview'
  - 'access contextual links'
  - 'access files overview'
  - 'access toolbar'
  - 'access tour'
  - 'administer url aliases'
  - 'create article content'
  - 'create page content'
  - 'create terms in tags'
  - 'create url aliases'
  - 'delete article revisions'
  - 'delete own article content'
  - 'delete own page content'
  - 'delete page revisions'
  - 'edit own article content'
  - 'edit own comments'
  - 'edit own page content'
  - 'edit terms in tags'
  - 'revert all revisions'
  - 'view all revisions'
  - 'view own unpublished content'
  - 'view the administration theme'

- I commit this in git to see differences when i'll remove the permission and export again.
- Remove 'edit own comments' from /admin/people/permissions
- Export config : drush cex -y
- git diff on user.role.authenticated.yml is now :

 permissions:
-  - 'access administration pages'
-  - 'access content overview'
-  - 'access contextual links'
-  - 'access files overview'
-  - 'access toolbar'
-  - 'access tour'
-  - 'administer url aliases'
-  - 'create article content'
-  - 'create page content'
-  - 'create terms in tags'
-  - 'create url aliases'
-  - 'delete article revisions'
-  - 'delete own article content'
-  - 'delete own page content'
-  - 'delete page revisions'
-  - 'edit own article content'
-  - 'edit own comments'
-  - 'edit own page content'
-  - 'edit terms in tags'
-  - 'revert all revisions'
-  - 'view all revisions'
-  - 'view own unpublished content'
-  - 'view the administration theme'
+  0: 'access administration pages'
+  1: 'access content overview'
+  2: 'access contextual links'
+  3: 'access files overview'
+  4: 'access toolbar'
+  5: 'access tour'
+  6: 'administer url aliases'
+  7: 'create article content'
+  8: 'create page content'
+  9: 'create terms in tags'
+  10: 'create url aliases'
+  11: 'delete article revisions'
+  12: 'delete own article content'
+  13: 'delete own page content'
+  14: 'delete page revisions'
+  15: 'edit own article content'
+  17: 'edit own page content'
+  18: 'edit terms in tags'
+  19: 'revert all revisions'
+  20: 'view all revisions'
+  21: 'view own unpublished content'
+  22: 'view the administration theme'

@GoZ a normal upgrade path would have a cache clear in there. Did you clear cache before or after the drush updb? Because without that, the schema change from 🐛 Role permissions not sorted in config export Fixed would not be in effect.

Confirmed steps in #13 trigger the issue even with a cache clear.

Even worse - after adding a new permission it's tacked on to the end, i.e there's no sorting at all.

I've tracked this down to the fact that user_role_grant_permissions and user_role_revoke_permissions call trustData.

Setting trustedData = TRUE skips casting in validation

Config::castValue is where sorting is applied.

UserPermissionsForm::submitForm calls user_role_change_permissions which uses the 2 functions mentioned above.

This can be reproduced on 10.1.x, no need to upgrade or anything just simply save the permissions form.

Pushed a fix to the MR, keen to see what fails before working on dedicated tests. Hiding the patch as well.

I couldn't find any existing tests for the permissions form so I've added a new class (searched all tests for admin/people/permissions or user.admin_permissions).

This test is very simple and correctly fails when reverting either of the changes in the first commit:

Add fail:

$ ./app/vendor/bin/phpunit app/core/modules/user/tests/src/Functional/UserPermissionsAdminTest.php
PHPUnit 9.5.28 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\Tests\user\Functional\UserPermissionsAdminTest
F                                                                   1 / 1 (100%)

Time: 00:15.051, Memory: 10.00 MB

There was 1 failure:

1) Drupal\Tests\user\Functional\UserPermissionsAdminTest::testPermissionsSorting
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'change own username'
-    1 => 'view user email addresses'
+    0 => 'view user email addresses'
+    1 => 'change own username'
 )

/data/app/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/data/app/core/modules/user/tests/src/Functional/UserPermissionsAdminTest.php:48

Remove fail:

$ ./app/vendor/bin/phpunit app/core/modules/user/tests/src/Functional/UserPermissionsAdminTest.php
PHPUnit 9.5.28 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\Tests\user\Functional\UserPermissionsAdminTest
F                                                                   1 / 1 (100%)

Time: 00:15.442, Memory: 10.00 MB

There was 1 failure:

1) Drupal\Tests\user\Functional\UserPermissionsAdminTest::testPermissionsSorting
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'view user email addresses'
+    1 => 'view user email addresses'
 )

/data/app/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/data/app/core/modules/user/tests/src/Functional/UserPermissionsAdminTest.php:60

Checked core for usages of user_role_change_permissions, user_role_grant_permissions, and user_role_revoke_permissions and almost all usages are in tests, except this permissions form and in FilterFormat::postSave. I can't find any test coverage for the FilterFormat permissions.

Patch looks simple enough, and has a test.

The ::trustData call was added in #2432791: Skip Config::save schema validation of config data for trusted data → in comment 57 it seems.

Will ping @alexpott about this given he worked on both that patch and the sorting one.

I've tweaked the fix to do a partial revert of 🐛 Role permissions not sorted in config export Fixed so that the behaviour from before that is maintained. This ensures that the actions also still work as before. I think this is analogous to the existing code for dependencies in ConfigEntityBase...

    if (!$this->isSyncing()) {
      // Ensure the correct dependencies are present. If the configuration is
      // being written during a configuration synchronization then there is no
      // need to recalculate the dependencies.
      $this->calculateDependencies();
      // If the data is trusted we need to ensure that the dependencies are
      // sorted as per their schema. If the save is not trusted then the
      // configuration will be sorted by StorableConfigBase.
      if ($this->trustedData) {
        $mapping = ['config' => 0, 'content' => 1, 'module' => 2, 'theme' => 3, 'enforced' => 4];
        $dependency_sort = function ($dependencies) use ($mapping) {
          // Only sort the keys that exist.
          $mapping_to_replace = array_intersect_key($mapping, $dependencies);
          return array_replace($mapping_to_replace, $dependencies);
        };
        $this->dependencies = $dependency_sort($this->dependencies);
        if (isset($this->dependencies['enforced'])) {
          $this->dependencies['enforced'] = $dependency_sort($this->dependencies['enforced']);
        }
      }
    }

If you update from 10.0.0 to this MR you'll see

My changes are now too extensive to leave at RTBC...

This looks good!

Checking for $this->hasTrustedData() and doing the sorting here is ok. But long term we should make sure to avoid the trusting of data except in the install phase. And there we could do a sorting/casting after the installation. But this would need lots of other work with sorting etc and is clearly out of scope here.

So I RTBC this again.

I've opened an issue to deprecate the trusted data concept - see 📌 Deprecate the trusted data concept in configuration Active

This will need to be backported to 9.5.x

I asked @alexpott about a follow-up to deprecate user_role_grant_permissions(), turns out we have a ten year old issue 📌 Deprecate user_role_grant_permissions() and user_role_revoke_permissions() Needs work already.

Committed/pushed to 10.1.x and cherry-picked to 10.0.x. This needs a re-roll for 9.5.x

Diff applies to 9.5.x even though the cherry-pick doesn't, so committed/pushed there too.

Thanks for the quick turn around! @alexpott just curious why we went with that solution rather than removing the trustData call?

@acbramley because it is not just these methods. Anywhere that called trustData() on a role would no longer be sorted. I broke that with 🐛 Role permissions not sorted in config export Fixed when I shouldn't have.

Thanks all, that was fast !

Automatically closed - issue fixed for 2 weeks with no activity.