Activity log: add user-login to failed login attempts by simison · Pull Request #8817 · Automattic/jetpack

simison · 2018-02-12T15:51:43Z

An activity for failed login attempt should look like:

[user_login] had a failed login attempt from IP Address: 123.123.123.123

But it currently looks like :

This adds user login we can then show in Calypso.

Send user_login (username or email) used in a failed login attempt.
Removes IP from args since it's already being sent in author object.

Backwards compatibility?

Should I be worrying about backwards compatibility for jpp_log_failed_attempt action?

In the new version, it'll send an array containing login_user instead of a string containing IP.

Before:

jetpack/modules/protect.php

Lines 272 to 281 in 1db8bc3

    
           		/** 
        
           		 * Fires before every failed login attempt. 
        
           		 * 
        
           		 * @module protect 
        
           		 * 
        
           		 * @since 3.4.0 
        
           		 * 
        
           		 * @param string jetpack_protect_get_ip IP stored by Protect. 
        
           		 */ 
        
           		do_action( 'jpp_log_failed_attempt', jetpack_protect_get_ip() );

After:

jetpack/modules/protect.php

Lines 273 to 285 in ee0f162

    
           		/** 
        
           		 * Fires before every failed login attempt. 
        
           		 * 
        
           		 * @module protect 
        
           		 * 
        
           		 * @since 3.4.0 
        
           		 * 
        
           		 * @param array Information about failed login attempt 
        
           		 *   [ 
        
           		 *     'user_login' => (string) Username or email used in failed login attempt 
        
           		 *   ] 
        
           		 */ 
        
           		do_action( 'jpp_log_failed_attempt', array( 'login_user' => $login_user ) );

Changelog entry

We improved the way we show failed login attempts in Jetpack's Activity Log.

roccotripaldi

This looks good, and I've tested fully and it works as expected. Nice work!

I left one minor comment. Once that is addressed, and we can get the unit tests passing, I'll approve!

roccotripaldi · 2018-02-13T16:03:24Z

modules/protect.php

 		 * @since 3.4.0
 		 *
-		 * @param string jetpack_protect_get_ip IP stored by Protect.
+		 * @param string IP


Let's use the same description from before

Tests for: - generated user - empty login_user (should return empty string)

(Breaks backwards compatibility)

roccotripaldi

LGTM!

simison · 2018-02-19T21:57:46Z

Noting that I got okay from @lezama regarding breaking backwards compatibility of jpp_log_failed_attempt action — it isn't used anywhere else at Jetpack and we account for the change at wpcom already.

lezama

🚢 IT

* update changelog.txt * Update readme.txt with scaffolding for 5.9 changelog and release draft shortlink * Add changelog entry for #8243 * Add changelog entry for #8296 * Add changelog entry for #8367 * Add changelog entry for #8686 * Add changelog entry for #8707 * Add changelog entry for #8709 and #8714 * Add changelog entry for #8729 * Add changelog entry for #8777 * Add changelog entry for #8780 * Add changelog entry for #8786 * Add changelog entry for #8787 * Add changelog entry for #8801 #8805 #8832 #8865 and #8804 * Add changelog entry for #8817 * Add changelog entry for #8822 * Add changelog entry for #8823 * Add changelog entry for #8829 * Add changelog entry for #8834 * move some items to major enhancements * Add changelog entry for #8836 * Add changelog entry for #8839 * Add changelog entry for #8861 * Add changelog entry for #8862 * Add changelog entry for #8863 * Add changelog entry for #8866 * Add changelog entry for #8870 * Add changelog entry for #8874 * Add changelog entry for #8875 * Add changelog entry for #8881 * Add changelog entry for #8890 * Add changelog entry for #8911 * Add changelog entry for #8927 * Add changelog entry for #8931 * Add changelog entry for #8933 * Add changelog entry for #8930 * fix wording * typo * minor fixes * replace partner scripts for Jetpack Start in changelog entry * Update to-test.md * Update to-test.md * minor style fixes to to-test.md * minor style fixes to to-test.md * minor fixes on to-test.md * Add changelog entry for #8868 * Add changelog entry for #8844 * Add changelog entry for #8664 * Add changelog entry for #8935 * Add changelog entry for #8425 * Add changelog entry for #8625

WIP

750e4a5

simison self-assigned this Feb 12, 2018

simison requested a review from roccotripaldi February 12, 2018 15:51

simison requested a review from a team as a code owner February 12, 2018 15:51

jeherve added [Feature] Protect Also known as Brute Force Attack Protection [Status] In Progress [Feature] Activity Log labels Feb 12, 2018

simison added 4 commits February 13, 2018 14:07

Drop IP from args

6c343ea

Simplify action param

6996f5b

Update docBlock

ee0f162

Make action backwards comp

2d1f7da

roccotripaldi reviewed Feb 13, 2018

View reviewed changes

simison added 4 commits February 14, 2018 12:41

Update doc

8ebf9e2

Fix test

2009932

Tests for: - generated user - empty login_user (should return empty string)

Remove IP param from jpp_log_failed_attempt action

ccb07fc

(Breaks backwards compatibility)

Rename login_user arg to login

d22d16b

roccotripaldi approved these changes Feb 19, 2018

View reviewed changes

lezama approved these changes Feb 20, 2018

View reviewed changes

simison merged commit b8954ce into master Feb 20, 2018

matticbot added the [Status] Needs Changelog label Feb 20, 2018

simison deleted the update/activity-failed-login-user branch February 20, 2018 14:14

oskosk removed the [Status] In Progress label Feb 27, 2018

oskosk added this to the 5.9 milestone Feb 27, 2018

oskosk added [Status] Has Changelog and removed [Status] Needs Changelog labels Feb 27, 2018

oskosk added a commit that referenced this pull request Feb 27, 2018

Add changelog entry for #8817

4327182

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Activity log: add user-login to failed login attempts#8817

Activity log: add user-login to failed login attempts#8817
simison merged 9 commits intomasterfrom
update/activity-failed-login-user

simison commented Feb 12, 2018 •

edited by oskosk

Loading

Uh oh!

roccotripaldi left a comment

Uh oh!

roccotripaldi Feb 13, 2018

Uh oh!

roccotripaldi left a comment

Uh oh!

simison commented Feb 19, 2018

Uh oh!

lezama left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	/**
	* Fires before every failed login attempt.
	*
	* @module protect
	*
	* @since 3.4.0
	*
	* @param string jetpack_protect_get_ip IP stored by Protect.
	*/
	do_action( 'jpp_log_failed_attempt', jetpack_protect_get_ip() );

Conversation

simison commented Feb 12, 2018 • edited by oskosk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backwards compatibility?

Changelog entry

Uh oh!

roccotripaldi left a comment

Choose a reason for hiding this comment

Uh oh!

roccotripaldi Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

roccotripaldi left a comment

Choose a reason for hiding this comment

Uh oh!

simison commented Feb 19, 2018

Uh oh!

lezama left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

simison commented Feb 12, 2018 •

edited by oskosk

Loading