Skip to content

feat: Add OAuth2 PKCE browser-based auth for AWS user/root identities#2148

Open
Benbentwo wants to merge 7 commits intomainfrom
feature/atmos-162-add-browser-based-auth-flow-for-root-and-iam-identities
Open

feat: Add OAuth2 PKCE browser-based auth for AWS user/root identities#2148
Benbentwo wants to merge 7 commits intomainfrom
feature/atmos-162-add-browser-based-auth-flow-for-root-and-iam-identities

Conversation

@Benbentwo
Copy link
Copy Markdown
Member

@Benbentwo Benbentwo commented Mar 6, 2026

what

  • Implement OAuth2 PKCE browser-based authentication as a third-tier fallback for aws/user identity
  • Add interactive browser flow with local callback server and bubbletea spinner
  • Add non-interactive mode that displays authorization URL and prompts for manual code entry
  • Cache refresh tokens to XDG cache directory for 12-hour session reuse
  • Automatically refresh temporary credentials every 15 minutes via refresh token grant

why

AWS recently introduced browser-based OAuth2 authentication for IAM users and root accounts, eliminating the need for static access keys. This implementation provides the same convenient web-based flow that SSO users already enjoy, as a fallback when YAML credentials and keychain credentials are unavailable. Refresh token support enables long-lived 12-hour sessions without requiring browser reopening.

references

Summary by CodeRabbit

  • New Features

    • Browser-based AWS sign-in fallback using OAuth2 PKCE with automatic browser launch, spinner UI, manual-entry fallback, refresh-token caching (12h), periodic refresh, region handling, and credential persistence.
  • Bug Fixes

    • Improved fallback behavior and clearer user-facing errors with additional debug logging when webflow or credential resolution fails.
  • Tests

    • Extensive tests covering PKCE, callback handling, token exchange/refresh, caching, UI flows, and error scenarios.
  • Documentation

    • Blog post and roadmap updated to announce and document the new browser-based AWS auth.

@Benbentwo Benbentwo requested a review from a team as a code owner March 6, 2026 18:42
@github-actions github-actions bot added the size/l Large size PR label Mar 6, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 6, 2026

Dependency Review

✅ No vulnerabilities or license issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 4139fc6.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2480c594b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 99 to +103
if err != nil {
// Try browser webflow as final fallback when no long-lived credentials available.
if i.isWebflowEnabled() {
webflowCreds, webflowErr := i.resolveCredentialsViaWebflow(ctx)
if webflowErr == nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restrict webflow fallback to missing-credential errors

Authenticate now attempts browser webflow for any error from resolveLongLivedCredentials, including hard configuration errors (for example, partially specified access_key_id/secret_access_key). That means an invalid identity config can be silently bypassed by a browser login, masking misconfiguration and potentially authenticating to a different AWS principal than intended; webflow should only be used when credentials are unavailable, while validation/config errors should be returned immediately.

Useful? React with 👍 / 👎.

Comment on lines +138 to +141
if err != nil {
// Clear expired/invalid refresh token.
i.deleteRefreshCache()
return nil, fmt.Errorf("%w: %w", errUtils.ErrWebflowRefreshFailed, err)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid deleting refresh cache on transient token errors

The refresh cache is deleted on every exchangeRefreshToken failure, but that call can fail for transient reasons (network interruption, temporary AWS signin outage, timeout) that do not invalidate the refresh token. Deleting the cache in those cases forces an unnecessary full browser re-authentication and can break unattended runs that rely on refresh-only auth; only invalidate cached tokens for definitive invalid-token responses.

Useful? React with 👍 / 👎.

errCh := make(chan error, 1)

go func() {
if formErr := form.Run(); formErr != nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Replace TUI prompt in non-interactive webflow path

This branch is explicitly the non-interactive fallback (browserWebflow calls it when webflowIsInteractive() is false), but it still runs a Huh TUI form for manual code entry. In no-TTY contexts, that prompt cannot be rendered and the flow exits with a prompt error instead of accepting manual input, so the advertised non-interactive auth path is not usable.

Useful? React with 👍 / 👎.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 6, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a browser-based AWS OAuth2 PKCE "webflow" fallback for aws/user authentication, new webflow error sentinels, comprehensive tests, a blog post, and a roadmap update; integrates webflow as a conditional fallback when long-lived credential resolution fails.

Changes

Cohort / File(s) Summary
Error Sentinels
errors/errors.go
Adds six exported webflow error sentinels: ErrWebflowAuthFailed, ErrWebflowDisabled, ErrWebflowTokenExchange, ErrWebflowCallbackServer, ErrWebflowTimeout, ErrWebflowRefreshFailed.
Webflow Implementation
pkg/auth/identities/aws/webflow.go
New OAuth2 PKCE browser webflow implementation: PKCE/state generation, loopback callback server, auth URL handling, token exchange (authorization_code & refresh_token), HTTP timeouts, error classification, and refresh-token cache persistence.
Authenticate Integration
pkg/auth/identities/aws/user.go
On long-lived credential resolution failure, conditionally attempts webflow fallback (when enabled and error isn’t ErrInvalidAuthConfig); completes region, writes AWS files, and returns webflow credentials on success.
Webflow Tests
pkg/auth/identities/aws/webflow_test.go
Adds extensive unit and integration-style tests: PKCE, URL construction, callback server handling, token exchange/refresh (mock HTTP), cache persistence/permissions, interactive vs non-interactive flows, spinner behavior, and error cases.
Docs / Roadmap / Blog
website/blog/2026-03-24-browser-based-auth-aws-user.mdx, website/src/data/roadmap.js
Adds blog post documenting the webflow and updates roadmap milestone to shipped (progress, PR/changelog/prd metadata, and description updated to OAuth2 PKCE with 12-hour refresh sessions).

Sequence Diagram

sequenceDiagram
    participant User as User
    participant Client as Atmos Client
    participant Browser as Browser
    participant Auth as AWS Auth Server
    participant Callback as Local Callback Server
    participant Token as AWS Token Endpoint

    User->>Client: authenticate()
    Client->>Client: generate PKCE verifier/challenge & state
    Client->>Browser: open or display authorization URL
    Browser->>Auth: request authorization (code_challenge & state)
    Auth->>Browser: redirect to callback with code & state
    Browser->>Callback: GET /callback?code=...&state=...
    Callback->>Client: deliver code & state
    Client->>Token: POST exchange (code + verifier) for tokens
    Token->>Client: return access_token, refresh_token, expiry
    Client->>Client: cache refresh_token, convert tokens to AWS creds
    Client->>User: return AWS credentials
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • aknysh
  • osterman
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically summarizes the main change: adding OAuth2 PKCE browser-based authentication for AWS identities.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/atmos-162-add-browser-based-auth-flow-for-root-and-iam-identities

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot previously approved these changes Mar 6, 2026
@Benbentwo Benbentwo added the minor New features that do not break anything label Mar 9, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 9, 2026

Warning

Release Documentation Required

This PR is labeled minor or major and requires documentation updates:

  • Changelog entry - Add a blog post in website/blog/YYYY-MM-DD-feature-name.mdx
  • Roadmap update - Update website/src/data/roadmap.js with the new milestone

Alternatively: If this change doesn't require release documentation, remove the minor or major label.

1 similar comment
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 9, 2026

Warning

Release Documentation Required

This PR is labeled minor or major and requires documentation updates:

  • Changelog entry - Add a blog post in website/blog/YYYY-MM-DD-feature-name.mdx
  • Roadmap update - Update website/src/data/roadmap.js with the new milestone

Alternatively: If this change doesn't require release documentation, remove the minor or major label.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 9, 2026

Codecov Report

❌ Patch coverage is 66.51885% with 151 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.23%. Comparing base (e33692b) to head (4139fc6).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
pkg/auth/identities/aws/webflow.go 68.02% 120 Missing and 21 partials ⚠️
pkg/auth/identities/aws/user.go 0.00% 9 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (66.51%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main    #2148    +/-   ##
========================================
  Coverage   77.22%   77.23%            
========================================
  Files        1017     1021     +4     
  Lines       96260    96932   +672     
========================================
+ Hits        74337    74863   +526     
- Misses      17729    17850   +121     
- Partials     4194     4219    +25     
Flag Coverage Δ
unittests 77.23% <66.51%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
errors/errors.go 100.00% <ø> (ø)
pkg/auth/identities/aws/user.go 81.15% <0.00%> (-1.34%) ⬇️
pkg/auth/identities/aws/webflow.go 68.02% <68.02%> (ø)

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

Warning

Release Documentation Required

This PR is labeled minor or major and requires documentation updates:

  • Changelog entry - Add a blog post in website/blog/YYYY-MM-DD-feature-name.mdx
  • Roadmap update - Update website/src/data/roadmap.js with the new milestone

Alternatively: If this change doesn't require release documentation, remove the minor or major label.

@github-actions
Copy link
Copy Markdown

Warning

Release Documentation Required

This PR is labeled minor or major and requires documentation updates:

  • Changelog entry - Add a blog post in website/blog/YYYY-MM-DD-feature-name.mdx
  • Roadmap update - Update website/src/data/roadmap.js with the new milestone

Alternatively: If this change doesn't require release documentation, remove the minor or major label.

@Benbentwo Benbentwo force-pushed the feature/atmos-162-add-browser-based-auth-flow-for-root-and-iam-identities branch from 2930ea2 to 84caada Compare March 23, 2026 15:38
@github-actions
Copy link
Copy Markdown

Warning

Release Documentation Required

This PR is labeled minor or major and requires documentation updates:

  • Changelog entry - Add a blog post in website/blog/YYYY-MM-DD-feature-name.mdx
  • Roadmap update - Update website/src/data/roadmap.js with the new milestone

Alternatively: If this change doesn't require release documentation, remove the minor or major label.

1 similar comment
@github-actions
Copy link
Copy Markdown

Warning

Release Documentation Required

This PR is labeled minor or major and requires documentation updates:

  • Changelog entry - Add a blog post in website/blog/YYYY-MM-DD-feature-name.mdx
  • Roadmap update - Update website/src/data/roadmap.js with the new milestone

Alternatively: If this change doesn't require release documentation, remove the minor or major label.

Benbentwo added a commit that referenced this pull request Mar 24, 2026
…date

- Skip TestCacheFilePermissions on Windows (NTFS doesn't support Unix perms)
- Add changelog blog post for browser-based auth feature
- Update roadmap: mark browser-based auth milestone as shipped (PR #2148)
- Update auth initiative progress from 85% to 92%

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mergify
Copy link
Copy Markdown

mergify bot commented Mar 24, 2026

💥 This pull request now has conflicts. Could you fix it @Benbentwo? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Mar 24, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
pkg/auth/identities/aws/webflow_test.go (2)

637-653: Context cancellation test uses time.Sleep — consider deterministic approach.

The test at line 648 uses time.Sleep(100 * time.Millisecond) to wait for server shutdown. This can be flaky on slow CI runners.

Consider polling with a timeout or using a done channel from the server shutdown.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/aws/webflow_test.go` around lines 637 - 653, The test
TestCallbackServer_ContextCancellation uses time.Sleep to wait for shutdown;
replace this with a deterministic wait by either updating startCallbackServer to
return a shutdown completion channel (e.g., done chan struct{}) and waiting on
it after cancel(), or implement a polling loop that repeatedly attempts a
connection to addr/webflowCallbackPath with a short backoff until it fails or a
timeout is reached; update the test to wait on that done channel or the polling
loop instead of time.Sleep so CI flakes are avoided.

1-26: Missing TestMain with _ATMOS_TEST_EXIT_ONE gate.

Per coding guidelines, test packages should include a TestMain with the environment gate check for cross-platform subprocess testing. If this package has subprocess tests elsewhere, add the gate; otherwise, add a minimal TestMain for future-proofing.

Add TestMain
func TestMain(m *testing.M) {
	if os.Getenv("_ATMOS_TEST_EXIT_ONE") == "1" {
		os.Exit(1)
	}
	os.Exit(m.Run())
}

As per coding guidelines: "Add TestMain with _ATMOS_TEST_EXIT_ONE environment gate check for cross-platform subprocess testing."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/aws/webflow_test.go` around lines 1 - 26, Add a TestMain
function to the aws test package (in webflow_test.go) that checks the
_ATMOS_TEST_EXIT_ONE environment gate and exits 1 when it's set, otherwise runs
the test suite; specifically implement func TestMain(m *testing.M) { if
os.Getenv("_ATMOS_TEST_EXIT_ONE") == "1" { os.Exit(1) } os.Exit(m.Run()) } so
the package-level tests (including any subprocess tests) honor the
cross-platform gate—ensure you import os at the top if not already referenced.
pkg/auth/identities/aws/webflow.go (3)

98-104: Global mutable variables may cause race conditions in tests.

The package-level variables defaultHTTPClient, openURLFunc, webflowIsTTYFunc, and displayWebflowDialogFunc are mutated during tests. This can cause data races if tests run in parallel.

Consider injecting these dependencies through functional options or struct fields rather than package-level variables. Alternatively, ensure tests that mutate these use t.Setenv patterns with proper cleanup and avoid t.Parallel().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/aws/webflow.go` around lines 98 - 104, The package-level
mutable variables defaultHTTPClient, openURLFunc, webflowIsTTYFunc, and
displayWebflowDialogFunc can cause race conditions in parallel tests; refactor
to inject them via the Webflow object (or constructor functional options)
instead of globals: add fields like httpClient, openURL, isTTY, and
displayDialog to the Webflow struct, update NewWebflow (or create
NewWebflowWithOptions) to accept overrides, and update all uses (tokenExchange,
Start/Flow methods) to reference those instance fields rather than the package
globals so tests can provide isolated mocks without mutating package state.

726-731: webflowIsInteractive is defined but never used.

This function checks viper.GetBool("force-tty") but the code uses webflowIsTTYFunc directly. Consider using this function or removing it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/aws/webflow.go` around lines 726 - 731, The function
webflowIsInteractive currently duplicates logic and is unused; either replace
direct uses of webflowIsTTYFunc with webflowIsInteractive so the force-tty viper
override is respected (search for webflowIsTTYFunc usages and switch them to
webflowIsInteractive), or delete webflowIsInteractive to remove dead code and
ensure no remaining references to it exist; pick the option that preserves
intended behavior (prefer using webflowIsInteractive if force-tty should take
precedence).

838-847: Recursive command pattern risks unbounded stack growth.

The checkResult method uses recursive polling that calls itself every 100ms. While unlikely to cause issues in practice given the 5-minute timeout, this pattern accumulates stack frames. Consider using a loop-based approach or tea.Tick.

Alternative using tea.Tick
 func (m webflowSpinnerModel) checkResult() tea.Cmd {
-	return func() tea.Msg {
-		select {
-		case res := <-m.tokenCh:
-			return webflowSpinnerTokenResult{resp: res.resp, err: res.err}
-		case <-time.After(100 * time.Millisecond):
-			return m.checkResult()()
-		}
-	}
+	return tea.Tick(100*time.Millisecond, func(t time.Time) tea.Msg {
+		select {
+		case res := <-m.tokenCh:
+			return res
+		default:
+			return nil // Will trigger another tick via Init batch.
+		}
+	})
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/aws/webflow.go` around lines 838 - 847, The current
webflowSpinnerModel.checkResult function uses recursion to poll tokenCh and
reschedules itself via m.checkResult(), which can build up stack frames; replace
the recursive pattern with an iterative/tick-based command: inside checkResult
use tea.Tick or a loop/ticker to wait 100ms intervals and select on m.tokenCh
and the ticker (or tea.Tick's channel), then return a webflowSpinnerTokenResult
when a value arrives (resp/err from tokenCh) and stop the ticker; ensure the
function returns a single tea.Cmd that schedules repeated ticks rather than
calling checkResult recursively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/auth/identities/aws/webflow.go`:
- Around line 325-326: In waitForCallbackWithSpinner, the context created with
context.WithTimeout (ctx, webflowCallbackTimeout) assigns cancel but never
guaranteed to be called on the success path; add a defer cancel() immediately
after ctx, cancel := ... so the timeout context is always cleaned up (it's safe
to later pass cancel into the model/goroutine and call it there as well). This
ensures cancel is invoked if prog.Run() returns nil without relying on other
code paths.
- Around line 269-282: The Validate callback on the NewInput for "Authorization
code" currently wraps the empty-field message with the sentinel
errUtils.ErrWebflowAuthFailed; change the Validate implementation to return a
plain user-facing error (e.g., "authorization code is required") without
wrapping the sentinel, so the message is clean for UI. Keep the sentinel
wrapping at the higher-level form/error handling site that already wraps form
errors (the place that inspects the form error and wraps with
errUtils.ErrWebflowAuthFailed), leaving that wrapping logic unchanged. Ensure
you update the Validate closure that references authCode inside
huh.NewInput(...) to return an unwrapped error.

---

Nitpick comments:
In `@pkg/auth/identities/aws/webflow_test.go`:
- Around line 637-653: The test TestCallbackServer_ContextCancellation uses
time.Sleep to wait for shutdown; replace this with a deterministic wait by
either updating startCallbackServer to return a shutdown completion channel
(e.g., done chan struct{}) and waiting on it after cancel(), or implement a
polling loop that repeatedly attempts a connection to addr/webflowCallbackPath
with a short backoff until it fails or a timeout is reached; update the test to
wait on that done channel or the polling loop instead of time.Sleep so CI flakes
are avoided.
- Around line 1-26: Add a TestMain function to the aws test package (in
webflow_test.go) that checks the _ATMOS_TEST_EXIT_ONE environment gate and exits
1 when it's set, otherwise runs the test suite; specifically implement func
TestMain(m *testing.M) { if os.Getenv("_ATMOS_TEST_EXIT_ONE") == "1" {
os.Exit(1) } os.Exit(m.Run()) } so the package-level tests (including any
subprocess tests) honor the cross-platform gate—ensure you import os at the top
if not already referenced.

In `@pkg/auth/identities/aws/webflow.go`:
- Around line 98-104: The package-level mutable variables defaultHTTPClient,
openURLFunc, webflowIsTTYFunc, and displayWebflowDialogFunc can cause race
conditions in parallel tests; refactor to inject them via the Webflow object (or
constructor functional options) instead of globals: add fields like httpClient,
openURL, isTTY, and displayDialog to the Webflow struct, update NewWebflow (or
create NewWebflowWithOptions) to accept overrides, and update all uses
(tokenExchange, Start/Flow methods) to reference those instance fields rather
than the package globals so tests can provide isolated mocks without mutating
package state.
- Around line 726-731: The function webflowIsInteractive currently duplicates
logic and is unused; either replace direct uses of webflowIsTTYFunc with
webflowIsInteractive so the force-tty viper override is respected (search for
webflowIsTTYFunc usages and switch them to webflowIsInteractive), or delete
webflowIsInteractive to remove dead code and ensure no remaining references to
it exist; pick the option that preserves intended behavior (prefer using
webflowIsInteractive if force-tty should take precedence).
- Around line 838-847: The current webflowSpinnerModel.checkResult function uses
recursion to poll tokenCh and reschedules itself via m.checkResult(), which can
build up stack frames; replace the recursive pattern with an
iterative/tick-based command: inside checkResult use tea.Tick or a loop/ticker
to wait 100ms intervals and select on m.tokenCh and the ticker (or tea.Tick's
channel), then return a webflowSpinnerTokenResult when a value arrives (resp/err
from tokenCh) and stop the ticker; ensure the function returns a single tea.Cmd
that schedules repeated ticks rather than calling checkResult recursively.
🪄 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: 6b2a56f2-d407-4967-a76f-d7347790acae

📥 Commits

Reviewing files that changed from the base of the PR and between 8e4a13b and 31a0ecf.

📒 Files selected for processing (4)
  • pkg/auth/identities/aws/webflow.go
  • pkg/auth/identities/aws/webflow_test.go
  • website/blog/2026-03-24-browser-based-auth-aws-user.mdx
  • website/src/data/roadmap.js

Benbentwo and others added 4 commits March 26, 2026 09:15
… (ATMOS-162)

Implement browser-based authentication as a third-tier fallback for aws/user identity.
When YAML credentials and keychain credentials are unavailable, the identity initiates
an interactive OAuth2 PKCE flow via AWS signin service, providing seamless browser-based
login for IAM users and root accounts. Refresh tokens enable 12-hour sessions with
automatic credential refresh every 15 minutes. Non-interactive mode prompts for manual
code entry. Credentials are cached in XDG directories following SSO patterns.

- Add webflow_enabled config option (default: true)
- Implement OAuth2 PKCE with AWS signin client arn:aws:signin:::devtools/same-device
- Support interactive (browser + spinner) and non-interactive (manual code) flows
- Cache refresh tokens to XDG cache for 12-hour session reuse
- Add webflow entry point in Authenticate() after long-lived creds fail
- Include 27 comprehensive unit tests with >80% coverage

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Add 25+ new tests covering processTokenResponse, resolveCredentialsViaWebflow
refresh path, browserWebflow dispatch, waitForCallbackSimple, callTokenEndpoint
error paths, spinner model methods, refresh cache edge cases, and end-to-end
interactive flow. Introduce mockable function variables (openURLFunc,
webflowIsTTYFunc, displayWebflowDialogFunc) following existing
PromptCredentialsFunc pattern for testability.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…date

- Skip TestCacheFilePermissions on Windows (NTFS doesn't support Unix perms)
- Add changelog blog post for browser-based auth feature
- Update roadmap: mark browser-based auth milestone as shipped (PR #2148)
- Update auth initiative progress from 85% to 92%

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Benbentwo Benbentwo force-pushed the feature/atmos-162-add-browser-based-auth-flow-for-root-and-iam-identities branch from 31a0ecf to 82a4759 Compare March 26, 2026 16:15
- Guard webflow fallback against invalid config errors (only trigger on missing credentials)
- Add transient vs definitive error classification for refresh token cache
- Replace huh TUI form with bufio.Scanner for non-interactive code entry
- Add defer cancel() for context.WithTimeout in spinner
- Add tests for isTransientTokenError and transient-error cache preservation

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mergify mergify bot removed the conflict This PR has conflicts label Mar 26, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
pkg/auth/identities/aws/webflow_test.go (1)

1832-1839: Consider using mockgen instead of manual mock.

The mockHTTPClient is a manual mock. Guidelines prefer generated mocks via go.uber.org/mock/mockgen with //go:generate directives for consistency across the codebase.

That said, this mock is minimal and self-contained. Low priority if you'd rather keep it simple.

♻️ Suggested approach

Add a generate directive near the interface definition in webflow.go:

//go:generate mockgen -destination=mock_http_client_test.go -package=aws github.com/cloudposse/atmos/pkg/auth/identities/aws HTTPClient

Then remove the manual mock and use the generated one.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/aws/webflow_test.go` around lines 1832 - 1839, The test
defines a manual mock type mockHTTPClient for the HTTPClient interface in
webflow tests; replace it with a generated mock to follow project conventions:
add a //go:generate mockgen directive next to the HTTPClient interface in
webflow.go (e.g., generating mock_http_client_test.go in package aws for
github.com/cloudposse/atmos/pkg/auth/identities/aws HTTPClient), run mockgen to
produce the mock, remove the manual mockHTTPClient type and its Do method from
webflow_test.go, and update tests to use the generated mock (named by mockgen)
instead of mockHTTPClient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/auth/identities/aws/webflow_test.go`:
- Around line 645-654: Replace the brittle time.Sleep(100 * time.Millisecond)
after cancel() with a retry loop that polls until the server refuses connections
or a context deadline elapses; repeatedly attempt the same
http.Get(fmt.Sprintf("http://%s%s", addr, webflowCallbackPath)) (or a TCP dial
to addr) with short intervals (e.g., 10–50ms) and fail the test if the timeout
is reached, then assert that the final attempt returns an error. This change
touches the test flow around cancel(), the subsequent sleep, and the
http.Get/assert.Error lines so search for cancel(), time.Sleep, and the
http.Get(fmt.Sprintf("http://%s%s", addr, webflowCallbackPath)) call to replace
with the retry-with-timeout logic.

In `@website/blog/2026-03-24-browser-based-auth-aws-user.mdx`:
- Line 4: Replace the top-matter authors field value "authors: [atmos]" with the
actual person's name listed in website/blog/authors.yml (use the corresponding
key for the blog author), i.e., update the authors: array to include the real
author string from authors.yml instead of the organization token "atmos" so the
post credits the correct individual.

---

Nitpick comments:
In `@pkg/auth/identities/aws/webflow_test.go`:
- Around line 1832-1839: The test defines a manual mock type mockHTTPClient for
the HTTPClient interface in webflow tests; replace it with a generated mock to
follow project conventions: add a //go:generate mockgen directive next to the
HTTPClient interface in webflow.go (e.g., generating mock_http_client_test.go in
package aws for github.com/cloudposse/atmos/pkg/auth/identities/aws HTTPClient),
run mockgen to produce the mock, remove the manual mockHTTPClient type and its
Do method from webflow_test.go, and update tests to use the generated mock
(named by mockgen) instead of mockHTTPClient.
🪄 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: e645c46d-8836-4ab2-93f4-0ca3d80b36a7

📥 Commits

Reviewing files that changed from the base of the PR and between 31a0ecf and 602cda1.

📒 Files selected for processing (6)
  • errors/errors.go
  • pkg/auth/identities/aws/user.go
  • pkg/auth/identities/aws/webflow.go
  • pkg/auth/identities/aws/webflow_test.go
  • website/blog/2026-03-24-browser-based-auth-aws-user.mdx
  • website/src/data/roadmap.js
✅ Files skipped from review due to trivial changes (2)
  • pkg/auth/identities/aws/user.go
  • pkg/auth/identities/aws/webflow.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • errors/errors.go
  • website/src/data/roadmap.js

- Replace time.Sleep with require.Eventually for server shutdown test
- Use actual author (Benbentwo) instead of generic 'atmos' in blog post

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@website/blog/2026-03-24-browser-based-auth-aws-user.mdx`:
- Line 23: Replace the hyphenation of "signin" with "sign-in" in the phrase
"signin authorization endpoint" so it matches the earlier change on line 14;
locate the string "signin authorization endpoint" in the blog post
(website/blog/2026-03-24-browser-based-auth-aws-user.mdx) and update it to
"sign-in authorization endpoint" to keep wording consistent.
- Line 14: Update the wording for the aws/user identity description: replace the
string "signin" with "sign-in" in the sentence that reads "OAuth2 PKCE flow via
the AWS signin service" so it conforms to AWS/OAuth terminology; ensure the
phrase becomes "AWS sign-in service" wherever it appears in the 3rd-tier
authentication fallback description for the aws/user identity type.
🪄 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: f9761cd5-6846-48cf-8639-ad3e71de24a8

📥 Commits

Reviewing files that changed from the base of the PR and between 602cda1 and 4139fc6.

📒 Files selected for processing (2)
  • pkg/auth/identities/aws/webflow_test.go
  • website/blog/2026-03-24-browser-based-auth-aws-user.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/auth/identities/aws/webflow_test.go


## What Changed

The `aws/user` identity type gains a new third-tier authentication fallback. When YAML credentials and keychain credentials are both unavailable, Atmos automatically initiates an OAuth2 PKCE flow via the AWS signin service. This provides the same convenient web-based authentication that SSO users already enjoy, without requiring static access keys.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Hyphenate "sign-in" for consistency.

The term "signin" should be hyphenated as "sign-in" per standard AWS and OAuth terminology conventions.

📝 Proposed fix
-The `aws/user` identity type gains a new third-tier authentication fallback. When YAML credentials and keychain credentials are both unavailable, Atmos automatically initiates an OAuth2 PKCE flow via the AWS signin service. This provides the same convenient web-based authentication that SSO users already enjoy, without requiring static access keys.
+The `aws/user` identity type gains a new third-tier authentication fallback. When YAML credentials and keychain credentials are both unavailable, Atmos automatically initiates an OAuth2 PKCE flow via the AWS sign-in service. This provides the same convenient web-based authentication that SSO users already enjoy, without requiring static access keys.
📝 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
The `aws/user` identity type gains a new third-tier authentication fallback. When YAML credentials and keychain credentials are both unavailable, Atmos automatically initiates an OAuth2 PKCE flow via the AWS signin service. This provides the same convenient web-based authentication that SSO users already enjoy, without requiring static access keys.
The `aws/user` identity type gains a new third-tier authentication fallback. When YAML credentials and keychain credentials are both unavailable, Atmos automatically initiates an OAuth2 PKCE flow via the AWS sign-in service. This provides the same convenient web-based authentication that SSO users already enjoy, without requiring static access keys.
🧰 Tools
🪛 LanguageTool

[grammar] ~14-~14: Ensure spelling is correct
Context: ...itiates an OAuth2 PKCE flow via the AWS signin service. This provides the same conveni...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@website/blog/2026-03-24-browser-based-auth-aws-user.mdx` at line 14, Update
the wording for the aws/user identity description: replace the string "signin"
with "sign-in" in the sentence that reads "OAuth2 PKCE flow via the AWS signin
service" so it conforms to AWS/OAuth terminology; ensure the phrase becomes "AWS
sign-in service" wherever it appears in the 3rd-tier authentication fallback
description for the aws/user identity type.

No configuration is required. Browser authentication is enabled by default for all `aws/user` identities. When triggered, Atmos:

1. Starts a local callback server on an ephemeral port
2. Opens your browser to the AWS signin authorization endpoint
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Same hyphenation issue here.

Consistent with line 14, use "sign-in" instead of "signin".

📝 Proposed fix
-2. Opens your browser to the AWS signin authorization endpoint
+2. Opens your browser to the AWS sign-in authorization endpoint
📝 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
2. Opens your browser to the AWS signin authorization endpoint
2. Opens your browser to the AWS sign-in authorization endpoint
🧰 Tools
🪛 LanguageTool

[grammar] ~23-~23: Ensure spelling is correct
Context: ...l port 2. Opens your browser to the AWS signin authorization endpoint 3. Exchanges the...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@website/blog/2026-03-24-browser-based-auth-aws-user.mdx` at line 23, Replace
the hyphenation of "signin" with "sign-in" in the phrase "signin authorization
endpoint" so it matches the earlier change on line 14; locate the string "signin
authorization endpoint" in the blog post
(website/blog/2026-03-24-browser-based-auth-aws-user.mdx) and update it to
"sign-in authorization endpoint" to keep wording consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor New features that do not break anything size/l Large size PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant