Add ConfigEntityAccessController

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigAccessController.php
   @@ -0,0 +1,34 @@
   +class ConfigAccessController extends EntityAccessController {
   

   Let's rename this ConfigEntityAccessController, its a bit clearer (we should have another for ConfigStorageController too)

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigAccessController.php
   @@ -0,0 +1,34 @@
   +      $status_is_toggleable = $operation == 'disable' ? $entity->status() : !$entity->status();
   

   This line is the only part of the patch that is confusing. 'toggleable' is not really a word, and it needs a code comment, and the $entity->status() : !$entity->status() is really odd looking...

3. +++ b/core/modules/views_ui/views_ui.routing.yml
   @@ -52,7 +52,7 @@ views_ui.enable:
   -    _permission: 'administer views'
   +    _entity_access: view.enable
   
   @@ -61,7 +61,7 @@ views_ui.disable:
   -    _permission: 'administer views'
   +    _entity_access: view.disable
   

   Nice!

So what happens if you want to check if someone has access to enable an entity that is currently already enabled? This patch means it will always deny access. Yeah, that is what we want when building operation links, but that seems like the only time we want that behaviour?

I just meant the ConfigEntityAccessController class itself, not sandwiching 'Entity' in after the entity type name.

Love the commit message though :)

I've had a look at this:

• Patch applies fine
• Installs
• No error messages in log
• Code looks clean

Only thing I could spot is you haven't fixed the terminology of 'toggleable' as @tim.plunkett pointed out.

If you fix that I would say this is RTBC.

Great, this looks good to me!

S0, regarding my comment in #10, we are happy that when a config entity is enabled, access for ENTITY_TYPE:enable will always be false, regardless of the usage?

#10 does seem like a problem - and the logic is wrong with the current patch. A user who has access to enable a config entity should have that access regardless of the state of that entity.

Yeah, I'm with Xano on this one. Why would you want to allow enabling an already enabled entity? Can you elaborate @alexpott or @damiankloip?

What I did in the other issue is not related, because it's about delete.
It's not as much about the state of the entity, IMO.

If the entity is enabled, and we allow you to enable it, it's a no-op.
Do we want to deny access for no-op cases?
That's the question we need to answer here.

I don't have a strong opinion either way.

The last patch in #2084323: EntityForm::actions() adds 'delete' without checking access is about the same, that is about delete there but it's exactly the same pattern for deciding that access is not allowed if the entity is in a given state (new).

So in theory - from an API design perspective - I don't have a strong preference either. However due to the fact that we bind the visibility of the operation links to the access of the operation itself already I see no reason to loosen that binding here just for the sake of allowing people to enable already enabled entities on an API level. That's the reason why I'm in favor of that approach.

Yeah, we obviously do not want to show enable for a config entity that is already enabled, the only question I think is who is responsible for preventing that, is it the list builder that shouldn't even try to show that operation or should the operation exist but you wouldn't have access?

Given that we eventually ( in 9.x or contrib) want to move toward separately defined entity operations as plugins or whatever and tie them even more to the entity access control system, it makes more sense for me as well to handle visibility of operations in the access class.

Well, this is needs review at least.

Assigning to @alexpott as apparently there's no further progress here.

So @tim.plunkett doesn't really have a strong preference and it seems @Berdir seems to be leaning slightly towards going with the current approach. @Xano and I also favor the current approach.

@alexpott Do you feel strongly that the current approach is incorrect? And if so, can you explain why? Thanks a lot.

Okay let's proceed with this. I feel it might come back to bite us since access != visibility but I'm not going to argue anymore - as @Xano says it does make things more secure by default.

So say I am trying to enable some entity via a rest api I have created for my site, I would just get a 403? So I could not use entity access alone for anything like that?

But how does that work generically? You cannot just have a call that enables things without access checking.

Then how do you know if the entity is already enabled or you actually don't have access? :)

Hmmm... that's a very interesting use-case/question @damiankloip. I suppose 404 would be a more semantic response code for an enabling an already enabled entity. Then it would be consistent with e.g. delete:

# Delete an entity
POST /node/1/delete # get back 200
# Try to delete again
POST /node/1/delete # get back 404

# Enable an entity
POST /node/1/enable # get back 200
# Try to enable again
POST /node/1/enable # get back 404

That said, enable/disable, is really a different animal that delete (especially in REST-world), so maybe the above is not really a very semantic analogy. Maybe we really do want to return 200 in that case. Hmm...

If I was building an API, and from past experience of its consumption, returning a 200 in the case it is already enabled usually works out best. Which kind of goes against how this patch would work? or not?

Interesting question.

Keep in mind that...

- We have no actual integration with entity operations/links in rest.module. The links are there based on the link templates, but they are not actually useful for rest.module because it doesn't implement them nor are they how rest.module works otherwise. A delete is *not* "POST /node/1/delete", it's "DELETE /node/1".

The way to enable/disable a config entity *would* be to do a POST and change the status property. and we have no way to interact or check that, because config entity entities have no field/property level access control or validation. At least not until we have a typed data wrapper system that would work based on the config schema.

*Would* because we have no config entity integration *at all* for rest.module right now anyway ;)

So based on that, it's a problem that we currently don't have (because we have far bigger problems before we would even get to this ;)) but it's still an interesting use case why this would be the wrong approach.

Sure, I am saying if I was building an API though :) Not just what is provided by REST module. Yes, the examples from #61 were not correct but this is not a question about guidelines of a REST API :)

Maybe I misunderstand, but I am not reading into it as much as you are ....

I am thinking I basically cannot do this:

 // Inside my PUT/POST callback...
 if ($entity->access('enable')) {
   $entity->enable();
 }

Well, that would work fine. If the entity is enabled, then the acces() call would yield FALSE, but in that case the ->enable() call would be a no-op anyway. So the code would work either way. Or am I missing something?

I would rather a no-op and a sensible return value than an access denied. The point is I cannot just call access() with this patch. So if I did get a 403 back, is it because I am really not allowed, or that the entity is already enabled? If I try to enable and already enabled entity, returning with access denied would not be useful IMO.

If someone works at this in Montpellier, feel free to add it back

Moving to NW for some of the follow up discussion after it moved to NR

Wondering if this is still needed for 9.5?