Implement login command with improved config handling #7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces a new CLI structure under cmd/cli with commands (connect, login) and a main entry point. Adds config file handling and server health checks for connect, plus token-based login flow. Updates Taskfile to build CLI and API separately, adjusts go.mod dependencies, tweaks API comment, and trims lint config whitespace. Changes
Sequence Diagram(s)sequenceDiagram
actor U as User
participant C as CLI (connect)
participant S as Server
participant F as Config (~/.tunnelr/config.json)
U->>C: tunnelr-cli connect --user [--server]
C->>F: loadConfig() (if --server missing)
C->>S: GET http://<server>/healthz (5s timeout)
S-->>C: 200 OK (healthy) / error
C->>F: saveServerConfig(server)
C-->>U: "Connecting to <server> as <user>..."
sequenceDiagram
actor U as User
participant C as CLI (login)
participant S as Server
participant F as Config (~/.tunnelr/config.json)
U->>C: tunnelr-cli login --user --password [--server]
C->>F: loadConfig() (if --server missing)
C->>S: POST http://<server>/api/login {user, password}
S-->>C: 200 {token} / 401 / 500 / other
C->>F: save token (+server if provided)
C-->>U: "Logged in" or error message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (9)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
cmd/cli/connect.go (4)
14-18: Consider potential config collision with existing Config type.There's already a
Configstruct defined inconfigs/configs.gowith a different structure (containing aPortfield). This could lead to naming conflicts or confusion.Consider renaming this to
CliConfigorClientConfigto avoid potential conflicts:-// Config represents the application configuration -type Config struct { +// ClientConfig represents the client configuration +type ClientConfig struct { Server string `json:"server"` Token string `json:"token,omitempty"` }
72-91: Consider adding HTTPS support and improving error handling.The health check currently uses HTTP which may not match the HTTPS used in the login command. Also, consider exposing more specific error information for better debugging.
- url := fmt.Sprintf("http://%s/healthz", server) + url := fmt.Sprintf("https://%s/healthz", server) resp, err := client.Get(url) if err != nil { + fmt.Printf("Health check failed: %v\n", err) return false }
104-104: Use os.UserHomeDir() for consistency.Line 125 in
loadConfig()usesos.UserHomeDir()while this function usesos.Getenv("HOME"). Usingos.UserHomeDir()is more cross-platform compatible.- configDir := filepath.Join(os.Getenv("HOME"), ".tunnelr") + homeDir, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("failed to get user home directory: %w", err) + } + configDir := filepath.Join(homeDir, ".tunnelr")
155-158: Clean up commented code or add a proper explanation.The commented code should either be removed or replaced with a clear comment explaining why the server flag is no longer required.
- // Remove the required flag for server since we can load it from config - // if err := connectCmd.MarkFlagRequired("server"); err != nil { - // fmt.Println(err) - // } + // Server flag is optional since it can be loaded from configcmd/cli/login.go (2)
108-110: Logic error in server preservation.The condition
server != "" && config.Server == ""will always use the server from the flag when config.Server is empty, but it won't update config.Server when a server is provided via flag and config.Server already exists. This could lead to inconsistent behavior.Consider this logic instead:
- if server != "" && config.Server == "" { - config.Server = server // Save the server if it wasn't already saved - } + // Update server in config if provided via flag, otherwise preserve existing + if server != "" { + config.Server = server + }
139-142: Remove production warning and dead code.The comment about not printing passwords in production and the placeholder authentication logic should be removed since actual authentication is already implemented above.
- // In production, don't print the password (security risk) - fmt.Printf("Logging in as %s...\n", user) - // Here you would add your actual authentication logicThe login message could be moved to before the actual login attempt for better UX:
+ fmt.Printf("Logging in as %s...\n", user) + // Make POST request url := fmt.Sprintf("https://%s/api/login", server)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cmd/cli/connect.go(2 hunks)cmd/cli/login.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/cli/login.go (2)
cmd/cli/connect.go (1)
Config(15-18)cmd/cli/root.go (1)
RootCmd(18-35)
cmd/cli/connect.go (2)
configs/configs.go (1)
Config(11-13)cmd/cli/root.go (1)
RootCmd(18-35)
🔇 Additional comments (3)
cmd/cli/connect.go (2)
36-52: LGTM! Well-structured config fallback logic.The implementation correctly handles the server flag fallback to config, with appropriate error messages guiding users to provide the server flag when config is unavailable.
54-66: LGTM! Good connectivity validation and config persistence.The health check before connection and saving server configuration for future use is a good UX improvement. The warning for config save failures is appropriate since it's not critical to the connection operation.
cmd/cli/login.go (1)
15-22: LGTM! Well-defined request/response structures.The JSON structures are properly defined with appropriate tags for the login API.
Comment on lines
41
to
57
| // If server flag is not provided, try to load from config | ||
| if server == "" { | ||
| config, err := loadConfig() | ||
| if err != nil { | ||
| fmt.Printf("Error loading config: %v\n", err) | ||
| fmt.Println("Please provide a server address using the --server flag") | ||
| return | ||
| } | ||
|
|
||
| if config.Server == "" { | ||
| fmt.Println("No server found in config. Please provide a server address using the --server flag") | ||
| return | ||
| } | ||
|
|
||
| server = config.Server | ||
| fmt.Printf("Using server from config: %s\n", server) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Code duplication with connect.go.
This exact same server loading logic exists in connect.go lines 36-52. Consider extracting this into a shared utility function.
Create a shared function in a utils package or add it to this file:
+// getServerFromFlagOrConfig returns the server from flag or config
+func getServerFromFlagOrConfig(serverFlag string) (string, error) {
+ if serverFlag != "" {
+ return serverFlag, nil
+ }
+
+ config, err := loadConfig()
+ if err != nil {
+ return "", fmt.Errorf("error loading config: %w. Please provide a server address using the --server flag", err)
+ }
+
+ if config.Server == "" {
+ return "", fmt.Errorf("no server found in config. Please provide a server address using the --server flag")
+ }
+
+ fmt.Printf("Using server from config: %s\n", config.Server)
+ return config.Server, nil
+}Then replace the duplicated logic in both files:
- // If server flag is not provided, try to load from config
- if server == "" {
- config, err := loadConfig()
- if err != nil {
- fmt.Printf("Error loading config: %v\n", err)
- fmt.Println("Please provide a server address using the --server flag")
- return
- }
-
- if config.Server == "" {
- fmt.Println("No server found in config. Please provide a server address using the --server flag")
- return
- }
-
- server = config.Server
- fmt.Printf("Using server from config: %s\n", server)
- }
+ var err error
+ server, err = getServerFromFlagOrConfig(server)
+ if err != nil {
+ fmt.Println(err)
+ return
+ }🤖 Prompt for AI Agents
In cmd/cli/login.go around lines 41 to 57, the server-loading block is
duplicated with connect.go (lines 36-52); extract this logic into a shared
function (e.g., in an internal/utils or cmd/cli package) that accepts the server
flag and returns the resolved server string or an error; replace the duplicated
block in both login.go and connect.go with a call to that function, have callers
print user-facing messages based on the returned error or server value, and
ensure the new function calls loadConfig() internally and preserves the same
error messages/behavior.
Comment on lines
70
to
76
| // Make POST request | ||
| url := fmt.Sprintf("https://%s/api/login", server) | ||
| resp, err := http.Post(url, "application/json", bytes.NewBuffer(bodyBytes)) | ||
| if err != nil { | ||
| fmt.Println("Error sending request:", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Protocol inconsistency with connect.go health check.
The login uses HTTPS while the health check in connect.go uses HTTP. This inconsistency could cause issues if the server only supports one protocol.
Consider making the protocol configurable or ensuring both commands use the same protocol. You could add a helper function:
+// buildServerURL constructs the appropriate URL for the given endpoint
+func buildServerURL(server, endpoint string, useHTTPS bool) string {
+ protocol := "http"
+ if useHTTPS {
+ protocol = "https"
+ }
+ return fmt.Sprintf("%s://%s%s", protocol, server, endpoint)
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/cli/login.go around lines 70 to 76, the login POST hardcodes "https"
while connect.go's health check uses "http", causing protocol inconsistency;
change login to use a configurable scheme instead of hardcoding by
adding/reading a shared scheme value (flag, config or helper) and build the URL
with that scheme, or add a helper function like buildServerURL(server, scheme,
path) used by both connect.go and login.go so both commands use the same
protocol; ensure a sensible default (e.g., https) and update both callers to
pass or use the shared scheme.
| } | ||
|
|
||
| // Create config directory | ||
| homeDir, _ := os.UserHomeDir() |
There was a problem hiding this comment.
Handle potential error from os.UserHomeDir().
The error from os.UserHomeDir() is ignored, which could cause a panic if the home directory cannot be determined.
- homeDir, _ := os.UserHomeDir()
+ homeDir, err := os.UserHomeDir()
+ if err != nil {
+ fmt.Println("Error getting user home directory:", err)
+ os.Exit(1)
+ }📝 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
| homeDir, _ := os.UserHomeDir() | |
| homeDir, err := os.UserHomeDir() | |
| if err != nil { | |
| fmt.Println("Error getting user home directory:", err) | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
In cmd/cli/login.go around line 93, os.UserHomeDir() is called with its error
ignored which can lead to a panic if the home directory cannot be determined;
update the code to capture both return values (homeDir, err :=
os.UserHomeDir()), check if err != nil, and handle it appropriately (return the
error to the caller, print a user-friendly message and exit with non-zero
status, or fall back to an alternative like os.Getenv("HOME") if that is
acceptable). Ensure subsequent code uses the validated homeDir only after the
error check and add a clear error message explaining that the home directory
could not be determined.
|
|
||
| // Save updated config | ||
| configPath := filepath.Join(configDir, "config.json") | ||
| configData, _ := json.MarshalIndent(config, "", " ") |
There was a problem hiding this comment.
Handle JSON marshaling error.
The error from json.MarshalIndent is ignored, which could lead to writing invalid JSON to the config file.
- configData, _ := json.MarshalIndent(config, "", " ")
+ configData, err := json.MarshalIndent(config, "", " ")
+ if err != nil {
+ fmt.Println("Error encoding config:", err)
+ os.Exit(1)
+ }📝 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
| configData, _ := json.MarshalIndent(config, "", " ") | |
| // Encode the config as pretty-printed JSON, but handle any errors. | |
| configData, err := json.MarshalIndent(config, "", " ") | |
| if err != nil { | |
| fmt.Println("Error encoding config:", err) | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
In cmd/cli/login.go around line 114, json.MarshalIndent's error is ignored;
capture its returned error, check it, and handle failures before proceeding to
write the config file. On marshal error, return the error (or log and exit) so
you don't write invalid data; only proceed to write the file when marshal
succeeds, and propagate or wrap the marshal error for upstream handling.
…ialization, and update Taskfile for building CLI and API applications
MennatAllahhxx
approved these changes
Aug 15, 2025
This was referenced Aug 23, 2025
Closed
ahmd7
added a commit
that referenced
this pull request
Sep 25, 2025
* Feature/cli skeleton (#5) * Add initial configuration files and scripts for project setup (#1) - Create CI pipeline configuration for automated builds and tests - Add auto review settings for code consistency checks - Include .gitignore to exclude sensitive files - Set up golangci-lint configuration for code quality checks - Implement Taskfile for managing development tasks - Add Go module file for dependency management - Create scripts for encrypting and decrypting environment files * Refactor CLI structure and enhance task definitions in Taskfile.yml; add connect command with user and machine flags; update package comments for clarity. * removed unnecessary file * fix connection to a server not a machine * tidy mod file * fix(build): ensure dependencies are declared and create bin directory before build --------- Co-authored-by: Ahmed Samir <134155493+ahmd7@users.noreply.github.com> * Implement backend server with health and version endpoints, add configuration management, and update Taskfile for CLI operations (#6) * Implement login command with improved config handling (#7) * Implement login command with server configuration loading and token management * Fix config directory path from ~/.tempssh to ~/.tunnelr in connect and login commands * Improve error handling for response body closure in connect and login commands * Implement CLI commands for login and connect, add backend server initialization, and update Taskfile for building CLI and API applications --------- Co-authored-by: Menna Hassan <h.menna85@gmail.com> * feat: add initial database setup and user authentication (#8) * feat: add initial database setup and user authentication - Introduced `atlas.hcl` for environment configuration and database connection strings. - Updated `main.go` to initialize database connection and set up user repository and authentication service. - Enhanced `configs.go` to manage database and JWT configurations with sensible defaults. - Implemented user model and repository for user management. - Created authentication service for user registration and login functionalities. - Added HTTP handlers for user operations including registration and login. - Implemented middleware for JWT authentication and role-based access control. - Established database migrations for user table schema. - Removed deprecated router implementation and organized routes with Chi router. * resolve coderabbit comments * fix linting * Implement CLI and backend server with user authentication (#9) * feat: add GetUserInfo endpoint and whoami command for user information retrieval * feat: enhance GetUserInfo to handle JWT claims normalization * Implement CLI and backend server with user authentication (#10) * feat: add GetUserInfo endpoint and whoami command for user information retrieval * feat: enhance GetUserInfo to handle JWT claims normalization * feat: implement team management functionality with repository, service, and handler layers * feat: enhance team service with JWT role extraction and user admin checks * feat: implement team deletion functionality and enforce unique team names * feat: update API versioning and enhance team-user relationships in models * feat: remove unused JWT utility function for role extraction * feat: enhance team management by adding GetTeamUsers method and updating team routes * refactor: rename ExtractUserRole to extractUserRole for consistency and clarity --------- Co-authored-by: Menna Hassan <h.menna85@gmail.com> * feat: enhance login command and add team management functionality (#12) * feat: enhance login command and add team management functionality * fix: linting * refactor: coderabbit comments * Add user information retrieval and team management features (#13) * feat: add GetUserInfo endpoint and whoami command for user information retrieval * feat: enhance GetUserInfo to handle JWT claims normalization * feat: implement team management functionality with repository, service, and handler layers * feat: enhance team service with JWT role extraction and user admin checks * feat: implement team deletion functionality and enforce unique team names * feat: update API versioning and enhance team-user relationships in models * feat: remove unused JWT utility function for role extraction * feat: enhance team management by adding GetTeamUsers method and updating team routes * refactor: rename ExtractUserRole to extractUserRole for consistency and clarity * feat: implement AddUserToTeam functionality with corresponding HTTP handler and router endpoint * feat: add RemoveUserFromTeam functionality with corresponding HTTP handler and router endpoint * refactor: clean up imports and remove unnecessary whitespace in whoami command --------- Co-authored-by: Menna Hassan <h.menna85@gmail.com> * feat: Add machine management functionality (#14) - Implemented GetMachinesByTeamID method in TeamService to retrieve machines associated with a team. - Created Machine model to represent machine data in the application. - Added MachineRepository interface for machine data operations. - Developed repository methods for creating, updating, retrieving, and deleting machines. - Introduced MachineHandler for handling HTTP requests related to machine operations. - Added routes for machine management in the router. - Implemented user and team access checks for machine operations. - Created SQL migration scripts for machines and machine_teams tables. - Added utility functions for extracting user ID from JWT claims. * Implement CLI and backend server with user and machine management (#15) * feat: implement machines management commands for CLI * feat: enhance machines command with team management and detailed operations * feat: implement machines management commands for CLI * feat: enhance machines command with team management and detailed operations * fix: update machine ID reference and clean up team addition command flags * refactor: streamline machine removal and response handling; improve error logging * feat: enhance machine removal command with URL encoding and add alias for delete * Implement CLI and backend server with user and machine management (#16) * feat: implement machines management commands for CLI * feat: enhance machines command with team management and detailed operations * feat: implement machines management commands for CLI * feat: enhance machines command with team management and detailed operations * fix: update machine ID reference and clean up team addition command flags * refactor: streamline machine removal and response handling; improve error logging * feat: enhance machine removal command with URL encoding and add alias for delete * fix: update key field type to text for better handling of larger key data * Refactor/cli teams commands (#18) * Refactor CLI commands to utilize centralized config and request handling - Moved configuration loading to utils package and updated CLI commands (connect, login, machines, team, whoami) to use utils.LoadConfig(). - Implemented MakeAuthenticatedRequest in utils to handle authenticated HTTP requests, replacing direct request handling in CLI commands. - Removed redundant loadConfig function from CLI commands. - Added team management commands (add, remove, get) with appropriate request handling and response parsing. - Updated router to include team-related endpoints. * feat: add shared types for Team, User, Machine, and responses - Introduced shared types for Team, User, and Machine in `cmd/cli/shared/types.go`. - Added response types for listing team members and machines in `cmd/cli/shared/types.go`. feat: implement table printing utilities for machines, users, and teams - Created utility functions in `cmd/cli/utils/tables.go` to print formatted tables for machines, users, and teams. refactor: update team service to return machines with team details - Modified `GetMachinesByTeamID` in `internal/api/app/service/team_service.go` to return a team with its machines. - Updated `GetTeamByID` to remove user ID check, simplifying the function. fix: adjust repository and model definitions for teams and machines - Updated `internal/api/core/model/team.go` to include a slice of machines in the Team model. - Changed return types in `TeamRepository` and its implementation to return slices of machines instead of pointers. chore: clean up unused migration files - Deleted initial schema migration files that are no longer needed in `internal/cli/infrastructure/migrations`. * refactor: update utils references to cliutils across multiple files * fix coderabbit comments * Implement access logging and ephemeral user management for shell access (#17) * feat: Implement access logging and ephemeral user management for shell access - Added AccessLogService and AccessLogRepository for managing access logs. - Created AccessLog model to represent SSH access records. - Enhanced MachineRepository to include password and key fields in GetMachineByID. - Introduced ShellHandler for WebSocket connections, enabling shell access with ephemeral accounts. - Implemented ephemeral account creation, management, and cleanup on target machines. - Added WebSocket endpoint for shell access, including session management and error handling. - Updated router to include new WebSocket endpoint and access log service. - Enhanced authentication middleware to log incoming requests. - Added utility functions to extract username from JWT claims. * fix: add access logs to atlas * added migration files * feat: Enhance access log service and WebSocket handler with context management and error handling * feat: Improve error handling and logging in WebSocket and SSH session management - Enhanced error handling for writing messages and closing connections in WebSocket handlers. - Refactored WebSocket session management to improve readability and maintainability by extracting setup and lifecycle management into helper functions. - Added context management for SSH session setup and ephemeral user creation, ensuring proper cleanup and error logging. - Updated access log status handling to ensure accurate session state tracking. * feat: Add WebSocket SSH access command and improve session handling (#19) * fix: Correct LoadConfig function reference in access command * coderabbit comments --------- Co-authored-by: Menna Hassan <h.menna85@gmail.com> * remove test ssh endpoint --------- Co-authored-by: Ahmed Samir <134155493+ahmd7@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a login command that authenticates users with the backend server and manages server configuration loading and token storage. Update the config directory path and enhance error handling for better user experience.
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation