Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#8448 closed enhancement (fixed)

Add new table to store nonmember opt-outs.

Reported by: dcavins's profile dcavins Owned by: dcavins's profile dcavins
Milestone: 8.0.0 Priority: normal
Severity: normal Version: 7.2.0
Component: Core Keywords: has-patch commit
Cc:

Description

As a prerequisite for site invitations, we will need a way to store opt-out requests from nonmembers. Our current unsubscribe setup attaches preferences to existing users as meta (which is great). The attached patch adds a simple opt-outs database table and WP-admin list table to show opt-outs. In the list table, I've added columns that would help a site admin figure out if a type of email or a particular user is abusing the system, resulting in a lot of opt-out requests. Also included are a few simple convenience functions to add, get and delete the new BP_Optout items.

@boonebgorges, I know you've dealt with opt-outs as part of Invite Anyone. If you have a chance to weigh in on this, please let me know if I've missed anything or should be doing something in a different way. Thanks!

Attachments (13)

8448.1.diff (57.4 KB) - added by dcavins 4 years ago.
Adds new db table, wp-admin list table and a few convenience functions to manage BP_Optouts
opt-outs-new-db-table.png (67.5 KB) - added by dcavins 4 years ago.
Screenshot of new, simple database table
opt-outs-list-table.png (86.9 KB) - added by dcavins 4 years ago.
Screenshot of new wp-admin list table showing opt-out requests.
opt-outs-list-table-2.png (88.5 KB) - added by dcavins 4 years ago.
Screenshot of new wp-admin list table showing opt-out requests. With hashed email addresses.
opt-outs-list-table-2-search.png (78.1 KB) - added by dcavins 4 years ago.
Searching for the complete email address finds the right record, with a message showing what you searched for in the header.
8448.2.diff (58.0 KB) - added by dcavins 4 years ago.
Adds new db table, wp-admin list table and a few convenience functions to manage BP_Optouts. Stores hashed email addresses.
8448.3.diff (59.2 KB) - added by dcavins 4 years ago.
Move admin screen to submenu of "Tools
8448.3.patch (59.1 KB) - added by imath 4 years ago.
8448.3-advices.patch (20.8 KB) - added by imath 4 years ago.
8448.3-tools-tabs.patch (11.6 KB) - added by imath 4 years ago.
8448.4.patch (68.1 KB) - added by dcavins 4 years ago.
Combination of .3.patch with imath's additional patches plus correct behavior in network admin.
8448.lowercase-emails.patch (4.4 KB) - added by dcavins 4 years ago.
When adding, updating or fetching opt-outs, be sure that the email address string has been converted to lowercase, or else the comparisons of the hashed values will fail.
8448.lowercase-emails.2.patch (4.6 KB) - added by imath 4 years ago.

Download all attachments as: .zip

Change History (41)

@dcavins
4 years ago

Adds new db table, wp-admin list table and a few convenience functions to manage BP_Optouts

@dcavins
4 years ago

Screenshot of new, simple database table

@dcavins
4 years ago

Screenshot of new wp-admin list table showing opt-out requests.

#1 @imath
4 years ago

Thanks for your work on this @dcavins 👍.

I agree it's important to avoid sending emails to people who don't want to receive them. I had to build something similar for a BP plugin and thought due to GDPR concerns we shouldn't store raw emails but encode them like passwords as these people won't have the opportunity to accept the privacy policy before some of their personal information has been saved/used (the email). I might be overthinking it though!

It's a detail, but it seems weird to have a non members sub menu to the WP Admin Users menu. I'd probably use the tools menu instead 😁

#2 @boonebgorges
4 years ago

Thanks, @dcavins ! General approach looks good.

It looks like 'order_by' is not properly sanitized against SQLi. We either need to check against a list of allowed values (column names should make this fairly easy) or run through a SQL-escaping function.

For the record, Invite Anyone uses a different approach. Each row in the invitation table has an 'invitee_email' column and an 'opt_out' column. When someone opts out of notifications, *all* rows matching the email in question are flagged as 'opt_out=1'. Then, when deciding whether to send future notifications, IA checks to see whether there are any rows in the invitations table WHERE invitee_email = %s and opt_out = 1. One fewer database table, but it also makes the tables and the logic a bit more confusing. So your approach looks good.

Regarding @imath's suggestion: if the emails are encoded, it's no longer useful to have an admin panel. And this means there's no obvious way to "undo" or delete an opt-out, at least not as pictured. As I think about it, it seems that GDPR concerns, and a more general gut feeling about privacy, suggest that we might not the kind of admin panel you've built. Users do not choose to receive invitations, and if a recipient chooses to opt out of future notifications, it's reasonable to assume that they also don't want the site admin to be able to see their email address.

We would then need a different kind of "undo" or "delete" mechanism. Something like: you manually enter an email address, and let the system hash it and compare against stored values, just like we do with passwords. @imath Did you build anything similar for Réception?

#3 @imath
4 years ago

Hi @boonebgorges & @dcavins sorry for this late reply. In Réception the need was to build an email verification mechanism to avoid people to spam members. The visitor needs to ask for an email verification before being able to contact the member. The admin is comparing hash according to a field input yes. I believe in the invitation case we could build a a mechanism hashing the email before checking into the db table if the invited user don’t want to receive invites. Into the invitation email, we could include a link to prevent new invitations using the email hash into the link. The admin part could use an input / hash / search hash mechanism to delete it or add it.

Due to user privacy concerns I think it’s an important point to check with a law specialist to avoid us to make a mistake. I wonder how share by email services are working : do they store emails ? Is a mailto: link less risky ?

#4 @dcavins
4 years ago

Thanks to you both for your feedback.

I also wondered about the data privacy issue of storing emails. As I understand it, imath's plugin stores a password hash of the email, then when doing a "match by email" query or a "search" query, the search term is hashed for comparison, like WP handles passwords. Of course, this means that "search" is not a substring match--you'd have to input the full email address to find the matching entry.

I do still think there's some utility to having an admin panel, if only to be able to check opt-outs to see if a user is abusing the ability to send emails from your site (this seems to be a common goal of hacks right now), or if emails from a particular component is producing a lot of opt-out requests, then the admin can do something about it.

I'll attached a new draft (that still needs some thought about where to hash the email in the logic for the best consistency), but gives an idea of how this could work. Thanks again!

@dcavins
4 years ago

Screenshot of new wp-admin list table showing opt-out requests. With hashed email addresses.

@dcavins
4 years ago

Searching for the complete email address finds the right record, with a message showing what you searched for in the header.

#5 @boonebgorges
4 years ago

Thanks for thinking about this so thoroughly, @dcavins. Your screenshots convince me that the admin panel is still useful as a list table. I'm not sure whether showing the email hash is ever useful, but I don't see any harm in it either.

#6 @imath
4 years ago

Looks great @dcavins 👌.

I’d suggest to rename the search button or add some more explanations about what needs to be into the search input.

Also, the more I see the title of the page the more I believe it should be into another Admin Menu like tools. This is probably due to the move French translators made about the « Users » menu (it’s now Account). I’m wondering the same for signups actually. Are Non members or signups users of the site ?

Last edited 4 years ago by imath (previous) (diff)

#7 @shawfactor
4 years ago

I think this is totally the wrong say to go.

Better to get rid of the sign ups table entirely and create an account immediately in all cases (with some sort of inactive flag). From there it is easy enough to classify some accounts as opted out so they don’t get invitations etc

This is the approach I take in my buddypress based SAAS.

No biggie in the scheme of things but buddypress already has far too many tables. This seems like one more unnecessary thing to support.

#8 @dcavins
4 years ago

Well, I'm not opposed to storing this information in an existing table. However, I think we agree that we shouldn't be storing personal information for users who wish to opt-out of communications from the site. So the hashing creates a special case that would be hard to handle in the signups table (assuming members have a plaintext email and opt-outs are hashed).

I used to be averse to special-purpose tables in WP, too, but I've definitely come around to the view that custom tables can be slim and simpler than stretching post types & meta or similar.

@dcavins
4 years ago

Adds new db table, wp-admin list table and a few convenience functions to manage BP_Optouts. Stores hashed email addresses.

#9 @shawfactor
4 years ago

I am not sure there is a problem.

Email is publicly available information and from memory wordpress core already creates an account immediately on registration (before valudating that the users intention).

So if there is a problem to be solved it is with core, not buddypress.

In terms of your broader point you are mostly wrong. I'm going to write an article on this at some stage, but for now I'll summarise the key point. Extra db tables are obviously better from a technical and performance point of vew. But wordpress (and buddypress) is an ecosystem. The value is mostly in the plugins. Harmonising on some simple db tables, apis, and standards makes this ecosystem so much more healthy than everyone going their own way. Obviously there is a balancing act here but buddypress is already too far down they custom table route IMO.

#10 @imath
4 years ago

@shawfactor I’m almost 100% sure we need to be very careful about unsollicited emails, because unlike WordPress or BuddyPress signup process the user has not consent his email to be used. An email is personal information : the least we can do is make them unusable by encrypting them.

We only create tables when absolutely needed. @dcavins is doing the right choice and has made great progress (thanks a lot 😍).

It’s important we make sure to follow good practices / comply with GDPR or other personal data related rules for all our potential users.

That’s what we did during the 4.0.0 milestone adding the consent checkbox and adding examples of privacy policy chapters. I’ll always encourage us to carry on this way 😊.

#11 @boonebgorges
4 years ago

Email is publicly available information and from memory wordpress core already creates an account immediately on registration (before valudating that the users intention).

"Registration" involves a user submitting their own email address, which constitutes consent. "Invitation" involves one user submitting someone else's email address, and is a BuddyPress-specific feature. And email addresses definitely are *not* public information. Perhaps you're thinking of user_nicename and user_login, which the WP project has generally considered public: https://core.trac.wordpress.org/ticket/20235#comment:7

@dcavins
4 years ago

Move admin screen to submenu of "Tools

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


4 years ago

#13 @shawfactor
4 years ago

@imath @boonebgorges you both miss the point. There is nothing stopping someone other than the person who owns the email address from registering in wordpress. The account is created immediately, it may never be activated but it exists and an activation email has been sent, and that email is potentially unsolicited.

I really don't think this is a problem, and if there is it exists in core. If there is not we should do it the wordpress way, just create an account immediately. And by all means flag them as opting out in some way if required.

@imath
4 years ago

#14 @imath
4 years ago

Hi @dcavins

I just gave a closer look to your patch and I have some more suggestions and advices. I know I could have asked you to "--no-prefix" your patch but as I knew I'd suggest you some possible improvements I've thought it was faster to do it myself: that's the purpose of 8448.3.patch.

First, thanks again for iterating on the patch in a great way like you did. I've tested it and I've "phpunit" tested it. And it works as expected.

Code Formatting / escaping improvements

In 8448.3-advices.patch you'll find some multiline functions, array parameters & equal signs alignment advices (things we could potentially make more automatic using WPCS if #7228 were fixed 😉). There's also some recommanded changes about escaping some outputs and about using the _n() to output the search information at the top of the Nonmember Opt-outs tool's page.

Keeping 1 and 1 only BuddyPress tools submenu

I believe just like we are doing for settings, we should use tabs to organize our tools pages, now we have more than one. So 8448.3-tools-tabs.patch is a first attempt (you'll need to improve if you agree with my belief) to move this way.

The first thing I've added is a specific section for the Nonmember opt-outs tool into the WP Available tools Admin Screen. Here's a screenshot below:

https://cldup.com/ljnAprrzHh.png
PS: you'll need to improve the text of the card ;)

Then I've made bp_core_admin_tabs() and bp_core_get_admin_tabs() usable for the 'tools' context, so that this area looks like this:

https://cldup.com/wz173xjx-B.png

Looking into this I've noticed you were using network_admin_url() in a way that might not generate the URL you expect it. I've tested it dumping network_admin_url( 'network-tools' ); and it output like a subdirectory url eg: site.url/wp-admin/network-tools/ which does not exist. I remember we had to create the Tools menu and available tools / BP Tools submenus on multisite admin but I believe the main admin page is admin.php?page=$slug where slug is the slug of your tool's page. I'd advice you to test this part on a Multisite config to check everything works as expected.

Thanks again for your work, it's shaping great 💪

#15 @dcavins
4 years ago

Thanks for your feedback, @imath. I really like your idea of assembling the "tools" screens in one location. I've assembled an uber patch from your work and a few further updates to be sure that the screen works in a single-site admin context and in a network admin context. You were correct that admin.php is the "parent url" in the network situation; network-tools is the "parent menu" for the submenu addition.

Please let me know if you have any other comments. Thanks!

@dcavins
4 years ago

Combination of .3.patch with imath's additional patches plus correct behavior in network admin.

#16 @imath
4 years ago

  • Keywords commit added; dev-feedback removed

Hi @dcavins

It looks good! I've tested it and it behaves as expected. One last advice before you commit, I just noticed the link on the hash was a mailto:, we should simply remove it. See screenshot below:

https://cldup.com/r5PLvikXtA.png

One @todo task for when you'll work on #8139 : replace the No opt-outs found. mention of the WP List Table by an information with a link to head to the settings in case they don't allow Network invites. Something like what we're doing for signups. See screenshot below:

https://cldup.com/tpvsFuI6Vq.png

Otherwise, patch looks ready to be committed 👍

#17 @dcavins
4 years ago

In 12897:

BP Admin Tools: Add tab interface to support additional screens.

Expand the BP Tools screen to use a tabbed interface
so that multiple tools screens can be grouped in a
single Tools > BuddyPress menu item.

Props imath.

See #8448.

#18 @dcavins
4 years ago

In 12898:

Introduce BP_Optouts.

Add capability to store opt-out requests from
nonmembers who have been contacted by
communication from a BuddyPress site. These
new objects are represented by the class BP_Optout
and are stored in a new database table
wp_bp_optouts. This commit adds the following capabilities:

  • Add new class BP_Optout.
  • Add new table wp_bp_optouts.
  • Create new table on installation or upgrade.
  • Add convenience functions for adding, fetching or

deleting opt-outs.

  • Add tests for basic opt-out management.

See #8448.

#19 @dcavins
4 years ago

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

In 12899:

BP_Optouts: Add WP Admin management screen.

Add a management screen for opt-out requests at the
single-site or network level. This screen is appended
to the BuddyPress Tools screen. Using this screen,
site admins can view opt-out requests, search for
opt-outs by specific email address, and monitor
opt-out requests for signs of abuse by a particular
email type or potentially malicious user.

Fixes #8448.

#20 @imath
4 years ago

In 12900:

Administration: improve indentation in common styles file

See #8448

@dcavins
4 years ago

When adding, updating or fetching opt-outs, be sure that the email address string has been converted to lowercase, or else the comparisons of the hashed values will fail.

#21 @dcavins
4 years ago

@imath, please take a quick look at the additional patch. When using opt-outs, I realized that the email addresses need to be always lowercased for comparison purposes (and case is irrelevant in email addresses). Thanks!

#22 @imath
4 years ago

@dcavins I’ve quickly looked at it but I need to see it applied as I would expect the email address to be « lowercased » before the hash and I don’t see if that’s the case in the patch 😉

#23 @dcavins
4 years ago

Thanks @imath. I'm using strtolower on the incoming string in the insert() and update() methods, and also in the get() method. Thanks for taking a look!

#24 @imath
4 years ago

Ok it looks good, sorry the $data['email_address_hash'] was confusing to me. In the above patch I've defined a intermediate $email = strtolower( $data['email_address_hash'] ) so that we have wp_hash( $email ) which seems less confusing to me.

But you can leave it the way it is, it's probably just me 😉.

Let's commit!

#25 @dcavins
4 years ago

In 12903:

BP_Optout: Ensure email_address is always lowercase.

When adding, updating, or fetching opt-outs,
be sure that the email address string has
been converted to lowercase, else the comparisons
of the hashed values will fail.

See #8448.

#26 @dcavins
4 years ago

In 12911:

BP_Optouts: Improve localization.

Improve localization and translation instructions.

Props imath.

See #8448.

#27 @dcavins
4 years ago

In 12912:

BP_Optouts: Improve unsubscribe behavior.

Improve unsubscribe link behavior when user is not
a site member, meaning that the unsubscribe is an
opt-out request.

  • Introduce bp_user_has_opted_out() as convenience

function for checking an email address's status

  • Build bp_email_get_unsubscribe_link() for

non-member case.

  • bp_email_unsubscribe_handler() handles

non-member case and uses wp_die() screen
to display message.

  • Fix unsusbscribe headers for opt-out links.

Props imath.

See #8448.

#28 @dcavins
4 years ago

In 12913:

BP_Optout: Prevent bp_send_email() delivery to opted-out address.

In BP_Email::validate() ensure that the recipient has
not opted out from communications from this site.

See #8448.

Note: See TracTickets for help on using tickets.