MFA improvements #140
MFA improvements #140
Conversation
WalkthroughRemoves HTTP 429 from the default retry status list. Adds explicit HTML title checks after MFA and final login, raising a uniform “MFA verification failed” error when the title is not “Success.” Bumps version to 0.5.18. Updates tests to reflect the new retry defaults and retain override behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as Client
participant S as SSO Service
U->>C: login(credentials)
C->>S: POST /login (credentials)
S-->>C: 200 HTML (may require MFA)
alt MFA required
C->>U: prompt for MFA
U->>C: submit MFA code
C->>S: POST /mfa (code)
S-->>C: 200 HTML (with title)
alt title == "Success"
C->>S: Complete login (_complete_login)
S-->>C: Tokens / session
C-->>U: success
else title != "Success"
C-->>U: throw GarthException("MFA verification failed")
end
else No MFA
C->>S: Complete login (_complete_login) after title check
alt title == "Success"
S-->>C: Tokens / session
C-->>U: success
else title != "Success"
C-->>U: throw GarthException("MFA verification failed")
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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 |
Removed 429 from client's status forcelist.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
===========================================
- Coverage 100.00% 99.94% -0.06%
===========================================
Files 45 45
Lines 1898 1896 -2
===========================================
- Hits 1898 1895 -3
- Misses 0 1 +1
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.
| if title != "Success": | ||
| raise GarthException(f"Unexpected title: {title}") | ||
| raise GarthException("MFA verification failed") | ||
| return _complete_login(client) |
There was a problem hiding this comment.
Incorrect message for non-MFA login failures.
If credentials are wrong (no MFA), this raises “MFA verification failed,” which is misleading. Only use that message when MFA was actually attempted.
Apply this diff within login():
- if title != "Success":
- raise GarthException("MFA verification failed")
+ if title != "Success":
+ raise GarthException(
+ "MFA verification failed" if mfa_attempted else f"Login failed: {title}"
+ )And add the supporting state (outside the selected range):
# right after: title = get_title(client.last_resp.text)
mfa_attempted = False# just before calling handle_mfa(...)
mfa_attempted = True🤖 Prompt for AI Agents
In src/garth/sso.py around lines 129-131, the code raises "MFA verification
failed" for any non-success title which is misleading for non-MFA failures; add
a boolean mfa_attempted flag initialized to False immediately after title =
get_title(client.last_resp.text), set mfa_attempted = True just before calling
handle_mfa(...), and change the error branch so that if title != "Success" you
raise a specific MFA error only when mfa_attempted is True, otherwise raise an
invalid-credentials (or generic login failure) error; leave the return
_complete_login(client) as-is.
1 participant
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'
Summary by CodeRabbit