feat(auth): enhance user role handling and context management #2533
Conversation
- Added ContextKeyUserRole to context keys for better role management. - Refactored authHelper to utilize userCache for role and status checks. - Updated WriteContext method in UserBase to include user role in context.
WalkthroughThe changes introduce a role-based authorization context key and refactor the authentication middleware to use cached user data instead of session values. The UserBase model is extended with a Role field that flows through the cache system into the request context. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/auth.go (1)
137-146: Remove commented-out dead code.This commented block duplicates the functionality now implemented at lines 99-134. Remove it to keep the codebase clean.
🔎 Proposed fix
userCache.WriteContext(c) c.Set("use_access_token", useAccessToken) - //userCache, err := model.GetUserCache(id.(int)) - //if err != nil { - // c.JSON(http.StatusOK, gin.H{ - // "success": false, - // "message": err.Error(), - // }) - // c.Abort() - // return - //} - //userCache.WriteContext(c) - c.Next()
🧹 Nitpick comments (1)
model/user_cache.go (1)
105-114: Role correctly populated from DB.The
Rolefield is properly included when constructing the cache object from the database user.Consider whether an
updateUserRoleCachehelper function should be added for consistency with other field-specific cache updaters (e.g.,updateUserStatusCache,updateUserGroupCache). This would support atomic role updates without full cache invalidation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
constant/context_key.gomiddleware/auth.gomodel/user_cache.go
🧰 Additional context used
🧬 Code graph analysis (1)
model/user_cache.go (2)
common/gin.go (1)
SetContextKey(99-101)constant/context_key.go (8)
ContextKeyUserId(46-46)ContextKeyUserGroup(51-51)ContextKeyUserQuota(48-48)ContextKeyUserStatus(49-49)ContextKeyUserEmail(50-50)ContextKeyUserName(53-53)ContextKeyUserSetting(47-47)ContextKeyUserRole(54-54)
🔇 Additional comments (4)
constant/context_key.go (1)
54-54: LGTM!The new
ContextKeyUserRoleconstant follows the established naming convention and is logically placed within the user-related keys section.model/user_cache.go (2)
17-26: LGTM - Role field added to UserBase.The new
Rolefield is properly added to the struct with consistent JSON tagging.
28-37: LGTM - Role propagated to context.The
WriteContextmethod now correctly stores the user's role in the Gin context using the newContextKeyUserRoleconstant.middleware/auth.go (1)
110-134: LGTM - Cache-based authorization.The refactored flow correctly validates user status and role from the cache, ensuring fresh data is used for authorization decisions. Using
userCache.WriteContext(c)to populate the context is cleaner than setting individual fields.
|
|
||
| userCache, err := model.GetUserCache(id.(int)) | ||
| if err != nil { | ||
| c.JSON(http.StatusOK, gin.H{ | ||
| "success": false, | ||
| "message": err.Error(), | ||
| }) | ||
| c.Abort() | ||
| return | ||
| } |
There was a problem hiding this comment.
Potential panic on type assertion.
Line 100 performs id.(int) which could panic if id was retrieved from the session with a different underlying type (e.g., int64 or float64 depending on session serialization). Consider using a safe type assertion or common.GetIntFromInterface() if available.
Additionally, exposing err.Error() directly to the client (line 104) may leak internal details. Consider returning a generic error message.
🔎 Proposed fix
- userCache, err := model.GetUserCache(id.(int))
+ userId, ok := id.(int)
+ if !ok {
+ c.JSON(http.StatusUnauthorized, gin.H{
+ "success": false,
+ "message": "无权进行此操作，用户ID无效",
+ })
+ c.Abort()
+ return
+ }
+ userCache, err := model.GetUserCache(userId)
if err != nil {
c.JSON(http.StatusOK, gin.H{
"success": false,
- "message": err.Error(),
+ "message": "获取用户信息失败",
})
c.Abort()
return
}🤖 Prompt for AI Agents
In middleware/auth.go around lines 99 to 108, the code does an unsafe type
assertion id.(int) which can panic if the session value is another numeric type
and it returns err.Error() to the client exposing internal details; change to a
safe conversion (use a type switch or the existing
common.GetIntFromInterface(id) helper) to obtain an int without panicking,
handle the conversion error path cleanly, and replace the client-facing message
with a generic error string (log the detailed err internally) before aborting.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.