Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broadcast events for logging, email notifications, etc #526

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 60 additions & 2 deletions class-two-factor-core.php
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,7 @@ public static function verify_login_nonce( $user_id, $nonce ) {
$login_nonce = get_user_meta( $user_id, self::USER_META_NONCE_KEY, true );

if ( ! $login_nonce || empty( $login_nonce['key'] ) || empty( $login_nonce['expiration'] ) ) {
self::broadcast( 'login_nonce_missing', compact( 'user_id' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that adding events for low-level validation is a bit over the top, yes, they could be used to pick up someone fuzzing the login forms, but they're not scenario's where a logger needs/should be aware of IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it'd be useful to have a detailed audit trail when investigating a security incident, but at the same time I agree that we also don't wanna clutter everyday logs with standard pentesting.

Broadcasting low-level events doesn't mean they'll necessarily be used, though. #459 might only log high-level stuff by default, but log everything if a user changes a filter. I imagine #476 would hardcode a very short list of events that it chooses to send emails about.

The specifics will vary based on the caller, and the site, though. So IMO it's best to leave that choice up to them. What do you think?

I don't feel strongly, though. Broadcasting high-level stuff would be better than nothing, and we can always add more in the future if we change our minds.

return false;
}

Expand All @@ -905,6 +906,8 @@ public static function verify_login_nonce( $user_id, $nonce ) {
// Require a fresh nonce if verification fails.
self::delete_login_nonce( $user_id );

self::broadcast( 'login_nonce_mismatch', compact( 'user_id' ) );
dd32 marked this conversation as resolved.
Show resolved Hide resolved

return false;
}

Expand Down Expand Up @@ -974,7 +977,13 @@ public static function is_user_rate_limited( $user ) {
* @param bool $rate_limited Whether the user login is rate limited.
* @param WP_User $user The user attempting to login.
*/
return apply_filters( 'two_factor_is_user_rate_limited', $rate_limited, $user );
$rate_limited = apply_filters( 'two_factor_is_user_rate_limited', $rate_limited, $user );

if ( $rate_limited ) {
self::broadcast( 'user_rate_limited', compact( 'user', 'rate_limit', 'last_failed' ) );
}

return $rate_limited;
}

/**
Expand Down Expand Up @@ -1006,6 +1015,7 @@ public static function login_form_validate_2fa() {
if ( isset( $providers[ $provider ] ) ) {
$provider = $providers[ $provider ];
} else {
self::broadcast( 'unavailable_provider', compact( 'user', 'provider' ) );
wp_die( esc_html__( 'Cheatin’ uh?', 'two-factor' ), 403 );
}
} else {
Expand Down Expand Up @@ -1240,6 +1250,7 @@ public static function enable_provider_for_user( $user_id, $new_provider ) {
$available_providers = self::get_providers();

if ( ! array_key_exists( $new_provider, $available_providers ) ) {
self::broadcast( 'invalid_provider', compact( 'user_id', 'new_provider' ) );
dd32 marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

Expand All @@ -1260,7 +1271,16 @@ public static function enable_provider_for_user( $user_id, $new_provider ) {
$has_primary = update_user_meta( $user_id, self::PROVIDER_USER_META_KEY, $new_provider );
}

return $enabled && $has_primary;

$success = $enabled && $has_primary;

if ( $success ) {
self::broadcast( 'provider_enabled_for_user', compact( 'user', 'new_provider' ) );
} else {
self::broadcast( 'provider_not_enabled_for_user', compact( 'user', 'new_provider' ) );
}

return $success;
}

/**
Expand Down Expand Up @@ -1311,4 +1331,42 @@ public static function rememberme() {

return (bool) apply_filters( 'two_factor_rememberme', $rememberme );
}


/**
* Broadcast an event to any listeners.
*
* This doesn't handle the event, it simply broadcasts it so that plugins can hook in and log it, send email notifications, etc.
*
* @param string $event_code A unique name for the event.
* @param array $event_data Contains any additional context about the event.
* ⚠️ DO NOT add any sensitive info, like tokens, passwords, etc. It could end up in
* error logs, emails, etc, and the plugin handling the event might not have enough
* context to be aware of it or redact it.
* The only exception to that is a `WP_User` object, which will automatically be
* reduced to the `user_login`.
Comment on lines +1346 to +1347
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* The only exception to that is a `WP_User` object, which will automatically be
* reduced to the `user_login`.
* The only exception to that is a `WP_User` object in the 'user' key, which will
* automatically be reduced to the `user_login`.

*
* @return void
*/
public static function broadcast( $event_code, $event_data = array() ) {
// Allow callers to pass in a WP_User object for convenience, but redact the password to prevent leaking it.
if ( $event_data['user'] instanceof WP_User ) {
$event_data['user'] = $event_data['user']->user_login;
}

/**
* Listen for all Two Factor events.
*
* @param string $event_code A unique name for the event.
* @param array $event_data Contains any additional context about the event.
*/
do_action( 'two_factor_event', $event_code, $event_data );

/**
* Listen for an individual Two Factor event.
*
* @param array $event_data Contains any additional context about the event.
*/
do_action( 'two_factor_event_' . $event_code, $event_data );
}
}
3 changes: 3 additions & 0 deletions providers/class-two-factor-backup-codes.php
Original file line number Diff line number Diff line change
Expand Up @@ -369,10 +369,13 @@ public function validate_code( $user, $code ) {
foreach ( $backup_codes as $code_index => $code_hashed ) {
if ( wp_check_password( $code, $code_hashed, $user->ID ) ) {
$this->delete_code( $user, $code_hashed );
Two_Factor_Core::broadcast( 'backup_code_used', compact( 'user' ) );
return true;
}
}
}

Two_Factor_Core::broadcast( 'backup_code_invalid', compact( 'user' ) );
return false;
}

Expand Down
5 changes: 5 additions & 0 deletions providers/class-two-factor-totp.php
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@ public function is_valid_key( $key ) {
*/
public function validate_authentication( $user ) {
if ( empty( $_REQUEST['authcode'] ) ) {
Two_Factor_Core::broadcast( 'totp_missing_authcode', compact( 'user' ) );
dd32 marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

Expand All @@ -469,18 +470,22 @@ public function validate_code_for_user( $user, $code ) {
);

if ( ! $valid_timestamp ) {
Two_Factor_Core::broadcast( 'totp_invalid_timestamp', compact( 'user' ) );
return false;
}

$last_totp_login = (int) get_user_meta( $user->ID, self::LAST_SUCCESSFUL_LOGIN_META_KEY, true );

// The TOTP authentication is not valid, if we've seen the same or newer code.
if ( $last_totp_login && $last_totp_login >= $valid_timestamp ) {
Two_Factor_Core::broadcast( 'totp_reused_or_old_code', compact( 'user' ) );
return false;
}

update_user_meta( $user->ID, self::LAST_SUCCESSFUL_LOGIN_META_KEY, $valid_timestamp );

Two_Factor_Core::broadcast( 'totp_auth_validated', compact( 'user' ) );

return true;
}

Expand Down