feat(auth): implement user management and authentication enhancements #9
Conversation
WalkthroughAdds a persisted, thread-safe UserStore and integrates it into the Authenticator and HTTP server for repo permission checks; adds a Cobra-based CLI for user management; updates docker-compose to persist users; updates go.mod and tightens README spacing. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as workgit CLI
participant Auth as Authenticator
participant US as UserStore
participant FS as users.json
Note over CLI,Auth: user management commands
CLI->>Auth: user command (create/list/delete/...)
Auth->>US: CreateUser / ListUsers / SetPermission / RegenerateToken
US->>FS: load / atomic save
US-->>Auth: UserInfo / result
Auth-->>CLI: formatted output (JSON or human)
sequenceDiagram
participant GitClient
participant Server
participant Auth as Authenticator
participant US as UserStore
GitClient->>Server: Git request (mTLS / Bearer / Basic)
Server->>Auth: Authenticate(ctx, creds)
alt mTLS
Auth->>Auth: extract CN → Username
else Bearer
Auth->>US: GetUserByToken(token)
else Basic
Auth->>US: AuthenticateUser(username, password or token)
end
US-->>Auth: UserInfo (if found)
Auth->>Server: attach Username to ctx
Server->>Auth: RequirePermission(ctx, repo, service)
Auth->>US: CheckAccess(username, repo, required)
US-->>Auth: allowed / denied
alt allowed
Server-->>GitClient: proceed
else denied
Server-->>GitClient: 401/403
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
techplayz32
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/server.go (1)
78-93: Auth middleware is not applied to Git endpoints; the handler expects authenticated context but none is provided.The
GitHandler.ServeHTTP()method checksauth.UserFromContext(r.Context())at line 52 of handler.go and returns 401 if the user is missing or anonymous. However,wrapHandler()in server.go applies only logging, metrics, rate-limiting, and request-ID middleware—no authentication middleware.The authenticator is instantiated in
NewGitHandler()but never used to wrap or protect the handlers. The context thatServeHTTP()depends on will never be populated, causing all authenticated git operations to fail with 401.Apply
auth.Middleware()in the middleware chain (e.g., add it towrapHandler()before other middleware) to populate the user context beforeServeHTTP()is invoked.
🧹 Nitpick comments (10)
internal/auth/auth.go (3)
88-90: Auth challenge header could include Bearer.If you support tokens, advertise both:
WWW-Authenticate: Basic realm="workgit", Bearer.
305-318: mTLS policy: RequestClientCert may be too lax.If you intend to enforce client certs when a CA is configured, consider
tls.RequireAndVerifyClientCert. Current setting only requests the cert.
249-256: CheckPermission returns false when store is nil, but RequirePermission skips checks.This is fine, but document it to avoid confusion for callers using
CheckPermissiondirectly.internal/auth/users.go (3)
293-304: Avoid returning secrets in ListUsers.
ListUsersexposesPasswordHashandToken. Provide a sanitized variant for API/CLI output and use that downstream.Example:
+type PublicUser struct { + Username string `json:"username"` + Email string `json:"email,omitempty"` + FullName string `json:"full_name,omitempty"` + Active bool `json:"active"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + Permissions map[string]Permission `json:"permissions"` + GlobalAdmin bool `json:"global_admin"` +} + +func (s *UserStore) ListUsersPublic() []PublicUser { + s.mu.RLock() + defer s.mu.RUnlock() + out := make([]PublicUser, 0, len(s.users)) + for _, u := range s.users { + out = append(out, PublicUser{ + Username: u.Username, Email: u.Email, FullName: u.FullName, + Active: u.Active, CreatedAt: u.CreatedAt, UpdatedAt: u.UpdatedAt, + Permissions: u.Permissions, GlobalAdmin: u.GlobalAdmin, + }) + } + return out +}
168-193: Consider token collision check on regenerate.Very unlikely, but add a loop to avoid collisions with existing tokens in
s.tokens.
333-357: Durability: optional fsync.For stronger crash consistency, consider
os.CreateTemp + file.Sync + Close + Rename. Current approach is usually fine.cmd/workgit/user.go (4)
146-153: Stop hand‑rolling JSON; use encoding/json to avoid broken/unsafe output.Manual fmt.Printf JSON will break on quotes and special chars; also inconsistent escaping.
Apply structured encoding. Example for Lines 146‑153:
- if jsonOutput { - fmt.Printf(`{"status":"created","username":"%s","token":"%s"}%s`, - user.Username, user.Token, "\n") - } else { + if jsonOutput { + if err := printJSON(map[string]any{ + "status": "created", + "username": user.Username, + "token": user.Token, + }); err != nil { + return err + } + } else { fmt.Printf("User created: %s\n", user.Username) fmt.Printf("API Token: %s\n", user.Token) fmt.Println("\nKeep this token secure! It won't be shown again.") }Repeat similarly for Lines 171‑181, 231‑235, 264‑269, 292‑297, 382‑386, and 409‑414.
Outside selected ranges, add helper and import:
import ( + "encoding/json" "fmt" "os" "strings" - "syscall" "text/tabwriter" @@ ) + +func printJSON(v any) error { + enc := json.NewEncoder(os.Stdout) + enc.SetEscapeHTML(true) + return enc.Encode(v) +}Also applies to: 171-181, 231-235, 264-269, 292-297, 382-386, 409-414
419-427: Prefer os.Stdin.Fd() over syscall.Stdin; consider wiping password bytes.For better portability use os.Stdin.Fd(). Also, avoid keeping passwords as Go strings and zero the byte slice after use.
Apply:
-func promptPassword(prompt string) (string, error) { +func promptPassword(prompt string) (string, error) { fmt.Print(prompt) - password, err := term.ReadPassword(int(syscall.Stdin)) + pwBytes, err := term.ReadPassword(int(os.Stdin.Fd())) fmt.Println() if err != nil { return "", err } - return string(password), nil + // Convert then wipe + pw := string(pwBytes) + for i := range pwBytes { + pwBytes[i] = 0 + } + return pw, nil }Also remove the unused syscall import in the file’s imports.
212-220: Safer confirmation input parsing.fmt.Scanln leaves trailing input and ignores lowercase “yes”. Trim and normalize.
Apply:
- fmt.Printf("Delete user '%s'? [y/N]: ", username) - var confirm string - fmt.Scanln(&confirm) - if confirm != "y" && confirm != "Y" { + fmt.Printf("Delete user '%s'? [y/N]: ", username) + var confirm string + if _, err := fmt.Scanln(&confirm); err != nil { + fmt.Println("Cancelled") + return nil + } + if strings.ToLower(strings.TrimSpace(confirm)) != "y" { fmt.Println("Cancelled") return nil }
130-134: Centralize user store via Authenticator instead of constructing stores ad‑hoc.Instantiate auth once per command and use GetUserStore to keep behavior consistent with server and future auth policies.
-userStore, err := auth.NewUserStore(cfg.Storage.Root) +authn, err := auth.NewAuthenticator(cfg.Auth, cfg.Storage.Root) if err != nil { - return fmt.Errorf("initialize user store: %w", err) + return fmt.Errorf("auth init: %w", err) } +userStore := authn.GetUserStore()Apply similarly at: 164-168, 222-226, 255-258, 282-286, 329-333, 373-376, 399-402
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
README.md(0 hunks)cmd/workgit/user.go(1 hunks)docker-compose.yml(1 hunks)go.mod(2 hunks)internal/auth/auth.go(7 hunks)internal/auth/users.go(1 hunks)internal/server/handler.go(1 hunks)internal/server/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (5)
internal/server/handler.go (1)
internal/auth/auth.go (1)
NewAuthenticator(45-79)
internal/server/server.go (1)
internal/auth/auth.go (1)
NewAuthenticator(45-79)
internal/auth/auth.go (2)
internal/config/config.go (1)
AuthConfig(53-57)internal/auth/users.go (5)
UserStore(51-56)NewUserStore(59-71)Permission(27-27)PermissionRead(31-31)PermissionWrite(32-32)
cmd/workgit/user.go (1)
internal/auth/users.go (2)
NewUserStore(59-71)Permission(27-27)
internal/auth/users.go (1)
internal/auth/auth.go (1)
ErrUnauthorized(21-21)
🔇 Additional comments (4)
go.mod (3)
8-8: No issues found - versions are valid and build successfully.The script execution confirms that
golang.org/x/term v0.36.0andgolang.org/x/sys v0.37.0are legitimate versions that exist in the Go module ecosystem. Bothgo mod tidyandgo list -m -jsoncompleted successfully without errors, demonstrating that the versions resolve correctly and the project builds without dependency issues.
3-3: ****The
go 1.25.1directive is valid. The go directive accepts valid Go version strings including patch versions such asgo 1.20.3andgo 1.23.0. No changes are needed; the current syntax is correct per Go module specifications.Likely an incorrect or invalid review comment.
23-23: The review comment is incorrect.go.yaml.in/yaml/v2 is a valid Go module import path and a maintained upstream replacement for gopkg.in/yaml.v2. The script output confirms successful module resolution with valid checksums in go.sum. No changes are needed.
Likely an incorrect or invalid review comment.
cmd/workgit/user.go (1)
250-254: No issues found. The "none" permission is correctly handled.The implementation already treats "none" as a delete operation: when
perm == PermissionNone, the code executesdelete(user.Permissions, repo)(line 210), removing the repository entry entirely rather than storing it as a literal value. This correctly removes access as intended.
nixilum
left a comment
nixilum
left a comment
There was a problem hiding this comment.
@techplayz32 Could you please look at my and CodeRabbit's comments? There are some changes are required to be made.
|
@coderabbitai help |
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go.mod (1)
3-3: Fix go directive: patch versions are invalid in go.mod.Use only major.minor, e.g.,
go 1.25(or your actual supported Go version).-go 1.25.1 +go 1.25internal/server/handler.go (1)
56-75: Enforce repository permissions via the authenticator.Apply per-request permission checks; Unauthorized -> 401, insufficient -> 403.
// Check authentication requireAuth := auth.RequireAuth(r.Method, service, h.cfg.Auth.AnonymousRead) if requireAuth { user, ok := auth.UserFromContext(r.Context()) if !ok || user.Anonymous { w.Header().Set("WWW-Authenticate", `Basic realm="workgit"`) http.Error(w, "Unauthorized", http.StatusUnauthorized) return } // Audit log for pushes if service == "git-receive-pack" { h.logger.WithContext(r.Context()).Info("push attempt", "user", user.ID, "repo", repoName, ) } } + + // Enforce per-repo permissions (allows anonymous read when configured) + if err := h.auth.RequirePermission(r.Context(), repoName, service); err != nil { + if errors.Is(err, auth.ErrUnauthorized) { + w.Header().Set("WWW-Authenticate", `Basic realm="workgit"`) + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + http.Error(w, "Forbidden", http.StatusForbidden) + return + }
♻️ Duplicate comments (1)
internal/server/server.go (1)
38-41: Fix handler construction, wire auth middleware, and align users storage path.
- Handle
NewGitHandlererror (signature returns(*GitHandler, error)).- Apply authentication middleware; otherwise
auth.UserFromContextis never set and all auth-required requests fail.- Avoid passing
<root>/userswhenNewAuthenticatoralready appends"users"(current code yields<root>/users/users). Pass the storage root instead.@@ -import ( +import ( "context" "crypto/tls" "errors" "fmt" "net/http" "os" "os/signal" - "path/filepath" "syscall" "time" @@ -func New(cfg *config.Config, store storage.Store, logger *log.Logger) (*Server, error) { - usersDir := filepath.Join(cfg.Storage.Root, "users") - authenticator, err := auth.NewAuthenticator(cfg.Auth, usersDir) +func New(cfg *config.Config, store storage.Store, logger *log.Logger) (*Server, error) { + authenticator, err := auth.NewAuthenticator(cfg.Auth, cfg.Storage.Root) if err != nil { return nil, fmt.Errorf("initialize authenticator: %w", err) } @@ - mux := http.NewServeMux() - s.registerRoutes(mux) + mux := http.NewServeMux() + if err := s.registerRoutes(mux); err != nil { + return nil, fmt.Errorf("register routes: %w", err) + } @@ -func (s *Server) registerRoutes(mux *http.ServeMux) { +func (s *Server) registerRoutes(mux *http.ServeMux) error { @@ - handler := NewGitHandler(s.store, s.logger, s.cfg) + handler, err := NewGitHandler(s.store, s.logger, s.cfg) + if err != nil { + return fmt.Errorf("new git handler: %w", err) + } + // Apply auth middleware (optional mode; handler decides if auth is required per request) + secured := s.auth.Middleware(false)(handler) @@ - mux.Handle("/", s.wrapHandler(handler)) + mux.Handle("/", s.wrapHandler(secured)) + return nil }Previous review asked to plumb the error and update registerRoutes; this completes the change in this file too.
Also applies to: 79-96, 51-53, 6-15
🧹 Nitpick comments (4)
internal/auth/users.go (3)
168-193: Ensure regenerated tokens are unique; avoid accidental collision.Low probability, but easy to harden by checking
s.tokens.- // Generate new token - newToken := generateToken() - user.Token = newToken + // Generate new token (ensure uniqueness) + var newToken string + for { + cand := generateToken() + if _, taken := s.tokens[cand]; !taken { + newToken = cand + break + } + } + user.Token = newToken
293-304: Avoid returning password hashes and tokens from ListUsers.Exposing
PasswordHashandTokenrisks leakage via logs/CLI/HTTP. Return a sanitized view or deep copies with sensitive fields redacted.Example new API (add alongside existing until callers migrate):
type PublicUser struct { Username string `json:"username"` Email string `json:"email,omitempty"` FullName string `json:"full_name,omitempty"` Active bool `json:"active"` CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` Permissions map[string]Permission `json:"permissions"` GlobalAdmin bool `json:"global_admin"` } func (s *UserStore) ListUsersPublic() []PublicUser { s.mu.RLock(); defer s.mu.RUnlock() out := make([]PublicUser, 0, len(s.users)) for _, u := range s.users { out = append(out, PublicUser{ Username: u.Username, Email: u.Email, FullName: u.FullName, Active: u.Active, CreatedAt: u.CreatedAt, UpdatedAt: u.UpdatedAt, Permissions: map[string]Permission(u.Permissions), GlobalAdmin: u.GlobalAdmin, }) } return out }
105-116: Return copies to avoid external mutation of internal state.
GetUser/GetPermissionexpose internal pointers/state. Consider copyingUserInfoandPermissionsmap before returning to prevent unsynchronized mutation.Also applies to: 221-243
internal/server/handler.go (1)
203-209: Use filepath.Base for filesystem paths.
path.Baseis for slash-separated paths; preferfilepath.Basefor OS paths.- unlock, lockErr := h.store.Lock(ctx, path.Base(repoPath)) + unlock, lockErr := h.store.Lock(ctx, filepath.Base(repoPath))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod(2 hunks)internal/auth/auth.go(8 hunks)internal/auth/users.go(1 hunks)internal/server/handler.go(2 hunks)internal/server/server.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/server/handler.go (2)
internal/config/config.go (1)
Config(14-21)internal/auth/auth.go (1)
NewAuthenticator(49-86)
internal/server/server.go (1)
internal/auth/auth.go (1)
NewAuthenticator(49-86)
internal/auth/users.go (1)
internal/auth/auth.go (1)
ErrUnauthorized(22-22)
internal/auth/auth.go (2)
internal/config/config.go (1)
AuthConfig(53-57)internal/auth/users.go (5)
UserStore(51-56)NewUserStore(59-71)Permission(27-27)PermissionRead(31-31)PermissionWrite(32-32)
🔇 Additional comments (1)
go.mod (1)
23-23: Incorrect review comment:go.yaml.in/yaml/v2is actually the canonical, maintained module path.The canonical path is
go.yaml.in/yaml/v2(or/v3for v3);gopkg.in/yaml.v2/v3are the older endpoints for historical compatibility. The entry in your go.mod is correct and should not be removed or changed.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/auth/users.go (3)
82-85: Critical: This code will not compile.
hashPasswordat lines 368-371 returns onlystring, but this call expects(string, error). The function signature must be updated to return an error.This relates to past review feedback that has been partially addressed. Apply this fix:
-func hashPassword(password string) string { - h, _ := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) - return string(h) -} +func hashPassword(password string) (string, error) { + h, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + if err != nil { + return "", fmt.Errorf("hash password: %w", err) + } + return string(h), nil +}
166-167: Handle hashing errors and use UTC timestamps consistently.Once
hashPasswordis updated to return errors (see previous comment), this call site must handle them. Additionally, use.UTC()for consistency with user creation.Apply this diff after fixing
hashPassword:- user.PasswordHash = hashPassword(newPassword) - user.UpdatedAt = time.Now() + hashed, err := hashPassword(newPassword) + if err != nil { + return fmt.Errorf("update password: %w", err) + } + user.PasswordHash = hashed + user.UpdatedAt = time.Now().UTC()
368-371: Critical: bcrypt errors are still being ignored.Despite past review comments marking this as "Fixed in the next push," the error from
bcrypt.GenerateFromPasswordis still being ignored. This will cause silent failures when hashing fails.This is the same issue flagged in previous reviews. Apply this diff:
-func hashPassword(password string) string { - h, _ := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) - return string(h) -} +func hashPassword(password string) (string, error) { + h, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + if err != nil { + return "", fmt.Errorf("hash password: %w", err) + } + return string(h), nil +}Then update all call sites (lines 82, 166) to handle the returned error.
🧹 Nitpick comments (1)
internal/auth/users.go (1)
377-385: Token generation is much improved, but consider the fallback.The error handling for
rand.Readand use ofRawURLEncodingaddress previous feedback well. However, the time-based fallback is cryptographically weak and produces a much shorter token.Consider panicking or returning an error instead of falling back to weak entropy:
func generateToken() string { b := make([]byte, 32) if _, err := rand.Read(b); err != nil { - // extremely unlikely; fall back to time-based entropy - ts := time.Now().UnixNano() - return base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf("%d", ts))) + // This should never happen in practice; if it does, the system + // is critically compromised and we should not silently degrade. + panic(fmt.Sprintf("crypto/rand failed: %v", err)) } return base64.RawURLEncoding.EncodeToString(b) }Alternatively, change the signature to return
(string, error)and propagate the failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/auth/users.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/auth/users.go (1)
internal/auth/auth.go (1)
ErrUnauthorized(22-22)
🔇 Additional comments (3)
internal/auth/users.go (3)
135-154: Authentication logic is sound.The function correctly checks if the user exists, is active, and verifies the password. The use of
ErrUnauthorizedfromauth.gois appropriate.
336-364: Excellent atomic write implementation.The use of a temporary file followed by atomic rename prevents corruption if the process crashes during save. File and directory permissions (0600/0700) appropriately restrict access.
387-405: Permission helpers are well-designed.The validation and hierarchical permission checking are implemented correctly.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
internal/auth/users.go (3)
308-318: Don’t return internal pointers or secrets from ListUsers.Returning live pointers enables external mutation and leaks
PasswordHashandToken. Provide a redacted copy.Option A (non-breaking): add a redacted variant and use it in CLI/UI.
// ListUsersRedacted returns safe copies without secrets. func (s *UserStore) ListUsersRedacted() []*UserInfo { s.mu.RLock() defer s.mu.RUnlock() out := make([]*UserInfo, 0, len(s.users)) for _, u := range s.users { c := *u c.PasswordHash = "" c.Token = "" out = append(out, &c) } return out }Option B (breaking): change ListUsers to redact and deep-copy (preferred long term).
358-371: Durability: fsync temp file before atomic rename.Current flow writes then renames without syncing; a crash can lose updates. Sync the temp file first.
- tmpFile := s.filePath + ".tmp" - if err := os.WriteFile(tmpFile, data, 0600); err != nil { - return fmt.Errorf("write temp file: %w", err) - } - - if err := os.Rename(tmpFile, s.filePath); err != nil { - return fmt.Errorf("rename file: %w", err) - } + dir := filepath.Dir(s.filePath) + f, err := os.CreateTemp(dir, "users-*.tmp") + if err != nil { + return fmt.Errorf("create temp file: %w", err) + } + tmpFile := f.Name() + if _, err := f.Write(data); err != nil { + f.Close() + _ = os.Remove(tmpFile) + return fmt.Errorf("write temp file: %w", err) + } + if err := f.Sync(); err != nil { + f.Close() + _ = os.Remove(tmpFile) + return fmt.Errorf("sync temp file: %w", err) + } + if err := f.Close(); err != nil { + _ = os.Remove(tmpFile) + return fmt.Errorf("close temp file: %w", err) + } + if err := os.Rename(tmpFile, s.filePath); err != nil { + _ = os.Remove(tmpFile) + return fmt.Errorf("rename file: %w", err) + }
409-417: Tiny perf nit: avoid re-allocating the levels map per call.Make
levelsa package-level var or use a switch.internal/server/handler.go (1)
33-45: Minor: pass storage root and let NewAuthenticator normalize users dir.Reduce coupling by passing the root;
NewAuthenticatoralready appends “users” when needed.-func NewGitHandler(store storage.Store, logger *log.Logger, cfg *config.Config) (*GitHandler, error) { - usersDir := filepath.Join(cfg.Storage.Root, "users") - authenticator, err := auth.NewAuthenticator(cfg.Auth, usersDir) +func NewGitHandler(store storage.Store, logger *log.Logger, cfg *config.Config) (*GitHandler, error) { + authenticator, err := auth.NewAuthenticator(cfg.Auth, cfg.Storage.Root) if err != nil { return nil, fmt.Errorf("auth init: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
go.mod(2 hunks)internal/auth/auth.go(8 hunks)internal/auth/users.go(1 hunks)internal/server/handler.go(4 hunks)internal/server/server.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/server/server.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/server/handler.go (3)
internal/storage/filesystem.go (1)
Store(27-45)internal/config/config.go (1)
Config(14-21)internal/auth/auth.go (2)
NewAuthenticator(49-89)ErrUnauthorized(22-22)
internal/auth/auth.go (2)
internal/config/config.go (1)
AuthConfig(53-57)internal/auth/users.go (5)
UserStore(51-56)NewUserStore(59-71)Permission(27-27)PermissionRead(31-31)PermissionWrite(32-32)
internal/auth/users.go (1)
internal/auth/auth.go (1)
ErrUnauthorized(22-22)
🔇 Additional comments (3)
go.mod (2)
8-8: Dependencies verified — both actively used.x/term is correctly imported in
cmd/workgit/user.gofor CLI operations, and bcrypt functions are actively used ininternal/auth/users.gofor password hashing. Dependency graph is clean.
3-3: go.mod file is correct and all changes are verified.Go 1.25 was released on August 12, 2025, confirming the
go 1.25directive is valid. All declared dependencies (x/term, x/crypto, x/sys) are actively used in the codebase:
- x/term:
cmd/workgit/user.go- x/crypto:
internal/auth/users.go(bcrypt)- x/sys: transitive dependency
Note: The repository contains no CI configuration files, so the CI verification request cannot be fulfilled. If CI tooling is managed externally, ensure it's configured separately.
internal/server/handler.go (1)
214-214: No action required.The lock key is correct. Since
repoPath = filepath.Join(s.root, repoName)andvalidateRepoNameprevents path separators (/,\),filepath.Base(repoPath)always equalsrepoName. The Lock implementation stores locks by this name in a map, which is exactly the repo identifier. The code is consistent and correct.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/server/server.go (2)
36-49: Propagate Git handler init errors; don’t continue with a nil handler.registerRoutes creates an error with fmt.Errorf but ignores it, and New() ignores route setup failures. Fail fast and bubble the error.
Apply:
func New(cfg *config.Config, store storage.Store, logger *log.Logger) (*Server, error) { - authenticator, err := auth.NewAuthenticator(cfg.Auth, cfg.Storage.Root) + authenticator, err := auth.NewAuthenticator(cfg.Auth, cfg.Storage.Root) if err != nil { return nil, fmt.Errorf("initialize authenticator: %w", err) } @@ - mux := http.NewServeMux() - s.registerRoutes(mux) + mux := http.NewServeMux() + if err := s.registerRoutes(mux); err != nil { + return nil, fmt.Errorf("register routes: %w", err) + } @@ -func (s *Server) registerRoutes(mux *http.ServeMux) { +func (s *Server) registerRoutes(mux *http.ServeMux) error { @@ - handler, err := NewGitHandler(s.store, s.logger, s.cfg) - if err != nil { - fmt.Errorf("failed to create Git handler: %w", err) - return - } + handler, err := NewGitHandler(s.store, s.logger, s.cfg) + if err != nil { + return fmt.Errorf("create Git handler: %w", err) + } @@ - mux.Handle("/", s.wrapHandler(handler)) + mux.Handle("/", s.wrapHandler(handler)) + return nil }Also applies to: 77-97
99-107: Add authentication middleware to the chain.Handlers rely on UserFromContext, but it’s never set. Without auth middleware, pushes always 401 and permission checks see anonymous users.
Apply:
func (s *Server) wrapHandler(h http.Handler) http.Handler { // Apply middleware in reverse order - h = s.loggingMiddleware(h) - h = s.metricsMiddleware(h) - h = s.rateLimitMiddleware(h) - h = s.requestIDMiddleware(h) + h = s.loggingMiddleware(h) + h = s.metricsMiddleware(h) + h = s.rateLimitMiddleware(h) + h = s.requestIDMiddleware(h) + h = s.auth.Middleware(false)(h) return h }
♻️ Duplicate comments (1)
internal/auth/users.go (1)
140-151: Stop user enumeration in AuthenticateUser.Distinct errors (“user not found”, “disabled”) leak account state. Return ErrUnauthorized uniformly.
Apply:
- user, exists := s.users[username] - if !exists { - return nil, ErrUserNotFound - } - - if !user.Active { - return nil, errors.New("user account is disabled") - } - - if !verifyPassword(password, user.PasswordHash) { - return nil, ErrUnauthorized - } + user, exists := s.users[username] + if !exists || !user.Active || !verifyPassword(password, user.PasswordHash) { + // Optional: run a dummy bcrypt compare here to flatten timing. + return nil, ErrUnauthorized + }
🧹 Nitpick comments (3)
internal/auth/users.go (1)
50-56: Consider storing API tokens non-reversibly (hashed).Storing raw tokens in users.json means a file read leaks all tokens. Prefer a lookup-safe scheme (e.g., keyed HMAC of token as map key plus bcrypt/argon2id for verification) to avoid plaintext token exposure. Can be a follow-up.
Also applies to: 122-133, 311-321
internal/server/handler.go (1)
213-219: Lock by repo name, not derived path (verify store expectation).Store.Lock typically expects the repository name key. Passing filepath.Base(repoPath) likely works, but using repoName directly avoids coupling to filesystem layout. Consider changing handleService signature to accept repoName and pass that to Lock.
cmd/workgit/user.go (1)
158-202: Optional: prefer RFC3339 timestamps for JSON output.Human-readable is fine; if you add a “--json” flag, consider RFC3339 for machine parsing consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/workgit/user.go(1 hunks)internal/auth/auth.go(8 hunks)internal/auth/users.go(1 hunks)internal/server/handler.go(4 hunks)internal/server/server.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/workgit/user.go (2)
internal/auth/users.go (2)
NewUserStore(59-71)Permission(27-27)internal/auth/auth.go (1)
NormalizeRepo(263-268)
internal/server/handler.go (3)
internal/storage/filesystem.go (1)
Store(27-45)internal/config/config.go (1)
Config(14-21)internal/auth/auth.go (2)
NewAuthenticator(49-89)ErrUnauthorized(22-22)
internal/auth/auth.go (2)
internal/config/config.go (1)
AuthConfig(53-57)internal/auth/users.go (5)
UserStore(51-56)NewUserStore(59-71)Permission(27-27)PermissionRead(31-31)PermissionWrite(32-32)
internal/server/server.go (2)
internal/auth/auth.go (1)
NewAuthenticator(49-89)internal/server/handler.go (1)
NewGitHandler(33-44)
internal/auth/users.go (1)
internal/auth/auth.go (2)
ErrUnauthorized(22-22)NormalizeRepo(263-268)
🔇 Additional comments (3)
internal/auth/auth.go (1)
174-179: Legacy token identity fix looks correct.You now avoid trusting the Basic username for legacy tokens and use a safe shortTokenPrefix in both Basic and Bearer paths. Good hardening.
Also applies to: 204-207
internal/server/handler.go (1)
32-44: Constructor error handling LGTM.NewGitHandler now returns (handler, error) and plumbs auth init errors correctly.
cmd/workgit/user.go (1)
240-274: Good: repo key normalization before storing permissions.Using auth.NormalizeRepo prevents “repo” vs “repo.git” mismatches between CLI and HTTP checks.
- Add `path/filepath` import to handle path manipulation - Modify user store initialization to use a dedicated "users" subdirectory for storage, ensuring consistent path structure - Update token generation to guarantee uniqueness by: - Implementing retry logic (max 10 attempts) - Verifying token doesn't already exist in the store - Returning error if unique token cannot be generated - Apply changes consistently across all user management functions: `Create`, `List`, `Delete`, `SetPermission`, `Show`, `SetAdmin`, `ResetPassword`, and `RegenerateToken`
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/auth/users.go (1)
199-229: Incomplete rollback on save failure leaves user locked out.If
save()fails, the rollback setsuser.Token = ""(line 227) but doesn't restore the original token value. The old token was deleted at line 200 and overwritten at line 220, leaving the user with no valid authentication token.Apply this diff to preserve and restore the old token:
user, exists := s.users[username] if !exists { return "", ErrUserNotFound } - // Remove old token + // Preserve old token for rollback + oldToken := user.Token delete(s.tokens, user.Token) // Generate new token (ensure uniqueness) var newToken string const maxRetries = 10 for i := 0; i < maxRetries; i++ { cand, err := generateToken() if err != nil { return "", fmt.Errorf("regenerate token: %w", err) } if _, taken := s.tokens[cand]; !taken { newToken = cand break } } if newToken == "" { // Re-add old token if new one couldn't be generated - s.tokens[user.Token] = username + s.tokens[oldToken] = username return "", errors.New("failed to generate unique token after retries") } user.Token = newToken user.UpdatedAt = time.Now().UTC() s.tokens[newToken] = username if err := s.save(); err != nil { - // Rollback on save failure + // Rollback to old token on save failure delete(s.tokens, newToken) - user.Token = "" // Will be invalid, but consistent + user.Token = oldToken + s.tokens[oldToken] = username return "", fmt.Errorf("regenerate token: %w", err) } return newToken, nil
🧹 Nitpick comments (2)
internal/auth/users.go (2)
179-186: Consider rollback on save failure for consistency.If
save()fails, the in-memory password hash is updated but the disk state retains the old hash. After a restart, the old password would work, creating temporary inconsistency.Apply this diff to rollback on save failure:
hashed, err := hashPassword(newPassword) if err != nil { return fmt.Errorf("update password: %w", err) } + oldHash := user.PasswordHash user.PasswordHash = hashed user.UpdatedAt = time.Now().UTC() - return s.save() + if err := s.save(); err != nil { + user.PasswordHash = oldHash + return err + } + return nil
336-347: Consider returning copies to prevent external mutation.
ListUsersreturns pointers to internalUserInfostructs, allowing callers to mutate user data outside the lock. If the API should prevent this, return copies instead.If mutation prevention is desired:
func (s *UserStore) ListUsers() []*UserInfo { s.mu.RLock() defer s.mu.RUnlock() users := make([]*UserInfo, 0, len(s.users)) for _, user := range s.users { - users = append(users, user) + userCopy := *user + users = append(users, &userCopy) } return users }Alternatively, document that callers must not modify returned structs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/workgit/user.go(1 hunks)internal/auth/users.go(1 hunks)internal/server/server.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/workgit/user.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/server/server.go (2)
internal/auth/auth.go (1)
NewAuthenticator(49-89)internal/server/handler.go (1)
NewGitHandler(33-44)
internal/auth/users.go (1)
internal/auth/auth.go (2)
ErrUnauthorized(22-22)NormalizeRepo(263-268)
🔇 Additional comments (12)
internal/server/server.go (3)
50-52: LGTM: Proper error handling for route registration.The error handling correctly propagates registration failures during server initialization.
80-98: LGTM: Error handling integrated into route registration.The updated signature properly propagates Git handler initialization errors.
108-108: LGTM: Authentication middleware integrated.The middleware is properly positioned in the chain.
internal/auth/users.go (9)
20-48: LGTM: Well-structured type definitions.The Permission enum and UserInfo struct follow Go best practices with appropriate field tags and documentation.
58-73: LGTM: Path normalization and initialization.The constructor correctly normalizes paths and gracefully handles the first-run case when users.json doesn't exist yet.
129-153: LGTM: Read operations are thread-safe and correct.Both
GetUserandGetUserByTokenproperly use read locks and handle missing entries.
234-286: LGTM: Permission management is well-implemented.Repository name normalization, permission validation, and global admin checks are correctly handled.
288-318: LGTM: Admin operations use UTC timestamps correctly.Both
SetGlobalAdminandSetActivefollow the consistent pattern with proper timestamp handling.
349-353: LGTM: Access check correctly delegates to permission helpers.
355-403: LGTM: Persistence uses atomic writes and appropriate permissions.The atomic write pattern (temp file + rename) prevents partial writes, and file permissions (0700/0600) properly restrict access.
412-430: LGTM: Security helpers correctly implement bcrypt and crypto/rand.Password hashing and token generation now properly handle errors and use cryptographically secure implementations. All past security issues are resolved.
432-450: LGTM: Permission validation and comparison logic is correct.The hierarchical permission model correctly validates and compares permission levels.
- Add rollback mechanism in CreateUser and DeleteUser when save fails to prevent data inconsistency after failed operations - Implement constant-time password comparison in AuthenticateUser to prevent timing attacks by: * Always performing bcrypt comparison regardless of user existence * Using generated hash for non-existent/inactive users * Maintaining consistent execution time for all authentication attempts - Improve error handling with proper fallback in bcrypt operations to ensure minimum security guarantees even in edge cases
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/auth/users.go (1)
157-190: Critical: Timing attack mitigation is flawed—generates hash on-the-fly for non-existent users.The current approach generates a bcrypt hash on-the-fly for non-existent users (line 172), which takes ~100-200ms. Combined with the comparison, non-existent users take ~200-300ms total, while existing users only take ~100ms for comparison. This creates a timing side-channel that allows username enumeration—the exact vulnerability this code attempts to prevent.
Apply this diff to use a pre-computed dummy hash:
+// Dummy hash for timing attack mitigation (computed once at init) +var dummyHash = "$2a$10$AAAAAAAAAAAAAAAAAAAAAO4Z8K6cF3YYPPVYJJJJJJJJJJJJJJJJJa" + func (s *UserStore) AuthenticateUser(username, password string) (*UserInfo, error) { s.mu.RLock() defer s.mu.RUnlock() user, exists := s.users[username] - - // To prevent timing attacks, always perform a bcrypt comparison - // Use the actual user's hash if they exist, otherwise generate one on-the-fly - var hashToCompare string - if exists && user.Active { - hashToCompare = user.PasswordHash - } else { - // Generate a bcrypt hash on-the-fly for non-existent/inactive users - // This ensures we can't be distinguished even with code inspection - hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) - if err != nil { - // Fallback: at minimum still do some CPU work - _ = bcrypt.CompareHashAndPassword([]byte("$2a$10$AAAAAAAAAAAAAAAAAAAAAO4Z8K6cF3YYPPVYJJJJJJJJJJJJJJJJJa"), []byte(password)) - return nil, ErrUnauthorized - } - hashToCompare = string(hash) + + // Use actual hash if user exists and is active, otherwise use dummy + hashToCompare := dummyHash + if exists && user.Active { + hashToCompare = user.PasswordHash } // Always perform the comparison valid := verifyPassword(password, hashToCompare) // Only return success if user exists, is active, and password matches if exists && user.Active && valid { return user, nil } return nil, ErrUnauthorized }The pre-computed dummy hash ensures consistent ~100ms timing for all authentication attempts, regardless of username validity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/auth/users.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/auth/users.go (1)
internal/auth/auth.go (2)
ErrUnauthorized(22-22)NormalizeRepo(263-268)
The token regeneration process now properly preserves the old token value throughout the operation, ensuring complete rollback capability in case of failures. Previously, the rollback could leave the system in an inconsistent state by clearing the user's token instead of restoring the original one. Key changes: - Store old token value before modification - Use stored old token for all rollback operations - Ensure token map consistency during rollback - Maintain proper token association during failure scenarios
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/auth/users.go (1)
157-190: Simplify timing attack mitigation with a constant dummy hash.The current approach generates a new bcrypt hash on-the-fly for non-existent/inactive users (line 172). While this achieves constant-time behavior, generating a hash on every failed authentication is expensive and more complex than necessary.
The standard pattern uses a pre-computed package-level dummy hash:
+// Dummy hash for timing attack mitigation (computed once) +const dummyHash = "$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92ldGxad68LJZdL17lhWy" + func (s *UserStore) AuthenticateUser(username, password string) (*UserInfo, error) { s.mu.RLock() defer s.mu.RUnlock() user, exists := s.users[username] - - // To prevent timing attacks, always perform a bcrypt comparison - // Use the actual user's hash if they exist, otherwise generate one on-the-fly - var hashToCompare string - if exists && user.Active { - hashToCompare = user.PasswordHash - } else { - // Generate a bcrypt hash on-the-fly for non-existent/inactive users - // This ensures we can't be distinguished even with code inspection - hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) - if err != nil { - // Fallback: at minimum still do some CPU work - _ = bcrypt.CompareHashAndPassword([]byte("$2a$10$AAAAAAAAAAAAAAAAAAAAAO4Z8K6cF3YYPPVYJJJJJJJJJJJJJJJJJa"), []byte(password)) - return nil, ErrUnauthorized - } - hashToCompare = string(hash) - } - - // Always perform the comparison - valid := verifyPassword(password, hashToCompare) - - // Only return success if user exists, is active, and password matches - if exists && user.Active && valid { + + // Use actual hash if user exists and is active, otherwise dummy hash + hashToCompare := dummyHash + if exists && user.Active { + hashToCompare = user.PasswordHash + } + + // Always perform bcrypt comparison for constant timing + valid := verifyPassword(password, hashToCompare) + + // Return success only if user exists, is active, and password valid + if exists && user.Active && valid { return user, nil } - + return nil, ErrUnauthorized }This change:
- Eliminates expensive hash generation on every failed auth
- Simplifies the code significantly
- Follows the industry-standard pattern recommended in OWASP guidance
- Maintains constant-time behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/auth/users.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/auth/users.go (1)
internal/auth/auth.go (2)
ErrUnauthorized(22-22)NormalizeRepo(263-268)
🔇 Additional comments (4)
internal/auth/users.go (4)
58-129: Excellent implementation of user creation with proper safeguards.The code correctly addresses all previously identified concerns:
- Path normalization prevents inconsistent file locations
- Token generation with uniqueness enforcement and bounded retries
- Error handling for hash generation
- UTC timestamps for portability
- Proper rollback on save failure to maintain consistency
192-367: Well-implemented CRUD operations with proper safeguards.All user management methods correctly implement:
- UTC timestamps throughout for consistency
- Proper rollback semantics in RegenerateToken and DeleteUser
- Error propagation from hashPassword
- Repository name normalization
- Thread-safe operations with appropriate locking
The rollback logic in RegenerateToken (lines 250-256) properly restores the old token on save failure, addressing the previously identified consistency issue.
408-436: Secure persistence with atomic writes and proper permissions.The save method implements best practices:
- Creates directory with restrictive permissions (0700)
- Uses atomic write pattern with temp file
- Sets secure file permissions (0600)
- Proper error handling and wrapping
This prevents partial writes and unauthorized access to user credentials.
445-463: Authentication helpers correctly implemented with proper error handling.The helper functions properly address all previously identified security concerns:
hashPasswordreturns errors from bcrypt (lines 445-451)generateTokenreturns errors from crypto/rand and uses RawURLEncoding (lines 457-463)- Token size of 32 bytes (256 bits) provides strong entropy
All critical security improvements from past reviews have been successfully implemented.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @techplayz32. * #9 (comment) The following files were modified: * `cmd/workgit/user.go` * `internal/auth/auth.go` * `internal/auth/users.go` * `internal/server/handler.go` * `internal/server/server.go`
This update introduces a comprehensive user management system with permission checks, enhancing user authentication through a user store. It also includes Docker configuration for user directory mounting and updates dependencies. Implements #8
Summary by CodeRabbit
New Features
Chores
Documentation