WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 6 months ago

#28603 closed enhancement (fixed)

Improve "previously approved comment" test for logged-in users

Reported by: nevermoor Owned by: rachelbaker
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.0
Component: Comments Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

In order to restrict spam, my site uses the following settings to restrict the ability to comment (from /wp-admin/options-discussion.php):

1: Other comment settings / Users must be registered and logged in to comment
2: Before a comment appears / Comment author must have a previously approved comment

This generally works as expected, however the second requirement appears to test the user's profile information rather than user ids. This means that whenever a user changes either their nickname or e-mail, their next comment is held for moderation.

In my view, a better implementation would allow comments from logged-in users with previously approved comments immediately after a change to whatever profile information is currently tested.

Apologies if this issue has been discussed before, I certainly couldn't find that discussion.

Attachments (6)

28603.diff (3.1 KB) - added by voldemortensen 2 years ago.
28603.2.diff (5.1 KB) - added by voldemortensen 2 years ago.
28603.3.diff (4.1 KB) - added by rachelbaker 2 years ago.
28603.4.diff (4.3 KB) - added by voldemortensen 2 years ago.
28603.5.diff (2.7 KB) - added by rachelbaker 6 months ago.
use get_user_by() instead of reaching parameter madness
28603.6.diff (3.4 KB) - added by rachelbaker 6 months ago.
Same as 28603.5.diff with additional unit test

Download all attachments as: .zip

Change History (22)

#1 @nacin
3 years ago

  • Component changed from Administration to Comments
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Hi nevermoor, thanks for the report. This does indeed seem like a good change.

@voldemortensen
2 years ago

#2 @voldemortensen
2 years ago

  • Keywords has-patch added; needs-patch removed

If user_id is available it checks if the user has been approved, else it checks the author and email.

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


2 years ago

#4 @voldemortensen
2 years ago

Updated patch to 1) strict check on $user_id, 2) run queries through $wpdb->prepare, and 3) provide unit tests for when a $user_id is passed in.

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


2 years ago

@rachelbaker
2 years ago

#6 @rachelbaker
2 years ago

  • Milestone changed from Future Release to 4.2

The minor change in 28603.3.diff expects $user_id to be an integer, and defaults to 0 (instead of false).

Everything looked good in my testing.

#7 @rachelbaker
2 years ago

  • Keywords dev-feedback added

#8 @DrewAPicture
2 years ago

Some notes on 28603.3.diff:

  • Patch still applies, unit tests pass
  • Adding $user_id here would constitute an 8th (!) parameter for check_comment(). Yikes.
  • If we do end up adding the $user_id parameter, we also need to add a changelog entry to the DocBlock mentioning that the new parameter was added e.g. "@since 4.2.0 The $user_id parameter was introduced."

#9 @voldemortensen
2 years ago

Added the missing @since in the doc block.

#10 @rachelbaker
2 years ago

@DrewAPicture I agree that adding an eighth parameter is not ideal. Do you have a better suggestion on how to approach?

Last edited 2 years ago by rachelbaker (previous) (diff)

This ticket was mentioned in Slack in #core by drew. View the logs.


2 years ago

#12 @jorbin
2 years ago

  • Milestone changed from 4.2 to Future Release

We can do a get_user_by( 'email', $email ) instead of passing in the 8th param.

Also, if the goal is to make it so that users who change there email address have approved comments, we should make sure to add a test for that scenario.

Overall, I think this is close, but not quite there for 4.2. Let's tighten it up for 4.3

@rachelbaker
6 months ago

use get_user_by() instead of reaching parameter madness

#13 @rachelbaker
6 months ago

  • Keywords needs-unit-tests added; dev-feedback removed
  • Milestone changed from Future Release to 4.7

In 28603.5.diff I took @jorbin's advice and used a get_user_by() conditional to avoid adding the evil 8th parameter to check_comment(). I added one unit test, more would be nice.

@rachelbaker
6 months ago

Same as 28603.5.diff with additional unit test

#14 @rachelbaker
6 months ago

  • Keywords has-unit-tests commit added; needs-unit-tests removed

28603.6.diff adds an additional unit test to 28603.5.diff and this looks ready for commit. @jorbin care to sanity check the logic here?

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


6 months ago

#16 @rachelbaker
6 months ago

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

In 38738:

Comments: Improve check for previous comments for authenticated users in check_comment().

When the 'comment_whitelist' option is enabled and the commenter is an authenticated user, query for the existence of an approved comment with a matching user_id. This allows authenticated users that have changed their email address to bypass having their comment held for moderation.

Props voldemortensen, rachelbaker.
Fixes #28603.

Note: See TracTickets for help on using tickets.