Conversation
|
@coderabbitai review this pr. |
|
@tbphp: I'll review the PR now. Let me analyze the changes and provide feedback. ✅ Actions performedReview triggered.
|
WalkthroughAdds a new integration info endpoint and supporting types, introduces a middleware dispatcher to route integration-info requests before proxy auth, and updates router wiring to register the new endpoint and integrate the dispatcher within group-aware proxy routes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router
participant ServerHandler as ServerHandler
participant GroupMgr as GroupManager/Service
note over Client,Router: Public integration info endpoint
Client->>Router: GET /integration/info?proxy_key=...
Router->>ServerHandler: GetIntegrationInfo(ctx)
ServerHandler->>GroupMgr: Resolve scope (group/all), list groups
ServerHandler->>GroupMgr: Check permissions per group
ServerHandler-->>Router: 200 {data:[IntegrationGroupInfo...]} or 4xx/5xx
Router-->>Client: Response
sequenceDiagram
autonumber
actor Client
participant Router
participant Middleware as ProxyRouteDispatcher
participant ServerHandler as ServerHandler
participant Proxy as Proxy Handler
note over Client,Middleware: Proxy path with dispatcher
Client->>Router: GET /proxy/:group_name/*path
Router->>Middleware: Invoke
alt path == "/api/integration/info"
Middleware->>ServerHandler: GetIntegrationInfo(ctx)
Middleware-->>Client: Response (aborts chain)
else Forward
Middleware->>Proxy: Next handler
Proxy-->>Client: Proxied response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/middleware/middleware.go (1)
174-185: Guard dispatcher by HTTP method to avoid intercepting non-GET requestsCurrently any verb to /proxy/:group_name/api/integration/info is intercepted. Restrict to GET for consistency with the public route.
func ProxyRouteDispatcher(serverHandler interface{ GetIntegrationInfo(*gin.Context) }) gin.HandlerFunc { return func(c *gin.Context) { - if c.Param("path") == "/api/integration/info" { + if c.Request.Method == "GET" && c.Param("path") == "/api/integration/info" { serverHandler.GetIntegrationInfo(c) c.Abort() return } c.Next() } }internal/handler/integration_handler.go (3)
36-48: Prefer c.Param("group_name") when available; fall back to path parsingMore robust in proxy context and avoids string splitting pitfalls.
- path := c.Request.URL.Path - isGroupSpecific := strings.HasPrefix(path, "/proxy/") + path := c.Request.URL.Path + isGroupSpecific := strings.HasPrefix(path, "/proxy/") @@ - if isGroupSpecific { - parts := strings.Split(strings.TrimPrefix(path, "/proxy/"), "/") - if len(parts) == 0 || parts[0] == "" { + if isGroupSpecific { + groupName := c.Param("group_name") + if groupName == "" { + parts := strings.Split(strings.TrimPrefix(path, "/proxy/"), "/") + if len(parts) == 0 || parts[0] == "" { + response.Error(c, app_errors.NewAPIError(app_errors.ErrValidation, "Invalid group path")) + return + } + groupName = parts[0] + } - } - - groupName := parts[0] + // groupName now set
21-26: Remove unused IntegrationInfoResponse typeIt’s not used; keep code lean.
130-143: Return consistent absolute paths to simplify clientsIn group-specific cases, returning "" (openai) or a bare validationEndpoint (custom) is inconsistent. Recommend always returning an absolute path clients can call directly.
-func buildPath(isGroupSpecific bool, groupName string, channelType string, validationEndpoint string) string { - if channelType == "custom" { - if isGroupSpecific { - return validationEndpoint - } - return "/proxy/" + groupName + validationEndpoint - } - - if isGroupSpecific { - return "" - } - return "/proxy/" + groupName -} +func buildPath(isGroupSpecific bool, groupName string, channelType string, validationEndpoint string) string { + base := "/proxy/" + groupName + if channelType == "custom" && validationEndpoint != "" { + return base + validationEndpoint + } + return base +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/handler/integration_handler.go(1 hunks)internal/middleware/middleware.go(1 hunks)internal/router/router.go(3 hunks)
🔇 Additional comments (3)
internal/router/router.go (2)
103-104: Public endpoint addition looks goodExposing GET /api/integration/info on the public API is consistent with the handler’s auth model.
172-184: Confirm middleware order is intentional (dispatcher before ProxyAuth)Placing ProxyRouteDispatcher before ProxyAuth bypasses proxy auth for this one route and relies on the handler’s key check. Confirm this is desired and covered in threat modeling.
internal/handler/integration_handler.go (1)
90-96: Consider ErrUnauthorized for invalid keyThe “invalid or unauthorized proxy key” case may better map to ErrUnauthorized than ErrValidation. Align with API semantics.
关联 Issue / Related Issue
Closes #
变更内容 / Change Content
自查清单 / Checklist
Summary by CodeRabbit