Conversation
appleboy
commented
Jan 13, 2026
appleboy
commented
- Add support for pluggable authentication providers: local database and external HTTP API
- Introduce environment variables and configuration for selecting authentication mode and HTTP API settings
- Implement LocalAuthProvider and HTTPAPIAuthProvider with unified authentication flow
- Extend user model to include external authentication fields (external ID, auth source, email, full name)
- Update UserService to route authentication per-user based on auth source and sync external user data
- Add store methods for upserting and querying users by external ID and auth source
- Add validation for authentication configuration at startup
- Update documentation to describe hybrid authentication architecture and configuration options
- Add support for pluggable authentication providers: local database and external HTTP API - Introduce environment variables and configuration for selecting authentication mode and HTTP API settings - Implement LocalAuthProvider and HTTPAPIAuthProvider with unified authentication flow - Extend user model to include external authentication fields (external ID, auth source, email, full name) - Update UserService to route authentication per-user based on auth source and sync external user data - Add store methods for upserting and querying users by external ID and auth source - Add validation for authentication configuration at startup - Update documentation to describe hybrid authentication architecture and configuration options Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
- Fix minor formatting issue in a markdown table - Add blank lines for improved readability in several sections Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
Resolved conflicts by combining features from both branches: - Retained PostgreSQL support (DATABASE_DRIVER, DATABASE_DSN) from main - Retained HTTP API authentication (AUTH_MODE, HTTP_API_*) from user - Combined environment variables and documentation - Both features now work together in the codebase Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
✅ Deploy Preview for authgate-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This pull request adds a hybrid authentication system with pluggable providers, enabling the application to authenticate users via either a local database or an external HTTP API. The system supports per-user authentication routing based on each user's auth_source field, allowing for mixed authentication modes within a single deployment.
Changes:
- Added LocalAuthProvider and HTTPAPIAuthProvider with a unified authentication interface
- Extended User model with external authentication fields (ExternalID, AuthSource, Email, FullName)
- Updated UserService to route authentication per-user based on auth_source field and sync external user data automatically
- Added configuration validation for authentication mode at startup and comprehensive documentation
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Added authentication provider initialization, configuration validation, and wiring of providers to UserService |
| internal/store/sqlite.go | Added GetUserByExternalID and UpsertExternalUser methods for managing external users |
| internal/services/user.go | Refactored Authenticate method to support per-user authentication routing and external user synchronization |
| internal/models/user.go | Extended User model with ExternalID, AuthSource, Email, and FullName fields, plus IsExternal helper method |
| internal/config/config.go | Added authentication configuration fields (AuthMode, HTTPAPIURL, HTTPAPITimeout, HTTPAPIInsecureSkipVerify) with parsing helpers |
| internal/auth/types.go | Defined AuthResult structure for unified authentication response |
| internal/auth/local.go | Implemented LocalAuthProvider for database-based authentication |
| internal/auth/http_api.go | Implemented HTTPAPIAuthProvider for external HTTP API authentication |
| internal/auth/errors.go | Defined authentication-specific error types |
| CLAUDE.md | Added comprehensive documentation of hybrid authentication architecture and configuration |
| .env.example | Added authentication configuration examples and comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fallthrough | ||
| default: | ||
| if s.localProvider == nil { | ||
| return nil, fmt.Errorf("%w: local provider not configured", ErrAuthProviderFailed) | ||
| } | ||
| providerName = AuthModeLocal | ||
| authResult, err = s.localProvider.Authenticate(user.Username, password) |
There was a problem hiding this comment.
The switch statement uses fallthrough from AuthModeLocal to default, treating both local and any unrecognized auth_source values the same way. While this provides a safe fallback, it means users with typos or corrupted auth_source values will silently be treated as local users. Consider explicitly handling only "local" and returning an error for unrecognized auth_source values to catch configuration issues early.
| fallthrough | |
| default: | |
| if s.localProvider == nil { | |
| return nil, fmt.Errorf("%w: local provider not configured", ErrAuthProviderFailed) | |
| } | |
| providerName = AuthModeLocal | |
| authResult, err = s.localProvider.Authenticate(user.Username, password) | |
| if s.localProvider == nil { | |
| return nil, fmt.Errorf("%w: local provider not configured", ErrAuthProviderFailed) | |
| } | |
| providerName = AuthModeLocal | |
| authResult, err = s.localProvider.Authenticate(user.Username, password) | |
| default: | |
| return nil, fmt.Errorf("%w: unsupported auth source %q", ErrAuthProviderFailed, user.AuthSource) |
| case "http_api": | ||
| if cfg.HTTPAPIURL == "" { | ||
| return errors.New("HTTP_API_URL is required when AUTH_MODE=http_api") | ||
| } |
There was a problem hiding this comment.
The validation checks if HTTPAPIURL is empty when AUTH_MODE is http_api, but doesn't validate if it's a valid URL format. An invalid URL like "not-a-url" will pass validation but cause runtime errors when the HTTP API provider attempts to make requests. Consider using url.Parse to validate the URL format during startup configuration validation.
| @@ -9,11 +9,23 @@ type User struct { | |||
| Username string `gorm:"uniqueIndex;not null"` | |||
| PasswordHash string `gorm:"not null"` | |||
There was a problem hiding this comment.
The PasswordHash field has a 'not null' constraint in the GORM tag, but external users are created with an empty password hash (empty string). While an empty string satisfies the 'not null' constraint, this might be confusing. Consider either removing the 'not null' constraint to allow NULL values for external users, or explicitly documenting that external users have an empty string (not NULL) as their password hash.
| PasswordHash string `gorm:"not null"` | |
| PasswordHash string `gorm:"not null"` // For external users, this is an empty string (not NULL) |
| // User doesn't exist, create new | ||
| user = models.User{ | ||
| ID: uuid.New().String(), | ||
| Username: username, | ||
| PasswordHash: "", // No local password for external users | ||
| Role: "user", | ||
| ExternalID: externalID, | ||
| AuthSource: authSource, | ||
| Email: email, | ||
| FullName: fullName, | ||
| } | ||
|
|
||
| if err := s.db.Create(&user).Error; err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &user, nil |
There was a problem hiding this comment.
The UpsertExternalUser function doesn't check for username conflicts when creating a new user. If an external user has the same username as an existing local user, the database insert will fail due to the unique constraint on the username field. Consider checking for existing usernames and either returning a more specific error or appending a suffix to prevent conflicts.
| if err == nil { | ||
| // User exists, update fields | ||
| user.Username = username | ||
| user.Email = email | ||
| user.FullName = fullName | ||
| if err := s.db.Save(&user).Error; err != nil { | ||
| return nil, err | ||
| } | ||
| return &user, nil |
There was a problem hiding this comment.
When updating an existing external user during authentication, only the username, email, and full name are synced. The ExternalID is not updated. If an external system reassigns external IDs, this could lead to inconsistencies. Consider whether ExternalID should also be updated during sync, or document that external IDs are immutable.
| func getEnvDuration(key string, defaultValue time.Duration) time.Duration { | ||
| if value := os.Getenv(key); value != "" { | ||
| if d, err := time.ParseDuration(value); err == nil { | ||
| return d | ||
| } | ||
| } | ||
| return defaultValue |
There was a problem hiding this comment.
The getEnvDuration function silently falls back to the default value when it encounters an invalid duration format, without logging any warning. If a user misconfigures HTTP_API_TIMEOUT with an invalid value like "10seconds" instead of "10s", the application will start successfully but use the default 10s timeout without notifying the user of the configuration error. Consider logging a warning when duration parsing fails.
- Add context.Context parameter to authentication methods across providers and services - Update authentication calls to pass context from HTTP requests - Refactor internal service logic to propagate context during user authentication Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
- Refactor Authenticate method signatures to use multi-line parameter formatting for improved readability Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
- Improve error handling in HTTP API authentication by checking status codes and parsing error messages from JSON responses - Add truncation for large error response bodies to prevent log overflow - Include Email and FullName fields in local authentication response Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
- Document the required and optional fields for HTTP API authentication responses - Enforce validation that user_id must be present when authentication succeeds - Add comprehensive tests for HTTP API authentication, covering success, missing user_id, authentication failure, non-2xx responses, and invalid JSON cases Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
- Add detailed handling and documentation for username conflicts during external authentication - Implement error propagation for username conflicts in user service and store layers - Update login handler to display specific error message for username conflicts - Add tests for username conflict scenarios when creating or updating external users - Improve error reporting for user creation and update failures in the store - Create a new error definitions file for store-level errors Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
