Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()

Seems like a good idea to me.

Dave and I talked about this at DCSF, and it seems like a winner to me. The trick is in backend data storage.

+∞

Makes sense - subscribing.

+1. In case it's helpful to have another use-case besides nodes, we're working on file access in File Entity module in #1227706: Add a file entity access API.

Sitting here next to Dave Reid, and he says I can unassign him from this issue, and move it to the entity system.

The other big question, which was being debated in IRC, is that we need to decide how / if we are going to enforce proper access controls on both *lists* and *single* entity requests.

E.g. Currently you cannot possible query for a list of "all nodes a user may edit". And that is a very desirable feature.

A unified entity_access API should probably support that logic. Or we have to *officially declare* that we don't think that feature is important.

Re #8, my position is that listings use node_access tag and not hook system. if you care about listings, you can't just ignore node_access and its grants/records system. If you do, you get inaccurate listings. You might call this answer "we deem it not important", but I think the position is a bit more subtle.

We have this dual system because we can't all agree on hooks versus grants/records. We tried to unify at #21613: Remove hook_access but we eventually decided on the current split brain approach at #143075: Grant equal time to nodeapi for access control responsibilities

Right, and I think the consensus on D7 so far is that this split is a failure, because it introduces inconsistencies and makes what should be easy tasks (list all nodes a user can perform operation X on) impossible.

Are you saying that you support #21613: Remove hook_access?

I think I lean more towards refactoring it in a way such that the rules it defines can be enforced both at the code level (for speed and ease of use) and the query level (for listings and consistency).

To be more clear, I disagree with this statement from merlinofchaos in the other issue:

[T]he node_access table should *never* have been used for 'update' and 'delete' types. You never need those on a multi-node basis, and so they add a LOT of unnecessary weight.

I think we do need lists of edit/delete options, since users expect them. Right now, they are nearly impossible to provide.

Here was a comment from Damien Tournoud on the security list where we talked about this issue:

We currently have a very inconsistent and incomplete access control implementation for nodes, users and comments. To get the proper access control, you have to:

• For nodes: add a 'node_access' tag and a status = 1 condition
• For comments: join to the node table and add a 'node_access' condition there. Core does this very inconsistently itself
• For users: add a status <> 0 and access <> 0 conditions

This is just a mess, and we need to fix it. Here is what I suggest:

• Adopt the convention that an entity type named 'foo' is by default associated to the access tag 'foo_access'; we can support a 'access query tag' key to hook_entity_info() if we want to add flexibility to this
• Add status = 1 in node_query_node_access_alter() and node_query_entity_field_access_alter()
• Implement both alter hooks for comments
• Implement both alter hooks for users
• Automatically add the access tag into EntityFieldQuery, even in base table mode
• (Drupal 8 only) Consider removing hook_query_entity_field_access_alter(), and automatically doing that for the caller

Standardizing on "entity" tag, which will be renamed to "Entity system".

A few one-off instances of this have come in through the security team list in recent months. I think this is more than a "normal" feature request, and probably belongs as a "major task".

Also marking for backport to D7. Probably not the whole thing, but we need to see how we can mitigate these risks today, not 3 years from now.

Marked #1440976: EntityFieldQuery inconsistently uses node access routine as duplicate.

Also from Damien:

For anyone interested in understanding the mess we are currently in, please see the code of Entity Reference to see how complex it is to write something as basic as "give me a list of entities of type XXX" (even if XXX is something as simple as a node, a user or a taxonomy term).

Stumbled on this, and after trying to do something in d7 related to user access (and plenty of other things in d6 with weird access stuff), quite excited for this issue. Some thoughts.

Before someone does a query for nodes, comments to display, they check a given permission (access content, access comments), so should this be accounted for in whatever is done? Two approaches I'd consider: 1) entity_access or such could have $entity as an optional parameter which then just defaults to checking that permission 2) entities could define a base 'access permission' which has a wrapper function that entity_access calls at the beginning to test that, and anyone doing entity queries can thus call this entity_access_permission($entity_type) permission before querying. (and a perhaps a 'bypass access' permissions where no additional hooks are called in entity_view).

'create' op doesn't match how the other operators work, and for a generalized entity_access, it should perhaps be done differently? Cause for creating users: entity_access('user', ??, 'create'). (I've had the desire for additional information passed to 'create' -- e.g. contextual information, like "for this organic group, can this user add blogs. Not sure if how relevant at all. )

Instead of an access callback, or along with access callback, could do an access hook ($entity_type . '_access') in entity_access, i.e. node_access => hook_node_access that entity_access calls. Not sure if that'd be a good idea either. Future developer confusion if hook_user_access, would have nothing to do with user_access.

How about:

For checking before doing a query:

function entity_access_base_permission($entity_type, $account = NULL) {
  if (!$account) {
    $account = $GLOBALS['user'];
  }
  // some static cache checking goes here.
  $info = entity_get_info($entity_type);
  if (!empty($info['base permission']) && !user_access($info['base permission'])) {
    return FALSE;
  }
  return TRUE;
}

for example entity_access_base_permission('node') would check 'access content' permission. It'd be used before executing a query. Could also add a hook in there, not sure if needed. (Can only access content every fifth day?).

For create or similar permissions (can't think of better word than operation). (for example, could have a 'administer' or 'manage' op).

function entity_access_operation($entity_type, $op, $entity_bundle_key = NULL, $account = NULL) {
  if (!$account) {
    $account = $GLOBALS['user'];
  }
  if (!entity_access_base_permission($entity_type, $account)) {
    return FALSE;
  }
  $info = entity_get_info($entity_type);

  // node would have array('create') for this but someone could alter and add their own. 
  if (!in_array($op, $info['access operation ops']) {
    return FALSE;
  }
  // some static cache checking here.

  $access = module_invoke_all('entity_' . $entity_type . '_access_operation', $op, $entity_bundle_key, $account);
  return !in_array(FALSE, $access);
}

alt, entity_access_create and take away $op. e.g

function entity_access_create($entity_type, $entity_bundle_key = NULL, $account = NULL) {
  if (!$account) {
    $account = $GLOBALS['user'];
  }
  if (!entity_access_base_permission($entity_type, $account)) {
    return FALSE;
  }
  $access = module_invoke_all('entity_' . $entity_type . '_access_create', $entity_bundle_key, $account);
  return !in_array(FALSE, $access);
}

so node_access would have create stripped out and instead implement that hook (not sure of hook naming here).

And last one

function entity_access_entity($entity_type, $op, $entity, $account = NULL) {
  if (!$account) {
    $account = $GLOBALS['user'];
  }
  if (!entity_access_base_permission($entity_type, $account)) {
    return FALSE;
  }
  // It occurs to me that it'd be nice to have a single function to
  // call to get a key for an entity.
  $info = entity_get_info($entity_type);

  // node would have array('create') for this but someone could alter and add their own. 
  if (!in_array($op, $info['access entity ops']) {
    return FALSE;
  }

  // some static cache checking here.
  $access = module_invoke_all('entity_' . $entity_type . '_access_entity', $op, $entity, $account);
  return !in_array(FALSE, $access);
}

Bad idea?

Tried combining to one function but was ugly.

I haven't yet read through this issue but due to the current node access api (and due to what happens in Field Storage, this ends up being a bug whereby implementing EntityFieldQuery on non-node entities with Field Conditions will add a field access tag to join with the node_access module grants and stop results from showing up (see #997394: field_sql_query hardcodes a 'node_access' tag on EFQs with a fieldCondition where I just added a test that shows this to the be the case).

Regarding webchick's hit list, the only problem on is Add status = 1. In D7 we deliberately made it possible to retrieve unpublished nodes and still honor node access. This is used by modules like Workbench.

Workbench does this in hook_node_access(), not hook_node_access_records(). Does that matter?

Here's a starter patch that access two keys two hook_entity_info

• access permission: check at the begining to see if user can ever access this type of entity (e.g. access content)
• bypass access permission: check if user can always access all of the entity

And updates relavent hook_entity_info; some do not have corresponding permissions, (not sure if they should, but seems like that'd be a separate topic).

It seems like these are useful starting point; new issue queue for it? I've known I've seen one issue where having this in place (without anything else) would be useful, but can't remember the details.

Also added two functions to check if a user has either of these permission. It occurs to me that since hook_entity_info_alter, someone could change one of the permissions. not sure if that's a good thing or not, but complicates thing (should node_module replace any reference to 'access content' to checking what's defined? ouch!).

How would that interact with the 'access callback' that EntityApi uses? Would it override it to either blanket deny or grant?

The two permissions seem too limited to be useful here. In the case of a node, the 'access permission' actually has 4 separate states (create, view, update, delete) whereas 'access content" only covers one of those states.

The interaction with an entity callback or an entity hook is when checking access for a single entity

• Got bypass access? return true
• Don't got any access? return false
• return result of a hook or a callback to test for more complex access

Unless something has changed in d8, 'access content' is the most basic (without it, can't do create,update, delete, etc..). If user doesn't have it, can immediately exit (or passthrough in case of bypass) instead of checking more complex/rewritting/etc.

I really wish I remembered what issue I saw where these would be useful to have.

That makes sense to me.

- access permission ( == 'access content'): necessary but not sufficient to do anything with this type of entity
- bypass access permission (== 'administer content'): admin access to do anything to this type of entity

OK, I see now. Thanks.

Note, I've added #1696660: Add an entity access API for single entity access for adding something like an entity_access() function of the d7 contrib entity API module - it's not supposed to cover query tags though, so I think it makes sense to concentrate on that as part of this issue.

Slightly adjusting the issue title to clarify why this is a major task.

Question, detailed at #1704168: Move node access functions to separate module. Should we make these sorts of advanced access controls entirely optional.

I think, the current node access layer is difficult. Too difficult. And this only node entity, cannot be used other core (user, comment, taxonomy) and custom entities.
Would it be possible to simplify it on him?

#1810320: Remove EntityTranslationControllerInterface::getAccess() once have entity access is postponed on this issue. (updated issue summary)

What would there be need for in order for the solution of the issue to go on?

The entity access controller for single entity access got in, I believe we also have separate issues about generic entity query access. I had a similar idea as @hefox to add something like basic configuration for the default entity access controller to make it as simple as possible to implement it for a simple entity which just needs a single permission but that, as well, sounds like a separate issue to me.

Given that, what exactly do we need this (major) task for?

This is D9 at this point.

EFQ does and always did this:

$query->addTag($entity_type . '_access');

That gives every entity type the option to implement whatever security check it wants. nodes have a complex grant system. Some other entity type could implement the same for itself. I don't think we need to standardize that and enforce for every entity type.

Doing some sort of entity-agnostic check would be a bit more complicated, but do-able if you really want to (implement hook_query_alter() and check the tags). But I don't really see a use case for that?

For single entities we have the AccessibleInterface, access controller, generic hook_$entity_type_access() that works like hook_node_access().

Do we really anything else here?

Edit: The comment as field issue has a use case, but trying to think about a solution melts my brain: It has a comment table, with entity_type and entity_id reference. It lists those comments and it should only lists comments where the referenced entity is visible.

Do we really anything else here?

I think we do. We never tried to fix #18. At the very least, we should have a queryAccess() method on EntityAccessControllerInterface, and let every entity type implement it based on their schema/access requirements.

Updating tags.

Updated issue summary. added info that #1810320: Remove EntityTranslationControllerInterface::getAccess() once have entity access is postponed on this issue.

Not sure there's anything left to do here. If it's #40 that looks like an addition we could add in 8.1.x.

Drupal 8.1? :)

Is this something worth working on or will it get shot down because we aren't sure of what we want yet?

I was thinking of something along these lines:

• Every (content?) entity type gets the same grant system
• It doesn't do anything unless a module actively sets grants for that entity type
• Instead of counting on entities extending the base class, the grant system is checked in an alter hook so it becomes hard to "forget" implementing it
• Node grants get replaced by this system
• Grants and access records are gathered using both the dedicated and the general function: hook_node_access_records($node) and hook_entity_access_records($entity) for example
• Rebuilds can be triggered for just one entity type or all at once

Do config entities need grants as well?

+100 for #46, the only precondition is that it must not break BC. Which AFAICT should be totally doable.

If you have the capacity to push this forward, I'd be happy to provide reviews.

This seems like a good challenge :)

The Group module could massively benefit from this, so I should try and find the time. I'm out skiing soon, but I definitely want to push this forward. I may need help with writing the tests, as I'm really into that part of D8 yet. The whole prophecy thing is new to me.

Still working on this, just got back from vacation and in e-mail hell right now :)

Kristiaan, got any patches of work in progress perhaps? We need this too.

Uhm, I'll try to upload what I've got later today or over the course of this week.

Right, here we go.

A few notes:

1. This is a work-in-progress patch, so don't shoot me if I missed something :D
2. It covers the basics already, i.e.: creating an entity grant system and using it for node. It does not cover the other items we want from #46 yet. We still need to:
   
   1. Allow entity types to enable grants in their annotation
   2. Implement grants in an access alter hook (not in EntityAccessControlHandler)
   3. Figure out the rebuild process (one vs all)
   4. Document and double-check the API changes: hook_entity_grants() vs hook_ENTITY_TYPEI_ID_grants(), make sure we call it correctly, ...
3. At first I tried to make NodeGrantDatabaseStorageInterface and NodeGrantDatabaseStorage extend the new generic entity versions, but because of type-hinting on NodeInterface that was not feasible. So I decided to remove that code altogether and make Node use the new entity grant storage. This only breaks BC if someone actually decided to extend (or implement) the node storage (interface), which is rather unlikely.
4. I tagged a test with @todo entity_access: Move to entity tests. because it was node-specific and thus in the wrong place for adding generic entity tests. Now that Pieter the test wizard is on the case as well, that may get fixed quite easily :)
5. I actually tested this WIP patch about a month ago and it worked fine on a fresh 8.2.x install.

Let's see how test bot likes it so far.

I forgot to mention:

1. As you can see, we already have a ::hasGrantImplementations(), but not ::getGrants() yet. Hence the @todo because I still called node_access_grants() directly here.
2. We need a user.entity_grants cache context

At first I tried to make NodeGrantDatabaseStorageInterface and NodeGrantDatabaseStorage extend the new generic entity versions, but because of type-hinting on NodeInterface that was not feasible. So I decided to remove that code altogether and make Node use the new entity grant storage. This only breaks BC if someone actually decided to extend (or implement) the node storage (interface), which is rather unlikely.

Well, that would be a BC break. You write extend, ... why do you try to extend and not just wrap the new api internally?

Allow entity types to enable grants in their annotation

Do we need to do that. Couldn't we just detect things based whether there are hook implementations and call it a day?

1. +++ b/core/core.services.yml
   @@ -566,6 +566,11 @@ services:
   +  entity.grant_storage:
   +      class: Drupal\Core\Entity\EntityGrantDatabaseStorage
   +      arguments: ['@database', '@module_handler', '@entity_type.manager', '@language_manager']
   +      tags:
   +        - { name: backend_overridable }
   

   In general I'm wondering whether that could be entity handlers instead of one service to rule them all. This would make it easier to swap things out for nodes for example.

2. +++ b/core/lib/Drupal/Core/Entity/EntityGrantDatabaseStorage.php
   @@ -0,0 +1,360 @@
   +  protected static function buildGrantsQueryCondition(array $entity_access_grants) {
   

   Why is that a state method?

3. +++ b/core/lib/Drupal/Core/Entity/EntityGrantDatabaseStorageInterface.php
   @@ -0,0 +1,153 @@
   + */
   +interface EntityGrantDatabaseStorageInterface {
   ...
   +   * @return int.
   ...
   +   */
   +  public function checkAll($entity_type_id, AccountInterface $account);
   +
   

   I'm wondering whether the entity type ID has to be part of the interface, or could be rather passed along somehow. I mean for example the Entity storage controllers also don't know they entity type IDs as part of their public APIs

Well, that would be a BC break. You write extend, ... why do you try to extend and not just wrap the new api internally?

Good call, I started by extending because it was a lot of code to convert and I wanted to keep it working in the mean time so I could test it. I should have thrown out the extending bit as soon as the entity storage was more fleshed out.

Allow entity types to enable grants in their annotation

Do we need to do that. Couldn't we just detect things based whether there are hook implementations and call it a day?

Crap, that's my sleepy eyes at work. I actually wrote your suggestion in #46already.

In general I'm wondering whether that could be entity handlers instead of one service to rule them all. This would make it easier to swap things out for nodes for example.

This is what I wanted to ask Wim about: Having a table for each content entity type would mean we could easily truncate that table to clear the grants. It would also result in faster queries. The downside is we'd then create tables that would perhaps go unused (i.e.: one per content entity type).

Why is that a state method?

It was like that in node.module :) Doesn't seem to make sense, though.

I'm wondering whether the entity type ID has to be part of the interface, or could be rather passed along somehow. I mean for example the Entity storage controllers also don't know they entity type IDs as part of their public APIs

Another good point, something worth looking into. How would we instantiate that, though? As we are dealing with a service here (ideally a singleton service even?)

I'd leave node grants alone for the first step. We can still consider to do that later, but this will be complicated enough on its own and I'm not 100% sure how much of that system needs to be considered for BC (interfaces is one part, but what about the storage, would we need update functions, queries could change, sites might have overridden services that will no longer work, ...).

Seems like a lot of questions that we better avoid for step 1.

dawehner had a good suggestion on IRC: move entity grants to an entity handler instead of a general service. That way it would know about its entity type without us having to ask for an entity type ID in the grant class' methods.

It may also help resolve the node problem: the old service would simply no longer be called and thus can be marked as deprecated? Although I do like the sound of #57 :)

Edit: dawehner also supported the idea of separate tables, so let's try to get that in too.

Great progress here, thanks @kristiaanvandeneynde!

What would there be need for in order for the solution of the issue to go on?

Time :)

We've already established most of what should go into this patch. See #46 for a start and then add in the following:

• Dedicated tables, e.g.: node_grants, user_grants, ...
• Grant controllers should be entity handlers (like storage, views_data, ...)
• We leave the old node code alone, it just won't do anything once this work goes in

Re-titling, keep getting confused when I see it.

„7 weeks from today Drupal 8.2.0 will go into beta”
https://twitter.com/gaborhojtsy/status/743117224358125568

Added issue tag "Contributed project blocker".

Contrib that relies on node_grant nowadays can leverage big when this issue lands in core.
Examples: acl, content access

Not sure, probably this is not a 'blocker', more like 'enabler'..

Changed title, removed "Security improvements"

Covered with issue-tag "Security improvements"

Yeah, I've been busy getting a beta1 out the door for the Group module. Now that I've achieved that, I may be able to use some of my "contrib hours" for this issue again.

And now?

I managed to get the basics rewritten in Dublin as per #61.

To be fair, I'm also working on the Group module for D8 and can only squeeze in so much time for this issue. I managed to get a good start in Dublin and am hoping to complete the patch by mid November.

On a personal note: Comments like "And now?" are counter-productive. Please consider the fact that I am doing my very best to fit a huge patch like this into my schedule. If my timing doesn't suit you, feel free to write the patch yourself.

@kristiaanvandeneynde I totally understand your feeling. But it could also be you are misinterpreting #68: perhaps he's just genuinely wondering what the current status is. It'd be okay to say "no progress, nothing new"! I personally was also interested to hear the current status. #69 answers that.

Provide the occasional status update, for example every few weeks. It's okay to let us know nothing happened. :) Then you won't get comments like that anymore.

Also, the progress in Dublin you describe in #69 sounds super exciting! :)

Yeah I may have overreacted a bit there. I'm just annoyed myself that I'm having such a hard time finding an open spot in my schedule to properly finish this. So such comments hit me where it hurts :)

On a positive note: The whole system is turning out to be a lot more clean when done as entity handlers instead of separately declared services.

The only concern I have right now is that the access record handler is doing both retrieval and storage whereas the old system retrieves the access records in NodeAccessControlHandlerInterface and stores them using NodeGrantDatabaseStorageInterface.

I'm not sure whether we want to do the access record retrieval in the access control handler, so I've moved it to the "access record handler" (renamed it from grant storage). The issue I have with that is that it now does two things instead of one: retrieval and storage. But perhaps that's okay seeing as plugin managers seem to do the same thing.

Anyways, that's a discussion we could have when I finish the patch as it would only require code to be moved around.

I recognize the feeling of being angry with yourself for lack of time+progress :) I think almost all of us do.

The positive note sounds very positive indeed!

Should have a full day to work on this tomorrow. Stay tuned.

Right, so here's the first proof of concept. It does the following:

• Adds an optional 'access_records' handler
• Provides an SQL implementation of the handler, allowing room for other storages to implement the system
• Assumes several ENTITY_TYPE_access_records tables where the column 'nid' has been renamed to 'id'
• Cleans up a lot of confusing use of the term 'grants' where 'access records' should be used
• Runs 2 hooks to gather grants (generic and entity type specific) and 2 hooks to alter them (ditto) and does the same for access records

Here's what it doesn't do yet:

• Actually generate all of the tables
• Cache access checks (see @todo re cache contexts)
• Implement any of this
• Test any of this
• Document the hooks

Here's some caveats / remarks:

• If we were to use this for node, it would have to override the generic class to account for isPublished()
• It seems there is a bug in NodeGrantDatabaseStorage::checkAll() where the query is needlessly joined to the user's grants. See the @todo in checkViewAllAccessRecord().

Some questions:

• Does anyone actually use grant_update and grant_delete? If not, we could simplify the system A LOT by just getting rid of the operation altogether (by always assuming 'view'). From what I've gathered, Views only adds the grant_view check and I have yet to see anyone make proper use of the two other options.
• What about the naming? We used to roll with grants/access records but now we have methods for grants in a class name AccessRecordHandler or vice versa. Perhaps rename the whole thing to make the system less complex to learn?

Re grant_delete and grant_update. I've done some digging and it turns out the whole system is a bit weird for those grants.

The function node_node_access() has got very little to say. Unless it returns an AccessResult::forbidden() or the node is unpublished and the author can view their own unpublished nodes, NodeGrantDatabaseStorage::access() always runs.

This is because of the following lines in EntityAccessControlHandler::access():

    // Also execute the default access check except when the access result is
    // already forbidden, as in that case, it can not be anything else.
    if (!$return->isForbidden()) {
      $return = $return->orIf($this->checkAccess($entity, $operation, $account));
    }

With checkAccess() checking the grant system.

Thing is, node_node_access() never returns a forbidden; only neutral or allowed. And as soon as any module implements hook_node_grants(), NodeGrantDatabaseStorage will always run a query which either returns neutral or allowed. This leads to:

1. Performance hit
If you have a node access module enabled which only takes care of view access, all update/delete access checks will also needlessly run a query even if they have no records whatsoever. This is the case on most sites.

2. Inconsistency
Even if you define grant_update or grant_delete in your module, this will only work if the user does not have any of the permissions "edit any", "edit own", etc. Because if they do, they will have "allowed" orIf("neutral") = "allowed" (see snippet above).

This is weird, because view access works differently: Even if you have the 'view published nodes' permission, you are not allowed to view a node unless you have a grant matching one of its access records.

Ha!

In March 2012 in comment#17 @webchick said...

Also marking for backport to D7. Probably not the whole thing, but we need to see how we can mitigate these risks today, not 3 years from now.

and 4.5 years later, this bug just bit me on the bum.

Good to see it's being worked on at the moment! =)

And 3 years ago at #38 @xjm pushed this out to D9! :(

@kristiaanvandeneynde - what help do you need here? Looks like you're making great progress! =)

All I need at this point is reviews, opinions and perhaps people agreeing we should drop the grant_* columns.

I'm really glad that this feature is going to Drupal 8, very useful for module developers.
@kristiaanvandeneynde very nice work :)

Referring to your comment #76, that's indeed makes the grant system more complex and less performant.
1. Performance hit: I read the code in EntityAccessControlHandler::access(), what i see is that checkAccess is always called unless some module returns forbidden which is IMHO correct. So if any one in the whole process says access is forbidden, that should be the end of access check otherwise it should continue till the end. I think what's needed is to find a way to figure out whether an entity has grant records or not and only do the access check when it does.
I used Drupal grant system quite some time ago so correct me if i'm wrong. One way to find out about that can be by checking for example if hook_entity_access_records has been implemented for that particular entity or even bundle. Or a new hook can be introduced hook_entity_access_records_info, so modules can indicate that. it can be done in such a way that does not cause any backward incompatibility. A flag or cache variable can also be used to mark entity or entity bundle when their first grant is saved in database or the first related access record is retrieved from the database, but personally i prefer the new hook, since it can much more accurate and can be used for other extra info as well in future.
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!EntityAcce...

2. Inconsistency: I guess that the behavior you explained might be to preserve backward compatibility. I'm not sure how it's related to removing grant_* columns since they're useful for per records access control when edit any or edit own are not available.
As i mentioned before to be on the safe side, in case of allow database grants should also be checked just like for view. So in case there is no grands for update and delete it can be skipped using similar solutions mentioned in my Performance hit "answer" to prevent any performance hit. I'm not 100% sure if this approach is backward compatible with modules that may have implemented grants for node but i'm sure it's possible to workaround the backward comparability barrier.

Hope it helps. keep up the good work

4 weeks: https://twitter.com/gaborhojtsy/status/816668184585584640

At this stage, I doubt we'll hit that deadline. I would love to see it go into 8.3 but there's still much ground to cover and I could use some more feedback on #75 and #76. Thanks for your feedback in #80, sinasalek!

Edit: FWIW I had no chance to work on this over the holidays. I was prescribed a lot of rest after being hospitalized mid-December.

Object Oriented approach to access grants: https://www.drupal.org/project/node_access_grants

Hi, Kristiaan!
First of all, thanks for hard work and taking initiative to bring this important functionality for Drupal core.
I've investigated patch, that you provided in #75. Maybe you already know it, but what I have noticed:

1. The patch creates the new table entity_access and all records with a grants will be recorded to this table. As result, the table node_access will not be used.
Probably we will have problems with modules that are using the table node_access in queries to a database.
We should check all moments where it might be a problem and fix.
2. Also the hooks like hook_node_access_records were replaced to hook_entity_access_records and hook_ENTITY_TYPE_access_records
and the hooks like hook_node_grants were replaced to hook_entity_grants and hook_ENTITY_TYPE_grants.
These hooks can be used with same names for the node module, but their arguments are different and I think it might be a problem.
3. In the EntityGrantDatabaseStorage.php file you try to record grants into the table node_access to the field entity_id. But this field doesn't exists in this table. As result you will receive error during creation of a node.

I would like to propose:
Add extra directive to the annotations of entity classes which will determine whether need to create table with grant records. Table can be named like ENTITY_TYPE_access. These tables can be created together with main table of entity and removed after uninstallation of a module. Remove the entity_id column and create identical column with name given in entity keys (because for example, node_access uses column nid).

I've given this some thought and I came to the conclusion that we actually have the opportunity to wipe the slate clean here. Instead of using the old system with hooks (and potential naming collisions), we could use subscribers (like route subscribers).

Furthermore, we are using a very confusing naming scheme right now with "access records" and "grants". Proof is that core says "grants" in a lot of places where it means to say "access records".

How about we introduce a new system? It could live side by side while we phase out node grants and we would no longer have naming collisions.

API name: Entity query access layer (or something similar)
Terminology: Lock, LockCollection, Key and Keychain

Theory: Any entity can get any number of locks, but once you have the key to open one, you gain access. This is 1:1 the same like how grants and ARs work now. When we gather someone's keys, we do so by calling methods on a Keychain object that is passed to the "gather someone's keys" subscribers.

Cool new functionality: The Keychain implements CacheableDependencyInterface and can be cached for a user. With the use of cache tags, we can invalidate someone's key chain.

The API would look something like this:

class NodeAccessLockSubscriber extends EntityAccessLockSubscriber {

  /**
   * {@inheritdoc}
   */
  public function applies(ContentEntityInterface $entity) {
    return $entity->getEntityTypeId() == 'node';
  }

  /**
   * {@inheritdoc}
   */
  public function setLocks(ContentEntityInterface $entity, EntityLockCollection $collection) {
    $collection->addSimpleLock('node_access_bypass');
    if ($entity->isPublished()) {
      $collection->addSimpleLock('node_published');
    }
    $collection->addVariableLock('node', $entity->bundle());
    $collection->addVariableLock('node_author', $entity->bundle(), $entity->getOwnerId());
  }

}

Where every lock would have to be defined in YAML detailing a label, description and how many parameters it uses. A simple lock is a yes/no lock and a variable lock requires you to set the defined parameters. Trying to add a lock without a YAML definition would throw an exception.

Keys would be handed out as follows:

class NodeAccessKeyProvider extends EntityAccessKeyProvider {

  /**
   * {@inheritdoc}
   */
  public function getKeys(EntityKeychain $keychain, AccountInterface $account, $op) {
    if ($account->hasPermission('bypass node access')) {
      $keychain->addSimpleKey('node_access_bypass');
    }

    switch ($op) {
      case 'view':
        if ($account->hasPermission('view published nodes')) {
          $keychain->addSimpleKey('node_published');
        }
        break;

      case 'update':
      case 'delete':
        foreach (NodeType::loadMultiple() as $type_id => $node_type) {
          if ($account->hasPermission("$op any $type_id node")) {
            $keychain->addVariableKey('node', $type_id);
          }
          elseif ($account->hasPermission("$op own $type_id node")) {
            $keychain->addVariableKey('node_author', $type_id, $account->id());
          }
        }
        break;
    }

    // All of our keys are permission based, so we can add the user.permission
    // cache context to the keychain.
    $keychain->addCacheContext('user.permission');
  }

}

If we were to give each variable lock type its own dedicated table, we could control the column names to make them more readable. This way, there would be no parameter limit as each variable lock type would have its own table. Simple locks could all be thrown into one single table as they are on/off.

The queries would be built by processing someone's (cacheable!) keychain in a dedicated class such as SqlEntityKeychainProcessor. This in turn would lead to a separation of concerns where there is no longer one class responsible for everything (like there is now).

Queries could be simplified too, we would no longer default to IN-conditions.

• Simple locks would become aggregated: "JOIN entity_lock b ON a.entity_id = b.entity_id WHERE b.entity_type = 'node' AND lock IN ('simple_key_a', 'simple_key_b')"
• Variable locks would join dedicated tables:
  • "JOIN entity_lock_node b ON a.entity_id = b.entity_id WHERE b.entity_type = 'node' AND b.type IN ('article')"
  • "JOIN entity_lock_node_author b ON a.entity_id = b.entity_id WHERE b.entity_type = 'node' AND b.type IN ('article') AND b.uid IN (1)"

About the performance of one table per lock type. We would not need to join every lock type table because we know what keys the user has. So we would only join those tables that we actually care about and potentially gain performance.

We should also implement some sort of applies() method on the KeyProvider which only provides the keys for the entity types it supports. We would then cache someone's keychain per entity type. That way we would only get Term keys in our keychain when querying the terms table.

How about we introduce a new system? It could live side by side while we phase out node grants and we would no longer have naming collisions.

As long as you make sure that we maintain BC: yes, that'd be splendid! Maintaining BC includes ensuring that all of today's hooks still work.

Cool new functionality: The Keychain implements CacheableDependencyInterface and can be cached for a user. With the use of cache tags, we can invalidate someone's key chain.

This sounds very interesting! The keychain is then effectively a value object. But how do you determine when the keychain changes, and what it varies by? What if some route subscriber depends on some configuration or varies by some cache context? That means the keychain object would have to implement \Drupal\Core\Cache\RefinableCacheableDependencyInterface…

This sounds very interesting for sure, but you'll want feedback from somebody who knows the current system inside-out, which I don't.

We wouldn't touch the old system. Both would alter the query in their own way. If someone still uses node_access, that would work fine. We should advise to move to the new API and deprecate the old one, but they should be able to live side by side happily ever after.

Let's not invent our own terminology for access handling without looking at how others are calling it.
In this case "lock" definitely feels wrong, because a lock usually means "a protection against concurrent writes", and is something some entity types actually have (in Commerce, for example).

Re the cacheability of the keychain, I'd have to look into the refinability.

The most important idea is that we must be able to invalidate someone's keychain easily. But if several users with the same permissions get the exact same keychain for node entities, I see no reason why we shouldn't cache it and give them all the same keychain. Shaves off quite a bit of processing, I'd imagine.

Re #90: Completely agree. The only reason I proposed new names is because core currently does not respect its own terminology and we would be best to use a new one to clearly show that these are two different APIs. I'm open to any terminology.

If we're looking at a new system, we should also consider a listing API that doesn't require additional/complex joins at least as an option.

Don't think there's a core issue for this, or if there is it's very old, but David Strauss did a blog post in 2009 that covers some of the ideas: https://www.fourkitchens.com/blog/article/anticipage-scalable-pagination... (see the iterative, optimistic permissions strategy section). tl;dr is you get results from the database without access queries, a few more than you need, then check access on each entity individually. Works best on sites where most users have access to most content.

Side-by-side, as in independent? Or integrated? That's what matters. They need to be integrated, i.e. it must work when you choose to use the old API and when you choose to use the new API. Only that way are modules able to gradually migrate to the new system!

In this case "lock" definitely feels wrong, because a lock usually means "a protection against concurrent writes", and is something some entity types actually have (in Commerce, for example).

Agreed, that was my initial reaction too. But I chose not to comment on the names, just on the concepts :)

Re #93: We can implement it anyway we like because we now have a proper abstraction layer. But yes, performance would be a very important thing to keep an eye on.

Re #94: I meant integrated. Node grants currently alter queries. The new system would too. It shouldn't be that hard to make sure the query conditions added by both systems play together nicely.

@kristiaanvandeneynde we could have implemented it any time since 2008-9 when we first discussed it with the node access system, but never got to it - the closest we've got was making mini-pagers the default with core views.

To me an ideal system resembles cache tags, you have a set of services that output access tags for storage, and later add them to the access query. A subquery then limits a query to the results returned by the access query: WHERE entity_id IN ($access_query here).

However, our minimum MySQL requirement doesn't allow us to use subqueries anywhere without destroying performance (subqueries got performant from 5.6)

Glad to see this issue is active again. This feature seems to be a very logical and vital for D8. We have experienced the limitation with the current system in Open Social and initial I could not believe there was no entity access grant system in core.

We have to acknowledge the ongoing struggle of getting this feature finished. And I am afraid that without a proper process this feature has the potential to never be completed. Not to diminish the great contributions people made (e.g. from Kristiaan) but the size and complexity of this change seems to be too great for one person to handle.

I would like to propose the following process:

1. Agree upon a proposal for the new system in this issue (e.g. make a Google drive document that also explains why this system is important)
2. Get approval on the proposal for Drupal Core (maybe even include Dries in this).
3. Cooperate on implementing the solution.
1. Use Github or a separate Drupal project for this effort and move out of this issue.
2. Create stories and estimations and determine the priority of them
3. Work on the stories together.
4. When a MVP is finished add it as experimental module in Drupal core and try to get it to the stable state as soon as possible.

Our Open Social team is willing to invest time to help with the implementation of this feature (at least 8 hours every three weeks). I am sure more companies beside Deeson are willing to help (e.g. Commerce guys, Acquia).

I was literally just on a call with my tech chapter lead discussing this issue and how we should split it up into smaller tasks once we agree upon approach and terminology. :)

Great Kristiaan :) Do you agree on making a proposal document in Google Drive?

For the terminology I agree that a comparison of other systems would be beneficial. I am not sure about replacing the 'Grant' name with 'Key' because I think most Drupal 7 developers already know what a node grant is. To avoid the conflict of the Lock name we could alter it to a 'GrantLock' or 'KeyLock'.

Yes but feel free to get started on outlining what I wrote in #86. I'm a few days removed from a week-long vacation so I don't know if I can spend much time on it right now. I should be able to follow this more closely in about two weeks' time.

Here is a quick draft of the proposal document. Everyone should be able to post suggestions and leave comments.
https://docs.google.com/document/d/1jIWKVoYbdVeEg-Kz_5yvlMvi7PycoHZWg4xX...

@kristiaanvandeneynde
@nielsvandermolen

Thanks for all the good work.

I tried to review 3 things on a functional level:
- Naming conventions
- Applications for this access system
- Google Docs specification as-is

75.q1
* Does anyone actually use grant_update and grant_delete? If not, we could simplify the system A LOT by just getting rid of the operation altogether (by always assuming 'view'). From what I've gathered, Views only adds the grant_view check and I have yet to see anyone make proper use of the two other options.

Ndf: Yes, I can imagine examples where grant_update and grant_delete will be used.
One example would be a news-website with 2 editorial teams: one for sport-news and the other for politics. Each team has several members and can create/read/update/delete any news-items in their team, but not in the other team.
The general concept here are lists of users that have shared access permissions (Access Control Lists).

75.q2
* What about the naming? We used to roll with grants/access records but now we have methods for grants in a class name AccessRecordHandler or vice versa. Perhaps rename the whole thing to make the system less complex to learn?
102

Ndf:

I have been reading on Wikipedia looking for software access control definitions.

One of them is "Role-based access control". This one is currently in core and available for entities. (https://en.wikipedia.org/wiki/Role-based_access_control)

The one that looks most similar to Drupal's "node_grant" system is "Attribute-Based Access Control (ABAC)" (wiki: https://en.wikipedia.org/wiki/Attribute-Based_Access_Control)

Then there is this specification XACML (https://en.wikipedia.org/wiki/XACML, http://docs.oasis-open.org/xacml/3.0/xacml-3.0-core-spec-os-en.html) that is a declarative language implementing ABAC.

What if we can select a widespread and popular specification and use it's namings in our implementation?

In XACML there are 3 levels of elements:
1. Rule
2. Policy
3. PolicySet

This is very close to the current patch! These namings are synonyms:
1. Rule == Lock == AccessRecord
2. Policy == LockCollection
3. PolicySet <> not implemented

Example of a Rule:
target: Entity 1
effect: View-access
condition: User has permission 'node_access_bypass'

Example of another Rule
target: Entity 1
effect: View-access
condition: User is author of target entity

Remarks:
- In the above examples the "Condition" part is called "Lock" in the current patch. The first example is a so called "SimpleLock" and the second one a "VariableLock".

When combining the above 2 rules, you get a "Policy" (in the current patch a "LockCollection".
A PolicySet is a super-set of Policies.

In the 2 example rules above I used these rule-attributes:
target, effect, condition

This is taken from http://docs.oasis-open.org/xacml/3.0/xacml-3.0-core-spec-os-en.html 3.3.1.1 Rule target
The Oasis spec uses 2 extra attributes for Rules:
obligation and advice

This wikipedia article https://en.wikipedia.org/wiki/Attribute-Based_Access_Control) uses different attributes for Rules namely:
resource (==target), action (==effect), subject + contextual

The declaration of Rule-attributes is different between the specification and that part makes me confused. So I'll try to read more about it.

My questions so far:
1. Is "Attribute-Based Access Control (ABAC)" equivalent to Drupal's node_grant system?
2. What is your opinion about following an existing specification that implements ABAC?

BTW I am in Seville this week for Drupal Developer Days. Let me know if your there, we can talk in personal

As a node access module maintainer, (and D7 system co-maintainer), I do use grant_update and grant_delete.

Those have limited value, however, since they can be overridden on a per-node basis in hook_node_access(), which makes it very difficult to create a list of "things I can edit" or "things I delete."

Conceptually, I like the idea that the grant column could be flexible, but I suspect that would lead to performance issues because of the need to and a WHERE on a varchar. The current system uses boolean INTs to optimize query performance.

Thanks @ndf, your comments added a real value to this issue I think.

I read the mentioned Wikipedia pages and checked the XACML'S specs. Even though I've been developing custom access control system in Drupal for a while I learned a lot.

In my opinion, the next generation access control system in Drupal is should follow the ABAC access-control paradigm indeed. Thanks for the Contextual attributes it would be much easier to implement organic group like-ish access control logics, where group memberships would be just additional contextual attributes.

Also, D8's future access-control system should not limit the $op (operation) to the built-in (and inherited from D7) view, edit, delete operations because a custom (content) entity could have multiple additional operations, just as Marek Kisiel mentioned in the Gdocs.

I'd say XACML may provide a lot more what we would actually need. It would be also an interesting question how many users have this access-control paradigm? I checked packagist for existing PHP implementation, unfortunately it was not too promising:
- The official lib is just a reserved namespace.
- enygma/xacmlphp seems the most used library, but I do not see how this could be integrated to Drupal.

What access-control paradigm implemented by other matured, enterprise level CMS systems?

Additional resource:
http://docs.oasis-open.org/xacml/3.0/xacml-3.0-core-spec-os-en.html

First, let me say, thank you @kristiaanvandeneynde! This is a great start!

I'm am incredibly excited to see what is coming along in here. Working in an agency, we deal with a large number of clients across a wide spectrum of applications. It seems that on nearly every project, we inevitable end up writing custom access rules. Many of them require something like the node grants system, but with Drupal 8 we're increasingly running into the issue with other entity types, User entities being the primary example.

I've been thinking of implementing an access policy on my own to solve these issues, but the work you've done suggests that we could actually get this done in Core much more easily.

For me, as @mxr576 suggested, Attribute Based Access Control (ABAC) is absolutely the way to go. Almost every bit of access code I write deals with either a field of the entity in question, or a field of an entity across an entity reference (E.G. The author of this node belongs to group Foo).

The beauty of ABAC in Drupal 8 is that we have well defined schema for any entity type and a generalized means for reading field values on them. This means we can easily define an elegant ABAC system, but also provide a generalized config page for all entities, i.e., right alongside "Manage display" we could provide a "Manage access" tab.

I think we should move this discussion into the Drupal Core ideas thread and start to build momentum on this front.

Right, I've looked into this ABAC thing and it looks really promising and yet similar to what we already have. I would go with the SARC-approach:

1. Subject: Who is asking for access (users)
2. Action: What action are they trying to perform (view, update, delete, buy, join, leave, ...)
3. Resource: What is being accessed (node, product, group, ...)
4. Context: Are there any outside influences (time, location, site config, ...)

Currently we already have some of it in place, albeit in a limited fashion:

1. Subject: Fully covered on every request because hook_node_grants() is live.
2. Action: Hard-coded in the DB as view/update/delete, this would need a rewrite to allow for other actions such as "buy".
3. Resource: Taken care of by hook_node_access_records(), but stored in the DB along with Action. This would need a rewrite to decouple the two.
4. Context: Semi-implemented because hook_node_grants() is live and we can leverage contextual data there.

Do we write our own incarnation of ABAC or try to implement an existing one?
Given how specific Drupal is when it comes to defining resources, it may be worth aiming for a minimalistic approach and avoiding third-party libraries altogether. Example lib in PHP: https://github.com/Kilix/php-abac

How would we query/leverage the new system?
The whole reason we currently have grants is so we can check access while retrieving records from the database. If we move to an ABAC layer, we may not be able to query entities that easily. If we have to first retrieve entities before we can check access, it beats the purpose of the system. Unless we do the scalable pagination as suggested by catch in #93.

We need to be careful that we do not undermine our current RBAC system
ABAC is typically regarded as a replacement for RBAC. If we implement ABAC for entity access while using RBAC for all other site permissions, we need to be careful that the lines do not blur. Our Subject properties (ABAC) would likely come from the RBAC system. While moving to an ABAC system entirely could be a cool idea, I don't see us doing away with roles any time soon in core.

So where do we draw the line? A permission such as "view published nodes" (RBAC) would need to be translated to its ABAC equivalent behind the scenes, whereas an "access in-place editing" permission might not.

---

More reading: http://stackoverflow.com/questions/36705901/alternatives-for-roles-claim...

Very cool. I see some consensus here.

I am totally into #107.

Couple of additions

Let's keep collecting use-cases
To everyone: to validate if this approach is valid for the whished use-cases please add those use-cases here or in the google-docs. So we can collect them.

@ #106 regarding drupal ideas queue
We have the google docs document (see #98). Let's get that done first and then decide if this belongs in the Drupal ideas queue.

Regarding rules/policies/policy-sets (lock/lock-collection)
In #86: "Theory: Any entity can get any number of locks, but once you have the key to open one, you gain access."
Can we improve on this by allowing more logical combinations? For example:

(lock-A AND lock-B)
OR
(lock-C OR lock-D)
AND-NOT
(lock-E)

This way we can support multiple modules at once that leverage entity_grants. I am not totally sure, but I believe in the current system you cannot combine domain and organic_groups together.

Can we improve on this by allowing more logical combinations?

This way we can support multiple modules at once that leverage entity_grants. I am not totally sure, but I believe in the current system you cannot combine domain and organic_groups together.

That's a hard thing to pull off. What if Group or OG make a promise that no-one will be able to see your content except for members of a group and then Domain comes along and tacks on a "OR it's being accessed under this domain"? Modules would unknowingly break other modules' access promises and we'd end up in a security nightmare.

Historically, node access modules already had a hard time playing together nicely in a sense that it would lead to confusing scenarios. I don't know if it will be that easy to get them to work together under an ABAC system. Which module would decide whether the conjunction being used is AND or OR?

@kristiaanvandeneynde Historically, that collision is why we added the _alter() hooks to node access operations, so that bridge modules can solve that problem.

I think in an ideal implementation, the AND/OR nature of access rules would be configurable. But the use-cases get complicated very quickly.

I've updated the Google Doc with some rough translations of a few user access issues I'm actively involved with in 3 different projects that could be solved with some of the approaches defined here.

Regarding #109 and #110.

In ABAC terminology there a 3 layers: rule, policy and policy-set.

Modules like Group or OG should provide single Rules (example: user_is_group_member) and probably some pre-defined Policies. For example: (user_is_group_member OR user_has_role).

I imagine the top-level (Policy-sets) should be managed side-wide. That way it can be guaranteed that there is one single source. Then it is up to site-builders to combine existing Policies in Policy-sets. For example: (content_is_in_current_domain) AND (user_is_group_member OR user_has_role).

So you mean that by default, all policies would be put in one big OR-conjunction, but we would show a UI with all of the defined policies allowing people to change that? While I like the idea, I think this UI might be a pain on all levels: code, UX, DX, ...

How about we allow policy definitions to optionally specify a policy_set? If left blank, it becomes 'default'. When checking for access all applicable policies are asked for their policy_set and AND-conjoined by policy set. If you want a module to stop trumping other modules, you could write a simple alter to change its policies to be tossed in the 'default' group.

Example below:

# Policies defined as YAML for ease of reading
user_has_role:
  # specs go here

time_of_day:
  # specs go here

user_has_group_permission:
  # specs go here
  - policy_set: group

Would become:

Grant access IF
  (user_has_role OR time_of_day) # Policy set 'default'
  AND
  (user_has_group_permission) # Policy set 'group'

If you were to alter Group's policy definitions to 'default' or Group would change them itself based on a checkbox in its settings screen, you would get:

Grant access IF
  (user_has_role OR time_of_day OR user_has_group_permission) # Policy set 'default'

Just a thought, but this way we don't have to build a huge UI that may not work as expected and will confuse the hell out of people.

I think it may be worth finding symmetries with the Condition Plugin System and the Entity Query Conditions/Groups. A lot of what is already built there is what we're discussing now.

For example, the entity query system already provides a group/condition model and the Condition Plugin System gives us a way to expose conditions throug the UI.

JSON API also has a query builder (I'm personally biased here) for building custom assertions out of essentially static values. This could be torn apart and re-purposed as a condition builder.

Relevant links:

• #1743686: Condition Plugin System
• #1912452: Condition Group User Interface
• https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
• http://cgit.drupalcode.org/jsonapi/tree/src/Query/QueryBuilder.php
• https://www.drupal.org/docs/8/modules/json-api/collections-filtering-sor...

I like where #113 is going!

Here's some more detailed aspirational config:

user.entity.condition.user_has_role.yml

label: 'User has role'
context: entity:user
conditions:
  group:
    conditions:
      - type: attribute
        field: role
        operator: 'CONTAINS'
        dynamic: role_id
    conjunction: 'AND' # this is how entity query conditions work - you have an implicit root AND group, even if you only have one condition.

mymodule.entity.condition.user_is_admin_or_has_role.yml # silly example

label: 'User is admin or has role'
context: entity:user
conditions:
  group:
    conditions:
      - type: condition
        name: user_has_role
        context: inherit
        values:
          role_id:
            dynamic: role_id
      - type: attribute
        field: uid
        operator: '='
        value: 1
    conjunction: OR

system.entity.access.node.article.yml

operations:
  view:
    user_is_admin_or_has_role:
      context: @current_user
      values:
        role_id: 'first_custom_role'
  edit:
    user_is_admin_or_has_role:
      context: @current_user
      values:
        role_id: 'second_custom_role'
    user_has_role:
      context: @current_user
      values:
        role_id: 'third_custom_role'

EDIT: Added additional config to 'system.entity.access.node.article.yml' to clarify after comment in #115

Re #114 I think currently the node access system defaults to OR. So changing that to AND in the top-level group will throw a serious wrench in the gears of a lot of sites :)

@kristiaanvandeneynde good point. If you look at the second file, you'll see that it's configurable to 'OR'. But I think maybe it wasn't clear that the only actual access related config file was the third one.

The *.entity.condition.*.yml files can be completely agnostic and unaware of how they're being used by this new access system. They're simply entity attribute assertions. It would be an added bonus if we could make those available for other applications beyond the access system (e.g. rules module).

The only access-specific config file is: system.entity.access.node.article.yml which takes advantage of these generic conditions. This might be the place that we use 'OR' for multiple role_ids.

#113

So you mean that by default, all policies would be put in one big OR-conjunction, but we would show a UI with all of the defined policies allowing people to change that? While I like the idea, I think this UI might be a pain on all levels: code, UX, DX, ...

#113

Just a thought, but this way we don't have to build a huge UI that may not work as expected and will confuse the hell out of people.

#110

@kristiaanvandeneynde Historically, that collision is why we added the _alter() hooks to node access operations, so that bridge modules can solve that problem.
I think in an ideal implementation, the AND/OR nature of access rules would be configurable. But the use-cases get complicated very quickly.

Also think UI is not very important here. A declarative interface with YAML would be a nice-to-have, but only if all API's are stable. What do you think about using the plugin system? We would have 3 plugin types: AccessRule, AccessPolicy and (optional) AccessPolicySet.

#113

How about we allow policy definitions to optionally specify a policy_set? If left blank, it becomes 'default'. When checking for access all applicable policies are asked for their policy_set and AND-conjoined by policy set. If you want a module to stop trumping other modules, you could write a simple alter to change its policies to be tossed in the 'default' group.

Do I understand you right that?
1. policy_sets will be combined with an AND condition
2. if the policy_set attribute is empty it has a default value that is 'default'

#115

Re #114 I think currently the node access system defaults to OR. So changing that to AND in the top-level group will throw a serious wrench in the gears of a lot of sites :)

+ #112
The 'rule, policy' layers from ABAC seem to be sufficient to mimic Drupal's current access-system. If a site has multiple Policies then each Policy would be in the 'default' Policy-set. Within this Policy-set each Rule is evaluated with OR.
So Policy-sets that are not 'default' is new functionality and we don't need backwards-compatibility.

#114

I think it may be worth finding symmetries with the Condition Plugin System and the Entity Query Conditions/Groups. A lot of what is already built there is what we're discussing now.
For example, the entity query system already provides a group/condition model and the Condition Plugin System gives us a way to expose conditions throug the UI.

Looks very similar indeed.

#104

Conceptually, I like the idea that the grant column could be flexible, but I suspect that would lead to performance issues because of the need to and a WHERE on a varchar. The current system uses boolean INTs to optimize query performance.

#97

To me an ideal system resembles cache tags, you have a set of services that output access tags for storage, and later add them to the access query. A subquery then limits a query to the results returned by the access query: WHERE entity_id IN ($access_query here).
However, our minimum MySQL requirement doesn't allow us to use subqueries anywhere without destroying performance (subqueries got performant from 5.6)

Is it avoidable that the initial calculation is top-heavy without dropping functionality? With cache-tags it should be possible to skip the re-calculations.
Would it be possible to do 'half-way' calculations and use them (or their cached results) to improve performance? For example lists with node-ids and user-ids per domain.

Do I understand you right that?
1. policy_sets will be combined with an AND condition
2. if the policy_set attribute is empty it has a default value that is 'default'

Exactly so

Added the Google Docs document to the issue summary.

Thanks, @ndf, @mxr576, @gabesullice and @kristiaanvandeneynde for moving this discussion forward! Great progress and I apologize for my absence.

I am very excited with the prospect to base the architecture on existing computer science theory and the link between the node grants system and ABAC is clearly there. I see there is also a broad consensus about implementing ABAC but as always the devil is in the details.

#117

Also think UI is not very important here. A declarative interface with YAML would be a nice-to-have, but only if all API's are stable. What do you think about using the plugin system? We would have 3 plugin types: AccessRule, AccessPolicy and (optional) AccessPolicySet.

I agree that an UI is not important for an MVP version. I can not remember using an UI when working with the node access module. However, it will be important for individual modules like the group module to correctly abstract and configure the access options. The plugin system seems to be a good fit. We similarly have used this for the notification system of Open Social to provide an UI to combine multiple plugins: https://www.getopensocial.com/blog/architecture-activity-streams and are using plugins for our analytics KPI architecture https://github.com/goalgorilla/kpi_analytics.

Beside plugins for the Rule, Policy and PolicySet (which is part of the PIP if I understand correctly) I feel we should allow modification for the Policy Decision Point (PDP) as well. But not in the form of a plugin. The PDP is the brains which makes the decision to block access or not considering all the policies. I can image that in complex platforms here are certain custom policies that always should have priority over policies from contributed modules.

A good default PDP is needed. It seems there are quite a bit of options to go for when there is a conflict in the policies: https://www.axiomatics.com/blog/entry/understanding-xacml-combining-algo...

Thinking about rules, policies, and PDP is quite abstract. It would be nice if we could come up with an example that is not too complex which we could use to communicate these ideas.

Cleaned upl and added some general concepts and examples of ABAC to the proposal document:
https://docs.google.com/document/d/1jIWKVoYbdVeEg-Kz_5yvlMvi7PycoHZWg4xX...

I would recommend reading the NIST Guide to Attribute Based Access Control (ABAC) Definition and Considerations document which provides a good explanation of the ABAC concepts and the document provides guidelines for the process for implementing a ABAC model.

http://nvlpubs.nist.gov/nistpubs/specialpublications/NIST.SP.800-162.pdf

The proposal document looks really good nielsvandermolen!
Why we want this, the functionality and the high-level technical architecture are written down thoughtful and understandable.

Now thinking about next steps:
- Is there general agreement on the proposal document? Are we close to that?
- As stated in #106; should we move this discussion into the Drupal Core ideas thread?
- How can we plan and manage everything that must be done? Should we do that in this issue? Is there a Drupal-approach for this type of major work?

Re moving to Core Ideas queue: yes, that's probably a good idea as a way of attracting from the framework manager and other major core architects before beginning coding.

I'm really glad to see some life back in this issue queue! It's perfectly timed.

I've been working the last week and a half on an implementation of the ideas in this issue queue. Though, perhaps in a less academic way. I have a pretty thorough proof of concept on the user facing side of things.

That work is spread across a few modules (linked below). But the result is that you can create your own attribute based policies like so:

id: 'first_letter_is_a'
entity_types: ['node', 'taxonomy_term']
operations: ['view', 'delete']
entity_condition:
  members:
  - type: condition
    property: 'name.0.value'
    operator: 'STARTS_WITH'
    comparison: 'a'
user_condition:
  members:
  - type: condition
    property: 'name.0.value'
    operator: 'STARTS_WITH'
    comparison: 'a'

I have two main modules written:

1. Entity Access Policies (https://github.com/gabesullice/entity_access_policies)
2. Attribute-based Access Policies (https://github.com/gabesullice/attribute_access_policies)

The first defines entity access policy plugins using the Lock and Key ideas provided above. The second allows you to express attribute-based access rules as config. This leverages the first module by providing a special plugin deriver that can interpret the config.

#2 relies on some internal modules tracked here: https://github.com/gabesullice/data_resolver and here: https://github.com/gabesullice/typed_data_conditions

Using #2, you can express complex attribute conditions using AND and OR groups like so:

entity_condition:
  members:
  - type: condition
    property: 'name.0.value'
    operator: 'STARTS_WITH'
    comparison: 'a'
  - type: condition_group
    conjunction: 'OR'
    members:
    - type: condition
      property: 'uid.0.value'
      operator: 'LESS_THAN'
      comparison: 500
    - type: condition
      property: 'uid.0.value'
      operator: 'GREATER_THAN'
      comparison: 1000

The groups are infinitely nestable.

Because I'm less familiar with the actual SQL-altering mechanisms used by the node_access system today, the entity_access_policies module is just translating everything above into hook_node_access and hook_entity_access implementations. But it's all written in a way that that should be very easy to remove the hooks and replace them with a lower level implementation.

@ndf asked me to comment here. This is a topic that would probably be more helped with input from a framework manager than from a product manager. What I suggest to start with is update the issue summary. A before/after in (pseudo?) code is not enough for people to understand what's being proposed and weigh pro/cons.

In the ideas issue queue we use the following questions to help define the elevator pitch for an idea:

Idea summary

What is the problem to solve?

(Missing functionality X, prevent Y from happening, make Z easier to do)

Who is this for?

(Evaluators, site visitors, content authors, site managers, (advanced) site builders, developers, core developers, site owners, other?)

Result: what will be the outcome?

(Describe the result: how will this improve things for the stated audience(s)?)

How can we know the desired result is achieved?

(Usability test, a metric to track, feedback)

It’s probably useful to do something like that here as well and move the TLDR version of what's in the google doc over to the issue summary.

The tag you want is "needs framework manager review". Not adding that yet because this really needs that updated issue summary first to help subject matter experts understand the gist of the proposal here.

You're all very welcome in the Ideas queue: https://www.drupal.org/project/issues/ideas. You can also consider opening a new one in the ideas queue that focusses on the Why and What. At 124 comments in I think I see a lot of discussion on the How. Why and What come first though. So maybe postpone this one on a new idea issue, but I leave that up to you all in here to decide :)

Thank you for your help @yoroy and your review of the issue!

I have done my best to synthesize this long issue queue using your template and have created this issue in the ideas queue: #2881492: Add a node_access like system for all entity types

I invite anyone here to feel free to update my synthesis if they feel I have left anything out.

Thanks, @ndf, @yoroy. Great write up and prototype @gabesullice!

I see we have a high level understanding and consensus to implement a ABAC for Drupal 8. #2881492: Add a node_access like system for all entity types describes the issue in clear language and I think we should go ahead to involve a framework manager. However, it seems we should still discuss the details of the architecture. There are a few potential options and requirements we should make explicit.

- Do we want to use something like XACML or another implementation protocol?
- Often in ABAC system policies are stored on an external service, do we want to take this into account?
- Do we focus on developers for now or do we want to support less techincal roles to work with policies (e.g. site managers)?

Personally, I find the terminology used in XACML very complex and they introduce a lot of additional concepts that will be difficult to understand (e.g. rules, effects, advices, PIP, PEP, PAP, etc..). I suggest to avoid these concepts for now and stick with the more core ABAC concepts and drupalize it. Furthermore, similar as for Node access, for an MVP version we should focus on developers only.

This is how I could see a ABAC system working with Drupal:

• EntityPolicyAccessControlHandler -> service that handles the logical access control -> implement all relevant Drupal hooks of the EntityAccessControlHandlerInterface so it plays well with default Drupal.
• Attribute -> Plugin to retrieve the current Subject, Object and environment attributes
• Policy -> Collection of conditions
• Condition -> Logical operations based on Subject, Object and environment attributes

If we make the attributes a plugin it should be possible to retrieve attributes from external services (e.g. to retrieve the temperature of a room). However, policies will be retrieved from Drupal only for the MVP version.

A process for the EntityPolicyAccessControlHandler could look something like this:

1. Retrieve all relevant attributes for a request
2. Retrieve all relevant policies
3. Check which policies apply to the current situation
4. Determine the access given te current policies (e.g. solve conflicting policies).

It seems the Lock/Key concepts will not be needed anymore in such an architecture.

Proposal looks great! Could be just ai addition & keep bc

@nielsvandermolen

- Do we want to use something like XACML or another implementation protocol?
- Often in ABAC system policies are stored on an external service, do we want to take this into account?
- Do we focus on developers for now or do we want to support less techincal roles to work with policies (e.g. site managers)?

- I think we should use that for inspiration and to help us explore the design space, but I agree this should be Drupal native. Part of this proposal is making the node_access system more accessible to more developers and eventually site builders.
- I think if we implement this using services/plugins, it is taken into account by default. Power-users with unique storage requirements can write their own plugins/swap the default services.
- I think this should follow the same path as REST module. That is, we should implement this as a developer API first, as node_access is today, and either let the community build various UIs, or experiment with a standard UI module in later iterations.

EntityPolicyAccessControlHandler -> service that handles the logical access control -> implement all relevant Drupal hooks of the EntityAccessControlHandlerInterface so it plays well with default Drupal.
Attribute -> Plugin to retrieve the current Subject, Object and environment attributes
Policy -> Collection of conditions
Condition -> Logical operations based on Subject, Object and environment attributes

EntityPolicyAccessControlHandler: This makes sense to do as an additional step, but we're still talking about persisting access policies to the database so that we can use JOINS like node_access does, correct?

Attribute: I can see the value of making this a plugin, but I wonder if it's not over-engineering the problem. Can the Condition plugins do this? I'd like for the end result of writing a custom access rule to basically be extending a base class and implementing just a few simple methods.

Retrieve all relevant attributes for a request
Retrieve all relevant policies
Check which policies apply to the current situation
Determine the access given te current policies (e.g. solve conflicting policies).
[...]

It seems the Lock/Key concepts will not be needed anymore in such an architecture.

Perhaps I'm still not understanding the approach (a likely possibility :P), but I think you're talking about a system that is computed whenever the operation takes place. However, the beauty of the node_access system is that it can essentially be pre-computed and persisted to the database. I thought this issue/the proposal was about implementing a node_access-like system for all entities. If I am misunderstanding, would you mind sketching the outline of how we go from your process to a SQL-alter for me?

To me, the Lock/Key and your proposed process are not necessarily mutually exclusive. The lock/key concept is a simple foundation on top of which one could build a persistable set of access policies/rules.

@gabesullice, I agree on the first three points

Attribute: I can see the value of making this a plugin, but I wonder if it's not over-engineering the problem. Can the Condition plugins do this?

An attribute can be used in more than one condition. I think we need this flexility and it also plays well with the ABAC terminology and performance.

For example:
Attribute: GroupMemberCountAttribute - counts the number of members in a group.
Condition1: GroupMemberCountAttribute > 10
Condition2: GroupMemberCountAttribute < 5

... we're still talking about persisting access policies to the database so that we can use JOINS like node_access does, correct?

... I think you're talking about a system that is computed whenever the operation takes place

It seems that the architecture should support attributes and conditions that could not be pre-calculated when an entity is created. The clearest examples are the environment attributes (e.g. current time and a temperature of a room). But also object (entity) attributes from external services and subject (user) attributes.

However, the beauty of the node_access system is that it can essentially be pre-computed and persisted to the database.

For the node access records (locks) you are right, but there is still a lot of computed calculations going on at operation time in the hook_node_grants to determine the user grants (keys). In essence the node_access system 'simpler' than ABAC but also contains a lot of issues which you also outlined in #2881492: Add a node_access like system for all entity types. The strong point of the node access system is that it provides an abstraction and you can program the node and user part separately. An ABAC system does have a provide a much more extensive abstraction where an attribute can basically be anything.

If I am misunderstanding, would you mind sketching the outline of how we go from your process to a SQL-alter for me?

Instead of focussing on the current implementation with locks/keys and SQL alters it seems smarter to make performance a requirement for the new ABAC system. There seems to be many solutions which we could think about. Those including pre-calculating certain conditions for entities and persisting them to the database. However, I also see a big opportunity to cache attributes and to do smart calculations regarding all the policies.

And what is needed now?

It is still important that we get consensus first about going with a key/lock approach vs attributes. Afterwards, I would suggest we try to get some collaborated development going on. Unfortunately, I do not have enough time to set up an initial proof of concept with my architectural proposal but the modules of gabesullice provide many technical solutions which we can use during the implementation phase (e.g. conditions in YAML).

- Entity Access Policies (https://github.com/gabesullice/entity_access_policies)
- Attribute-based Access Policies (https://github.com/gabesullice/attribute_access_policies)

Added the "needs framework manager review" tag to the broader idea issue #2881492. It would be helpful to get more input from core developers on the concepts and technical architecture.

Please consider adding the issue summary of #2881492: Add a node_access like system for all entity types to the issue here. This is a big plan, sample before/after code is not a good introduction to the problem.

Thanks everyone for the thoughtful work so far to design this API.

@catch and I discussed this issue this morning and agreed that the current, query-alter-based node grants system is a legacy API that does not scale well and that couples access control to the storage, which is not a good pattern for D8. It should be deprecated rather than extended. So, I'm retitling the issue to reflect that.

To evaluate the proposal, it'd be also helpful to have an issue summary update that reflects the current proposal (since there has been a lot of architectural discussion since @catch's last comment in #93). So marking NW for an IS update.

Also replacing "Contributed project blocker" with "Contributed project soft blocker" since this does not block existing contrib from being ported to D8.

I've given this some more thought and came to the following set-up:

• An access policy is a single "rule" that applies to a single entity type and a single action and an be expressed in one human readable sentence. They should not become too convoluted.
• Every access policy belongs to a policy set ('default' by default)
• When access is requested, each policy set is checked. Every policy set must grant access; i.e.: policy sets are combined with an AND conjunction.
• Policy sets can define their conjunction. The default behavior is OR.
• Policy sets can define a weight; the default being 0. Lower weights run first. This allows you to have a policy set with inexpensive checks that could fail quickly, allowing you to skip the checking of further policy sets (because of the top-level AND conjunction)
• An access policy's checks uses plugins, similar to the way Migrate uses plugins. Every plugin receives the subject entity as its first argument and the plugin-specific config as its second.

Both access policies and access policy sets are config entities and may look like this:

# access_policy_set.default.yml
description: 'The default policy set. If any access policy in this set grants access, the whole set grants access.'
weight: 0 # Can be omitted because 0 is the default
conjunction: 'or' # Can be omitted because 'or' is the default

# access_policy.node_view_published.yml
access_policy_set: 'default' # Can be omitted because 'default' is the default
description: 'People can view published nodes if they have the permission to do so' # Required key
entity_type: 'node' # Required key
action: 'view' # Required key

rules:
  -
    plugin: 'attribute'
    attribute: 'status'
    value: 1
  -
    plugin: 'permission'
    permission: 'view published nodes'

# access_policy.node_delete.yml
description: 'People can delete any node if they have the permission to do so'
entity_type: 'node'
action: 'delete'
rules:
  -
    plugin: 'permission'
    permission: 'delete any node'

# access_policy.node_delete_own.yml
description: 'People can delete their own nodes if they have the permission to do so'
entity_type: 'node'
action: 'delete'
rules:
  -
    plugin: 'author' # Special plugin that checks entities which implement EntityOwnerInterface
  -
    plugin: 'permission'
    permission: 'delete own node'

Now suppose you only want people to edit content between office hours. Because this is an inexpensive check, we want these to run first.

# access_policy_set.office_open.yml
description: 'Employees may only edit content while the office is open'
weight: -1
conjunction: 'and'

# access_policy.node_update_work_days.yml
access_policy_set: 'office_open'
description: 'People can only update their content on work days.'
entity_type: 'node'
action: 'update'
rules:
  -
    plugin: 'day'
    operator: 'in'
    value: {'Mon', 'Tue', 'Wed', 'Thu', 'Fri'}

# access_policy.node_update_work_hours.yml
access_policy_set: 'office_open'
description: 'People can only update their content during work hours.'
entity_type: 'node'
action: 'update'
rules:
  -
    plugin: 'time'
    operator: 'gte' # Greater than or equal
    value: '9am'
  -
    plugin: 'time'
    operator: 'lte' # Less than or equal
    value: '5pm'

Now here's the kicker: If we were to be able to designate that some policies should run before entities are retrieved, we could skip querying the database altogether if those policies fail. A good example would be the environmental checks for time of day above. If the office is closed and a user request a list of nodes they can edit, we should simply not run a query and return an empty list. How we designate that policies should run early is up for debate. It could be a config entity key, but that's just one way of dealing with it.

Finally, the above proposal is slightly flawed in a sense that all plugins would receive the target entity as their first argument, even if the plugin is an environmental one like the 'time' one used above. In true ABAC, we'd distinguish between environmental, resource (entity) and user.

Updated the summary with parts of the issue mentioned in #133.

I did not copy over the implementation-specific details because that has not been decided on yet. It could be plugins, it could be config entities, it could be hamsters on a treadmill. I also did not copy over the part about it having to be backed by SQL because we might want to move away from such a thing in favor of the scalable pagination mentioned in #93.

Would this modification of your proposal make sense:

1. Policy sets can be part of other policy sets.
2. A policy set that is not part of any other policy set is assumed to be part of the 'root' policy set; possibly its conjunction could be configurable.
3. Policies can have weights, same as policy sets.
4. A policy is part of the 'root' policy set by default.

As I see it this would give you:

1. more flexibility in logic, in that you can do things like (A and (B or (C and D))) that are otherwise impossible.
2. a simpler API, in that policy sets become a special case of policies, not a completely different animal.
3. It would help with designating policies that should run early.

Another possibility:
make the weight a property of the plugin, not the policy or set. After all it's the plugin that should know what it has to do to return its result and how performant it is. Then at runtime analyse the entire policy tree and find which sequence rules should be run in, based on their conjunction and weight. In other words, find all rules in the tree that must pass because their ancestry in the tree is all 'AND', then run them in weight order.

In OR branches an estimate of the probability of a rule returning a result is vital to knowing the best sequence to run in. One simple heuristic would be to use the average of the plugin weights in the OR set as an estimate of its weight. Conceivably one could store simple stats of past results to make a more realistic estimate.

I don't understand much about this stuff so please feel free to ignore if unhelpful.

Policy sets can be part of other policy sets.

A policy set that is not part of any other policy set is assumed to be part of the 'root' policy set; possibly its conjunction could be configurable.

I actually considered allowing you to nest policy sets but decided against it because it could quickly become a huge mess. Keep in mind that a policy is actually an AND-conjunction in its own right (the rules key). Although policies should be clean and concise, so there are limits as to how much you should put in one.

Don't get me wrong, I actually considered it. But I'm afraid this might turn an otherwise performant, nice looking system into a spaghetti of access policies. So how about we first try to build the system without nested policy sets and see what we can do with that? We can always build in nested policy sets after a first proof of concept.

Policies can have weights, same as policy sets.

Good call, this could be useful. If there is a very performant check that might grant access, we should be able to make sure it runs first to save resources.

A policy is part of the 'root' policy set by default.

This is what the 'default' one is for right now.

make the weight a property of the plugin, not the policy or set.

The idea behind this is great; making plugins decide how early they should run. But implementation-wise it's not possible because one policy can use multiple plugins (rules), so there's no way to figure out which one should take precedence.

In OR branches an estimate of the probability of a rule returning a result is vital to knowing the best sequence to run in.

This is a great idea that could be applied to AND and OR sets. Unless a weight is hard-coded, we could use result analysis to determine which one should run first. We'd have to allow for overrides by hard-coding a weight, though, to prevent us from forcing people to always run an expensive check first because it so happens to grant access a lot.

a policy is actually an AND-conjunction in its own right (the rules key)

The idea behind this is great; making plugins decide how early they should run. But implementation-wise it's not possible because one policy can use multiple plugins (rules), so there's no way to figure out which one should take precedence.

I'm suggesting you can build a logic tree that includes all the rules in all the policies in all the policy sets. Then you can decide which rule (not which policy or policy set) to run first. The idea is to shift the focus of sequence evaluation away from policies to rules. So at run time we do this:

1. build a logic tree of sets, policies and rules
2. Interrogate the rule plugins to learn their weight
3. Interrogate the trees logic and rule weights and past performance to determine a weight for every rule
4. Run the rules in sequence, updating the tree as results come in.
5. As soon as a YES/NO has cascaded up the logical tree (through the policies and sets) to its apex, stop and return the result.

You don't need to figure out which rule in a policy has precedence and make that rule's weight the weight of the policy, because it is rules that need to be sequenced not policies.

an estimate of the probability of a rule returning a result is vital to knowing the best sequence to run in.

This is a great idea that could be applied to AND and OR sets.

You're right, it applies to AND too.

We'd have to allow for overrides by hard-coding a weight, though, to prevent us from forcing people to always run an expensive check first because it so happens to grant access a lot.

I'm not sure that's true. Ideally:
Priority weight = probability * cost

So if it's expensive but common then we don't necessarily run it before rare but cheap.
A tricky question is how to judge cost: does one use average cost (which produces the lowest average wait times across all requests) or median (which delivers low wait times for most requests by could leave some waiting longer) or some kind of 99% worst case metric designed so that what is prioritised is that even the slowest requests don't take long.

It's a fun subject in the abstract but the practical answers to it are heavily determined by the realities of storing and computing the information. Looks like a job for some kind of swappable AccessEvaluationStatistics and AccessEvaluationPriority services.

An access policy is a single "rule" that applies to a single entity type and a single action and can be expressed in one human readable sentence. They should not become too convoluted.

This will lead to duplicate policies. E.g. for Open Social we use a visibility field on posts and nodes. When this field is reused on other entities it would not work without creating an additional policy as well. Could the entity type not just be another plugin? The job of every attribute plugin would be to check if the entity is valid for the plugin (e.g. if the field exists).

Every access policy belongs to a policy set ('default' by default)

Could you give an example of how you see the policy sets would be used and what the benefits will be? My feeling is that it is not really needed and it does add another concept for people to understand.

policy sets are combined with an AND conjunction. .. Policy sets can define their conjunction. The default behavior is OR.

So for policies it will be the OR conjunction by default? Seems like an AND conjunction covers more use cases. We could create support for a recursive OR attribute and AND attribute which would admittedly making the policies more difficult to understand but we would keep the flexibility of ABAC.

RE #139:

Priority weight = probability * cost

I don't fully agree here. If admins have access by default, and the is_admin check is very inexpensive, don't we always want to run that one first even if 90% of the users are not admins? For regular users it's perhaps a 1ms performance hit, whereas admins might see a 30ms performance gain from not having to run the other checks first.

RE #140:

Could the entity type not just be another plugin?

It could, but not without exposing risks. The reason I listed the entity type as a key on the config entity is that it allows us to quickly select the access policies that might apply to an entity type. By making it another plugin, it would allow people to omit said plugin and make some policies apply to any entity type.

Catch-all policies are dangerous as it could potentially grant access to something that shouldn't be accessible. They're also tricky because it requires other entity types to actively opt out of them if they don't want those policies to apply to them.

Perhaps a better approach is to turn the 'entity_type' key into an 'entity_types' key which allows you to list multiple entity types?

Could you give an example of how you see the policy sets would be used and what the benefits will be? My feeling is that it is not really needed and it does add another concept for people to understand.

I already provided an example with the office hours above. It basically says: "No matter what access policies were defined for nodes, if the office is closed you can't administer nodes." It's a top-level AND-conjunction: IF office is open AND all other node access policies allow access.

So for policies it will be the OR conjunction by default? Seems like an AND conjunction covers more use cases.

This is how it currently works in Drupal and how ABAC works per definition: If one of the defined policies allows access, you have access unless something else explicitly denies access.

E.g.: If you have two policies to view nodes "view published" or "admins can see all nodes", then only one needs to apply for you to have access.

Re #141

Perhaps a better approach is to turn the 'entity_type' key into an 'entity_types' key which allows you to list multiple entity types?

Good point and agreed. That would give use the option to discover and add new entities in a config override.

It's a top-level AND-conjunction: IF office is open AND all other node access policies allow access.

Ah ok, I am starting get your concepts for the policy set.

Furthermore, I can agree with the default OR conjunction for policies because it fits better with Drupal standards.

Going to update the proposal document with your technical proposal. Afterwards, the core maintainers (and other people) can give it a review and we should start the development effort. Would you (Kristiaan) and others be interested in spending some time during DrupalCon Vienna for a session where we work on this issue?

Would you (Kristiaan) and others be interested in spending some time during DrupalCon Vienna for a session where we work on this issue?

Definitely. My goal for the Friday sprints was to work on this and try and get #540008: Add a container parameter that can remove the special behavior of UID#1 committed.

Great Kristiaan, I am not sure if I attend the sprint day on Friday but I know I am available from Monday till Thursday.

Updated the proposal document with the latest changes (section 4.3 and 4.4). After reflecting the discussion the following questions came to mind.

- Should we rename rules to attributes to reflect the ABAC terminology better and avoid confusion with the rules module?
- Similarly, should we rename action to operation which is also used as term in Drupal core?

Please extend the proposal document if needed. I might have missed some discussions and decisions taken in the thread.

- Should we rename rules to attributes to reflect the ABAC terminology better and avoid confusion with the rules module?
- Similarly, should we rename action to operation which is also used as term in Drupal core?

Both seem okay to me. The problem with trying to apply ABAC to Drupal, though, is that Drupal has very specific terminology. So either we implement the spirit of ABAC with Drupal's terminology (operation, ...) or we implement ABAC with ABAC's terminology. Given how the most recent proposal actually implements the spirit of ABAC but doesn't follow it's specifications 100%, using Drupal's terminology might make more sebse.

Given how the most recent proposal actually implements the spirit of ABAC but doesn't follow it's specifications 100%, using Drupal's terminology might make more sebse.

+1

:(

This is looking really nice. I'm 90% on board.

In the current iteration of the proposal, policy sets seem to offer very little. In the example in the doc, the office_hours does nothing of value, all the rules could simply be put under one policy.

To my eye, and perhaps I'm missing something, they're only there to permit AND/OR logic, which could be more elegantly done under the rules key. Therefore, I think we can get rid of the policy set piece altogether - I think it is there only because we were attempting to match the terminology of ABAC.

Since we are Drupalizing the taxonomy of ABAC, we already have natural "sets" in entity types. Every policy simply applies to one or more entity types. E.g. the 'node' policy set or the 'taxonomy_term' set.

In order to accommodate boolean logic, we need only have a pair of special rule plugins which allow nested rules. Much as Migrate has the sub_process plugin (formerly, iterator).

The entirety of the office_hours policy set could then become something like this (added a lunch break to demo conjunction logic). The top level rules implicitly are AND'd together.

To handle "expensiveness", policies can still have weight and rules (where the actual complexity resides) should be evaluated in order.

# access_policy.node_update_business_hours.yml
description: 'People can only update content during business hours with an enforced lunch hour.'
entity_types: ['node']
action: 'update'
rules:
  -
    plugin: 'day'
    operator: 'in'
    value: {'Mon', 'Tue', 'Wed', 'Thu', 'Fri'}'
  -
    plugin: 'or'
    rules:
      -
        plugin: 'and'
        rules:
          -
            plugin: 'time'
            operator: 'gte' # Greater than or equal
            value: '8am'
          -
            plugin: 'time'
            operator: 'lte' # Less than or equal
            value: '12pm'
      -
        plugin: 'and'
        rules:
          -
            plugin: 'time'
            operator: 'gte' # Greater than or equal
            value: '1pm'
          -
            plugin: 'time'
            operator: 'lte' # Less than or equal
            value: '5pm'

Taking the last comment a little further..

If we ditch policy sets, policies should be able to be "enabled"/"disabled" per entity type. This would allow modules to provide optional policies. By default, they should be "disabled", but modules could override this in their config/install directory. For example, the node module might provide installation config to enable default policies for the node entity type.

I'm actually on board with the above. There are just a few things I'm still thinking of.

Policy sets AKA priority groups

Currently in Drupal, access policies are conjoined using OR; i.e.: If any policy allows access, access is allowed across the board. What if we want to add a policy that must match at all costs? Using the previous office hours example, people would still have access to the nodes if they have the 'view published content' permissions, even if the office is closed.

We don't want to resort to a top-level AND conjunction, because then you'd be very limited as to what value you can add with a new policy. It would also be a nightmare from the get go because you'd either need very convoluted policies that don't mean anything (i.e. "view nodes") or risk having no-one receive access. Because who would ever match "can view nodes" AND "can admin nodes" AND "can view own node", etc.?

So we have to default to OR, but I still feel like no matter what conjunction we use at the top level, we'll always be locking out possible scenarios. Which is why I came up with the policy sets in the first place. I'm not convinced that policy sets are "the" best approach, though. What if we replace policy sets by "priority groups"? The default being 0. Similar to how we can change hook execution order in core.

All policies of the same priority group are concatenated using OR. Priority groups are concatenated using AND and execute in priority order. Meaning if a higher priority group fails to give access, the rest of the access check fails as well. So the above example becomes:

# access_policy.node_update_business_hours.yml
description: 'People can only update content during business hours with an enforced lunch hour.'
entity_types: ['node']
action: 'update'
# Seeing as all access policies default to 0, this or any other priority-1
# policy MUST match before we even begin to evaluate the regular policies.
priority: 1
rules:

Negate flags

All plugins should have a negate flag. Just like you have in Rules. If I want to check that someone is NOT the author (e.g. to prevent the approval of their own drafts), we should not have to write a isNotAuthor plugin but simply be able to negate the isAuthor one.

What if we replace policy sets by "priority groups"?

I see your point about the need for a higher-order policy set. I like the direction you're going with. How about this idea? A rule plugin for policies.

I'm not in love with either solution, but wanted to throw this idea out. This one is a little weird because what effect would the actions key of the sub-policies have? Would it simply be overridden by the top-level policy?

# access_policy.highest_order.yml
description: 'Top-level policy composing different policies.'
entity_types: ['node']
action: 'update'
rules:
  - plugin: 'or'
    rules:
      - plugin: 'policy'
        policy: 'can_administer_nodes'
      - plugin: 'policy'
        policy: 'node_update_business_hours'

All plugins should have a negate flag.

Agreed.

Or... we do keep the policy set... as a single config per entity type and operation (i.e. you can't have multiple sets).

# system.access_policy.node.update.yml
policies:
  - policy: 'can_administer_nodes'
    required: 'TRUE'
  - policy: 'node_update_business_hours'

EDIT: added operations to the proposed config file name.

If we explicitly have to mention policies, then the system will never function well for several reasons:

1. Modules can never know which policies will be on the site except for their own
2. Policies won't be picked up automatically
3. As a result of the above, the burden is on the site builder to arrange and keep track of all of the policies

Using priority weighting solves this to some extent by allowing modules to say: "I don't know exactly which policies will be enabled, but I know for a fact that mine MUST always allow access before any of the default ones are checked." This can be pulled off by assigning any priority higher than zero.

It also enables another use case where you can target a specific module, provided not every module uses priority 1 for their special policies. E.g.: The Group module might run a policy at priority 5. On your site, it actually does not have to allow access as long as your custom logic does. The solution: Write a custom policy and add it to priority group 5.

It's not perfect, but at least it deals with modules being agnostic to a certain extent. If anyone has an airtight suggestion for modules not knowing about other modules' policies and their priorities, I'd love to hear it though!

We could build a complex drag-and-drop-nested-table UI allowing the site builder to arrange policies exactly the way they want, but it would again put the burden on them and would not allow for a "default state" because modules can't possibly know about all of the possible policies out there.

Actually, using priority groups we could build a UI that allows you to drag and drop default policies into different priority groups. We could store said order in a config entity that is specific to an entity type and operation. So policies can still be multi-entity (and even multi-operation), but policy sets / priority groups are specific.

This allows us to build a UI called "Configure entity access rules" which lists all entity types and their possible operations. When you click on edit behind any of the operations, you get taken to the config page for that entity type + operation.

We'd need a big fat warning at the top of the page, though, saying if you don't know what you're doing you might compromise your site security.

A nice thing to have for a follow-up issue: If we define possible entity type operations in the entity type annotation, we can make sure people don't add random operations to their policies. It would also allow us to check for supported operations in access checks, meaning we can exit early with an exception if someone is asking for an unsupported operation.

@kristiaanvandeneynde I think we're just millimeters from having this figured out. Perhaps this will be the final piece of the idea. We're so close however, that even if you don't like this idea I'm happy to agree to your latest proposal in the interest of moving towards a prototype.

At this point, I don't think modules need to provide policies of their own at all. They need only provide their own rules and then provide the default configuration which enables those rules.

We should make it very simple to provide custom rules. I suggest we do that by following in the footsteps of permission and link YAML files by letting rules be defined as annotated plugins (the most primitive ones will be) or as YAML (just as links or new permissions are).

To permit modules to affect policies by default their custom rules can be added via appropriate configuration in config/install for whichever entity type + operation they need.

Sample files are worth a million words:

# system.access_policy.node.update.yml
rules:
  - plugin: 'day' # Could have been added via a config UI
    operator: 'in'
    value: {'Mon', 'Tue', 'Wed', 'Thu', 'Fri'}'
    weight: 0

Here's an example of how OG might ensure its group rules are enabled by default:

# og/config/install/og.access_policy.node.update.yml
rules:
  - plugin: 'user_in_content_group' # see below
    required: 'TRUE'
    weight: -1

Custom rules could be provided as YAML using custom classes or annotated plugins:

# og.access_policy.rule.user_in_content_group.yml
label: 'User in content group'
description: 'True when the current user belongs to the same group as the entity'
category: 'Organic Groups'
class: '\Drupal\og\Plugin\AccessPolicy\Rule\UserInGroup' # or something similar

But, most rules would be compositions or extensions of other more primitive rules:

# shape_based_access_module.access_policy.rule.entity_is_triangle.yml
label: 'Triangle'
description: 'True when the entity is a triangle'
category: 'Shapes'
rules:
  - plugin: 'field_value' # field_value would be a more primitive rule provided as an annotated plugin.
    field: 'field_shape.0'
    operator: '='
    value: 'triangle'

# shape_based_access_module.access_policy.rule.entity_is_four_sided.yml
label: 'Four-sided'
description: 'True when the entity shape is four-sided'
category: 'Shapes'
rules:
  - plugin: 'or'
    rules:
      - plugin: 'field_value'
        field: 'field_shape.0'
        operator: '='
        value: 'rectangle'
      - plugin: 'field_value'
        field: 'field_shape.0'
        operator: '='
        value: 'parallelogram'

As you say, in a future iteration, we could provide a configuration UI with a glaring security warning that would let users remove rules from the entity_type + operation config. If a module really needed to be sure that their rules are enforced, they could provide a config.factory.override service.

But, most rules would be compositions ... of other more primitive rules

This I love. It avoids any limitations on the possible logic caused by hard-coded and/or. You can have a deeply nested tree of ands and ors.

It looks like we're finally imitating rules condition plugins here. Isn't a good part of this now in core?

It looks like we're finally imitating rules condition plugins here. Isn't a good part of this now in core?

We're getting close to it, but we're not imitating it per se. Condition plugins could be used at the lowest level of an access policy, but the system that enforces them and actually grants or denies access based on them still needs to be built :)

Re #156, I think we could go with the following:

• We will use Condition plugins as the most low-level component. At least for now, until we can better decide whether it's working for us or not.
• Modules can (obviously) define their own conditions.
• Modules can also define access policies using said conditions. Most modules won't have to, but some modules such as Group, OG, Commerce definitely will need to.

This still leaves the question of precedence. What if a module was specifically designed to allow you to trump existing policies? This can still be solved with the priority idea from #154. While I love the idea of nesting policies within policies, allowing for an unlimited amount of AND/OR combinations, we still have the issue of agnostic modules.

Suppose both module A and B want to trump "view published nodes". Module A is installed first and nests it within a policy that says "view published nodes AND conditionA". If module B wants to do the same, we'd end up with 2 policies: VPB+conditonA and VPB+ConditionB where we actually need VPB+ConditionA+ConditionB. By using priorities we can be sure both ConditionA and ConditionB have to apply because both would have a higher priority than VPB.

Unless someone has a great idea how we would allow other modules to influence/trump policies defined by core/other modules.

a great idea how we would allow other modules to influence/trump policies defined by core/other modules.

Here's a shot. The basic idea is that if rules have names then you can do things like override them by having the same name and a higher weight, or add to them by declaring that this rule "supplements" a specified rule.

# shape_based_access_module.access_policy.rule.entity_is_regular_quadrilateral.yml
label: 'Four-sided regular'
description: 'True when the entity shape is a regular quadrilateral'
category: 'Shapes'
rules:
  - plugin: 'and'
  - shape_is_quadrilateral:
      rules:
        - plugin: 'or'
        shape_is_rectangle:
          - plugin: 'field_value'
          - field: 'field_shape.0'
          - operator: '='
          - value: 'rectangle'
        shape_is_parrallelogram:
          - plugin: 'field_value'
          - field: 'field_shape.0'
          - operator: '='
          - value: 'parrallelogram'
  - shape_is_regular
    - inputs:
      shape:
        source_plugin: field_value
          - field: 'field_sides'

# shape_based_access_module.access_policy.rule.shape_is_regular.yml
label: 'Shape is regular'
description: 'True when the current shape is regular'
category: 'Shapes'
class: '\Drupal\shape_based_access_module\Plugin\AccessPolicy\Rule\ShapeIsRegular' # or something similar

# another_shape_based_access_module.access_policy.rule.shape_is_regular.yml
label: 'Shape is regular'
description: 'True when the current shape is regular'
category: 'Shapes'
weight: 100
rules:
  always_regular: TRUE

# yet_another_ shape_based_access_module.access_policy.rule.square_aware.yml
label: 'Shape is a square'
description: 'True when the current shape is a square'
category: 'Shapes'
supplements: share_is_quadrilateral
rules:
        shape_is_square:
          - plugin: 'field_value'
          - field: 'field_shape.0'
          - operator: '='
          - value: 'square'

It's a separate subject, but you'll also notice above

  - shape_is_regular
    - inputs:
      shape:
        source_plugin: field_value
          - field: 'field_sides'

I was wondering if we could have a flexible syntax for sourcing values - maybe "source" plugins. A rule about users doesn;t have to care where the User comes from, it just needs to be passed a user as an argument.

I started coding for a proof-of-concept this weekend. This has helped me understand the issues and give me some clarity around what you're saying @kristiaanvandeneynde.

We will use Condition plugins as the most low-level component. At least for now, until we can better decide whether it's working for us or not.

Agreed, and in my implementation it seems they'll work just fine. There are already some good examples in Drupal\block\BlockAccessControlHandler of this exact use case.

Modules can (obviously) define their own conditions.

Yep. I suggest we implement a plugin deriver which will read YAML files and thus allow for very simple condition composition.

Modules can also define access policies using said conditions. Most modules won't have to, but some modules such as Group, OG, Commerce definitely will need to.

I think we've been saying something similar without using the same vocabulary. I'm on board with this. It is my fault for just letting it be implied, but not explicitly stating it in #156.

If you look at the og/config/install/og.access_policy.node.update.yml example file (this file name and format is refined below). I think this is very similar to what you're saying about priority groups and config. Note the weight and required keys. I think my use of "weight" is the pretty much equivalent to your use of the word "priority". "Required" serves to let policies be trumped. The config file name specifies the entity type and operation. There could be many of these files defined by modules.

After starting to write some code, I ended up refining the policy config this:

This defines a policy config object.

# config/install/access_policy.schema.yml
access_policy.*.*.*: # Format is: access_policy.entity_type.operation.arbitrary_label.yml
  type: config_object
  mapping:
    enabled:
      type: boolean
      label: 'Enabled'
#   required: # removed per #163
#     type: boolean
#     label: 'Required'
    weight:
      type: integer
      label: 'Weight'
    conditions:
      type: sequence
      label: 'Access Conditions'
      sequence:
        type: condition.plugin.[id]
        label: 'Access Condition'

A module would define a policy config object like so:

# config/install/access_policy.node.update.user_is_authenticated.yml
enabled: 1
# required: 1 # removed per #163
weight: -1
conditions:
  user_role:
    id: user_role
    roles:
      authenticated: authenticated
      negate: false
      context_mapping:
        user: '@user.current_user_context:current_user'

The "required" flag makes it trump other policies and the weight of -1 ensures it runs earlier than other policies.

@kristiaanvandeneynde, let me know if the above is still missing elements of your ideas in #154 and #155.

Essentially, all policies would first be grouped by "required" then sorted by "weight".

A: required: true, weight: 0
B: required: false, weight: 0
C: required: true, weight: -1
D: required: false, weight: -1

would result in:

(C && A) && (C || D || A || B)

EDIT: updated per #163

Actually, nix everything about required above. I reread #151, specifically:

All policies of the same priority group are concatenated using OR. Priority groups are concatenated using AND and execute in priority order. Meaning if a higher priority group fails to give access, the rest of the access check fails as well. So the above example becomes:

I've finally internalized that and I like it. I think that should resolve any remaining dissimilarities.

If we change weight to priority and remove anything about required from the above examples do you feel comfortable with everything @kristiaanvandeneynde?

@jonathanshaw, regarding source plugins... As long as we're proceeding with condition plugins, I believe this should be handled by the plugin context and the context_mapping part of the config above.

If we change weight to priority and remove anything about required from the above examples do you feel comfortable with everything @kristiaanvandeneynde?

I think so, yeah. We can do some iterations on what we want the final YAML file to look like, but at this point we should start writing some example YAML files to actually try out different scenarios.

Regarding source plugins: We know we will always have an entity to work with and we know Condition plugins can define any number of require contexts. So all we need is something that takes an entity and some config (from YAML) and reads out the necessary data to then pass to the Condition plugins.

Perhaps we should define it at the top of the policy YAML file as contexts:

# config/install/system.access_policy.author_is_user_1337.yml

label: 'Author is user 1337'
description: 'Only user 1337 can edit their own nodes and terms'
entity_types: [node, term]
actions: [update]

# SIMPLY AN EXAMPLE, BUT YOU GET THE IDEA
contexts:
  current_user:
    plugin: current_user

conditions:
  entity_id_match:
    entity: current_user
    id: 1337
  entity_owner:
    entity: entity # You get a context called 'entity' for free
    owner: current_user

We can focus on mapping contexts in a later phase, though. If we start with a set-up that only works properly with conditions that expect a single entity and we manually pass on the subject entity, we can at least try out something already. Also, the above is starting to look very "Rules/CTools/Page Manager"-esque that way. Maybe there is something we can borrow from Rules D8 for mapping contexts.

P.S.: I changed 'action' into 'actions' (perhaps use 'operations'?) because it would make sense for user_can_administer_my_module type of policies to have one policy check for one permission that allows multiple operations.

I think so, yeah.

Sweet.

Perhaps we should define it at the top of the policy YAML file as contexts.

If we go with Condition plugins, or even our own plugin type, I'd like to be prepared for a UI. I imagine any UI will have some sort of configuration form for each individual condition. This is the case with block visibility today and Condition plugins are able to specify their own configuration forms. Therefore, I think it makes sense to store configuration and context information alongside one another. #162's config format is actually taken right from the block visibility code.

I changed 'action' into 'actions' (perhaps use 'operations'?) because it would make sense for ... policies to [allow for] multiple operations.

Yes, this makes sense. I think operations is the right term. This follows the existing

hook_entity_access($entity, $operation,
 $account)

terminology. It also avoids conflict with "Action" plugins.

Right, I'm fine with trying to use context_mapping. We'd need to make sure the entity that is being checked for access is available as a context, though. There is one drawback I remember from contexts: They can be really slow, where access checks need to be snappy.

We can start off with them and see how well they perform.

For those attending DrupalCon Vienna: I've booked a BoF room on Thursday during lunch. We can try to formulate a battle plan there and then: https://events.drupal.org/vienna2017/bofs/reinventing-entity-access-laye...

@kristiaanvandeneynde thanks! +1

I'm working and hoping to be there.

Hi All,

Will be at the BoF session tomorrow. I recently wrote an introductory blog article with an example for Open Social.

Many separate topics where discussed in the previous months and I think we should clarify these concepts in the session tomorrow:

Priority groups
It seems there is consensus on the need of priority groups and it seems that priority groups would be better than policy sets. Maybe we could make the priority group optional where default policies would get the 0 priority?

#154

Using priority weighting solves this to some extent by allowing modules to say: "I don't know exactly which policies will be enabled, but I know for a fact that mine MUST always allow access before any of the default ones are checked." This can be pulled off by assigning any priority higher than zero.

It also enables another use case where you can target a specific module, provided not every module uses priority 1 for their special policies. E.g.: The Group module might run a policy at priority 5. On your site, it actually does not have to allow access as long as your custom logic does. The solution: Write a custom policy and add it to priority group 5.

Lets say the priority 5 group gives access but the priority 0 group gives no access. Will the subject still have access to the entity? As I understand the subject would not have access. The priority is not a true priority then right but more a method to group policies? The priority would then mainly be useful for performance reasons and to provide AND / OR capabilities.

Nested rules / conditions (and/or)
This will make the system more flexible but it will also make it more complex (both technically and UX). My vote would go to not support nested conditions. For the low amount of complex use cases developers can create custom plugins to do more complex And / Or checks.

Looking forward to meet some of you in person tomorrow!

Guys I am not in Vienna this week.

The proposed architecture looks solid to me.
I hope the BOF will help to get this proposal in a conclusive state so we can move on to the development part.

Did some small preparations for the BoF session in these slides:
https://docs.google.com/presentation/d/1Bfwh3RbAKfGCRKB9nfK6r41qNAb-u3xP...

I suggest the following changes for the priority grouping:

• Instead of using numbers which apply to the order has an impact use names instead (e.g. core, or group) making it more descriptive and avoids confusion about how to use the grouping
• Make it possible to put a policy in multiple priority groups for bypass permissions

The YAML file:

# config/install/system.access_policy.author_is_user_1337.yml

label: 'Author is user 1337'
description: 'Only user 1337 can edit their own nodes and terms'
entity_types: [node, term]
operations: [update]
priority_groups: [core]

# SIMPLY AN EXAMPLE, BUT YOU GET THE IDEA
contexts:
  current_user:
    plugin: current_user

conditions:
  entity_id_match:
    entity: current_user
    id: 1337
    negate: false
  entity_owner:
    entity: entity # You get a context called 'entity' for free
    owner: current_user

@Niels Can you explain this line:

Make it possible to put a policy in multiple priority groups for bypass permissions

Why not use a very negative weight then? Like -99?

Ah I might misunderstood the way you envisioned it to work then. As I understood it to get access all priority groups / policy sets should provide access if they have active policies for that entities operation. E.g. you should still have the published permission if a higher priority group gives access. If only the highest priority group gives access then we have to look at all lower priority policies and include them in the higher priority group which feels inefficient.

For people that were not here today here is a short summary of the BoF session:

Attendees:
- kristiaanvandeneynde
- gabesullice
- nielsvandermolen
- reallifedigital
- timmillwood
- kingdutch

It was a very productive session which is a bit complex to summarize here but here are the key takeaways.
- A caveat was identified for overviews with many nodes (e.g. 10k results) could need a innovative solution in order to keep the performance high. Catch #93 actually provided a solution for this which was discussed as a potential option.
- We decided to make a contrib project first and open gates we need in Drupal 8 core with small patches. When the module is stable we will move it to core similar as with the workflow initiative.

We will use drupal.org for creating issues and Contrib Kanban to visualise the status. We will also try to make this effort in a official Drupal initiative to get more traction when we have a prototype.

I created this d.o. project which we can be used during the sprint tomorrow (If you change the name of your sandbox project Gabe I can change the name to Entity access policies.

https://www.drupal.org/project/entity_access_policies

Cheers for the summary Niels

@nielsvandermolen thanks for the wonderful project management! Thank you also for setting up the shared repo. I've changed the name on my sandbox and changed the title of the shared module.

So after a really long discussion, we came to the following concept:

• We ditch Condition plugins as defined by core because they were not intended to be used solely in an access scenario.
• Instead, we define three EntityAccessCondition plugin types which are all handled by one plugin manager, similar to how both entity types (content/config) are handled by the EntityTypeManager. These are as follows (naming under discussion):

1. PreCondition
2. FieldCondition
3. PostCondition

The PreCondition plugins don't need anything but the environment to run. So they can already tell you whether the user has a permission, whether the site is in maintenance mode, etc. If by any chance this denies access in a way that the whole policy set would then fail (because of conjunctions), then we can exit early.

The FieldCondition plugins would be up next, telling us what values they need. We could then alter the query so we only retrieve entities that at least match those required attributes. We'd still need to account for the conjunctions, so our query would need to reflect that using AND and OR keywords similarly to how the policy is built up.

Finally, we would pass the retrieved plugins to the PostCondition plugins which actually need to run some code that requires both the environment and the loaded entity and then tells us whether we have access to that entity or not.

Both the Pre- and PostCondition plugins would return an AccessResult class. If the end result is neutral, entity access is denied. The FieldCondition would return a value object which instructs the controller how to alter the EntityQuery.

Some more possibilities with the above are:

1. We could compile a policy set once (the conjunctions, that is) to make future access checks run faster
2. We could also use this system to check for access on already loaded entities instead of manipulating just queries
3. The *Condition plugins could list what interfaces the entity type needs to implement. This way we can throw a warning or exception when a policy set for say, products, is using a condition that does not apply to products (e.g.: is_published needs PublishedEntityInterface)

re: #176 and before. Can we document for posterity who was involved in the discussions so we don't have to ask if "so and so" has seen this proposal yet?

Regarding the field condition plugin, you might be able to use parts of https://github.com/fago/entity/pull/50/files which implement a really basic entity field based access system.

Development on this feature is taking place in the Entity Access Policies module.

Ongoing discussions can be found in the module issue queue.

We will continue to summarize and document major milestones here.

@heddn, #173 has the attendee list.

And now? :(

@nevergone, no need for a frown!

There haven't been any significant things to report yet but I did I put in a flurry of effort about 6 weeks ago. I had to put this on my back burner since then. Other devs have had to do the same.

But it's definitely not forgotten! It's at the top of many people's priority lists, we're just looking for time to permit us to work on it again. Personally, it is the thing I want to work on most as soon as I get the opportunity.

If you're very interested in what progress has been made, you can take a peek at the module codebase. Where we have some working example/proof-of-concepts in it. However, much is still likely to change so don't count on anything in there.

If you're interested in helping, looking at the module issue queue and adding to the discussion where you feel we're missing things would help a lot.

Hey folks... bump :-)

[context: was looking at domain_access module and reln to entity permissions]

@cambridgepublishers, this issue has been stale for a few months. Its not forgotten, and just came up in relations to decoupled projects, and we may be looking more into this soon. However, I wouldn't expect this to move forward all of a sudden. There is a version of the feature that @gabsullice, developed available as the Contrib Entity Access Policy module. If you need an alternative solution to build access control on top of, you can try looking into Advanced Access, which is available for use. I am planning on implementing similar attribute type access as part of Advanced Access at some point in the future, if thats the feature in particular that your are looking for.

I'm going to try and raise awareness about this at Drupal Europe so that we might get more resources to work on this: https://www.drupaleurope.org/session/entity-access-lists-crucially-missi...

Cool stuff! I'll be sure to watch that in the replays, can't be present myself unfortunately but it would still be cool to see this moving forward :)

Question, probably a rude question. How is this system going to make it easier to generate a list of entities a certain user can perform a certain action on?

That was what got me excited about the initial issue. But halfway through this thread, storing access in the database was shelved. So all we're doing now is redesigning hook_access. Which is fine, but not half as exciting.

Now, I know it's impossible to fully store access in the database. Anything that relies on environments is out, essentially. But I'm still more interested in a way of storing access results in such a way that I can use them in my queries and only fetch most of what is needed instead of entirely too much and having to run checks on every row. The current system, and what you're proposing, is a fine way to handle entity level access. But list access is still left out to rot. Honestly, I don't have any ideas on how to make a properly scalable solution to storing access results in the database either, and keeping them all up to date as things change and get invalidated. But I was hoping, when I started reading this issue, that that was going to be what everyone would be applying their smarts to. Perhaps that the plugins would describe how to check the access not only during runtime but also during queries, where possible; what columns to check, what joins to do, as generic as possible so as to remain agnostic to the backend implementation specifics. A pipe dream, perhaps. But a dream nevertheless.

#189: An entity access layer completely backed up by the database, absolutely desirable. I've seen this need multiple times already, especially in context of decoupled architectures. Guess this is a long way to go, though.

Actually I somehow completely missed #176, which has the FieldCondition for query alters. So if that is part of the roadmap (didn't see any mention of it in the google doc) then my excitement levels just shot back up and I should stop reading and commenting on stuff at 1AM.

#2909970: Implement a query-level entity access API

Not sure if this is can be related or anything, but the idea of having access controllers as plugins looks like a neat an simple idea and would eliminate the "old" hook_ solutions. Have a look at https://www.drupal.org/project/pach

Crosslinking plus want to draw some attention to the module because there is no reaction from maintainers despite the fact that there was some patches provided and there is some outstanding admin tasks that needs to be done: #3007218: Add composer.json with patch, #3007224: Create dev release and enable automated testing.

Hi everyone!

In the meantime, before a full solution lands, which is the best way to create a custom access check for views without using Entity API contrib module?
I want to limit access to media items in the "Media Library" view, but the rows in the view are still present even if they are inaccessible (using a custom access control handler that extends MediaAccessControlHandler class).

I’ve seen on other places (like here https://drupal.stackexchange.com/questions/212634/node-access-not-taken-...) proposed solutions using hooks (hook_views_post_execute()) but I would like to check if there is a 'cleaner' way to approach this.

Just for reference, as I did not see it mentioned here: Meanwhile there is a new implementation of an access API in the Entity API module:

https://www.drupal.org/project/entity/issues/2909970

@anairamzap If you don't like to use the entity API module, you can still use the node grants API in core.

See this example for Drupal 7: https://cgit.drupalcode.org/examples/tree/node_access_example?h=7.x-1.x

The API in 8 is basically the same. This is the best way at the moment to make sure views are filtered correctly.

Maybe Drupal 8.9.x/9.0?

This will probably be 9.1 at the earliest.

Hey all. The node access maintainers haven't been active here in years (myself included). I appreciate the hard work and apologize for my silence.

I'm on a large D8 project with some interesting node access requirements. I've enabled entity module from Contrib written a custom query_access handler. I must say that this was much more straightforward than what I had been fearing. I have lots of enthusiasm for this approach in core. See the Change Record, and review the linked issue there if you want the back story.

The only 2 patches on this issue are from @kristiaanvandeneynde, author of the awesome Group module. I can only surmise that he supports the entity module approach since Group now depends on it. So, seems like we have consensus. I'm not aware of a core patch that brings in the entity module code. That seems like a strong next step.

Hey moshe, great you chime in and bring some time with you. I like that approach and maybe we can put it on steroids.

Backstory: For quite some time i wondered about the future of access.
When i saw bojan's code, it inspired me to completely rewrite it to handle single AND list access.
See how it is used in an access subscriber here and find the POC at entity_unified_access.

It is currently an underdocumented POC that i wanted to mature a lot before sharing, but it looks like it's time to share now. Feel free to PM me for any questions that arise.

+1 to #202.

Although this issue started being about a grants API, I wonder if it would be easier to take it in steps, where first we bring in the query_access API from the Entity project, and then in a followup we look at the grants API?

Retitling for that and recategorizing this back to a feature request, since it's a significant API addition to core.

Or, if we'd rather open a spin-off issue for the query_access API, and return this issue to being about a grants API, that's fine too, but then we'd likely postpone this one on that one.

Yes, I think thats a good plan. I forgot to distinguish between the new handlers and a new grants API in my comment.

The only 2 patches on this issue are from @kristiaanvandeneynde, author of the awesome Group module. I can only surmise that he supports the entity module approach since Group now depends on it.

Yup, the Entity module's approach is the way forward if you ask me.

Ideally, this would kill grants altogether, but I do appreciate the fact that many websites might have millions of nodes and grants were created to speed up certain queries on these websites. That said, I do believe we could avoid/mitigate the performance hit for most scenarios with only query access handlers and perhaps leave grants as an optional system modules opt in to in case their use case would otherwise blow up queries.

Although, modules that have specific per-node access rules tend to already lead to insane queries on very large databases, grants or no.

Sorry, forgot to unassign this. As it's now part of Entity API (contrib) and Group relies on that, my main blocker is gone. I still think this could massively benefit core so I'd like to ask what the process was for moving other functionalities from Entity APi (contrib) into core and whether we can apply the same process here?

+1 for #209. @moshe weitzman, and the others, how can we proceed here? Are there any plans?

+1 for #209.

There's one more related issue #2809177: Introduce entity permission providers which looks like duplicate of #2909970: Implement a query-level entity access API

Faced it again for contrib domain entity module #3155277: Node and User entities should be ignored by the UI would be great to have no collisions when group and domain modules enabled

This issue description states that the existing node_access system is inhibited because:

1. It is not widely understood
2. It is hard to understand because of poor terminology and conflicting use of terminology
3. It is limited the the Node entity type

I'm not so sure that the entity module's approach addresses points 1 and 2.

I have found dealing with query access handlers more difficult to understand or work with compared to hook_node_grants / hook_node_access_records.

Furthermore, if multiple modules want to have a say on access, it is harder for them to work together if each module is modifying the query directly. The current node_access system ensures that each module puts its rules into a single place (node_access table) which is great where multiple modules want input into the access ruleset.

I would prefer to be working with something similar to the existing node_access system, but addresses the 3rd point above, so extends beyond just nodes. It might however be possible for such a system to be built on top of the entity api approach, giving more flexibility, but then we would have different developers using different approaches, which again makes it harder for multiple modules to work together on access.

Thank you for your feedback, @drasgardian!

It's important to have such discussions and point out the weaknesses to reflect the current status. Anyway, I totally disagree that working with hook_node_grants / hook_node_access_records was ever simple. Perhaps you worked with similar logics before and so it was what you expected or you're simply a genius ;) , but I saw many people struggling with that and I personally can understand why.

The current node_access system ensures that each module puts its rules into a single place (node_access table) which is great where multiple modules want input into the access ruleset.

I also disagree here. Yes it's one table, but it's not good DX, it's hard to understand and it's far away from OOP concepts. I think that old way is simply unexpected but may live under the hood, if it fits.

Anyway, I absolutely agree that your points should be taken into consideration to once again think about the big picture and edge cases mentioned, like for example several modules setting combined & complex access rules. At runtime, statically, etc.
Some examples, which are needed to solve real-world use-cases and I think can't be solved in SQL-queries for all cases like:

• https://www.drupal.org/project/content_access
• https://www.drupal.org/project/entity_access_by_reference_field
• https://www.drupal.org/project/entity_access_by_role_field
• https://www.drupal.org/project/group
• https://www.drupal.org/project/access_by_ref
• ...

(any access check that has to be done at runtime)

The only way I see here is to have at least the option to post-process (opt-in) on hook_entity_access in views and entity queries. This should be provided by an API which ensures best possible caching and we should limit this to as few cases as possible!

It should indeed be simple to understand and flexible enough for all use cases. Other aspects like performance are also very relevant, but like said, these should be solved behind the API, with perfect UX. Perhaps we should once again have a look at how other large systems solve this, which patterns exist and what's best practice today. In the Drupal 6 / 7 past, I guess the access logic grew from static database grants to dynamic access api calls and their combination, which already wasn't good DX...

I don't know if this issue is the right place or better in a separate thread / issue... or perhaps core maintainers already had many discussions about, so this has been done already. Just wanted to share my experience and opinion regarding your points.

Over and out ;)

Furthermore, if multiple modules want to have a say on access, it is harder for them to work together if each module is modifying the query directly. The current node_access system ensures that each module puts its rules into a single place (node_access table) which is great where multiple modules want input into the access ruleset.

That's not how it works. The Group module already uses the new system and you can alter query access just fine. The whole lot of conditions is only added to the query after all modules have had a chance to say something. So unlike your assumptions, you are definitely NOT modifying the query directly.

Having said that, and with the most respect for whoever designed it at the time: The current node access system needs to go. It's dated, scales poorly to other entity types and operations and does not play nice at all with the other entity access layers. So -1 from me when it comes to keeping node grants.

Let's face it, the only thing that the query access API provides is a well-formed mechanic to declare access-related conditions to a query on runtime. But this API is currently not capable of replacing the whole node access system, for the matter of fact that there is currently not such a table for placing access rules. And I need to agree with that point made in #214, that it might be even harder now to declare "who wins" on runtime than having a table that distinctively declares those access rules. That argument is especially valid if not modules declare such access rules, but a Rules-like system does.

I think there is already another issue out there that addresses making node access records more generic, but cannot recall it ATM.

Thanks everyone for the input, it's great to keep this discussion moving.

My views above were largely influenced by trying to add additional access control rules to work alongside those of the group module (which as a whole I'm a massive fan of btw). In the end though, patched back in the node_access approach.

If there's a way to achieve that with a better DX than traditional node_access, I'm perhaps misunderstanding how to do it.

The documentation for extending group's access control model outlines disabling sql rewriting and using hook_views_post_execute to filter out items the user doesn't have access to. i.e. disabling query level access control if you need anything other than the default group permissions.
https://www.drupal.org/docs/contributed-modules/group/extending-groups-a...

My views above were largely influenced by trying to add additional access control rules to work alongside those of the group module (which as a whole I'm a massive fan of btw). In the end though, patched back in the node_access approach.

In Group 8.x-1.x you can swap out the group_node plugin's access control handler. In Group 2.0.x, which I hope to release Q1/Q2 2022, all you need to do is declare a small service. But please do not use Group as an example for what a generic access API should look like. I've actually gone ahead and circumvented parts of Entity API's query access layer because Group is such a specific use case.

Yet, here I am advocating for a generic query access API because I've seen it work for other entity types in Group and client projects and it kicks ass way more than node grants will ever be able to.

But this API is currently not capable of replacing the whole node access system, for the matter of fact that there is currently not such a table for placing access rules.

Indeed it's not, but when I said the node grants system runs into limits, it's those imposed by the creator. The table only supports view, update and delete, but what about revision operations, scheduling operations, etc.? You honestly can't expect to add an extra column every time you have a new operation you want query support for and then patch all of your installed modules to support said operation. Trying to duckpunch said operations into the view/update/delete trifecta often leads to even more trouble as you are now granting access to something another module didn't want the user to have access to.

Additionally, if we follow the example of node_grants, we have to create an access table for every single content entity type and hope nothing breaks. Add to that that the grants table is "first one to grant access does so for everyone" while the entity access layer in code is "first one to deny access does so for everyone" and you can see why people have grown to dislike the system:

• It only works for nodes
• It is diametrically opposed to the entity access layer in code
• It only supports three out of potentially dozens of operations
• It does not allow a module to properly deny access
• The words grants and access records are used incorrectly in some places, leading to even more confusion

It was a nice system at the time, but Drupal has grown and it's no longer unfeasible to expect your access layer to support any number of operations on any number of entities. Something node grants doesn't do at all.

I'm also not a fan of a "fixed" flat-table in regards of access control, it additionally feels like a "hidden" layer as it's not exposed in any manageable ways, and the API is indeed something a developer needs to get used to first in order to feed it right.

Yet I think we are still in the need of a generic concept that covers the storage of complex access rules, also addressing the mentioned limits in #219 esp. in regards of having possibly a higher variety in operations to be covered.

I'm currently thinking about the idea of exposing stored access rules to be managed as standard content entities ("Access records") using the Query Access API to join in those access records. Hope I'll find some time to kick off a PoC implementation. I've created a project for that and will do some work in there: https://www.drupal.org/project/access_records
I can repost here in case I made some noteworthy progress on it.

@mxh I wonder how access records is different from https://www.drupal.org/project/acl

Except Group module there's Domain access which needs to be compatible with group when both enabled

@andypost great question, guess the main difference is that ACL is bound to nodes (again) and using flat tables (again). The main idea of Access Records would be to use fieldable content entities instead. Another difference would be that Access Records would be joined to entity and views queries of any content entity by using the Query Access API (guess that's how Group currently works in regards of Group Content).

During development of Access Records, I noticed that Query Access API does not support subqueries, which makes it hard to let this module be based on the Query Access API. Created an issue for that: #3259313: Let Query Access API support subqueries but guess for the time-being, will need to build my own query alter hook implementations :(

Turned out that Query Access API does support subqueries, so my assumption made in #223 is wrong.

Created a first alpha release of the Access Records module. Feel free to check this one out, any feedback is highly appreciated to be posted in the issue queue of that project.

It does by nesting condition groups, but custom joins are still to be done manually last time I checked.

It does by nesting condition groups, but custom joins are still to be done manually last time I checked.

Exactly, that's still the case. Probably the ability of adding inner joins instead of subqueries may help preventing some performance problems in case the query optimizer of the used database cannot properly translate the formulated SQL query into something more efficient. But inner joins also may bring a new hurdle to take care about: Avoiding unwanted duplicate rows.

How can we move this issue forward?

After running into the described issue with Views and Custom Entities in combination with custom access modules that determine access based on field values and relations at runtime, I wrote https://www.drupal.org/project/views_entity_access_check as (more or less hacky) workaround to do an additional access check on each views row: $value->_entity->access('view')

This is surely expensive, but okay as opt-in for simple cases. Eventually I'll create a Core issue for Views to discuss adding something like this as advanced settings. With complex hook_entity_access() conditions on non-node entity types, a solution becomes more and more important, I think.

Happy to receive feedback and improvements in the module's issues. And even happier if we get this fixed in core one day, so this won't be needed anymore! :)

I would expect that with Modern Drupal's caching system we could probably take care of 70% of the performance issues of doing what is proposed in #231

We took the PoC patch in #75 and converted into a module since we needed this functionality.

As such, the overall design is still very similar to the core node access system of records and grants extended to custom content entity types. This is what we needed since we need something that will scale to larger number of entities and where we could easily apply the same kind of logic we are using for our custom OG-based node access records and grants.

Contributions are, of course, welcome (especially tests and review)
https://www.drupal.org/project/raft_entity_access

Curios about the differences between RAFT Entity Access and Flexible Permissions.

With regard to #3371246: [Meta, Plan] Pitch-Burgh: Policy based access in core is what is proposed in this issue still a good idea? I would rather we don't have multiple access and control systems hanging around. Maybe once #3371246: [Meta, Plan] Pitch-Burgh: Policy based access in core is in core the resolution here would be to deprecate the existing node access rights system rather than expand it into entities. Or maybe break node access and entity access into a node sub-module. Having multiple systems that do the same thing is generally bad DX.

If anyone is looking for a solution to this today there is also the Access Policy module which has implemented many of these concepts. If/When these API changes make it into core I will also integrate those changes into the module.

To learn more about it you can read the documentation or watch a recent episode of the Talking Drupal podcast where it was featured.

EDIT: To add, this module does support query access. However, it doesn't have a new API. It is using hook_query_TAG_alter and a collection of plugins. They are similar to views filter plugins (though automatic). I'm currently working on a submodule to add further optimizations around queries.

Re #235 and #236 as I explained in the other issue, Flexible Permissions and the work I'm currently doing to get it into core has nothing to do with query access. See #3371246-17: [Meta, Plan] Pitch-Burgh: Policy based access in core

While implementing a custom content type entity, I looked into access checking related to entity query logic and found this issue. I also created this support request #3418715: Does entity query access checking perform any access checking for custom content type entity? to confirm my understanding of entity query access checking.

In that support request, I've also proposed a few documentation changes and happy to have these documentation discussions over there. I think be helpful to communicate more clearly for custom content type entity developers.

Changing to the DX special tag defined on Issue tags -- special tags.

Coming back to this after all these years, I no longer feel like the solution we have in Entity API is sufficient for core. I tried it out with Group and ended up having to copy a bunch of it because of Group's complicated structure.

So what I instead propose is a service collector that gathers the right metadata from either an entity query or a views query (like Entity API and Group do now) and then passes that along to the tagged services for alteration. In a second phase, we could introduce a core tagged service that copies some of the work from Entity API so that people can still define entity type query access handlers that return some of the value objects we saw earlier.

• Phase 1: Service collector, allow modules to alter queries more easily
• Phase 2: Port the entity type handler from Entity API as a tagged service
• Phase 3: Try to provide sane defaults for every entity type in said handler

#240 @kristiaanvandeneynde

This lacks one key feature of entity module approach:
IMHO the entity module approach is the right direction in adding a declarative field condition that can be applied to single-entity AND entity-query access. I think it is NOT expressive enough though.

You can kind of do this now with hook_entity_query_ENTITY_TYPE_alter. The problem is that you cannot access the accessCheck property without using reflection.

A very simple implementation could be to either provide access to that property, or by adding an alterTag for access.

I think that mostly solves the problem described in the issue? I imagine it could be a more comprehensive API, but it's a good first step?

Honestly if we want to be stricter about it, we could add a method to EntityAccessControlHandlerInterface that forces entities to alter the entity query (or pass). Which makes it clearer when implementing a custom entity that you need to handle access controlled queries.

I was doing some research on where we use hook_query_TAG_alter in core for #3511909: Add a getter for Entity\Query\QueryBase::$accessCheck and I was surprised that it's only really used in NodeHooks1::queryNodeAccessAlter, which is probably why this issue is so long because that method is a beast and I think we need to find a better solution for all of the other entities first and then it might be more straightforward how to solve this problem.

For instance, if you run an entity query with accessCheck for a list of users, I would expect that query to return no results unless the requesting user has access user profiles (or one of the administrative privlidges). But right now, from what I can tell, it returns all of the results!

The same can be said for media and the view media permission. There are probably many more examples.

I'm starting to think we should indeed add a method to EntityAccessControlHandlerInterface that forces/allows for altering the EntityQuery and the logic is similar to EntityAccessControlHandlerInterface::access. Might also be nice to add a method to AlterableInterface that forces the query to immediatly return zero results. Lastly, we need some way to handle cacheability metadata that doens't weirdly rely on the rendererer.

All of that feels like a lot and has nothing to do with node grants, but, imho, is needed for a more comprehensive solution. Like if you don't have access to view media do more granualar permissions beyond that even matter? We aren't even handling the permissions we have right now let alone more granualar ones.

Here's my proposal to add to EntityAccessControlHandlerInterface

use Drupal\Core\Entity\Query\QueryInterface;

/**
 * Checks access to query an entity.
 */
public function queryAccess(QueryInterface $query, ?AccountInterface $account = NULL): AccessResultInterface

I think this should be simple enough to handle simple permissions like view media and access user profiles, but flexible enough to handle view own unpublished content which would require modifying the $query. This also allows appropriate cacheability metadata to be added and would only be executed if QueryInterface::accessCheck is TRUE. If an AccessResultForbidden is returned, we can no-op the query and immediately return zero results.

It does not expand node grants outside of nodes, but it should allow us to simplify the query entity access system? We can add hooks similar to hook_entity_access or hook_entity_field_access, maybe hook_entity_query_access ? Then modules can take further action.

What do you all think?

In principle having this on the handler seems good. But I see it as a small addition to the current system, not a simplification. And something that could be a separate issue.

I'm interested in the cacheability question you raise, but I don't understand how your handler proposal helps it. Can you say more?