Add a "Role-based access" processor

One of our projects required this for Commerce Product entities, which I've generalized for any entities, excluding nodes and comments as suggested.
This processor is based on the Content Access processor.

With the lack of entity alternative for node_access_grants() (did I overlook it?), similar function is included.
In time, https://www.drupal.org/project/drupal/issues/777578 should provide something for this.

Proposal attached.

It does make sense.

Wasn't aware of the checkItemAccess() function, was entirely extrapolation from ContentAccess.
Not sure yet wat the implications are for entities and permission to view all of its bundles or just specific ones. Will see when we get there.

Working on a rewrite including:

1. Approach as described above
2. Processor applicable to any search item
3. Refactor to class/file to RoleBasedAccess

Hope to post new version tomorrow.

Updated version using suggested approach. However, 2 issues:

1. Manipulating roles and faking 'authenticated' user on a new User object seems problematic, as many underlying functionality requires a full/existing user object. So I ended up using user 1 (the only user we can be sure exists), and manipulating its roles. This seems work more or less as desired, keeping in mind user '1's special status.
However, during testing I got stuck on the next issue:

2. Caching

Using checkItemAccess() will end up calling Drupal\Core\Entity\EntityAccessControlHandler::access, which uses static cache for given user and entity. Hence, check on first role for authenticated users will return the expected result, all following checks will return the cached result.

public function access(EntityInterface $entity, $operation, AccountInterface $account = NULL, $return_as_object = FALSE) { 
   ...
   if (($return = $this->getCache($cid, $operation, $langcode, $account)) !== NULL) {
      // Cache hit, no work necessary.
      return $return_as_object ? $return : $return->isAllowed();
    }
   ...
   $result = $this->setCache($return, $cid, $operation, $langcode, $account);
   return $return_as_object ? $result : $result->isAllowed();

Any suggestions?

I discussed this with @andreasderijcke at the DUGBE sprint on saturday. This cache is a static cache, not something that is actually saved in a database. It's a protected property on a class.

I mentioned that we might be able to use reflection to reset this cache, but got angry looks from Nick and swentel, so we didn't investigate that further.

We also shortly discussed creating a user, but creating a user every time we're indexing seems like asking for performance troubles (we might have to create a new user every 5 minutes, and deleting it afterwards).
It might be an option to create new locked users that we can reference from code, those would only have to be created once, but they could be used for logging in as well, and that doesn't sound like the best plan either (I think).

However, it seems using different (new) user objects would/might also resolve the caching problem? So my intuition would still be to get it working with separate "mock" users.

Well yes! However creating a mock registered user is the/my problem. You cannot add 'authenticated' role to a new User object. User entity code prevents this. To me, this implies replicating user registration in code, which i'd rather avoid.

In addition, even if we manage to mock a User object with role 'authenticated', without having an user id, function getRoles() (and possibly others) will still consider this user as 'anonymous'.
Therefore, using a real user as starting point, seems a nicer way to go.

Although, i'm not happy using user '1', creating a dedicated user for indexing also adds additional complications, as mentioned by @borisson_. Also, from website owner perspective, it feels wrong.

I'll check if indexing works using a blocked user. If so, the static cache (also in User entity class) remains a blocker.

No, I don't think there were any actual concerns apart from that reflex :). I agree that would be the most simple way to solve this issue. It might not be the most pretty code, but if it works and we add sufficient tests I don't think it'll be a big problem.

After more studying of the user module, I realized you can create a fake user (object) with id of your choosing.
I used the sequences value increased with an arbitrary value, to avoid collision with new users created during indexing.

Result looks good in the database, still have to test this on an real use case.

Does not works patch.
I get the user ID with an error.
Which means I can't load the user object

I created a processor that checks each entity for access.
This works for my project.

This would be a great processor to add, especially given most sites I've encountered have opted not to use the grants system for access. I believe this would work better for the majority of use-cases, even for nodes and comments.

@miroslav-lee, yeah, any access check that depends on reloading a real user will fail. Under that circumstance, the access check is not role based, so this kind of access plugin is not appropriate. Additionally, I don't believe the access system stipulates a user must be saved, so for cases like the module you've highlighted, bug reports can be filed and fixed to exit gracefully when access checking an unsaved user.

Here is a tweaked approach based on the work already completed by @andreasderijcke. I renamed the plugin class but git did not recognise this as a move since there were so many changes, instead a delete + add, so the interdiff was not really useful.

Here is a summary of the changes I've implemented and why:

• I've tweaked the explanation of node grants in the plugin class, since so few sites actually use the grants system.
• I've changed the plugin to extend ProcessorPluginBase, since it does not conceptually build upon the access checks in ContentAccess.
• I've removed all aspects of the processor which seemed to be copied from ContentAccess, which weren't provably required by the test coverage. Those may need to come back, but I'd like to understand the scenario for each case better + add a test.

Would appreciate any reviews or feedback.

All those changes look great @drunken monkey. This gets a +1 from me.

1. +++ b/src/Plugin/search_api/processor/RoleAccess.php
   @@ -125,7 +128,9 @@ public function addFieldValues(ItemInterface $item) {
   +   * The user entity is not saved.
   

   I'm not sure this comment adds much value, since an instance of UserSession is not an entity and can't be saved.

2. +++ b/tests/src/Kernel/Processor/RoleAccessTest.php
   @@ -41,23 +41,25 @@ class RoleAccessTest extends ProcessorTestBase {
   +    // role, we  do not want to test existing core access mechanisms.
   

   Double space in the comment.

This works nicely but I would like to clarify something:

I spend some time debugging because at first it did not work as expected: we have granted the "access_content" (View published content) to Anonymous and Authenticated roles. It seems as if this does not inherit properly to the other roles.

If I have a role "foo", I expected it to have "access_content" as well because I checked "Authenticated" (and this removes the checkboxes for all the other roles). But this is not the case. When RoleAccess.php iterates the roles, only Anonymous and Authenticated have access. The other roles don't use this. This ended up in solr as sm_role_access:["administrator"] After I unchecked the "Authenticated" checkbox and enabled every role on its own, the result was as expected (sm_role_access:["administrator", "another role with access", "...."]) and the access for the different roles worked as expected.

This seems to be the way Roles work but I found it to be a not-so-desired behavior in this case (unless it's a bug than I misunderstood the concept)

Hm, maybe the authenticated/anonymous role should be manually added to each UserSession. I looked up some instances of UserSession in core and found this in some test code:

  /**
   * Setups a user session for the test.
   *
   * @param array $rids
   *   The rids of the user.
   * @param bool $authenticated
   *   TRUE if it is an authenticated user.
   *
   * @return \Drupal\Core\Session\AccountInterface
   *   The created user session.
   */
  protected function createUserSession(array $rids = [], $authenticated = FALSE) {
    array_unshift($rids, $authenticated ? RoleInterface::AUTHENTICATED_ID : RoleInterface::ANONYMOUS_ID);
    return new UserSession(['roles' => $rids]);
  }

So that would seem to support the confusing behaviour you saw @DrColossos.

Fair enough, although this seems like a bug inside the Role system. I tried to look for an issue but couldn't find any.

Anyways, this should be added to the code to make it consistent with the the permission UI tells me.

I tried to handle this case: when the role is neither Anonymous nor Authenticated, add the Authenticated role to the user.

Nice patch, but we should add a test case for this additional behavior.

The isAuthenticated implementation definitely leads me to believe we should be using very high UIDs, instead of negative ones. Nice spotting on that one.

I would imagine the only caching that comes into play when calling off to the access API would be static caching, so I can't imagine the impact would be that huge, but it's certainly something that could be optimised now or later.

Nice additional tests too!

Hello,

I found problem with watchdog when we use negative or PHP_INT_MAX as uid.
Drupal can't index content after some log message appear.
We should use uid in range: 0 - int(10) to have possibility save logs into db.