Make WordPress Core

Opened 10 years ago

Closed 8 years ago

#28603 closed enhancement (fixed)

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

Reported by: nevermoor's profile nevermoor Owned by: rachelbaker's profile 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 10 years ago.
28603.2.diff (5.1 KB) - added by voldemortensen 10 years ago.
28603.3.diff (4.1 KB) - added by rachelbaker 10 years ago.
28603.4.diff (4.3 KB) - added by voldemortensen 10 years ago.
28603.5.diff (2.7 KB) - added by rachelbaker 8 years ago.
use get_user_by() instead of reaching parameter madness
28603.6.diff (3.4 KB) - added by rachelbaker 8 years ago.
Same as 28603.5.diff with additional unit test

Download all attachments as: .zip

Change History (22)

#1 @nacin
10 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.

#2 @voldemortensen
10 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.


10 years ago

#4 @voldemortensen
10 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.


10 years ago

@rachelbaker
10 years ago

#6 @rachelbaker
10 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
10 years ago

  • Keywords dev-feedback added

#8 @DrewAPicture
10 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
10 years ago

Added the missing @since in the doc block.

#10 @rachelbaker
10 years ago

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

Version 0, edited 10 years ago by rachelbaker (next)

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


10 years ago

#12 @jorbin
10 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
8 years ago

use get_user_by() instead of reaching parameter madness

#13 @rachelbaker
8 years 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
8 years ago

Same as 28603.5.diff with additional unit test

#14 @rachelbaker
8 years 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.


8 years ago

#16 @rachelbaker
8 years 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.