Fix MFA verification error handling and remove 429 from retry list #140
Conversation
WalkthroughThe PR removes 429 (Too Many Requests) from the default HTTP retry status codes, updates error messages in the SSO login flow for clarity, and refactors Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 45 45
Lines 1911 1916 +5
=========================================
+ Hits 1911 1916 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/garth/sso.py (1)
1-263: Ruff formatting is failing CI for this file.
Please run: ruff format src/garth/sso.py (orruff check --fix) to satisfy the formatter.
🧹 Nitpick comments (1)
tests/test_http.py (1)
76-87: Make the assertion resilient across urllib3 versions.
Depending on urllib3,status_forcelistcan be a tuple, set, or frozenset. Compare as sets to avoid brittle failures.Apply this diff:
def test_configure_status_forcelist(client: Client): - assert client.status_forcelist == (408, 500, 502, 503, 504) + assert client.status_forcelist == (408, 500, 502, 503, 504) adapter = client.sess.adapters["https://"] assert isinstance(adapter, HTTPAdapter) - assert adapter.max_retries.status_forcelist == client.status_forcelist + assert set(adapter.max_retries.status_forcelist) == set(client.status_forcelist) client.configure(status_forcelist=(200, 201, 202)) assert client.status_forcelist == (200, 201, 202) adapter = client.sess.adapters["https://"] assert isinstance(adapter, HTTPAdapter) - assert adapter.max_retries.status_forcelist == client.status_forcelist + assert set(adapter.max_retries.status_forcelist) == set(client.status_forcelist)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/garth/http.py(1 hunks)src/garth/sso.py(2 hunks)src/garth/version.py(1 hunks)tests/test_http.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/**
⚙️ CodeRabbit configuration file
tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
Files:
tests/test_http.py
🧬 Code graph analysis (2)
tests/test_http.py (1)
tests/conftest.py (1)
client(21-22)
src/garth/sso.py (1)
src/garth/exc.py (1)
GarthException(7-10)
🪛 GitHub Actions: CI
src/garth/sso.py
[error] 1-1: Ruff format check failed. 1 file would be reformatted: src/garth/sso.py (44 files already formatted).
🔇 Additional comments (4)
src/garth/version.py (1)
1-1: Version bump acknowledged.
All good.src/garth/http.py (1)
27-27: Good call removing 429 from the default retry list.
Prevents retry loops on rate limits while keeping other transient errors retriable.tests/test_http.py (1)
77-77: Tests updated for new default forcelist — LGTM.
Defaults reflect removal of 429 and adapter sync is verified.src/garth/sso.py (1)
198-201: Post-MFA title check — LGTM.
This prevents futile retries and standardizes the MFA failure path.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/garth/sso.py (1)
1-1: Fix formatting to pass CI.The pre-commit formatter check failed. Run the formatter to resolve:
#!/bin/bash # Fix formatting in the repository pre-commit run --all-files
♻️ Duplicate comments (1)
src/garth/sso.py (1)
130-131: Error message misleading for non-MFA login failures.This issue was previously flagged and remains unaddressed. When credentials are invalid or login fails for non-MFA reasons, this raises "MFA verification failed," which misleads users into thinking MFA was the problem.
Track whether MFA was actually attempted and tailor the error message accordingly, as detailed in the previous review comment.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/garth/http.pysrc/garth/sso.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/garth/sso.py (2)
src/garth/exc.py (1)
GarthException(7-10)tests/conftest.py (1)
client(21-22)
🪛 GitHub Actions: CI
src/garth/sso.py
[error] 1-1: Pre-commit lint failed: 1 file would be reformatted by the formatter (Would reformat: src/garth/sso.py). Run 'pre-commit run --all-files' or fix formatting to pass.
🪛 Ruff (0.14.10)
src/garth/sso.py
131-131: Avoid specifying long messages outside the exception class
(TRY003)
201-201: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
src/garth/sso.py (1)
199-201: Good addition: fixes missing error check after MFA submission.This correctly validates the MFA verification result, addressing the gap where
handle_mfa()previously didn't check for invalid MFA input. The error message is appropriate in this context since MFA was definitely attempted.src/garth/http.py (1)
27-27: Sensible change: respecting rate limits.Removing HTTP 429 from the automatic retry list prevents the client from exacerbating rate-limit issues. When rate-limited, the caller should implement appropriate backoff logic rather than having the client retry immediately.
cyberjunky
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/garth/sso.pysrc/garth/version.py
✅ Files skipped from review due to trivial changes (1)
- src/garth/version.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/garth/sso.py (3)
tests/conftest.py (3)
client(21-22)oauth1_token(37-38)oauth2_token(55-61)tests/test_sso.py (1)
prompt_mfa(50-51)src/garth/exc.py (1)
GarthException(7-10)
🪛 GitHub Actions: CI
src/garth/sso.py
[error] 1-1: pre-commit: Would reformat: src/garth/sso.py. 1 file would be reformatted; run 'pre-commit run --all-files' to apply formatting.
🪛 Ruff (0.14.10)
src/garth/sso.py
205-205: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
src/garth/sso.py (3)
118-118: LGTM! Addresses the past review comment correctly.The
mfa_attemptedflag implementation properly differentiates between MFA verification failures and general login failures, preventing misleading error messages when credentials are incorrect. This directly addresses the concern raised in the previous review.Also applies to: 128-128, 132-135
203-205: Excellent fix for invalid MFA handling.This properly validates the MFA response and prevents the retry loop issue described in the PR objectives. By checking the title and raising an exception immediately when MFA verification fails, the function now fails fast with a clear error message instead of allowing invalid MFA attempts to cascade into 429 rate limit errors.
244-244: Good defensive programming for token synchronization.Explicitly setting the tokens on the client after MFA completion ensures that the client state is properly updated, which is important since
resume_login()operates outside the normal login flow. This enhances robustness and makes the token availability more predictable for callers.Also applies to: 246-250
src/garth/sso.py
Outdated
| handle_mfa(client, signin_params, lambda: mfa_code) | ||
| return _complete_login(client) | ||
| oauth1, oauth2 = _complete_login(client) | ||
|
|
There was a problem hiding this comment.
Fix formatting issue to resolve pre-commit failure.
Line 245 appears to have trailing whitespace, which is causing the pre-commit formatting check to fail.
Run the following command to fix formatting:
pre-commit run --all-files🤖 Prompt for AI Agents
In src/garth/sso.py around line 245 there is trailing whitespace causing the
pre-commit formatter to fail; remove the trailing whitespace on that line (or
re-save the file without extra spaces) and then run the pre-commit hooks (e.g.,
pre-commit run --all-files) to apply/verify formatting fixes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/garth/sso.py (1)
130-130: Optional: Consider removing redundant title fetch.Since
handle_mfa()now validates the title at line 205, fetching it again here is redundant for the MFA path (though the check at lines 132-137 is still needed for the non-MFA path). This is a minor optimization opportunity.💡 Optional refactor
mfa_attempted = True handle_mfa(client, SIGNIN_PARAMS, prompt_mfa) - title = get_title(client.last_resp.text) if title != "Success":Note: The check at lines 132-137 must remain for non-MFA failures, so only the
get_titlecall can be removed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/garth/sso.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/garth/sso.py (3)
tests/conftest.py (3)
client(21-22)oauth1_token(37-38)oauth2_token(55-61)tests/test_sso.py (1)
prompt_mfa(50-51)src/garth/exc.py (1)
GarthException(7-10)
🪛 Ruff (0.14.10)
src/garth/sso.py
207-207: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
src/garth/sso.py (4)
118-118: LGTM! Correct implementation of MFA tracking flag.The
mfa_attemptedflag properly differentiates between MFA and non-MFA login failures, addressing the past review comment about misleading error messages.Also applies to: 128-128
133-137: LGTM! Error messages are now accurate and specific.The conditional error handling correctly distinguishes between MFA verification failures and general login failures, providing users with more meaningful error messages.
205-207: LGTM! Critical fix for MFA validation.This title check catches invalid MFA codes immediately, preventing the 429 retry loop mentioned in the PR objectives. The unified error message "MFA verification failed" is clear and consistent.
Note: The static analysis hint (TRY003) appears to be a false positive—the message length is reasonable for inline exception construction.
247-253: LGTM! Explicit token synchronization improves state consistency.Setting the tokens explicitly on the client after MFA completion ensures the client's internal state is properly synchronized with the authentication tokens, which is good defensive programming.
|
@felipao-mx @matin I found this old PR from myself, updated it and added a new fix just discovered: |
Removed 429 from client's status forcelist.
…FA, and update version.
matin
force-pushed
the
MFA-improvements
branch
from
b9870a5 to
3c88c1c
Compare
| oauth1, oauth2 = _complete_login(client) | ||
|
|
||
| # Explicitly set tokens on the client | ||
| client.oauth1_token = oauth1 | ||
| client.oauth2_token = oauth2 | ||
|
|
||
| return oauth1, oauth2 |
There was a problem hiding this comment.
@cyberjunky why is this necessary? it's already handled if you call garth.http.Client.resume_login(), instead of calling this function direction
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/garth/sso.py (1)
202-204: Good defensive error handling for MFA verification.Adding validation after MFA submission prevents silent failures and addresses the issue where invalid MFA input could cause retry loops. The error message is clear and specific to MFA context.
Optional: Static analysis suggests avoiding long messages outside exception classes (TRY003). Consider creating a custom exception class with predefined messages if error handling expands further, but this is fine for current scope.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/garth/http.pysrc/garth/sso.pytests/test_http.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_http.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/garth/sso.py (2)
src/garth/exc.py (1)
GarthException(7-10)tests/conftest.py (2)
oauth1_token(37-38)oauth2_token(55-61)
🪛 Ruff (0.14.10)
src/garth/sso.py
134-134: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
src/garth/http.py (1)
27-27: Removing 429 from default retry list prevents rate limit escalation.The removal of HTTP 429 (Too Many Requests) from the default retry list is appropriate. Aggressively retrying rate-limited requests can worsen the situation, especially during authentication flows. Users can still override this via
configure(status_forcelist=...)if needed for specific use cases.src/garth/sso.py (2)
134-134: LGTM! Error message is now clearer.The updated error message "Login failed: {title}" is more accurate and not misleading for non-MFA failures.
244-250: Explicit token assignment fixes timing issues and improves consistency.The refactored
resume_login()now explicitly sets tokens on the client before returning, ensuring the client state is fully updated. This addresses the timing-related oauth1/oauth2 errors mentioned in the PR description and makes the function consistent with the regularlogin()flow.
2 participants
I have some small MFA improvements.
While testing I ran into a 429 retry limit, because handle_mfa() was not checking for error results (invalid MFA input)
I have removed HTTP status 429 from the retry list, since this only makes things worse.
And made the error messages the same for both MFA methods, change it to 'MFA verification failed'
In garth Library - Ensure Tokens Set Before Return in sso.resume_login()
The sso.resume_login() should configure the client's tokens before returning:
Summary by CodeRabbit
Bug Fixes
Changes
✏️ Tip: You can customize this high-level summary in your review settings.