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

Optionally Force 2fa #239

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
c5e1502
Add network-level settings to force 2-factor auth for whole site or s…
mikeselander Sep 3, 2018
571bc63
Add helper functions to evaluate whether the current user requires 2f…
mikeselander Sep 3, 2018
48e4ba1
Add basic force-2fa-view functionality
mikeselander Sep 3, 2018
4e2873c
Add form submission and AJAX handling of two-factor settings form
mikeselander Sep 3, 2018
7d478a9
Small self-review cleanups
mikeselander Sep 4, 2018
de2d5bb
Load force-2fa script more intelligently
mikeselander Sep 4, 2018
28024b8
Replace Back to Site link with Logout link as an escape hatch
mikeselander Sep 4, 2018
8ace959
Do not block REST requests
mikeselander Sep 4, 2018
0ed9963
Clean up title styling
mikeselander Sep 4, 2018
884baed
Prepare value getters to work with either multi or single site
mikeselander Sep 4, 2018
82ee390
Handle REST check with two separate hooks to properly capture late-re…
mikeselander Sep 4, 2018
1db7029
Add hook for providers to save their data against on AJAX submission …
mikeselander Sep 4, 2018
5b644f7
Remove inline styles and use user-edit CSS instead
mikeselander Sep 4, 2018
1ace71e
Hide unnecessary label on force-2fa view
mikeselander Sep 4, 2018
bad9706
CS cleanup
mikeselander Sep 4, 2018
681da69
Add tests for business-critical methods
mikeselander Sep 4, 2018
8d531bf
Add returns to maybe_force_2fa_settings for better testability
mikeselander Sep 4, 2018
d4712ce
Self-review cleanup
mikeselander Sep 4, 2018
bcbfec3
Register and save values against a single site'
mikeselander Sep 4, 2018
0898fdc
Enqueue assets for Fido 2f on forced 2f takeover
mikeselander Sep 4, 2018
26824fc
Add spacing
mikeselander Sep 5, 2018
51b93a3
Remove unnecessary ternary
mikeselander Sep 5, 2018
2723f88
Update language on global 2fa forcing option
mikeselander Sep 5, 2018
8980d90
Split up core class into Force and Core for better separation
mikeselander Sep 5, 2018
e8ccab1
Add custom path and handling for force-2fa
mikeselander Sep 5, 2018
13a8cf7
Clean up
mikeselander Sep 5, 2018
0b356f3
Clean up according to es linter on Travis
mikeselander Sep 5, 2018
de8ea84
Add phpcs:ignore for incorrectly flagged issues
mikeselander Sep 17, 2018
7b4cce3
Properly space these
mikeselander Sep 17, 2018
f7cc27e
Allow user to remove forced 2fa on all roles
tcrsavage May 8, 2019
252e3e0
Merge pull request #2 from humanmade/force-2fa-fix-empty-role-config
joehoyle Jul 18, 2019
d923927
Merge remote-tracking branch 'upstream/master' into force-2fa
joehoyle Aug 4, 2023
4d6d681
Allow enforcing 2fa for super adminns
shadyvb Feb 14, 2022
98a3165
Translate role name
shadyvb Feb 14, 2022
1adfe47
Fix totp with force 2fa
joehoyle Aug 4, 2023
57bfc73
Checks in multisite only
joehoyle Aug 4, 2023
753c258
Added required dependancy as there is no build step using composer
mikelittle Oct 10, 2023
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
88 changes: 88 additions & 0 deletions assets/js/force-2fa.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/* global ajaxurl, jQuery */

/**
* Checks that an element has a non-empty `name` and `value` property.
*
* @param {Element} element The element to check
* @return {Boolean} true if the element is an input, false if not
*/
var isValidElement = function( element ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we wrap these methods in an anonymous function to isolate the namespace? Some of these function names are very generic and could cause conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd really rather not - anonymous functions are a scourge to test and/or maintain and make the code far less readable. I would be happy to make a "namespace" via variable although the plugin would be better to add a build pipeline from ES6 to address this kind of issue instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly, it would work great if this was imported as an ES6 module but currently everything in the WP core is added to the same global namespace and we should do our best to avoid any collisions.

Most of the core JS files appear to just wrap things in jQuery(document).ready( function($) {} );. Using a variable or prefixing the function names would work, too.

return element.name && element.value;
};

/**
* Checks if an element’s value can be saved (e.g. not an unselected checkbox).
*
* @param {Element} element The element to check
* @return {Boolean} true if the value should be added, false if not
*/
var isValidValue = function( element ) {
return ( ! [ 'checkbox', 'radio' ].includes( element.type ) || element.checked );
};

/**
* Checks if an input is a checkbox, because checkboxes allow multiple values.
*
* @param {Element} element The element to check
* @return {Boolean} true if the element is a checkbox, false if not
*/
var isCheckbox = function( element ) {
return 'checkbox' === element.type;
};

/**
* Retrieves input data from a form and returns it as a JSON object.
*
* @param {HTMLFormControlsCollection} elements the form elements
* @return {Object} form data as an object literal
*/
var formToJSON = function( elements ) {
return [].reduce.call( elements, function( data, element ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

jQuery has serializeArray() -- could we re-use that instead?


// Make sure the element has the required properties and should be added.
if ( ! isValidElement( element ) || ! isValidValue( element ) ) {
return data;
}

/*
* Some fields allow for more than one value, so we need to check if this
* is one of those fields and, if so, store the values as an array.
*/
if ( isCheckbox( element ) ) {
data[ element.name ] = ( data[ element.name ] || [] ).concat( element.value );
} else {
data[ element.name ] = element.value;
}

return data;
}, {} );
};

/**
* A handler function to prevent default submission and run our custom script.
*
* @param {Event} event the submit event triggered by the user
*/
var handleFormSubmit = function( event ) {

// Get form data.
var formData = formToJSON( event.target.elements );

event.preventDefault();

formData.action = 'two_factor_force_form_submit';

// Submit data to WordPress.
jQuery.post(
ajaxurl,
formData,
function () {
window.location.reload();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're not using the AJAX response, could we just skip all AJAX/serialization concept and point the form to the login URL?

}
);
};

window.addEventListener( 'load', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could stick to jQuery helpers to support more browsers? window.addEventListener( 'load', function() {} ) could be replaced with $( document ).ready(function() {}) since we actually need to wait for DOMContentLoaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand how this would support more browsers - addEventListener has excellent support: https://caniuse.com/#feat=addeventlistener

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just suggesting that switching to the jQuery helpers (since we're using jQuery for AJAX already) would help with the compatibility even more. And would also keep it consistent with the rest of the plugin JS files until we switch to a proper build pipeline which adds the necessary polyfills.

var form = document.querySelector( '#force_2fa_form' );
form.addEventListener( 'submit', handleFormSubmit );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be replaced with $( '#force_2fa_form' ).on( 'submit', handleFormSubmit ); for better browser compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the above comment on compatibility, it really doesn't improve things much if at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm only suggesting that it would make it consistent with the rest of the plugin JS and the compatibility it currently provides.

} );
21 changes: 18 additions & 3 deletions class-two-factor-core.php
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,28 @@ public static function add_hooks( $compat ) {
add_filter( 'authenticate', array( __CLASS__, 'filter_authenticate_block_cookies' ), PHP_INT_MAX );

add_filter( 'attach_session_information', array( __CLASS__, 'filter_session_information' ), 10, 2 );

add_action( 'admin_init', array( __CLASS__, 'trigger_user_settings_action' ) );
add_filter( 'two_factor_providers', array( __CLASS__, 'enable_dummy_method_for_debug' ) );

add_action( 'two_factor_ajax_options_update', array( __CLASS__, 'user_two_factor_options_update' ) );
add_action( 'init', array( __CLASS__, 'register_scripts' ) );

$compat->init();
}

/**
* Register scripts.
*/
public static function register_scripts() {
wp_register_style(
'user-edit-2fa',
plugins_url( 'user-edit.css', __FILE__ ),
[],
TWO_FACTOR_VERSION
);
}

/**
* Loads the plugin's text domain.
*
Expand Down Expand Up @@ -1694,7 +1709,7 @@ 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 ) {
wp_enqueue_style( 'user-edit-2fa', plugins_url( 'user-edit.css', __FILE__ ), array(), TWO_FACTOR_VERSION );
wp_enqueue_style( 'user-edit-2fa' );

$enabled_providers = array_keys( self::get_available_providers_for_user( $user ) );
$primary_provider = self::get_primary_provider_for_user( $user->ID );
Expand Down Expand Up @@ -1732,7 +1747,7 @@ public static function user_two_factor_options( $user ) {
<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="form-table">
<tr>
<th>
<th class="two-factor-main-label">
<?php esc_html_e( 'Two-Factor Options', 'two-factor' ); ?>
</th>
<td>
Expand Down
Loading
Loading