Use dynamic MFA method from login response#215
Merged
matin merged 1 commit intomatin:mainfrom Mar 19, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughWhen MFA is required during login, the code extracts the last-used MFA method from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SSO as SSO Service
participant MFA as MFA Provider
participant Auth as Auth Server
Client->>SSO: login(credentials)
SSO-->>Client: needs_mfa(mfa_method)
Client->>SSO: submit_mfa(code, mfa_method)
SSO->>MFA: verify(code, mfa_method)
MFA-->>SSO: verification_result
SSO->>Auth: exchange_for_token(verification_result)
Auth-->>SSO: tokens
SSO-->>Client: tokens
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)
📝 Coding Plan
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/garth/sso.py`:
- Around line 136-137: mfa_method currently uses
mfa_info.get("mfaLastMethodUsed", "email") which only uses the default when the
key is missing, so falsy values like None or "" get preserved; change both
occurrences to coerce falsy values to "email" (e.g. set mfa_method =
mfa_info.get("mfaLastMethodUsed") or "email") and ensure that same sanitized
mfa_method is what you store in client_state and send to
/mobile/api/mfa/verifyCode so null/empty strings never get posted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc9cbd9a-0ef1-4f0c-a9b3-a82d6f2d8d7b
📒 Files selected for processing (1)
src/garth/sso.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/garth/sso.py (1)
205-221:⚠️ Potential issue | 🟡 MinorNormalize falsy
mfa_methodvalues at the verify boundary.Line 245 can still forward persisted
None/""values, and Line 221 posts them verbatim. That leaves the oldMFA_CODE_INVALIDpath open for resumed MFA sessions and any direct caller that passes a falsy method.🛠️ Proposed fix
def handle_mfa( client: http.Client, login_params: dict, prompt_mfa: Callable, *, mfa_method: str = "email", ) -> str: + mfa_method = mfa_method or "email" if inspect.iscoroutinefunction(prompt_mfa): mfa_code = asyncio.run(prompt_mfa()) @@ - mfa_method = client_state.get("mfa_method", "email") + mfa_method = client_state.get("mfa_method") or "email"Also applies to: 245-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/sso.py` around lines 205 - 221, The handler handle_mfa currently forwards falsy mfa_method values (None or empty string) to the verify API; normalize the value before sending by replacing any falsy mfa_method with the default "email" (e.g., mfa_method = mfa_method or "email") and use that normalized variable in the client.post JSON payload; apply the same normalization to the other MFA call block referenced (the one around the later verify/submit code at the 245-248 area) so no direct caller or resumed session can send a falsy mfa_method to the server.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/garth/sso.py`:
- Around line 205-221: The handler handle_mfa currently forwards falsy
mfa_method values (None or empty string) to the verify API; normalize the value
before sending by replacing any falsy mfa_method with the default "email" (e.g.,
mfa_method = mfa_method or "email") and use that normalized variable in the
client.post JSON payload; apply the same normalization to the other MFA call
block referenced (the one around the later verify/submit code at the 245-248
area) so no direct caller or resumed session can send a falsy mfa_method to the
server.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41874b04-c766-4c48-ae95-519ee8db5aeb
📒 Files selected for processing (1)
src/garth/sso.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #215 +/- ##
=======================================
Coverage 99.91% 99.91%
=======================================
Files 68 68
Lines 3566 3569 +3
=======================================
+ Hits 3563 3566 +3
Misses 3 3
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:
|
Owner
|
@tobias-goertz looks good. can you bump the version? I'll merge and release thanks for the contribution! |
The MFA verification endpoint requires the correct mfaMethod value
("email", "phone", etc.) but it was hardcoded to "email". Users with
SMS/phone MFA got MFA_CODE_INVALID errors.
Read mfaLastMethodUsed from the login response's customerMfaInfo and
pass it through to handle_mfa() and resume_login().
Coerce falsy mfaLastMethodUsed to "email"
If the key exists but is null/empty, .get() returns None instead of
the default. Use `or "email"` to handle that case.
bump version
tobias-goertz
force-pushed
the
fix/dynamic-mfa-method
branch
from
fa5df3e to
e9edcf6
Compare
Contributor
Author
|
@matin done, thanks! |
matin
added a commit
that referenced
this pull request
Mar 19, 2026
PR #215 re-introduced browser headers on the login POST and MFA verify endpoints. These are mobile API endpoints that expect the default GCM-iOS user agent — browser headers cause 401/429 from Garmin. Same regression as 0.7.7, fixed in 0.7.8, re-introduced in 0.7.10. Browser headers should ONLY be on page requests (sign-in, embed). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mfaLastMethodUsedfrom the login response (customerMfaInfo) instead of hardcoding"email"handle_mfa()andresume_login()MFA_CODE_INVALIDbecause the verify endpoint received"email"instead of"phone"Changes
login(): extractmfaLastMethodUsedfrom response, include inreturn_on_mfastatehandle_mfa(): acceptmfa_methodkeyword argument (default"email"for backwards compat)resume_login(): forwardmfa_methodfromclient_stateTest plan
MFA_CODE_INVALID, now worksSummary by CodeRabbit
Improvements
Chores