Support for Flagging by anonymous users

Changing this to a bug, as it's a regression.

I expect we need to add a session ID field to the flagging entity -- so I'd like #2564445: add a 'global' field to Flagging entity to get in first, as the two patches will probably clash.

looks a good for start but needs tests
otoh that could use some deviceID(uuid) to prevent starting sessions

Yeah, I'm not exactly sure about the use cases here. If you rely on the flag count for something, then just a session based approach could get you into trouble. Users just need to open an anonymous window, flag, close and repeat to get as many flaggings as they want.

So is that using session ID and IP together? Because using just IP will cause problems with people who share an IP, won't it?

My company's entire headquarters shares one public IP. We would definitely need a session-based solution.

ETA: Obviously whether you want session or IP depends on your use case. For flags that aren't prone to abuse, sessions make more sense. In our case, it would be "I found this content helpful". For abuse-prone flags, you want IP address tracking. Maybe it needs to be two separate issues.

Here's a working proof of concept for session-based anonymous flagging. I took the patches from #4 and #12 and added session IDs on flagging save and in at least one place where flaggings are retrieved. There may be more.

I have no idea how to get Drupal to write a session cookie for an anonymous session without actually saving data of some kind in the session. This patch saves a 'flag' string to the session which we never look at again. If there's another way to do it, that would be great.

If there's strong feeling that IP-based checking is necessary, that can be added in also, as an additional field on the entity. But I think session support is necessary at a minimum. Without sessions, we have to disallow anonymous unflagging, since IP addresses are shared. Unflagging is a useful feature. ("Whoops! Didn't mean to downvote that post... wait, how do I undo?") It was also available in the D7 version, so losing it would be a regression.

For my personal use case, I really don't want IP-based checking, because I don't look forward to all the bugs that will get filed when people can't see the flag links because they share an IP address with a previous flagger. It would be quite useful for voting-type situations though. Maybe we could add it as an optional per-flag feature?

Thanks for moving this forwards! :)

> It would be quite useful for voting-type situations though.

I would be inclined to say that if you want voting, use VotingAPI.

I will in a few hours write another patch which stores the flag into the session instead of storing the session id into the flag. It's not just voting situations, it's stuff like "mark as spam" this of course borders voting -- if N people vote as spam then it might be put in a moderation queue and while on some sites this will not work (because brigading) some others just might.

This does not change the stored flags rather stores things in the session. Of course, CSRF checks fail but the policy on https://www.drupal.org/node/2319205 says it's OK not to CSRF check anonymous users and the related core issue #1803712: Allow form tokens to be used on anonymous forms in some cases has seen practically zero progress in four years so I'd rather not wait on it.

Edit: any similarities between FlagServiceProvider and TestServiceProvider are not a coincidence.

Oh look, a modern browser test.

Even better test: now we switch "browsers" to assert another anonymous user is not affected.

1. +++ b/src/Access/CsrfAccessCheck.php
   @@ -0,0 +1,57 @@
   +   */
   +  public function access(Route $route, Request $request, RouteMatchInterface $route_match) {
   +    return $this->account->isAnonymous() ? AccessResult::allowed() : $this->original->access($route, $request, $route_match);
   +  }
   

   I would put a description why we need it. I kinda would have expected to be sort of neutral here.

2. +++ b/tests/Functional/AnonymousFlagTest.php
   @@ -0,0 +1,100 @@
   +    // Warning: $this->getDatabaseConnection() is NOT what you want!
   

   Do you mind explaining why? I'm just curious here

3. +++ b/tests/Functional/AnonymousFlagTest.php
   @@ -0,0 +1,100 @@
   +    $old_mink = $this->mink;
   +    $this->initMink();
   

   Oh wow, why is that even needed? Oh is this the way to have different users? Note: getSession takes a name, not sure how to use it though

Added comments. I can't get mink session switching to work, I will let the core developers add a session switcher to the base class and I will use it when one is available. Here's my code:

    $driver = clone $this->mink->getSession()->getDriver();
    $driver->reset();
    $this->mink->registerSession('new', new Session($driver));
    $this->mink->setDefaultSessionName('new');
    $this->drupalGet(Url::fromRoute('entity.node.canonical', ['node' => $this->node->id()]));
    $this->assertNotEmpty($this->getSession()->getPage()->findLink('switch_this_on'));
    // Switch back to the original.
    $this->mink->setDefaultSessionName('default');

this switches over to the new one just fine but does not switch back properly and I do not know why, posted question on SO. What we have now works and is rather simple.

Nice :)

this switches over to the new one just fine but does not switch back properly and I do not know why

Maybe $this->sessionName doesn't get rebuilt as well (the stuff we send in the cookie)?

We discussed this on IRC and realized that a) sessionName is Drupal's and this is a mink problem b) perhaps the http client is common between the two sessions and if that's so then we are getting to the point where we need to copy initMink into the test except perhaps 2-3 lines and that's simply not worth it.

So: we keep the current code.

> Yikes. Is there no better way to do this?

What is that actually doing?

#27 and #29: not until #2650812: Container::has() doesn't work for service aliases (consequently, decorating services doesn't work) is fixed and even then I am not 100% what a decorator does with tags. The service provider is a workaround around that bug.

#28: as the class doxygen mentions, there is security policy at https://www.drupal.org/node/2319205 about anonymous CSRF. As for "all access to anon users no matter the route" -- no, it just skips the CSRF access checking. I guess we could add a test that it doesn't -- for now, you can easily verify yourself by removing the grantPermission call from setUp of the supplied test, it will bomb. This CSRF checker is a workaround for the core CSRF access checker being broken.

Can't we just register our own custom csrf access check that extends from the default implementation and then we use that. We already have such access checks. We could even make csrf checking part of those?

Hm. I guess the problem with that is generating the CSRF token, that wouldn't happen automatically anymore? But since we control the generation of those links we could just do it ourself too.

Less fancy, but easier to understand in the end I think?

Custom csrf should work otoh how that will work for REST?

Sounds like this needs more thinking about.

At any rate, workarounds needs to be a) documented and b) refer to the core issue they are a workaround for.

> Custom csrf should work otoh how that will work for REST?

REST paths do not call the CSRF checker as far as I am aware?

As I mentioned, even if the alias-decorator bug gets fixed, we'd need the service provider to move the tags. Check DecoratorServicePass in Symfony; it doesn't. Of course if there would be a core issue for fixing or amending the CSRF access checker the service provider and the new access checker could both go away. Definitely the easiest technically but there are other concerns here and so I will not be the one who will file such a core issue.

I am working as fast on the core patch as I can. It should be ready, actually.

I use the patch in #23 for a small site made for fun and it's working pretty well, but as flag is available for anonymous users, we should prevent GoogleBot and friends from "clicking" a lot on the flag link... so, for the record, when this patch will be reworked, we should add $render['#attributes']['rel'][] = 'nofollow'; in src/Plugin/ActionLink/AJAXactionLink.php in the buildLink() function.

No conflicts just automerging..

The nofollow argument (#40) seems a good insight but

before working on this more can someone comment on the state of core changes.

I don't think this patch was ready:

1. +++ b/src/FlagServiceProvider.php
   @@ -0,0 +1,29 @@
   +class FlagServiceProvider implements ServiceModifierInterface {
   

   This class needs documentation. What does it do?

2. +++ b/src/FlagServiceProvider.php
   @@ -0,0 +1,29 @@
   +    for ($id = 'access_check.csrf'; $container->hasAlias($id); $id = (string) $container->getAlias($id));
   

   Could be because it's 6 in the morning, but I can't figure this out.

   Needs a comment on it please.

3. +++ b/tests/Functional/AnonymousFlagTest.php
   @@ -0,0 +1,105 @@
   +namespace Drupal\Tests\yourmodule\Functional;
   

   yourmodule?

Also, #2825993: Fix Views support for anonymous flaggings (Unknown column 'flagging_node_field_data.sid') has come up.

Possibly this is just clean-up. However, this approach of storing flagging IDs in the session does mean that we lose the functionality of being able to filter anonymous flaggings by the current user.

Thanks!

I still don't get the empty for() line... but that's probably moot.

I think we need to revert this patch and rethink this. I don't think storing flaggings in the session is a good idea.

There's the problems with Views, and also #2831393: Unable to unflag() flags for Anon Users when not logged in as a particular Anon User has just come up.

What's currently here isn't viable. And do you really want to have to write a migration script to take flaggings from sessions into the regular table? We can't even access those flaggings to delete them, let alone read from them!

(And by not viable, I mean can't satisfy use cases such as 'show me a list of all the flaggings made to this node', or 'tell me the number of people who have flagged this node'.

Yup, we're in dev hell, and other modules have released alphas and betas and stables.

Part of the problem I think is that Flag touches a lot of things: it sounds like a simple module on the surface, but there's a lot going on: caching, theming, entities, a complex system of configuration and permissions, and so on.

But also, my impression is that those modules went for an incremental approach to the core version upgrade. They changed the bits that touched core and left the rest for later. That's an approach that's been previously seen to work: make one new major version for the core upgrade, and then a second for the internals. It's why Flag 7.x-2.x still only worked with nodes, users, and comments, and it wasn't until Flag 7.x-3.x that Flag became properly entity-aware. It's an approach that's been proven over the years with other projects and other core version changes.

However, with Flag, there was a ground-up rewrite. I said at the time I thought this was a terrible idea, and nobody listened. (I also said it was too soon to start D8 work, as a lot of energy was spent chasing a moving target, and that I wanted to clean up the 7.x-3.x branch first as preparation work, but nobody listened to that either.)

This is the reason we have a ton of regressions -- the work is simply incomplete. When the D8 code landed here, I was shocked at how much was missing. It honestly looks to me like all the initial work on github was done by people who didn't really know all that much about what Flag does beyond the very basic, and certainly hadn't looked at the Flag 3.x-7.x codebase as a starting point. That's how we've ended up with Flag D8 code that doesn't even have theming, that didn't have any kind of access control, that doesn't support anonymous users. Those aren't extras, they're core features. An incremental approach wouldn't have missed those, because they'd have been there and we'd have seen them in the code. So yes, we've been on the wrong path for a very long time.

If we wanted to drop features that we think are too much maintenance weight, then the time to do that would have been 3 years ago before a single line of D8 code was committed. We would have had time there to run a survey over several months, raise awareness on the project page and at Drupal camps and cons, and get an idea of what's used and what's not. As it stands, we're in the dark.

This particular feature shouldn't be hard. You get a session ID from something, you store it in the flagging. Then you use that when retrieving flaggings for an anonymous user. So I don't this this is a feature that it's reasonable to drop.

Sorry if all this is a bit blunt -- I've been surviving on 2-5 hours of sleep a night for the last three and a half weeks with a new baby.

I don't know where we go from here. There's only one alpha blocker left. The beta blocker and release blocker lists are a fair bit longer. I was hoping to make a dent in those while I am on paternity leave, but I am exhausted and just don't have the brain-power or indeed the free time.

> At least, then, can we cut this as a new issue?

Sure. My thinking was to revert this patch, but I guess the CRSF stuff is still fine, it's just the storage that is the problem. So I'll commit your docs patch that's here, and we'll move on to a new issue.

Thanks for the long discussion,

It was interesting to see the differing perspectives.

You get a session ID from something, you store it in the flagging. Then you use that when retrieving flaggings for an anonymous user.

There is now an issue for that ... and I am going to work on it.

Committed the docs patch.

I was reminded here

https://www.drupal.org/node/2832106#comment-11806506

#40 is still a good observation, that should not be dropped on the floor.

As suggested by @joachim I created an issue for the 'nofollow' thing : #2832509: Add 'nofollow' on flag link to prevent search bots from visiting it when anonymous flagging will be functional