Ask about SSO only where the answer would change anything - #1217
Open
ericmj wants to merge 11 commits into
Open
Conversation
An organization that requires SSO expires its members' access on a clock it sets, and when that lapses the token grant stops carrying the organization's scope and says which ones it dropped for that reason. The first thing the client can do with that is ask. Asking once for the batch is the part that takes work. Every private organization a resolution can need is named in the project's own dependencies, since a published package's dependencies come from the public repository or from its own organization, so the needed set is known before any fetch. Intersecting it with what the server flagged is what makes the question "acme requires SSO authentication. Authenticate now?" instead of a 403 halfway through, and it is why a member of ten SSO organizations who depends on two is asked about two. Saying yes gets a URL bound to this session, so the session and its refresh token survive; the refresh afterwards is what picks up the scopes, since the access token that lacks them has not expired and would otherwise be used for another half hour. Saying no says what it costs and continues. The flag is stored with the token rather than kept for the run. It is learned at refresh, and a later run inside the access token's 30 minutes does not refresh, so keeping it in memory would mean the second mix deps.get 403s with nothing to explain it. CI is untouched: it authenticates as the organization, which is never governed and never flagged.
Adversarial review of the prompt found four ways it fires or fails badly. It was not gated on whether the resolution uses the stored token at all. An organization authenticated with its own key, or a build running with HEX_REPOS_KEY, fetches without ever touching it, so the prompt asked about an organization that was already working. check_and_refresh_auth one line up had that gate; this now uses the same one. Offline and HEX_API_KEY both say so instead of asking. Offline, the flow would go to the network for a URL it cannot use. With HEX_API_KEY set, the request authenticates as the key rather than as the session that needs authorizing, so the URL would come back bound to the wrong thing and the flag would never clear. A verification_uri that is not an http URL threw out of open_browser and took the resolution with it, after the user had already answered yes, and a "message" that was an object rather than a string raised on interpolation. Opening a browser is a convenience on top of a printed URL, so it no longer ends anything. The URL is in the prompt rather than beside it. Mix.Shell.Quiet drops info output and keeps prompts, so under --quiet the flow asked people to finish in a browser without telling them where.
All 30 conflicts were in the vendored hex_core files: both sides had revendored from different hex_core commits (766ae61 on this branch, cf6a12c on main). Resolved by merging hex_core main into hex_core's organization-sso-reauth branch (a6e8a52) and re-running scripts/vendor_hex_core.sh against it, no manual edits to vendored files. auth.ex and remote_converger.ex were touched by both sides and auto-merged cleanly.
A stale release_docs.sh artifact swept into d111676 by a careless add. Nothing references it.
The prefetch walk that decides whether authentication is worth refreshing and the one that decides which organizations to ask about now share one pass that dedupes repositories before the config lookup, so a project with two hundred packages from one organization does one lookup rather than two hundred. Persisting a refreshed token carries the flagged organizations over instead of dropping and rewriting them, which removes the second config write on every steady-state refresh; one token_map/4 builds the stored shape everywhere, and the raw device-flow map is normalized before storage so an empty flag list is dropped the same way the callback path drops it. The SSO page opens through Hex.Utils.system_open/1 like hex.docs does, so the test stub and WSL fallback apply and the hex_core passthrough is gone. Deauth clears the local token once, revocation is remote-only, the repo token expiry check reuses the vendored predicate instead of restating the 300-second buffer, Hex.OAuth.get_token/0 and the unused auth opts go, and the API-key notice no longer names an env var the key may not have come from. hex_core revendored at eb5508a.
hex.config holds the OAuth access and refresh tokens and was written with no mode, so under the default umask it landed world readable and any local user could mint access tokens from the refresh token. It is written 0600 in a 0700 home. The Windows opener dropped the empty title argument that start expects and escaped only &, while the SSO verification URI it now opens comes from the server. It passes the title and escapes what cmd.exe acts on. Config writes were an unlocked read-merge-write of the whole file, and the two callers hold different locks in hex_core, so a repository token written concurrently with a global one could drop it. Every write takes one lock. With HEX_API_KEY set, SSO renewal was refused for a reason that is not true: repository requests never use the API key, they fall through to the stored session, so those fetches kept a lapsed session with no way to renew it. The re-authorization request resolves the session explicitly and the refusal is gone. The authentication preflight passed auth_inline with optional, which cannot prompt, and swallowed its own error. It prompts, and it says what happened when it cannot. It also ran before the offline check, so an offline resolution could still issue a refresh. The SSO check walked every prefetch and looked up every repository before testing a flag list that is empty for anyone with nothing lapsed. The organizations are computed once for both callers and the flag is tested first. mix hex.user deauth discarded the revocation result and reported success regardless, so a token that was never revoked server side looked revoked. It warns. Repository fetches that end without credentials now report what happened rather than an inspected tuple, since that is the ordinary shape once the 401 path stops prompting.
Write hex.config through a temporary file that is created empty, chmodded to 0600, and renamed over the target. The tokens were written in place and the mode applied afterwards, so they sat readable for the length of the write, and a reader could see a truncated file. A failed chmod now fails the write instead of being discarded. Keep the existing session until the new one arrives. mix hex.user auth revoked and cleared the stored credentials before starting the device flow, so a denied, timed out, or interrupted authentication left the user with nothing. Resolve the resolution preflight through the stored session rather than HEX_API_KEY. The fetches it runs ahead of use the session, so an API key resolved here and left the session to be refreshed mid-fetch, past the point where the SSO prompt can be offered. Escape %% for cmd.exe, which expands %%NAME%% before it looks for command separators. Strip control characters from server-supplied verification URLs and device codes before printing them, so a response cannot rewrite the terminal. Persist a repository token inside the config transaction. The repository map was read outside it and written whole, so two parallel exchanges in one run could drop one of the two tokens. Treat end of input as a cancelled OTP prompt. Mix.shell().prompt returns :eof rather than nil, so a challenge with no tty raised instead of returning the auth error.
`do_write/1` only chmodded the config directory when it had just created it, so an install predating the private-write change stays world-readable. Mine is `drwxr-xr-x` right now. The 0600 on the file is no help if anyone can list the directory and open what is in it. Revendors hex_core for the refused-refresh prompt: `with_api/4` consulted `should_authenticate` only when there were no credentials at all, so a session the server refused produced three "run mix hex.user auth" messages in one `mix deps.get` and no prompt.
`update_repo/2` read `$repos` from the config file and wrote the result to both the file and `Hex.State`, so any repository the file does not have was replaced the moment any repository exchanged an API key for an OAuth token. `HEX_REPOS`, `HEX_MIRROR` and a caller that set `:repos` directly all land in state without ever reaching the file, so the next request went wherever the file said. The two halves start from different places now. The file is what another process may have written since, so the disk write still starts from the file, which is what the lock is for. The state write starts from the state. This is what made `Hex.RepoTest`'s OAuth cache tests fail depending on test order: they set the hexpm URL to the local test server in state only, and the first exchange replaced it with the real repo.hex.pm from the config file. Reproduced with `mix test --seed 24390` before the fix.
The server's refusal no longer names a client command, since it cannot know which client asked. A full mix hex.user auth re-establishes organization access at approval, so it is the fallback whatever kept the in-place flow from starting.
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.
Prompts once, at the terminal, when an organization the project depends on needs its SSO session renewed.
mix deps.getalready knows every repository it needs before it fetches anything, because a published package's dependencies can only come from the public repository or the package's own organization. That set is intersected with the organizations the last token refresh flagged, so a member of ten SSO organizations who depends on two is asked about two:On yes it requests a re-authorization URI, prints it, and waits. Completing it in a browser renews the existing session rather than replacing it, so the refresh token survives and nobody re-runs device auth. On no it warns and carries on without those packages.
It asks only where the answer would change something. A repository authenticated with an organization key never touches the stored token, so nothing about it is worth asking. Offline says so instead of prompting, and
HEX_API_KEYsays it authenticates as itself.Vendors hexpm/hex_core#213, which has to merge first.