Conversation
…pe and permission checks
…dd Redis configuration support
…d update related migrations
…support and integrate into session management
|
Caution Review failedThe pull request is closed. WalkthroughAdds Redis configuration and Docker service; implements a global Redis client plus ServiceCache and TokenRevocationCache; wires caches into session and auth services and middleware; adds domain support for services with DB migrations and cache invalidation; removes unique user email constraint and deletes some migration/session tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as Auth Middleware
participant RepoAdapter as ServiceRepo Adapter
participant Cache as ServiceCache (Redis)
participant DB as Database
participant Redis as Redis
participant Handler as Route Handler
Client->>Middleware: Request + JWT + Origin/Referer
activate Middleware
Middleware->>Middleware: extract domain from Origin/Referer
Middleware->>RepoAdapter: FindByDomain(domain)
activate RepoAdapter
RepoAdapter->>Cache: GetByDomain(domain)
activate Cache
Cache->>Redis: GET service:domain:<domain>
activate Redis
alt cache hit
Redis-->>Cache: service JSON
else cache miss
Redis-->>Cache: nil
Cache->>DB: repo.FindByDomain(domain)
activate DB
DB-->>Cache: service record
Cache->>Redis: SET service:domain:<domain> (TTL=1h)
Redis-->>Cache: OK
deactivate DB
end
deactivate Redis
Cache-->>RepoAdapter: ServiceInfo
deactivate Cache
RepoAdapter-->>Middleware: ServiceInfo (code, active)
deactivate RepoAdapter
Middleware->>Middleware: verify issuer/exp and aud matches ServiceInfo.Code
Middleware->>Middleware: extract scopes -> store in context
alt valid + active
Middleware-->>Handler: proceed with Identity + Scopes
else
Middleware-->>Client: 401/403
end
deactivate Middleware
Handler->>Handler: handle request
Handler-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @zeedivx. * #13 (comment) The following files were modified: * `internal/cache/redis.go` * `internal/cache/service_cache.go` * `internal/domain/auth/middleware.go` * `internal/domain/auth/service_adapter.go` * `internal/domain/permission/helpers.go` * `internal/domain/permission/model.go` * `internal/domain/service/service.go` * `internal/domain/session/service.go` * `internal/server/router.go`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (16)
internal/domain/auth/model.go (1)
129-135: NewScopesfield onIdentityaligns withAccessTokenClaims.GetScopesAdding
Scopes map[string]uint64toIdentitymatches the existingGetScopes()helper and will make downstream authorization checks easier. Just ensure every place constructing anIdentityinitializesScopesto a non-nil map (ideally viaclaims.GetScopes()) so future code that writes into the map doesn’t risk nil-map panics.internal/server/http.go (1)
7-12: Redis initialization and error handling look appropriateWiring
cache.ConnectRedis(&cfg.Redis)right after a successful DB connection, with structured logging and fail-fast behavior on error, is a good fit for the new Redis-backed caches. Longer term, you might consider adding symmetric cleanup (closing DB/Redis clients on shutdown), but that’s not a blocker for this change.Also applies to: 20-30
go.mod (1)
20-20: Moveredis/go-redis/v9to direct dependencies withgo mod tidyThe
go-redis/v9library is directly imported ininternal/cache/redis.gobut currently marked as indirect in go.mod. Runninggo mod tidywill move it to the direct dependency section, while the transitive dependencies (xxhashandgo-rendezvous) will correctly remain indirect.config.yaml (1)
24-28: Redis config defaults are fine for local; document production expectationsThe new
redisblock matches the Go config struct and is sensible for local dev, but it would help to explicitly document/ensure that production deployments overridepassword(and possibly host/port) via env‑specific config, so no one accidentally runs Redis in prod without auth.internal/migrations/000009_add_domain_to_services.up.sql (1)
1-2: Domain column and partial unique index look correct; confirm case/normalization strategyThe
domaincolumn +UNIQUE INDEX ... WHERE domain IS NOT NULLis a clean way to enforce uniqueness only when a domain is set. Just ensure the rest of the stack (e.g., origin parsing and service creation) consistently normalizes domains (case, trailing dots, etc.) to avoid “duplicate” domains slipping through with different representations.internal/domain/session/repository.go (1)
13-17: Clarify semantics of FindByID vs FindByIDForRevoke
FindByIDForRevokecorrectly drops therevoked = falsefilter so revocation logic can see all sessions. To avoid accidental misuse later, consider adding brief comments on both interface methods describing when each should be used (normal lookups vs admin/revocation flows).Also applies to: 40-47
internal/server/router.go (1)
7-7: Redis‑backed caches are wired correctly; document/guard Redis dependency and issuer/domain assumptionsThe new wiring (service cache, token‑revocation cache, cache‑aware session/auth services, auth service repository adapter) is coherent, but there are two subtle points worth tightening:
- Ensure
SetupRoutesis only called after Redis has been successfully initialized (or thatServiceCache/TokenRevocationCachegracefully handle a nil/absent Redis client), otherwise tests or alternative entrypoints that skipConnectRedisrisk runtime issues.- With domain‑based service resolution in the auth middleware, double‑check that the
issuervalue you pass (cfg.App.Name) matches your token issuer and service/domain validation strategy, so issuer checks don’t accidentally conflict with the new domain logic.Both are mostly about clarifying invariants; behavior here otherwise looks good.
Also applies to: 33-36, 38-38, 57-57, 66-67, 73-73
internal/domain/session/service.go (1)
148-178: Revoke uses best‑effort cache writes; consider whether that’s acceptable for your threat modelThe DB revocation happens first, and Redis revocation is best‑effort (errors are logged and ignored). That’s safer than failing the whole operation on cache issues, but it means access‑token revocation checks that rely only on Redis can silently fall back to “not revoked” if Redis is unavailable or
RevokeSessionfails. If revocation is security‑critical rather than best‑effort, consider either surfacing cache errors to the caller or treating cache failures as a hard failure so callers can choose to fail closed.internal/domain/auth/service.go (1)
188-209: Tighten IsTokenRevoked behavior for missing SID and cache failuresCurrently:
- No
claimsnil check (would panic if ever called with nil).- Missing
sidor cache errors both result in “not revoked”.- Cache unavailability is silently tolerated (just logged).
If you expect all valid access tokens to carry a
sid, it’s safer to treat a missingsid(or malformed claims) as invalid rather than “not revoked”. Likewise, you may want to distinguish “revocation check failed” from “not revoked” so callers/middleware can decide whether to fail closed when Redis is down.internal/domain/service/handler.go (1)
19-24: Domain propagation through service HTTP handlers looks correct
domainis cleanly threaded into Create/Update, andErrServiceDomainExistsis correctly surfaced as HTTP 409 on create. If you later add domain‑uniqueness checks to the update path (e.g. returningErrServiceDomainExistsfromUpdate), remember to extend this handler’s error mapping similarly so clients see a 409 instead of a generic 500.Also applies to: 37-44, 128-133, 139-140
internal/domain/auth/service_adapter.go (1)
10-41: Service adapter is straightforward; consider context and nil‑safetyThe adapter cleanly maps
svc.Serviceto theServiceInfo/ServiceRepositoryview, and delegating toServiceCache.GetByDomainkeeps cache policy out of middleware. Two minor follow‑ups you might consider:
- Guard against a nil
cachebeing passed toNewServiceRepositoryAdapter, which would panic on use.- Long‑term, if you need request‑scoped cancellation, you may want
FindByDomainto accept acontext.Contextinstead of usingcontext.Background()here.internal/domain/service/service.go (1)
124-166: Consider surfacing domain conflicts on update and invalidating cache on deleteThe partial‑update logic and domain cache invalidation in
Updateare generally good, but there are two behavioral gaps:
- If a domain change on update violates a unique constraint, the DB error currently bubbles up as a generic error; clients will see a 500. For parity with create, you may want to detect that condition and return
ErrServiceDomainExistsso the handler can translate it to HTTP 409.- You invalidate domain cache entries on update but not on delete. If a service is deleted while a domain‑based entry is cached, callers using the cached adapter may keep seeing an active service until the cache TTL expires. Consider invalidating by domain in
Deleteas well (which may require looking up the service/domain before deleting).Also applies to: 168-171
internal/cache/service_cache.go (2)
45-56: Distinguish cache miss from Redis errors.The code treats any Redis
Geterror as a cache miss. A genuine Redis failure (connection timeout, network issue) will silently fall through to the database, masking operational problems.// Try to get from Redis cache cached, err := RedisClient.Get(ctx, cacheKey).Result() - if err == nil { + if err != nil && err != redis.ErrNil { + slog.Warn("Redis cache error, falling back to database", "domain", domain, "error", err) + } else if err == nil { var info cachedServiceInfo if err := json.Unmarshal([]byte(cached), &info); err == nil { slog.Debug("Service cache hit from Redis", "domain", domain, "key", cacheKey) return &service.Service{ Code: info.Code, Active: info.Active, }, nil + } else { + slog.Warn("Failed to unmarshal cached service data", "domain", domain, "error", err) } }This requires importing
"github.com/redis/go-redis/v9"to accessredis.ErrNil(or usingredis.Nildepending on version).
60-63: Consider passing context to repository method.
repo.FindByDomaindoesn't accept acontext.Context, which prevents request cancellation and timeout propagation to the database layer. This could lead to resource leaks if the caller's context is cancelled but the DB query continues.If the
service.Repositoryinterface can be modified, consider updatingFindByDomainto accept a context:FindByDomain(ctx context.Context, domain string) (*service.Service, error)internal/domain/auth/middleware.go (2)
144-159: Handle edge cases in domain extraction.The function doesn't validate that the parsed URL has a scheme, which could lead to unexpected behavior for malformed origins like
example.com/path(parsed as path, not host).func extractDomainFromOrigin(origin string) (string, error) { origin = strings.TrimSuffix(origin, "/") parsedURL, err := url.Parse(origin) if err != nil { return "", err } + if parsedURL.Host == "" { + return "", fmt.Errorf("invalid origin: no host found") + } + domain := parsedURL.Host if idx := strings.Index(domain, ":"); idx != -1 { domain = domain[:idx] } return domain, nil }
112-122: Scopes stored twice in context.Scopes are stored both in
Identity.Scopes(line 118) and separately underScopesKey(line 122). This duplication could lead to inconsistencies if one is modified. Consider accessing scopes solely throughIdentity.If both access patterns are needed, this is acceptable. Otherwise, consolidate to reduce potential drift:
- c.Locals(ScopesKey, scopes) + // Scopes accessible via GetIdentity(c).ScopesAnd update
GetScopesto retrieve fromIdentity:func GetScopes(c *fiber.Ctx) map[string]uint64 { identity := GetIdentity(c) if identity == nil || identity.Scopes == nil { return make(map[string]uint64) } return identity.Scopes }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
config.yaml(1 hunks)docker-compose.yml(1 hunks)go.mod(2 hunks)internal/cache/redis.go(1 hunks)internal/cache/service_cache.go(1 hunks)internal/cache/token_revocation.go(1 hunks)internal/config/config.go(2 hunks)internal/domain/auth/errors.go(1 hunks)internal/domain/auth/handler.go(1 hunks)internal/domain/auth/middleware.go(5 hunks)internal/domain/auth/model.go(1 hunks)internal/domain/auth/service.go(3 hunks)internal/domain/auth/service_adapter.go(1 hunks)internal/domain/permission/adapter.go(1 hunks)internal/domain/permission/helpers.go(1 hunks)internal/domain/permission/model.go(1 hunks)internal/domain/permission/service.go(1 hunks)internal/domain/service/errors.go(1 hunks)internal/domain/service/handler.go(4 hunks)internal/domain/service/model.go(3 hunks)internal/domain/service/repository.go(4 hunks)internal/domain/service/service.go(6 hunks)internal/domain/session/repository.go(2 hunks)internal/domain/session/service.go(3 hunks)internal/domain/session/service_test.go(0 hunks)internal/domain/user/model.go(1 hunks)internal/migrations/000009_add_domain_to_services.down.sql(1 hunks)internal/migrations/000009_add_domain_to_services.up.sql(1 hunks)internal/migrations/000010_remove_unique_from_email.down.sql(1 hunks)internal/migrations/000010_remove_unique_from_email.up.sql(1 hunks)internal/migrations/migrate_test.go(0 hunks)internal/server/http.go(2 hunks)internal/server/router.go(5 hunks)
💤 Files with no reviewable changes (2)
- internal/migrations/migrate_test.go
- internal/domain/session/service_test.go
🧰 Additional context used
🧬 Code graph analysis (13)
internal/server/http.go (1)
internal/cache/redis.go (1)
ConnectRedis(19-36)
internal/domain/user/model.go (2)
internal/migrations/migrate_test.go (1)
TestMigrationSQL_UsersTable(201-240)internal/domain/user/repository.go (1)
r(41-47)
internal/domain/service/repository.go (1)
internal/domain/service/model.go (2)
Service(17-25)Service(27-29)
internal/domain/session/service.go (2)
internal/domain/session/repository.go (1)
Repository(10-17)internal/cache/token_revocation.go (1)
TokenRevocationCache(17-17)
internal/domain/auth/service.go (10)
internal/cache/token_revocation.go (1)
TokenRevocationCache(17-17)internal/domain/session/service.go (2)
NewService(45-47)Service(30-36)internal/domain/service/service.go (2)
NewService(34-36)ServiceInterface(16-25)internal/domain/user/service.go (2)
NewService(26-28)Service(15-18)internal/domain/session/repository.go (1)
Repository(10-17)internal/domain/service/repository.go (1)
Repository(6-15)internal/domain/permission/repository.go (1)
Repository(9-27)internal/domain/user/repository.go (1)
Repository(6-14)internal/domain/auth/keys.go (1)
KeyStore(16-19)internal/domain/auth/model.go (1)
AccessTokenClaims(12-15)
internal/cache/token_revocation.go (1)
internal/cache/redis.go (1)
RedisClient(15-15)
internal/cache/redis.go (1)
internal/config/config.go (1)
RedisConfig(51-56)
internal/domain/auth/middleware.go (5)
internal/domain/auth/keys.go (1)
KeyStore(16-19)internal/domain/auth/service.go (1)
AuthService(26-30)internal/utils/response.go (1)
ErrorResponse(22-32)internal/domain/auth/errors.go (4)
ErrTokenExpiredOrInvalid(177-177)ErrInvalidOrigin(189-189)ErrServiceNotFoundForDomain(192-192)ErrUnauthorized(186-186)internal/domain/auth/model.go (1)
Identity(130-135)
internal/cache/service_cache.go (2)
internal/cache/redis.go (1)
RedisClient(15-15)internal/domain/user/service.go (1)
repo(21-23)
internal/domain/auth/service_adapter.go (3)
internal/domain/service/model.go (2)
Service(17-25)Service(27-29)internal/cache/service_cache.go (1)
ServiceCache(26-28)internal/domain/auth/middleware.go (2)
ServiceRepository(23-25)ServiceInfo(28-31)
internal/domain/service/service.go (4)
internal/domain/service/model.go (2)
Service(17-25)Service(27-29)internal/domain/service/repository.go (1)
Repository(6-15)internal/cache/service_cache.go (1)
CacheInvalidator(21-23)internal/domain/service/errors.go (2)
ErrServiceDomainExists(13-13)ErrServiceNotFound(7-7)
internal/domain/service/handler.go (2)
internal/domain/service/errors.go (2)
ErrServiceCodeExists(10-10)ErrServiceDomainExists(13-13)internal/utils/response.go (1)
ErrorResponse(22-32)
internal/server/router.go (6)
internal/cache/service_cache.go (1)
NewServiceCache(31-33)internal/cache/token_revocation.go (1)
NewTokenRevocationCache(20-22)internal/domain/session/service.go (2)
NewServiceWithCache(50-52)NewService(45-47)internal/domain/auth/service.go (1)
NewService(43-52)internal/domain/auth/service_adapter.go (1)
NewServiceRepositoryAdapter(26-28)internal/domain/permission/adapter.go (1)
NewServiceRepositoryAdapter(9-11)
🔇 Additional comments (19)
internal/domain/permission/helpers.go (1)
52-52: No functional changes detected.This is a trivial end-of-file newline change with no impact on logic or behavior.
internal/domain/permission/adapter.go (1)
26-26: No functional changes—formatting only.This change adds a trailing newline. The adapter logic remains sound.
internal/migrations/000010_remove_unique_from_email.up.sql (1)
1-3: Migration correctly removes uniqueness but consider the implications.The migration successfully removes the unique constraint and replaces it with a non-unique partial index. However, consider these points:
Duplicate emails allowed: After this migration, multiple users can have identical non-NULL email addresses. Ensure this aligns with your authentication and user management requirements.
Partial index behavior: The
WHERE email IS NOT NULLclause means NULL emails won't be indexed, which could impact query performance if you filter on NULL emails.Coordinate with application code: Verify that all email-based operations (login, password reset, notifications) handle potential duplicate emails correctly.
Consider whether NULL emails should be allowed. If not, add a NOT NULL constraint:
ALTER TABLE users ALTER COLUMN email SET NOT NULL;internal/domain/auth/handler.go (1)
72-84: No-op formatting change; GetUserInfo behavior remains correctOnly EOF/brace formatting changed here; the existing flow (identity check, userService call, 500 on error, success response) stays intact and looks sound.
internal/domain/permission/service.go (1)
210-226: EOF formatting only; permission service logic unchangedThis change is purely structural (closing brace/newline); the permission service API and behavior remain the same and look consistent.
internal/domain/permission/model.go (1)
136-145: EOF formatting only; models and bit helpers remain validNo behavioral modifications here; Permission/UserPermission models and the bit helper functions still correctly guard
bit > 63and implement HasAll/HasAny as expected.internal/migrations/000009_add_domain_to_services.down.sql (1)
1-2: Down migration cleanly reverts domain-related schemaDropping
idx_services_domainbefore removing thedomaincolumn is the right order for PostgreSQL and matches the described up migration. As long as the up script only adds this index and column for domain support, this down migration should fully revert it.docker-compose.yml (1)
20-33: Redis service definition matches new cache usageThe Redis service (image, ports, volume, append-only config, and healthcheck) is wired correctly, and the
redis_datavolume is declared alongsidepostgres_data, which should work well for local/dev setups using the new Redis-backed features.Also applies to: 37-37
internal/domain/service/errors.go (1)
12-13: Domain‑duplicate error is well‑aligned with existing service errors
ErrServiceDomainExistsmirrorsErrServiceCodeExistsnicely and will make handler‑level 409/conflict handling straightforward. No issues here.internal/domain/auth/errors.go (1)
188-192: New origin/domain errors improve middleware clarityAdding
ErrInvalidOriginandErrServiceNotFoundForDomaingives you finer‑grained failure modes in the middleware (e.g., 400 vs 404 vs 401). The naming and placement alongside other middleware errors look good.internal/config/config.go (1)
18-19: RedisConfig wiring and Address() helper look consistentThe
Redisfield onConfig, YAML tags, andRedisConfig.Address()are consistent withconfig.yamland howConnectRedisconsumes them. Mirroring the existingServerConfig.Address()pattern keeps things straightforward. No changes needed here.Also applies to: 50-61
internal/domain/service/repository.go (1)
10-14: Domain lookup and persistence wiring looks correct
FindByDomainmirrors the existingFindByCodepattern, and including"domain": service.Domaininupdateskeeps the repository aligned with the new model field. No issues from the repository side.Also applies to: 50-57, 90-95
internal/domain/service/model.go (1)
22-22: Domain field is consistently exposed through the model and responseAdding
DomaintoService,ServiceResponse, andToResponsekeeps the API and persistence model in sync; the wiring looks consistent.Also applies to: 39-39, 53-53
internal/domain/session/service.go (1)
40-52: Optional revocation cache injection is cleanly designedExtending the service with a
revocationCachefield and addingNewServiceWithCachekeeps the existing constructor intact while enabling cache-backed revocation without forcing Redis into tests or non-Redis deployments.internal/domain/auth/service.go (1)
33-51: Auth service constructor update for revocation cache looks consistentAdding
revocationCacheto the Service struct and wiring it via the extendedNewService(...)signature is straightforward and keeps dependencies explicit; no issues here as long as all call sites are updated.internal/domain/service/service.go (1)
10-25: Domain‑aware service interface and Create/FindByDomain behavior look solidThe
CacheInvalidatorabstraction, newCreate(..., domain string)signature, andFindByDomainimplementation all line up with the repository model and error mapping viaErrServiceNotFound/ErrServiceDomainExists. The explicit domain uniqueness check on create (for non‑empty domains) is a good complement to any DB‑level uniqueness you have.Also applies to: 38-74, 100-110
internal/domain/auth/middleware.go (3)
22-31: LGTM on interface definitions.Clean abstractions for service lookup. The separation of
ServiceRepositoryandServiceInfointerfaces follows interface segregation principle well.
161-176: LGTM on RequireScope middleware.The implementation correctly checks for scope existence and non-zero bitmask. The use of
fiber.StatusForbidden(403) is appropriate since the user is authenticated but lacks authorization.
209-216: LGTM on GetScopes helper.Returns an empty map as safe default when scopes are missing, preventing nil map panics in callers.
| func (c *ServiceCache) GetByDomain(ctx context.Context, domain string) (*service.Service, error) { | ||
| cacheKey := ServiceCachePrefix + domain | ||
|
|
||
| // Try to get from Redis cache | ||
| cached, err := RedisClient.Get(ctx, cacheKey).Result() |
There was a problem hiding this comment.
Add nil check for global RedisClient.
If RedisClient is not initialized before GetByDomain is called, this will panic with a nil pointer dereference.
func (c *ServiceCache) GetByDomain(ctx context.Context, domain string) (*service.Service, error) {
cacheKey := ServiceCachePrefix + domain
+ if RedisClient == nil {
+ slog.Warn("Redis client not initialized, falling back to database", "domain", domain)
+ return c.repo.FindByDomain(domain)
+ }
+
// Try to get from Redis cache
cached, err := RedisClient.Get(ctx, cacheKey).Result()📝 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.
| func (c *ServiceCache) GetByDomain(ctx context.Context, domain string) (*service.Service, error) { | |
| cacheKey := ServiceCachePrefix + domain | |
| // Try to get from Redis cache | |
| cached, err := RedisClient.Get(ctx, cacheKey).Result() | |
| func (c *ServiceCache) GetByDomain(ctx context.Context, domain string) (*service.Service, error) { | |
| cacheKey := ServiceCachePrefix + domain | |
| if RedisClient == nil { | |
| slog.Warn("Redis client not initialized, falling back to database", "domain", domain) | |
| return c.repo.FindByDomain(domain) | |
| } | |
| // Try to get from Redis cache | |
| cached, err := RedisClient.Get(ctx, cacheKey).Result() |
🤖 Prompt for AI Agents
In internal/cache/service_cache.go around lines 42 to 46, add a nil check for
the global RedisClient before using it to avoid a nil pointer dereference: if
RedisClient is nil, return a clear error (or fallback behavior) immediately
instead of calling RedisClient.Get; this entails checking RedisClient==nil at
the top of GetByDomain, returning an error such as fmt.Errorf("redis client not
initialized") (or a package-level sentinel error) so callers can handle the
uninitialized state.
| return &service.Service{ | ||
| Code: info.Code, | ||
| Active: info.Active, | ||
| }, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the service.Service struct definition
fd -t go -e go | xargs rg -l "type Service struct" | head -5Repository: Anvoria/authly
Length of output: 309
🏁 Script executed:
# Find the GetByDomain method in service_cache.go
rg -n "GetByDomain" internal/cache/service_cache.go -A 15Repository: Anvoria/authly
Length of output: 693
🏁 Script executed:
# Search for all calls to GetByDomain to understand usage patterns
rg -n "GetByDomain" --type go -B 2 -A 5Repository: Anvoria/authly
Length of output: 1295
🏁 Script executed:
# Get the full context of the service_cache.go file around the cached lines
cat -n internal/cache/service_cache.go | head -80Repository: Anvoria/authly
Length of output: 2724
🏁 Script executed:
# Find the service.Service struct definition
cat -n internal/domain/service/model.goRepository: Anvoria/authly
Length of output: 2149
🏁 Script executed:
# Find the serviceInfoAdapter implementation and ServiceInfo interface
cat -n internal/domain/auth/service_adapter.goRepository: Anvoria/authly
Length of output: 1385
🏁 Script executed:
# Check if there are any other callers of GetByDomain
rg -n "GetByDomain" --type-listRepository: Anvoria/authly
Length of output: 447
🏁 Script executed:
# Better search for other GetByDomain callers
rg -n "GetByDomain" -A 3 -B 1Repository: Anvoria/authly
Length of output: 847
🏁 Script executed:
# Find the ServiceInfo interface definition
rg -n "interface ServiceInfo" -A 10Repository: Anvoria/authly
Length of output: 40
🏁 Script executed:
# Search for any direct field access on service objects returned from cache
rg -n "\.Name|\.Domain|\.Description|\.IsSystem|\.ID" internal/domain/auth/ internal/cache/Repository: Anvoria/authly
Length of output: 547
🏁 Script executed:
# Check if there are other uses of service.Service that might access uninitialized fields
rg -n "service\\.Service" --type-list-output | head -20Repository: Anvoria/authly
Length of output: 505
🏁 Script executed:
# Better search for service.Service usage
rg -n "\\*service\\.Service" -B 2 -A 3Repository: Anvoria/authly
Length of output: 512
🏁 Script executed:
# Search for ServiceInfo interface definition more carefully
rg -n "ServiceInfo" internal/domain/auth/ -A 5Repository: Anvoria/authly
Length of output: 1619
Cache hit returns service.Service with uninitialized fields beyond Code and Active.
On cache hit, only Code and Active are populated. While current callers access the service through the ServiceInfo interface (which only requires these two fields), any future direct access to other fields like ID, Domain, Name, Description, or IsSystem will get zero values. This is inconsistent with the database hit path, which returns a fully initialized object. Consider either: (1) caching all necessary fields, or (2) documenting that this method returns an incomplete object and should only be consumed through the ServiceInfo interface.
🤖 Prompt for AI Agents
In internal/cache/service_cache.go around lines 51 to 54, the cache-hit branch
returns a service.Service with only Code and Active populated, leaving other
fields zeroed and inconsistent with the DB path; update the cache-return to
populate all Service fields present in the cached ServiceInfo (ID, Domain, Name,
Description, IsSystem, etc.) so the returned *service.Service matches the
DB-returned object, or alternatively change the function signature/documentation
to clearly return only the ServiceInfo interface and enforce callers use that
interface; implement the former by reading all fields from info and assigning
them to the service.Service before returning.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/domain/session/service.go (1)
181-200:Exists()is unused and doesn't check the revocation cache.The
Exists()method is defined in theServiceinterface but is not called anywhere in the codebase. While this creates an inconsistency withRevoke()(which caches revocation status), the actual token revocation checks in production useauth.Service.IsTokenRevoked(), which properly queries the cache. Consider either removing the unusedExists()method or ensuring it checks the cache for consistency if it's intended for future use.
♻️ Duplicate comments (3)
internal/domain/auth/middleware.go (3)
74-80: Expiration checks may be redundant.As noted in a previous review, if
keyStore.Verify()usesjwt.Parse()from lestrrat-go/jwx v3, theexpclaim is validated by default during parsing. These explicit checks would only be reached for valid tokens.#!/bin/bash # Check how keyStore.Verify handles token parsing ast-grep --pattern 'func ($_ *KeyStore) Verify($$$) { $$$ }' rg -n 'jwt.Parse|jwt.ParseString' --type go internal/domain/auth/
83-111: Origin/Referer headers can be spoofed by malicious clients.As flagged in a previous review, relying on
Origin/Refererheaders for audience validation is problematic since these headers are client-controlled. This approach also rejects legitimate non-browser API clients (curl, mobile apps, service-to-service calls) that don't send these headers.Consider documenting the intended use case (browser-only) or providing an alternative authentication path for server-to-server communication.
207-209: Potential bit shift overflow whenrequiredBit >= 64.
requiredBitisuint8(0-255), but the bitmask isuint64. IfrequiredBit >= 64, the shift1 << requiredBitoverflows. In Go, the result wraps (effectively becomes 0 for values >= 64), causing the permission check to always fail silently.func RequirePermission(requiredScope string, requiredBit uint8) fiber.Handler { return func(c *fiber.Ctx) error { + if requiredBit >= 64 { + slog.Error("invalid permission bit", "bit", requiredBit) + return utils.ErrorResponse(c, ErrUnauthorized.Error(), fiber.StatusForbidden) + } + scopes, ok := c.Locals(ScopesKey).(map[string]uint64)
🧹 Nitpick comments (4)
internal/domain/service/service.go (2)
153-164: Cache invalidation always runs even when domain hasn't changed.The invalidation logic runs whenever
s.cache != nil, regardless of whether the domain actually changed. This causes unnecessary cache invalidations when onlyname,description, oractivefields are updated.// Invalidate cache if domain changed or if cache is available - if s.cache != nil { + if s.cache != nil && (domain != nil && oldDomain != *domain) { ctx := context.Background() // Invalidate old domain cache if oldDomain != "" { _ = s.cache.InvalidateByDomain(ctx, oldDomain) } // Invalidate new domain cache if svc.Domain != "" { _ = s.cache.InvalidateByDomain(ctx, svc.Domain) } }
158-158: Silently ignoring cache invalidation errors may cause stale data issues.Cache invalidation failures are discarded with
_ =. While this prevents cache errors from blocking updates, stale cached data could cause authorization issues (e.g., a domain pointing to the wrong service code). Consider logging these failures at minimum.- if oldDomain != "" { - _ = s.cache.InvalidateByDomain(ctx, oldDomain) - } - // Invalidate new domain cache - if svc.Domain != "" { - _ = s.cache.InvalidateByDomain(ctx, svc.Domain) - } + if oldDomain != "" { + if err := s.cache.InvalidateByDomain(ctx, oldDomain); err != nil { + // Log but don't fail the update + // Consider adding structured logging here + } + } + if svc.Domain != "" { + if err := s.cache.InvalidateByDomain(ctx, svc.Domain); err != nil { + // Log but don't fail the update + } + }Also applies to: 162-162
internal/domain/auth/middleware.go (2)
171-187: HTTP 403 returned for authentication failures uses "unauthorized" error message.
RequireScopeandRequirePermissionreturn HTTP 403 Forbidden (authorization failure) but useErrUnauthorizederror message. This is semantically confusing—403 indicates the client is authenticated but lacks permission, while "unauthorized" typically refers to authentication failure (401).Consider using a more appropriate error like
ErrForbiddenorErrInsufficientPermissionsfor clarity:+var ErrForbidden = errors.New("forbidden: insufficient permissions") func RequireScope(requiredScope string) fiber.Handler { return func(c *fiber.Ctx) error { // ... if !exists || bitmask == 0 { - return utils.ErrorResponse(c, ErrUnauthorized.Error(), fiber.StatusForbidden) + return utils.ErrorResponse(c, ErrForbidden.Error(), fiber.StatusForbidden) }Also applies to: 195-213
153-169:extractDomainFromOriginaccepts any URL scheme.The function parses any valid URL without validating the scheme. This could allow unexpected origins like
file://,javascript:, or custom schemes. Consider restricting tohttpandhttpsonly.func extractDomainFromOrigin(origin string) (string, error) { origin = strings.TrimSuffix(origin, "/") parsedURL, err := url.Parse(origin) if err != nil { return "", err } + if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { + return "", fmt.Errorf("unsupported scheme: %s", parsedURL.Scheme) + } + domain := parsedURL.Host
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/cache/redis.go(1 hunks)internal/cache/service_cache.go(1 hunks)internal/domain/auth/middleware.go(4 hunks)internal/domain/auth/service_adapter.go(1 hunks)internal/domain/permission/helpers.go(1 hunks)internal/domain/permission/model.go(1 hunks)internal/domain/service/service.go(6 hunks)internal/domain/session/service.go(4 hunks)internal/server/router.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/domain/permission/helpers.go
- internal/domain/permission/model.go
- internal/cache/redis.go
- internal/cache/service_cache.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/domain/session/service.go (3)
internal/domain/session/repository.go (1)
Repository(10-17)internal/cache/token_revocation.go (1)
TokenRevocationCache(17-17)internal/domain/auth/service.go (2)
NewService(43-52)Service(33-40)
internal/domain/auth/service_adapter.go (4)
internal/domain/session/service.go (1)
Service(30-36)internal/domain/service/model.go (2)
Service(17-25)Service(27-29)internal/cache/service_cache.go (1)
ServiceCache(26-28)internal/domain/auth/middleware.go (2)
ServiceRepository(23-25)ServiceInfo(28-31)
🔇 Additional comments (7)
internal/domain/service/service.go (1)
10-13: Good interface design for optional cache dependency.The
CacheInvalidatorinterface is minimal and focused. The constructor properly documents thatcachecan be nil, enabling flexible wiring without forcing cache usage.Also applies to: 33-37
internal/server/router.go (2)
37-42: Cache initialization and wiring looks correct.The service cache and token revocation cache are properly initialized and passed to dependent services. The session service correctly receives the revocation cache for Redis-backed revocation tracking.
43-43: This design is correct—the adapters intentionally use different parameters.
perm.NewServiceRepositoryAdapter()requires aservice.Repositoryinterface (for Create, Update, Delete, FindAll, etc.), whileauth.NewServiceRepositoryAdapter()accepts*cache.ServiceCachewhich only provides cached lookups. The permission service needs full repository capabilities for domain operations, while the auth middleware only needs to look up cached service info. UsingserviceCachefor the permission adapter would not be possible without breaking type safety, and bypassing the cache for auth would eliminate its benefit. The current wiring is intentional and correct.Likely an incorrect or invalid review comment.
internal/domain/session/service.go (1)
150-178: Revoke implementation with cache integration is well-designed.The approach of fetching session details before revocation to calculate TTL, combined with logging cache failures without blocking the primary operation, is a good resilience pattern. The 1-hour minimum TTL for expired sessions ensures cache entries don't persist indefinitely.
internal/domain/auth/service_adapter.go (2)
35-42: Consider accepting context as parameter instead of usingcontext.Background().
FindByDomaincreates a background context internally. If the caller (middleware) has access to a request-scoped context with deadlines or cancellation, this would ignore those signals. However, since Fiber's*fiber.Ctxdoesn't directly provide acontext.Context, this is acceptable for now.
10-23: Clean adapter implementation.The
serviceInfoAdapterprovides a minimal interface that exposes only whatServiceInforequires, following interface segregation principles.internal/domain/auth/middleware.go (1)
226-234:GetScopesreturns empty map on failure, which is a safe default.Returning an empty map instead of nil prevents nil pointer dereferences when callers iterate over scopes without checking the return value.
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 (1)
internal/domain/service/service.go (1)
40-68: Domain field stores empty string instead of NULL, violating UNIQUE constraint on second insert.The UNIQUE index is defined as
CREATE UNIQUE INDEX idx_services_domain ON services(domain) WHERE domain IS NOT NULL. This allows multiple NULL values but treats empty strings''as real values subject to uniqueness. TheCreatemethod skips the uniqueness check whendomain == ""but still persistsDomain: domainwith the empty string. The second call toCreatewith an empty domain will fail with a UNIQUE constraint violation.Fix: Change
domainparameter fromstringto*stringand persistNULL(via a nil pointer orsql.NullString) when domain is not provided, aligning with the migration'sWHERE domain IS NOT NULLfilter. Alternatively, make domain required by validatingdomain != ""at the start and adding a DBNOT NULLconstraint.
♻️ Duplicate comments (5)
internal/domain/auth/middleware.go (2)
67-81: Expiration checks may be redundant depending onKeyStore.Verify(avoid double-validation).
IfKeyStore.Verifyuses jwx parsing with default validators,expis already validated during parse; consider removing manualexpchecks (or ensure Verify is configured to not validate exp if you want to centralize it here).#!/bin/bash set -euo pipefail rg -n "func \\(.*\\*KeyStore\\) Verify\\(" --type go -n -S
195-209: Fix unsafe bit shifting inRequirePermission(requiredBitmust be < 64).
As-is, shifts can misbehave forrequiredBit >= 64and the operand should beuint64(1).func RequirePermission(requiredScope string, requiredBit uint8) fiber.Handler { return func(c *fiber.Ctx) error { + if requiredBit >= 64 { + slog.Error("invalid permission bit", "bit", requiredBit) + return utils.ErrorResponse(c, ErrUnauthorized.Error(), fiber.StatusForbidden) + } scopes, ok := c.Locals(ScopesKey).(map[string]uint64) if !ok || scopes == nil { return utils.ErrorResponse(c, ErrUnauthorized.Error(), fiber.StatusForbidden) } @@ - if (bitmask & (1 << requiredBit)) == 0 { + if (bitmask & (uint64(1) << requiredBit)) == 0 { return utils.ErrorResponse(c, ErrUnauthorized.Error(), fiber.StatusForbidden) }internal/cache/service_cache.go (3)
41-79: GuardRedisClientnil and handle Redis errors explicitly (fallback to DB).
This will panic ifRedisClientisn’t initialized, and it also silently treats non-redis.Nilerrors as cache misses. Additionally, cache-hit returns a partially-populated*service.Service(only Code/Active).func (c *ServiceCache) GetByDomain(ctx context.Context, domain string) (*service.Service, error) { cacheKey := ServiceCachePrefix + domain + if RedisClient == nil { + slog.Warn("Redis client not initialized; bypassing cache", "domain", domain) + return c.repo.FindByDomain(domain) + } + // Try to get from Redis cache cached, err := RedisClient.Get(ctx, cacheKey).Result() if err == nil { var info cachedServiceInfo if err := json.Unmarshal([]byte(cached), &info); err == nil { slog.Debug("Service cache hit from Redis", "domain", domain, "key", cacheKey) return &service.Service{ Code: info.Code, Active: info.Active, }, nil } }
82-89: AddRedisClientnil check in invalidation to avoid panics.
Same nil deref risk as inGetByDomain.
91-104: Replace RedisKEYSwithSCANinInvalidateAllto avoid O(N) blocking.
KEYScan stall Redis in production; use cursor-basedSCANand delete in batches.
🧹 Nitpick comments (3)
internal/domain/session/service.go (1)
44-53: Constructor split is clean; consider unifying on a single constructor for DI consistency.
NewServiceWithCacheis fine, but you might simplify wiring by havingNewService(repo)callNewServiceWithCache(repo, nil)to keep one initialization path.internal/domain/service/service.go (2)
10-13: Avoid duplicatingCacheInvalidatortypes across packages (minor cohesion win).
You now have aCacheInvalidatorhere and ininternal/cache/service_cache.go; consider importing and using a single canonical interface to reduce drift.
125-165: Cache invalidation is best-effort, but should be bounded + observable (and comment is slightly misleading).
Right now invalidation always runs whens.cache != nil(even if domain didn’t change), usescontext.Background(), and discards errors. Consider: (1) only invalidate whenoldDomain != svc.Domain, (2) add a short timeout, and (3) log errors at debug/warn.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/cache/redis.go(1 hunks)internal/cache/service_cache.go(1 hunks)internal/domain/auth/middleware.go(4 hunks)internal/domain/auth/service_adapter.go(1 hunks)internal/domain/permission/helpers.go(1 hunks)internal/domain/permission/model.go(1 hunks)internal/domain/service/service.go(6 hunks)internal/domain/session/service.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/domain/auth/service_adapter.go
- internal/cache/redis.go
- internal/domain/permission/model.go
- internal/domain/permission/helpers.go
🧰 Additional context used
🧬 Code graph analysis (4)
internal/domain/service/service.go (4)
internal/domain/service/model.go (2)
Service(17-25)Service(27-29)internal/domain/service/repository.go (1)
Repository(6-15)internal/cache/service_cache.go (1)
CacheInvalidator(21-23)internal/domain/service/errors.go (2)
ErrServiceDomainExists(13-13)ErrServiceNotFound(7-7)
internal/domain/auth/middleware.go (5)
internal/domain/auth/keys.go (1)
KeyStore(16-19)internal/domain/auth/service.go (1)
AuthService(26-30)internal/utils/response.go (1)
ErrorResponse(22-32)internal/domain/auth/errors.go (4)
ErrTokenExpiredOrInvalid(177-177)ErrInvalidOrigin(189-189)ErrServiceNotFoundForDomain(192-192)ErrUnauthorized(186-186)internal/domain/auth/model.go (1)
Identity(130-135)
internal/cache/service_cache.go (1)
internal/cache/redis.go (1)
RedisClient(15-15)
internal/domain/session/service.go (3)
internal/domain/session/repository.go (1)
Repository(10-17)internal/cache/token_revocation.go (1)
TokenRevocationCache(17-17)internal/domain/service/service.go (1)
NewService(35-37)
⏰ 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: Build
🔇 Additional comments (1)
internal/domain/auth/middleware.go (1)
3-13: The project'sgo.moddeclares Go version 1.25.4, which exceeds the Go 1.21+ requirement for the stdlibslicespackage. The import is valid and will compile without issues.
internal/domain/auth/middleware.go
Outdated
| func extractDomainFromOrigin(origin string) (string, error) { | ||
| origin = strings.TrimSuffix(origin, "/") | ||
|
|
||
| parsedURL, err := url.Parse(origin) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| domain := parsedURL.Host | ||
| if idx := strings.Index(domain, ":"); idx != -1 { | ||
| domain = domain[:idx] | ||
| } | ||
|
|
||
| return domain, nil | ||
| } |
There was a problem hiding this comment.
Use parsedURL.Hostname() (IPv6-safe) and reject empty hosts.
Current strings.Index(domain, ":") breaks IPv6 hosts like [::1]:3000. Also, url.Parse can succeed with an empty Host (e.g., missing scheme), leading to domain == "".
func extractDomainFromOrigin(origin string) (string, error) {
origin = strings.TrimSuffix(origin, "/")
parsedURL, err := url.Parse(origin)
if err != nil {
return "", err
}
- domain := parsedURL.Host
- if idx := strings.Index(domain, ":"); idx != -1 {
- domain = domain[:idx]
- }
+ domain := parsedURL.Hostname()
+ if domain == "" {
+ return "", ErrInvalidOrigin
+ }
return domain, 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.
| func extractDomainFromOrigin(origin string) (string, error) { | |
| origin = strings.TrimSuffix(origin, "/") | |
| parsedURL, err := url.Parse(origin) | |
| if err != nil { | |
| return "", err | |
| } | |
| domain := parsedURL.Host | |
| if idx := strings.Index(domain, ":"); idx != -1 { | |
| domain = domain[:idx] | |
| } | |
| return domain, nil | |
| } | |
| func extractDomainFromOrigin(origin string) (string, error) { | |
| origin = strings.TrimSuffix(origin, "/") | |
| parsedURL, err := url.Parse(origin) | |
| if err != nil { | |
| return "", err | |
| } | |
| domain := parsedURL.Hostname() | |
| if domain == "" { | |
| return "", ErrInvalidOrigin | |
| } | |
| return domain, nil | |
| } |
🤖 Prompt for AI Agents
In internal/domain/auth/middleware.go around lines 155 to 169, replace the
current host extraction with a safer approach: use parsedURL.Hostname() instead
of slicing on ":" so IPv6 addresses (e.g. [::1]:3000) are handled correctly, and
add a check that the resulting hostname is non-empty (returning an error if
empty) because url.Parse can succeed with an empty Host when the scheme is
missing; keep returning the hostname on success.
| // GetScopes returns the scopes map stored in the Fiber context under ScopesKey. | ||
| // If no scopes are present or the stored value has a different type, it returns an empty map. | ||
| func GetScopes(c *fiber.Ctx) map[string]uint64 { | ||
| scopes, ok := c.Locals(ScopesKey).(map[string]uint64) | ||
| if !ok { | ||
| return make(map[string]uint64) | ||
| } | ||
| return scopes | ||
| } |
There was a problem hiding this comment.
GetScopes docs vs behavior: typed-nil map returns nil, not empty.
If AuthMiddleware stores a typed-nil map[string]uint64, GetScopes will return nil (despite the doc promising an empty map). Either adjust the doc or normalize nil to map[string]uint64{}.
🤖 Prompt for AI Agents
In internal/domain/auth/middleware.go around lines 226 to 234, the doc says
GetScopes returns an empty map when no scopes are present, but if a typed-nil
map[string]uint64 is stored it currently returns nil; change the function to
normalize typed-nil maps to an empty map by performing the type assertion, and
if ok but scopes==nil return make(map[string]uint64) so callers always receive a
non-nil empty map (keep the existing behavior of returning an empty map when the
assertion fails).
…rviceRepository interface for code-based service retrieval
…ware to ensure valid bit positions
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/domain/auth/middleware.go (2)
72-78: Expiration validation is redundant.As noted in a previous review,
keyStore.Verify()usesjwt.Parse()from lestrrat-go/jwx v3, which validates theexpclaim by default during token parsing. If the token reaches these checks, expiration has already been validated. Remove the explicitexp.IsZero()andtime.Now().After(exp)checks to eliminate code duplication.Apply this diff:
// Validate issuer and expiration iss := claims.Issuer() if issuer != "" && iss != issuer { slog.Error("token issuer mismatch", "expected", issuer, "got", iss) return utils.ErrorResponse(c, ErrTokenExpiredOrInvalid.Error(), fiber.StatusUnauthorized) } - exp := claims.Expiration() - if exp.IsZero() { - return utils.ErrorResponse(c, ErrTokenExpiredOrInvalid.Error(), fiber.StatusUnauthorized) - } - if time.Now().After(exp) { - return utils.ErrorResponse(c, ErrTokenExpiredOrInvalid.Error(), fiber.StatusUnauthorized) - } - aud := claims.Audience()Based on learnings from previous review.
193-201: Normalize typed-nil map to empty map as documented.The documentation states that
GetScopesreturns an empty map when no scopes are present. However, if a typed-nilmap[string]uint64is stored in the context, the function currently returnsnilinstead of an empty map, violating the documented contract.Apply this diff:
func GetScopes(c *fiber.Ctx) map[string]uint64 { scopes, ok := c.Locals(ScopesKey).(map[string]uint64) - if !ok { + if !ok || scopes == nil { return make(map[string]uint64) } return scopes }Based on learnings from previous review.
internal/cache/service_cache.go (1)
57-60: Cache hit returns incompleteservice.Serviceobject.On cache hit, only
CodeandActivefields are populated in the returnedservice.Service, while other fields (ID,Domain,Name,Description,IsSystem, andBaseModeltimestamps) remain zero-valued. This is inconsistent with the database path which returns a fully initialized object.While current callers access services through the
ServiceInfointerface (which only requiresCodeandActive), any future code accessing other fields will get incorrect zero values. Consider either:
- Cache all necessary fields in
cachedServiceInfoand populate the fullservice.Serviceon cache hit- Document that this cache layer should only be consumed through the
ServiceInfointerface and never accessed directlyBased on learnings from previous review.
Also applies to: 101-104
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/cache/redis.go(1 hunks)internal/cache/service_cache.go(1 hunks)internal/domain/auth/middleware.go(4 hunks)internal/domain/auth/service_adapter.go(1 hunks)internal/migrations/migrate_test.go(0 hunks)internal/server/router.go(5 hunks)
💤 Files with no reviewable changes (1)
- internal/migrations/migrate_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cache/redis.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/domain/auth/middleware.go (6)
internal/domain/auth/keys.go (1)
KeyStore(16-19)internal/domain/auth/service.go (1)
AuthService(26-30)internal/domain/permission/service.go (1)
ServiceRepository(47-49)internal/utils/response.go (1)
ErrorResponse(22-32)internal/domain/auth/errors.go (3)
ErrTokenExpiredOrInvalid(177-177)ErrTokenValidationError(180-180)ErrUnauthorized(186-186)internal/domain/auth/model.go (1)
Identity(130-135)
internal/server/router.go (6)
internal/cache/service_cache.go (1)
NewServiceCache(33-35)internal/cache/token_revocation.go (1)
NewTokenRevocationCache(20-22)internal/domain/session/service.go (2)
NewServiceWithCache(51-53)NewService(45-47)internal/domain/auth/service.go (1)
NewService(43-52)internal/domain/auth/service_adapter.go (1)
NewServiceRepositoryAdapter(27-29)internal/domain/auth/middleware.go (1)
AuthMiddleware(43-131)
internal/domain/auth/service_adapter.go (3)
internal/domain/service/model.go (2)
Service(17-25)Service(27-29)internal/cache/service_cache.go (1)
ServiceCache(28-30)internal/domain/auth/middleware.go (2)
ServiceRepository(21-24)ServiceInfo(27-30)
⏰ 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: Build
🔇 Additional comments (1)
internal/server/router.go (1)
37-71: LGTM! Cache integration is well-structured.The router properly initializes the cache layer and wires cache-backed services into the authentication flow. The separation of concerns is clear: caches are initialized, services consume them, and the middleware receives the cache-backed adapter.
… parameter for FindByDomain and FindByCode methods
…nt potential blocking
1 participant
Summary by CodeRabbit
New Features
Improvements
Database
✏️ Tip: You can customize this high-level summary in your review settings.