WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#36546 closed defect (bug) (fixed)

user marked as spam can log in

Reported by: websupporter Owned by: jeremyfelt
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.7
Component: Login and Registration Keywords: has-patch has-unit-tests
Focuses: multisite Cc:

Description

When the admin marks a user as "spam" the function wp_authenticate_spam_check() is supposed to block this user from logging in.

wp_authenticate_spam_check() utilizes is_user_spammy() to do so. This function expects the WP User Object. If it is not given, it will fall back to the currently logged in user, but - if I am not mistaken - the user is not logged in yet, so the fallback does not work.

Since we have the user object when performing the is_user_spammy-test, we can simply hand it over and the user can't login no more.


P.S.: Its the first time, I try to patch something, I hope I get everything right.

Attachments (3)

user.diff (654 bytes) - added by websupporter 3 years ago.
36546.02.patch (1.7 KB) - added by websupporter 3 years ago.
Changed the error message to a more generic Your account has been disabled and merged it together with #24617 (dont know, if this is a good practice, but since they are quite related)
36546.03.patch (790 bytes) - added by websupporter 3 years ago.
This is the patch without the "merge".

Download all attachments as: .zip

Change History (14)

@websupporter
3 years ago

#1 @websupporter
3 years ago

  • Keywords has-patch added

#2 @swissspidy
3 years ago

  • Component changed from Networks and Sites to Login and Registration
  • Keywords needs-unit-tests added
  • Version changed from trunk to 3.7

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


3 years ago

#4 @websupporter
3 years ago

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

#5 @swissspidy
3 years ago

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

#6 @websupporter
3 years ago

I had a second look into the wp_authenticate_spam_check() and was wondering. I found this problem, when I tried to patch #24617. We discussed over there, if we should let the user know, he is marked as spam:

Let's change the error text to something more generic so that we're not necessarily passing that info on to the user. ticket:24617#comment:10

We decided to go with a standard message instead. I was now wondering about the message, returned by wp_authenticate_spam_check():

Your account has been marked as a spammer

Maybe, we should use also here something more generic to keep it consistent. We could keep the error code (in regards to #19445), but return to the user something like "Your account has been disabled"?

Any thoughts here?

This ticket was mentioned in Slack in #core-multisite by websupporter. View the logs.


3 years ago

#8 @jeremyfelt
3 years ago

  • Milestone changed from Awaiting Review to 4.6

@websupporter
3 years ago

Changed the error message to a more generic Your account has been disabled and merged it together with #24617 (dont know, if this is a good practice, but since they are quite related)

@websupporter
3 years ago

This is the patch without the "merge".

This ticket was mentioned in Slack in #core-multisite by websupporter. View the logs.


3 years ago

#10 @jeremyfelt
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Owner set to jeremyfelt
  • Status changed from new to reviewing

Thanks @websupporter! Great catch on the initial bug and great work on the patch.

I'm going to leave the error message as is and go with your original patch. I've also added some tests for coverage of wp_authenticate_spam_check.

#11 @jeremyfelt
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 37316:

Users: Provide a full user object when checking for a spammy multisite user

is_user_spammy() falls back to the current user if one is not provided. There is no current user during authentication, so the result is always false. Pass a user to fill the void.

Adds tests for wp_authenticate_spam_check().

Props websupporter.
Fixes #36546.

Note: See TracTickets for help on using tickets.