Modify auth sso login syntax and add logout command #4904
Conversation
WalkthroughThis pull request restructures the SSO command functionality. The main auth command now sources its SSO subcommand from a newly introduced SSO root command. Refactoring in the SSO login logic includes renaming types, functions, and tests to focus on login-specific functionality. Additionally, a new SSO logout feature has been implemented with a corresponding suite of tests. These changes involve package renaming from "auth" to "sso", updates to command usage, and the addition of confirmation and force flag logic for logout. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as SSO Logout Cmd
participant O as LogoutOptions
participant S as Util/Token Handler
U->>C: Trigger logout command (with/without --force)
C->>S: Call SetupRepoConfig to initialize repo
alt Force flag not set
C->>U: Prompt for confirmation
U-->>C: Return confirmation (Yes/No)
end
C->>O: Execute runSSOLogout()
O->>S: Retrieve token file path and invoke WriteToken to clear token
S-->>O: Return success/failure response
O-->>C: Return logout outcome
C->>U: Display logout confirmation or error
Assessment against linked issues
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: 1
🧹 Nitpick comments (4)
cmd/cli/auth/sso/login_test.go (1)
18-18: Fix comment inconsistency in assertion.The assertion correctly checks for "login" as the command use, but the comment still refers to "sso".
- assert.Equal(t, "login", cmd.Use, "Command use should be 'sso'") + assert.Equal(t, "login", cmd.Use, "Command use should be 'login'")cmd/cli/auth/sso/logout.go (2)
49-49: Use the underscore variable for unused return valueThe
urlSchemereturned byConstructAPIEndpointis not used in this function.- apiURL, _ := util.ConstructAPIEndpoint(cfg.API) + apiURL, _ := util.ConstructAPIEndpoint(cfg.API)
63-70: Add check for existing token before logoutThe current implementation doesn't check if a token exists before attempting to delete it. It would be more user-friendly to provide different messaging based on whether a token actually exists.
authTokenPath, err := cfg.JWTTokensPath() if err != nil { return bacerrors.New("unable to find local SSO session file") } + // Check if token exists + existingCred, readErr := util.ReadToken(authTokenPath, apiURL) + if readErr != nil { + return bacerrors.New("unable to read SSO session credentials") + } + + if existingCred == nil { + fmt.Fprintf(cmd.OutOrStdout(), "\nNo active session found for %s\n", apiURL) + return nil + } if err = util.WriteToken(authTokenPath, apiURL, nil); err != nil { return bacerrors.New("unable to delete SSO session credentials") }cmd/cli/auth/sso/logout_test.go (1)
81-101: Simplify input simulation in the testThe test uses two different mechanisms to simulate user input: setting the command's input with
cmd.SetIn(inBuf)and manipulatingos.Stdindirectly with a pipe. This is redundant and potentially confusing.Choose one approach for consistency - preferably using just
cmd.SetIn()as it's cleaner:// Create a buffer for input simulation with "y" response inBuf := bytes.NewBufferString("y\n") outBuf := new(bytes.Buffer) // Set up the command's input and output cmd.SetIn(inBuf) cmd.SetOut(outBuf) - // Create a new reader for Scanf - oldStdin := os.Stdin - defer func() { os.Stdin = oldStdin }() - - // Create a pipe and use it for stdin - r, w, _ := os.Pipe() - os.Stdin = r - - // Write "y" to the pipe - go func() { - w.Write([]byte("y\n")) - w.Close() - }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/cli/auth/root.go(2 hunks)cmd/cli/auth/sso/login.go(4 hunks)cmd/cli/auth/sso/login_test.go(2 hunks)cmd/cli/auth/sso/logout.go(1 hunks)cmd/cli/auth/sso/logout_test.go(1 hunks)cmd/cli/auth/sso/root.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
cmd/cli/auth/root.go (1)
cmd/cli/auth/sso/root.go (1) (1)
NewSSORootCmd(7-16)
cmd/cli/auth/sso/root.go (2)
cmd/cli/auth/sso/login.go (1) (1)
NewSSOLoginCmd(39-66)cmd/cli/auth/sso/logout.go (1) (1)
NewSSOLogoutCmd(25-45)
cmd/cli/auth/sso/login_test.go (1)
cmd/cli/auth/sso/login.go (1) (1)
NewSSOLoginCmd(39-66)
cmd/cli/auth/sso/logout.go (1)
cmd/cli/auth/sso/login.go (1) (1)
o(69-156)
🔇 Additional comments (13)
cmd/cli/auth/sso/root.go (1)
1-16: Well-structured SSO root command implementation.This is a clean implementation of the SSO root command that properly organizes login and logout as subcommands. The hierarchy follows good CLI design patterns.
cmd/cli/auth/root.go (2)
4-4: LGTM: Appropriate import for the new SSO package.The import statement is correctly added to access the new SSO root command implementation.
17-17: LGTM: Command structure properly updated.The command hierarchy has been successfully updated to use the new SSO root command, which will improve CLI organization.
cmd/cli/auth/sso/login_test.go (1)
1-1: LGTM: Package name and test function properly updated.The package name and test function have been correctly updated to match the new structure and naming conventions.
Also applies to: 13-15
cmd/cli/auth/sso/login.go (3)
1-1: LGTM: Package and type names effectively refactored.The package has been correctly renamed to "sso" and the types have been renamed to more clearly indicate their specific purpose for login functionality.
Also applies to: 25-37
39-66: LGTM: Command structure and function properly updated.The command creation function has been successfully refactored to reflect its specific login purpose, with the appropriate use of "login" instead of "sso".
68-156: LGTM: Method correctly renamed for clarity.The method has been appropriately renamed from
runSSOtorunSSOLoginto better reflect its specific functionality, maintaining consistency with the overall refactoring approach.cmd/cli/auth/sso/logout.go (2)
13-23: Excellent struct design and initializationThe LogoutOptions struct and its constructor are well-designed, with clear documentation and appropriate default values.
25-45: Well-structured command implementationThe command structure follows Cobra best practices with clear usage descriptions, appropriate argument validation, and proper flag setup.
cmd/cli/auth/sso/logout_test.go (4)
15-35: Well-designed test helpersThe test helper functions
setupTestCmdandcreateTempTokenFileare well-designed and facilitate clean, isolated test cases.
37-65: Comprehensive force flag testThe
TestLogout_ForceFlagfunction thoroughly tests the force flag behavior, including verifying the output message and token file updates.
119-148: Excellent test for cancellation scenarioThe
TestLogout_ConfirmNofunction properly tests the cancellation path, ensuring the token remains unchanged and the correct message is displayed.
150-162: Thorough command structure verificationThe
TestNewSSOLogoutCmdfunction effectively verifies all aspects of the command structure, including the force flag configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/cli/auth/sso/logout_test.go (1)
82-101: Redundant input simulation in TestLogout_ConfirmYesThere appears to be redundant input simulation. The test sets both
cmd.SetIn(inBuf)(line 87) and also manipulatesos.Stdindirectly (lines 90-101). This is unnecessary and could lead to inconsistent behavior.- // Create a buffer for input simulation with "y" response - inBuf := bytes.NewBufferString("y\n") - outBuf := new(bytes.Buffer) - - // Set up the command's input and output - cmd.SetIn(inBuf) - cmd.SetOut(outBuf) - - // Create a new reader for Scanf - oldStdin := os.Stdin - defer func() { os.Stdin = oldStdin }() - - // Create a pipe and use it for stdin - r, w, _ := os.Pipe() - os.Stdin = r - - // Write "y" to the pipe - go func() { - w.Write([]byte("y\n")) - w.Close() - }() + // Create input/output buffers + inBuf := bytes.NewBufferString("y\n") + outBuf := new(bytes.Buffer) + + // Set up the command's input and output + cmd.SetIn(inBuf) + cmd.SetOut(outBuf)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
cmd/cli/auth/sso/logout.go(1 hunks)cmd/cli/auth/sso/logout_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cli/auth/sso/logout.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testcontainers-suite / tests
🔇 Additional comments (7)
cmd/cli/auth/sso/logout_test.go (7)
1-13: Clean and well-structured importsThe package declaration and imports are organized correctly, with a clear separation between standard library, third-party, and internal packages.
15-35: Well-designed test helper functionsGood practice implementing reusable helper functions for test setup. The
createTempTokenFilefunction correctly handles cleanup with the returned function, ensuring resources are properly managed.
37-65: Comprehensive test for force logout functionalityThe test correctly validates the force logout scenario, including:
- Proper setup with a mock token file
- Verification of command output
- Confirmation that the token file is properly updated
This ensures the force flag works as expected.
67-81: Well-structured confirmation testThe test correctly validates the confirmation scenario with a "yes" response, including proper cleanup and token verification.
Also applies to: 102-117
119-148: Complete negative test case for logout cancellationGood test coverage for the case when a user cancels the logout with "n". The test properly verifies:
- The cancellation message is displayed
- The token file remains unchanged
This ensures the command respects user choice.
150-179: Edge case handling for empty confirmation inputExcellent coverage of the edge case when a user just presses Enter. The test properly treats empty input as cancellation, which is a good UX practice.
181-193: Verify command structure and flagsThe test properly validates the structural aspects of the command:
- Command name and description
- Force flag properties (shorthand and default value)
This helps ensure the command interface remains consistent.
Add SSO Logout Command and Improve SSO Authentication Structure
This PR enhances the SSO authentication functionality by adding a logout command and improving the overall command structure. The changes provide users with a complete authentication lifecycle, allowing them to both login and logout from their SSO sessions.
Changes
logoutcommand to SSO authenticationauth ssonamespaceNew Commands
bacalhau auth sso login- Login using SSO (existing command, moved)bacalhau auth sso logout- Logout from current SSO session (new command)-f, --forceflag to skip confirmation promptUsage Examples
Checklist
Linear: https://linear.app/expanso/issue/ENG-714/add-logout-option-for-sso
Summary by CodeRabbit