fix(local-dev): complete Google sign-in against the Auth emulator on LAN - #12453
fix(local-dev): complete Google sign-in against the Auth emulator on LAN#12453formed2forge wants to merge 6 commits into
Conversation
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks @formed2forge — this is a genuinely well-built piece of local-dev tooling, and the failure-class doc plus tests on both sides make it easy to review. The design (skip Safari, complete PKCE against the Auth emulator with response_mode=json) is fail-closed where it matters. I'm requesting changes for three concrete items, none of which are structural.
What I verified as solid
backend/routers/auth.py—response_mode=jsonis rejected with a 400 whenFIREBASE_AUTH_EMULATOR_HOSTis unset, the redirect-uri allowlist (_validate_redirect_uri) still runs before any code is minted, and_auth_code_data_from_sessionkeeps the emulator code PKCE-bound._generate_emulator_custom_tokenis only reachable when the stored credentials carry"emulator": true, which only the env-gated path sets — a real Google ID-token exchange can't accidentally route to it.app/lib/services/auth_service.dart— the extract of_launchAuthorizationAndWaitForCallbackis an improvement on the old inline flow (thefinallynow always cancels the app_links subscription and clears the method-channel handler), and the external-browser launch for plain-HTTP URLs is correctly commented._completeEmulatorAuthorizationfails closed on a missing code.backend/tests/unit/test_auth_redirect_uri.py— the newTestEmulatorAuthorizetests cover the fail-closed json-mode rejection, Google redirect not called, and the emulator custom-token mint; the autouse_clear_auth_emulator_hostfixture keeps the env var from leaking into the PKCE suite.app/test/unit/emulator_authorize_callback_test.dartcovers URI building, callback rebuild, and the no-code failure.
Requested changes
- Formatting check is failing —
dart format --line-length 120wants to reformat 2 of the 3 changed.dartfiles (CI pins Flutter 3.44.5; a local formatter version may produce different output). Please rundart formaton the changed files and push. - Docs now contradict the harness and setup script on Tailscale —
docs/doc/developer/AppSetup.mdxandapp/README.mdnarrowOMI_DEV_HOSTto "LAN (RFC 1918)" and drop Tailscale, butscripts/dev-harness/dev_harness/safety.pystill deliberately accepts CGNAT100.64.0.0/10(its comment cites #11730/#11652 as the documented device path), andapp/setup.shstill warns to set "LAN or Tailscale address". Your ENETUNREACH was the phone not being on the tailnet, not a harness restriction. Please either keep Tailscale documented as supported or narrow all three places consistently. app/ios/Runner/Info-Dev.plist— strayAppGroupIdentifieraddition —$(APP_GROUP_IDENTIFIER)isn't defined anywhere inproject.pbxproj, no code reads this plist key (BatteryWidget hardcodes the group id inSharedDefaults.swift), andgenerate_ios_dev_info_plist.shregenerates the file fromInfo.plistand would silently drop the key. It looks like it drifted in from another branch. Please remove it or explain what consumes it. (TheNSLocalNetworkUsageDescriptionaddition is correct and needed for the iOS local-network prompt — good catch adding it to both the plist and the generator so it survives regeneration, with coverage inios_dev_ats_config_test.sh.)
Notes for maintainers
- The emulator branch in
auth_authorizetriggers for every authorize client whenFIREBASE_AUTH_EMULATOR_HOSTis set — an operator who sets that var in a real deployment silently converts all Google/Apple sign-ins into unauthenticated emulator logins (local-googleuid). That matches how the rest of the Firebase emulator harness behaves, but it's worth a maintainer's explicit sign-off on an auth-surface change of this shape. There's also a duplicated_log_auth_eventpair (auth_code_created+authorize_redirect_created) in the emulator branch — harmless, but intentional? - The one-line
app/AGENTS.mdaddition under Auth Methods documents this flow for coding agents; it's accurate against this diff, but it encodes this PR's design — if #11784's separate-button design lands instead, that line needs rewording. The PR description itself flags the #11784 overlap; the two shouldn't both land, so that's a direction call for a maintainer. .github/failure-classes/FC-emulator-authorize-opens-browser.jsonmatches the established failure-class schema nicely.
Human maintainer sign-off requested on the auth-surface gating and the choice between this and #11784; the three requested changes above are mechanical and don't need that discussion to resolve.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
CI note on the red Backend unit suite run for this head (run 33350310129): it is not caused by this PR. The only failing file is None of the three items from the earlier review are affected by this — they stand as listed (formatting, Tailscale docs consistency, the stray by AI on behalf of David. |
fbd9bf1 to
43aebbc
Compare
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks @formed2forge — re-checked after the rebase onto newer main (43aebbc). One thing improved on its own: the red Backend unit suite run cleared exactly as predicted once the chat-gateway stub fix from main was picked up by the rebase, and the hermetic/Dart gates are green on this head. Your own auth tests were green throughout.
The three items from the earlier review are still open on this head, though — none of them structural:
-
Formatting still red on this head — the
Formattingcheck (run 33649065643) fails withdart format --line-length 120 --set-exit-if-changednamingapp/lib/providers/auth_provider.dartandapp/lib/services/auth_service.dart— the same two files as before (likely a local formatter version differing from the pinned CI toolchain).app/test/unit/emulator_authorize_callback_test.dartpasses formatting fine. -
Docs still contradict the harness and setup script on Tailscale —
docs/doc/developer/AppSetup.mdxandapp/README.mdnarrowOMI_DEV_HOSTto "LAN (RFC 1918)", but on this basescripts/dev-harness/dev_harness/safety.pystill deliberately accepts CGNAT100.64.0.0/10(its comment cites #11730/#11652/#11782 as the documented device path) andapp/setup.shstill tells developers to set the "LAN or Tailscale address". Either keep Tailscale documented as supported or narrow all three places together. -
AppGroupIdentifierstill drifts inapp/ios/Runner/Info-Dev.plist—$(APP_GROUP_IDENTIFIER)is defined nowhere: zero occurrences inproject.pbxprojand none repo-wide (code search), andgenerate_ios_dev_info_plist.shregenerates this file fromInfo.plist, which doesn't carry the key — so it would silently disappear on the next regeneration anyway. Please drop it or point at what consumes it. (NSLocalNetworkUsageDescriptionis the opposite and done right: added in the plist, the generator, and asserted byios_dev_ats_config_test.sh.)
What I re-verified as solid on this head, for the record:
backend/routers/auth.py—response_mode=jsonstill 400s whenFIREBASE_AUTH_EMULATOR_HOSTis unset;_validate_redirect_uristill runs before any code is minted; emulator codes stay PKCE-bound via_auth_code_data_from_session;_generate_emulator_custom_tokenrequires both the stored"emulator": truecredentials and the env flag, and those credentials are server-stored with the auth code, not client-submitted. (Tiny nit, still present: the emulator branch logsauth_code_createdandauthorize_redirect_createdback-to-back.)app/lib/services/auth_service.dart—_completeEmulatorAuthorizationfails closed on non-200 and on a missing code, and_signInWithOAuthCredentialsnow throws rather than silently returning when the emulator response lacks a custom token. The_launchAuthorizationAndWaitForCallbackextract still cleans up the app_links subscription and method-channel handler infinally.backend/tests/unit/test_auth_redirect_uri.py—TestEmulatorAuthorizestill covers json-mode rejection, app-scheme redirect (Google not called), and the custom-token mint; the autouse_clear_auth_emulator_hostfixture keeps the env var out of the PKCE suite.app/AGENTS.md— the new Web OAuth/USE_WEB_AUTHline matches this diff exactly (local_dev completes against the emulator, no Safari), so agents reading it get an accurate picture..github/failure-classes/FC-emulator-authorize-opens-browser.json— the stated contract matches the implemented behavior on both ends.
For maintainers: #11784 (separate "Sign in (local dev)" button + out-of-band remint) is still open from the same author, and the emulator branch in auth_authorize still applies to every authorize client whenever FIREBASE_AUTH_EMULATOR_HOST is set — a deployment that sets that var silently converts all Google/Apple sign-ins into emulator logins. The choice between the two designs, and explicit sign-off on that auth-surface shape, needs a human maintainer before merge.
by AI on behalf of David.
|
All three review items addressed: Dart/Python formatting (reverted to Flutter 3.44.5 pinned toolchain style), stray |
|
Formatting is now clean — the local dart 3.13.2 and CI's Flutter 3.44.5 bundled dart disagree on the indentation of a multi-line |
Dismissed: all three blocking items (Formatting check red, Tailscale docs contradiction, stray AppGroupIdentifier in Info-Dev.plist) are resolved on head 8ced740 — Formatting check green, docs consistent with the harness CGNAT acceptance again, and the key removed.
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks @formed2forge — re-reviewed on 8ced740. All three items from the last review are resolved, and I've dismissed the outstanding change requests:
- Formatting — green on this head; extracting the l10n lookup into
authFailedMsginapp/lib/providers/auth_provider.dartwas the right fix for the dart 3.13.2 / CI 3.44.5??-indentation divergence, and it reads better anyway. - Tailscale docs —
docs/doc/developer/AppSetup.mdxandapp/README.mdnow say "LAN or Tailscale (RFC 1918 / CGNAT)", consistent again withscripts/dev-harness/dev_harness/safety.py(which deliberately accepts100.64.0.0/10) andapp/setup.sh's "LAN or Tailscale address" guidance. AppGroupIdentifier— gone; zero occurrences inapp/ios/Runner/Info-Dev.pliston this head.NSLocalNetworkUsageDescriptionremains done right: declared in the plist, re-added bygenerate_ios_dev_info_plist.sh, and asserted byios_dev_ats_config_test.sh.
Re-verified the rest on this head, for the record:
backend/routers/auth.py—response_mode=jsonstill 400s whenFIREBASE_AUTH_EMULATOR_HOSTis unset;_validate_redirect_uristill runs before any code is minted; emulator codes stay PKCE-bound via_auth_code_data_from_session; the emulator custom-token mint requires both the server-stored"emulator": truecredentials and the env flag, so a real Google exchange can't route to it.app/lib/services/auth_service.dart—_completeEmulatorAuthorizationfails closed on non-200 and on a missing code; the extracted_launchAuthorizationAndWaitForCallbacknow cancels the app_links subscription and clears the method-channel handler infinally(an improvement over the old inline flow); only plain-http://authorize URLs switch toLaunchMode.externalApplication, with a comment explaining the SFSafariViewController/ATS reason.- Tests on both sides hold up:
TestEmulatorAuthorizeinbackend/tests/unit/test_auth_redirect_uri.pycovers the fail-closed json rejection, Google-redirect-not-called, and the emulator token mint, with the autouse_clear_auth_emulator_hostfixture keeping the env var out of the PKCE suite;app/test/unit/emulator_authorize_callback_test.dartcovers URI building, callback rebuild, and the no-code failure. .github/failure-classes/FC-emulator-authorize-opens-browser.jsonmatches the registry schema and the behavior actually implemented on both ends.app/AGENTS.md— the new Web OAuth/USE_WEB_AUTHline is accurate (the env flag is real, and onlylocal_devsets the emulator profile flag), so agents working inapp/get a correct picture of the local-dev auth flow.
Two things still need a human maintainer before merge, same as before — neither is a code defect:
- #11784 (separate "Sign in (local dev)" button + out-of-band remint) is still open from the same author and covers the same user outcome; a maintainer needs to pick the design so both don't land.
- The emulator branch in
auth_authorizeapplies to every authorize client wheneverFIREBASE_AUTH_EMULATOR_HOSTis set — sign-off on that auth-surface shape (vs. a narrower opt-in) is a security/product decision.
That's why security-review and needs-maintainer-review stay; I've dropped docs-accuracy since the doc contradiction is fixed. Thanks for the quick, complete turnaround on all three items.
by AI on behalf of David.
The offline harness has no Google client, and iOS will not render the plain-HTTP authorize page. local_dev now finishes PKCE over JSON and signs in with an emulator custom token, so a physical phone on the Mac's LAN can tap Google without Safari. Failure-Class: new Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Use the already-validated redirect_uri and state instead of the mixed session dict so the pre-push typecheck can pass. Failure-Class: new Co-authored-by: Cursor <cursoragent@cursor.com>
…, Tailscale docs - Dart formatting: revert constructor-initialiser and map-literal indentation in auth_service.dart and auth_provider.dart to Flutter 3.44.5 (Dart) style; the PR was authored with a newer formatter that produces a different shape for `:` lists and `=>` map literals, and for the long ternary in auth_provider.dart. - Remove stray `AppGroupIdentifier`/`$(APP_GROUP_IDENTIFIER)` key-value pair from app/ios/Runner/Info-Dev.plist; the variable is undefined in project.pbxproj and would be silently dropped on the next generate_ios_dev_info_plist.sh regeneration. - Restore Tailscale as a documented OMI_DEV_HOST option in docs/doc/developer/AppSetup.mdx and app/README.md to match scripts/dev-harness/dev_harness/safety.py (which accepts CGNAT 100.64.0.0/10) and app/setup.sh (which still says "LAN or Tailscale address"); the original narrowing to RFC 1918-only was inconsistent across the three places. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5TqoWB9unzNKthnD1pJUP
8ced740 to
12bb5ef
Compare
|
Rebased onto BasedHardware/omi
Was DIRTY; now MERGEABLE. Conflict in auth_service.dart kept the LAN Google-button emulator path. Independent of #11784. |
|
Re-reviewed on the rebased head
The red Backend unit suite on this head is not from this PR: What's left is genuinely human: sign-off on the emulator completion semantics (a stable by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Summary
local_devGoogle sign-in on a physical iPhone no longer opens Safari against plain-HTTP/v1/auth/authorize(blank page; offline harness has no Google client). The app asks forresponse_mode=json, exchanges the PKCE code, and signs in with an Auth-emulator custom token.OMI_DEV_HOSTfor bothsetup.shandmake dev-up. iOS now prompts for local-network access (NSLocalNetworkUsageDescription). Two Home Screen icons can both be named Omi Dev.feat/local-dev-emulator-auth) adds a separate "Sign in (local dev)" button and out-of-band remint. This PR keeps the Google button, which is what a newcomer taps. Do not land both blindly.Failure-Class: new
The new class is: when
FIREBASE_AUTH_EMULATOR_HOSTis set,/v1/auth/authorizemust complete PKCE against the emulator (JSON or app-scheme redirect) instead of redirecting to Google/Apple. A production deployment without the emulator must rejectresponse_mode=json.Test plan
backend/.venvpytesttests/unit/test_auth_redirect_uri.py— 65 passed (JSON mode without emulator fails closed; emulator mints a code and custom token; Google redirect is not called)flutter test test/unit/emulator_authorize_callback_test.dart— 3 passedapp/test/shell/ios_dev_ats_config_test.sh— ArbitraryLoads kept, LocalNetworking not combined, usage description present00008150-001C3DAC0A38401C):OMI_DEV_HOST=192.168.2.200, harness bound, onboarding GET 200, Google sign-in completed without Safari. Tailscale100.81.134.49wasENETUNREACH(phone not on the tailnet)./tmptree):npm install mintlify;docs/npm cisucceeds.mintlify broken-linksdoes not reportAppSetup.mdx.npm run buildstill fails on missingrollout/(pre-existing on main).mintlify broken-linksstill hits a pre-existing parse error indoc/hardware/omiglass/monitoring.mdx.This commit was not re-run on the phone against current
main; the live path was exercised on the same authorize-JSON + LAN host combination before the slice was ported ontoupstream/main.