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. Add proper dependencies to all dynamic permissions

Remaining tasks

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.
CommentFileSizeAuthor
#80 2571235-80.patch48.7 KBalexpott
#80 77-80-interdiff.txt6.84 KBalexpott
#77 2571235-77.patch42.5 KBalexpott
#77 74-77-interdiff.txt2.08 KBalexpott
#75 2571235-74.patch40.42 KBalexpott
#75 72-74-interdiff.txt523 bytesalexpott
#72 2571235-72.patch39.91 KBalexpott
#72 68-72-interdiff.txt6.12 KBalexpott
#69 66-68-interdiff.txt1.12 KBalexpott
#68 2571235-68.patch35.72 KBWim Leers
#68 interdiff.txt1.36 KBWim Leers
#66 2571235-66.patch35.6 KBalexpott
#66 61-66-interdiff.txt2.93 KBalexpott
#61 2571235-61.patch34.69 KBalexpott
#61 59-61-interdiff.txt5.17 KBalexpott
#59 2571235-59.patch29.5 KBalexpott
#59 55-59-interdiff.txt17.1 KBalexpott
#57 interdiff-2571235.txt4.36 KBdawehner
#57 2571235-55.patch19.85 KBdawehner
#52 2571235-52.patch19.73 KBalexpott
#52 42-52-interdiff.txt2.03 KBalexpott
#51 40-42-interdiff.txt3.87 KBalexpott
#42 2571235-42.patch17.71 KBdawehner
#40 36-40-interdiff.txt454 bytesalexpott
#40 2571235-40.patch16.21 KBalexpott
#36 2571235-36.patch16.21 KBalexpott
#26 2571235-26.patch14.29 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,568 pass(es). View
#23 2571235-23.patch14.22 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,230 pass(es). View
#20 2571235-20.patch12.96 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,080 pass(es), 107 fail(s), and 0 exception(s). View
#16 interdiff.txt806 bytesclaudiu.cristea
#16 2571235-16.patch13.6 KBclaudiu.cristea
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
#13 interdiff.txt6.46 KBclaudiu.cristea
#13 2571235-13.patch13.59 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,913 pass(es), 0 fail(s), and 232 exception(s). View
#10 interdiff.txt2.79 KBclaudiu.cristea
#10 2571235-10.patch11.63 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,477 pass(es), 4 fail(s), and 4 exception(s). View
#7 2571235-7.patch10.29 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,077 pass(es), 28 fail(s), and 560 exception(s). 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
Members fund testing for the Drupal project. Drupal Association Learn more

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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

alexpott’s picture

Re-rolled the patch after a couple of years. I think we should do this instead of #2026983: [regression] Grants for permissions defined by a module are not revoked / removed from Role config entities when that module is uninstalled.

No interdiff because it's been 2 years and by the looks of it #26 and quite a few unrelated changes in it so an interdiff is just going to confuse matters and be unnecessarily big.

alexpott’s picture

We need to implement and test for:

  • content_translation
  • rest

We need to add test coverage for:

  • content_moderation
  • field_ui
  • vocabulary

Status: Needs review » Needs work

The last submitted patch, 36: 2571235-36.patch, failed testing. View results

alexpott’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
Issue tags: +Needs tests, +Needs upgrade path

core/modules/taxonomy/src/TaxonomyPermissions.php has changed in 8.5.x so we're going to need to do this there. And I think it is likely this will be 8.5.x only because it will need an upgrade path.

alexpott’s picture

Status: Needs review » Needs work

The last submitted patch, 40: 2571235-40.patch, failed testing. View results

dawehner’s picture

This does not yet resolve the test failures.

alexpott’s picture

Ah the fun of module install :) So the fails are caused by calling $format->url() whilst creating configuration during a module install and the route provider can't kind the new routes of previous installed modules. Fortunately we already have a fix for this - #2913912: URL generator may have a stale route provider during module installation.

alexpott credited Berdir.

alexpott credited dmouse.

alexpott credited herom.

alexpott credited longwave.

alexpott’s picture

Title: Roles should depend on objects that are building the granted permissions » [regression] Roles should depend on objects that are building the granted permissions
Priority: Major » Critical

I have closed #2026983: [regression] Grants for permissions defined by a module are not revoked / removed from Role config entities when that module is uninstalled in favour of this issue because the approach to fixing the issue is more correct. However that means that this issue should be critical for the reasons that #2026983: [regression] Grants for permissions defined by a module are not revoked / removed from Role config entities when that module is uninstalled was. This is also a regression from Drupal 7. I'm crediting the people who worked on that issue.

alexpott’s picture

Issue summary: View changes

Added blockers to issue summary.

alexpott’s picture

FileSize
3.87 KB

Here's an interdiff for #42

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
19.73 KB

Here's the patch with the fix from #2913912: URL generator may have a stale route provider during module installation to see if anything else is broken.

Status: Needs review » Needs work

The last submitted patch, 52: 2571235-52.patch, failed testing. View results

claudiu.cristea’s picture

Some thoughts:

  • Shouldn't we test also the case with dependencies declared on a static permission?
  • It still needs upgrade path and tests. We have to test if the stale permissions are removed from roles.

Nits:

  1. +++ b/core/modules/user/src/Entity/Role.php
    @@ -186,4 +186,67 @@ public function preSave(EntityStorageInterface $storage) {
    +      // @todo Remove this if() when https://www.drupal.org/node/2569741 is in.
    

    Let's drop the @todo. Seems that #2569741: FilterFormat should add the roles as config dependencies is a "won't fix"

  2. +++ b/core/modules/user/tests/src/Kernel/UserRoleDeleteTest.php
    @@ -70,5 +74,73 @@ public function testRoleDeleteUserRoleReferenceDelete() {
    +    // The role $role has the permissions to use $format and edit $node_type.
    ...
    +    // The $role config entity exists before removing dependencies.
    ...
    +    // The $role config entity exists after removing the config dependency.
    ...
    +      // The $format permission should have been revoked.
    ...
    +    // The $role config entity exists after removing the module dependency.
    ...
    +      // The $node_type permission should have been revoked too.
    

    We use to use the standard "Check that ..." in comments explaining the assertions.

  3. +++ b/core/modules/user/tests/src/Kernel/UserRoleDeleteTest.php
    @@ -70,5 +74,73 @@ public function testRoleDeleteUserRoleReferenceDelete() {
    +    if ($this->assertNotNull($role = Role::load($rid))) {
    ...
    +    if ($this->assertNotNull($role = Role::load($rid))) {
    

    I don't think assertions are returning something in PHPUnit. Probably this if() is left here from the WebTestBase version of the test?

alexpott’s picture

Status: Needs work » Needs review

Thanks for the review @claudiu.cristea

  1. I'm not sure why this if actually exists - if a permission does not exist as this point then shouldn't it be removed?
  2. I've never come across this standard and don't feel particularly strongly about that. These are comments you wrote btw :) this test is from your patch in #26
  3. Good point. fixed.

I outlined the need for an upgrade path and tests in #37 and #39.
The static permission thing - I'm not sure we should even support that - what would the use case be? These only really can depend on their provider.

The last submitted patch, 42: 2571235-42.patch, failed testing. View results

dawehner’s picture

Here are some test failure fixes, not all yet.

I don't think assertions are returning something in PHPUnit. Probably this if() is left here from the WebTestBase version of the test?

Nice catch!

We use to use the standard "Check that ..." in comments explaining the assertions.

I don't think we have anything like a standard about that :)

Status: Needs review » Needs work

The last submitted patch, 57: 2571235-55.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
17.1 KB
29.5 KB

Some more fixes. The implementation of \Drupal\user\Entity\Role::onDependencyRemoval has been rewritten.

Status: Needs review » Needs work

The last submitted patch, 59: 2571235-59.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.17 KB
34.69 KB

Oh wow - so a lot of the API first tests added are depending on permissions that don't actually exist!

Status: Needs review » Needs work

The last submitted patch, 61: 2571235-61.patch, failed testing. View results

Wim Leers’s picture

#61: Oh, hah! So evidently it's possible to grant non-existent permissions and have them be respected by the permission checking logic … otherwise those API-First tests could never have passed!

Wim Leers’s picture

Issue tags: +API-First Initiative

This helps API-First Drupal become more reliable (in core, but also in the contrib JSON API and GraphQL modules), so tagging API-First Initiative.

dawehner’s picture

#61: Oh, hah! So evidently it's possible to grant non-existent permissions and have them be respected by the permission checking logic … otherwise those API-First tests could never have passed!

Its indeed possible. I'm wondering whether we should add validation of some kind on the grantPermissio
level. This would still lead to a fast runtime, but we have a form of validation at least.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Needs followup
FileSize
2.93 KB
35.6 KB

@dawehner I think we should explore whether or not we should do that in a follow-up. I guess there is a question about whether or not this change in behaviour is allowed even in a minor. The problem being that granting non-existent perms is a critical as well because I think it is a data integrity issue.

Here's a patch that might be green. We have test coverage for:

  • content_moderation
  • content_translation
  • field_ui

Missing test coverage for

  • rest
  • node
  • filter
  • taxonomy

There is some implied test coverage for node and filter in UserRoleDeleteTest::testDependenciesRemoval() but I think we should have explicit test coverage that the dependencies are added correctly in each module that provides dynamic permissions that depend on things other than the module.

In the issue summary it used to say

Fix at least one use case. Let's fix filter_format in this issue.

I don't think this is a good way to go because once we add dependencies to roles than we need to try to keep them correct all the time. Doing this in steps just means we have lots and lots of upgrade headaches. Rather than one. We still need to write the upgrade path and tests for it.

The interdiff attached is pseudo interdiff because #2913912: URL generator may have a stale route provider during module installation landed so I had to rebase.

Wim Leers’s picture

Expanded test coverage to explicitly ensure that the access content case is handled correctly. I'm not sure if it really is that very useful though…

@alexpott: feel free to remove this small bit of extra test coverage for checking the #2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes special case explicitly, if you feel it doesn't add much (and feel free to uncredit me).

alexpott’s picture

FileSize
1.12 KB

The interdiff on #68 is not correct. Here's the correct one. I think having explicit testing about not removing the access content permission is fine. It proves that other non-related permissions are maintained through module uninstall.

Wim Leers’s picture

:O how the hell did that #68 interdiff end up looking like THAT!? I haven't seen any changes like that in as long as I can remember. I'd swear d.o mixed up files here… :O

The #69 interdiff is the one I remember seeing an hour ago, thanks for uploding that.

And: great!

Status: Needs review » Needs work

The last submitted patch, 68: 2571235-68.patch, failed testing. View results

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.12 KB
39.91 KB

Patch adds test coverage for

  • rest
  • node
  • filter
  • taxonomy

And reverts the change to ModuleUninstallTest because access content is now provided by System there are no config updates when uninstalling Node.

alexpott’s picture

Issue summary: View changes

@Berdir talked to me about this issue on IRC and he brought up a great point. During Drupal install there's the ability for install profiles to provide alternate configuration to be used in place of the configuration provided by the module. People are using this to provide alternate user.role.anonymous and user.role.authenticated config entities. These entities have permissions that do not exist when user is installed. Therefore we have a big problem because the current implementation of this patch is likely to remove those permissions. Ideas of how to fix this are welcome!

Berdir’s picture

Thanks for writing that down here.

I mostly just wanted to make sure that we are aware of that problem, install profiles config/overrides are excluded from BC, we've broken that before e.g. for views and so on. But if we do, we should at least make sure we are aware of the impact and to document in the change record what should be done.

The only viable alternative that currently exists is to move that to hook_install() of the install profile and grant the permissions there once all modules are enabled.

That said, this is a general problem that doesn't just affect this, it's especially common in form/view displays, views and other complex config with a lot of dependencies.. e.g when you want to add another custom field that some module in your install profile adds to admin/content. A possible generic solution would be a config/post_override folder, which is config that would only be replaced once the installation is finished.. Or config_installer based install profiles, although I could imagine problems there as well, e.g. when the auth/anon user roles are installed only very late due to dependencies and other modules somehow rely on them in code.

alexpott’s picture

Here's a patch which shows the nature of the problem @Berdir is talking about... it should fail.

Status: Needs review » Needs work

The last submitted patch, 75: 2571235-74.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
42.5 KB

So one idea would be to not override configuration entities that are provided by profiles. And just update the configuration when the profile is installed. This seems to work. Perhaps we could just do all the profile updates when the profile is being installed and not do the early swap out.

Hmmm... actually that won't work because profiles can provide overrides for modules that are optional... ie. be installed after Drupal has be installed.

Patch attached tries to get the best of both worlds - also ConfigInstallProfileUnmetDependenciesTest is still failing because in this instance the dependencies can now be met!

Status: Needs review » Needs work

The last submitted patch, 77: 2571235-77.patch, failed testing. View results

alexpott’s picture

So let's split off the installer stuff into a new blocker to make it easier to review / think about in isolation... created #2922417: Profile provided configuration entities can have unmeetable dependencies

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs upgrade path
FileSize
6.84 KB
48.7 KB

Patch includes an upgrade path and a test and the current patch from #2922417: Profile provided configuration entities can have unmeetable dependencies

So I think once the blocker lands we are good to go here.

effulgentsia’s picture

I don't understand the nature of the problem in #75.

These entities have permissions that do not exist when user is installed.

Why isn't the following enough to fix that?

+++ b/core/profiles/testing_config_overrides/config/install/user.role.authenticated.yml
@@ -0,0 +1,11 @@
+dependencies:
+  module:
+    - tour
effulgentsia’s picture

Berdir’s picture

@effulgentsia because you can't install any module before installing user.module, that won't end well :) Also, config dependency order doesnot override module order, it just makes it fail if the dependencies aren't already installed. But maybe your "never mind" meant you got that already :)

Wim Leers’s picture

Issue tags: -API-First Initiative

On second thought, I don't think this is a significant impact for API-First. I'm disagreeing with myself in #64 :) Untagging.

larowlan’s picture

Can we get a change record here too - because obviously contrib etc will need to add their support for it.

I really like the approach here - nice work

@effulgentsia, @xjm, @plach, @catch and I discussed this on a triage call and agree this is critical, primarily because the presence of the permission on the role grants access, even if the permission doesn't exist.

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityFormDisplay/EntityFormDisplayResourceTestBase.php
    @@ -11,7 +11,7 @@
    -  public static $modules = ['node'];
    +  public static $modules = ['node', 'field_ui'];
    

    I assume this is because we're using permissions with dependencies in tests - where previously those modules weren't enabled?

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -450,12 +450,17 @@ public function testGet() {
    +      // Filter out definitions that do not have a plugin.
    +      // @see \Drupal\rest\Plugin\rest\resource\EntityResource::addLinkHeaders()
    +      $link_relations = array_values(array_filter(array_keys($this->entity->getEntityType()->getLinkTemplates()), function ($relation_name) use ($link_relation_type_manager) {
    +        return $link_relation_type_manager->hasDefinition($relation_name);
    +      }));
           $expected_link_relation_headers = array_map(function ($relation_name) use ($link_relation_type_manager) {
             $link_relation_type = $link_relation_type_manager->createInstance($relation_name);
             return $link_relation_type->isRegistered()
               ? $link_relation_type->getRegisteredName()
               : $link_relation_type->getExtensionUri();
    -      }, array_keys($this->entity->getEntityType()->getLinkTemplates()));
    +      }, $link_relations);
    

    unrelated?

  3. +++ b/core/modules/user/src/Entity/Role.php
    @@ -186,4 +186,68 @@ public function preSave(EntityStorageInterface $storage) {
    +      // @todo Why is this necessary? Surely for a permission to be on role it
    +      // has to exist.
    

    Should we have a follow up for this?

  4. +++ b/core/modules/user/src/Entity/Role.php
    @@ -186,4 +186,68 @@ public function preSave(EntityStorageInterface $storage) {
    +        $name = in_array($type, ['config', 'content']) ? $dependency->getConfigDependencyName() : $dependency;
    ...
    +      if (in_array($permission_definitions[$permission]['provider'], $dependencies['module'], TRUE)) {
    

    can we use strict checking here because they're strings

  5. +++ b/core/modules/user/src/PermissionHandlerInterface.php
    @@ -34,7 +34,20 @@
    +   *     The module providing this permission doesn't have to be added as
    +   *     dependency, because is automatically parsed, stored and retrieved from
    

    nit 'as a dependency'

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.