#8448 closed enhancement (fixed)
Add new table to store nonmember opt-outs.
Reported by: | dcavins | Owned by: | 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)
Change History (41)
#1
@
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
@
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
@
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
@
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!
@
4 years ago
Screenshot of new wp-admin list table showing opt-out requests. With hashed email addresses.
@
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
@
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
@
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 ?
#7
@
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
@
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.
@
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
@
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
@
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
@
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
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
4 years ago
#13
@
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.
#14
@
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:
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:
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
@
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!
@
4 years ago
Combination of .3.patch with imath's additional patches plus correct behavior in network admin.
#16
@
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:
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:
Otherwise, patch looks ready to be committed 👍
#19
@
4 years ago
- Owner set to dcavins
- Resolution set to fixed
- Status changed from new to closed
In 12899:
@
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
@
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
@
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
@
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
@
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!
Adds new db table, wp-admin list table and a few convenience functions to manage BP_Optouts