fix(auth): prefer fido2 for reauth fallback#17391
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the pyu2f dependency with fido2 for security key reauthentication, updating the challenge implementation, credentials documentation, setup dependencies, and associated tests. Feedback from the review highlights that fido2 version 2.0.0 does not exist on PyPI, so the dependency range in setup.py should target 1.x instead. Additionally, it is recommended to handle potential OSError exceptions when listing devices or communicating with the security key to prevent unexpected crashes and improve error handling.
parthea
added
kokoro:force-run
yoshi-kokoro
removed
kokoro:run
goutamadwant
changed the title
|
Thanks for checking @parthea updated this PR to keep the transition safer. The flow is now WebAuthn first, then fido2, then pyu2f only if fido2 is not installed and pyu2f is already present. The pyu2f path emits a DeprecationWarning and google-auth[reauth] still installs fido2 so new installs do not pull in pyu2f. aslo added docstrings and type hints for the new helpers, plus coverage for the fido2 to pyu2f fallback path. let me know if this looks good now or do you have any suggestions |
parthea
added
kokoro:force-run
yoshi-kokoro
removed
kokoro:run
parthea
left a comment
parthea
left a comment
There was a problem hiding this comment.
LGTM once checks pass.
google/oauth2/challenges.py:143: error: Cannot find implementation or library stub for module named "fido2.ctap1" [import-not-found]
google/oauth2/challenges.py:143: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
google/oauth2/challenges.py:253: error: Missing return statement [return]
tests/oauth2/test_challenges.py:58: error: Need type annotation for "devices" (hint: "devices: list[<type>] = ...") [var-annotated]
tests/oauth2/test_challenges.py:66: error: Need type annotation for "calls" (hint: "calls: list[<type>] = ...") [var-annotated]
tests/oauth2/test_challenges.py:67: error: Need type annotation for "side_effects" (hint: "side_effects: list[<type>] = ...") [var-annotated]
tests/oauth2/test_challenges.py:103: error: Need type annotation for "origins" (hint: "origins: list[<type>] = ...") [var-annotated]
tests/oauth2/test_challenges.py:104: error: Need type annotation for "calls" (hint: "calls: list[<type>] = ...") [var-annotated]
tests/oauth2/test_challenges.py:105: error: Need type annotation for "side_effects" (hint: "side_effects: list[<type>] = ...") [var-annotated]
Found 8 errors in 2 files (checked 164 source files)
|
Thanks for your comments @parthea Changes made as below :
Let me know if it looks good now or you have any suggestions. thanks. |
parthea
added
kokoro:force-run
yoshi-kokoro
removed
kokoro:run
parthea
left a comment
parthea
left a comment
There was a problem hiding this comment.
Instead of:
sys.stderr.write("No security key found.\n")
return None
We should do this:
raise exceptions.ReauthFailError("No security key found.")
This can also be done as a follow up
| except self._SecurityKeyTimeout: | ||
| return None | ||
|
|
||
| sys.stderr.write("Ineligible security key.\n") |
There was a problem hiding this comment.
Is there a reason to use stderr.write here? IIRC, other parts of the library uses logging
There was a problem hiding this comment.
Thanks @daniel-sanche.. I kept stderr here because this challenge flow already uses it for interactive prompts, and the legacy pyu2f path also writes user facing challenge messages there.
but based on @parthea comment.. I changed the terminal fido2 failures to raise ReauthFailError instead of writing to stderr.
I kept stderr only for the interactive prompts since this challenge flow already uses stderr for user prompts like asking the user to touch the key.
updated the fido2 tests for all these cases. Let me know if you have any other suggestions!
parthea
added
kokoro:force-run
yoshi-kokoro
removed
kokoro:run
4 participants
Uses
fido2as the preferred security-key fallback for google-auth reauth, while keeping the WebAuthn handler path first.For compatibility, the old
pyu2fpath is still available whenfido2is not installed andpyu2fis already present. That path now emits aDeprecationWarning.The
google-auth[reauth]extra installsfido2, so new installs do not pull in deprecatedpyu2f.Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #17381