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

Fixing bug where Super Admins cannot setup Time Based One-Time Password as first option Two Factor #4410

Closed
wants to merge 1 commit into from

Conversation

spenserhale
Copy link
Contributor

@spenserhale spenserhale commented Apr 28, 2023

Changelog Description

I am setting the controller parameter configuration to have user_id as an integer instead of a number because strict comparison requires matching types. Please have a look at #4409 for more information.

Fixes #4409

Checklist

Please make sure the items below have been covered before requesting a review:

  • [Y] This change works and has been tested locally (or has an appropriate fallback).
  • [Y] This change works and has been tested on a Go sandbox.
    • I used filters to apply this change in our WP VIP production.
  • [Y] This change has relevant unit tests.
    • No tests were found
  • [Y] This change uses a rollout method to ease with deployment
    • Not applicable
  • [Y] This change has relevant documentation additions/updates.
    • I do not believe applicable
  • [Y] I've created a changelog description that aligns with the provided examples.

Hotfix Code

function hooks_filter_two_factor_endpoint_configuration(array $endpoints): array {
    if ( isset($endpoints['/two-factor/1.0/totp']) ) {
        $endpoints['/two-factor/1.0/totp'][0]['args']['user_id']['type'] = 'integer';
        $endpoints['/two-factor/1.0/totp'][1]['args']['user_id']['type'] = 'integer';
    }

    if ( isset($endpoints['/two-factor/1.0/generate-backup-codes']) ) {
        $endpoints['/two-factor/1.0/totp'][0]['args']['user_id']['type'] = 'integer';
    }

    return $endpoints;
}
add_filter( 'rest_endpoints', 'hooks_filter_two_factor_endpoint_configuration' );

Steps to Test

  1. Setup a multisite
  2. Create a new user that is a super_admin
  3. Log in as the new super_admin
  4. Go to wp-admin > Edit Profile
  5. submit an authentication code

@spenserhale spenserhale requested a review from a team as a code owner April 28, 2023 04:36
@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@spenserhale
Copy link
Contributor Author

Tagging @rebeccahum

@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #4410 (39eda05) into develop (98341a2) will decrease coverage by 0.46%.
The diff coverage is n/a.

❗ Current head 39eda05 differs from pull request most recent head 53065db. Consider uploading reports for the commit 53065db to get more accurate results

@@              Coverage Diff              @@
##             develop    #4410      +/-   ##
=============================================
- Coverage      28.78%   28.32%   -0.46%     
+ Complexity      4606     4548      -58     
=============================================
  Files            270      265       -5     
  Lines          20405    20192     -213     
=============================================
- Hits            5873     5719     -154     
+ Misses         14532    14473      -59     

see 71 files with indirect coverage changes

@rebeccahum
Copy link
Contributor

Hi @spenserhale! Thank you for your contribution, but would you be able to PR directly to the plugin at https://github.com/WordPress/two-factor upstream please? Once that PR is approved, we will pull the changes in from there. 🙏 Thank you!!

@spenserhale
Copy link
Contributor Author

Created PR upstream: WordPress/two-factor#560

@rinatkhaziev rinatkhaziev reopened this Jul 13, 2023
@sjinks
Copy link
Member

sjinks commented Aug 18, 2023

It should be fixed in #4684

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

Successfully merging this pull request may close these issues.

Bug: Super Admins cannot setup Time Based One-Time Password as first option Two Factor
5 participants