Follow-up to #2884441: Enabling a flag adds session cookies to anonymous users, add warning or option for only authenticated users: as mentioned on that issue and #2865991: provide an API to forcibly start a session:

  • adding any flag using this module adds session cookies to anonymous users.
  • This makes all pages un-cacheable which is of course expected when you want anonymous users to flag things, but if you are intending to use flags only for authenticated users and are not looking to make your entire site un-cacheable this is not useful.

In the original 2884441 case, the issue was closed because it only covered the case of a field on the user object, but the problem remains in FlagService::populateFlaggerDefaults(), which currently always starts a session with dummy content any time flagged content is just displayed, instead of having the data be added to the session only when the anonymous user starts their first flagging interaction.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fgm created an issue. See original summary.

fgm’s picture

Status: Closed (works as designed) » Active
joachim’s picture

Version: 8.x-4.0-alpha2 » 8.x-4.x-dev
Component: User interface » Flag core
Category: Feature request » Bug report
Priority: Normal » Critical

Thanks for filing this.

I'm going to say this is a critical, as breaking caching is pretty awful.

fgm’s picture

Issue summary: View changes
jmarcou’s picture

Critical AF, it took me 4 entire days to find why my caching was all screwed up by a random cookie appearing on some random pages and not others.

It's even more critical as I disabled the ability to Flag Content for Anonymous Users, so the Flag Button is not showing up at all for these users, and still, the Cookie get created for no reason at all !

A first fix should be not to create this cookie when anonymous can't flag, and a warning should be written down somewhere when the module is enable to warn admin users of this massive flaw !

mr.baileys’s picture

Status: Active » Needs review
FileSize
3.24 KB

Also ran into this issue on a page where a flag-link is displayed on a page, but anonymous users do not have flagging-permission. Even though the flag-link is not rendered (as expected), a session is started and the page becomes uncacheable.

Currently, \Drupal\flag\ActionLink\ActionLinkTypeBase::getAsFlagLink first tries to determine the action for the flag/entity (where action is 'flag' or 'unflag'), and then checks permission for that action/user/entity. However, as part of the "getAction" call, the anonymous session is started. It makes more sense to first check permissions, and only then determine action and start the session.

Attached is a patch that fixes this particular scenario.

joachim’s picture

Status: Needs review » Needs work

> Currently, \Drupal\flag\ActionLink\ActionLinkTypeBase::getAsFlagLink first tries to determine the action for the flag/entity (where action is 'flag' or 'unflag'), and then checks permission for that action/user/entity. However, as part of the "getAction" call, the anonymous session is started.

Thank you so much for figuring out the problem!

The patch looks like a decent fix... however, I am concerned that others may fall into the same trap. Our API pretty much leaves it wide open:

$action = $this->getAction($flag, $entity);
$access = $flag->actionAccess($action, $this->currentUser, $entity);

The methods are defined in such a way that clearly you're MEANT to use them like this:

1. determine the action this flag has available
2. determine the access for this action

Except that leads you right back to this bug.

If our API suggests it be used in a way that creates a bug, then it's badly designed.

+++ b/src/ActionLink/ActionLinkTypeBase.php
@@ -95,39 +95,43 @@ abstract class ActionLinkTypeBase extends PluginBase implements ActionLinkTypePl
+    $access['flag'] = $flag->actionAccess('flag', $this->currentUser, $entity);
+    $access['unflag'] = $flag->actionAccess('unflag', $this->currentUser, $entity);
+
+    if ($access['flag']->isAllowed() || $access['unflag']->isAllowed()) {

This fixes it, but it's counter-intuitive to check BOTH types of access first. It's also not very nice DX.

The problem of this bug, simply stated, is when we determine which action applies to the current flag and entity, we start a session so that we can make this check for anonymous users as well as authenticated users.

But this logic misses two important points:

1. We could completely bypass the session business if we first check the flag's permissions: if the anonymous role doesn't have the permission, then there's no need to check anonymous users at all!
2. Furthermore, there's a special case which we could use: if the current user is anonymous, and there is currently no session, then *IT IS IMPOSSIBLE FOR THEM TO UNFLAG!*. If you're anonymous, and you've flagged things already, then you must have a session. If you don't have a session, you have NOT FLAGGED ANYTHING.

However, this logic isn't something we should expect developers to implement. It's Flag's job to do it and wrap it up in something nice.

So, to resolve this, I think we either:

1. Make getAction() smarter, if that's possible
2. Combine getAction() / actionAccess into a single method on the flag, which basically answers the question "can user U use flag F on flaggable entity E?" where 'use' means both flag/unflag, whichever is applicable. Or put more concisely, "should I show a flag link for flag F, user U, on flaggable entity E?"

Evaldas Užkuras’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

The session is created then \Drupal\flag\ActionLink\ActionLinkTypeBase::getAction() is checking if entity is flagged or not.

My solution is to allow to select if `FlagService::populateFlaggerDefaults()` should start new session.
Now we can control if we need a session or not.

Then we are checking if an entity is flagged and an anonymous user does not have the session we simply return FALSE, because if you do not have a session - you have not flagged anything.

One note, I am not sure if I have chosen the right place to check if we have user or session.
Maybe we should move it to `FlaggingStorage::loadIsFlaggedMultiple()` for better reusability.

Evaldas Užkuras’s picture

FileSize
3.69 KB
1.1 KB

I have made some fixes.
Found that sessions were still created then trying to get its id, so I added an extra check if the session is started.
And fixed check if a user is anonymous because this data was populated by `FlagService::populateFlaggerDefaults()` and $account variable becomes not null.

joachim’s picture

Status: Needs review » Needs work
+++ b/src/FlagService.php
@@ -91,7 +91,7 @@ class FlagService implements FlagServiceInterface {
-  public function populateFlaggerDefaults(AccountInterface &$account = NULL, &$session_id = NULL) {
+  public function populateFlaggerDefaults(AccountInterface &$account = NULL, &$session_id = NUL, $create_session = TRUE) {

I really don't like this.

We're taking a fairly clearly-defined helper method and loading extra complexity onto it.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
3.94 KB

If you're anonymous, and you've flagged things already, then you must have a session. If you don't have a session, you have NOT FLAGGED ANYTHING.

Here is an attempt to make getAction() smarter. It does require altering the constructors for ActionLinkTypeBase and AJAXactionLink, since we need to inject the session manager to check whether or not a session is active for an anonymous visitor.

If you don't have a session, you have NOT FLAGGED ANYTHING.

Is there a way to set the "default" state for a flag? If a flag starts out as "flagged", then the above assumption would be incorrect...

martin107’s picture

If you don't have a session, you have NOT FLAGGED ANYTHING.

That seems like a good rule to have in place.

The test failure raises the hair on the back of my neck... where is it ... it may be unrelated.. I am paranoid .. but that section of test code did take awhile to become settled. [ Full sentences ... never!!! ]

So while I am clearing up trivial nits, my actual objective is to see if this test failure is repeatable.

Evaldas Užkuras’s picture

The solution to make the `getAction()` smarter does not solve all issues related to session creation.
With this solution you also solve session creation then action link is displayed.

But what, if you want to check if an entity is flagged and make extra actions with it, f.e. maybe add additional styling or show/hide some content?
And what about listing pages of all flagged entities?
In all these situations the session will be created unless the extra code to check if a session is not created would be added.

martin107’s picture

@Evaldas Užkuras ... thanks for taking the time to post your comments ...

That is a useful thing to read ...

I am going to work on resolving out test instability first. Which in turn means we need to fix this issue [ see attached link ]. Sorry if it just looks like I am throwing up roadblocks.

joachim’s picture

> Is there a way to set the "default" state for a flag? If a flag starts out as "flagged", then the above assumption would be incorrect...

Good point.
There is, but only for global flags, and what it does is create a flag at the same time as you create a flaggable entity. For global flags, we don't need to check for a session anyway to know if something is flagged.

> But what, if you want to check if an entity is flagged and make extra actions with it, f.e. maybe add additional styling or show/hide some content?
> In all these situations the session will be created unless the extra code to check if a session is not created would be added.

I don't really follow. Can you outline the logic the code would take that would create a session?

If there is no session already, then the code should simply assume that something is not flagged, and not start a session.

socketwench’s picture

The ::getAction() method was something I was looking at for #1857300: permission to view a flag's status, but not use it as well. Right now it works because it's only an flag/unflag binary status. If we introduce something like 'view', the method becomes clumsy.

Thinking about it, it also seems we are forcing the end-user to figure out the next status before checking the permission. What if the module took that on itself with a method that does both? ::nextActionAccess()?

MiroslavBanov’s picture

I also have a use case where I should be able to easily defer starting an anonymous session until the anonymous user flags something. I get the gist that there are other use cases to consider, but I think the sensible default should be to not start the session.

MiroslavBanov’s picture

Manually tested #12. It did work, but the link didn't change on clicking it from "flag" to "unflag" version. Reloading the page finally changed it. sessionManager->isStarted() is false until the next request that has the session cookie from the browser. I guess that's how it works in Drupal, because of how the session is "lazy" or whatever.

Adding a patch with a small change that did fix it for me.

Regarding #10 and how populateFlaggerDefaults is a fairly clearly-defined helper method without extra complexity. I do agree that it doesn't need to get more complex, but I think it already is confusing and bad as it is.

MiroslavBanov’s picture

MiroslavBanov’s picture

Strange that locally the tests run just fine with patch #19, and manual review is also as expected. But it's failing here.

I guess we are back to #18.

dieuwe’s picture

The patch for #19 works for me, and that's the only one that will delay session start until a user flags something. The rest don't quite do the job.

Berdir’s picture

Issue tags: +Needs tests

Seems like an important case to have an automated test for, anyone up for trying to write that? Branch tests should no pass again, added a retest.

joachim’s picture

+++ b/src/ActionLink/ActionLinkTypeBase.php
@@ -144,7 +156,14 @@ abstract class ActionLinkTypeBase extends PluginBase implements ActionLinkTypePl
+    if ($this->currentUser->isAnonymous() && !$this->sessionManager->isStarted() && empty($_SESSION['flag'])) {

Nitpick, and off-topic a bit, but is $_SESSION['flag'] starting to pop up all over our code? It was meant to be a workaround for a Symfony bug... I'm wondering if there's a way to keep this DRY.

MiroslavBanov’s picture

Patch #19 doesn't have the nitpick from #23, but looking at the tests took to long and I can't fix it now.

neclimdul’s picture

Tweaking the logic in #19 to see if it passes. Mostly just removing the explicit anonymous user creation and removing the ensureSession if optimization.

Giving it to testbot because like Miroslav I couldn't get 19 to fail locally. Weird testbot environment things are common enough.

Berdir’s picture

Seems surprisingly easy to add test coverage for this, we just need to assert in \Drupal\Tests\flag\Functional\AnonymousFlagTest::testAnonymousFlagging() that just viewing the node doesn't already set a session cookie.

Berdir’s picture

Status: Needs review » Fixed
Issue tags: -Needs tests

Thanks everyone who worked on this. Committed.

  • Berdir committed 08270a1 on 8.x-4.x
    Issue #2894095 by MiroslavBanov, Evaldas Užkuras, Berdir, mr.baileys,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.