added filter to control whether authcode autosubmits#741
added filter to control whether authcode autosubmits#741eric-michel wants to merge 8 commits intoWordPress:masterfrom
Conversation
kasparsd
left a comment
There was a problem hiding this comment.
This looks good! Had one optional suggestion for moving the PHP state into JS variable to help move these scripts into standalone JS eventually.
Could you please document the filter, add the docblock comment (similar to other filters)?
class-two-factor-core.php
Outdated
| if ( undefined !== form.requestSubmit ) { | ||
| form.requestSubmit(); | ||
| form.submit.disabled = "disabled"; | ||
| <?php if ( $auto_submit_authcode ) { ?> |
There was a problem hiding this comment.
Could we use the variable state to generate a JS variable instead? That would allow us to eventually move this JS into an external file and pass that as data to the script.
Something like const autoSubmitEnabled = <?php echo json_encode( ... ); ?> and then use that here in pure JS if-check?
There was a problem hiding this comment.
Happy to make this change! How/where would you like the JS variable to be output? I could output it as a data attribute on the form element (something like data-auto-submit) or could output it as a small <script> snippet in the <head> tag so it's a global variable that can be accessed. I kind of lean toward the data attribute to keep it scoped to the form itself.
There was a problem hiding this comment.
@kasparsd any thoughts on the question above on approach?
class-two-factor-core.php
Outdated
| ?> | ||
|
|
||
| <form name="validate_2fa_form" id="loginform" action="<?php echo esc_url( self::login_url( array( 'action' => $action ), 'login_post' ) ); ?>" method="post" autocomplete="off"> | ||
| <form name="validate_2fa_form" id="loginform" action="<?php echo esc_url( self::login_url( array( 'action' => $action ), 'login_post' ) ); ?>" method="post" autocomplete="off"<?php if ( $auto_submit_authcode ) { ?> data-auto-submit="true"<?php } ?>> |
There was a problem hiding this comment.
I appreciate this running through a data attribute -- though as I'm looking at a bit of code from TOTP that adds a data-digits parameter in the provider's authentication_page()
that's what populates the expectedLength variable -- I'm wondering if instead it would be better to filter the data-digits property on the element? Maybe not include the attribute or something if it's filtered to null instead?
Also, kind of out-of-scope for this, but I'd like to see the autosubmit also work for codes that are alphanumeric or the like, so if it could maybe be elevated out of the "only numbers" conditional, but that's minor quibbles.
|
Hey @eric-michel, Could you take a moment to review the comments from @georgestephanis? Thanks again for your contributions! |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Co-authored-by: George Stephanis <daljo628@gmail.com>
|
@masteradhoc @georgestephanis My apologies for being AWOL for a bit! Work got busy for a while. @georgestephanis if I'm understanding correctly, you're suggesting instead of having a separate data attribute for whether autosubmit is active or not, instead utilize the preexisting |
|
Yup! Basically if it doesn't have a length set, it won't autosubmit? |
Co-authored-by: Brian <brian@brianhaas.li>
|
@georgestephanis See if you're happy with this implementation. |
|
I'm gonna have to think on this a bit -- I wanna make sure we're passing stuff around and parsing it out of the dataset well. I'll do some tinkering and get back to you ideally this evening? |
What?
Adds a filter to allow a theme or plugin to turn off auto submission of authcodes
Why?
Plugins that enhance two-factor may want to turn off auto-submission to allow the user time to interact with other form elements on the page prior to submission. See #723.
How?
A filter is added and assigned to a variable. That variable is checked prior to the relevant JS being output (and prevents it from printing entirely if the value is false).
Testing Instructions
functions.php, add the following:Changelog Entry