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

Add filter for plugins to add support link to login form. #615

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

StevenDufresne
Copy link

@StevenDufresne StevenDufresne commented Jul 11, 2024

What?

Sometimes users have difficulty with their two-factor settings. In the "Having Problems?" section, we currently don't have a way to add links that are not providers.

This PR adds a filter called two_factor_login_support_links to allow plugins to provide non-provider support links.

Why?

We want to give users access to documentation that can help them with using 2fa or with recovering their account.

For example, it would allow us to do

Having Problems?
- Use your authenticator app
- Use a recovery code
- Recover your account

How?

It adds a filter inside the <ul> so HTML can be passed in like so:

function my_custom_login_support_links( $links ) {
    // Add li.a items to the list
    return $links;
}
add_filter( 'two_factor_login_support_links', 'my_custom_login_support_links' )

Testing Instructions

  1. Add the filter mentioned above
  2. With 2fa configured, try logging in
  3. Expect to see the new link in the list.

Changelog Entry

Added - Filter named two_factor_login_support_links to support adding links to login form.

@StevenDufresne StevenDufresne changed the title Add/filter for support links Add filter for plugins to add support link to login form. Jul 11, 2024
@jeffpaul jeffpaul requested a review from kasparsd July 11, 2024 19:23
@jeffpaul jeffpaul added this to the 0.10.0 milestone Jul 11, 2024
Comment on lines +840 to +848
/*
* Allow plugins to add links to the two-factor login form.
*/
$links = apply_filters( 'two_factor_login_support_links', $links );

// Echo out the filtered links
foreach ( $links as $link ) {
echo wp_kses_post( $link );
}
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
/*
* Allow plugins to add links to the two-factor login form.
*/
$links = apply_filters( 'two_factor_login_support_links', $links );
// Echo out the filtered links
foreach ( $links as $link ) {
echo wp_kses_post( $link );
}
/*
* Allow plugins to filter the backup method links on the login form.
*/
$links = apply_filters( 'two_factor_login_backup_links', $links );
// Echo out the filtered links
echo implode( '', $links );

Given this filter only runs when there are backup providers, calling it a support links might be confusing for some, we can probably name it something like two_factor_login_backup_links?

There's probably an argument that this should be built prior to the if ( $backup_providers ) conditional to output filtered data even when there exist no backup methods for the given user?

I don't think we need to explicitly sanitize the HTML here either, it's either hard-coded HTML or from a PHP function that can do anything anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be addressed in the follow-up commits.

Copy link
Member

Choose a reason for hiding this comment

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

These points remain:

Given this filter only runs when there are backup providers, calling it a support links might be confusing for some, we can probably name it something like two_factor_login_backup_links?

OR

There's probably an argument that this should be built prior to the if ( $backup_providers ) conditional to output filtered data even when there exist no backup methods for the given user?

I suspect the intention of the filter was actually to be used when the user has no backup provider, or, at least that others who would have a use for it would be in that group.

Copy link
Collaborator

@kasparsd kasparsd left a comment

Choose a reason for hiding this comment

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

@StevenDufresne Was there anything else you wanted to include here or is this ready for merge?

If you don't get around to it, we'll extend the inline docblock for the filter to include documentation for the arguments (like here) and also add it to the relevant readme section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants