Entity::access should default to FALSE

[docwilmot]

Not sure about this, but had the thought so posting. Tried debugging a new module Registration because anonymous was getting access that they shouldn't. Traced this to our implementation of entity_access(). In D7 entity_access() would check for an access callback on the hook_entity_info() of the entity; if there was none, then I presume the default return would be NULL. But for Backdrop, our version of entity_access() looks for an access() method on the entity. D7 ports would not have this access() method on their entities, so the method on the parent class (Entity::access()) ends up being called.

Entity::access() just returns TRUE. So by default, seems any call to entity_access() on a module port would return TRUE.

Should we default to FALSE?

I suppose if you're porting a module you should recognize this (part of the reason to document this here, for any searchers) but in case you don't, and thought you had access covered like it was in D7, you'd end up exposing private stuff to anonymous. Maybe we should help by defaulting to preventing that.

[indigoxela]

Entity::access() just returns TRUE

Hm, that sounds like a dangerous default... api page of Entity::access. The same happens in Entity::createAccess

On the other hand - changing a default value might cause trouble, too.

My feeling says: let it default to NULL, but I didn't dig deeper.

[klonos]

OMG, it should really default to FALSE (or NULL) - it does seem like a security risk otherwise 🤔

Pinging @backdrop/core-committers

[klonos]

...my personal opinion is that we should fix this, even if it turns out to "break" contrib. It will not really be a breakage rather than security hardening by default.

[argiepiano]

...my personal opinion is that we should fix this, even if it turns out to "break" contrib. It will not really be a breakage rather than security hardening by default.

I agree, this needs to be fixed. But I wanted to mention that most contrib ports from D7 that used Entity API there will/should use entity_plus_access() rather than entity_access() in Backdrop. entity_access() did not exist in core in D7, but it does (partially and imperfectly) in Backdrop. Further, entity_plus_access() did not use Entity:access() since it did not exist in D7 Entity API.

Eventually when entity_access and Entity:access() are fixed we could deprecate entity_plus_access()

[herbdool]

access() method is defined in EntityInterface so doesn't that mean that any class that extends Entity will also need to have that method? Someone would likely know already that the parent method returns TRUE and either add parent::access() or implement their own checking. So I'm not sure if what's there now is such an issue.

[argiepiano]

@herbdool that makes sense. I think the issue stemmed from the original OP porting Registrations from D7 to Backdrop, and not realizing that they should have used entity_plus_access() rather than entity_access(). They trusted that entity_access() would work out of the box without the need to override access() in their entity class.

[argiepiano]

access() method is defined in EntityInterface so doesn't that mean that any class that extends Entity will also need to have that method?

One more thing: this is not quite true. Entity already implements access(), which means that it's perfectly legitimate to extend Entity with a new class without overriding access(). The new class would use the access() method provided by Entity.

[indigoxela]

Someone would likely know already that the parent method returns TRUE

@herbdool that's a dangerous assumption, because if they do not know (yet), they might shoot in their own foot. The official docs do not warn at all.

@argiepiano the long-term goal of the current work on individual access() and createAccess() methods is to make entity_access() a useful and reliable function - it currently isn't, unfortunately. The downside is, that this needs work in several places. Entity::access might be one of those places.