Skip to content

Add --interactive option for ha auth reset#558

Merged
sairon merged 3 commits into
masterfrom
interactive-auth-reset
Apr 9, 2025
Merged

Add --interactive option for ha auth reset#558
sairon merged 3 commits into
masterfrom
interactive-auth-reset

Conversation

@sairon
Copy link
Copy Markdown
Member

@sairon sairon commented Apr 8, 2025

Add option to perform interactive reset of user password. In that case, when no username argument is provided, a list of existing users is shown first. With username selected or given, prompt for a password is shown, read with no echo to the terminal for privacy.

This can be especially useful when the username contains characters that can't be entered in the OS console. While the framebuffer console still may not show all the characters, it may give enough clues to select the users for whom the password should be reset. See:
home-assistant/operating-system#3879

Summary by CodeRabbit

  • New Features
    • Enhanced input methods now validate numeric entries and provide secure password handling with optional confirmation.
    • The authentication reset process has been revamped with an interactive mode, allowing users to select a username and input their password more flexibly, along with improved feedback for input errors.

@sairon sairon requested a review from agners April 8, 2025 17:15
Add option to perform interactive reset of user password. In that case,
when no username argument is provided, a list of existing users is shown
first. With username selected or given, prompt for a password is shown,
read with no echo to the terminal for privacy.

This can be especially useful when the username contains characters that
can't be entered in the OS console. While the framebuffer console still
may not show all the characters, it may give enough clues to select the
users for whom the password should be reset. See:
home-assistant/operating-system#3879
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 8, 2025

📝 Walkthrough

Walkthrough

The changes introduce two new functions in the client package: ReadInteger, which validates integer input within a specified range and allows for a limited number of retries, and ReadPassword, which securely reads a password without echoing it back and optionally requires confirmation. Additionally, the authentication reset command is enhanced with a new User struct, functions to fetch and display user data, and an interactive mode that modifies flag requirements and standardizes error messaging.

Changes

File(s) Change Summary
client/helper.go Added ReadInteger for integer input validation with limited retries; added ReadPassword for secure password input with optional confirmation; added related imports (os/signal, strconv, syscall).
cmd/auth_reset.go Introduced a new User struct; implemented getUsers to retrieve user details and listUsers to display them; updated authResetCmd to support an --interactive flag, modifying flag requirements and error messaging using MsgLimitedAccess.

Sequence Diagram(s)

Interactive Auth Reset Flow

sequenceDiagram
    participant U as User
    participant C as AuthResetCmd
    participant S as AuthService
    participant RP as ReadPassword
    U->>C: Run auth reset (--interactive)
    C->>S: Call getUsers()
    S-->>C: Return user list
    C->>C: Invoke listUsers() to display users
    U->>C: Select username and initiate password prompt
    C->>RP: Invoke ReadPassword(repeat)
    alt Passwords match
       RP-->>C: Return password
       C-->>U: Proceed with auth reset
    else Passwords mismatch
       RP-->>C: Return error
       C-->>U: Report password mismatch error
    end
Loading

ReadInteger Flow

sequenceDiagram
    participant U as User
    participant RI as ReadInteger
    U->>RI: Enter integer input
    alt Valid integer within range
       RI-->>U: Return (true, value)
    else Invalid or out of range
       loop for allowed attempts
          RI->>U: Prompt for valid input
       end
       RI-->>U: Return (false, -1)
    end
Loading

ReadPassword Flow

sequenceDiagram
    participant U as User
    participant RP as ReadPassword
    U->>RP: Enter password (hidden)
    alt Confirmation not required
       RP-->>U: Return password
    else Confirmation required
       U->>RP: Re-enter password (hidden)
       alt Passwords match
         RP-->>U: Return password
       else
         RP-->>U: Return error (mismatch)
       end
    end
Loading

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)

Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2
Failed executing command with error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
cmd/auth_reset.go (1)

99-135: Interactive user and password input handling.

The implementation provides a good user experience by listing available users and prompting for password with confirmation. However, the error handling paths could be refactored to reduce duplication.

Consider extracting the error handling logic into a helper function:

+func handleInteractiveError(cmd *cobra.Command, message string, err error) {
+	cmd.PrintErrln(MsgLimitedAccess)
+	fmt.Println(err)
+	ExitWithError = true
+}

 		if interactive {
 			// prompt for user if not given
 			if options["username"] != nil && options["username"] != "" {
 				fmt.Printf("Changing password for user '%s'\n", options["username"])
 			} else {
 				users, err := getUsers()
 				if err != nil {
-					cmd.PrintErrln(MsgLimitedAccess)
-					fmt.Println(err)
-					ExitWithError = true
+					handleInteractiveError(cmd, MsgLimitedAccess, err)
 					return
 				}

Apply similar changes to the password error handling block.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b4329b7 and b3f68ce.

📒 Files selected for processing (2)
  • client/helper.go (2 hunks)
  • cmd/auth_reset.go (4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure t...

*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.

  • Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
  • In step-by-step instructions, front the location phrase in the instructional sentence.
  • In step-by-step instructions, front the 'goal' in the instructional sentence.
  • In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
  • do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
  • cmd/auth_reset.go
  • client/helper.go
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - If "" are used to mark UI strings, replace them by bold.

*/**(html|markdown|md): - Use bold to mark UI strings.

  • If "" are used to mark UI strings, replace them by bold.
  • cmd/auth_reset.go
  • client/helper.go
`*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

  • cmd/auth_reset.go
  • client/helper.go
`*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.

*/**(html|markdown|md): - Use sentence-style capitalization also in headings.

  • cmd/auth_reset.go
  • client/helper.go
`*/**(html|markdown|md)`: do not comment on HTML used for icons

*/**(html|markdown|md): do not comment on HTML used for icons

  • cmd/auth_reset.go
  • client/helper.go
`*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.

*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.

  • cmd/auth_reset.go
  • client/helper.go
🧬 Code Definitions (1)
cmd/auth_reset.go (2)
client/client.go (2)
  • GenericJSONGet (57-59)
  • GenericJSONPost (66-68)
client/helper.go (3)
  • Response (35-39)
  • ReadInteger (227-256)
  • ReadPassword (258-298)
🔇 Additional comments (8)
client/helper.go (1)

227-256: Solid implementation for integer input with validation.

The function properly handles user input validation, bounds checking, and retry logic. It provides a clean interface for prompting users with a range of valid values.

cmd/auth_reset.go (7)

11-17: Good structured representation of user data.

The User struct appropriately captures all relevant user properties required for the interactive authentication reset feature.


19-19: Well-placed constant for standardized error messaging.

Extracting this message as a constant improves maintainability and consistency across the codebase.


59-64: Clear user listing format.

The function presents user information in a readable format with appropriate indentation and labeling.


77-77: Helpful example for the new interactive mode.

The example clearly demonstrates how to use the new interactive flag.


140-140: Good use of standardized error message.

Using the constant MsgLimitedAccess ensures consistency across error messages.


151-152: Clear flag descriptions.

The descriptions clearly explain the purpose of each flag and their interaction with interactive mode.


153-154: Appropriate flag requirement handling.

Using MarkFlagsOneRequired properly enforces that either username/password or interactive mode must be specified.

Comment thread client/helper.go
Comment thread cmd/auth_reset.go
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
cmd/auth_reset.go (3)

21-56: Add nil checks to prevent potential panics when processing user data.

While the implementation is generally solid, the type assertions in the user data processing don't have nil checks, which could lead to panics if the API response structure changes.

Consider adding nil checks before type assertions:

 for _, user := range data.Data["users"].([]interface{}) {
     user := user.(map[string]interface{})
+    username, _ := user["username"].(string)
+    name, _ := user["name"].(string)
+    owner, _ := user["is_owner"].(bool)
+    active, _ := user["is_active"].(bool)
+    localOnly, _ := user["local_only"].(bool)
     result = append(result, User{
-        Username:  user["username"].(string),
-        Name:      user["name"].(string),
-        Owner:     user["is_owner"].(bool),
-        Active:    user["is_active"].(bool),
-        LocalOnly: user["local_only"].(bool),
+        Username:  username,
+        Name:      name,
+        Owner:     owner,
+        Active:    active,
+        LocalOnly: localOnly,
     })
 }

98-134: Consider environment validation for interactive mode.

The interactive mode may not work properly in all environments (as mentioned in the PR description). Consider validating that the current environment supports interactive features before proceeding.

 interactive, _ := cmd.Flags().GetBool("interactive")
 if interactive {
+    // Check if terminal supports interactive features
+    if !helper.IsInteractiveTerminal() {
+        cmd.PrintErrln("Interactive mode is only supported in terminal environments")
+        ExitWithError = true
+        return
+    }
     // prompt for user if not given
     if options["username"] != nil && options["username"] != "" {

Note: This assumes there's a IsInteractiveTerminal() helper function available or that you'll implement one.


114-114: Ensure error handling for ReadInteger.

The ReadInteger function could return an error, but the current implementation only checks the boolean return value. Consider handling potential errors explicitly.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b3f68ce and 56e0e59.

📒 Files selected for processing (2)
  • client/helper.go (2 hunks)
  • cmd/auth_reset.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • client/helper.go
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure t...

*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.

  • Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
  • In step-by-step instructions, front the location phrase in the instructional sentence.
  • In step-by-step instructions, front the 'goal' in the instructional sentence.
  • In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
  • do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
  • cmd/auth_reset.go
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - If "" are used to mark UI strings, replace them by bold.

*/**(html|markdown|md): - Use bold to mark UI strings.

  • If "" are used to mark UI strings, replace them by bold.
  • cmd/auth_reset.go
`*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

  • cmd/auth_reset.go
`*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.

*/**(html|markdown|md): - Use sentence-style capitalization also in headings.

  • cmd/auth_reset.go
`*/**(html|markdown|md)`: do not comment on HTML used for icons

*/**(html|markdown|md): do not comment on HTML used for icons

  • cmd/auth_reset.go
`*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.

*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.

  • cmd/auth_reset.go
🔇 Additional comments (6)
cmd/auth_reset.go (6)

11-17: LGTM: User struct definition is clear and appropriate.

The User struct correctly captures all needed attributes for user information display and selection.


19-19: Good practice: Standardizing error message.

Moving the error message to a constant variable improves maintainability and consistency across the codebase.


58-63: LGTM: Good user list formatting.

The function provides clear output formatting for user information, making it easier for users to select from the list.


136-137: Good code clarity: Added comment before password change.

Adding a comment clarifies the logical separation between gathering input and executing the password change.


139-139: Improved consistency: Using the standardized error message.

Using the MsgLimitedAccess constant improves maintainability and consistency.


150-153: Good flag configuration and clear descriptions.

The updated flag descriptions accurately reflect their purpose, and the MarkFlagsOneRequired settings correctly enforce that either username+password or interactive mode must be specified.

Comment thread client/helper.go Outdated
Comment thread client/helper.go
@home-assistant home-assistant Bot marked this pull request as draft April 9, 2025 07:44
@home-assistant
Copy link
Copy Markdown

home-assistant Bot commented Apr 9, 2025

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@sairon sairon marked this pull request as ready for review April 9, 2025 07:58
@home-assistant home-assistant Bot requested a review from agners April 9, 2025 07:58
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
client/helper.go (1)

259-275: Good terminal state management for secure password input.

The signal handling implementation correctly captures interrupts to restore terminal state, addressing a potential usability issue where hitting Ctrl+C could leave the terminal in a non-echoing state.

However, SIGKILL cannot be caught or handled by applications according to POSIX standards.

-	signal.Notify(signalChan, syscall.SIGINT, syscall.SIGTERM)
+	signal.Notify(signalChan, syscall.SIGINT, syscall.SIGTERM)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56e0e59 and 862c870.

📒 Files selected for processing (2)
  • client/helper.go (2 hunks)
  • cmd/auth_reset.go (4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure t...

*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.

  • Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
  • In step-by-step instructions, front the location phrase in the instructional sentence.
  • In step-by-step instructions, front the 'goal' in the instructional sentence.
  • In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
  • do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
  • client/helper.go
  • cmd/auth_reset.go
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - If "" are used to mark UI strings, replace them by bold.

*/**(html|markdown|md): - Use bold to mark UI strings.

  • If "" are used to mark UI strings, replace them by bold.
  • client/helper.go
  • cmd/auth_reset.go
`*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

  • client/helper.go
  • cmd/auth_reset.go
`*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.

*/**(html|markdown|md): - Use sentence-style capitalization also in headings.

  • client/helper.go
  • cmd/auth_reset.go
`*/**(html|markdown|md)`: do not comment on HTML used for icons

*/**(html|markdown|md): do not comment on HTML used for icons

  • client/helper.go
  • cmd/auth_reset.go
`*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.

*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.

  • client/helper.go
  • cmd/auth_reset.go
🧬 Code Graph Analysis (2)
client/helper.go (1)
spinner/spinner.go (1)
  • New (46-60)
cmd/auth_reset.go (2)
client/client.go (2)
  • GenericJSONGet (57-59)
  • GenericJSONPost (66-68)
client/helper.go (3)
  • Response (36-40)
  • ReadInteger (228-257)
  • ReadPassword (259-299)
🔇 Additional comments (10)
client/helper.go (3)

228-257: Well-implemented integer input function with proper validation and error handling.

The function follows Go best practices with a clear (int, error) return pattern and handles all edge cases appropriately including invalid inputs, out-of-range values, and retry limits.


278-283: Clean password reading implementation.

Using the term package for password input ensures the password isn't visible when typed.


285-296: Password confirmation logic addresses security concerns.

The confirmation check is a good security practice, ensuring users don't make typos when setting passwords.

cmd/auth_reset.go (7)

11-17: Well-structured User struct with appropriate fields.

The struct cleanly encapsulates all necessary user attributes for the authentication system.


19-19: Good practice using a constant for error messaging.

This ensures consistency in error reporting throughout the codebase.


58-63: Clean implementation of user list display.

The formatting is clear and provides all relevant user information.


115-125: Correctly handles user selection with 1-based indexing for display.

The code properly converts between the 1-based indexing shown to users and the 0-based indexing used in the slice.


127-135: Good integration with the secure password reading function.

The interactive password entry makes good use of the helper.ReadPassword function with confirmation enabled.


137-146: Consistent error handling using the standard message.

The standardized error message improves consistency throughout the application.


151-154: Clear flag descriptions and appropriate flag requirements.

Using MarkFlagsOneRequired is a good approach to allow either username+password or interactive mode.

Comment thread cmd/auth_reset.go
Comment on lines +98 to +110
interactive, _ := cmd.Flags().GetBool("interactive")
if interactive {
// prompt for user if not given
if options["username"] != nil && options["username"] != "" {
fmt.Printf("Changing password for user '%s'\n", options["username"])
} else {
users, err := getUsers()
if err != nil {
cmd.PrintErrln(MsgLimitedAccess)
fmt.Println(err)
ExitWithError = true
return
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Check for empty users list before attempting to display.

If the API returns an empty list of users, the current implementation would continue and potentially cause issues when calling ReadInteger with an invalid range.

 				users, err := getUsers()
 				if err != nil {
 					cmd.PrintErrln(MsgLimitedAccess)
 					fmt.Println(err)
 					ExitWithError = true
 					return
 				}
+
+				if len(users) == 0 {
+					fmt.Println("No users found.")
+					ExitWithError = true
+					return
+				}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
interactive, _ := cmd.Flags().GetBool("interactive")
if interactive {
// prompt for user if not given
if options["username"] != nil && options["username"] != "" {
fmt.Printf("Changing password for user '%s'\n", options["username"])
} else {
users, err := getUsers()
if err != nil {
cmd.PrintErrln(MsgLimitedAccess)
fmt.Println(err)
ExitWithError = true
return
}
interactive, _ := cmd.Flags().GetBool("interactive")
if interactive {
// prompt for user if not given
if options["username"] != nil && options["username"] != "" {
fmt.Printf("Changing password for user '%s'\n", options["username"])
} else {
users, err := getUsers()
if err != nil {
cmd.PrintErrln(MsgLimitedAccess)
fmt.Println(err)
ExitWithError = true
return
}
if len(users) == 0 {
fmt.Println("No users found.")
ExitWithError = true
return
}
// Continue with logic for selecting a user
}
}

Comment thread cmd/auth_reset.go
Comment on lines +21 to +56
func getUsers() ([]User, error) {
resp, err := helper.GenericJSONGet("auth", "list")

if err != nil {
return nil, err
}

var data *helper.Response
var result []User

if resp.IsSuccess() {
data = resp.Result().(*helper.Response)

if data.Result != "ok" {
err := fmt.Errorf("error returned from Supervisor: %s", data.Message)
return nil, err
}

for _, user := range data.Data["users"].([]interface{}) {
user := user.(map[string]interface{})
result = append(result, User{
Username: user["username"].(string),
Name: user["name"].(string),
Owner: user["is_owner"].(bool),
Active: user["is_active"].(bool),
LocalOnly: user["local_only"].(bool),
})
}
} else {
data = resp.Error().(*helper.Response)
err := fmt.Errorf("error returned from Supervisor: %s", data.Message)
return nil, err
}

return result, nil
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for type assertions in getUsers function.

The function correctly processes API responses, but type assertions like user["username"].(string) could panic if fields are missing or have unexpected types.

 			user := user.(map[string]interface{})
+			username, ok := user["username"].(string)
+			if !ok {
+				return nil, fmt.Errorf("invalid user data: username field missing or not a string")
+			}
+
+			name, ok := user["name"].(string)
+			if !ok {
+				return nil, fmt.Errorf("invalid user data: name field missing or not a string")
+			}
+
+			owner, ok := user["is_owner"].(bool)
+			if !ok {
+				return nil, fmt.Errorf("invalid user data: is_owner field missing or not a boolean")
+			}
+
+			active, ok := user["is_active"].(bool)
+			if !ok {
+				return nil, fmt.Errorf("invalid user data: is_active field missing or not a boolean")
+			}
+
+			localOnly, ok := user["local_only"].(bool)
+			if !ok {
+				return nil, fmt.Errorf("invalid user data: local_only field missing or not a boolean")
+			}
+
 			result = append(result, User{
-				Username:  user["username"].(string),
-				Name:      user["name"].(string),
-				Owner:     user["is_owner"].(bool),
-				Active:    user["is_active"].(bool),
-				LocalOnly: user["local_only"].(bool),
+				Username:  username,
+				Name:      name,
+				Owner:     owner,
+				Active:    active,
+				LocalOnly: localOnly,
 			})
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func getUsers() ([]User, error) {
resp, err := helper.GenericJSONGet("auth", "list")
if err != nil {
return nil, err
}
var data *helper.Response
var result []User
if resp.IsSuccess() {
data = resp.Result().(*helper.Response)
if data.Result != "ok" {
err := fmt.Errorf("error returned from Supervisor: %s", data.Message)
return nil, err
}
for _, user := range data.Data["users"].([]interface{}) {
user := user.(map[string]interface{})
result = append(result, User{
Username: user["username"].(string),
Name: user["name"].(string),
Owner: user["is_owner"].(bool),
Active: user["is_active"].(bool),
LocalOnly: user["local_only"].(bool),
})
}
} else {
data = resp.Error().(*helper.Response)
err := fmt.Errorf("error returned from Supervisor: %s", data.Message)
return nil, err
}
return result, nil
}
func getUsers() ([]User, error) {
resp, err := helper.GenericJSONGet("auth", "list")
if err != nil {
return nil, err
}
var data *helper.Response
var result []User
if resp.IsSuccess() {
data = resp.Result().(*helper.Response)
if data.Result != "ok" {
err := fmt.Errorf("error returned from Supervisor: %s", data.Message)
return nil, err
}
for _, user := range data.Data["users"].([]interface{}) {
user := user.(map[string]interface{})
username, ok := user["username"].(string)
if !ok {
return nil, fmt.Errorf("invalid user data: username field missing or not a string")
}
name, ok := user["name"].(string)
if !ok {
return nil, fmt.Errorf("invalid user data: name field missing or not a string")
}
owner, ok := user["is_owner"].(bool)
if !ok {
return nil, fmt.Errorf("invalid user data: is_owner field missing or not a boolean")
}
active, ok := user["is_active"].(bool)
if !ok {
return nil, fmt.Errorf("invalid user data: is_active field missing or not a boolean")
}
localOnly, ok := user["local_only"].(bool)
if !ok {
return nil, fmt.Errorf("invalid user data: local_only field missing or not a boolean")
}
result = append(result, User{
Username: username,
Name: name,
Owner: owner,
Active: active,
LocalOnly: localOnly,
})
}
} else {
data = resp.Error().(*helper.Response)
err := fmt.Errorf("error returned from Supervisor: %s", data.Message)
return nil, err
}
return result, nil
}

Copy link
Copy Markdown
Member

@agners agners left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Comment thread client/helper.go

res, err := reader.ReadString('\n')
if err != nil {
log.Fatalf("error: %v", err)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess this could be:

Suggested change
log.Fatalf("error: %v", err)
return 0, fmt.Errorf("reading input failed: %w", err)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure what error this handles, as I followed the pattern from the other function (the docs say "often io.EOF") but with your suggestion it will bail out on the error immediately, while this still logs the error and allows for retry.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let's keep it aligned with AskForConfirmation here. We can change it in another PR, as it will also affect other commands.

@sairon sairon merged commit e7b778a into master Apr 9, 2025
9 checks passed
@sairon sairon deleted the interactive-auth-reset branch April 9, 2025 10:24
sairon added a commit that referenced this pull request Apr 9, 2025
As discussed in review of #558, handling of errors (e.g. EOF) when
reading the input from users could be better. All commands that read
input from users now exit with non-zero status code when the operation
is aborted and print the message to error out instead of logging it.
sairon added a commit that referenced this pull request Apr 9, 2025
As discussed in review of #558, handling of errors (e.g. EOF) when
reading the input from users could be better. All commands that read
input from users now exit with non-zero status code when the operation
is aborted and print the message to error out instead of logging it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants