Problem/Motivation

Scenario:

  • A new node type 'article' creates a new 'edit any article content' permission.
  • A new text format 'safe_html' creates a new 'use text format safe_html' permission.

When those permissions are granted to a role, that role config entity will contain the 2 permissions.

If an object that is the foundation of a permission is removed the role doesn't revoke that permission. This is proved in the attached test.

Proposed resolution

  1. Make role config entity dependent on the provider of each granted permission.
  2. Make role config entity dependent on each entity (config or content) that was used to build that permission.
  3. The dynamic permission callbacks should optionally, return the dependencies used to build a specific permission (except the module which is already added as 'provider').
  4. The user_role config entity should implement onDependencyRemoval().
  5. Fix at least one use case. Let's fix filter_format in this issue.

Remaining tasks

@todo Open followups for all the cases when permissions are provide dynamically.

User interface changes

None.

API changes

Dynamic permission callbacks to optionally return dependencies.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it doesn't cleanup removed dependencies.
Issue priority Major because it leaves the configuration in an inconsistent state.
Disruption No disruption.
Files: 
CommentFileSizeAuthor
#26 2571235-26.patch14.29 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,568 pass(es). View
failing_test.patch2.76 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,276 pass(es), 3 fail(s), and 0 exception(s). View

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

Title: Roles should depend on objects that are responsible to build permissions » Roles should depend on objects that are building the granted permissions

Status: Needs review » Needs work

The last submitted patch, failing_test.patch, failed testing.

The last submitted patch, failing_test.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Active

Q.E.D.

claudiu.cristea’s picture

Issue summary: View changes

Update issue summary.

claudiu.cristea’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
10.29 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,077 pass(es), 28 fail(s), and 560 exception(s). View

Full patch.

I'm using filter_format as use case. For the others, I will fill followups.

Status: Needs review » Needs work

The last submitted patch, 7: 2571235-7.patch, failed testing.

The last submitted patch, 7: 2571235-7.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
11.63 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,477 pass(es), 4 fail(s), and 4 exception(s). View
2.79 KB

Status: Needs review » Needs work

The last submitted patch, 10: 2571235-10.patch, failed testing.

The last submitted patch, 10: 2571235-10.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: +rc target
FileSize
13.59 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,913 pass(es), 0 fail(s), and 232 exception(s). View
6.46 KB

Status: Needs review » Needs work

The last submitted patch, 13: 2571235-13.patch, failed testing.

The last submitted patch, 13: 2571235-13.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
13.6 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2571235-16.patch. Unable to apply patch. See the log in the details link for more information. View
806 bytes

Fixed the missed 'dependencies' key in permissions array.

claudiu.cristea’s picture

Issue summary: View changes

On a medium/long term I think that classes providing dynamic permissions must implement an interface and dependencies should be provided in a separate method of the class. I opened an 8.1.x followup #2574251: Dynamic permissions callbacks classes must implement an interface for this.

Added beta evaluation.

claudiu.cristea queued 16: 2571235-16.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 16: 2571235-16.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
12.96 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,080 pass(es), 107 fail(s), and 0 exception(s). View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 20: 2571235-20.patch, failed testing.

The last submitted patch, 20: 2571235-20.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
14.22 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,230 pass(es). View

test fixing.

Status: Needs review » Needs work

The last submitted patch, 23: 2571235-23.patch, failed testing.

xjm’s picture

Issue tags: -rc target

See https://groups.drupal.org/node/484788 for more information on the rc target tag.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
14.29 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,568 pass(es). View

Rerolled.

@xjm, I added that tag because there's reverse issue #2569741: FilterFormat should add the roles as config dependencies tagged by @catch in the same way. They are both dealing with config dependencies.

claudiu.cristea’s picture

A better plan, IMO, would be:

  1. Deprecate permission_callbacks array from *.permissions.yml. But keep it till 9.0.x to allow the existing custom modules to work.
  2. Create a new config entity type 'permission', holding everything that belongs to a permission: id (the actual permission key), label, description, etc.
  3. Add a new interface DynamicPermissionsInterface:
    interface DynamicPermissionsInterface {
      public function getPermissions();
      public function calculatePermissionsDependencies();
    }
      

    Here, calculatePermissionsDependencies(), would return dependencies in the same format as usual, but nested under a top-level permission key:

    return [
      'use text format full_html' => [
        'config' => [...],
        'content' => [...],
        ...
      ],
      ...
    ];
    
  4. Classes implementing DynamicPermissionsInterface are placed in the "Permissions" namespace. Example: Drupal\node\Permissions. Can be Drupal\node\DynamicPermissions?
  5. .

  6. Rework \Drupal\user\PermissionHandler::buildPermissionsYaml() to collect also permissions from all the classes implementing DynamicPermissionsInterface and stored in the namespace Drupal\{module}\Permissions and store all permissions in config entities of type 'permission'.
  7. The permission provider (the module) doesn't have to be added via DynamicPermissionsInterface. This will be magically, appended to the list of dependencies in PermissionHandler.
  8. Implements all needed dependency procedures in permissions and roles config entities. Example: (1) permission will depend on provider, (2) when the provider is uninstalled, the permission gets deleted but this means (3) all roles depending on that permission will be updated because we are implementing onDependencyRemoval on Role configurable.
  9. Convert the 12 occurrences from core modules to use the new API.

In 9.0.x just:

  1. Drop *.permissions.yml file. Provide static permissions as default config .yml files, under core/module/{module}/config/install as {module}.permission.{permission}.yml. Discuss how the {permission} name will be built (we can rely on a processed ->id() that only replaces spaces with dash '-'.
  2. Adapt PermissionHandler to only grab dynamic permissions from the Drupal\{module}\Permissions namespace.

This approach has the benefit of providing the BC but still prepare for a cleaner 9.0.0 approach and assuring permission caching through standard config entity cache.

And this is doable right now, in 8.0.x because it breaks no API and assures the BC!

EDIT: Added 9.0.x tasks.

alexpott’s picture

So I don't think we're going to get round to #27 now. We do have a problem with cleaning up roles when things are deleted that provide dynamic permissions. We have exactly the same problems for the node type based permissions.

claudiu.cristea’s picture

@alexpott, off-topic but related: permissions must be config entities in 9.x. Then we simply do config dependencies format <=> permission <=> role. That would fix a lot of other issues (like caching the permissions)

alexpott’s picture

Oh and we still don't cleanup permissions on uninstall of a module either.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpi’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.