feat: Implement OIDC user management and authentication flow #126
Conversation
WalkthroughThis update introduces comprehensive OpenID Connect (OIDC) authentication support, enabling enterprise SSO workflows alongside local authentication. It includes new configuration fields, database schema changes, service logic, web routes, and user interface updates for OIDC. Supporting code, documentation, and tests are added or modified to integrate and secure the OIDC flow, including error handling and user management. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant OIDCProvider
participant Database
User->>WebApp: Clicks "Login with SSO"
WebApp->>User: Redirect to OIDCProvider (with state)
User->>OIDCProvider: Authenticates
OIDCProvider->>User: Redirect back to WebApp (with code, state)
User->>WebApp: Callback with code and state
WebApp->>WebApp: Validate state cookie
WebApp->>OIDCProvider: Exchange code for ID token
OIDCProvider->>WebApp: Return ID token and user info
WebApp->>Database: Create or update user (by OIDC subject)
Database-->>WebApp: User record
WebApp->>Database: Create session
Database-->>WebApp: Session info
WebApp->>User: Set session cookie, redirect to dashboard
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi @pascalinthecloud , Thank you very very much for your PR, give me some time to review and merge it, this should solve part of #16, thanks for your help |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Docstrings generation was requested by @eduardolat. * #126 (comment) The following files were modified: * `cmd/app/main.go` * `cmd/changepw/main.go` * `internal/config/env_validate.go` * `internal/service/oidc/oidc.go` * `internal/service/service.go` * `internal/service/users/users.go` * `internal/view/web/auth/login.go` * `internal/view/web/auth/router.go` * `internal/view/web/dashboard/profile/update_user.go` * `internal/view/web/oidc/router.go`
|
Note Generated docstrings for this pull request at #127 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
go.mod (1)
3-3:go 1.23.5is an invalid directive and will breakgo mod tidy.The
godirective only acceptsmajor.minor(e.g.,go 1.22). Patch levels are not allowed.-go 1.23.5 +go 1.22Adjust to the minimum compiler version you actually need, update CI, and re-tidy.
🧹 Nitpick comments (9)
go.mod (2)
25-28: Keep thex/*andtestifyversions aligned or rely on the transitive ones to avoid version skew.
golang.org/x/crypto v0.36.0,golang.org/x/oauth2 v0.30.0, andgolang.org/x/sync v0.12.0are all fine individually, but bumping them inconsistently can introduce subtle API changes. Two suggestions:- github.com/stretchr/testify v1.10.0 - golang.org/x/crypto v0.36.0 - golang.org/x/oauth2 v0.30.0 - golang.org/x/sync v0.12.0 + # Pin to the same date-coherent tag set after testing + github.com/stretchr/testify v1.10.1 + golang.org/x/crypto v0.37.0 + golang.org/x/oauth2 v0.31.0 + golang.org/x/sync v0.13.0or simply let
go mod tidy -compat=1.22decide the minimal versions that satisfygo-oidc.
48-48:go-jose/v4will be pulled in indirectly; mark it// indirector drop it.Because
go-oidcdepends on JOSE, declaring it explicitly as a direct requirement is redundant unless you use it in your own code. Consider:- github.com/go-jose/go-jose/v4 v4.0.5 // indirect +# Removed; inherited from go-oidcRun
go mod tidyafterwards to confirm no build-list changes.internal/service/oidc/queries.sql (1)
1-4: Consider adding conflict handling for user creation.The INSERT query doesn't handle potential conflicts. Consider adding
ON CONFLICTclause if duplicate OIDC identities should be handled gracefully.-- name: OIDCServiceCreateUser :one INSERT INTO users (name, email, oidc_provider, oidc_subject) VALUES (@name, lower(@email), @oidc_provider, @oidc_subject) ON CONFLICT (oidc_provider, oidc_subject) DO UPDATE SET name = EXCLUDED.name, email = EXCLUDED.email, updated_at = NOW() RETURNING *;internal/service/oidc/oidc_test.go (1)
215-238: Remove duplicate UserInfo tests.The
TestUserInfo_StructValidationfunction duplicates the earlierTestUserInfotests (lines 132-146). Consider removing this redundant test.-func TestUserInfo_StructValidation(t *testing.T) { - t.Run("UserInfo Fields", func(t *testing.T) { - userInfo := UserInfo{ - Email: "[email protected]", - Name: "Test User", - Username: "testuser", - Subject: "sub-123", - } - - assert.Equal(t, "[email protected]", userInfo.Email) - assert.Equal(t, "Test User", userInfo.Name) - assert.Equal(t, "testuser", userInfo.Username) - assert.Equal(t, "sub-123", userInfo.Subject) - }) - - t.Run("Empty UserInfo", func(t *testing.T) { - userInfo := UserInfo{} - - assert.Empty(t, userInfo.Email) - assert.Empty(t, userInfo.Name) - assert.Empty(t, userInfo.Username) - assert.Empty(t, userInfo.Subject) - }) -}internal/view/web/dashboard/profile/update_user.go (1)
21-22: Consider extracting OIDC user check to a helper method.The OIDC user detection logic is duplicated. Consider using the
IsOIDCUsermethod if available in the user model.- // Check if user is OIDC user - isOIDCUser := reqCtx.User.OidcProvider.Valid && reqCtx.User.OidcSubject.Valid + // Check if user is OIDC user + isOIDCUser := h.servs.UsersService.IsOIDCUser(reqCtx.User)Also applies to: 59-60
internal/view/web/oidc/router.go (1)
132-136: Consider sanitizing user email in error logs.While logging is important for debugging, consider whether logging the full email address could be a privacy concern in production environments.
logger.Error("failed to create/update OIDC user", logger.KV{ "ip": c.RealIP(), "ua": c.Request().UserAgent(), - "email": userInfo.Email, + "email_domain": strings.Split(userInfo.Email, "@")[1], // Log only domain for privacy "error": err, })internal/service/oidc/oidc.go (3)
40-66: Handle empty scopes string more gracefully.The constructor is well-implemented with proper error handling and early returns. Consider improving the scopes parsing to handle empty strings:
- scopes := strings.Split(env.PBW_OIDC_SCOPES, " ") + var scopes []string + if env.PBW_OIDC_SCOPES != "" { + scopes = strings.Split(env.PBW_OIDC_SCOPES, " ") + }This prevents creating a slice with an empty string when
PBW_OIDC_SCOPESis empty.
88-148: Strong OIDC token handling with minor improvement needed.The token exchange and verification implementation follows OIDC security best practices. The fallback logic is reasonable, but consider adding email validation before using it for username generation:
// Fallback to email as username if username not provided if userInfo.Username == "" && userInfo.Email != "" { + if !strings.Contains(userInfo.Email, "@") { + return nil, fmt.Errorf("invalid email format for username fallback: %s", userInfo.Email) + } userInfo.Username = strings.Split(userInfo.Email, "@")[0] }This prevents potential issues if the email claim doesn't contain a valid email format.
150-191: Solid user management with account takeover prevention.The user creation and update logic properly prevents account takeover by checking for email conflicts with existing local users. The implementation correctly handles both new and returning OIDC users.
Consider making the OIDC provider identifier configurable rather than hardcoded:
_, err := s.dbgen.OIDCServiceGetUserByOIDC(ctx, dbgen.OIDCServiceGetUserByOIDCParams{ - OidcProvider: sql.NullString{String: "oidc", Valid: true}, + OidcProvider: sql.NullString{String: s.env.PBW_OIDC_PROVIDER_NAME, Valid: true}, OidcSubject: sql.NullString{String: userInfo.Subject, Valid: true}, })This would allow supporting multiple OIDC providers in the future while maintaining clear separation.
📜 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 (23)
OIDC_INTEGRATION.md(1 hunks)cmd/app/main.go(1 hunks)cmd/changepw/main.go(1 hunks)go.mod(4 hunks)internal/config/env.go(1 hunks)internal/config/env_validate.go(1 hunks)internal/database/migrations/20250708000000_add_oidc_support_to_users.sql(1 hunks)internal/service/auth/cookies.go(2 hunks)internal/service/auth/login.go(1 hunks)internal/service/auth/login_oidc.go(1 hunks)internal/service/oidc/oidc.go(1 hunks)internal/service/oidc/oidc_test.go(1 hunks)internal/service/oidc/queries.sql(1 hunks)internal/service/service.go(4 hunks)internal/service/users/create_user.go(2 hunks)internal/service/users/users.go(1 hunks)internal/view/middleware/inject_reqctx.go(1 hunks)internal/view/web/auth/create_first_user.go(2 hunks)internal/view/web/auth/login.go(3 hunks)internal/view/web/auth/router.go(2 hunks)internal/view/web/dashboard/profile/update_user.go(3 hunks)internal/view/web/oidc/router.go(1 hunks)internal/view/web/oidc/router_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
cmd/app/main.go (4)
internal/service/service.go (1)
New(31-61)internal/service/oidc/oidc.go (1)
New(40-66)internal/logger/logger.go (1)
FatalError(70-73)internal/logger/kv.go (1)
KV(6-6)
internal/service/auth/login.go (1)
internal/util/cryptoutil/bcrypt.go (1)
VerifyBcryptHash(19-25)
internal/service/auth/login_oidc.go (3)
internal/service/users/users.go (1)
Service(5-7)internal/service/service.go (1)
Service(19-29)internal/service/oidc/oidc.go (1)
Service(26-31)
internal/service/users/create_user.go (1)
internal/util/cryptoutil/bcrypt.go (1)
CreateBcryptHash(10-16)
🔇 Additional comments (37)
go.mod (2)
13-13: Confirm the newgo-oidcdependency is strictly necessary andtidythe graph afterwards.Adding
github.com/coreos/go-oidc/v3 v3.14.1makes sense for the new OIDC flow, but remember thatgo-oidcalready pulls ingolang.org/x/oauth2,golang.org/x/net, andgithub.zerozr99.workers.dev/go-jose/go-jose/v4transitively. If you list it as a direct dependency, rungo mod tidyto ensure the transitive packages are downgraded to indirect entries unless you really need them promoted to direct.
62-64: Large jumps onx/net,x/sys,x/textdemand CI coverage on all supported Go versions.These libraries often track the tip of Go and occasionally introduce breaking behavioural changes. Ensure your CI matrix still covers the lowest Go version you claim to support (currently unclear because of the invalid
go 1.23.5directive—see next note).cmd/app/main.go (1)
41-44: LGTM! Proper error handling for service initialization.The change correctly handles the updated
service.Newfunction signature that now returns an error due to OIDC service initialization. The error handling pattern is consistent with other initialization error handling in the file and appropriately usesFatalErrorto terminate the application on service initialization failure.internal/service/users/users.go (1)
15-18: Clean and focused utility method for OIDC user identification.The
IsOIDCUsermethod is well-implemented with clear logic that checks bothOidcProviderandOidcSubjectvalidity. This provides a clean abstraction for distinguishing OIDC users from local users throughout the application.cmd/changepw/main.go (1)
63-63: Correct adaptation to nullable password schema.The change properly wraps the hashed password in
sql.NullStringwithValid: true, which aligns with the database schema changes to support OIDC users. Since this is a password reset utility, the password should indeed be valid (non-null).internal/view/web/auth/router.go (2)
8-8: Appropriate import for OIDC integration.The import correctly brings in the OIDC router functionality needed for the authentication flow.
36-37: Clean integration of OIDC routes.The OIDC router mounting follows the established pattern and correctly passes the necessary middleware and services. The comment clearly explains the purpose of this addition.
internal/view/web/auth/create_first_user.go (2)
4-4: Necessary import for nullable password support.The
database/sqlimport is correctly added to support thesql.NullStringtype used for password handling.
132-132: Proper nullable password handling for user creation.The change correctly wraps the password in
sql.NullStringwithValid: true, which is appropriate for first user creation where a password is always provided. This maintains compatibility with the new user creation logic that supports both local and OIDC users.internal/service/auth/login.go (1)
20-26: LGTM! Proper validation for nullable password field.The addition of password validity checking correctly handles the case where OIDC users don't have local passwords set. The error messaging is clear and the logic flow properly validates before attempting password verification.
internal/view/middleware/inject_reqctx.go (1)
38-40: LGTM! Proper extension of user context for OIDC support.Adding the OIDC provider, subject, and password fields to the request context enables downstream handlers to distinguish between local and OIDC users and implement appropriate authentication logic.
internal/service/auth/login_oidc.go (1)
10-27: LGTM! Clean implementation of OIDC session creation.The
LoginOIDCmethod correctly implements session creation for OIDC users by reusing the existing session infrastructure. The method signature is appropriate and error handling follows established patterns.internal/service/auth/cookies.go (1)
21-22: LGTM – Cookie security enhancements are correctly applied
- Verified: there are no existing development‐only overrides for the
Secureflag in the codebase.- If your local/dev environment doesn’t serve over HTTPS, consider conditionally setting
Secure(e.g. onlytruein production via an ENV check) to prevent issues when testing.Affected locations:
- internal/service/auth/cookies.go
• Lines 21–22: SetSessionCookie (Secure: true,SameSite: Lax)
• Lines 34–35: ClearSessionCookie (same attributes)internal/config/env_validate.go (1)
19-33: LGTM! Solid validation approach for OIDC configuration.The validation logic correctly enforces required OIDC environment variables only when OIDC is enabled. The error messages are clear and follow the existing pattern in the codebase.
internal/view/web/auth/login.go (3)
35-42: Good error handling with proper URL decoding.The error message extraction and URL decoding is implemented correctly with appropriate error handling.
70-87: Clean OIDC integration with good UX.The conditional OIDC login button with visual separation ("OR" divider) provides a clear user experience. The hardcoded endpoint "/auth/oidc/login" should be consistent with the OIDC router configuration.
52-68: Toaster Initialization VerifiedThe
window.toasterobject is initialized ininternal/view/static/js/init-notyf.jsbefore any inline scripts execute, ensuring your snippet’s call towindow.toaster.error()is safe. Using a data attribute to pass the error message also correctly mitigates XSS risks—approving these changes.internal/service/service.go (1)
34-34: All callers properly handle the new error returnThe only
service.New(…)invocation is incmd/app/main.go, where its returnederris checked and handled. No other callers were found—this breaking change is fully accommodated.internal/service/oidc/queries.sql (1)
6-18: Well-structured queries with proper email normalization.The queries correctly use email normalization with
lower()and follow good parameterized query patterns. The naming convention is clear and consistent.internal/config/env.go (1)
16-25: Comprehensive and well-designed OIDC configuration.The OIDC environment variables are well-structured with:
- Appropriate feature flag for enabling/disabling
- Sensible defaults for optional fields (scopes, claim mappings)
- Clear naming convention following the existing pattern
- Good separation of required vs optional configuration
The default claim mappings (
preferred_username,name) follow OIDC standards and should work with most providers.internal/database/migrations/20250708000000_add_oidc_support_to_users.sql (1)
1-24: LGTM! Well-structured migration for OIDC support.The migration correctly implements the dual authentication approach with proper constraints and indexes. The temporary column approach for making password nullable is safe and preserves existing data.
internal/service/oidc/oidc_test.go (1)
12-121: Good test coverage for OIDC service functionality.The tests thoroughly cover service initialization, state generation, and error scenarios. The comment acknowledging the limitation in testing the OAuth2 flow without mocks is appreciated.
internal/view/web/dashboard/profile/update_user.go (1)
17-28: Good implementation of OIDC user profile protection.The logic correctly prevents OIDC users from modifying their profiles locally, which aligns with the external identity provider management model.
internal/view/web/oidc/router.go (1)
20-175: Well-implemented OIDC authentication flow with good security practices.The implementation includes proper CSRF protection via state parameter validation, secure cookie settings, comprehensive error handling, and appropriate logging.
internal/view/web/oidc/router_test.go (1)
16-327: Excellent test coverage for OIDC router handlers.The tests comprehensively cover:
- State validation and CSRF protection
- Cookie handling and expiration
- Error scenarios and edge cases
- Both HTMX and regular browser requests
- Integration with the OIDC service
Great attention to security-critical scenarios!
internal/service/oidc/oidc.go (4)
1-16: LGTM! Well-structured imports for OIDC implementation.The import selection is appropriate for an OIDC service, including necessary cryptographic, database, and OAuth2 libraries from reputable sources.
18-24: Well-defined custom errors for OIDC scenarios.The error definitions follow Go conventions and provide clear, descriptive messages for the main OIDC failure scenarios. The exported nature allows for proper error handling in calling code.
26-38: Clean struct definitions with appropriate encapsulation.The Service and UserInfo structs are well-designed with clear responsibilities and appropriate field types for OIDC operations.
68-86: Excellent security practices in helper methods.The helper methods are well-implemented with proper security considerations:
GenerateState()uses cryptographically secure random generation with sufficient entropy (256 bits)GetAuthURL()safely handles disabled OIDC scenarios- Clean API design with appropriate error handling
OIDC_INTEGRATION.md (8)
1-16: Clear and comprehensive overview of OIDC integration.The introduction effectively communicates the purpose and key benefits of the OIDC integration, including enterprise SSO support and seamless authentication workflows.
17-38: Comprehensive configuration guide with practical examples.The environment variable examples are well-structured with clear required vs. optional sections and sensible defaults for claim mappings.
40-68: Excellent provider-specific setup instructions.The detailed setup guides for Authentik, Keycloak, and generic OIDC providers are comprehensive and include all necessary configuration steps with appropriate security settings.
69-92: Accurate database schema documentation with proper constraints.The SQL migration examples correctly show the necessary database changes including appropriate constraints for data integrity and security.
94-108: Clear user flow description with comprehensive security considerations.The documentation accurately describes the user experience and covers essential security measures including CSRF protection, token validation, and secure cookie handling.
109-134: Good architectural overview and UI description.The implementation details provide helpful context about the codebase organization and user interface changes for OIDC integration.
135-153: Practical troubleshooting guidance for common OIDC issues.The troubleshooting section effectively addresses typical configuration problems and provides clear debugging guidance for OIDC integration issues.
154-180: Clear migration guidance and practical development instructions.The documentation effectively explains the coexistence of local and OIDC authentication and provides helpful development setup commands.
| // Convert sql.NullString to string for hashing | ||
| passwordStr := "" | ||
| if params.Password.Valid { | ||
| passwordStr = params.Password.String | ||
| } | ||
|
|
||
| hash, err := cryptoutil.CreateBcryptHash(passwordStr) | ||
| if err != nil { | ||
| return dbgen.User{}, err | ||
| } | ||
| params.Password = hash | ||
|
|
||
| // Convert hash back to sql.NullString | ||
| params.Password = sql.NullString{String: hash, Valid: true} |
There was a problem hiding this comment.
Fix logic for handling null passwords.
The current implementation has a flaw: when params.Password is invalid (null), it creates a bcrypt hash of an empty string and marks it as valid. This is incorrect - if no password is provided, the field should remain null for OIDC users.
Apply this fix to properly handle null passwords:
- // Convert sql.NullString to string for hashing
- passwordStr := ""
- if params.Password.Valid {
- passwordStr = params.Password.String
- }
-
- hash, err := cryptoutil.CreateBcryptHash(passwordStr)
- if err != nil {
- return dbgen.User{}, err
- }
-
- // Convert hash back to sql.NullString
- params.Password = sql.NullString{String: hash, Valid: true}
+ // Only hash password if one was provided
+ if params.Password.Valid && params.Password.String != "" {
+ hash, err := cryptoutil.CreateBcryptHash(params.Password.String)
+ if err != nil {
+ return dbgen.User{}, err
+ }
+ params.Password = sql.NullString{String: hash, Valid: true}
+ }
+ // If password is null/empty, leave it as null for OIDC users📝 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.
| // Convert sql.NullString to string for hashing | |
| passwordStr := "" | |
| if params.Password.Valid { | |
| passwordStr = params.Password.String | |
| } | |
| hash, err := cryptoutil.CreateBcryptHash(passwordStr) | |
| if err != nil { | |
| return dbgen.User{}, err | |
| } | |
| params.Password = hash | |
| // Convert hash back to sql.NullString | |
| params.Password = sql.NullString{String: hash, Valid: true} | |
| // Only hash password if one was provided | |
| if params.Password.Valid && params.Password.String != "" { | |
| hash, err := cryptoutil.CreateBcryptHash(params.Password.String) | |
| if err != nil { | |
| return dbgen.User{}, err | |
| } | |
| params.Password = sql.NullString{String: hash, Valid: true} | |
| } | |
| // If password is null/empty, leave it as null for OIDC users |
🤖 Prompt for AI Agents
In internal/service/users/create_user.go around lines 14 to 26, the code
incorrectly hashes an empty string and sets the password as valid when
params.Password is null. Instead, check if params.Password is valid before
hashing; if it is invalid, leave params.Password as null without hashing. Only
create and assign the bcrypt hash when a valid password is provided.
| -- Make password required again (this will fail if there are OIDC users) | ||
| ALTER TABLE users ALTER COLUMN password SET NOT NULL; | ||
| -- +goose StatementEnd |
There was a problem hiding this comment.
Add safety check before making password NOT NULL in down migration.
The down migration will fail with a constraint violation if any OIDC users exist (users with null passwords). Consider adding a check or removing such users first.
-- Make password required again (this will fail if there are OIDC users)
+-- First check if there are OIDC users
+DO $$
+BEGIN
+ IF EXISTS (SELECT 1 FROM users WHERE password IS NULL) THEN
+ RAISE EXCEPTION 'Cannot rollback: OIDC users exist with null passwords';
+ END IF;
+END $$;
ALTER TABLE users ALTER COLUMN password SET NOT NULL;📝 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.
| -- Make password required again (this will fail if there are OIDC users) | |
| ALTER TABLE users ALTER COLUMN password SET NOT NULL; | |
| -- +goose StatementEnd | |
| -- Make password required again (this will fail if there are OIDC users) | |
| -- First check if there are OIDC users | |
| DO $$ | |
| BEGIN | |
| IF EXISTS (SELECT 1 FROM users WHERE password IS NULL) THEN | |
| RAISE EXCEPTION 'Cannot rollback: OIDC users exist with null passwords'; | |
| END IF; | |
| END $$; | |
| ALTER TABLE users ALTER COLUMN password SET NOT NULL; | |
| -- +goose StatementEnd |
🤖 Prompt for AI Agents
In internal/database/migrations/20250708000000_add_oidc_support_to_users.sql
around lines 33 to 35, the down migration attempts to set the password column as
NOT NULL without checking for existing users with null passwords, causing
constraint violations. To fix this, add a safety check before altering the
column by either deleting or updating users with null passwords to valid values,
or conditionally applying the NOT NULL constraint only if no null passwords
exist.
| c.SetCookie(&http.Cookie{ | ||
| Name: "oidc_state", | ||
| Value: state, | ||
| HttpOnly: true, | ||
| Secure: true, | ||
| SameSite: http.SameSiteLaxMode, | ||
| MaxAge: 300, // 5 minutes | ||
| Path: "/", | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify HTTPS enforcement for Secure cookie flag.
The state cookie has Secure: true, which requires HTTPS. Ensure the application enforces HTTPS in production, otherwise the cookie won't be set in HTTP environments.
🏁 Script executed:
#!/bin/bash
# Check if there's HTTPS enforcement or configuration
rg -A 5 "https|TLS|SSL|Secure.*cookie" --glob "*.go" --glob "*.md"Length of output: 16147
🏁 Script executed:
#!/bin/bash
# Locate entrypoint files
echo "Finding main.go files:"
fd --hidden --glob "main.go"
echo -e "\nSearching for ListenAndServe and ListenAndServeTLS usage:"
rg -n "ListenAndServe" -A 2 --glob "*.go"
echo -e "\nSearching for ServeTLS usage:"
rg -n "ServeTLS" -A 2 --glob "*.go"
echo -e "\nSearching for HTTP-to-HTTPS redirect middleware or handlers:"
rg -n "Redirect" -A 3 --glob "*.go" | rg -i "https|TLS"Length of output: 617
🏁 Script executed:
#!/bin/bash
# Display the beginning of the application entrypoint to inspect HTTP server setup
sed -n '1,200p' cmd/app/main.goLength of output: 1764
Enforce HTTPS for Secure Cookies
The application currently starts an HTTP server (app.Start(address)) without TLS, so cookies with Secure: true won’t be set over plain HTTP. In production you must terminate TLS or enable HTTPS in the app itself.
- File:
cmd/app/main.go, around theapp.Start(address)call - Options:
- Terminate TLS at the edge (e.g. reverse proxy or load balancer) and ensure all traffic is HTTPS
- Use Echo’s
StartTLSmethod with your certificate/key - Add middleware or a separate HTTP listener that redirects HTTP→HTTPS
Please update deployment or code to enforce HTTPS so that Secure cookies are honored.
🤖 Prompt for AI Agents
In internal/view/web/oidc/router.go lines 47 to 55, the cookie is set with
Secure: true but the app currently starts an HTTP server without TLS, so the
cookie won't be sent over plain HTTP. To fix this, update the deployment to
terminate TLS at the edge (e.g., reverse proxy or load balancer) ensuring all
traffic is HTTPS, or modify the app startup in cmd/app/main.go to use Echo's
StartTLS method with proper certificates, or add middleware or a separate
listener to redirect HTTP traffic to HTTPS. This will ensure Secure cookies are
properly set and honored.
There was a problem hiding this comment.
This is actually a good catch. Have you tested with a self-generated certificate or with a real one (e.g. Let's Encrypt) ?
There was a problem hiding this comment.
@cheggerdev I haven't tested this myself as I'm an AI assistant and cannot run applications or set up servers. My recommendation was based on analyzing the code where I noticed the Secure: true flag on cookies while the application starts an HTTP server.
However, I can guide you on testing both scenarios:
Self-signed certificate testing:
# Generate self-signed cert
openssl req -x509 -newkey rsa:4096 -keyout key.pem -out cert.pem -days 365 -nodes
# Modify cmd/app/main.go to use StartTLS:
# app.StartTLS(address, "cert.pem", "key.pem")Let's Encrypt certificate:
- Use certbot or similar tools
- Same
StartTLSapproach with real certificates
Key points for testing:
- The
Secureflag behavior is identical for both certificate types - it just requires any HTTPS connection - Browser dev tools will show if cookies are being set/sent
- Self-signed certs require browser security exceptions but cookies still work
- Test the OIDC flow in both HTTP (cookies won't set) and HTTPS (cookies will set)
Would you like me to help you modify the startup code to support TLS, or do you prefer handling HTTPS termination at the reverse proxy level?
This will also solve #72 . Thank you. |
Depends on what you have in mind with 2FA. If you configure your IdP (e.g. Authentik) to do 2FA then this solves #16 completely. |
3 participants
Hi there,
I've added the functionality of single sign on via OIDC. I tested that feature for about a week and didn't come across any problems.
Here are some screenshots showing the implementation.
Summary by CodeRabbit
New Features
User Interface
Bug Fixes
Documentation
Chores
Tests