garth icon indicating copy to clipboard operation
garth copied to clipboard

MFA improvements

Open cyberjunky opened this issue 4 months ago • 2 comments

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.

cyberjunky avatar Sep 06 '25 07:09 cyberjunky

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 defaults
src/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 validation
src/garth/sso.py
Add post-MFA and final-login HTML title checks; raise GarthException("MFA verification failed") when title != "Success"; standardize error messaging.
Version bump
src/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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 06 '25 07:09 coderabbitai[bot]

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.

codecov[bot] avatar Sep 06 '25 07:09 codecov[bot]