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.

Sign up for GitHub

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

Encourage setting up a second recovery method #642

Merged
merged 4 commits into from
Dec 2, 2024
Merged

Encourage setting up a second recovery method #642

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
240bcce
Add notices
kasparsd Sep 19, 2024
1225521
Add the default notice as a permanent information
kasparsd Sep 19, 2024
11c97f3
Merge branch 'master' into 485-add-backup-notice
kasparsd Dec 2, 2024
2284d6a
Add additional escaping just in case
kasparsd Dec 2, 2024
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
31 changes: 22 additions & 9 deletions class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1788,6 +1788,8 @@ public static function manage_users_custom_column( $output, $column_name, $user_
* @param WP_User $user WP_User object of the logged-in user.
*/
public static function user_two_factor_options( $user ) {
$notices = [];

wp_enqueue_style( 'user-edit-2fa', plugins_url( 'user-edit.css', __FILE__ ), array(), TWO_FACTOR_VERSION );

$enabled_providers = array_keys( self::get_available_providers_for_user( $user ) );
Expand All @@ -1803,16 +1805,16 @@ public static function user_two_factor_options( $user ) {
$show_2fa_options = self::current_user_can_update_two_factor_options();

if ( ! $show_2fa_options ) {
$url = self::get_user_two_factor_revalidate_url();
$url = add_query_arg( 'redirect_to', urlencode( self::get_user_settings_page_url( $user->ID ) . '#two-factor-options' ), $url );
$url = add_query_arg(
'redirect_to',
urlencode( self::get_user_settings_page_url( $user->ID ) . '#two-factor-options' ),
self::get_user_two_factor_revalidate_url()
);

printf(
'<div class="notice notice-warning inline"><p>%s</p></div>',
sprintf(
__( 'To update your Two-Factor options, you must first revalidate your session.', 'two-factor' ) .
'<br><a class="button" href="%s">' . __( 'Revalidate now', 'two-factor' ) . '</a>',
$notices['warning two-factor-warning-revalidate-session'] = sprintf(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like the hidden meaning of the first key string being the class suffix for notice-* but that's how we can allow for multiple notices of the same type without introducing some kind of an abstraction.

esc_html__( 'To update your Two-Factor options, you must first revalidate your session.', 'two-factor' ) .
' <a class="button" href="%s">' . esc_html__( 'Revalidate now', 'two-factor' ) . '</a>',
esc_url( $url )
)
);
}

Expand All @@ -1821,9 +1823,20 @@ public static function user_two_factor_options( $user ) {
$show_2fa_options ? '' : 'disabled="disabled"'
);

wp_nonce_field( 'user_two_factor_options', '_nonce_user_two_factor_options', false );
if ( 1 === count( $enabled_providers ) ) {
$notices['warning two-factor-warning-suggest-backup'] = esc_html__( 'To prevent being locked out of your account, consider enabling a backup method like Recovery Codes in case you lose access to your primary authentication method.', 'two-factor' );
}
?>
<h2><?php esc_html_e( 'Two-Factor Options', 'two-factor' ); ?></h2>
<?php foreach ( $notices as $notice_type => $notice ) : ?>
<div class="<?php echo esc_attr( $notice_type ? 'notice inline notice-' . $notice_type : '' ); ?>">
<p><?php echo $notice; ?></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the values are already escaped early should there be a wp_kses here to be safe / should we mention the values are already escaped above?

Copy link
Member

Choose a reason for hiding this comment

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

@kasparsd FYI on the question above for you

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now done in 2284d6a. We're using wp_kses_post() similar to how WP core does it here:

https://github.com/WordPress/WordPress/blob/369a6754887a948830f2107602648e46cb370dfb/wp-admin/includes/class-wp-site-health.php#L2419

</div>
<?php endforeach; ?>
<p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are now permanent instructions suggest the best-practice setup.

<?php esc_html_e( 'Configure a primary two-factor method along with a backup method, such as Recovery Codes, to avoid being locked out if you lose access to your primary method.', 'two-factor' ); ?>
</p>
<?php wp_nonce_field( 'user_two_factor_options', '_nonce_user_two_factor_options', false ); ?>
<input type="hidden" name="<?php echo esc_attr( self::ENABLED_PROVIDERS_USER_META_KEY ); ?>[]" value="<?php /* Dummy input so $_POST value is passed when no providers are enabled. */ ?>" />
<table class="wp-list-table widefat fixed striped table-view-list two-factor-methods-table">
<thead>
Expand Down
Loading