fix: Enable automatic Playwright browser driver download for SAML authentication #1747
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughAdds Playwright-based browser driver detection and optional auto-download to the AWS SAML provider, exposes browser fields in the schema, introduces a logrus→Atmos adapter, adds unit/integration tests and docs, and enables CI opt-in for Playwright integration. Changes
Sequence Diagram(s)sequenceDiagram
%% Styling hint: boxes are conceptual, colors for emphasis avoided
participant User
participant Atmos
participant SAML as "AWS SAML Provider"
participant Playwright
participant Cache as "XDG / LOCALAPPDATA Cache"
User->>Atmos: start auth (SAML provider)
Atmos->>SAML: Authenticate(config)
SAML->>SAML: ConfigureLogrusForAtmos()
SAML->>SAML: createSAMLConfig() / createLoginDetails()
alt shouldDownloadBrowser == true
SAML->>Cache: probe platform cache for drivers
alt no drivers found
SAML->>Playwright: download & install browser
Playwright->>Cache: populate cache
else drivers found
Cache-->>SAML: return driver paths
end
end
SAML->>Playwright: launch browser (BrowserType / ExecutablePath)
Playwright-->>SAML: browser session (SAML capture)
SAML-->>Atmos: credentials
Atmos-->>User: auth success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
go.mod(1 hunks)pkg/auth/providers/aws/saml.go(2 hunks)pkg/auth/providers/aws/saml_driver_detection_test.go(5 hunks)pkg/auth/providers/aws/saml_driver_download_integration_test.go(1 hunks)pkg/auth/providers/aws/saml_test.go(1 hunks)website/docs/cli/commands/auth/usage.mdx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/providers/aws/saml_test.gopkg/auth/providers/aws/saml.gopkg/auth/providers/aws/saml_driver_detection_test.gopkg/auth/providers/aws/saml_driver_download_integration_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
Files:
pkg/auth/providers/aws/saml_test.gopkg/auth/providers/aws/saml_driver_detection_test.gopkg/auth/providers/aws/saml_driver_download_integration_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
Files:
pkg/auth/providers/aws/saml_test.gopkg/auth/providers/aws/saml.gopkg/auth/providers/aws/saml_driver_detection_test.gopkg/auth/providers/aws/saml_driver_download_integration_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/auth/providers/aws/saml.go
go.{mod,sum}
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
go.{mod,sum}: Manage dependencies with Go modules
Keep dependencies up to date
Files:
go.mod
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in website/ when adding features
Ensure consistency between CLI help text and website documentation
Follow the website's documentation structure and style
Keep website code in website/ and follow its architecture/style; test changes locally
Keep CLI and website documentation in sync; document new features with examples and use cases
Files:
website/docs/cli/commands/auth/usage.mdx
🧠 Learnings (11)
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
pkg/auth/providers/aws/saml.gopkg/auth/providers/aws/saml_driver_detection_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/auth/providers/aws/saml_driver_detection_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/auth/providers/aws/saml_driver_detection_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
pkg/auth/providers/aws/saml_driver_detection_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
pkg/auth/providers/aws/saml_driver_detection_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to go.{mod,sum} : Manage dependencies with Go modules
Applied to files:
go.mod
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to go.{mod,sum} : Keep dependencies up to date
Applied to files:
go.mod
📚 Learning: 2025-01-17T00:21:32.987Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:3-3
Timestamp: 2025-01-17T00:21:32.987Z
Learning: Go version 1.23.0 was deliberately introduced by the maintainer (aknysh) in January 2025. While this might be a pre-release or development version of Go, it has been approved for use in this project.
Applied to files:
go.mod
📚 Learning: 2025-01-17T00:21:32.987Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:3-3
Timestamp: 2025-01-17T00:21:32.987Z
Learning: The project uses Go version 1.23.0 which has been confirmed by the maintainer to be working in production for months. Do not flag this as an invalid Go version.
Applied to files:
go.mod
📚 Learning: 2025-04-25T20:54:19.701Z
Learnt from: mcalhoun
Repo: cloudposse/atmos PR: 963
File: website/docs/core-concepts/projects/configuration/stores.mdx:286-286
Timestamp: 2025-04-25T20:54:19.701Z
Learning: For the AWS SSM Parameter Store implementation in Atmos, support for `read_role_arn` and `write_role_arn` options is essential to enable cross-account access, allowing users to run operations like `terraform plan` in multiple accounts while accessing values across keystores. Azure Key Vault would need similar capabilities for cross-tenant/subscription authentication.
Applied to files:
website/docs/cli/commands/auth/usage.mdx
📚 Learning: 2025-09-10T17:34:52.568Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/providers/github/oidc.go:96-100
Timestamp: 2025-09-10T17:34:52.568Z
Learning: The ATMOS_ environment variable binding guideline applies to Atmos configuration variables, not external service-required environment variables like GitHub Actions OIDC variables (GITHUB_ACTIONS, ACTIONS_ID_TOKEN_*) which must use their standard names.
Applied to files:
website/docs/cli/commands/auth/usage.mdx
🧬 Code graph analysis (2)
pkg/auth/providers/aws/saml_test.go (2)
pkg/auth/providers/aws/saml.go (1)
NewSAMLProvider(53-73)pkg/schema/schema_auth.go (1)
Provider(21-35)
pkg/auth/providers/aws/saml_driver_download_integration_test.go (2)
pkg/auth/providers/aws/saml.go (1)
NewSAMLProvider(53-73)pkg/schema/schema_auth.go (1)
SessionConfig(38-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: Lint (golangci)
- GitHub Check: autofix
- GitHub Check: Analyze (go)
- GitHub Check: Run pre-commit hooks
- GitHub Check: website-deploy-preview
- GitHub Check: Review Dependency Licenses
- GitHub Check: Summary
🔇 Additional comments (5)
go.mod (1)
80-80: Direct dependency makes sense.Good call promoting
github.com/playwright-community/playwright-go v0.4702.0to a direct require now that production code references it.pkg/auth/providers/aws/saml.go (2)
193-196: Nice wiring of DownloadBrowser.Setting
LoginDetails.DownloadBrowserfromshouldDownloadBrowser()keeps saml2aws in sync with the IDP config.
423-425: Cache path fix looks correct.Switching the cache probes to
ms-playwrightaligns with the Playwright CLI layout and resolves the prior detection miss.pkg/auth/providers/aws/saml_driver_detection_test.go (1)
146-238: Tests now mirror the runtime paths.Swapping the fixtures over to
ms-playwrightkeeps the detection tests aligned with the new cache layout.pkg/auth/providers/aws/saml_test.go (1)
233-309: Solid coverage for LoginDetails.DownloadBrowser.The new table test keeps
createLoginDetails()honest across the explicit, auto, and “drivers already installed” paths.
…hentication Fixes SAML authentication failing with "please install the driver (v1.47.2) and browsers first" error when download_browser_driver is configured. Root cause: The DownloadBrowser flag was set on saml2aws IDPAccount config but NOT on LoginDetails, which is what saml2aws actually checks when deciding whether to download drivers. Changes: - Set LoginDetails.DownloadBrowser in createLoginDetails() to match shouldDownloadBrowser() logic - Corrected Playwright cache directory paths from ms-playwright-go to ms-playwright (actual playwright-go location) - Added comprehensive integration test that downloads real Chromium drivers and validates installation - Enhanced driver detection to verify actual browser binaries exist (not just empty version directories) - Updated documentation to recommend automatic download and clarify manual installation complexity Testing: - Unit tests verify LoginDetails.DownloadBrowser is set correctly across all scenarios - Integration test downloads Chromium (~140 MB) and validates 160+ files installed - Driver detection tests verify empty directories don't register as valid installations Documentation: - Removed broken manual installation command (go run playwright install) - Added warning about manual installation requiring advanced knowledge - Clarified cache directory locations and why PATH is not required - Emphasized download_browser_driver: true as the recommended approach 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
osterman
force-pushed
the
saml-driver-install
branch
from
53dfc94 to
1fb48e0
Compare
…compile-time syntax Improves test clarity by using explicit runtime type assertions rather than blank variable assignments. This makes the intent clearer and provides better error messages when types don't match. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…tion Enhances driver detection to check Atmos XDG cache directory in addition to playwright-go's hardcoded locations. This allows users to: - Consolidate all Atmos data using ATMOS_XDG_CACHE_HOME or XDG_CACHE_HOME - Customize cache locations for CI/CD pipelines - Follow XDG Base Directory Specification consistently Detection now checks (in priority order): 1. $XDG_CACHE_HOME/atmos/ms-playwright (XDG-compliant, user-configurable) 2. ~/.cache/ms-playwright (Linux - playwright-go default) 3. ~/Library/Caches/ms-playwright (macOS - playwright-go default) 4. %LOCALAPPDATA%\ms-playwright (Windows - playwright-go default) Also adds constants for magic numbers to improve code quality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Corrects misleading documentation that implied Browser driver opens the user's regular browser. In reality, it: - Launches a sandboxed Chromium instance via Playwright - Shows a visible (non-headless) browser window for user interaction - Does NOT provide access to user's saved passwords or extensions - Automatically captures SAML response after successful authentication Also clarifies: - "browser driver" → "Chromium browser binary" (more accurate terminology) - Downloads the actual browser, not just a driver - Corrects cache paths from ms-playwright-go to ms-playwright This resolves confusion about why Chromium needs to be downloaded and what the Browser driver actually does during authentication. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Prevents accidental 140 MB Chromium downloads during normal test runs by requiring explicit opt-in via RUN_PLAYWRIGHT_INTEGRATION=1 environment variable. Changes: - Added environment variable check to TestPlaywrightDriverDownload_Integration - Added environment variable check to TestPlaywrightDriverDownload_WithSAML2AWS - Updated test comments to document opt-in requirement - Added "Heavy Integration Tests" section to tests/README.md Usage: # Normal test run - skips heavy tests go test ./pkg/auth/providers/aws # Explicit opt-in - downloads Chromium RUN_PLAYWRIGHT_INTEGRATION=1 go test -v ./pkg/auth/providers/aws -run Integration Unit tests (TestPlaywrightDriverDownload_ConsistentBehavior and TestPlaywrightDriverDownload_ConfigMapping) continue to run normally as they don't download browsers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds RUN_PLAYWRIGHT_INTEGRATION=1 to the acceptance tests step to enable validation of Playwright browser driver download functionality in CI. This ensures: - Browser driver download actually works on Linux, macOS, and Windows - Chromium installation is validated (160+ files) - Driver detection logic correctly identifies installed browsers - Integration with saml2aws works correctly The tests download ~140 MB of Chromium, but this is acceptable in CI to ensure this critical SAML authentication feature works correctly across all supported platforms. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1747 +/- ##
==========================================
+ Coverage 69.60% 69.65% +0.04%
==========================================
Files 397 398 +1
Lines 36288 36416 +128
==========================================
+ Hits 25260 25367 +107
- Misses 8720 8734 +14
- Partials 2308 2315 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Fixes two issues causing Windows test failures in CI: 1. **Integration test path isolation**: playwright-go on Windows uses os.UserCacheDir() which checks LOCALAPPDATA, not USERPROFILE. Added LOCALAPPDATA environment variable override to ensure browser downloads go to the test temp directory instead of the real user directory. 2. **Unit test isolation**: TestSAMLProvider_GetProviderType was failing because integration tests installed drivers to the user's cache directory, causing driver detection to return true when tests expected false. Added environment variable isolation to the unit test to ensure it runs in a clean environment regardless of whether integration tests ran first. Changes: - Set LOCALAPPDATA in integration tests for Windows compatibility - Isolate TestSAMLProvider_GetProviderType with temp directory environment - Ensures tests pass on Windows, macOS, and Linux consistently 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Fixes Windows test failure by correcting the expected cache directory path. Issue: When LOCALAPPDATA is set to testHomeDir on Windows, playwright-go's os.UserCacheDir() returns testHomeDir directly (not testHomeDir/AppData/Local). Playwright-go then appends "ms-playwright", resulting in: testHomeDir/ms-playwright The test was incorrectly expecting: testHomeDir/AppData/Local/ms-playwright This caused the test to check the wrong path and fail with "file not found". Fix: Update Windows cache path construction to match playwright-go's actual behavior when LOCALAPPDATA is overridden. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…rding
Bridge saml2aws's logrus output to Atmos's charmbracelet/log logger.
**Problem:**
Previous implementation used `logrus.SetOutput(io.Discard)` to suppress
saml2aws's logging output. This completely hid useful messages like
"opening browser" and "waiting..." from users.
**Solution:**
Created a logrus adapter (`logrusAdapter`) that implements `io.Writer` and
forwards all logrus output to Atmos's logger at debug level. This maintains
consistent log formatting while preserving visibility into SAML authentication flow.
**Changes:**
- Added `pkg/auth/providers/aws/logrus_adapter.go`:
- `logrusAdapter` implements `io.Writer` to forward logs to Atmos logger
- `ConfigureLogrusForAtmos()` configures global logrus instance:
- Sets output to adapter that forwards to Atmos logger
- Disables logrus timestamps/colors (Atmos handles formatting)
- Sets level to Info to avoid excessive debug logs
- Updated `pkg/auth/providers/aws/saml.go`:
- Changed from `logrus.SetOutput(io.Discard)` to `ConfigureLogrusForAtmos()`
- Removed unused `io` and `logrus` imports from saml.go
- Added `pkg/auth/providers/aws/logrus_adapter_test.go`:
- Tests adapter Write() implementation
- Tests ConfigureLogrusForAtmos() configuration
**Behavior:**
saml2aws messages like "INFO[0037] opening browser" now appear in Atmos's
logger output format at debug level, maintaining consistency with other
Atmos log messages.
**References:**
Similar pattern used in `pkg/store/artifactory_store_noop_logger.go` for
adapting third-party loggers.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Move logrus adapter from AWS provider-specific location to general logger package. **Problem:** The logrus adapter was located in `pkg/auth/providers/aws/` which implied it was AWS-specific, but it's actually a general-purpose adapter that can be reused anywhere in the codebase where we need to bridge logrus to Atmos logger. **Solution:** Moved `logrus_adapter.go` and `logrus_adapter_test.go` to `pkg/logger/` to make them available as general logger utilities. This allows any part of the codebase to configure third-party libraries using logrus to forward their logs to Atmos's logger. **Changes:** - Moved `pkg/auth/providers/aws/logrus_adapter.go` → `pkg/logger/logrus_adapter.go` - Moved `pkg/auth/providers/aws/logrus_adapter_test.go` → `pkg/logger/logrus_adapter_test.go` - Updated package declaration from `package aws` to `package logger` - Updated internal references from `log.Debug()` to `Debug()` (same package) - Updated `pkg/auth/providers/aws/saml.go` to call `log.ConfigureLogrusForAtmos()` - All tests pass after relocation **Benefits:** - Reusable across entire codebase - Clear location in logger utilities - No longer tied to AWS-specific code - Other packages can easily configure logrus for Atmos logging 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/auth/providers/aws/saml.go (1)
427-467: Enhanced driver detection with multiple cache locations.The refactored logic now checks XDG cache first (allowing consolidated Atmos data), then platform-specific defaults. The addition of Windows
LOCALAPPDATAsupport via viper is functional but could be simplified.Consider simplifying the
LOCALAPPDATAcheck:- if runtime.GOOS == "windows" { - v := viper.New() - if err := v.BindEnv("LOCALAPPDATA"); err == nil { - if localAppData := v.GetString("LOCALAPPDATA"); localAppData != "" { - playwrightPaths = append(playwrightPaths, filepath.Join(localAppData, playwrightCacheDir)) - } - } - } + if runtime.GOOS == "windows" { + if localAppData := os.Getenv("LOCALAPPDATA"); localAppData != "" { + playwrightPaths = append(playwrightPaths, filepath.Join(localAppData, playwrightCacheDir)) + } + }Using
os.Getenvdirectly is more straightforward unless you need viper's additional features for testing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pkg/auth/providers/aws/logrus_adapter.go(1 hunks)pkg/auth/providers/aws/logrus_adapter_test.go(1 hunks)pkg/auth/providers/aws/saml.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/providers/aws/logrus_adapter_test.gopkg/auth/providers/aws/logrus_adapter.gopkg/auth/providers/aws/saml.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
Files:
pkg/auth/providers/aws/logrus_adapter_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
Files:
pkg/auth/providers/aws/logrus_adapter_test.gopkg/auth/providers/aws/logrus_adapter.gopkg/auth/providers/aws/saml.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/auth/providers/aws/logrus_adapter.gopkg/auth/providers/aws/saml.go
🧠 Learnings (8)
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/auth/providers/aws/logrus_adapter_test.go
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
pkg/auth/providers/aws/logrus_adapter_test.gopkg/auth/providers/aws/logrus_adapter.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/auth/providers/aws/logrus_adapter_test.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
pkg/auth/providers/aws/logrus_adapter_test.gopkg/auth/providers/aws/logrus_adapter.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/auth/providers/aws/logrus_adapter_test.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/auth/providers/aws/logrus_adapter.gopkg/auth/providers/aws/saml.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
pkg/auth/providers/aws/logrus_adapter.go
📚 Learning: 2025-02-21T20:56:05.539Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Applied to files:
pkg/auth/providers/aws/logrus_adapter.go
🧬 Code graph analysis (3)
pkg/auth/providers/aws/logrus_adapter_test.go (1)
pkg/auth/providers/aws/logrus_adapter.go (1)
ConfigureLogrusForAtmos(40-54)
pkg/auth/providers/aws/logrus_adapter.go (1)
pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/providers/aws/saml.go (4)
pkg/auth/providers/aws/logrus_adapter.go (1)
ConfigureLogrusForAtmos(40-54)pkg/schema/schema_auth.go (1)
Provider(21-37)pkg/xdg/xdg.go (1)
GetXDGCacheDir(32-34)pkg/logger/log.go (1)
Debug(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
- GitHub Check: website-deploy-preview
- GitHub Check: Review Dependency Licenses
- GitHub Check: Analyze (go)
- GitHub Check: autofix
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (8)
pkg/auth/providers/aws/logrus_adapter.go (1)
1-54: Clean logrus integration implementation.The adapter pattern correctly routes logrus output to Atmos's logger. The
Writemethod properly implementsio.Writerby returning the input length, and trimming the trailing newline is correct since logrus appends one. The formatter configuration appropriately delegates timestamp and color handling to Atmos.pkg/auth/providers/aws/logrus_adapter_test.go (1)
1-49: Solid test coverage for the adapter.Both test functions properly verify the adapter behavior. The cleanup pattern in
TestConfigureLogrusForAtmoscorrectly preserves the original logrus configuration, preventing test pollution.pkg/auth/providers/aws/saml.go (6)
11-11: Imports support the new functionality.The
runtime,viper, andxdgimports enable platform detection, environment variable binding, and cache directory management for the enhanced driver detection logic.Also applies to: 18-18, 29-29
32-41: Constants correctly updated for Playwright cache.The change from
ms-playwright-gotoms-playwrightaligns with the actual Playwright cache directory naming convention. This fixes one of the root causes mentioned in the PR objectives.
132-136: Logrus integration properly positioned.Calling
ConfigureLogrusForAtmos()before creating the SAML client ensures saml2aws log messages are captured and formatted consistently with Atmos's logging style.
196-197: BrowserType and BrowserExecutablePath propagation looks correct.These fields are now passed through to the saml2aws
IDPAccount, enabling custom browser configuration as per the schema changes.
207-207: Critical fix: DownloadBrowser now set on LoginDetails.This addresses the root cause where the flag was set on
IDPAccountbut not onLoginDetails. Since saml2aws checksLoginDetails.DownloadBrowser, this fix enables automatic driver downloads when configured.
481-521: Proper validation of browser binary presence.The enhanced check ensures empty version directories aren't treated as valid installations. Iterating through version directories and confirming they contain actual files prevents false positives.
Make logrus log level dynamically match Atmos's current log level setting. **Problem:** The logrus adapter was hardcoded to InfoLevel, which meant that even when users set Atmos to debug/trace level, they wouldn't see debug logs from saml2aws or other libraries using logrus. **Solution:** - Added `atmosLevelToLogrus()` function to convert Atmos log levels to logrus levels - Modified `ConfigureLogrusForAtmos()` to call `GetLevel()` and set logrus level accordingly - Mapping: Trace/Debug → DebugLevel, Info → InfoLevel, Warn → WarnLevel, Error → ErrorLevel, Fatal → FatalLevel **Changes:** - `pkg/logger/logrus_adapter.go`: - Added `atmosLevelToLogrus()` helper function - Changed from hardcoded `logrus.InfoLevel` to `atmosLevelToLogrus(GetLevel())` - `pkg/logger/logrus_adapter_test.go`: - Enhanced `TestConfigureLogrusForAtmos` to test all log levels - Added `TestAtmosLevelToLogrus` to verify level conversion **Behavior:** Now when users run `atmos auth login --verbose` or set log level to debug/trace, they'll see saml2aws's debug logs (like browser automation details) formatted through Atmos's logger. The logrus level automatically matches whatever Atmos log level is currently active. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Skip automatic browser driver download when user specifies custom browser.
**Problem:**
When users configure a custom browser (via `browser_type` or `browser_executable_path`),
they're using their own browser installation and don't need Playwright-managed Chromium.
However, the auto-download logic didn't check for this, potentially downloading
unnecessary drivers.
**Solution:**
Enhanced `shouldDownloadBrowser()` to skip auto-download when either `browser_type`
or `browser_executable_path` is set. This indicates the user is bringing their own
browser and shouldn't trigger automatic downloads.
**Auto-download behavior:**
Auto-download is enabled by default ONLY when:
1. User hasn't explicitly set `download_browser_driver`
2. Driver is "Browser" (not GoogleApps/Okta/ADFS)
3. No custom browser configured (`browser_type` and `browser_executable_path` both empty)
4. No Playwright drivers found in cache
**Changes:**
- `pkg/auth/providers/aws/saml.go`:
- Added check for `BrowserType` and `BrowserExecutablePath` in `shouldDownloadBrowser()`
- Added debug logging when custom browser is detected
- Updated function comment to reflect custom browser logic
- `pkg/auth/providers/aws/saml_test.go`:
- Added test case: "custom browser_type specified, skips auto-download"
- Added test case: "custom browser_executable_path specified, skips auto-download"
- Added test case: "both custom browser fields specified, skips auto-download"
**Example configs:**
Auto-download enabled (default):
```yaml
providers:
my-saml:
kind: aws/saml
driver: Browser
# Auto-downloads Chromium on first use
```
Auto-download skipped (custom browser):
```yaml
providers:
my-saml:
kind: aws/saml
driver: Browser
browser_type: msedge
browser_executable_path: /usr/bin/microsoft-edge
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Remove unused provider_type backward compatibility check and deprecation warning. **Rationale:** The `provider_type` field is already marked as deprecated in the schema with a comment "Deprecated: use driver". However, the code was still checking for it and logging a warning. Since nobody is actually using `provider_type` in practice (checked infra-live configs), we should remove the compatibility code. **Changes:** - `pkg/auth/providers/aws/saml.go`: - Removed `provider_type` backward compatibility check from `getDriver()` - Removed deprecation warning log message - Simplified logic to only check `Driver` field - `pkg/auth/providers/aws/saml_test.go`: - Changed `ProviderType: "Okta"` to `Driver: "Okta"` in test cases - Updated `TestSAMLProvider_GetProviderType` explicit driver test - Updated `TestSAMLProvider_createSAMLConfig_AllFields` test **Note:** The `ProviderType` field remains in the schema (for now) to avoid breaking changes for anyone who might still have it in their configs, but the code no longer actively supports or warns about it. The schema comment already clearly indicates it's deprecated. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
pkg/auth/providers/aws/saml.go(7 hunks)pkg/auth/providers/aws/saml_test.go(5 hunks)pkg/logger/logrus_adapter.go(1 hunks)pkg/logger/logrus_adapter_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/logger/logrus_adapter.gopkg/auth/providers/aws/saml.gopkg/logger/logrus_adapter_test.gopkg/auth/providers/aws/saml_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
Files:
pkg/logger/logrus_adapter.gopkg/auth/providers/aws/saml.gopkg/logger/logrus_adapter_test.gopkg/auth/providers/aws/saml_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/logger/logrus_adapter.gopkg/auth/providers/aws/saml.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
Files:
pkg/logger/logrus_adapter_test.gopkg/auth/providers/aws/saml_test.go
🧠 Learnings (8)
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
pkg/logger/logrus_adapter.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
pkg/logger/logrus_adapter.gopkg/logger/logrus_adapter_test.go
📚 Learning: 2025-01-30T16:56:43.004Z
Learnt from: mcalhoun
Repo: cloudposse/atmos PR: 980
File: pkg/utils/log_utils.go:62-63
Timestamp: 2025-01-30T16:56:43.004Z
Learning: In pkg/utils/log_utils.go, LogTrace is intentionally redirected to LogDebug since charmbracelet logger doesn't support Trace level, maintaining backward compatibility with the original LogTrace functionality.
Applied to files:
pkg/logger/logrus_adapter.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/auth/providers/aws/saml.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/logger/logrus_adapter_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/logger/logrus_adapter_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/logger/logrus_adapter_test.gopkg/auth/providers/aws/saml_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Use table-driven tests for multiple scenarios
Applied to files:
pkg/auth/providers/aws/saml_test.go
🧬 Code graph analysis (4)
pkg/logger/logrus_adapter.go (1)
pkg/logger/log.go (11)
Debug(24-26)SetOutput(94-96)SetLevel(84-86)GetLevel(89-91)Level(203-203)TraceLevel(211-211)DebugLevel(213-213)InfoLevel(215-215)WarnLevel(217-217)ErrorLevel(219-219)FatalLevel(221-221)
pkg/auth/providers/aws/saml.go (3)
pkg/logger/logrus_adapter.go (1)
ConfigureLogrusForAtmos(38-51)pkg/schema/schema_auth.go (1)
Provider(21-37)pkg/xdg/xdg.go (1)
GetXDGCacheDir(32-34)
pkg/logger/logrus_adapter_test.go (2)
pkg/logger/log.go (10)
GetLevel(89-91)SetLevel(84-86)Level(203-203)TraceLevel(211-211)DebugLevel(213-213)InfoLevel(215-215)WarnLevel(217-217)ErrorLevel(219-219)FatalLevel(221-221)LevelToString(126-143)pkg/logger/logrus_adapter.go (1)
ConfigureLogrusForAtmos(38-51)
pkg/auth/providers/aws/saml_test.go (2)
pkg/schema/schema_auth.go (1)
Provider(21-37)pkg/auth/providers/aws/saml.go (1)
NewSAMLProvider(58-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: autofix
- GitHub Check: Review Dependency Licenses
- GitHub Check: website-deploy-preview
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
Fixes error: "Error saving storage state open /Users/user/.aws/saml2aws/storageState.json: no such file or directory" The saml2aws library hardcodes browser storage state to ~/.aws/saml2aws/storageState.json with no configuration option to override. To respect XDG Base Directory Specification while maintaining compatibility, this change: - Creates XDG-compliant directory: ~/.cache/atmos/aws-saml/<provider-name>/ - Creates symlink: ~/.aws/saml2aws → ~/.cache/atmos/aws-saml/<provider-name>/ - Ensures per-provider isolation to support multiple SAML configurations - Matches existing SSO cache pattern in pkg/auth/providers/aws/sso_cache.go Implementation details: - Added setupBrowserStorageDir() to create XDG directory and symlink - Added ensureStorageSymlink() to manage symlink creation/updates - Added isCorrectSymlink() helper to validate symlink correctness - Modified setupBrowserAutomation() to initialize storage directory - All functions handle existing directories and symlinks gracefully 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds test coverage for the three new storage functions: - setupBrowserStorageDir: Tests directory creation and symlink management - ensureStorageSymlink: Tests symlink creation, updates, and replacement - isCorrectSymlink: Tests symlink validation logic Test scenarios covered: - Creating new XDG directory structure and symlink - Handling existing correct symlinks (idempotent) - Replacing incorrect symlinks - Replacing existing directories with symlinks - Replacing regular files with symlinks - Error handling when storage setup fails gracefully Coverage improvements: - setupBrowserStorageDir: 73.3% - ensureStorageSymlink: 81.8% - isCorrectSymlink: 100% (all paths tested) All tests use isolated temporary directories and cross-platform environment variable overrides for reliable testing on Linux, macOS, and Windows. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/auth/providers/aws/saml_storage_test.go (1)
349-378: Verify graceful error handling; consider cleanup for test isolation.The test correctly verifies that setupBrowserAutomation continues despite storage setup failures. The approach of creating a file at ~/.aws (line 360) to trigger failure is effective.
Line 377 checks the
SAML2AWS_AUTO_BROWSER_DOWNLOADenvironment variable set by the function under test, but doesn't clean it up afterward. For better test isolation, consider adding cleanup:// Verify the environment variable was still set (proves function continued). + defer os.Unsetenv("SAML2AWS_AUTO_BROWSER_DOWNLOAD") assert.Equal(t, "true", os.Getenv("SAML2AWS_AUTO_BROWSER_DOWNLOAD"))This prevents the environment variable from leaking into subsequent tests in the same process. Based on learnings, cleanup is acceptable to skip, but this improves isolation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/auth/providers/aws/saml_storage_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/providers/aws/saml_storage_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
Files:
pkg/auth/providers/aws/saml_storage_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
Files:
pkg/auth/providers/aws/saml_storage_test.go
🧠 Learnings (2)
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/auth/providers/aws/saml_storage_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
pkg/auth/providers/aws/saml_storage_test.go
🧬 Code graph analysis (1)
pkg/auth/providers/aws/saml_storage_test.go (1)
pkg/auth/providers/aws/saml.go (1)
NewSAMLProvider(58-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
- GitHub Check: Build (linux)
- GitHub Check: autofix
- GitHub Check: Run pre-commit hooks
- GitHub Check: website-deploy-preview
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Review Dependency Licenses
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/auth/providers/aws/saml_storage_test.go (4)
14-153: Excellent table-driven test with comprehensive scenario coverage.This test thoroughly covers the setupBrowserStorageDir functionality across multiple scenarios: fresh creation, existing correct symlink (idempotency), incorrect symlink replacement, and directory-to-symlink replacement. The cross-platform environment variable setup (HOME/USERPROFILE/XDG_CACHE_HOME) and proper use of
os.Lstatfor symlink checking demonstrate solid test engineering.
155-249: Solid test coverage for ensureStorageSymlink edge cases.The test properly covers creation, idempotency, and replacement scenarios for various path types (wrong symlink, directory, file). The detail at lines 200-201 where a test file is added inside the directory to verify complete removal is a nice touch.
251-318: Complete validation test for symlink correctness checking.Covers all relevant scenarios for the isCorrectSymlink validator: correct symlink, wrong target, regular directory/file, and non-existent path. The boolean return validation is clean and straightforward.
320-347: Good integration test verifying storage setup flow.This test validates that setupBrowserAutomation actually triggers storage setup by checking for created directories and symlinks. Using the proper NewSAMLProvider factory ensures realistic initialization.
…t error drops Critical fix: The logrus adapter was routing all messages through Debug(), causing errors and warnings from saml2aws to be silently dropped when Atmos runs at Info level (the default). This made critical diagnostics invisible. Changes: - Parse log level from formatted logrus output (level=error, level=warn, etc.) - Route messages to appropriate Atmos log levels: Error, Warn, Info, Debug - Handle fatal, panic, error → Error level - Handle warn → Warn level - Handle debug, trace → Debug level - Default to Info for unrecognized or info-level messages - Case-insensitive level detection for robustness Test coverage: - Added TestLogrusAdapter_Write_PreservesLogLevel with 9 test cases - Covers all log levels: fatal, panic, error, warn, info, debug, trace - Tests mixed case handling and messages without explicit levels - Write function coverage: 100% This ensures saml2aws authentication errors are visible at default log levels, maintaining critical diagnostic capability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/logger/logrus_adapter_test.go (2)
10-24: Consider verifying actual logging output, not just return values.The test checks that
Writereturns the correct length and no error, but doesn't verify that messages are actually routed to the Atmos logger. Consider capturing the Atmos logger output to confirm the adapter's integration works end-to-end.
10-147: Missing error condition tests.Per coding guidelines, tests should cover both happy paths and error conditions. Consider adding tests for edge cases like:
Write()with nil or empty input- Malformed log messages that fail to parse
- Behavior when the Atmos logger is not configured
As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/logger/logrus_adapter.go(1 hunks)pkg/logger/logrus_adapter_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/logger/logrus_adapter.go
🧰 Additional context used
📓 Path-based instructions (3)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/logger/logrus_adapter_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
Files:
pkg/logger/logrus_adapter_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
Files:
pkg/logger/logrus_adapter_test.go
🧠 Learnings (4)
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/logger/logrus_adapter_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/logger/logrus_adapter_test.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
pkg/logger/logrus_adapter_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/logger/logrus_adapter_test.go
🧬 Code graph analysis (1)
pkg/logger/logrus_adapter_test.go (2)
pkg/logger/log.go (10)
GetLevel(89-91)SetLevel(84-86)Level(203-203)TraceLevel(211-211)DebugLevel(213-213)InfoLevel(215-215)WarnLevel(217-217)ErrorLevel(219-219)FatalLevel(221-221)LevelToString(126-143)pkg/logger/logrus_adapter.go (1)
ConfigureLogrusForAtmos(49-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
- GitHub Check: Build (windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Review Dependency Licenses
- GitHub Check: Analyze (go)
- GitHub Check: autofix
- GitHub Check: Lint (golangci)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/logger/logrus_adapter_test.go (2)
81-126: Solid test coverage here.Good job saving and restoring the original logrus configuration, testing all level mappings, and verifying formatter settings. This comprehensively validates the configuration function.
128-147: Clean and comprehensive level mapping test.All Atmos levels are covered, and the table-driven approach makes it easy to verify correctness.
| func TestLogrusAdapter_Write_PreservesLogLevel(t *testing.T) { | ||
| adapter := newLogrusAdapter() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| message string | ||
| }{ | ||
| { | ||
| name: "error level message", | ||
| message: "level=error msg=\"authentication failed\" provider=browser\n", | ||
| }, | ||
| { | ||
| name: "fatal level message", | ||
| message: "level=fatal msg=\"critical error\" component=saml2aws\n", | ||
| }, | ||
| { | ||
| name: "panic level message", | ||
| message: "level=panic msg=\"panic occurred\"\n", | ||
| }, | ||
| { | ||
| name: "warn level message", | ||
| message: "level=warn msg=\"retrying connection\" attempts=3\n", | ||
| }, | ||
| { | ||
| name: "info level message", | ||
| message: "level=info msg=\"authentication successful\"\n", | ||
| }, | ||
| { | ||
| name: "debug level message", | ||
| message: "level=debug msg=\"processing request\" url=https://idp.example.com\n", | ||
| }, | ||
| { | ||
| name: "trace level message", | ||
| message: "level=trace msg=\"detailed trace information\"\n", | ||
| }, | ||
| { | ||
| name: "message without level defaults to info", | ||
| message: "msg=\"some message without level\"\n", | ||
| }, | ||
| { | ||
| name: "mixed case level", | ||
| message: "LEVEL=ERROR msg=\"error in mixed case\"\n", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Test that Write successfully processes messages with different levels. | ||
| n, err := adapter.Write([]byte(tt.message)) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, len(tt.message), n) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test name doesn't match what's being verified.
The test is named TestLogrusAdapter_Write_PreservesLogLevel but only checks that Write() succeeds with different message formats. It doesn't verify that log levels are actually preserved or that messages are logged at the correct level in the Atmos logger. Either rename the test to reflect what it actually does (e.g., TestLogrusAdapter_Write_VariousFormats) or enhance it to verify log level translation.
Improves the logrus adapter implementation to use JSON parsing instead of string pattern matching, providing a more robust and maintainable solution. Changes: - Configure logrus with JSONFormatter instead of TextFormatter - Parse JSON output to extract level and structured fields - Forward structured fields as key-value pairs to Atmos logger - Graceful fallback for non-JSON messages (logs at Info level) - Preserve log levels: fatal/panic/error → Error, warn/warning → Warn, debug/trace → Debug, default → Info Benefits over string parsing: - More reliable: No dependency on text format variations - Structured logging: Preserves all logrus fields (provider, component, etc.) - Maintainable: JSON parsing is explicit and well-defined - Extensible: Easy to add support for additional logrus fields - Case-insensitive: Handles level variations automatically Test updates: - Updated tests to use JSON formatted messages - Added test for non-JSON fallback - Verified structured fields are preserved (provider, component, attempts, url) - Coverage: 100% on Write() and ConfigureLogrusForAtmos() Example structured output: 2025/11/04 13:55:51 ERRO authentication failed provider=browser 2025/11/04 13:55:51 WARN retrying connection attempts=3 This ensures saml2aws errors, warnings, and structured diagnostic info are visible at default log levels with proper formatting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…gfault Addresses segfault issue reported by user when custom browser is configured and authentication times out or browser window is closed prematurely. Root Cause Analysis: - saml2aws v2.36.19 has a nil pointer dereference bug at browser.go:191 - When browser timeout occurs, ExpectRequest() returns (nil, error) - Code then calls nil.PostData() causing segmentation violation - Cannot fix directly as it's in external dependency Diagnostic Improvements: 1. Add logging for custom browser configuration: - Log browser_type when configured - Log browser_executable_path when configured - Helps diagnose what browser user attempted to use 2. Add browser executable validation: - Check if file exists before authentication - Validate it's a file (not directory) - Check for execute permissions (Unix) - Log file size and permissions for diagnostics - Warn if execute permissions are missing 3. Document the saml2aws bug: - Added detailed comment in authenticateAndGetAssertion() - Explains the bug, location, and workarounds - User should not close browser window manually - Ensure authentication completes before timeout (5 minutes) Error handling: - Added ErrInvalidBrowserExecutable static error - Wrapped errors with proper error chain - Validation failures log warnings but don't block authentication Test coverage: - Added TestSAMLProvider_validateBrowserExecutable with 5 scenarios - Tests valid executable, non-executable file, missing file, directory, empty path - Validates error handling for all edge cases Example diagnostic output users will now see: INFO Custom browser type configured browser_type=chrome INFO Custom browser executable path configured browser_executable_path=/usr/bin/chrome WARN Browser executable validation failed error="file not found" path=/usr/bin/chrome This will help users diagnose configuration issues before hitting the saml2aws segfault. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/logger/logrus_adapter_test.go (1)
26-85: Still missing the level assertion. Appreciate the expanded table, but this test still just checks byte counts, so a regression could again drop error/warn logs without failing here. Please hook a stub or expose the adapter so we can assert thatWriteactually emits at Error/Warn/Info as intended.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
errors/errors.go(1 hunks)pkg/auth/providers/aws/saml.go(8 hunks)pkg/auth/providers/aws/saml_test.go(6 hunks)pkg/logger/logrus_adapter.go(1 hunks)pkg/logger/logrus_adapter_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/providers/aws/saml_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
Files:
errors/errors.gopkg/logger/logrus_adapter.gopkg/logger/logrus_adapter_test.gopkg/auth/providers/aws/saml.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
errors/errors.gopkg/logger/logrus_adapter.gopkg/auth/providers/aws/saml.go
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/logger/logrus_adapter.gopkg/logger/logrus_adapter_test.gopkg/auth/providers/aws/saml.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
Files:
pkg/logger/logrus_adapter_test.go
🧠 Learnings (12)
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
pkg/logger/logrus_adapter.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/logger/logrus_adapter.gopkg/auth/providers/aws/saml.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
pkg/logger/logrus_adapter.gopkg/logger/logrus_adapter_test.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/logger/logrus_adapter.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
pkg/logger/logrus_adapter.go
📚 Learning: 2024-11-10T19:28:17.365Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_utils.go:0-0
Timestamp: 2024-11-10T19:28:17.365Z
Learning: When TTY is not supported, log the downgrade message at the Warn level using `u.LogWarning(cliConfig, ...)` instead of `fmt.Println`.
Applied to files:
pkg/logger/logrus_adapter.go
📚 Learning: 2025-01-30T16:56:43.004Z
Learnt from: mcalhoun
Repo: cloudposse/atmos PR: 980
File: pkg/utils/log_utils.go:62-63
Timestamp: 2025-01-30T16:56:43.004Z
Learning: In pkg/utils/log_utils.go, LogTrace is intentionally redirected to LogDebug since charmbracelet logger doesn't support Trace level, maintaining backward compatibility with the original LogTrace functionality.
Applied to files:
pkg/logger/logrus_adapter.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
pkg/logger/logrus_adapter.go
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
pkg/logger/logrus_adapter.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/logger/logrus_adapter_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/logger/logrus_adapter_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/logger/logrus_adapter_test.go
🧬 Code graph analysis (3)
pkg/logger/logrus_adapter.go (1)
pkg/logger/log.go (12)
Warn(44-46)Debug(24-26)SetOutput(94-96)SetLevel(84-86)GetLevel(89-91)Level(203-203)TraceLevel(211-211)DebugLevel(213-213)InfoLevel(215-215)WarnLevel(217-217)ErrorLevel(219-219)FatalLevel(221-221)
pkg/logger/logrus_adapter_test.go (2)
pkg/logger/log.go (10)
GetLevel(89-91)SetLevel(84-86)Level(203-203)TraceLevel(211-211)DebugLevel(213-213)InfoLevel(215-215)WarnLevel(217-217)ErrorLevel(219-219)FatalLevel(221-221)LevelToString(126-143)pkg/logger/logrus_adapter.go (1)
ConfigureLogrusForAtmos(70-82)
pkg/auth/providers/aws/saml.go (4)
pkg/logger/logrus_adapter.go (1)
ConfigureLogrusForAtmos(70-82)pkg/schema/schema_auth.go (1)
Provider(21-37)pkg/xdg/xdg.go (1)
GetXDGCacheDir(32-34)errors/errors.go (1)
ErrInvalidBrowserExecutable(388-388)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build (macos)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: website-deploy-preview
- GitHub Check: autofix
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Review Dependency Licenses
- GitHub Check: Summary
| // Remove existing path if it exists (wrong symlink, directory, or file). | ||
| if _, err := os.Lstat(symlinkPath); err == nil { | ||
| log.Debug("Removing existing .aws/saml2aws", "path", symlinkPath) | ||
| if err := os.RemoveAll(symlinkPath); err != nil { | ||
| return fmt.Errorf("failed to remove existing .aws/saml2aws: %w", err) | ||
| } | ||
| } | ||
|
|
||
| // Create symlink. | ||
| if err := os.Symlink(targetPath, symlinkPath); err != nil { | ||
| return fmt.Errorf("failed to create symlink: %w", err) | ||
| } | ||
|
|
||
| log.Debug("Created browser storage symlink", "path", symlinkPath, "target", targetPath) | ||
| return nil |
There was a problem hiding this comment.
Don’t delete ~/.aws/saml2aws until the symlink is guaranteed. On Windows the default CreateSymbolicLink privilege isn’t granted, so os.Symlink routinely returns EPERM. Right now we RemoveAll the existing folder first, fail to create the link, log a warning, and leave users without a real ~/.aws/saml2aws directory. The next browser run then blows up trying to read/write storageState.json because the path vanished. Please guard this by staging the old directory and restoring it (or recreating a plain directory) when symlink creation fails. One option:
- if _, err := os.Lstat(symlinkPath); err == nil {
- log.Debug("Removing existing .aws/saml2aws", "path", symlinkPath)
- if err := os.RemoveAll(symlinkPath); err != nil {
- return fmt.Errorf("failed to remove existing .aws/saml2aws: %w", err)
- }
- }
-
- if err := os.Symlink(targetPath, symlinkPath); err != nil {
- return fmt.Errorf("failed to create symlink: %w", err)
- }
+ if info, err := os.Lstat(symlinkPath); err == nil {
+ if info.Mode()&os.ModeSymlink == 0 {
+ if err := os.Rename(symlinkPath, symlinkPath+".bak"); err != nil {
+ return fmt.Errorf("failed to stage existing storage dir: %w", err)
+ }
+ defer os.RemoveAll(symlinkPath + ".bak")
+ } else {
+ if err := os.Remove(symlinkPath); err != nil {
+ return fmt.Errorf("failed to remove stale symlink: %w", err)
+ }
+ }
+ }
+
+ if err := os.Symlink(targetPath, symlinkPath); err != nil {
+ _ = os.Rename(symlinkPath+".bak", symlinkPath)
+ return fmt.Errorf("failed to create symlink: %w", err)
+ }This keeps existing state intact and avoids breaking default Windows installs.
what
Fixes SAML authentication failing with "please install the driver (v1.47.2) and browsers first" error when
download_browser_driver: trueis configured.why
Root cause: The
DownloadBrowserflag was set on saml2awsIDPAccountconfig but NOT onLoginDetails, which is what saml2aws actually checks when deciding whether to download drivers.Additionally, the code was using incorrect Playwright cache directory paths (
ms-playwright-goinstead ofms-playwright), causing driver detection to fail.Changes
Code Changes
LoginDetails.DownloadBrowserincreateLoginDetails()to matchshouldDownloadBrowser()logicms-playwright-gotoms-playwright(actual playwright-go location)Testing
LoginDetails.DownloadBrowseris set correctly across all scenariosDocumentation
go run playwright install)download_browser_driver: trueas the recommended approachreferences
download_browser_driver: truestill failsSummary by CodeRabbit
New Features
Documentation
Tests