Drupal 8 Node Access Hitlist

It's that time again... With Drupal 7 released, we can start looking at fixes and features for Drupal 8.

I'm hoping to explain each of the items below in the next week or two, and open issues as appropriate. If you already know of issues that apply, please add the link.

This list is in general order or priority, with easiest items going in first, and hard tasks deferred to later.

Improve node access query performance

Type: Bug
Priority: High
Entered by: agentrickard
Owned by:
Issue #: http://drupal.org/node/681760

Using an EXISTS query greatly speeds performance by eliminating temporary tables.

Extensible node access operations

Type: Feature
Priority: High
Entered by: agentrickard
Owned by:
Issue #:

Currently, node_access() passes requests through a whitelist of operations (view, create, update, delete). This is limiting and insufficient. In practice, I have use for 'preview', 'revise', and 'publish' permissions. (See http://api.drupal.org/api/drupal/modules--node--node.module/function/nod...)

Better to make hook_node_access_operations() and have node module implement that for the base operations. Then pass the node_access request through the extensible whitelist.

Discussion:

[muriqui]
do we even need a whitelist? If we're going to allow modules to extend the list with whatever ops they want to define, why have a whitelist at all? If a module passes in an undefined op, do we care, or can we safely just ignore it?

[agentrickard]
I think we do, since it will be very hard for modules to know what actions can be performed otherwise. The original core check is a bit of paranoia that I think is overly restrictive, but having a registry at least lets us rely on known operations. Imagine the chaos that might ensue if any module decided to declare an $op without notifying other modules. That's how I envision the hook working, so that modules know what operations they have to account for. But even that would be tough to support.

Would we be better off expanding the whitelist?

[muriqui]
I get your point, and I agree that the core list is too paranoid, but I'm not sure having a hook and registry prevents the chaos you're envisioning. If module X declares an op, module Y can't simply query the registry and dynamically "know" what to do with it... The maintainer of Y would still need to know about module X and what ops it provides and what they mean, and then add code to integrate with it.

There's also the problem of name collision... if module X and Y both independently add a "revise" op, there's no guarantee that they mean those to be the same thing. A hook might be useful there if it provided a means to resolve collisions, but would it be simpler to just make it a standard for modules to prefix their custom ops with the module name and document new ops in their API file? That would give the developer the info he needs without having to call a hook to announce the existence of every op... Then just provide a conditional for the ops from other modules that your module cares to modify.

[agentrickard]
The prefixing seems a sane idea. It's really a nasty problem, actually. It may be easier to expand core's operations list or remove the whitelist.

Streamline node_access_view_all_nodes()

Type: Bug
Priority: High
Entered by: EclipseGC
Owned by:
Issue #:

node_access_view_all_nodes() currently uses a JOIN where it should us an IN. Hilarity does not ensue.

Remove columns from {node_access}

Type: Task
Priority: High
Entered by: agentrickard
Owned by:
Issue #:

With hook_node_access() providing means to limit access to single node operations (like edit and delete), the {node_access} table does not need the update and delete columns anymore. In Drupal 8, {node_access} should only be used for filtering node list queries. Doing so will lead to a more consistent API implementation.

Remove $op from hook_node_access()

Type: Task
Priority: Med
Entered by: agentrickard
Owned by:
Issue #:

We killed hook_nodeapi() and hook_user() $op functions, but not for hook_node_access(). Kill it. Kill it dead, making hook_node_access_view($node, $account) and so forth. Also remember to handle extensible operations.

Replace {node_access} with general {entity_access}

Type: Task
Priority: Med
Entered by: agentrickard
Owned by:
Issue #:

Allow {node_access} to become {entity_access}. Note that this should happen after dropping the extra columns in {node_access}.

Replace node_access() with general entity_access()

Type: Task
Priority: Med
Entered by: agentrickard
Owned by:
Issue #:

Remove the node-centric aspects and change to an entity-based access system.

Provide default node View access by type

Type: Feature
Priority: Low
Entered by: Itangalo
Owned by:
Issue #:

Provide core node module support for filtering View access by role and node type.

Clean up checks for module_implements('node_grants')

Type: Task
Priority: Low
Entered by: agentrickard
Owned by:
Issue #:

This check (littered throughout core) is not the best way to check that a node access module is active, since often node access modules must be configured. There might be a better way to test for node access module activity. Suggestions?

Description goes here.

Discussion:

[EclipseGC]
Specifically looking at the "clean up checks for module_implements('node_grants')" task, I have to wonder if we might not be better off removing that hook in its entirety. The logic follows that hook_node_access() and hook_query_alter() (I think) can functionally provide everything that's needed for controlling access to a given node (or entity as we move that direction) and lists of nodes/entities. That being said, core's mechanism for calculating access on nodes is horribly broken at the moment per "Streamline node_access_view_all_nodes()" and family, and if that same mechanism insinuates some control over entities as a whole, we will be in for a WORLD of hurt. I've done quite a bit of thinking on this topic and anything short of a pluggable access control mechanism per access instance (which might be totally doable with a serious plugin architecture in core) is really insufficient for any access control scheme outside of what core has set up to begin with. My work with tens of thousands of access definitions, while an EXTREME case, and arguably "broken" from a conceptual stand point (as I've been told a few times) will actually work with just the hook_node_access() + hook_query_alter() implementations ignoring the typical drupal node_access system. This is because I can write a custom query that will return tens of thousands of access calculations where core would build a 2MB query and just die uselessly.

I think hook_node_access() + hook_query_alter() with some sane defaults in core that can be overridden could stand in for core's current access mechanism with a stronger upside, less difficulty explaining how our access control system works, and more flexibility. Perhaps some OOP centric plugin could solve all of this in some perfect future where we have plugins in core.

[agentrickard]

The idea of hook_node_grants() is to write one query_alter instead of a gaggle of them. We could get in worse trouble if every module is query_altering things independently.