Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow author email addresses to stay private #577

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

WPprodigy
Copy link
Contributor

@WPprodigy WPprodigy commented Jan 16, 2020

Fixes #264

Adds a filter that allows for hiding the email address of users in the notifications panel, and removes the user's email from the comment's markup.


I thought about adding additional filters / an action for more customizing of the markup, but ultimately this UI is going to need to be altered and integrated with the block editor in the future, and keeping compatibility with PHP filters/actions at that point will be tough. So I'd rather keep it as slim as possible. Could even just exclude the email address completely in the future UIs (or maybe even now?).

I had a hunch that the user emails would be easy to find in other places in the UI. I checked markup for revisions, the post author dropdown, and even API responses that GB requires. Core will hash the email to get the gravatar image, but it never seems to expose the email address of others (except if you're an admin or it's your own account).

The only other place I could find an email was when somebody left an editorial comment. And after my above findings with how core treats emails, I believe we should just remove the email from there as well. For now though, I've extended the filter to be used in that location as well.

Steps to Test

  1. Check out PR.
  2. View the notifications metabox on a post, or user list in the user groups settings.
  3. Add: add_filter( 'ef_users_select_form_show_email_address', '__return_false' );
  4. Note the email addresses are now hidden in the notifications and comments area.

email-address-missing

CC @jerclarke for thoughts.

I'm also not opposed to removing emails entirely. Especially given how core treats them, and since it means less requirements when moving to a new UI in the block editor. But would need further input from others first.

@WPprodigy WPprodigy force-pushed the fix/264-hide-email-addresses branch from 2317f77 to a8723b8 Compare January 16, 2020 05:19
@cojennin
Copy link
Contributor

Removing the email makes sense but I'd like to substitute the user role in it's place (which would be more helpful anyway). Let's remove the email and substitute it for the role in this PR, then it's good to merge.

@cojennin
Copy link
Contributor

Let's add a filter for this value on the user list as well, in case folks want to substitute for some alternative (like username).

@WPprodigy
Copy link
Contributor Author

WPprodigy commented Jan 16, 2020

Removing the email makes sense but I'd like to substitute the user role in it's place (which would be more helpful anyway). Let's remove the email and substitute it for the role in this PR, then it's good to merge.

To confirm, you're thinking the default behavior should be no email displayed but usernames instead?

If so, I'm definitely not opposed - but do need to note that there is a "feature" removed, in that clicking on a commenters name currently opens up a mailto link to directly email them. Doesn't seem all the needed IMO, but I would need to defer to those using these in real editorial flows.

Secondly, it could look a bit funny if the user has chosen their display name to be the username. That's fixable though.

Let's add a filter for this value on the user list as well, in case folks want to substitute for some alternative (like username).

I'd like to avoid more filters/actions in regards to the markup output. Integrating with the block editor in the future will probably mean a new UI, utilizing Gutenberg styles/components. Keeping compatibility with PHP-level custom markup will be tricky at that point, so ideally we keep things as light as possible moving forward.

@cojennin
Copy link
Contributor

cojennin commented Jan 17, 2020

To confirm, you're thinking the default behavior should be no email displayed but usernames instead?

I was only thinking of the notifications panel. Swap the email with username or role (or both) to help with identification and make it filterable

If so, I'm definitely not opposed - but do need to note that there is a "feature" removed, in that clicking on a commenters name currently opens up a mailto link to directly email them.

Keep it filterable for comments, don't remove it. If you're commenting on a post it's because you've got access to it. As opposed to the notifications panel, where you can find folks emails regardless of whether you should have access to them.

If Beyoncé contributed to my site, everyone who is a contributor or higher can find her email by creating a post and looking her up in the notifications panel. The only folks who could find her email through the editorial metadata are editors or higher, and only if she comments on the post. Editorial metadata needs rethinking, but the notifications panel is more problematic.

@WPprodigy
Copy link
Contributor Author

Keep it filterable for comments, don't remove it. If you're commenting on a post it's because you've got access to it. As opposed to the notifications panel, where you can find folks emails regardless of whether you should have access to them.

True, though I think for the case in #264 the commenters don't necessarily want their email public either.

I see what you're thinking now. Makes sense. I'll try out some variations of username / role and see what will make a good default.

If Beyoncé contributed to my site

One can only hope 😆.

@WPprodigy
Copy link
Contributor Author

WPprodigy commented Jan 21, 2020

Hmm, so the more I look into this, the more it seems we should just leave it empty underneath the user's display name.

  • user_login: Could be sensitive / contain email address still - [email protected]
  • user_nicename: If the above was an email, then this would just be userexample-com, still not good.
  • ID: Not terribly useful.
  • Role: Users can have more than one role. Picking just the first and displaying it could be misleading.

Whereas the display_name, what we are currently displaying, is a customizable value that the user has control over. Defaulting to the First + Last name.

I'm definitely leaning towards "less is more" here. If we start with leaving it empty, we can see what happens. If no reports come in, then all is good. If there is a future report requesting more specific identifiers be shown, then we could act on that.

@cojennin
Copy link
Contributor

cojennin commented Jan 21, 2020

Using just display_name makes it both more difficult to determine "at a glance" who's receiving the notification, and also creates situations where it would be impossible to determine who's receiving a notification (with folks who share first/last names).

Email is the most useful information to show (as a unique identifier to help make "at a glance" and definitive determinations) but problematic from a privacy standpoint (see: Beyoncé). However, positively and absolutely identifying a user will be even more important for cases like #273

The concerns around user_login/user_nicename are legit, but it seems like the next best to use if we want to eliminate email. We can add an apply_filter to the user_login/user_nicename (for folks who may want to bring back email or substitute some alternative identifying information).

@jerclarke
Copy link
Contributor

Sorry I never replied, lol. FWIW I'd be fine with just names, though showing user_nicename would be apprciated as it would always be unique and help you differentiate two seemingly-identical accounts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Privacy/Security: Need ability to hide user emails in Notifications metabox
4 participants