Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 5 years ago

#7856 closed task (fixed)

Privacy: Review of cookie behavior

Reported by: boonebgorges's profile boonebgorges Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: Core Keywords: early has-patch
Cc:

Description

Parent ticket: #7698. Related: #7827.

BP uses cookies in a number of ways. Some of them are critical to BP functionality, while others are for convenience only. Some are for logged-in users only, while others apply to all site visitors. Let's use the description of this ticket as an inventory of what we currently do. Please correct any mistakes I make. Afterward, I'll make suggestions about potential changes that would make it easier for sites to comply with GDPR etc.

===

Activity

Name: bp-activity-oldestpage
Description: bp-legacy only. Applies to all site visitors. Used to store the proper page of results for the next 'Load More' request.
Recommendation: There's no reason to store this in a cookie. It's reset between pageloads. We should remove it.

Name: bp-activity-filter
Description: bp-legacy only. Applies to all site visitors. Used to store the last-selected "Filter by"
Recommendation: I have always found this behavior a little useless. My preference is to remove it altogether. If people like it, we should make it logged-in user only, so that cookie approval can be consolidated.

Name: bp-activity-scope
Description: bp-legacy only. Applies to all site visitors. Used to store the last clicked activity tab - "My Groups", "Mentions", etc, and then switch to it on the next load.
Recommendation: see bp-activity-filter

Name: bp-activity-extras
Description: No idea what this does. Seems to have been added in [2477] just for extensibility.
Recommendation: Is it possible it's being used by a plugin? Since it's Legacy only, I'd say leave it, but make it logged-in only.

Messages

Messages has some cookie code for bp_messages_send_to, bp_messages_subject, and bp_messages_content, but it appears to be unused. Probably no action needed here, and no reference necessary in the default privacy policy.

Core

Name: bp-message and bp-message-type
Description: Used to store success/failure messages for template_notices display on next pageload. In practice, it's generally logged-in users only who perform actions that would require this, though third-party plugins might violate this.
Recommendation: Keep.

Groups

Name: bp_new_group_id, bp_completed_create_steps
Description: Used to store progress in the multi-step group creation process. Logged-in user only.
Recommendation: Keep.

Name: bp-groups-filter, bp-groups-scope, bp-groups-extras
Description: See similar activity filters.

Members

Name: bp-members-filter, bp-members-scope, bp-members-extras
Description: See similar activity filters.

Attachments (2)

7856.diff (9.3 KB) - added by boonebgorges 6 years ago.
7856.2.diff (9.7 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (10)

#1 follow-up: @r-a-y
6 years ago

Thanks for taking the time to audit our cookie usage!

As for:

Name: bp-activity-oldestpage
Description: bp-legacy only. Applies to all site visitors. Used to store the proper page of results for the next 'Load More' request.
Recommendation: There's no reason to store this in a cookie. It's reset between pageloads. We should remove it.

We can remove the bp-activity-oldestpage cookie when the page is loaded, but we cannot outright remove it. See #7896.

The other recommendations seem fine to me.

#2 in reply to: ↑ 1 @ryanzz1k28
6 years ago

Replying to r-a-y:
Name: bp-activity-oldestpage

Description: bp-legacy only. Applies to all site visitors. Used to store the proper page of results for the next 'Load More' request.
Recommendation: There's no reason to store this in a cookie. It's reset between pageloads. We should remove it.

We can remove the bp-activity-oldestpage cookie when the page is loaded, but we cannot outright remove it. See #7896.

Could it be made so it’s only for logged-in users, so that cookie approval can be consolidated?

@boonebgorges
6 years ago

#3 @boonebgorges
6 years ago

  • Keywords early has-patch added

7856.diff is a first pass at refactoring the 'bp-x-scope' etc cookies. The basic idea is this:

  • Only set the cookies for logged in users, but this behavior is configurable
  • In addition to the cookies, all users will have their "preferences" stored in a JS variable, so that it'll persist for the pageload
  • When fetching the "preferences" in order to set up the page or send the AJAX request, first try grabbing from the cookie (if cookie storage is enabled) but otherwise pull from the local JS variable

There could be edge cases not yet covered by this, but I think the general idea is correct. Could I get another set of eyes?

@boonebgorges
5 years ago

#4 @boonebgorges
5 years ago

7856.2.diff is an update of 7856.diff that removes Object.assign() (I forgot that it's ES2015+ only). I've reviewed the approach more generally and I think it's a good one, so I'm going to go ahead and commit it. That way, I can apply the same thing to the activity directory filters, and we can have ourselves a pretty significant cookie win for 4.0.

#5 @boonebgorges
5 years ago

In 12255:

bp-legacy: Disable cookie-based directory filter "preferences" for logged-out users.

Logged-out users who refresh the page will see the filters reset to their
default values, rather than the last-used value. Logged-in users continue
to have their preferences saved in a cookie.

This behavior is configurable with the 'bp_legacy_store_filter_settings'
filter.

See #7856.

#6 @boonebgorges
5 years ago

In 12256:

jshint fixes after [12255].

See #7856.

#7 @boonebgorges
5 years ago

In 12257:

bp-legacy: Disable cookie-based filter "preferences" for activity directory when logged out.

Extends the changes in [12255] to the activity directory, which uses
a different technique for directory filter template requests.

See #7856.

#8 @boonebgorges
5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

After the last few commits, plus #7896, all of my recommendations from above have been implemented.

Note: See TracTickets for help on using tickets.