Add user last seen information #4765
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4765 +/- ##
=============================================
+ Coverage 29.52% 29.69% +0.16%
- Complexity 4780 4863 +83
=============================================
Files 282 283 +1
Lines 20577 20795 +218
=============================================
+ Hits 6076 6175 +99
- Misses 14501 14620 +119 ☔ View full report in Codecov by Sentry. |
luismulinari
force-pushed
the
add/users-last-seen
branch
from
9af0556
to
e122725
Compare
|
Thanks for such a thoughtful review @WPprodigy ❤️ |
Co-authored-by: Mohammad Jangda <mo@automattic.com>
Co-authored-by: Mohammad Jangda <mo@automattic.com>
luismulinari
force-pushed
the
add/users-last-seen
branch
2 times, most recently
from
48b1594
to
31cfcdc
Compare
luismulinari
force-pushed
the
add/users-last-seen
branch
from
31cfcdc
to
26bdd27
Compare
security/class-user-last-seen.php
Outdated
|
|
||
| add_action( 'admin_init', array( $this, 'register_release_date' ) ); | ||
| add_action( 'set_user_role', array( $this, 'user_promoted' ) ); | ||
| add_action( 'vip_support_user_added', function( $user_id ) { |
There was a problem hiding this comment.
Since we already have an existing job that clears out the vip_support users (
| return; | ||
| } | ||
|
|
||
| $this->release_date = get_option( self::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY ); |
There was a problem hiding this comment.
Are we using this property anywhere or just directly checking the option?
There was a problem hiding this comment.
The options main purpose is for: https://github.com/Automattic/vip-go-mu-plugins/pull/4765/files#diff-d6cd56ee4fe98abf6b5d384fc94675502abd9ad6a5d412954a4ccccc35cb2bb4R311-R314
But I agree, let's just call get_option() when it's needed. There is runtime caching in place for it, and validation is handled internally already. So no need for the class property to maintain it's own state. As-is, the current version will run this DB query/memcache get on every request rather unnecessarily.
| $error = __( 'User not found.', 'wpvip' ); | ||
| } | ||
|
|
||
| if ( ! current_user_can( 'edit_user', $user_id ) ) { |
There was a problem hiding this comment.
Minor, but can we move this permissions check up earlier?
…is_application_passwords_available_for_user filter to block REST requests with application passwords
security/class-user-last-seen.php
Outdated
| * @return bool | ||
| */ | ||
| public function application_password_authentication( $available, $user ) { | ||
| global $wp_last_seen_application_password_error; |
There was a problem hiding this comment.
Let's use a class property rather than a global
There was a problem hiding this comment.
Good call. Done: 182dc0d
luismulinari
force-pushed
the
add/users-last-seen
branch
from
4b4120b
to
fce37b0
Compare
luismulinari
force-pushed
the
add/users-last-seen
branch
from
fce37b0
to
182dc0d
Compare
|
I have a higher level thought on this ... can we reduce the instances of VIP-specific naming inside the class (meta/option keys, filter names) and make it more generic? This functionality is something that has wider appeal outside of VIP and by removing VIP references, re create an easier path to open sourcing this as a standalone plugin one day. It's always a goal to contribute back when possible and this seems like it can be a great standalone plugin in the future. |
| * | ||
| * @param array $skip_users The list of user IDs to skip. | ||
| */ | ||
| $skip_users = apply_filters( 'vip_security_last_seen_skip_users', array() ); |
There was a problem hiding this comment.
What do you think about moving this filter to is_considered_inactive()? That gives more flexibility for filtering the results of the check and might be more predictable.
|
|
||
| public function ignore_inactivity_check_for_user( $user_id, $until_timestamp = null ) { | ||
| if ( ! $until_timestamp ) { | ||
| $until_timestamp = strtotime( '+2 days' ); |
There was a problem hiding this comment.
The default of +2 days feels a little too magic and is hard to predict. Maybe we should just require $until_timestamp be specified?
There was a problem hiding this comment.
Minor, but I'd also consider naming the function ignore_inactivity_check_for_user_until( $user_id, $until_timestamp ). The _until makes it obvious that it's a limited time action, vs. permanent (which is what I thought it would do before I read the code)
|
|
This pull request has been marked stale because it has been open for 60 days with no activity. If there is no activity within 7 days, it will be closed. This is an automation to keep pull requests manageable and actionable and is not a comment on the quality of this pull request nor on the work done so far. Closed PRs are still valuable to the project and their branches are preserved. |
|
|
|
This pull request has been marked stale because it has been open for 60 days with no activity. If there is no activity within 7 days, it will be closed. This is an automation to keep pull requests manageable and actionable and is not a comment on the quality of this pull request nor on the work done so far. Closed PRs are still valuable to the project and their branches are preserved. |
Description
This PR introduces a
Last Seenuser meta to the users, which is going to be used to report and block inactive users, improving security by reducing the surface attack.Inactive users blocked on login
Inactive users on the Users page (network and site)
Inactive users
Changelog Description
Pre-review checklist
Please make sure the items below have been covered before requesting a review:
Pre-deploy checklist
Steps to Test
wp-admin>Users