Skip to content

Update endpoints used for MFA operations#131

Merged
maxdeviant merged 9 commits intomainfrom
update-endpoints-used-for-mfa-operations
Jul 15, 2022
Merged

Update endpoints used for MFA operations#131
maxdeviant merged 9 commits intomainfrom
update-endpoints-used-for-mfa-operations

Conversation

@maxdeviant
Copy link
Contributor

This PR updates the endpoints used for two of the MFA operations:

  • challenge_factor
  • verify_challenge

The verify_factor method is now deprecated and has been replaced with verify_challenge. verify_factor will be removed in the next major version.

Resolves SDK-547.

@maxdeviant maxdeviant self-assigned this Jul 5, 2022
@linear
Copy link

linear bot commented Jul 5, 2022

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2022

Codecov Report

Merging #131 (327dc7e) into main (2a6c5a8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
+ Coverage   93.24%   93.25%   +0.01%     
==========================================
  Files          22       22              
  Lines         577      578       +1     
==========================================
+ Hits          538      539       +1     
  Misses         39       39              
Impacted Files Coverage Δ
workos/mfa.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@maxdeviant maxdeviant requested a review from a team July 5, 2022 21:24
@maxdeviant maxdeviant changed the title Update endpoints used for mfa operations Update endpoints used for MFA operations Jul 5, 2022
workos/mfa.py Outdated
DeprecationWarning,
)

params = {
Copy link
Contributor

@mthadley mthadley Jul 7, 2022

Choose a reason for hiding this comment

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

Given the differences between verify_factor and verify_challenge are...

  • One has a warn since it is deprecated
  • The other uses the new auth/challenges/{challenge_id}/verify endpoint, but is otherwise backwards compatible

Can we define verify_factor in terms of verify_challenge until it is finally removed?

    def verify_factor(
        self,
        authentication_challenge_id=None,
        code=None,
    ):
        """
        Verifies the one time password provided by the end-user.

        Kwargs:
            authentication_challenge_id (str) - The ID of the authentication challenge that provided the user the verification code.
            code (str) - The verification code sent to and provided by the end user.

        Returns: Dict containing the  challenge factor details.
        """

        warn(
            "'verify_factor' is deprecated. Please use 'verify_challenge' instead.",
            DeprecationWarning,
        )

        return self.verify_challenge(
           authentication_challenge_id=autentication_challenge_id, code=code
        )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point.

I updated verify_factor to be implemented in terms of verify_challenge.

@@ -127,6 +126,11 @@ def verify_factor(
Returns: Dict containing the challenge factor details.
Copy link
Contributor

@mthadley mthadley Jul 7, 2022

Choose a reason for hiding this comment

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

Should we also update the Docstring for verify_factor to mention that it is now deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

@mthadley mthadley left a comment

Choose a reason for hiding this comment

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

Looking good!

@maxdeviant maxdeviant merged commit d0f432e into main Jul 15, 2022
@maxdeviant maxdeviant deleted the update-endpoints-used-for-mfa-operations branch July 15, 2022 03:14
@maxdeviant maxdeviant mentioned this pull request Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants