Problem:
The postSave method in Subscriber.php line 273 saves the user entity received bei $this->getUser().
This user entity could be the anonymous user with ID 0.
When the user 0 is saved the cache tags user:0 and user_list are invalidated and can lead to unexpected caching behaviors.

Solution:
The $user->save() call in Line 282 should be secured by validating if the user ID is > 0.

CommentFileSizeAuthor
#7 simplenews-prevent-anon-user-syncing-2937372-7.patch1.06 KBsubson
#5 simplenews-prevent-user-0-syncing-2937372-1.patch990 bytesdtv_rb

Comments

dtv_rb created an issue. See original summary.

dtv_rb’s picture

dtv_rb commented
Issue summary: View changes
disclose’s picture

disclose commented

Yes, we have the same issue on our page. When using Simple News subscription functions from e.g. webhooks to add new subscribers it leads to an invalidation of cache_tags related to the Anonymous user (which in some cases invalidates nearly all pages).

Needs urgent attention.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Patches welcome :)

dtv_rb’s picture

dtv_rb commented
StatusFileSize
new simplenews-prevent-user-0-syncing-2937372-1.patch990 bytes

Here is a first patch.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Active » Needs work 
  1. +++ b/src/Entity/Subscriber.php
@@ -275,12 +275,15 @@ class Subscriber extends ContentEntityBase implements SubscriberInterface {
+      if ($user->id() > 0) {

    I think it is prefered to use $user->isAuthenticated() instead of comparing the id. You could also roll the condition into the if-statement above. 

  2. +++ b/src/Entity/Subscriber.php
@@ -275,12 +275,15 @@ class Subscriber extends ContentEntityBase implements SubscriberInterface {
+        foreach ($this->getUserSharedFields($user) as $field_name) {
+          $user->set($field_name, $this->get($field_name)->getValue());
+        }

    While we are here, we could skip $user->save() if there are no shared fields (and thus no updates), and this not invalidating the cache for that user unless required?

subson’s picture

subson commented
Status: Needs work » Needs review
StatusFileSize
new simplenews-prevent-anon-user-syncing-2937372-7.patch1.06 KB

Updating patch to use $user->isAuthenticated() and added condition for count($this->getUserSharedFields($user)) > 0 to run user->save() only if there are sharedfields.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Makes sense, can we test this?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
Issue tags: +Need tests

I assume we are triggering this in our tests already when we update subscribers. search such case, assert that the anonymous user wasn't updated.

Also I think there is a better fix for it, we should instead check that we have a non-zero user ID in \Drupal\simplenews\Entity\Subscriber::getUser().

adamps’s picture

adamps commented
Priority: Major » Normal
Related issues: +#3049348: Problems with "Synchronize between account and subscriber fields"

There is an easy workaround to disable subscriber.sync_fields.

I'm unsure whether that whole feature is desirable/workable or should be removed in next major release #3049348: Problems with "Synchronize between account and subscriber fields"

adamps’s picture

adamps commented
Version: 8.x-1.x-dev » 8.x-2.x-dev
Status: Needs work » Fixed
Issue tags: -Need tests
Related issues: +#3062530: Fix and simplify subscriber creation/lookup/sync

This was fixed by #3062530: Fix and simplify subscriber creation/lookup/sync patch #13 which included the improvement from comment #9.

Status: Fixed » Closed (fixed)

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