NodeAddAccessCheck allows roles holding the "Administer content types" permission to create nodes

Changing permissions in failing test cases

Fixing few more permissions

The reason this exists is that you can create content types with that permission, otherwise we would not allow access to node/add if there are no node types.

Having that permission but not being able to create any content and there being node types on the page seems like a pretty weird special case. You could create node types that you then can't create content for?

Also, administer nodes is definitely not better. If anything, it would need to be bypass, but then you never get to that point if there are node types.

\Drupal\Core\Entity\EntityCreateAnyAccessCheck::access() was added in a consistent way, on purpose. and it was discussed in #2696669: Incomplete/broken access checking in EntityController::addPage().

Note that you can *not* create any content. You simply see the page with a link to create a new type, which is exactly what this page allows.

Works as designed IMHO

Note that you can *not* create any content. You simply see the page with a link to create a new type, which is exactly what this page allows.

Huh? According to the IS, you *can* visit and submit the /node/add/[content type] page with merely the "Administer content types" permission and no other permission. I tried this just now and found it to be true as well. This does mean that someone with just "Administer content types" permission is able to create a node of any type. I don't know if that's by design or not, but there's nothing in the name or description of that permission that communicates that.

Oh wow, you are right. node/add works correctly and shows that you are not allowed to create any types, but if you know how they are named, then you can acess them.

This might be a regression from #2431281: Drop support for _access_mode routes and always assume ALL which added the content types check to \Drupal\node\Access\NodeAddAccessCheck::access() but doesn't limit it to the non-content type specific case. I think it should be below that if ($node_type) if.

Lets see if using the generic implementation works or results in test fails, we might also want to fix the access checks, but IMHO we probably want to deprecate them in favor of the generic ones anyway.

Interesting find! I totally agree with @berdir and @effulgentsia that this is NOT the expected behaviour.

This might be a regression from #2431281: Drop support for _access_mode routes and always assume ALL which added the content types check to \Drupal\node\Access\NodeAddAccessCheck::access() but doesn't limit it to the non-content type specific case. I think it should be below that if ($node_type) if.

I think this analysis is totally right. We simply didn't thought about the fact that this class is used in multiple places.

To get this issue done we IMHO though need tests :)

Oops, was missing an _.

Ok, so we have a bunch of tests failing, fixed those, often happens because they create new node types and don't add permission for them. I just added the bypass permission.

Also added a new test that should fail on HEAD.

Still need to deprecate the node specific access check. Won't be very visible and I guess needs a change record, so maybe a separate issue?

Oh well, lets just do it :)

https://www.drupal.org/node/2836069 is the change record.

The core committers and the entity team discussed this issue at a recent triage meeting. It was decided to downgrade this issue to normal priority. The reasoning behind the decision is that Administer content types is a restricted access permission that provides many ways to change the configuration of a site. The fact that a role holding Administer content types could add content is not significant enough to raise the issue priority.

I ran into this too today. The "Add content" action at /admin/content does NOT show up when the user has the administer nodes permission. Fixing NodeAddAccessCheck fixes it.

Rebased this patch… only to find out that this still does not solve that. :/

This change fixes my use case. I think it ought to be integrated into this patch/issue.

(And wow, I never knew we had so many similar things: bypass node access, administer nodes, et cetera!)

I don't think that is correct. Administer nodes does not allow create access. It only gives access to a bunch of node fields like author and status.

Yes, It is a very weird permission, basically all the left-overs after splitting bypass and access overview away.

:O That was not clear to me at all.

So how does one allow a user to CRUD all nodes, and allow to create nodes of any bundle, without granting administer content types, which also allows changing field configuration? It sounds like there's no single permission for that at all?

That's bypass node access

Oh wow. This is … very confusing :( Thanks for clarifying, @Berdir!

So, my patch in #22 should be ignored.

We should continue with #18, which I rerolled in #21, but I failed to resolve one of the conflicts. Resolved that last conflict. (That's the least I could do in exchange for your guidance, @Berdir!)

No problem, I'll take a look at this next week, currently enjoying the amazing weather on Jersey/Guernsey for another week :)

WOOOT, nice! Enjoy :) 🏝

So much for "next week".

The patch looks fine to me, I think it was just confused about changed context in that test.

This should fix the date test fails.

It feels like it is a tiny bit safer to use create date_content content instead of bypass node access. Do we care about that level of test coverage?

THis would require quite some chances to those tests. Most tests actually use test entities, it's just some methods that even create that node type, and when the type doesn't exist yet, we can't create a user with that permision, so we'd have to get his role and update the role.

The test is about testing date widget, I don't think it's relevant what kind of access we grant there. Pretty sure we also wouldn't actually need administer content types permission, as we create the node type through the API.

Fair point, I was just afraid of accidentally loosing some sort of access test coverage, but I agree with your argument. This is not about node access test coverage, but rather purely about date module functionality.

Random fail?

Green again, back to RTBC.

Also, leaving that for the maintainers to decide, but I think this is still fine for 8.4?

Thanks @Berdir.

+++ b/core/modules/node/node.services.yml
@@ -16,6 +16,7 @@ services:
+    deprecated: The "%service_id%" service is deprecated. Use _entity_create_access or _entity_create_any_access access checks instead.

+++ b/core/modules/node/src/Access/NodeAddAccessCheck.php
@@ -12,6 +12,9 @@
+ * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0. Use
+ * _entity_create_access or _entity_create_any_access access checks instead.

We should reference the change record for this change here as well. (See examples at https://www.drupal.org/core/deprecation.)

Also I think we need to fix the "in Drupal 8.3.0"; it's at least 8.4.0 now. I would backport this up to the beta code freeze (so through Aug. 15) since this is a security hardening, but thereafter it's 8.5.x only since it's disruptive.

Oh, re: the datetime tests, I had the same question as @dawehner when reading the test, but when you look at the permissions list, it already grants everything and the kitchen sink permission-wise, so I think it's fine as it is in the patch.

Oh and also the @deprecated is missing its indentation for the second line.

Jersey/Guernsey

Readers might be interested to know that both Jersey and Guernsey are also breeds of dairy cow. I know this because I am from Wisconsin, which we have established is higher in per capita cow population than Switzerland, though a lower total. ;)

Jersey/Guernsey

Readers might be interested to know that both Jersey and Guernsey are also breeds of dairy cow. I know this because I am from Wisconsin, which we have established is higher in per capita cow population than Switzerland, though a lower total. ;)

I would like to point out that I was *not*, at any time, *on* a cow, but only on the islands those cows are coming from. But yes, they're proud of their cows there ;)

Also, addressed the deprecetion messages, is it correct like that?

Reroll of #45

Fixed failing test, updated deprecation format in node.services.yml

Updated issue summary. Removed link to https://www.drupal.org/node/132202 as that is for Drupal 6.

This looks ready to go to me, it was RTBC before and moved back to update the deprecation format.

Committed/pushed to 9.3.x, thanks!

Since this has a new deprecation, not backporting to 9.2.x - also if a site is for some reason relying on this behaviour with its permissions set-up, then probably better for them to find out in a minor release.

This actually managed to break a test in token module. No big deal, just happened to work before and I needed to add the extra permission. But, I do wonder if we should mention this in a change record or even the release notes?

We have a change record but it only mentions the deprecation, maybe use the same change record and add a note about the permission at the top?

Not sure about a release note because that's really for things potentially affecting sites, and this should really only affect test coverage.

Ended up just adding a new change record for this issue.

🐄