Remove browser flow as auth method#36
Merged
carole-lavillonniere merged 2 commits intomainfrom Feb 19, 2026
Merged
Conversation
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the callback-server device flow with an interactive login provider: renames browserLogin→login, introduces newLoginProvider/loginProvider that emits a verification URL and code, optionally opens the browser, waits for user confirmation, exchanges tokens, updates tests, and deletes the old browser-callback integration test. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant LoginProvider
participant PlatformAPI
participant Browser
participant Keyring
CLI->>LoginProvider: Login(ctx)
LoginProvider->>PlatformAPI: CreateAuthRequest()
PlatformAPI-->>LoginProvider: auth request (verification_url, user_code, request_id)
LoginProvider-->>CLI: print URL and code, prompt to open browser
CLI->>Browser: open verification_url (optional)
Browser->>PlatformAPI: user authenticates & confirms request
PlatformAPI-->>LoginProvider: request confirmed
LoginProvider->>PlatformAPI: ExchangeAuthRequest(request_id)
PlatformAPI-->>LoginProvider: access token / license token
LoginProvider->>Keyring: store token
LoginProvider-->>CLI: "Login successful"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/auth/login.go`:
- Around line 49-61: The code ignores errors from reader.ReadString and
browser.OpenURL; update the interactive flow in the function that constructs
reader and uses authURL (the reader variable, browser.OpenURL call, and
subsequent reader.ReadString before output.EmitLog) to check and handle errors:
capture and handle the first ReadString error (treat io.EOF as empty response
and return or log a user-facing message for non-interactive shells), check the
error returned by browser.OpenURL and emit a warning via output.EmitLog (using
l.sink) if it fails, and also handle the second ReadString error before polling
for auth status so the function fails fast with a clear log rather than silently
continuing.
🧹 Nitpick comments (1)
test/integration/login_test.go (1)
99-108: Consider removing fixed sleeps; stdin buffering already handles ordering.Fixed sleeps add flakiness and latency. Writing to stdin immediately should still work and speeds the test.
♻️ Suggested cleanup
- // Wait for instructions - time.Sleep(100 * time.Millisecond) @@ - // Wait a bit for browser to open - time.Sleep(100 * time.Millisecond) @@ - // Wait for instructions to be printed - time.Sleep(100 * time.Millisecond) @@ - // Wait a bit for browser to open - time.Sleep(100 * time.Millisecond)Also applies to: 167-176
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/integration/login_test.go (1)
116-121: Remove duplicate assertions.Lines 119-120 are redundant:
"Auth request confirmed"is a substring of"Auth request confirmed, exchanging for token"(already asserted on line 117)"Fetching license token"is asserted twice (lines 118 and 120)♻️ Proposed fix to remove duplicates
// Should complete authentication successfully assert.Contains(t, output, "Checking if auth request is confirmed") assert.Contains(t, output, "Auth request confirmed, exchanging for token") assert.Contains(t, output, "Fetching license token") - assert.Contains(t, output, "Auth request confirmed") - assert.Contains(t, output, "Fetching license token") assert.Contains(t, output, "Login successful")
carole-lavillonniere
force-pushed
the
carole/drg-530
branch
from
19c5650 to
c80da5d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/integration/login_test.go (1)
67-130: Trim self-explanatory comments in the new interaction steps.The new inline comments mostly restate the code; consider removing them or replacing with rationale that isn’t obvious from the code itself.
🧹 Suggested cleanup
- // Simulate answering "Y" to open browser _, err = stdinPipe.Write([]byte("Y\n")) require.NoError(t, err) - // Simulate pressing ENTER after completing auth in browser _, err = stdinPipe.Write([]byte("\n")) require.NoError(t, err) select { case out := <-outputCh: output := string(out) - // Should show instructions assert.Contains(t, output, "Verification code:") assert.Contains(t, output, "TEST123") assert.Contains(t, output, "Open browser now? [Y/n]") assert.Contains(t, output, "Waiting for authentication (Press [ENTER] when complete)") - // Should complete authentication successfully assert.Contains(t, output, "Checking if auth request is confirmed") assert.Contains(t, output, "Auth request confirmed, exchanging for token") assert.Contains(t, output, "Fetching license token") assert.Contains(t, output, "Login successful")- // Simulate answering "Y" to open browser _, err = stdinPipe.Write([]byte("Y\n")) require.NoError(t, err) - // Simulate pressing ENTER after "completing" auth in browser _, err = stdinPipe.Write([]byte("\n")) require.NoError(t, err) select { case out := <-outputCh: output := string(out) assert.Contains(t, output, "Verification code:") assert.Contains(t, output, "Open browser now? [Y/n]") assert.Contains(t, output, "Waiting for authentication (Press [ENTER] when complete)") - // Should attempt authentication but fail because request not confirmed assert.Contains(t, output, "Checking if auth request is confirmed") assert.Contains(t, output, "auth request not confirmed")As per coding guidelines, "Avoid adding comments for self-explanatory code; only comment when the 'why' isn't obvious from the code itself".
Also applies to: 132-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/login_test.go` around lines 67 - 130, In TestLoginSuccess, remove or shorten the self-explanatory inline comments that merely restate the code (e.g., comments around licenseToken/env check, mockServer creation, stdin handling, output assertions) and replace any that remain with brief rationale explaining non-obvious intent; focus on keeping clarifying comments for why we manipulate stdinPipe, the purpose of envWithout("LOCALSTACK_AUTH_TOKEN")/createMockAPIServer, or timing choices for context.WithTimeout, but delete comments that repeat what functions like cleanup(), createMockAPIServer, exec.CommandContext, stdinPipe.Write, and the assert.Contains checks already express.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/integration/login_test.go`:
- Around line 67-130: In TestLoginSuccess, remove or shorten the
self-explanatory inline comments that merely restate the code (e.g., comments
around licenseToken/env check, mockServer creation, stdin handling, output
assertions) and replace any that remain with brief rationale explaining
non-obvious intent; focus on keeping clarifying comments for why we manipulate
stdinPipe, the purpose of
envWithout("LOCALSTACK_AUTH_TOKEN")/createMockAPIServer, or timing choices for
context.WithTimeout, but delete comments that repeat what functions like
cleanup(), createMockAPIServer, exec.CommandContext, stdinPipe.Write, and the
assert.Contains checks already express.
120094f to
1156a00
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/auth/auth.go`:
- Around line 44-45: Change the misleading log string passed to output.EmitLog
so it doesn't say "Opening browser..."; update the message in the call where
output.EmitLog(a.sink, "...") is used (near the authentication sequence invoking
a.login.Login(ctx)) to a neutral instruction such as "Authentication required.
Please complete authentication using the provided device flow instructions" or
similar wording that does not imply an automatic browser open.
In `@internal/auth/login.go`:
- Around line 29-41: loginProvider.Login currently only logs authReq.Code so
users lack the actionable verification URL; update the AuthRequest struct to
include a VerificationURL (or VerificationURI) field and change
platformClient.CreateAuthRequest to populate it, then modify loginProvider.Login
to emit a clear message via output.EmitLog and include the full URL and short
instructions (e.g., "Visit {authReq.VerificationURL} and enter code
{authReq.Code}") and update the prompt passed to output.EmitUserInputRequest
accordingly so callers and users see both the URL and code; ensure all other
uses of AuthRequest and CreateAuthRequest are updated to handle the new field.
d2865e6 to
3177af7
Compare
carole-lavillonniere
force-pushed
the
carole/drg-530
branch
from
3177af7 to
1f74942
Compare
silv-io
approved these changes
Feb 18, 2026
Member
silv-io
left a comment
silv-io
left a comment
There was a problem hiding this comment.
Love a good cleanup!
Just a thought on expanding the userinputevent functionality. The output nit we can handle later. We'll freshen it up either way now that we default to the TUI.
Also, for posterity, this is how it looks with TUI now:
▟████▖ LocalStack (lstk)
▟██▙█▙█▟ dev
▀▛▀▛▀
Authentication required. Opening browser...
Visit: https://app.localstack.cloud/auth/request/...
Verification code: 3FE3
Checking if auth request is confirmed...
Auth request confirmed, exchanging for token...
Fetching license token...
Login successful.
Pulling localstack/localstack-pro:latest...
latest: Pulling from localstack/localstack-pro
: Digest: sha256:dab91774aa81fc565ff8019962749810d2184f28f4c7acbea514c9754235de9f
: Status: Image is up to date for localstack/localstack-pro:latest
localstack-aws: validating license (4.13.2.dev137)
Starting localstack-aws...
Waiting for localstack-aws to be ready...
localstack-aws ready (containerId: c5326e33aa84)
internal/auth/auth.go
Outdated
| @@ -42,7 +42,7 @@ func (a *Auth) GetToken(ctx context.Context) (string, error) { | |||
| } | |||
|
|
|||
| output.EmitLog(a.sink, "Authentication required. Opening browser...") | |||
Member
There was a problem hiding this comment.
nit: Maybe we can rename this log now, because we're asking later if we want to open it
Collaborator
Author
There was a problem hiding this comment.
Good catch, I lost this when resolving conflicts
| } | ||
| w.WriteHeader(http.StatusOK) | ||
| tokenCh <- token | ||
| // Ask whether to open the browser; ENTER or Y accepts (default yes), N skips |
Member
There was a problem hiding this comment.
thought: we might want to change the type a bit so that we can codify the default option more clearly. But for now this is fine
Collaborator
Author
There was a problem hiding this comment.
Agreed.
| a.inputPrompt = a.inputPrompt.Hide() | ||
| return a, responseCmd | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
nice that this works so well :)
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.
Reasons for removing browser flow:
Output:
