Fix SSO Login regression #4907
Fix SSO Login regression #4907
Conversation
WalkthroughThe changes refactor the API client management by introducing a new structure, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.64.8){"Issues":[{"FromLinter":"typecheck","Text":"pattern build/: no matching files found","Severity":"","SourceLines":["//go:embed build/"],"Pos":{"Filename":"webui/webui.go","Offset":0,"Line":23,"Column":12},"ExpectNoLint":false,"ExpectedNoLintLinter":""}],"Report":{"Linters":[{"Name":"asasalint"},{"Name":"asciicheck"},{"Name":"bidichk"},{"Name":"bodyclose","Enabled":true},{"Name":"canonicalheader"},{"Name":"containedctx"},{"Name":"contextcheck"},{"Name":"copyloopvar","Enabled":true},{"Name":"cyclop"},{"Name":"decorder"},{"Name":"deadcode"},{"Name":"depguard","Enabled":true},{"Name":"dogsled","Enabled":true},{"Name":"dupl"},{"Name":"dupword"},{"Name":"durationcheck"},{"Name":"errcheck","Enabled":true,"EnabledByDefault":true},{"Name":"errchkjson"},{"Name":"errname"},{"Name":"errorlint"},{"Name":"execinquery"},{"Name":"exhaustive"},{"Name":"exhaustivestruct"},{"Name":"exhaustruct"},{"Name":"exportloopref"},{"Name":"exptostd"},{"Name":"forbidigo","Enabled":true},{"Name":"forcetypeassert"},{"Name":"fatcontext"},{"Name":"funlen","Enabled":true},{"Name":"gci"},{"Name":"ginkgolinter"},{"Name":"gocheckcompilerdirectives"},{"Name":"gochecknoglobals"},{"Name":"gochecknoinits","Enabled":true},{"Name":"gochecksumtype"},{"Name":"gocognit"},{"Name":"goconst","Enabled":true},{"Name":"gocritic"},{"Name":"gocyclo","Enabled":true},{"Name":"godot"},{"Name":"godox"},{"Name":"err113"},{"Name":"gofmt","Enabled":true},{"Name":"gofumpt"},{"Name":"goheader"},{"Name":"goimports","Enabled":true},{"Name":"golint"},{"Name":"mnd","Enabled":true},{"Name":"gomnd"},{"Name":"gomoddirectives"},{"Name":"gomodguard"},{"Name":"goprintffuncname","Enabled":true},{"Name":"gosec","Enabled":true},{"Name":"gosimple","Enabled":true,"EnabledByDefault":true},{"Name":"gosmopolitan"},{"Name":"govet","Enabled":true,"EnabledByDefault":true},{"Name":"grouper"},{"Name":"ifshort"},{"Name":"iface"},{"Name":"importas"},{"Name":"inamedparam"},{"Name":"ineffassign","Enabled":true,"EnabledByDefault":true},{"Name":"interfacebloat"},{"Name":"interfacer"},{"Name":"intrange"},{"Name":"ireturn"},{"Name":"lll","Enabled":true},{"Name":"loggercheck"},{"Name":"maintidx"},{"Name":"makezero"},{"Name":"maligned"},{"Name":"mirror"},{"Name":"misspell"},{"Name":"musttag"},{"Name":"nakedret","Enabled":true},{"Name":"nestif"},{"Name":"nilerr"},{"Name":"nilnesserr"},{"Name":"nilnil"},{"Name":"nlreturn"},{"Name":"noctx","Enabled":true},{"Name":"nonamedreturns"},{"Name":"nosnakecase"},{"Name":"nosprintfhostport"},{"Name":"paralleltest"},{"Name":"perfsprint"},{"Name":"prealloc"},{"Name":"predeclared"},{"Name":"promlinter"},{"Name":"protogetter"},{"Name":"reassign"},{"Name":"recvcheck"},{"Name":"revive"},{"Name":"rowserrcheck"},{"Name":"sloglint"},{"Name":"scopelint"},{"Name":"sqlclosecheck"},{"Name":"spancheck"},{"Name":"staticcheck","Enabled":true,"EnabledByDefault":true},{"Name":"structcheck"},{"Name":"stylecheck","Enabled":true},{"Name":"tagalign"},{"Name":"tagliatelle"},{"Name":"tenv"},{"Name":"testableexamples"},{"Name":"testifylint"},{"Name":"testpackage"},{"Name":"thelper"},{"Name":"tparallel"},{"Name":"typecheck","Enabled":true,"EnabledByDefault":true},{"Name":"unconvert","Enabled":true},{"Name":"unparam"},{"Name":"unused","Enabled":true,"EnabledByDefault":true},{"Name":"usestdlibvars"},{"Name":"usetesting"},{"Name":"varcheck"},{"Name":"varnamelen"},{"Name":"wastedassign"},{"Name":"whitespace","Enabled":true},{"Name":"wrapcheck"},{"Name":"wsl"},{"Name":"zerologlint"},{"Name":"nolintlint","Enabled":true}]}} 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/util/api.go (1)
299-333: Well-structured implementation of shouldSkipAuthentication.The function correctly parses command paths and identifies specific commands that should bypass authentication. This implementation handles all the required cases including the "auth sso login" command that was previously failing.
Consider adding a comment explaining why these specific commands need to bypass authentication for better code maintainability.
func shouldSkipAuthentication(cmd *cobra.Command) bool { + // These commands need to bypass authentication either because they're informational + // or because they are part of the authentication process itself // Split command path by space and remove the first element (command itself) commandParts := strings.Split(strings.TrimSpace(cmd.CommandPath()), " ") if len(commandParts) <= 1 { return false } commandParts = commandParts[1:]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/util/api.go(3 hunks)cmd/util/api_test.go(2 hunks)pkg/publicapi/client/v2/client.go(0 hunks)test_integration/17_basic_auth_config_suite_test.go(1 hunks)test_integration/18_sso_auth_config_suite_test.go(1 hunks)test_integration/common_assets/nodes_configs/18_sso_auth_enabled_compute_node.yaml(1 hunks)test_integration/common_assets/nodes_configs/18_sso_auth_enabled_orchestrator.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/publicapi/client/v2/client.go
🧰 Additional context used
🧬 Code Definitions (3)
test_integration/17_basic_auth_config_suite_test.go (2)
test_integration/18_sso_auth_config_suite_test.go (4) (4)
s(25-39)s(41-44)s(46-53)s(55-74)test_integration/base_suite.go (10) (10)
s(31-71)s(73-81)s(83-86)s(88-91)s(93-110)s(112-123)s(125-153)s(155-157)s(159-185)s(187-221)
cmd/util/api.go (2)
pkg/publicapi/client/v2/api.go (1) (1)
New(37-39)pkg/config/config.go (1) (1)
New(95-242)
test_integration/18_sso_auth_config_suite_test.go (1)
test_integration/17_basic_auth_config_suite_test.go (8) (8)
s(29-43)s(45-48)s(50-141)s(143-151)s(153-172)s(174-189)s(191-210)s(212-236)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testcontainers-suite / tests
🔇 Additional comments (11)
test_integration/common_assets/nodes_configs/18_sso_auth_enabled_compute_node.yaml (1)
1-9: Configuration looks good for SSO-enabled compute node.The configuration correctly sets up a compute node with the necessary authentication token. This will enable proper integration testing of the SSO login flow.
test_integration/common_assets/nodes_configs/18_sso_auth_enabled_orchestrator.yaml (1)
1-21: Well-structured OAuth2 configuration for SSO testing.The orchestrator configuration includes all necessary OAuth2 parameters for complete SSO testing, including authorization endpoints, token endpoints, and appropriate scopes.
cmd/util/api.go (2)
71-74: Good implementation to skip authentication for specific commands.This conditional check correctly addresses the core issue by bypassing authentication for commands that shouldn't require it, including the SSO login command.
127-127: Simplified authentication flow logic.The code now correctly determines when to use the new authentication flow without relying on the removed skipAuthentication flag.
cmd/util/api_test.go (1)
509-593: Comprehensive test coverage for shouldSkipAuthentication.The tests cover all expected command paths and edge cases, ensuring that only the appropriate commands bypass authentication. The command hierarchy creation logic is particularly well implemented.
test_integration/17_basic_auth_config_suite_test.go (2)
174-189: Good addition of test forauth infowithout credentialsThis test properly validates the output of the
bacalhau auth infocommand when no authentication credentials are provided. It ensures all credential fields show as "Not Set" and verifies that the server doesn't support SSO login.
191-210: Excellent test coverage for authentication bypass commandsThis test properly verifies that essential commands (
bacalhau version,bacalhau agent version,bacalhau agent alive) work without authentication. This directly supports the PR's goal of fixing the SSO login regression by ensuring these commands properly bypass authentication.Note that this test complements the existing
TestCommandsBypassingAuthentication(lines 143-151), providing more comprehensive coverage with additional test cases.test_integration/18_sso_auth_config_suite_test.go (4)
1-44: Well-structured test suite setup for SSO authentication testingThe test suite is properly set up with all necessary initialization, using the SSO-specific configuration files (lines 29 and 32). The structure follows the existing pattern used in other test suites.
46-53: Good test for SSO login command authentication bypassThis test verifies that the
bacalhau auth sso logincommand can proceed far enough to attempt the SSO login flow without requiring authentication first. This is critical to the PR's objective of fixing the SSO login regression.The comment on line 47 correctly explains that the error being returned is expected and is being used to verify that the authentication configuration was successfully fetched without being blocked by authentication requirements.
55-74: LGTM - Comprehensive test for authentication-bypassing commandsThis test effectively verifies that essential commands can bypass authentication in an SSO-enabled environment. It mirrors the similar test in the
BasicAuthConfigSuitebut provides coverage specifically for SSO authentication configurations.
76-78: LGTM - Proper test registrationThe test suite is correctly registered with the testing framework using
suite.Run().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test_integration/13_agent_license_inspect_suite_test.go (1)
49-56: Improved test assertions for better failure diagnosisThe refactoring from a single multiline string assertion to individual contains checks for each expected output line is a good improvement. This approach provides more precise failure messages that pinpoint exactly which expected content is missing, making debugging easier.
However, I noticed that similar test methods in other files (like
TestValidateLocalLicense,TestValidateRemoteLicenseWithMetadata, andTestValidateRemoteExpiredLicense) still use the old approach with a single multiline string assertion. For consistency, consider updating those tests to follow this improved pattern as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
test_integration/13_agent_license_inspect_suite_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
test_integration/13_agent_license_inspect_suite_test.go (3)
test_integration/12_local_license_inspect_suite_test.go (6) (6)
s(24-34)s(36-39)s(41-61)s(63-83)s(85-121)s(123-133)test_integration/14_agent_license_inspect_with_metadata_suite_test.go (4) (4)
s(24-34)s(36-39)s(41-59)s(61-85)test_integration/16_agent_license_inspect_expired_suite_test.go (3) (3)
s(24-34)s(36-39)s(41-59)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testcontainers-suite / tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (23)
cmd/cli/agent/alive.go(1 hunks)cmd/cli/agent/config.go(1 hunks)cmd/cli/agent/license/inspect.go(1 hunks)cmd/cli/agent/node.go(1 hunks)cmd/cli/agent/version.go(1 hunks)cmd/cli/auth/info.go(1 hunks)cmd/cli/auth/sso/login.go(1 hunks)cmd/cli/docker/docker_run.go(1 hunks)cmd/cli/job/describe.go(1 hunks)cmd/cli/job/executions.go(1 hunks)cmd/cli/job/get.go(1 hunks)cmd/cli/job/history.go(1 hunks)cmd/cli/job/list.go(1 hunks)cmd/cli/job/logs.go(1 hunks)cmd/cli/job/run.go(2 hunks)cmd/cli/job/stop.go(1 hunks)cmd/cli/node/action.go(1 hunks)cmd/cli/node/describe.go(1 hunks)cmd/cli/node/list.go(1 hunks)cmd/cli/version/version.go(1 hunks)cmd/cli/wasm/wasm_run.go(2 hunks)cmd/util/api.go(3 hunks)cmd/util/api_test.go(2 hunks)
🧰 Additional context used
🧬 Code Definitions (9)
cmd/cli/node/action.go (1)
cmd/util/api.go (1) (1)
NewAPIClientManager(34-41)
cmd/cli/agent/license/inspect.go (1)
cmd/util/api.go (1) (1)
NewAPIClientManager(34-41)
cmd/cli/docker/docker_run.go (1)
cmd/util/api.go (1) (1)
NewAPIClientManager(34-41)
cmd/cli/job/logs.go (1)
cmd/util/api.go (1) (1)
NewAPIClientManager(34-41)
cmd/cli/node/list.go (1)
cmd/util/api.go (1) (1)
NewAPIClientManager(34-41)
cmd/cli/job/executions.go (1)
cmd/util/api.go (1) (1)
NewAPIClientManager(34-41)
cmd/cli/agent/node.go (1)
cmd/util/api.go (1) (1)
NewAPIClientManager(34-41)
cmd/util/api.go (4)
pkg/publicapi/client/v2/api.go (3) (3)
API(6-11)New(37-39)NewAPI(33-35)cmd/util/tokens.go (1) (1)
WriteToken(73-86)pkg/publicapi/apimodels/constants.go (7) (7)
HTTPHeaderBacalhauGitVersion(18-18)HTTPHeaderBacalhauGitCommit(20-20)HTTPHeaderBacalhauBuildDate(22-22)HTTPHeaderBacalhauBuildOS(24-24)HTTPHeaderBacalhauArch(26-26)HTTPHeaderBacalhauInstanceID(29-29)HTTPHeaderBacalhauInstallationID(28-28)pkg/repo/sysmeta.go (2) (2)
sysmeta(177-177)LoadSystemMetadata(37-53)
cmd/cli/auth/sso/login.go (1)
cmd/util/api.go (1) (1)
NewAPIClientManager(34-41)
🪛 GitHub Check: lint / go-lint (ubuntu-latest)
cmd/util/api.go
[failure] 53-53:
ineffectual assignment to err (ineffassign)
🪛 GitHub Actions: Main Pipeline
cmd/util/api.go
[error] 53-53: ineffectual assignment to err (ineffassign)
🔇 Additional comments (26)
cmd/cli/node/action.go (1)
38-38: Consistent implementation of the API client management refactorThe change from using a direct API client creation to the new
APIClientManagerpattern is appropriate. This approach correctly uses the authenticated API client since the action commands require authentication.cmd/cli/job/describe.go (1)
72-72: Properly implements the new API client management patternThe change correctly utilizes the new
APIClientManagerto get an authenticated API client, which is appropriate for job description operations requiring authentication.cmd/cli/auth/sso/login.go (1)
46-46: Key fix for the SSO login regressionThis change addresses the core issue mentioned in the PR objectives. By explicitly using
GetUnauthenticatedAPIClient()instead of an authenticated client, the SSO login flow now correctly bypasses authentication as intended. This is crucial for the login process since the user is not yet authenticated.cmd/cli/job/history.go (1)
72-72: Consistent implementation of the API client management refactorThe change appropriately switches to the new
APIClientManagerpattern and correctly uses an authenticated client for job history operations, which require authentication.cmd/cli/job/stop.go (1)
74-74: LGTM: API client creation updated to use new APIClientManager patternThis change correctly updates the API client instantiation to use the new
APIClientManagerstructure, which improves the authentication flow management.cmd/cli/job/executions.go (1)
74-74: LGTM: API client creation refactored consistentlyThe API client creation method has been properly updated to match the new pattern used throughout the codebase, ensuring consistent authentication handling.
cmd/cli/job/get.go (1)
62-62: LGTM: Correctly refactored to use APIClientManagerThe change appropriately implements the new API client instantiation pattern, maintaining consistent authentication behavior.
cmd/cli/job/list.go (1)
82-82: LGTM: API client creation updated properlyThis change correctly implements the new API client instantiation pattern using the
APIClientManager, which aligns with the overall refactoring to fix the SSO login regression.cmd/cli/auth/info.go (1)
48-48: LGTM: Good change to explicitly use unauthenticated clientChanging from
GetAPIClientV2to explicitly usingGetUnauthenticatedAPIClient()makes the authentication flow clearer and more predictable. This is appropriate for theauth infocommand which should work without requiring prior authentication.cmd/cli/job/logs.go (1)
59-59: LGTM: Appropriate use of authenticated clientThe job logs command correctly uses
GetAuthenticatedAPIClient()which makes the auth requirement explicit. This is appropriate since accessing job logs should require proper authentication.cmd/cli/agent/config.go (1)
29-29: LGTM: Properly requires authenticationUsing
GetAuthenticatedAPIClient()for agent configuration is appropriate as this operation should be secured with authentication. The refactoring improves code clarity by making the authentication requirement explicit.cmd/cli/version/version.go (1)
69-69: LGTM: Correctly uses unauthenticated client for version infoUsing
GetUnauthenticatedAPIClient()for the version command aligns with best practices. Version information should be accessible without requiring authentication, making this change appropriate and resolving part of the SSO login regression issue.cmd/cli/agent/license/inspect.go (1)
40-40: Implementation improved with structured API client management.The change from
util.GetAPIClientV2(cmd, cfg)toutil.NewAPIClientManager(cmd, cfg).GetAuthenticatedAPIClient()enhances the code structure by clearly indicating authentication requirements for this license inspection command.cmd/cli/agent/node.go (1)
40-40: Appropriate use of authenticated API client.The switch to structured API client management using
NewAPIClientManagerwithGetAuthenticatedAPIClient()makes sense here as node information retrieval requires authentication. This change is consistent with the broader refactoring across the codebase.cmd/cli/agent/alive.go (1)
39-39: Good usage of unauthenticated client for the alive command.Excellent choice using
GetUnauthenticatedAPIClient()instead of the authenticated version. The alive endpoint likely doesn't require authentication since it's just checking server health, making this the right approach to fix the SSO Login regression by bypassing authentication when appropriate.cmd/cli/node/describe.go (1)
43-43: Correctly using authenticated client for node information retrieval.The change to use
util.NewAPIClientManager(cmd, cfg).GetAuthenticatedAPIClient()is appropriate as retrieving detailed node information is a privileged operation that requires authentication. This maintains consistency with the authentication pattern established across the codebase.cmd/cli/job/run.go (1)
73-73: LGTM: Improved API client managementThe change to use the new
APIClientManagerpattern with an authenticated client makes the authentication requirements clearer and aligns with the overall refactoring to fix the SSO login regression.cmd/cli/docker/docker_run.go (1)
97-97: LGTM: Adoption of API client manager patternConsistent with the codebase refactoring, replacing direct API client creation with the manager pattern that explicitly requests an authenticated client. This makes authentication requirements clearer and helps fix the SSO login regression issue.
cmd/cli/agent/version.go (1)
40-40: LGTM: Appropriate use of unauthenticated clientThe version command correctly uses an unauthenticated client, which is appropriate since retrieving version information shouldn't require authentication. This explicit distinction is part of the solution to the SSO login regression issue.
cmd/cli/node/list.go (1)
58-58: LGTM: Consistent use of API client managerThe change follows the established pattern, using the new
APIClientManagerto explicitly request an authenticated client for the node list command. This approach improves code clarity regarding authentication requirements.cmd/cli/wasm/wasm_run.go (1)
110-110: API client initialization refactored to use the new APIClientManager.The change from
util.GetAPIClientV2(cmd, cfg)toutil.NewAPIClientManager(cmd, cfg).GetAuthenticatedAPIClient()aligns with the new API client management structure, providing better separation of concerns between authenticated and unauthenticated clients.cmd/util/api_test.go (1)
512-692: Comprehensive test coverage for API request options generation.Good job on adding thorough test cases for validating the behavior of
generateAPIRequestsOptionsunder various configurations, including TLS settings, CA file handling, and header generation. The test structure with setup, verification, and expected error validation is well-organized.cmd/util/api.go (4)
28-41: Good refactoring of API client management with a dedicated manager struct.The new
APIClientManagerstructure encapsulates the command, configuration, and base URL, providing a cleaner organization of responsibilities. The constructor function properly initializes all fields.
43-50: Clean implementation of the unauthenticated client method.The
GetUnauthenticatedAPIClientmethod effectively handles the creation of an unauthenticated API client with the appropriate request options.
108-109: Authentication flow simplified by removing SkipAuthentication flag.The code now directly determines whether to use the new authentication flow based on the presence of API key or SSO credentials, which simplifies the logic and addresses the SSO login regression mentioned in the PR objectives.
125-166: Well-encapsulated API request options generation function.The
generateAPIRequestsOptionsfunction nicely consolidates the logic for setting up TLS options and headers, making the code more maintainable. It includes thorough error checking for CA file existence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cmd/util/api.go (5)
34-41: Consider using the discard operator for unused return values.When calling
ConstructAPIEndpoint, you're ignoring the second return value (scheme) without explicitly discarding it with_. It's better practice to use the discard operator for clarity.- baseURL, _ := ConstructAPIEndpoint(cfg.API) + baseURL, _ := ConstructAPIEndpoint(cfg.API)
44-44: Ensure consistent variable naming across methods.Variable names should be consistent across similar methods to improve readability and maintainability:
- apiRequestOptions, err := generateAPIRequestsOptions(cm.cfg) + apiRequestsOptions, err := generateAPIRequestsOptions(cm.cfg)or consistently use
apiRequestOptionsin both methods.Also applies to: 53-53
52-127: Consider breaking down the GetAuthenticatedAPIClient method.This method is quite complex and handles multiple responsibilities. Consider breaking it down into smaller, focused methods for better maintainability and testability. For example, you could extract methods for credential resolution, token reading, and client creation.
99-99: Fix typos in comments.- // Legacy Auth FLow + // Legacy Auth FlowAlso applies to: 106-106
149-156: Consider consistent error handling for non-critical errors.The error handling for system metadata loading differs from other parts of the function. Since you're continuing execution after logging the error, consider adding a comment explaining that this is intentional to clarify that system metadata is optional for API requests.
sysmeta, err := repo.LoadSystemMetadata(cfg.DataDir) if err == nil { if sysmeta.InstanceID != "" { headers[apimodels.HTTPHeaderBacalhauInstanceID] = []string{sysmeta.InstanceID} } } else { + // Continue without system metadata as it's not critical for API requests log.Debug().Err(err).Msg("failed to load system metadata from repo path") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
cmd/util/api.go(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
cmd/util/api.go (4)
pkg/publicapi/client/v2/api.go (3) (3)
API(6-11)New(37-39)NewAPI(33-35)cmd/util/tokens.go (1) (1)
WriteToken(73-86)pkg/publicapi/client/v2/options.go (4) (4)
OptionFn(61-61)WithCACertificate(73-77)WithInsecureTLS(80-84)WithHeaders(116-120)pkg/publicapi/apimodels/constants.go (7) (7)
HTTPHeaderBacalhauGitVersion(18-18)HTTPHeaderBacalhauGitCommit(20-20)HTTPHeaderBacalhauBuildDate(22-22)HTTPHeaderBacalhauBuildOS(24-24)HTTPHeaderBacalhauArch(26-26)HTTPHeaderBacalhauInstanceID(29-29)HTTPHeaderBacalhauInstallationID(28-28)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testcontainers-suite / tests
🔇 Additional comments (3)
cmd/util/api.go (3)
28-32: Good job introducing APIClientManager to encapsulate client creation.This structure nicely encapsulates the command, configuration, and base URL, which aligns with the separation of concerns principle. This refactoring makes the authentication flow much clearer by providing dedicated methods for authenticated and unauthenticated clients.
43-50: Good implementation of the unauthenticated client method.Creating a dedicated method for unauthenticated API clients helps solve the SSO login regression by providing a clear path for commands that should bypass authentication.
129-170: Well-structured helper function for generating API request options.This function nicely consolidates the logic for constructing headers and TLS configurations, making the code more maintainable and testable. The error handling for the CA certificate file is thorough.
Fix SSO Login regression
The command
bacalhau auth sso loginwas failing since we did not skip authentication flow for it.This PR fixes the regression, improved the code flow, and added integration tests to catch similar regressions.
Linear: https://linear.app/expanso/issue/ENG-736/fix-sso-login-regression
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Tests