Flagging access system not extensible

So I see a few different ways we can move forward:

1. Just rename the hook. It's a bodge, but it'll work.
2. Create a FLAG_ACCESS event. This would likely be more future-proof, although there might definitely would be performance implications.
3. Find a way to leverage hook_ENTITY_TYPE_access() ourselves. The problem would be in passing the target flaggable, as well as the correct operation. I'm not sure how proper it is to pass additional arguments with a core hook.

I personally prefer option 2, but I'm expecting that to be thrown out first.

Can events return data to the original instigator? I thought they were more of an observer pattern.

Yes, you set data in the event object passed to the subscriber function. The original broadcaster can then read changes from the event object.

Either way though, since core still uses hooks for access, it makes sense for us to do so too.

True, blazing a new trail here probably would be bad DX. We should try for option 3 then. If we need to pass the flaggable, we could do so via an extra parameter. I don't think module_invoke_all() really cares if we pass a hook *more* parameters than it asks for.

Yeah, you shouldn't hack the standard access hook, that would IMHO be confusing. Especially since the argument then would be optional for the standard operations like view and edit, and you'd need to document when it is there and when not.

It seems like a completely different thing to check whether you have access to edit/delete/view a flag config entity or whether you are a allowed to flag something or not. So +1 to separate hook, I'd also be OK with an event.

One thing that you should do while re-writing this is to use the AccessResult API (the single hook currently does return that but multiple doesn't). And I'd recommend to merge single and multiple hooks together. It's only slightly more complicated to write a check with a list of entities and right now, flag_flag_access() and flag_flag_access_multiple() are conceptually identical, just with a different implementation.

hook and event are different
I prefer event otoh hook is easy to understand (inline with core)

Suppose a checkFlagAccess() method in flag entity access handler

I once had the idea that whatever hook this ends up being doesn't use a giant switch statement, but instead delegates the access function to the FlagType plugin. Big switches like the one we used to have are often a code smell in OOP.

Field access does something like that, see EntityAccessControlHandler::fieldAccess(), it first ask the field class for a default, then allows the entity type to add to it (we don't have that part, obviuosly) and then a hook.

It'd be great to get e.g. @Berdir's feedback on that.

I think that's a possible way to do it, yes.

That handler just provides the default logic for a given entity type, right? After asking that, you still trigger the hook to allow other modules to add additional checks or override the default?

One thing you need to define is if and how far that default should be overridable. A common way to do that is to respect a forbidden() from the handler and allow anything else to be changed through the hook. So a forbidden() would not even need to call the hook. If someone *really* wants to override that, they can still replace the handler.

Flag links are never cached. We're using a lazy builder and we check access in there. Which means we check access to all links on a page, on all pages, every time. Lazy-builders are not cached, at least not with `'#create_placeholder' => TRUE,`. Yes, that is a problem but I don't really see a solution for that. We could try to build something fancy and cache the shown flags per URL and pre-fetch access checking/loading for them. We could also try to have a very small cache for the information whether a flagging exists for a user/entity or not. And use a specific cache bin for that, then sites could both either use fast cache backend for it or none. Maybe even default to null.

Also, an actual advantage of lazy builders is that they play well with https://www.drupal.org/project/big_pipe, so if you use that, Drupal can deliver the page early and then gradually deliver the flag links later.

Whatever we do (by default), this is the same for all users in all conditions (since we don't cache), just gets worse the more flag links you have on a page. And even if it wouldn't be, an entity handler is not the right approach. You want to use a handler if your logic varies by entity type and you want to be able to override it. And yes, as you said, the per-entity type part is covered through flag type plugins.

You can either put this into an existing or standalone service or as a new method on the FlagAccessControlHandler. Both can be overriden if necessary.

Well, my point is that you can't preload since you're called for every link separately. Not without some intelligent system that caches per URL which flags are checks, similar to how drupal core caches which paths are displayed on a page and then uses that information to preload that information again.

Yes, that's what I meant, small cache entries and possibly lots of them. But access "just" needs to do the entity query and not actually load it right? You just need to know if it exists and the query is enough to tell you that. There's also the option to have a custom storage class and that as a specific, optimized, hardcoded query because the entity query query builder is quite slow.

Ah, I see, misunderstanding. You''re talking about pre-loading for all flag types for a specific user/entity, which I guess is still possible all.

What I meant is preloading in case you have a view that lists 10 nodes or a node with 20 comments. That was possible in 7.x at least in some cases, e.g. through hook_entity_prepare_view() but it's not so easy anymore in 8.x because everything is rendered single and in callbacks, basically. So the useful only way to do that would be that self-learning process that I described.

That might be better than a cache. That would force site builders to use the SQL DB storage for flagging entities, wouldn't it? Though in practical terms, I don't think many sites would do anything else anyway.

Well, you you could even consider both (and default the cache backend to null, so only someone that can use redis for it and skip the DB completely can enable it). But yes, for the default database backend, it wouldn't really make sense to use caching then.

If you define a FlaggingStorageInterface then it's still possible for e.g. MongoDB to provide a specific storage. Core does that too, see FileStorageInterface, UserStorageInterface and others.

We now have FlaggingStorage, can that be leveraged to preload access checks (and counts)?

Tagging as a beta blocker pursuant to #2738795: [Plan] Use tags for release blockers, not priorities and upping to critical as per https://www.drupal.org/node/2716615#comment-11143889

Suppose flag type plugin defines different render, this is what comment types for, also blocks or formatters implements access in plugins OTOH there's quickedit in-place)
Exactly like a case for contact form enable/disable/show/submit

So the more I look at Flag::hasActionAccess(), the more I want to rip the thing out and replace it with a service. While this can be done in this issue, I think we need to break this down into smaller issues since this one looks like it's stalled.

What I think we should start doing is remove hasActionAccess() and create a service to handle flag access. We can move the logic out of the entity class and into this proposed service instead. Once that is done, we can continue by adding access caching to the service -- whatever that'll look like.

What's the benefit of moving it to a service?

I think I'm confused as to what in the hell this issue is really about. Originally this was about not providing third party code from altering permissions due to a name clash with a core hook. Then somewhere along the way caching got involved, and I really wish someone would update the issue summary with a proper description.

Removed FlagInterface::hasActionAccess().
Injected ModuleHandler into FlagService.
Implemented hook_flag_action_accesss().

I'm not exactly happy with this patch, since we need to call the module handler in multiple places. I keep thinking it would be nice to replace this with another service that can reduce some of the code duplication.

It also occurred to me that we could make the ability to define flag permissions extensible. Right now that's the responsibility of FlagInterface::getPermissions().

Fixed whitespace issues.

Both. The default behavior is booleans.

We probably should, but that really heightens the need to centralize this check somehow. A service makes sense since we'd need to inject module_handler. Furthermore, FlagService would need to inject this proposed FlagAccessManager.

Created FlagAccessManager, sooooo much better!

Found a coding mistake, squashed it.

Fixed merge error. >_<

I think the main confusion here because flags are attached to entities by "extra field"
Suppose it we change a way of attaching flags we can re-use field access system better (see comment module field)
I guess we can use hook_entity_base_field_info_alter() or field_entity_bundle_field_info()
Also there's computable fields and BaseFieldOverride

1. +++ b/flag.module
   @@ -431,3 +431,19 @@ function flag_hook_info() {
   + * Implements hook_flag_action_access().
   ...
   +function flag_flag_action_access($action, FlagInterface $flag,
   

   Entity level access hooks allows similar

   Btw how many actions possible?
   I mean is there a way for custom module implement "toggle" action for example?

2. +++ b/flag.services.yml
   @@ -15,7 +15,7 @@ services:
      flag:
        class: Drupal\flag\FlagService
   -    arguments: ['@plugin.manager.flag.flagtype', '@event_dispatcher', '@entity.query', '@current_user', '@entity_type.manager']
   +    arguments: ['@plugin.manager.flag.flagtype', '@event_dispatcher', '@entity.query', '@current_user', '@entity_type.manager', '@flag.access']
   

   this service looks doing too much, very probably needs split

3. +++ b/flag.services.yml
   @@ -27,3 +27,6 @@ services:
   +  flag.access:
   +    class: Drupal\flag\FlagAccessManager
   +    arguments: ['@module_handler', '@current_user']
   
   +++ b/src/FlagAccessManager.php
   @@ -0,0 +1,67 @@
   +class FlagAccessManager implements FlagAccessManagerInterface {
   ...
   +  public function access($action,
   +                         FlagInterface $flag,
   +                         AccountInterface $account = NULL,
   +                         EntityInterface $flaggable = NULL) {
   

   I'd better get rid of current user service and require to pass user account and entity

   That's a place where we can very similar signature for entity access hooks

4. +++ b/src/FlagService.php
   @@ -310,4 +315,34 @@ class FlagService implements FlagServiceInterface {
   +  protected function filterFlagsForViewAccess(&$flags, AccountInterface $account) {
   ...
   +    foreach ($flags as $flag) {
   ...
   +      $flag_access = $this->flagAccessManager->access('flag', $flag, $account);
   ...
   +      $unflag_access = $this->flagAccessManager->access('unflag', $flag, $account);
   ...
   +      if ($flag_access || $unflag_access) {
   

   looks like a lot of overhead here... too much hooks

   I'd better delay access check for prerender, this way we can keep less cache variations and will have strict access check at runtime, like http://cgit.drupalcode.org/drupal/tree/core/modules/comment/src/Plugin/F...

I think what I'm thinking of then are plugins. We want some flags to have the basic access system and some to have the basic + author system.

I thought I suggested something like that months ago...

In either case, could we just augment the FlagTypePlugin to include access? It seems like a logical idea to me given how access+author is entity type specific.

IIRC, what we discussed earlier in this issue is that that the flag type plugin has the first say for handling access. A new plugin seems a bit complicated to me, it will require configuration anyway and might want multiple features, so you'd need a plugin collection with multiple plugins and who knows what else.

I also think we should consider to (re-)implement the author/owner access use case in this issue, then we have a practical, real use case for which we can add tests.

Should we still support access hooks if we kick access down to the FlagTypePlugin? My sense is that we should, although there'd be no way to inject the modulehandler into the plugin (I think?).

See #13-#19. I'd expect that we call a method on the plugin first an then the hook(s), just like the fieldAccess() example that I mentioned there. Then the logic for the hooks and merging the access results doesn't need to be in the plugin.

This version does not define a new access service, but instead pushes access control to the flag type plugin.

They can also be shown in entity links (node links, code links), and rendered independently.

You can render fields independently and that what @Berdir said in #13 and #19

Having real relation to entity will make things much cleaner, why you wanna keep flags attachments in "fragile" way?

+++ b/flag.api.php
+ * @param \Drupal\Core\Entity\EntityInterface $flaggable
+ *   The flaggable entity.
...
+                                 EntityInterface $flaggable = NULL) {

$flagged_entity looks more descriptive, also why NULL allowed
For me that allows to flag nothing... and to flag something you need access to that before check access to action

From 46.2

this service looks doing too much, very probably needs split

I have only glanced at that .... but what I can see is that given recent changes, the event_dispatcher is now an unused dependancy.

+++ b/flag.services.yml
@@ -15,7 +15,7 @@ services:
flag:
class: Drupal\flag\FlagService
- arguments: ['@plugin.manager.flag.flagtype', '@event_dispatcher', '@entity.query', '@current_user', '@entity_type.manager']
+ arguments: ['@plugin.manager.flag.flagtype', '@event_dispatcher', '@entity.query', '@current_user', '@entity_type.manager', '@flag.access']

AndyPost++

Comment 46.2 is the gift that keeps on giving.

I'm having one of those days

the more I pick at loose threads the more loose threads I see.

Defined hook_flag_action_permission().
Renamed FlagInterface::getPermission() to FlagInterface::accessPermisssion().
Defined FlagTypePluginInterface::accessPermissions().
Leveraged the module handler in FlagTypeBase to invoke hook_flag_action_permission().
Defined flag_flag_action_permission().

Formatting fixes.

Possibly, although I do like pushing the permission generation to the flagtype plugin.

Removed hook_flag_action_permission(), left everything else.

Not sure if there have been changes to the plan for implementing hook_flag_action_access() since this issue was last updated, but this functionality is critical to something I'm building right now, and it would be very helpful if the flaggable entity were passed to the hook from the route access checkers and from ActionLinkTypeBase::getLink(). I'm attaching an update to the patch from #65, which implements that change.

Are any of the things I suggested in #64 still to do as well?

I checked over the changes from @socketwench's patch in #65, and confirmed that there is a permissions callback class \Drupal\flag\Permissions\FlagPermissions, which is referenced in flag.permissions.yml. The updated patch I'm attaching modifies this class to use dependency injection to access the flag service, instead of relying on the static \Drupal::service() approach.

Though I think the order of parameters should be action, flaggable, account, as account is the one that may be omitted when calling actionAccess().

I started changing the parameter order for Flag::actionAccess(), but then realized that FlagService::getFlags() calls this method and passes in only the action name and account object:

    $filtered_flags = [];
    foreach ($flags as $flag) {
      if ($flag->actionAccess('flag', $account)->isAllowed() ||
          $flag->actionAccess('unflag', $account)->isAllowed()) {
        $filtered_flags[] = $flag;
      }
    }

In the context where FlagService::getFlags() calls the actionAccess() method, it actually makes sense not to pass any particular flaggable entity, because the service is attempting to retrieve a list of all flags that are available to a given account (optionally filtered by entity type and bundle).

Given this consideration, rather than changing the parameter order in Flag::actionAccess(), I've updated the patch to inject the current_user service into the action link plugins, and to pass the account proxy returned by that service as the second parameter to Flag::actionAccess().

This is looking really good. I think we'll need some tests around the new hook though, so adding that tag.

Yep, looking really good! Just needs tests and it should be ready to go.

I am fleshing out the skeleton of a test ..

The test scenario I have seems simple

Two test users alice and bob.
and a simple test module to override the hook function ... which by design it is dumb....it just flips permission.

granted becomes denied .... denied becomes granted.

Test steps:-
1)
Assert alice has access
Assert bob does not have access

2) After module is enabled.

Assert Alice has no access
Assert Bob has access.

by "assert has access" I mean test both the ability to flag and unflag.

Anyway I am just posting early ... if that is radically different from what you want .. now is the time to speak ... I will be flexible.

That sounds like a fine approach to a test @martin107!

I have a first draft test

But I am doing something stupid ... so this a request for help.

My understanding which is clearly wrong and needs correction.

If a functional level test needs to install a test-only module then the directory structure needed is

moduleName >
test >
modules >
place module here

When I put my permission inverting module in that place

I get this message when I type

drush en simpletest

The following module is missing from the file system: test_flag_access_flip [warning]
bootstrap.inc:234

Can someone please help me get over the hump.. otherwise the test looks promising

PS when I place to test-only module at the same level as flag_bookmark or flag_follower it installs and inverts happily. -- but as this freaky friday module would make a great hacking tool lets no do that !

PPS I fixed up a few trivial cs errors as pointed out by phpcs.

Ah I get a different set of errors locally

I am fixing up the errors I can see reported by testbot.

moduleName >
test >
modules >
place module here

Close, it's actually tests, but in the patch it looks like you've got that already. Not sure what that test failure is indicating.

@jhedstrom Thanks for looking, I appreciate it

I will investigate more over the weekend.

when using ->install() 's the first param is an array - that is one fix.

initially the hook_flag_action_access() mechanism returns AccessResult::neutral()

if I comment out 1 doesUserGetDeniedAccess call things pass!!!

I am showing that in this patch

Over the weekend I want to address a misconception

We need to demonstrate a test case where a third party module wants to be neutral about access.
That is the flaw in my cunning plan

We need to demonstrate a test the case where a third party module wants to be neutral about access.

Perhaps you could use the state api to set a value you check for in the hook? For instance, take a look at the core entity_test hook. That uses the state api to conditionally do something in the hook, then the test method can toggle the value as needed.

@jhedstrom thanks for the advice .. that is clearly the way to go.

I am posting early to signal a major change in the direction of testing. I will fix the corner cases tomorrow.
.
In this new way of doing things - the concept of alice and bob is only useful to testing the default hook.

BUT also I want to test something else....

Other contrib modules need to be certain that when there are many third party modules overriding access then the master arbiter, that this patch adds, is going to function correctly

Looks like there are some fails unconnected with the patch

coincidentally the same unconnected fails appear in

#2792163: Removing Deprecated EntityInterface::urlInfo() from Flag code base

so core might have tightened up schema checking or something and flag tests are broken at the mo...

1. +++ b/flag.api.php
   @@ -57,3 +60,24 @@ function hook_flag_options_alter(array &$options, FlagInterface $flag) {
   +function hook_flag_action_access($action, FlagInterface $flag,
   +                                 AccountInterface $account,
   +                                 EntityInterface $flaggable = NULL) {
   +  return AccessResult::neutral();
   

   I don't think that our coding standard says to format many arguments like this?

2. +++ b/flag.module
   @@ -576,3 +576,19 @@ function flag_hook_info() {
   +  if ($action === 'flag' || $action === 'unflag') {
   +    $permission = $action . ' ' . $flag->id();
   +    if ($account->hasPermission($permission)) {
   +      return AccessResult::allowed();
   +    }
   +  }
   

   The correct way to do this is AccessResult::allowedIfHasPermission()

   I generally don't like implementing your own hooks in a module, might come back to this later.

   Also, per documentation, action must be one of those values, so why check it, should be checked earlier and an exception thrown or something?

3. +++ b/src/Access/UnFlagAccessCheck.php
   @@ -14,16 +17,39 @@ use Drupal\flag\FlagInterface;
       */
   -  public function access(FlagInterface $flag) {
   -    return AccessResult::allowedIf($flag->hasActionAccess('unflag'));
   +  public function access(RouteMatchInterface $route_match, FlagInterface $flag, AccountInterface $account) {
   +    $flaggable_id = $route_match->getParameter('entity_id');
   +    $flaggable = $this->flagService->getFlaggableById($flag, $flaggable_id);
   +    return $flag->actionAccess('unflag', $account, $flaggable);
   

   would have been a lot less duplicated code if we'd made a single access check with an argument for $action :) (_flag_access: 'unflag')

4. +++ b/src/ActionLink/ActionLinkTypeBase.php
   @@ -115,7 +140,7 @@ abstract class ActionLinkTypeBase extends PluginBase implements ActionLinkTypePl
   +    if ($flag->actionAccess($action, $this->currentUser, $entity)->isAllowed()) {
    
          $link = $this->buildLink($action, $flag, $entity);
   

   the access metadata should be passed to the returned render array, both in the case of having access and not having access.

   Something like:

   CacheableMetadata::createFromRenderArray($link)
   ->addCacheableDependency($access)
   ->applyTo($link);
   You can also simplify it a bit and just do ::createFromObject($access)->applyTo() if the original link is wrapped like it is now and re-assigned, then $link['#cache'] will certainly be empty. (applyTo() overwrites existing data)

5. +++ b/src/FlagServiceInterface.php
   @@ -167,6 +167,7 @@ interface FlagServiceInterface {
       *
   +   *
       * @param \Drupal\Core\Entity\EntityInterface $entity
   

   here's the mentioned empty line.

6. +++ b/src/FlagType/FlagTypeBase.php
   @@ -14,14 +19,35 @@ use Drupal\Core\Session\AccountInterface;
   -  public function __construct(array $configuration, $plugin_id, array $plugin_definition) {
   +  public function __construct(ModuleHandlerInterface $module_handler,
   +                              array $configuration, $plugin_id, array $plugin_definition) {
        parent::__construct($configuration, $plugin_id, $plugin_definition);
   

   as mentioned before, never seen this pattern in core.

7. +++ b/src/FlagType/FlagTypeBase.php
   @@ -102,4 +128,59 @@ abstract class FlagTypeBase extends PluginBase implements FlagTypePluginInterfac
   +    $results = $this->moduleHandler->invokeAll('flag_action_access', [
   +      $action,
   +      $flag,
   +      $account,
   +      $flaggable
   +    ]);
   

   as mentioned earlier, I would recommend to also add a method to the action type plugin. Code like this in flag_entity_view() can then be inside the action type plugins specific access check:

   if ($entity instanceof AccountInterface &&
   $entity->id() == \Drupal::currentUser()->id() &&
   !$flag_type_plugin->getAccessUidSetting()) {
   continue;
   }

8. +++ b/src/FlagType/FlagTypeBase.php
   @@ -102,4 +128,59 @@ abstract class FlagTypeBase extends PluginBase implements FlagTypePluginInterfac
   +    $allowed = FALSE;
   +    $forbidden = FALSE;
   +    foreach ($results as $result) {
   +      if ($result->isAllowed()) {
   +        $allowed = TRUE;
   +      }
   +
   +      if ($result->isForbidden()) {
   +        $forbidden = TRUE;
   +      }
   +    }
   +
   +    if ($allowed && !$forbidden) {
   +      return AccessResult::allowed();
   +    }
   +    elseif ($forbidden) {
   +      return AccessResult::forbidden();
   +    }
   +
   +    return AccessResult::neutral();
   

   this loses cacheablity metadata. You don't have to re-invent this yourself. See \Drupal\Core\Entity\EntityAccessControlHandler::processAccessHookResults().

   If you enforce that the action plugin must return an access object, then you know that you always have at least one and can skip the first check.

Combining 2 and 7, flag_flag_action_access() would then be the default implementation in the flag action type plugin.

It is always a pleasure to read such a thoughtful review.

Berdir++

I am spinning off part of #2 and the whole of #3 into issues in their own right. and then I will fix up the other stuff tomorrow.

Addressing the review from #92 - There is a lot going on, so fingers crossed I hope I have made the correct adjustments. - the new tests are still sideways.

1) Fixed.

2) Refactored, I think this is the place for a second assert.

3) Moved to a side issue

4) Partially done ... a hole in my knowledge - I am not sure how to add context to a empty render array.

5) Fixed.

6) Fixed - I think the coding standards were relaxed recently, to permit cases where core duplicates sections of Symfony code - but none of that applies here.

7) Fixed. - code is now in UserFlagType and the process is subject to override by third party modules.
actionAccess is mandated by the FlagTypePluginInterface

8) Fixed.

PS testing updated.

I see the module only ever looks for

actionAccess()->isAllowed()

and only every sets neutral() or allowed() never forbidden() --- cos that would obviously blow the hole page up

and yet to programme defensively, we need to demonstrate in tests how we handle third party module setting forbidden....

Can I ask what people think about how the module should behave ... should be ignore/ban forbid?

Fixing my mistake.

1. +++ b/flag.module
   @@ -334,10 +334,9 @@ function flag_entity_view(array &$build, EntityInterface $entity, EntityViewDisp
   -
   -    if ($entity instanceof AccountInterface &&
   -        $entity->id() == \Drupal::currentUser()->id() &&
   -        !$flag_type_plugin->canUsersFlagThemselves()) {
   +    $currentUser = \Drupal::currentUser();
   +    $action = $flag->isFlagged($entity) ? 'unflag' : 'flag';
   +    if (!$flag_type_plugin->actionAccess($action, $flag, $currentUser, $entity)->isAllowed()) {
          continue;
        }
   

   access checking already happens later on in the link builder/getLink(), you don't have to repeat that here.

2. +++ b/flag.module
   @@ -580,15 +579,7 @@ function flag_hook_info() {
   +function flag_flag_action_access($action, FlagInterface $flag, AccountInterface $account, EntityInterface $flaggable = NULL) {
   +  assert($action === 'flag' || $action === 'unflag', 'Unexpected $action string given.');
   +  return AccessResult::allowedIfHasPermission($account, $action . ' ' . $flag->id());
   

   imho not even the assert is needed here, only in the place where we call the hook.

3. +++ b/src/ActionLink/ActionLinkTypeBase.php
   @@ -140,9 +141,13 @@ abstract class ActionLinkTypeBase extends PluginBase implements ActionLinkTypePl
    
   -    if ($flag->actionAccess($action, $this->currentUser, $entity)->isAllowed()) {
   +    $access = $flag->actionAccess($action, $this->currentUser, $entity);
   +    if ($access->isAllowed()) {
    
          $link = $this->buildLink($action, $flag, $entity);
   +      CacheableMetadata::createFromRenderArray($link)
   +        ->addCacheableDependency($access)
   +        ->applyTo($link);
   

   almost correct, but you also need to do this when access is denied, so do it outside of the if.

4. +++ b/src/FlagType/FlagTypeBase.php
   @@ -150,37 +149,28 @@ abstract class FlagTypeBase extends PluginBase implements FlagTypePluginInterfac
   +  public function actionAccess($action, FlagInterface $flag, AccountInterface $account, EntityInterface $flaggable = NULL) {
    
        $results = $this->moduleHandler->invokeAll('flag_action_access', [
          $action,
   ...
   -        $allowed = TRUE;
   

   now I'm missing the call to actionAccess() on the flag type plugin here and that being merged into $results. Also needs to be defined on the interface and the base class.

5. +++ b/src/FlagType/FlagTypeBase.php
   @@ -150,37 +149,28 @@ abstract class FlagTypeBase extends PluginBase implements FlagTypePluginInterfac
   +    if (empty($results)) {
   +      // Return early with no opinion.
   +      $return = AccessResult::neutral();
        }
   

   because then you know that there is always one result and you can skip this.

6. +++ b/src/Plugin/Flag/UserFlagType.php
   @@ -90,4 +92,17 @@ class UserFlagType extends EntityFlagType {
   +
   +    $isFlaggableValid = !is_null($flaggable) && ($account->id() === $flaggable->id());
   +    $themselves_access = AccessResult::allowedIf($isFlaggableValid && $this->canUsersFlagThemselves());
   

   I think this condition is not quite correct yet.

   Also, when can $flaggable be NULL? Don't you always need a flaggable to be flag or unflag?

   You want to deny acess only if flagging yourself is not allowed and $entity is yourself. this is what you want IMHO:

   $is_current_user = $flaggable && $account->id() == $flaggable->id();
   AccessResult::allowedIf(!$is_current_user || !$this->canUserFlagThemeself())->addCacheContexts(['user']);

   (the access check depends on the user id, so it varies by user)

   (beware of type safe checks, id() is not type safe)

   Note how the only call to canUsersFlagThemselves() is now wrapped within the class, so it no longer needs to be public. showOnProfile() is already not called anywhere, not sure where that should be done or what it even means exactly :)

7. +++ b/tests/modules/test_flag_access_one/test_flag_access_one.module
   @@ -13,23 +13,22 @@ use Drupal\Core\Access\AccessResult;
      $state = \Drupal::state()->get('accessStateOne');
   

   state is not namespaced, so I would use flag_test_access_state_one here or so.

Please rollback wrong patch #2794767-7: Assert action is 'flag' or 'unflag'

@andypost, @joachim

Regarding #105

I responded as best as I know how with

https://www.drupal.org/node/2794767#comment-11584949

Sorry, I have just not had the spare capacity to resolve my mistakes/ move this thing forward ... until now.

before I start working on it again.

I think it is worth discussing one point early

regarding calls to UserFlagType::actionAccess()

Also, when can $flaggable be NULL? Don't you always need a flaggable to be flag or unflag?

The thing I want to draw attention to is FlagService::getFlags()

Where one use case for this function is

I'm root/admin and want to you tell me about all flags so I am just going to supply all parameters as NULL to get the master list.

I don;t have a opinion just yet ... the answer maybe to refactor ... but I am just laying things out so we can see what is what

As usual when there is a lot going on I am posting early -- to keep the review process less tortuous. I am still not completely done/happy with it.

A) I have added more testing, see the newly renamed Drupal\Tests\flag\Kernel\AccessTest::testUserFlag ( class was DefaultAccessTest )

I acknowledge, that much of the new testing is a duplicate of the access testing that happens in FlagPermissionsTest and UserFlagTypeTest
but it was quicker to write new Kernel test rather than debug slow Webtests.

B)
I want to highlight a previously undisclosed bug in that exists in the repo.
When responding to a NULL input the correct way to insert the current users in Flag::actionAccess()

    $account = $account ? $account : \Drupal::currentUser();

I don't want to spin it off into a separate issue as it is only tested by the new tests - but will do IF prodded.
[ Me: quietly slides the cattle 'prod' down the back of the sofa. while whistling innocently ]

So back to the review from #102

1) Fixed. OMG thank you ... a failure of epic proportions - that if committed songs would have been written about the size of my failure!.

2) Fixed. as you wish.

3) Fixed but as someone who has almost had song written about himself - this change may not be correct - I am adding cacheable metadata to an empty render array.

4)

now I'm missing the call to actionAccess() on the flag type plugin here and that being merged into $results. Also needs to be defined on the interface and the base class.

I know that this is the second time you have pointed this out but Sorry I don't follow the point you make.

5) Fixed.

6) Fixed. Thanks for correcting the type safe thing that is going to change a patterns I use a lot in daily life.

7) Yep I belatedly agree - swapped 'flag' with 'test'

Anyway more tomorrow.

Ah yeah I was playing with issues around $flaggable being null ( see #108 )

I'll have a look at this.

Ah, I see why some of my feedback didn't really make sense, I misread something. I thought the access implementation was on the flag class but it is already on the plugin. That's fine I guess.

And yes, you're right, flaggable needs to be NULL when checking access in general and not for a specific entity is a supported use case.

Your patch seems to have reverted a few changes, tried to undo that. $account ?: \Drupal::currentUser() is perfectly valid, that's the shortform for what you wrote.

Made a bunch of changes and fixed the test. Your main problem was around the fact that the first user in a kernel test is UID 1 and has all permissions. So you haven't been testing what you thought you were. When changing that, the next problem was that you gave a user administer flags permission.. but there is no code anywhere that implicitly gives those flag access.. neither before nor now. So I had to move things around a bit to give them proper permissions.

I'm not sure that AccessOverrideTest is really needed or that we need two test modules. Note that a data provider in a kernel test is fairly slow. You are basically testing AccessResult orIf(), which is already well tested. IMHO, it is enough to have one test module and have another test method in the new AccessTes (which I think is nice to have) to make sure you can e.g. forbid access despite having the permission.

You're also testing access for an invalid action, I don't think that should be allowed. I added the assert in actionAccess() now instead of the hook where you removed it now. I quickly tried adding and explicitly testing it but it is a bit tricky as it depends on ini settings whether it works. I removed the test for now, the only thing you are testing there is that no user has a 'corrupt value something' permission, which isn't different from the existing flag/unflag checks.

I wanted AccessOverrideTest to be a prepared defense against third party developers filing issue saying access permissions from module A are being overridden by module B etc

but yes the code is little more than an orIf() and coupled with the protection from the new assert - I have simply deleted all that stuff.

$account ?: \Drupal::currentUser() is perfectly valid.

I learn something new every day.

Is this patch ready ... It seems that way to me

Fixed! Thank you everybody!

Great to see this land!

One issue I've discovered:

+++ b/src/Plugin/Flag/UserFlagType.php
@@ -90,4 +92,20 @@ class UserFlagType extends EntityFlagType {
+  public function actionAccess($action, FlagInterface $flag, AccountInterface $account, EntityInterface $flaggable = NULL) {
+    $access = parent::actionAccess($action, $flag, $account, $flaggable);
+
+    // If the acting upon yourself check for permission.
+    $is_current_user = $account->id() == $flaggable->id();

Since the $flaggable object can be null, this results in fatal errors under certain circumstances. I've opened #2802653: The flaggable object is not always available in UserFlagType::actionAccess(), and it should be noted I cannot reproduce this with just the flag module in place, but other contrib may potentially use this method with a null flaggable.

Becaue \Drupal\flag\FlagService::getFlags() has the concept of being able to flag or onflag a certain flag in general, not for a specific entity.

That exists for fields too. E.g. views checks view access for a field without a specific entity to be able to hide a table column with that field completely. Flag could do the same possibly.