Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes

This is a blocker of #2843139, see #2843139-78: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler.

A duplicate of this was created: #2907121: Move 'access content' permission to system module.

IS refined.

+++ b/core/modules/system/system.permissions.yml
@@ -24,3 +24,5 @@ link to any page:
+  title: 'View published content'

❓ I'm wondering whether we could add a description which makes it clearer that this is not just about nodes

I'm not really sure we want to move the permission from node.module to system.module as #2 proposes. I haven't caught up on everything that's been written in #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST yet, but for anyone else reviewing this, that would be a good place to start.

I do agree that it's currently a bug though for the places identified in this issue's Problem/Motivation section to be checking a permission that's defined in node.module. So we need to either move the permission, or else figure out one or more new permissions that would be more appropriate for those cases.

In trying to test what happens to those cases currently when node.module is uninstalled, I ran into the following quirks:

• node_install() grants 'access content' permission to everyone.
• Those grants are not removed when node.module is uninstalled. That's a separate bug: #2026983: [regression] Grants for permissions defined by a module are not revoked / removed from Role config entities when that module is uninstalled
• Both Minimal and Standard profile enable node module.
• Therefore, installing either of those profiles and then uninstalling node.module does not lead to expected access denials for the issue summary's use cases, such as machine name transliteration and the Who's New block.

However, if I locally edit minimal.info.yml, comment out the - node dependency and install the profile that way, then I can get the following bugs:

• Setup: Enable Views and add the Who's New block. Create a role and grant it all permissions. Create a user with that role and login as them.
• Bug 1: This user can't see the Who's New block, because it requires "access content" permission, which was not granted during the setup,
  since node module isn't installed.
• Bug 2: This user can create a new role, but on the page to do so, if they type in a role name with any non-ascii characters (such as from a non-English language), a machine name is not generated due to /machine_name/transliterate returning an access denied.

or else figure out one or more new permissions that would be more appropriate for those cases.

This would be a BC break.

I agree with #11 that creating new permissions for the current use cases would be BC break.

If we created new permissions we could in an update assign the new permissions to any role that currently role has 'access content' permission the new permissions we create. But I don't think that would cover BC concerns. Custom code and contrib could already have logic that checks for 'access content' permission for interacting with portions of their site that have to deal with viewing taxonomy with correct expectation that if they have that permission they will be able to view taxonomy terms.

Although I don't like the fact that we use 'access content' and we label nodes on the UI as 'content' and we use this permission for other things that aren't nodes I feel like we are stuck with it.

Pinged yoroy on IRC. I'm tempted to move this back to RTBC, but #10 means we need sign-off from a usability maintainer.

Not sure what to review. What's the decision or trade-off we're looking to make here?

Didn't even know all of the cases this permission was used. It is totally unclear that you grant access to other things than nodes when adding the permission for a user.
I agree separating the permission and creating new ones for the use cases that 'Access content' was abused is the real solution. I also see how this breaks BC.

If we are choosing to keep this permission and move it, I think we need to at least update the title as mentioned in #9.

One a side note:
Using 'Access content' for anything other than checking access to content (nodes) is wrong imho, so I wonder if we could think of a way to make this clear and fix this before Drupal 9.

#14: short version: the reality is that the node module's access content permission is being used for a lot more than just granting access to nodes (== "content"). Therefore this issue proposes to move that permission from the node module to the system module, to reflect reality.
The question for you: is this okay from a usability POV?

#15: RE sidenote: no, fixing this before Drupal 9 would be a BC break. And fixing it not before 9, but in 9, would also be a BC break, per https://dri.es/making-drupal-upgrades-easy-forever.

The issue summary says no user interface changes. If that's correct then I don't see how moving code around needs usability review. If it does change the UI or changes the meaning of what the permissions (don't) do or where people can find them then the issue summary needs an update.

@yoroy re #17 I have updated summary to include the UI changes, namely a change on admin/people/permissions

I am not convinced this is good from a usability point of view. All content related permissions are there, and we would suddenly be splitting of just one - but the most binary one, to a different category - to support a >10% usecase.

I suggest we add logic, that in the case the "node" module is disabled, that we move it to "system".

Updating with @Bojhan's suggestion.

I like this because it won't have an effect on vast majority of sites because most will have Node enabled.

Not providing a interdiff because no changes left from #2

Updated IS to reflect no "User interface changes" changes.

Thanks for the review @bojhan, it captured my thoughts as well. Glad to see we can accomodate the suggested added logic, thanks @tedbow.

+++ b/core/modules/system/system.permissions.yml
@@ -24,3 +24,5 @@ link to any page:
+permission_callbacks:
+  - \Drupal\content_moderation\Permissions::transitionPermissions

Whoops copy/paste error forgot to update to reference new class in System module

Bojhan++ — that strikes a nice middle ground! ❤️

+++ b/core/modules/system/src/Permissions.php
@@ -0,0 +1,61 @@
+   * If the 'node' module is enabled this method will provide the

s/enabled/not installed/

@Wim Leers thanks for review. Fixed

Update the summary to reflect the change to leave the permission in the node module but provide it in the system module if the node module is not enabled.

This blocks #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler, which is the last blocker for #2824572: Write EntityResourceTestBase subclasses for every other entity type.!

I like the approach but it looks as though we're missing some test coverage.

+++ b/core/modules/system/src/Permissions.php
@@ -0,0 +1,61 @@
+        'title' => $this->t('View published content'),

Can of worms but the word "published" is extremely problematic here. I have no idea what to do about that.

I like the approach but it looks as though we're missing some test coverage.

You'd like test coverage that shows that the access content permission is now available even when the node module is not yet installed, I think? Or is there something else you'd like to see tested?

Can of worms but the word "published" is extremely problematic here. I have no idea what to do about that.

Yes :( This is already problematic in HEAD. Legacy. Hard to fix. I do think this is out of scope here, since it's already a problem in HEAD. This issue/patch does not make the problem worse.

Yep a test that shows the perm is still there when node is not is exactly what we need. And yes I agree that not dealing with the published can of worms is acceptable here.

On it

Here you go.

@zaporylie thanks for the patch!

+++ b/core/modules/system/tests/src/Kernel/PermissionsTest.php
@@ -0,0 +1,45 @@
+    $permissions = $this->permissionHandler->getPermissions();
+    $this->assertArrayHasKey('access content', $permissions);

Instead of the checking for the check we could just check for
$permissions['access content']['provider'] === 'system'. Then we would still only need 1 assert but also confirm that system is providing it.

As you wish.

Probably no need to upload test-only patch again so I'm skipping it here for sake of DrupalCI.

+++ b/core/modules/system/tests/src/Kernel/PermissionsTest.php
@@ -0,0 +1,45 @@
+/**
+ * @group Drupal
+ */

I think @group should be "system" here since Permissions.php is system module..

should we add @covers here?

I've used @group Drupal because neighbour test file within the same dir was using it (PathHooksTest.php). But I see now most of system Kernel tests use @group system so that should be probably changed. I'll add issue to address incorrect test group in PathHooksTest.php file.

Adding @covers for now - it can be removed on commit if not needed.

@zaporylie looks great. RTBC again since was RTBC in #27

We now have tests per @alexpott's request in #29.

I agree with @Wim Leers in #29 that the 'View published content' permission string is existing problem and out of scope here

+++ b/core/modules/system/tests/src/Kernel/PermissionsTest.php
@@ -0,0 +1,48 @@
+  public function testAccessContentPermission() {
+    $permissions = $this->permissionHandler->getPermissions();
+    $this->assertSame('system', $permissions['access content']['provider']);
+  }

I think this is a good start, but we want a bit more.

After this, install the node module, then check that the provider is 'node', not 'system'.

And then uninstall the node module, and check that the provider is again 'system'.

@Wim Leers good point.

Here is this test changes. They aren't passing for me locally. I am not sure if I need to do anything besides install the module to get the permissions to reset.

The reason #41 fails is pretty simple: you're using services from before the module was installed, i.e. without a rebuilt container. Converting this to a BrowserTestBase test will make that easier. Confusing indeed — I've pulled my hair out over this in the past.

Also fixed a bunch of nits.

+++ b/core/modules/system/tests/src/Kernel/PermissionsTest.php
@@ -0,0 +1,44 @@
+   * Tests 'access content' permission existence.

Oh one more nit: s/existence/provider/

Since you transformed Kernel test into Functional test, namespace should reflect that change.

It can still be a KernelTestbBase - in fact it is slightly easier with a KTB because its container gets updated when the container is rebuilt via the module installer.

Fixed #44 too. And #45 - because it is a KTB again :)

Just a comment on the earlier patches:

+++ b/core/modules/system/tests/src/Kernel/PermissionsTest.php
@@ -0,0 +1,65 @@
+    $this->permissionHandler = $this->container->get('user.permissions');
+    $this->moduleInstaller = $this->container->get('module_installer');

This type of storing a service on the test class is not a great pattern. It is super vulnerable to container rebuilds and things not being in-sync and not testing what you think it does.

Thanks @alexpott for your patch (#46), and even more for taking the time to comment on container handling in tests classes (#47) - much appreciated!

@alexpott++++++ I tried to achieve that for 10 minutes, but failed, figured that #43 was still a step forward :) Thanks for the explanation! ❤️

ditto @alexpott++ thanks for the explanations!

This is going to be tricky for #2026983: [regression] Grants for permissions defined by a module are not revoked / removed from Role config entities when that module is uninstalled / #2571235: [regression] Roles should depend on objects that are building the granted permissions since they are removing permissions when modules are uninstalled. At least for the version that uses proper config dependencies to manage this I think we might have to hard code something special.

It'd be best if the permission could just be in the system module - the whole moving to the node module so that to keep the tickbox in the node section when the node module is installed is a bit icky. Maybe we could move it to system and change the title? Not sure. Very tricky. Or maybe just hack the form to special case it and not do the whole shimmy with the code. In fact, thinking about it I'd way prefer the permission to always by provided by system and the weird logic to list in the node permissions to be in the form,

The reason for my preference is once roles get proper config dependencies this will make everything much simpler.

Here's a patch that maintains the position but ensures that the permission is always provided by system.

WFM!

I'd ask for node access maintainer signoff; fortunately that's me. +1 for this approach.

I don't think there should be any BC impacts from this as System module can't be disabled, but let's provide a CR to inform developers?

Created the change record - https://www.drupal.org/node/2921484

This is at least a major since it is blocking #2571235: [regression] Roles should depend on objects that are building the granted permissions

Committed and pushed to 8.5.x, and published the change record.

YAYYYYYYY!!!!!!!!!!!! I'm working on the follow-ups.

1. Rerolled #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, which is now less blocked, but still blocked (on another issue)
2. Queued #2339235: Remove taxonomy hard dependency on node module for retesting, @dagmar is already rerolling it.
3. Rerolled #2571235: [regression] Roles should depend on objects that are building the granted permissions.