fix(sso): launch the SSO browser through the launcher abstraction - #964
Open
sbldevnet wants to merge 3 commits into
Open
fix(sso): launch the SSO browser through the launcher abstraction#964sbldevnet wants to merge 3 commits into
sbldevnet wants to merge 3 commits into
Conversation
The device-code flow executed the browser binary with the URL as a bare argument. Safari resolves that as a file path relative to its sandbox container, so the login page never opened. Route the SSO flow through launcher.ForBrowser, which the console path already had, and fold its two diverged copies into it. Fixes #602
Both packages assert with testify elsewhere; the new tests were written in the style of pkg/launcher, which does not. pkg/launcher is left alone so it stays internally consistent.
The tests hardcoded 'open', which only holds on macOS. On Linux browser.OpenCommand resolves to xdg-open, so the Safari and Arc cases failed in CI. Take the command from OpenCommand: what these assert is the '-a <App>' shape, not which system handler runs it. Also trims the comments in the new tests.
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
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.
Fixes #602
The bug
The device-code login flow executed the configured browser binary with the URL
as a bare argument:
Safari does not parse a URL from
argv. It resolves the argument as a filepath relative to its sandbox home, so users got a tab pointing at
~/Library/Containers/com.apple.Safari/Data/https:/…and the login nevercompleted.
exec.Commandsucceeded, so Granted reported no error.Granted already knew how to launch Safari correctly —
open -a Safari <url>—but only on the console path. The browser-key switch existed in two diverged
copies (
pkg/assume/assume.go,pkg/granted/console.go) and the SSO path hadno dispatch at all.
This is not an unusual configuration. Selecting Safari in
granted browser set-ssostores exactly this path, so following the documented setup is enoughto end up with a broken SSO login.
The fix
Factor the switch into
launcher.ForBrowserand route all three call sitesthrough it. The
Launcherinterface moves frompkg/assumetopkg/launcherso
pkg/idclogincan use it without importing the assume command.Browser identification goes through
browser.GetBrowserKey, which now matchesonly the executable's base name. It compares with unanchored substrings — which
is what lets one implementation handle a macOS bundle ID, a Linux
.desktopfile and a Windows ProgId — but the directory components of a path are chosen by
the user. Without the base name restriction,
/Users/marcus/bin/browsermatches"arc" and launches Arc. This also means browsers installed outside their default
location are now recognised (
/opt/homebrew/bin/firefox).ChromeProfilenow omits--profile-directorywhen no profile is given. Boththe SSO flow and
granted consolelaunch without one, and an empty--profile-directory=changes which profile the browser opens.Compatibility
Paths that Granted does not recognise keep being executed directly, which is the
behaviour those configurations already have. Only browsers Granted recognises —
and currently mishandles — change.
Behaviour changes on the console path
Both console commands shared the switch, so unifying it fixes two gaps in the
granted consolecopy:ChromeProfilenow receives theBrowserTypeit was missing, so Chromeprofile lookup no longer silently no-ops.
launchers instead of falling through to
open. Previously, configuring Arc inGranted still opened whatever the OS default browser was.
These warrant a release note.
Testing
ForBrowserargv per browser key,GetBrowserKeyacross allthree input domains (display names, vendor identifiers, executable paths),
ChromeProfileprofile resolution against a fakeLocal State, and theidcloginwiring.emits
open -a Safari <url>and the login completes. Chrome, an unrecognisedbinary, the launch-template path and the unconfigured fallback were each
checked against the debug launch command.