Problem/Motivation

When using a ConfigFactoryOverrideInterface to override permissions, these new permissions will be displayed on /admin/people/permissions. So when saving the form, the overridden permissions will be saved to the database.

Steps to reproduce

Implement a permission override in a module:


namespace Drupal\foo\Config;

use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Config\ConfigFactoryOverrideInterface;
use Drupal\Core\Config\StorageInterface;

/**
 * Example configuration override.
 */
class PermissionOverrides implements ConfigFactoryOverrideInterface {

  /**
   * {@inheritdoc}
   */
  public function loadOverrides($names) {
    $overrides = [];
    if (in_array('user.role.webmestre', $names)) {
      $overrides['user.role.webmestre']['permissions'][9999] = 'foo';
    }
    return $overrides;
  }

  /**
   * @return string
   */
  public function getCacheSuffix() {
    return 'foo';
  }

  /**
   * @param string $name
   *
   * @return \Drupal\Core\Cache\CacheableMetadata
   */
  public function getCacheableMetadata($name) {
    return new CacheableMetadata();
  }

  /**
   * @param mixed $name
   * @param string $collection
   *
   * @return \Drupal\Core\Config\StorableConfigBase|null
   */
  public function createConfigObject($name, $collection = StorageInterface::DEFAULT_COLLECTION) {
    return NULL;
  }

}

Then browse to /admin/people/permissions and save the form.
The overridden permission is saved in the config.

Proposed resolution

UserPermissionsForm should load the permissions from the config without any override.

Remaining tasks

N/A

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3196245

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

prudloff created an issue. See original summary.

prudloff’s picture

Status: Active » Needs review
StatusFileSize
new720 bytes

The attached patch loads the permissions without overrides.

ilgnerfagundes’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new119.84 KB

I applied the patch and did some tests and for me it's working.

longwave’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new768 bytes

Patch in #2 has the wrong path and doesn't apply.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tagging for tests to show the issue.

Did not test.

prudloff’s picture

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

prudloff’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

I added some tests.

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work

Small comments on the MR.

Thank you for starting with test coverage, guarantees this will probably get done faster

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

prudloff’s picture

Status: Needs work » Needs review

I committed the type suggestions.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed.

catch’s picture

Status: Reviewed & tested by the community » Needs work

One comment on the MR.

berdir’s picture

Agreed with @catch, also making this a child issue of #2910353: Prevent saving config entities when configuration overrides are applied which is about enforcing this in the long-term for all config.

prudloff’s picture

Status: Needs work » Needs review
berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks good to me now, one small step toward resolving the parent issue.

berdir’s picture

I had a look at the subclasses as well now that this overides getRoles(), it should be fine.

I didn't verify it, but \Drupal\user\Form\UserPermissionsRoleSpecificForm::getRoles() uses the already loaded route from the route, that _should_ be the one without overrides thanks to \Drupal\Core\ParamConverter\AdminPathConfigEntityConverter.

  • catch committed 02513abe on 11.x
    Issue #3196245 by prudloff, longwave, smustgrave, berdir, catch:...

  • catch committed f1bd4c55 on 10.4.x
    Issue #3196245 by prudloff, longwave, smustgrave, berdir, catch:...

  • catch committed 892267a1 on 10.5.x
    Issue #3196245 by prudloff, longwave, smustgrave, berdir, catch:...
catch’s picture

Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 11.1.x, 10.5.x and 10.4.x, thanks!

  • catch committed 53634233 on 11.1.x
    Issue #3196245 by prudloff, longwave, smustgrave, berdir, catch:...

Status: Fixed » Closed (fixed)

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