feat: dashboard auth subsystem — token hierarchy + session cookies (AD-2)#17
Conversation
Create api_tokens table for client token authentication with bcrypt hash storage, prefix-based lookup index, and usage tracking columns.
GORM-based store with Create, List, FindByPrefix, Revoke, IncrementStats, IncrementErrorCount, GetByID, and BatchIncrementStats methods. APIToken model added to models.go.
Handlers for POST /api/auth/login (master token validation + HMAC cookie), POST /api/auth/logout (cookie clear), GET /api/auth/me (status check), GET/POST /api/auth/tokens (list/create), DELETE /api/auth/tokens/:id (revoke). All handlers include swaggo annotations.
TokenAuth now supports three auth methods: master token (admin), client API tokens (eng_* prefix with bcrypt verification and scope enforcement), and HMAC-SHA256 signed session cookies (dashboard). Read-only tokens are restricted to GET + whitelisted POST endpoints. HMAC cookie key derived deterministically from master token via SHA-256.
Add public routes (login, logout) and authenticated routes (me, tokens CRUD) to setupRoutes(). Create TokenStore in initializeAsync() and wire it into TokenAuth middleware after DB initialization.
Background goroutine reads token IDs from a buffered channel and flushes accumulated request counts to the database every 5 seconds, reducing per-request UPDATE overhead for client token authentication.
WalkthroughДобавлена система управления API-токенами с поддержкой аутентификации на основе токенов и сессий. Включены миграция БД, модель данных, хранилище токенов, HTTP-обработчики и фоновый сборщик статистики использования токенов. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as Middleware
participant TokenStore as TokenStore
participant Database as Database
participant Handler as Handler
Client->>Middleware: POST /api/auth/tokens (create token)
Middleware->>Handler: authenticateClientToken (validate master token)
Handler->>Handler: Generate random token + bcrypt hash
Handler->>TokenStore: Create(name, hash, prefix, scope)
TokenStore->>Database: INSERT INTO api_tokens
Database-->>TokenStore: token created
TokenStore-->>Handler: APIToken object
Handler-->>Client: 200 OK (id, name, raw_token, scope)
Client->>Middleware: GET /api/auth/tokens (list tokens)
Middleware->>Handler: validateMasterToken
Handler->>TokenStore: List()
TokenStore->>Database: SELECT * FROM api_tokens
Database-->>TokenStore: tokens list
TokenStore-->>Handler: []APIToken (without hashes)
Handler-->>Client: 200 OK (token list)
Client->>Middleware: Request with client token header
Middleware->>Middleware: Extract token prefix
Middleware->>TokenStore: FindByPrefix(prefix)
TokenStore->>Database: SELECT FROM api_tokens WHERE token_prefix = ?
Database-->>TokenStore: token found
Middleware->>Middleware: bcrypt verify token hash
Middleware->>Middleware: Check token not revoked
Middleware->>Middleware: Enforce read-only scope if needed
Middleware->>Handler: Set context with token scope
Handler-->>Client: Request processed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the dashboard's security and access control by implementing a robust token hierarchy authentication system. It provides granular control over API access through client tokens with defined scopes, alongside a secure session management mechanism for administrative users. The changes lay the groundwork for a more scalable and manageable authentication infrastructure, improving both security and performance by optimizing database interactions for token usage tracking. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive token-based authentication system, including a token hierarchy with master and client tokens, and session management using HMAC-signed cookies. The implementation is solid, with good use of security best practices like constant-time comparisons for tokens, bcrypt for hashing, and secure cookie attributes. The addition of a buffered stats flusher is a great performance optimization. I've identified a few areas for improvement related to error handling robustness, code consistency, and efficiency.
| // Extract prefix: chars 4-12 (first 8 hex chars after "eng_") | ||
| if len(rawToken) < 12 { | ||
| http.Error(w, "unauthorized", http.StatusUnauthorized) | ||
| return true | ||
| } | ||
| prefix := rawToken[4:12] |
There was a problem hiding this comment.
The token prefix extraction logic in authenticateClientToken uses magic numbers (e.g., len(rawToken) < 12, rawToken[4:12]). This makes the code harder to maintain and understand. The constants tokenRawPrefix and tokenPrefixLen are already defined in the worker package (in handlers_auth.go) and should be used here for consistency and clarity.
| // Extract prefix: chars 4-12 (first 8 hex chars after "eng_") | |
| if len(rawToken) < 12 { | |
| http.Error(w, "unauthorized", http.StatusUnauthorized) | |
| return true | |
| } | |
| prefix := rawToken[4:12] | |
| // Extract prefix: first 8 hex chars after "eng_" | |
| if len(rawToken) < len(tokenRawPrefix)+tokenPrefixLen { | |
| http.Error(w, "unauthorized", http.StatusUnauthorized) | |
| return true | |
| } | |
| prefix := rawToken[len(tokenRawPrefix) : len(tokenRawPrefix)+tokenPrefixLen] |
| if err := tokenStore.Revoke(r.Context(), id); err != nil { | ||
| log.Error().Err(err).Str("token_id", id).Msg("auth: failed to revoke token") | ||
| http.Error(w, "not found", http.StatusNotFound) | ||
| return | ||
| } |
There was a problem hiding this comment.
The error handling for tokenStore.Revoke is not specific enough. It treats all errors, including potential database connection issues, as a "not found" error and returns a 404 status. This can mask underlying problems. It's better to specifically check for gorm.ErrRecordNotFound to return a 404, and return a 500 Internal Server Error for all other errors.
You'll need to add import ("errors"; "gorm.io/gorm") to this file.
| if err := tokenStore.Revoke(r.Context(), id); err != nil { | |
| log.Error().Err(err).Str("token_id", id).Msg("auth: failed to revoke token") | |
| http.Error(w, "not found", http.StatusNotFound) | |
| return | |
| } | |
| if err := tokenStore.Revoke(r.Context(), id); err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| http.Error(w, "token not found", http.StatusNotFound) | |
| return | |
| } | |
| log.Error().Err(err).Str("token_id", id).Msg("auth: failed to revoke token") | |
| http.Error(w, "internal error", http.StatusInternalServerError) | |
| return | |
| } |
| // isDuplicateKeyError checks if the error is a unique constraint violation. | ||
| func isDuplicateKeyError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
| msg := err.Error() | ||
| // PostgreSQL unique violation error code 23505 | ||
| return containsDuplicateKey(msg) | ||
| } | ||
|
|
||
| // containsDuplicateKey checks error message for duplicate key indicators. | ||
| func containsDuplicateKey(msg string) bool { | ||
| for _, s := range []string{"duplicate key", "23505", "UNIQUE constraint"} { | ||
| if len(msg) >= len(s) { | ||
| for i := 0; i <= len(msg)-len(s); i++ { | ||
| if msg[i:i+len(s)] == s { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The current implementation for detecting duplicate key errors relies on string matching, which is fragile and can break with database or driver updates. A more robust approach is to type-assert the error to *pq.Error and check for the specific SQLSTATE code 23505. This makes the error handling independent of the error message string. Additionally, the containsDuplicateKey function is an inefficient, manual implementation of strings.Contains.
This suggestion replaces both isDuplicateKeyError and containsDuplicateKey with a single, more robust function. You'll need to add import ("errors"; "strings"; "github.com/lib/pq") to this file.
// isDuplicateKeyError checks if the error is a unique constraint violation.
func isDuplicateKeyError(err error) bool {
if err == nil {
return false
}
// Check for PostgreSQL unique violation error code 23505
var pqErr *pq.Error
if errors.As(err, &pqErr) {
return pqErr.Code == "23505"
}
// Fallback for other DB drivers or error types
msg := err.Error()
return strings.Contains(msg, "UNIQUE constraint") || strings.Contains(msg, "duplicate key")
}| for k := range pending { | ||
| delete(pending, k) | ||
| } |
There was a problem hiding this comment.
The pending map is cleared by iterating over all its keys. A more idiomatic and efficient way to clear a map in Go is to create a new one, which avoids the loop and lets the garbage collector handle the old map's memory.
pending = make(map[string]int)![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
🧹 Nitpick comments (1)
internal/worker/handlers_auth.go (1)
358-370: Рекомендуется использоватьstrings.Containsвместо ручного поиска подстроки.Текущая реализация вручную итерирует по строке для поиска подстроки. Стандартная библиотека Go предоставляет
strings.Containsдля этой цели.♻️ Предлагаемый рефакторинг
+import "strings" + // containsDuplicateKey checks error message for duplicate key indicators. func containsDuplicateKey(msg string) bool { - for _, s := range []string{"duplicate key", "23505", "UNIQUE constraint"} { - if len(msg) >= len(s) { - for i := 0; i <= len(msg)-len(s); i++ { - if msg[i:i+len(s)] == s { - return true - } - } - } - } - return false + return strings.Contains(msg, "duplicate key") || + strings.Contains(msg, "23505") || + strings.Contains(msg, "UNIQUE constraint") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_auth.go` around lines 358 - 370, The containsDuplicateKey function manually searches substrings which is error-prone and inefficient; replace the manual loop with calls to strings.Contains for each indicator string. Import the "strings" package if not already present, then in containsDuplicateKey iterate the slice {"duplicate key","23505","UNIQUE constraint"} and return true when strings.Contains(msg, s) is true, otherwise return false. Ensure function name containsDuplicateKey remains unchanged and no other behavior is modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/db/gorm/token_store.go`:
- Around line 63-77: In TokenStore.Revoke, the code currently checks
result.RowsAffected before result.Error which can hide real DB errors; change
the logic to first inspect result.Error and return it if non-nil, then check
result.RowsAffected == 0 and return gorm.ErrRecordNotFound; keep the same update
payload (APIToken fields "revoked" and "revoked_at") and context usage
(s.db.WithContext(ctx)) but swap the error/rows-affected checks so the real DB
error from result.Error is propagated.
In `@internal/worker/handlers_auth.go`:
- Around line 118-125: The logout cookie currently cleared via the
http.SetCookie call for sessionCookieName is missing the Secure flag; update
that http.SetCookie invocation (the block that sets Name: sessionCookieName,
Value: "" etc.) to include Secure: true so the cookie is only sent over HTTPS
(matching the login cookie behavior).
- Around line 95-102: The session cookie is being set without the Secure flag
which allows transmission over plain HTTP; update the http.SetCookie call that
constructs the cookie (the block using Name: sessionCookieName, Value:
cookieValue, Path: "/", MaxAge: sessionMaxAge, HttpOnly: true, SameSite:
http.SameSiteStrictMode) to include a Secure field whose value is derived from
runtime configuration or an environment variable (e.g., isProd or
cfg.IsProduction) so Secure is true in production and false in local dev; ensure
the chosen config symbol is checked where the handler runs and used to set
Secure accordingly.
In `@internal/worker/middleware.go`:
- Around line 236-249: The extra unreachable return after calling
authenticateClientToken should be removed: in the client-token branch (where
providedToken is checked for the "eng_" prefix and store != nil) call
ta.authenticateClientToken(w, r, next, providedToken, store) and then
immediately return only once (or simply rely on authenticateClientToken to
handle responses and stop execution) — delete the redundant second return so the
block contains a single return after the authenticateClientToken invocation and
no unreachable statement remains.
---
Nitpick comments:
In `@internal/worker/handlers_auth.go`:
- Around line 358-370: The containsDuplicateKey function manually searches
substrings which is error-prone and inefficient; replace the manual loop with
calls to strings.Contains for each indicator string. Import the "strings"
package if not already present, then in containsDuplicateKey iterate the slice
{"duplicate key","23505","UNIQUE constraint"} and return true when
strings.Contains(msg, s) is true, otherwise return false. Ensure function name
containsDuplicateKey remains unchanged and no other behavior is modified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5fdff9a-7ea5-4586-99b3-a5131a1ed7c1
📒 Files selected for processing (7)
internal/db/gorm/migrations.gointernal/db/gorm/models.gointernal/db/gorm/token_store.gointernal/worker/handlers_auth.gointernal/worker/middleware.gointernal/worker/service.gointernal/worker/token_stats.go
| // Revoke marks a token as revoked. | ||
| func (s *TokenStore) Revoke(ctx context.Context, id string) error { | ||
| now := time.Now() | ||
| result := s.db.WithContext(ctx). | ||
| Model(&APIToken{}). | ||
| Where("id = ?", id). | ||
| Updates(map[string]interface{}{ | ||
| "revoked": true, | ||
| "revoked_at": &now, | ||
| }) | ||
| if result.RowsAffected == 0 { | ||
| return gorm.ErrRecordNotFound | ||
| } | ||
| return result.Error | ||
| } |
There was a problem hiding this comment.
Потенциальная проблема: проверка ошибки после RowsAffected.
Если result.Error не nil, метод вернёт gorm.ErrRecordNotFound, скрывая реальную ошибку БД. Следует сначала проверить result.Error.
🐛 Предлагаемое исправление
func (s *TokenStore) Revoke(ctx context.Context, id string) error {
now := time.Now()
result := s.db.WithContext(ctx).
Model(&APIToken{}).
Where("id = ?", id).
Updates(map[string]interface{}{
"revoked": true,
"revoked_at": &now,
})
+ if result.Error != nil {
+ return result.Error
+ }
if result.RowsAffected == 0 {
return gorm.ErrRecordNotFound
}
- return result.Error
+ return nil
}📝 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.
| // Revoke marks a token as revoked. | |
| func (s *TokenStore) Revoke(ctx context.Context, id string) error { | |
| now := time.Now() | |
| result := s.db.WithContext(ctx). | |
| Model(&APIToken{}). | |
| Where("id = ?", id). | |
| Updates(map[string]interface{}{ | |
| "revoked": true, | |
| "revoked_at": &now, | |
| }) | |
| if result.RowsAffected == 0 { | |
| return gorm.ErrRecordNotFound | |
| } | |
| return result.Error | |
| } | |
| // Revoke marks a token as revoked. | |
| func (s *TokenStore) Revoke(ctx context.Context, id string) error { | |
| now := time.Now() | |
| result := s.db.WithContext(ctx). | |
| Model(&APIToken{}). | |
| Where("id = ?", id). | |
| Updates(map[string]interface{}{ | |
| "revoked": true, | |
| "revoked_at": &now, | |
| }) | |
| if result.Error != nil { | |
| return result.Error | |
| } | |
| if result.RowsAffected == 0 { | |
| return gorm.ErrRecordNotFound | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/db/gorm/token_store.go` around lines 63 - 77, In TokenStore.Revoke,
the code currently checks result.RowsAffected before result.Error which can hide
real DB errors; change the logic to first inspect result.Error and return it if
non-nil, then check result.RowsAffected == 0 and return gorm.ErrRecordNotFound;
keep the same update payload (APIToken fields "revoked" and "revoked_at") and
context usage (s.db.WithContext(ctx)) but swap the error/rows-affected checks so
the real DB error from result.Error is propagated.
| http.SetCookie(w, &http.Cookie{ | ||
| Name: sessionCookieName, | ||
| Value: cookieValue, | ||
| Path: "/", | ||
| MaxAge: sessionMaxAge, | ||
| HttpOnly: true, | ||
| SameSite: http.SameSiteStrictMode, | ||
| }) |
There was a problem hiding this comment.
Отсутствует флаг Secure для production-окружения.
Сессионная cookie не имеет флага Secure: true, что позволит передачу cookie через HTTP. В production это риск перехвата сессии.
🔒 Предлагаемое исправление
http.SetCookie(w, &http.Cookie{
Name: sessionCookieName,
Value: cookieValue,
Path: "/",
MaxAge: sessionMaxAge,
HttpOnly: true,
+ Secure: true, // Set to false only for local development
SameSite: http.SameSiteStrictMode,
})Рассмотрите возможность определения флага Secure на основе конфигурации или переменной окружения для различения dev/prod.
📝 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.
| http.SetCookie(w, &http.Cookie{ | |
| Name: sessionCookieName, | |
| Value: cookieValue, | |
| Path: "/", | |
| MaxAge: sessionMaxAge, | |
| HttpOnly: true, | |
| SameSite: http.SameSiteStrictMode, | |
| }) | |
| http.SetCookie(w, &http.Cookie{ | |
| Name: sessionCookieName, | |
| Value: cookieValue, | |
| Path: "/", | |
| MaxAge: sessionMaxAge, | |
| HttpOnly: true, | |
| Secure: true, // Set to false only for local development | |
| SameSite: http.SameSiteStrictMode, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/worker/handlers_auth.go` around lines 95 - 102, The session cookie
is being set without the Secure flag which allows transmission over plain HTTP;
update the http.SetCookie call that constructs the cookie (the block using Name:
sessionCookieName, Value: cookieValue, Path: "/", MaxAge: sessionMaxAge,
HttpOnly: true, SameSite: http.SameSiteStrictMode) to include a Secure field
whose value is derived from runtime configuration or an environment variable
(e.g., isProd or cfg.IsProduction) so Secure is true in production and false in
local dev; ensure the chosen config symbol is checked where the handler runs and
used to set Secure accordingly.
| http.SetCookie(w, &http.Cookie{ | ||
| Name: sessionCookieName, | ||
| Value: "", | ||
| Path: "/", | ||
| MaxAge: -1, | ||
| HttpOnly: true, | ||
| SameSite: http.SameSiteStrictMode, | ||
| }) |
There was a problem hiding this comment.
Отсутствует флаг Secure в cookie logout.
Аналогично cookie при login, здесь также следует добавить Secure: true.
🔒 Предлагаемое исправление
http.SetCookie(w, &http.Cookie{
Name: sessionCookieName,
Value: "",
Path: "/",
MaxAge: -1,
HttpOnly: true,
+ Secure: true,
SameSite: http.SameSiteStrictMode,
})📝 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.
| http.SetCookie(w, &http.Cookie{ | |
| Name: sessionCookieName, | |
| Value: "", | |
| Path: "/", | |
| MaxAge: -1, | |
| HttpOnly: true, | |
| SameSite: http.SameSiteStrictMode, | |
| }) | |
| http.SetCookie(w, &http.Cookie{ | |
| Name: sessionCookieName, | |
| Value: "", | |
| Path: "/", | |
| MaxAge: -1, | |
| HttpOnly: true, | |
| Secure: true, | |
| SameSite: http.SameSiteStrictMode, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/worker/handlers_auth.go` around lines 118 - 125, The logout cookie
currently cleared via the http.SetCookie call for sessionCookieName is missing
the Secure flag; update that http.SetCookie invocation (the block that sets
Name: sessionCookieName, Value: "" etc.) to include Secure: true so the cookie
is only sent over HTTPS (matching the login cookie behavior).
| // 1b. Check if it's a client token (eng_* prefix) | ||
| if strings.HasPrefix(providedToken, "eng_") && store != nil { | ||
| if ta.authenticateClientToken(w, r, next, providedToken, store) { | ||
| return | ||
| } | ||
| // authenticateClientToken wrote the error response if it failed | ||
| return | ||
| } | ||
|
|
||
| // Token provided but doesn't match anything | ||
| log.Warn().Str("path", r.URL.Path).Str("remote_addr", r.RemoteAddr).Msg("auth: rejected request with invalid token") | ||
| http.Error(w, "unauthorized", http.StatusUnauthorized) | ||
| return | ||
| } |
There was a problem hiding this comment.
Избыточный return после вызова authenticateClientToken.
Строки 241-243 содержат избыточный return. Метод authenticateClientToken всегда возвращает true и сам обрабатывает как успешные, так и неуспешные случаи. Второй return на строке 243 недостижим.
🧹 Предлагаемое исправление
// 1b. Check if it's a client token (eng_* prefix)
if strings.HasPrefix(providedToken, "eng_") && store != nil {
- if ta.authenticateClientToken(w, r, next, providedToken, store) {
- return
- }
- // authenticateClientToken wrote the error response if it failed
+ ta.authenticateClientToken(w, r, next, providedToken, store)
return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/worker/middleware.go` around lines 236 - 249, The extra unreachable
return after calling authenticateClientToken should be removed: in the
client-token branch (where providedToken is checked for the "eng_" prefix and
store != nil) call ta.authenticateClientToken(w, r, next, providedToken, store)
and then immediately return only once (or simply rely on authenticateClientToken
to handle responses and stop execution) — delete the redundant second return so
the block contains a single return after the authenticateClientToken invocation
and no unreachable statement remains.
1 participant
Summary
Implements the token hierarchy auth system for the Engram Dashboard (Phase 1 of dashboard v1.0):
api_tokenstable with UUID PK, bcrypt hash, prefix index, usage statseng_*+ bcrypt), HMAC-SHA256 session cookieeng_{32-hex}— shown once at creation, bcrypt hash storedArchitecture Decisions
Test plan
POST /api/auth/loginwith master token → returns cookiePOST /api/auth/tokens→ returnseng_*raw token (shown once)go build ./cmd/worker/compilesSummary by CodeRabbit
Примечания к выпуску
Новые функции
Миграции базы данных