Streamlines API Architecture and Enhances Authentication - #45
Conversation
Restructures core API directories and packages for improved organization and clarity. Upgrades the JWT authentication system to use access and refresh token pairs. Introduces a flexible JWT middleware with support for user loading, caching, and distinct authentication levels. Modifies user login to issue and set secure HTTP-only token cookies. Updates JWT claims to include user ID for more robust session management.
Renames `handlers` to `handler` and `services` to `service` directories and packages for improved consistency. Removes `firestore_` prefixes from internal package names, simplifying the codebase. Introduces an in-memory cache for JWT token validation to optimize user lookup performance in authentication middleware. Decouples the `UserService` dependency from the JWT middleware configuration, relying directly on the `service` layer.
Centralizes JWT claims definition and adds user type field. Simplifies authentication middleware by removing direct user database lookup, focusing on claims validation. Introduces a global in-memory token cache for efficient management and blacklisting. Standardizes access token cookie and header names to `access_token`. Updates token validation endpoint to return claims data directly. Increases token cache cleanup interval for improved performance.
There was a problem hiding this comment.
Pull Request Overview
This refactor modernizes the API architecture by reorganizing the codebase into cleaner layers and implementing a comprehensive JWT authentication system with access and refresh tokens. The changes improve security, maintainability, and development experience.
- Restructures packages from
internal/usecasesandinternal/infrastructureto distinctservice,database, andserverlayers - Implements enhanced JWT authentication with separate access/refresh tokens and automatic token refresh middleware
- Adds in-memory caching for user lookups during authentication to reduce database load
Reviewed Changes
Copilot reviewed 31 out of 35 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| api/internal/service/*.go | Moved service logic from usecases package with updated import paths |
| api/internal/server/middleware/token/*.go | New comprehensive JWT token management system with caching and refresh capabilities |
| api/internal/server/handler/*.go | Updated handlers to use new service layer and token system |
| api/internal/server/routes.go | New centralized route configuration with improved middleware setup |
| api/internal/domain/token.go | New domain models for token configuration and caching interfaces |
| api/common/*.go | Moved common utilities from internal package with new domain parsing function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if err != nil { | ||
| return err | ||
| // Split chapters by priority | ||
| // Chpaters with chpater in title take priority |
There was a problem hiding this comment.
Typo in comment: 'Chpaters' should be 'Chapters' and 'chpater' should be 'chapter'
| // Chpaters with chpater in title take priority | |
| // Chapters with chapter in title take priority |
| return err | ||
| // Split chapters by priority | ||
| // Chpaters with chpater in title take priority | ||
| // Notes, Synopsys and everything else will be processes last |
There was a problem hiding this comment.
Typo in comment: 'Synopsys' should be 'Synopsis' and 'processes' should be 'processed'
| // Notes, Synopsys and everything else will be processes last | |
| // Notes, Synopsis and everything else will be processed last |
| } | ||
|
|
||
| // Cache user if cache is available | ||
| cacheKey := fmt.Sprintf("user:%s", claims.ID) |
There was a problem hiding this comment.
Using claims.ID instead of claims.Subject. The refresh token only contains Subject (user ID) in RegisteredClaims, not an ID field
| cacheKey := fmt.Sprintf("user:%s", claims.ID) | |
| user, err := service.GetUserByID(claims.Subject, ctx) | |
| if err != nil { | |
| return "", err | |
| } | |
| // Cache user if cache is available | |
| cacheKey := fmt.Sprintf("user:%s", claims.Subject) |
| func GetDomains(v string) []string { | ||
| env_variable := os.Getenv(v) | ||
| if env_variable == "" { | ||
| log.Fatal(&Error{Err: errors.New("Environmental Variable " + v + " Not Found"), Status: http.StatusNotFound}) |
There was a problem hiding this comment.
log.Fatal will terminate the program immediately, but the Error struct with Status field suggests this should return an error instead of calling log.Fatal
|
|
||
| result := strings.Split(env_variable, ",") | ||
|
|
||
| return result |
There was a problem hiding this comment.
The function will never reach this return statement if the environment variable is empty due to log.Fatal being called above
| func RegisteredRoutes(r *gin.Engine) { | ||
| domains := cmn.GetDomains("DOMAIN") | ||
| if gin.Mode() == gin.DebugMode && len(domains) == 0 { | ||
| domains[0] = "*" |
There was a problem hiding this comment.
Attempting to assign to index 0 of an empty slice will cause a panic. Should use domains = []string{"*"} instead
| domains[0] = "*" | |
| domains = []string{"*"} |
| } | ||
|
|
||
| // Cache user if cache is available | ||
| cacheKey := fmt.Sprintf("user:%s", claims.ID) |
There was a problem hiding this comment.
Using claims.ID which doesn't exist in RegisteredClaims. Should use claims.Subject instead
| cacheKey := fmt.Sprintf("user:%s", claims.ID) | |
| user, err := service.GetUserByID(claims.Subject, ctx) | |
| if err != nil { | |
| return "", err | |
| } | |
| // Cache user if cache is available | |
| cacheKey := fmt.Sprintf("user:%s", claims.Subject) |
This refactor overhauls the backend project structure and significantly upgrades the authentication system.
common,database,service, andserverlayers for improved modularity and clarity.