MFA improvements
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
- Bug Fixes
- Standardized MFA failure handling with a clear, consistent error message and stricter validation after MFA.
- Refactor
- Updated default HTTP retry policy to no longer retry on rate-limited responses, reducing unnecessary delays while preserving retries for transient server errors.
- Chores
- Bumped version to 0.5.18.
Walkthrough
Removes 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
| Cohort / File(s) | Summary of Changes |
|---|---|
HTTP retry defaultssrc/garth/http.py, tests/test_http.py |
Remove 429 from Client.status_forcelist default; tests updated to expect (408, 500, 502, 503, 504) and to verify configure()-based overrides propagate to the adapter. |
SSO MFA validationsrc/garth/sso.py |
Add post-MFA and final-login HTML title checks; raise GarthException("MFA verification failed") when title != "Success"; standardize error messaging. |
Version bumpsrc/garth/version.py |
Increment __version__ from "0.5.17" to "0.5.18". |
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
- matin/garth#92 — Also modifies sso.login MFA handling and resume flow; overlaps in post-MFA control logic.
- matin/garth#84 — Adds return_on_mfa/resume_login in sso.py; interacts with the same MFA/login pathway updated here.
- matin/garth#119 — Adjusts MFA control flow and error handling in sso.py; closely related to the new title checks and standardized errors.
Suggested reviewers
- felipao-mx
✨ Finishing Touches
- [ ] 📝 Generate Docstrings
🧪 Generate unit tests
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
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 @coderabbitai help to get the list of available commands and usage tips.
Codecov Report
:x: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 99.94%. Comparing base (4b027e1) to head (5627eaa).
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/garth/sso.py | 75.00% | 1 Missing :warning: |
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
| Flag | Coverage Δ | |
|---|---|---|
| unittests | 99.94% <85.71%> (-0.06%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.