If you are new to this issue please read #2881492: Add a node_access like system for all entity types first for a clear summary and introduction.

Before:

db_select('node')->addTag('node_access')->execute();
$access = node_access('view', $node, $account);

After:

db_select('node')->addTag('entity_access')->addMetaData('entity', 'node')->execute();
$access = entity_access('node', $node, 'view', $account); //Parameter order subject to bikeshed

function entity_access($entity_type, $entity, $op, stdClass $account) {
  $info = entity_get_info($entity_type);
  list($id, $vid, $bundle) = entity_extract_ids($entity_type, $entity);

  // A bundle-specific callback takes precedence over the generic one for the
  // entity type.
  if (isset($info['bundles'][$bundle]['access callback'])) {
    $access_callback = $info['bundles'][$bundle]['access callback'];
  }
  elseif (isset($info['access callback'])) {
    $access_callback = $info['access callback'];
  }
  else {
    $access_callback = NULL;
  }
  
  if (isset($access_callback) && funciton_exists($access_callback)) {
    return $access_callback($entity, $op, $account);
  }
  
  // If there is no access callback, default to allowing access.
  return TRUE;
}

We should take advantage of an 'access callback' in the entity info for each entity.

Please contribute to technical proposal for a generic entity grants system in Drupal 8 Core

We want to reach a consensus about the architecture and terminology used in the new generic entity access system before we start with the implementation. Therefore, the audience of the proposal is for technical maintainers of Drupal core and people following the drupal.org issue.
https://docs.google.com/document/d/1jIWKVoYbdVeEg-Kz_5yvlMvi7PycoHZWg4xX...

Related issues

#1810320: Remove EntityTranslationControllerInterface::getAccess() once have entity access is postponed on this issue.
#2881492: Add a node_access like system for all entity types follow up with a clear summary and introduction to get other people involved

Comments

joachim’s picture

Seems like a good idea to me.

agentrickard’s picture

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

Dave Reid’s picture

Well this would be fairly easy as this helps us transition to a full-on entity access API like http://drupalbin.com/17312. This just gives us one nice function to call but leaves the work/storage to the individual modules.

Dave Reid’s picture

casey’s picture

+∞

fago’s picture

Makes sense - subscribing.

effulgentsia’s picture

Component: node.module » entity system
Assigned: Dave Reid » Unassigned

+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.

agentrickard’s picture

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.

moshe weitzman’s picture

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

agentrickard’s picture

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.

moshe weitzman’s picture

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

agentrickard’s picture

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).

agentrickard’s picture

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.

webchick’s picture

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
webchick’s picture

Issue tags: +Security improvements
sun’s picture

Issue tags: -entity API, -entity cleanup

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

webchick’s picture

Category: feature » task
Priority: Normal » Major
Issue tags: +needs backport to D7

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.

webchick’s picture

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).

hefox’s picture

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.

hefox’s picture

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.

BTMash’s picture

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).

moshe weitzman’s picture

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.

agentrickard’s picture

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

hefox’s picture

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!).

joachim’s picture

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

agentrickard’s picture

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.

hefox’s picture

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.

joachim’s picture

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

agentrickard’s picture

OK, I see now. Thanks.

fago’s picture

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.

sun’s picture

Title: Expand the node access API to a general entity access API » Expand the node access API to a general entity access API to improve security

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

agentrickard’s picture

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

nevergone’s picture

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?

YesCT’s picture

sun’s picture

Issue tags: +Entity Access
nevergone’s picture

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

Berdir’s picture

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?

xjm’s picture

Version: 8.x-dev » 9.x-dev

This is D9 at this point.

Berdir’s picture

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.

amateescu’s picture

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.

Berdir’s picture

Updating tags.

Berdir’s picture

Issue summary: View changes

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

catch’s picture

Issue summary: View changes
Status: Active » Postponed (maintainer needs more info)
Issue tags: -Node access, -needs backport to D7 +Needs issue summary update

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.

YesCT’s picture

Version: 9.x-dev » 8.1.x-dev
nevergone’s picture

Drupal 8.1? :)

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

Status: Postponed (maintainer needs more info) » Active

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?

Wim Leers’s picture

Issue tags: +D8 cacheability

+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.

kristiaanvandeneynde’s picture

Assigned: Unassigned » kristiaanvandeneynde

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.

kristiaanvandeneynde’s picture

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

pfrenssen’s picture

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

kristiaanvandeneynde’s picture

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

kristiaanvandeneynde’s picture

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.
kristiaanvandeneynde’s picture

Status: Active » Needs review

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
dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 52: drupal-entity-access-777578-52-WIP.patch, failed testing.

kristiaanvandeneynde’s picture

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?)

Berdir’s picture

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.

kristiaanvandeneynde’s picture

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.

Wim Leers’s picture

Great progress here, thanks @kristiaanvandeneynde!

nevergone’s picture

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

kristiaanvandeneynde’s picture

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
catch’s picture

Title: Expand the node access API to a general entity access API to improve security » Expand the node grants system to a generic entity grants system to improve security

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

nevergone’s picture

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

ndf’s picture

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'..

ndf’s picture

Title: Expand the node grants system to a generic entity grants system to improve security » Expand the node grants system to a generic entity grants system

Changed title, removed "Security improvements"

Covered with issue-tag "Security improvements"

kristiaanvandeneynde’s picture

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nevergone’s picture

And now?

kristiaanvandeneynde’s picture

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.

Wim Leers’s picture

@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.

Wim Leers’s picture

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

kristiaanvandeneynde’s picture

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.

Wim Leers’s picture

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!

kristiaanvandeneynde’s picture

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

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
24.51 KB

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?
kristiaanvandeneynde’s picture

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.

kattekrab’s picture

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! =)

kattekrab’s picture

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! =)

kristiaanvandeneynde’s picture

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

sinasalek’s picture

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

nevergone’s picture

kristiaanvandeneynde’s picture

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.

sinasalek’s picture

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alex.ksis’s picture

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).

kristiaanvandeneynde’s picture

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)"
kristiaanvandeneynde’s picture

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.

Wim Leers’s picture

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.

kristiaanvandeneynde’s picture

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.

bojanz’s picture

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).

kristiaanvandeneynde’s picture

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.

kristiaanvandeneynde’s picture

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.

catch’s picture

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.

Wim Leers’s picture

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 :)

kristiaanvandeneynde’s picture

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.

catch’s picture

@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.

bojanz’s picture

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)

nielsvandermolen’s picture

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).

kristiaanvandeneynde’s picture

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. :)

nielsvandermolen’s picture

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'.

kristiaanvandeneynde’s picture

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.

nielsvandermolen’s picture

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...

ndf’s picture

@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

agentrickard’s picture

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.

mxr576’s picture

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

gabesullice’s picture

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.

kristiaanvandeneynde’s picture

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...

ndf’s picture

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.

kristiaanvandeneynde’s picture

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?

agentrickard’s picture

@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.

gabesullice’s picture

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.

ndf’s picture

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).

kristiaanvandeneynde’s picture

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.

gabesullice’s picture

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:

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

kristiaanvandeneynde’s picture

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 :)

gabesullice’s picture

@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.

ndf’s picture

#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.

kristiaanvandeneynde’s picture

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

ndf’s picture

Issue summary: View changes

Added the Google Docs document to the issue summary.

nielsvandermolen’s picture

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.

nielsvandermolen’s picture

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

ndf’s picture

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?

jonathanshaw’s picture

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.

gabesullice’s picture

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.

yoroy’s picture

@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 :)

gabesullice’s picture

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.

nielsvandermolen’s picture

Issue summary: View changes

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.

andypost’s picture

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