feat: add sqlite database for storing sessions#326
Conversation
WalkthroughRemoves secret-file/secret configuration and secret-based session handling; adds SQLite-backed session persistence with embedded migrations and a DatabaseService. Updates CLI/config to include database-path, injects DB into AuthService, renames several AuthService methods to Is*-prefixed variants, and adds container/compose mounting for /data. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as Entrypoint/CLI
participant App as App Bootstrap
participant DBSvc as DatabaseService
participant GORM as GORM(SQLite)
participant Migrator as golang-migrate
participant Auth as AuthService
CLI->>App: Setup()
App->>DBSvc: NewDatabaseService(cfg)
App->>DBSvc: Init()
DBSvc->>GORM: Open sqlite at /data/tinyauth.db
DBSvc->>Migrator: Up() using embedded migrations
Migrator-->>DBSvc: OK / ErrNoChange
DBSvc-->>App: GetDatabase() -> *gorm.DB
App->>Auth: NewAuthService(cfg, docker, ldap, db)
App-->>CLI: Setup complete
sequenceDiagram
autonumber
actor Client as User
participant Proxy as ProxyController
participant Auth as AuthService
participant DB as SQLite (GORM)
Client->>Proxy: HTTP request
Proxy->>Auth: IsBypassedIP(labels, clientIP)?
alt bypassed
Proxy-->>Client: Forward without auth
else
Proxy->>Auth: IsAuthEnabled(uri, labels)?
alt auth disabled
Proxy-->>Client: Forward
else
Proxy->>Auth: GetSessionCookie(request)
Auth->>DB: SELECT * FROM sessions WHERE uuid=...
alt session valid
Proxy->>Auth: IsResourceAllowed(ctx, userCtx, labels)?
alt allowed
Proxy->>Auth: IsInOAuthGroup(ctx, userCtx, labels)?
alt in group
Proxy-->>Client: Forward
else
Proxy-->>Client: 403/redirect
end
else
Proxy-->>Client: 401/redirect
end
else
Proxy-->>Client: 401/redirect (OAuth)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ 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
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #326 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 31 32 +1
Lines 2344 2363 +19
=====================================
- Misses 2344 2363 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
Dockerfile (1)
41-41: Enable CGO for SQLite3 migrations (mattn/go-sqlite3) or switch to a pure-Go driverThe code imports the GORM driver
github.com/glebarez/sqlite(pure-Go, works with CGO_DISABLED) but also uses the migration drivergithub.com/golang-migrate/migrate/v4/database/sqlite3, which depends ongithub.com/mattn/go-sqlite3and requires CGO. Building withCGO_ENABLED=0will fail.– Affected Dockerfile sections: builder stage (line 41) and runtime stage (lines 54–56)
– Migration code:internal/service/database_service.goimportssqliteMigrate "github.com/golang-migrate/migrate/v4/database/sqlite3"(CGO-dependent)Apply one of the following:
Option A: Enable CGO and install SQLite C library for migrations
FROM golang:1.25-alpine3.21 AS builder +RUN apk add --no-cache build-base sqlite-dev @@ -ENV CGO_ENABLED=0 +ENV CGO_ENABLED=1 RUN go build -ldflags "-s -w \ - -X tinyauth/internal/constants.Version=${VERSION} \ - -X tinyauth/internal/constants.CommitHash=${COMMIT_HASH} \ - -X tinyauth/internal/constants.BuildTimestamp=${BUILD_TIMESTAMP}" + -X tinyauth/internal/constants.Version=${VERSION} \ + -X tinyauth/internal/constants.CommitHash=${COMMIT_HASH} \ + -X tinyauth/internal/constants.BuildTimestamp=${BUILD_TIMESTAMP}" FROM alpine:3.22 AS runner -RUN apk add --no-cache curl +RUN apk add --no-cache curl sqlite-libsOption B (if you must keep CGO disabled): replace the migrate driver with a pure-Go alternative (e.g. one built on
modernc.org/sqlite) instead ofdatabase/sqlite3.internal/controller/oauth_controller.go (1)
172-178: Fallback when PreferredUsername is emptySome providers may not populate PreferredUsername. Fall back to email local-part to avoid blank usernames in session/context and downstream headers.
controller.Auth.CreateSessionCookie(c, &config.SessionCookie{ - Username: user.PreferredUsername, + Username: func() string { + if user.PreferredUsername != "" { + return user.PreferredUsername + } + return strings.Split(user.Email, "@")[0] + }(), Name: name, Email: user.Email, Provider: req.Provider, OAuthGroups: utils.CoalesceToString(user.Groups), })
🧹 Nitpick comments (22)
internal/middleware/zerolog_middleware.go (3)
52-57: Prefer typed duration and consider IP anonymization
- Use zerolog's Dur for a machine-parseable field and fewer allocs.
- Optional: anonymize/mask client IPs to reduce PII exposure.
Apply this diff here:
- subLogger := log.With().Str("method", method). - Str("path", path). - Str("address", address). - Str("client_ip", clientIP). - Int("status", code). - Str("latency", latency).Logger() + subLogger := log.With().Str("method", method). + Str("path", path). + Str("address", address). + Str("client_ip", clientIP). + Int("status", code). + Dur("duration", time.Since(tStart)).Logger()And outside this hunk (Line 50), drop the latency string allocation:
// remove: latency := time.Since(tStart).String()Optional (outside this hunk): stabilize path cardinality to route templates:
p := c.FullPath() if p == "" { p = c.Request.URL.Path } path = pIf IP anonymization is desired, I can add a helper (e.g., zero last octets for IPv4 / prefix for IPv6) and a config toggle.
61-66: Include status text (and errors) for faster triageAdd HTTP status text as a field; optionally attach gin errors when present.
Apply this diff:
- case code >= 400 && code < 500: - subLogger.Warn().Msg("Client Error") + case code >= 400 && code < 500: + subLogger.Warn().Str("status_text", http.StatusText(code)).Msg("Client Error") - case code >= 500: - subLogger.Error().Msg("Server Error") + case code >= 500: + subLogger.Error().Str("status_text", http.StatusText(code)).Msg("Server Error") - default: - subLogger.Info().Msg("Request") + default: + subLogger.Info().Str("status_text", http.StatusText(code)).Msg("Request")Also add to imports (outside this hunk):
import ( "net/http" )Optional (outside this hunk):
if len(c.Errors) > 0 { subLogger = subLogger.With().Strs("errors", c.Errors.Errors()).Logger() }
69-69: Skip debug logs for skipped paths to cut noiseHealth/icon endpoints still emit debug logs; consider returning early.
Apply this diff:
- if m.logPath(method + " " + path) { - switch { + if !m.logPath(method + " " + path) { + return + } + switch { case code >= 400 && code < 500: ... - default: - subLogger.Info().Msg("Request") - } else { - subLogger.Debug().Msg("Request") - } + default: + subLogger.Info().Msg("Request") + }Dockerfile (1)
54-56: Mount point looks good; ensure runtime creates and owns /data.Without creating/chowning /data, writes can fail in restricted environments. Consider creating it at build time or on start.
FROM alpine:3.22 AS runner WORKDIR /tinyauth -RUN apk add --no-cache curl +RUN apk add --no-cache curl \ + && mkdir -p /data.env.example (1)
31-33: Fix dotenv-linter warnings: ordering and trailing newline.
- Move RESOURCES_DIR and DATABASE_PATH into alphabetical position (before SECURE_COOKIE, and before DISABLE_CONTINUE, respectively).
- Add a blank line at EOF.
Minimal fix for EOF newline:
-RESOURCES_DIR=/data/resources -DATABASE_PATH=/data/tinyauth.db +RESOURCES_DIR=/data/resources +DATABASE_PATH=/data/tinyauth.db +Optional: reorder keys to satisfy UnorderedKey hints.
docker-compose.dev.yml (1)
43-43: Beware host bind-mounts with SQLite on macOS/Windows; consider a named volume.Bind mounts can cause SQLite locking/perf issues on Docker Desktop. A named volume is safer for DB files.
- - ./data:/data + - tinyauth_data:/data + +volumes: + tinyauth_data:If you must keep a bind mount, verify local OS and test concurrent writes.
internal/model/session_model.go (1)
3-12: Naming consistency: TOTP/OAuth capitalization differs across code.Elsewhere (controller) fields use Totp/Oauth. It’s fine internally, but keep API/DB naming consistent to avoid confusion.
docker-compose.example.yml (1)
25-26: Use a named volume for portability and document DATABASE_PATH.Named volume avoids host FS quirks and aligns with Dockerfile VOLUME. Also consider showing DATABASE_PATH explicitly for clarity.
- volumes: - - ./data:/data + volumes: + - tinyauth_data:/data + environment: + - APP_URL=https://tinyauth.example.com + - USERS=user:$$2a$$10$$UdLYoJ5lgPsC0RKqYH/jMua7zIn0g9kPqWmhYayJYLaZQ/FTmH2/u # user:password + - DATABASE_PATH=/data/tinyauth.db + +volumes: + tinyauth_data:internal/config/config.go (1)
114-121: Prefer cookie to carry only the UUID to minimize PIIWith DB-backed sessions, consider serializing only UUID in the cookie and deriving the rest from DB. If any of Username/Email/Name/etc. are still serialized, mark them deprecated and phase them out to reduce attack surface.
go.mod (1)
9-9: Consolidate to a Single SQLite DriverYour code is currently pulling in both a CGO-based stack (mattn/go-sqlite3 via gorm.io/driver/sqlite and migrate’s sqlite3 driver) and a pure-Go stack (modernc.org/sqlite via github.com/glebarez/sqlite). This increases binary size and risks CGO-related issues. Pick one of the following:
• Option A – CGO only
– Use gorm.io/driver/sqlite (mattn/go-sqlite3) for GORM and migrate/v4/database/sqlite3
– Drop modernc.org/sqlite, glebarez/sqlite
– go.mod require block becomes:-require ( - gorm.io/driver/sqlite v1.6.0 - gorm.io/gorm v1.30.1 - modernc.org/sqlite v1.38.2 -) +require ( + gorm.io/driver/sqlite v1.6.0 + gorm.io/gorm v1.30.1 +) … -go.mod indirect deps: - github.com/glebarez/go-sqlite v1.21.2 // indirect - github.com/glebarez/sqlite v1.11.0 // indirect +… + github.com/mattn/go-sqlite3 v1.14.32 // indirect• Option B – Pure Go only
– Use github.com/glebarez/sqlite (modernc.org/sqlite) for GORM and migrate/v4/database/sqlite
– Drop gorm.io/driver/sqlite (mattn/go-sqlite3)
– go.mod require block becomes:-require ( - gorm.io/driver/sqlite v1.6.0 - gorm.io/gorm v1.30.1 -) +require ( + github.com/glebarez/sqlite v1.11.0 + gorm.io/gorm v1.30.1 +) … -go.mod indirect deps: - github.com/mattn/go-sqlite3 v1.14.32 // indirect +… + modernc.org/sqlite v1.38.2 // indirectUpdates to imports (if you choose Option B, for example):
internal/service/database_service.go -import ( - "github.com/glebarez/sqlite" +import ( + "github.com/glebarez/sqlite" … - sqliteMigrate "github.com/golang-migrate/migrate/v4/database/sqlite3" + sqliteMigrate "github.com/golang-migrate/migrate/v4/database/sqlite" … )Files to update:
- go.mod (lines 18–21, 28–39)
- internal/service/database_service.go (imports at lines 7–10)
internal/assets/migrations/000001_init_sqlite.up.sql (1)
1-10: Tighten schema: remove redundant UNIQUE and add index for expiry-based cleanup
- PRIMARY KEY is already unique; drop the extra UNIQUE.
- Add an index on expiry to speed up pruning of expired sessions.
- "uuid" TEXT NOT NULL PRIMARY KEY UNIQUE, + "uuid" TEXT NOT NULL PRIMARY KEY, @@ "expiry" INTEGER NOT NULL ); + +-- Speeds up deletion/lookup of expired sessions +CREATE INDEX IF NOT EXISTS "idx_sessions_expiry" ON "sessions" ("expiry");internal/assets/assets.go (2)
7-11: Add GoDoc for exported FrontendAssetsDocument exported identifiers to satisfy linters and improve discoverability.
-// Frontend -// +// FrontendAssets contains the embedded frontend build artifacts. //go:embed dist var FrontendAssets embed.FS
12-15: Add GoDoc for exported Migrations FSClarify purpose and source path of embedded SQL files.
-// Migrations -// +// Migrations contains the embedded SQL migration files (internal/assets/migrations/*.sql). //go:embed migrations/*.sql var Migrations embed.FSinternal/controller/proxy_controller.go (1)
92-112: Bypass path uses new IsBypassedIPRename looks correct; behavior unchanged.
Cache the basic password secret once to avoid duplicate lookups:
- if labels.Basic.Username != "" && utils.GetSecret(labels.Basic.Password.Plain, labels.Basic.Password.File) != "" { + secret := utils.GetSecret(labels.Basic.Password.Plain, labels.Basic.Password.File) + if labels.Basic.Username != "" && secret != "" { log.Debug().Str("username", labels.Basic.Username).Msg("Setting basic auth header") - c.Header("Authorization", fmt.Sprintf("Basic %s", utils.GetBasicAuth(labels.Basic.Username, utils.GetSecret(labels.Basic.Password.Plain, labels.Basic.Password.File)))) + c.Header("Authorization", fmt.Sprintf("Basic %s", utils.GetBasicAuth(labels.Basic.Username, secret))) }internal/service/database_service.go (3)
29-31: Ensure DB directory exists (first-run robustness).Creating /data may fail if the directory is missing.
+ // Ensure parent directory exists + if err := os.MkdirAll(filepath.Dir(ds.Config.DatabasePath), 0o755); err != nil { + return err + } - gormDB, err := gorm.Open(sqlite.Open(ds.Config.DatabasePath), &gorm.Config{}) + gormDB, err := gorm.Open(sqlite.Open(ds.Config.DatabasePath), &gorm.Config{})Add imports:
+ "os" + "path/filepath"
65-71: Close migrator to release resources.Not closing can leak handles.
- migrator, err := migrate.NewWithInstance("iofs", data, "tinyauth", target) + migrator, err := migrate.NewWithInstance("iofs", data, "tinyauth", target) if err != nil { return err } - return migrator.Up() + defer func() { _ = migrator.Close() }() + return migrator.Up()
29-31: Optional: enable WAL + busy timeout via DSN.Improves read concurrency and reduces lock errors.
- gormDB, err := gorm.Open(sqlite.Open(ds.Config.DatabasePath), &gorm.Config{}) + dsn := fmt.Sprintf("file:%s?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)&cache=shared", ds.Config.DatabasePath) + gormDB, err := gorm.Open(sqlite.Open(dsn), &gorm.Config{})Add import:
+ "fmt"internal/service/auth_service.go (5)
188-201: Avoid shadowing the uuid package; tiny readability win.Rename the local var to prevent package shadowing.
- uuid, err := uuid.NewRandom() + sid, err := uuid.NewRandom() ... - UUID: uuid.String(), + UUID: sid.String(),
219-220: Cookie domain handling can break on localhost/IP; don’t force a leading dot.Use the configured domain as-is (trim any leading dot) or leave empty to set a host-only cookie.
- c.SetCookie(auth.Config.SessionCookieName, session.UUID, expiry, "/", fmt.Sprintf(".%s", auth.Config.Domain), auth.Config.SecureCookie, true) + domain := strings.TrimPrefix(auth.Config.Domain, ".") + c.SetCookie(auth.Config.SessionCookieName, session.UUID, expiry, "/", domain, auth.Config.SecureCookie, true)
249-259: Tighten not-found handling; optionally clear stale cookie.First check ErrRecordNotFound/RowsAffected, then clear cookie to stop repeated DB hits.
- res := auth.Database.Unscoped().Where("uuid = ?", cookie).First(&session) - if res.Error != nil { - return config.SessionCookie{}, res.Error - } - if res.RowsAffected == 0 { - return config.SessionCookie{}, fmt.Errorf("session not found") - } + res := auth.Database.Unscoped().Where("uuid = ?", cookie).First(&session) + if errors.Is(res.Error, gorm.ErrRecordNotFound) || res.RowsAffected == 0 { + domain := strings.TrimPrefix(auth.Config.Domain, ".") + c.SetCookie(auth.Config.SessionCookieName, "", -1, "/", domain, auth.Config.SecureCookie, true) + return config.SessionCookie{}, fmt.Errorf("session not found") + } + if res.Error != nil { + return config.SessionCookie{}, res.Error + }Add import if needed:
+ "errors"
261-269: Clear expired cookie after purging DB record.Prevents clients from repeatedly sending an expired cookie.
if currentTime > session.Expiry { - res := auth.Database.Unscoped().Where("uuid = ?", session.UUID).Delete(&model.Session{}) + res := auth.Database.Unscoped().Where("uuid = ?", session.UUID).Delete(&model.Session{}) if res.Error != nil { log.Error().Err(res.Error).Msg("Failed to delete expired session") } + domain := strings.TrimPrefix(auth.Config.Domain, ".") + c.SetCookie(auth.Config.SessionCookieName, "", -1, "/", domain, auth.Config.SecureCookie, true) return config.SessionCookie{}, fmt.Errorf("session expired") }
187-221: Consider SameSite on auth cookies.Gin’s helper can’t set SameSite. If CSRF is a concern, switch to http.SetCookie with SameSite=Lax/Strict.
📜 Review details
Configuration used: CodeRabbit UI
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 ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.env.example(1 hunks)Dockerfile(1 hunks)cmd/root.go(1 hunks)docker-compose.dev.yml(1 hunks)docker-compose.example.yml(1 hunks)go.mod(3 hunks)internal/assets/assets.go(1 hunks)internal/assets/migrations/000001_init_sqlite.down.sql(1 hunks)internal/assets/migrations/000001_init_sqlite.up.sql(1 hunks)internal/bootstrap/app_bootstrap.go(1 hunks)internal/config/config.go(2 hunks)internal/controller/oauth_controller.go(2 hunks)internal/controller/proxy_controller.go(4 hunks)internal/middleware/context_middleware.go(1 hunks)internal/middleware/zerolog_middleware.go(1 hunks)internal/model/session_model.go(1 hunks)internal/service/auth_service.go(6 hunks)internal/service/database_service.go(1 hunks)internal/utils/security_utils.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/utils/security_utils.go
🧰 Additional context used
🧬 Code graph analysis (5)
internal/model/session_model.go (1)
internal/controller/context_controller.go (1)
Status(10-20)
internal/service/database_service.go (2)
internal/config/config.go (1)
Config(18-58)internal/assets/assets.go (1)
Migrations(15-15)
internal/controller/oauth_controller.go (2)
internal/service/github_oauth_service.go (1)
internal/service/google_oauth_service.go (1)
internal/bootstrap/app_bootstrap.go (4)
internal/service/database_service.go (2)
NewDatabaseService(23-27)DatabaseServiceConfig(14-16)internal/config/config.go (1)
Config(18-58)internal/service/docker_service.go (1)
NewDockerService(21-23)internal/service/auth_service.go (1)
NewAuthService(46-54)
internal/service/auth_service.go (4)
internal/config/config.go (3)
Config(18-58)SessionCookie(113-121)UserContext(123-133)internal/utils/security_utils.go (1)
CheckFilter(71-96)internal/model/session_model.go (1)
Session(3-12)internal/controller/user_controller.go (1)
controller(163-172)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 31-31: [UnorderedKey] The GENERIC_SKIP_SSL key should go before the GENERIC_TOKEN_URL key
(UnorderedKey)
[warning] 32-32: [UnorderedKey] The RESOURCES_DIR key should go before the SECURE_COOKIE key
(UnorderedKey)
[warning] 33-33: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 33-33: [UnorderedKey] The DATABASE_PATH key should go before the DISABLE_CONTINUE key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
internal/config/config.go (1)
57-57: DatabasePath looks good; ensure secure file creation and parent dir handlingField and tags align with the CLI flag. Please confirm the DB initializer creates the parent directory (if missing) and opens the DB with restrictive perms (e.g., 0600) to avoid leaking session data on shared volumes.
cmd/root.go (1)
115-115: New --database-path flag matches config key and env (DATABASE_PATH) — LGTMNice addition. Ensure docs/examples mention mounting /data and writable perms for the path.
internal/assets/migrations/000001_init_sqlite.down.sql (1)
1-1: Down migration — LGTMMatches the up migration scope.
internal/middleware/context_middleware.go (1)
86-90: Renaming Verified: No StaleEmailWhitelistedReferencesAll instances of the old method name have been replaced with
IsEmailWhitelisted, and no occurrences ofEmailWhitelisted(remain in the codebase. Changes are safe to merge.internal/controller/oauth_controller.go (1)
147-160: Rename alignment: EmailWhitelisted → IsEmailWhitelistedMatches the new public API; flow remains correct.
internal/controller/proxy_controller.go (3)
138-153: Auth enablement check now returns (bool, error)Error path handled correctly with appropriate response per client type.
198-229: Resource authorization rename to IsResourceAllowedSignature/usage aligns; log and redirect logic intact.
232-265: OAuth group check rename to IsInOAuthGroupRename applied correctly; unauthorized handling preserved.
internal/bootstrap/app_bootstrap.go (2)
112-113: Inject DB into AuthServiceConstructor updated correctly to include the DB handle.
95-110: Config validation for DatabasePath confirmedThe
DatabasePathfield in the application configuration is already marked withvalidate:"required", ensuring that it cannot be empty at startup.Key location:
- internal/config/config.go:57
DatabasePath string `mapstructure:"database-path" validate:"required"`No further changes needed—approving these code updates.
internal/service/auth_service.go (2)
43-53: DB dependency injection looks good.Constructor now requires a DB and stores it; simple and clear.
183-186: API renames read well; behavior parity retained.Whitelist/resource/group/bypass checks mirror prior semantics.
Also applies to: 286-295, 296-317, 383-399
|
lgtm |
1 participant
Summary by CodeRabbit
New Features
Changes
Documentation