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

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

@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 #3039499: Role permissions not sorted in config export 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 #3039499: Role permissions not sorted in config export 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 #3347842: Deprecate the trusted data concept in configuration

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 #2025089: Deprecate user_role_grant_permissions() and user_role_revoke_permissions() 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 #3039499: Role permissions not sorted in config export when I shouldn't have.