Migrate SSO to mobile JSON API, enable telemetry by default#200
Merged
Conversation
Garmin has migrated their SSO authentication endpoints. The old HTML
form-based flow (/sso/embed, /sso/signin with CSRF tokens) is being
replaced by a mobile JSON API flow (/mobile/api/login,
/mobile/api/mfa/verifyCode). Some users were hitting 403 "Client app
validation failed" errors on OAuth2 token refresh.
Changes:
- Rewrite login() to use /mobile/api/login JSON endpoint
- Rewrite handle_mfa() to use /mobile/api/mfa/verifyCode JSON endpoint
- Update get_oauth1_token() login-url to mobile.integration.{domain}
- Add audience parameter to OAuth2 exchange
- Add _parse_sso_response() for structured SSO error messages
- Remove CSRF token and HTML title parsing (no longer needed)
- Update User-Agent to GCM-iOS-5.22.1.4
- Fix logfire warning when telemetry enabled but not configured
- Re-record all login-related VCR cassettes
Closes #198
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Telemetry is now on by default so auth issues can be diagnosed from logfire logs. Each session gets a unique session_id that users can share when reporting issues. Changes: - Default telemetry to enabled with built-in logfire write token - Use pydantic-settings (GARTH_TELEMETRY_ prefix) for config - Use logfire.configure(local=True) to avoid overwriting host app config - Include session_id in every log entry for per-session tracking - Move logfire + pydantic-settings to core dependencies - Remove service_name from public API (hardcoded to "garth") - Update auth issue template to ask for session ID - Disable telemetry in test conftest to prevent network requests Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Users need their session_id to share when reporting auth issues. Print it once at Client init so it's visible without extra code. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. 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 (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors telemetry into a Pydantic BaseSettings model (enabled by default with session_id), rewrites SSO login to a JSON mobile API flow with explicit MFA ticket handling, updates telemetry wiring in the HTTP client, upgrades dependencies (pydantic, logfire), updates docs/templates, and bumps version to 0.7.0. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SSO_Mobile_API as MobileAPI
participant OAuth_Service as OAuthServer
participant Telemetry
Client->>MobileAPI: POST /mobile/api/login (email,password,clientId,serviceUrl)
MobileAPI-->>Client: {type: "SUCCESSFUL", serviceTicketId}
Client->>OAuth_Server: GET /oauth/preauthorized?ticket=serviceTicketId
OAuth_Server-->>Client: 200 + OAuth1 token
Client->>OAuth_Server: POST /oauth2/exchange (audience, oauth1_token[, mfa_token])
OAuth_Server-->>Client: OAuth2 token
Note over Client,Telemetry: Telemetry.session_id attached to request/response payloads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
===========================================
- Coverage 100.00% 99.97% -0.03%
===========================================
Files 68 68
Lines 3508 3492 -16
===========================================
- Hits 3508 3491 -17
- 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:
|
- Wrap raise_for_status() in handle_mfa() with GarthHTTPError for consistent exception types across all SSO functions - Bump version to 0.7.0 (breaking: pydantic>=2.0 required, logfire now a core dependency, removed [telemetry] extra) - Add telemetry to README features - Sync uv.lock Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
![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: 4
🤖 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/http.py`:
- Around line 48-49: Unconditional printing of the telemetry session in the
client initializer (the block checking self.telemetry.enabled and printing
telemetry.session_id) should be removed to avoid spurious stdout during
import/Client() construction; replace the print with a debug log using a module
logger (e.g., logger = logging.getLogger(__name__) and logger.debug(f"Garth
session: {self.telemetry.session_id}")) or eliminate the emission entirely so no
stdout is produced during normal initialization.
In `@src/garth/sso.py`:
- Around line 87-91: The bootstrap GET to
client.sess.get(f"{sso_base}/mobile/sso/en/sign-in", params={"clientId":
CLIENT_ID}, timeout=client.timeout) must be validated: after the request, check
the response.status_code and if it's not in the 2xx range, log or raise an
explicit error including response.status_code and response.text (and the
sso_base/CLIENT_ID context) and abort before proceeding to post credentials so
bootstrap failures are surfaced immediately instead of masking them as a later
generic login failure.
🪄 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: 08f22288-da20-4041-a40b-ec1b2b3187fe
⛔ Files ignored due to path filters (9)
tests/cassettes/test_login_command.yamlis excluded by!tests/**/cassettes/**tests/cassettes/test_login_email_password_fail.yamlis excluded by!tests/**/cassettes/**tests/cassettes/test_login_mfa_fail.yamlis excluded by!tests/**/cassettes/**tests/cassettes/test_login_return_on_mfa.yamlis excluded by!tests/**/cassettes/**tests/cassettes/test_login_success.yamlis excluded by!tests/**/cassettes/**tests/cassettes/test_login_success_mfa.yamlis excluded by!tests/**/cassettes/**tests/cassettes/test_login_success_mfa_async.yamlis excluded by!tests/**/cassettes/**tests/cassettes/test_resume_login.yamlis excluded by!tests/**/cassettes/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/ISSUE_TEMPLATE/auth-issue.ymlREADME.mddocs/telemetry.mdpyproject.tomlsrc/garth/http.pysrc/garth/sso.pysrc/garth/telemetry.pysrc/garth/version.pytests/conftest.pytests/test_http.pytests/test_sso.pytests/test_telemetry.py
src/garth/sso.py
Outdated
Comment on lines
87
to
91
| client.sess.get( | ||
| f"{sso_base}/mobile/sso/en/sign-in", | ||
| params={"clientId": CLIENT_ID}, | ||
| timeout=client.timeout, | ||
| ) |
Contributor
There was a problem hiding this comment.
Check the bootstrap request before posting credentials.
The initial /mobile/sso/en/sign-in call is now a required cookie/bootstrap step, but its status is ignored. If that request starts returning 4xx/5xx, the code falls through to a later generic login failure, which makes auth regressions much harder to diagnose.
Suggested fix
- client.sess.get(
+ bootstrap = client.sess.get(
f"{sso_base}/mobile/sso/en/sign-in",
params={"clientId": CLIENT_ID},
timeout=client.timeout,
)
+ try:
+ bootstrap.raise_for_status()
+ except HTTPError as e:
+ raise GarthHTTPError(msg="SSO bootstrap failed", error=e)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| client.sess.get( | |
| f"{sso_base}/mobile/sso/en/sign-in", | |
| params={"clientId": CLIENT_ID}, | |
| timeout=client.timeout, | |
| ) | |
| bootstrap = client.sess.get( | |
| f"{sso_base}/mobile/sso/en/sign-in", | |
| params={"clientId": CLIENT_ID}, | |
| timeout=client.timeout, | |
| ) | |
| try: | |
| bootstrap.raise_for_status() | |
| except HTTPError as e: | |
| raise GarthHTTPError(msg="SSO bootstrap failed", error=e) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/garth/sso.py` around lines 87 - 91, The bootstrap GET to
client.sess.get(f"{sso_base}/mobile/sso/en/sign-in", params={"clientId":
CLIENT_ID}, timeout=client.timeout) must be validated: after the request, check
the response.status_code and if it's not in the 2xx range, log or raise an
explicit error including response.status_code and response.text (and the
sso_base/CLIENT_ID context) and abort before proceeding to post credentials so
bootstrap failures are surfaced immediately instead of masking them as a later
generic login failure.
- Replace magic strings with SSO_SUCCESSFUL/SSO_MFA_REQUIRED constants - Consolidate duplicate USER_AGENT (single def in http.py, ref from sso.py) - Remove unused MAX_WORKERS duplicate from body_battery/events.py - Use client.get()/client.post() in SSO instead of client.sess directly (consistent error handling, sets last_resp) - Replace print() with logger.info() for session ID - Add bootstrap GET validation - Add test for unexpected SSO response type (100% sso.py coverage) - Add Philosophy section to CLAUDE.md - Add CodeRabbit config to allow embedded logfire write token - Document why telemetry is on by default (Garmin SSO migration context) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
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
1 participant
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
/mobile/api/login,/mobile/api/mfa/verifyCode), fixing 403 "Client app validation failed" errors on OAuth2 token refreshBreaking changes
pydantic>=2.0.0(drops v1 support)logfireandpydantic-settingsare now core dependenciesgarth[telemetry]extra removed (telemetry is always available)telemetry_service_nameparameter removed fromconfigure()return_on_mfaclient state dict now haslogin_paramskey instead ofsignin_paramsTest plan
Closes #198
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores