-
Notifications
You must be signed in to change notification settings - Fork 43
RHINENG-22821: fix rbac/kessel switch #2054
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the authenticated routes middleware chain so that RBAC and Kessel are mutually exclusive, applying Kessel when enabled and RBAC otherwise to avoid conflicting access control handlers. Sequence diagram for request handling with Kessel vs RBAC middlewaresequenceDiagram
actor Client
participant GinRouter as GinRouterGroup_api
participant UserAuth as GinRouterGroup_userAuth
participant DBMW as DatabaseWithContext
participant KesselMW as Kessel
participant RBACMW as RBAC
participant PublicAuth as PublicAuthenticator
participant Handler as EndpointHandler
Client->>GinRouter: HTTP request
GinRouter->>DBMW: Apply DatabaseWithContext
DBMW-->>GinRouter: Next
GinRouter->>UserAuth: Route to userAuth group
alt KesselEnabled
UserAuth->>KesselMW: Apply Kessel
KesselMW-->>UserAuth: Next
else KesselDisabled
UserAuth->>RBACMW: Apply RBAC
RBACMW-->>UserAuth: Next
end
UserAuth->>PublicAuth: Apply PublicAuthenticator
PublicAuth-->>UserAuth: Next
UserAuth->>Handler: Invoke handler
Handler-->>Client: HTTP response
Flow diagram for InitAPI middleware selection between Kessel and RBACflowchart TD
A["InitAPI called"] --> B["Create api RouterGroup"]
B --> C["api.Use DatabaseWithContext"]
C --> D["Create userAuth = api.Group /"]
D --> E{CoreCfg.KesselEnabled?}
E -->|true| F["userAuth.Use Kessel"]
E -->|false| G["userAuth.Use RBAC"]
F --> H["userAuth.Use PublicAuthenticator"]
G --> H["userAuth.Use PublicAuthenticator"]
H --> I["Register authenticated routes on userAuth"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2054 +/- ##
==========================================
- Coverage 59.39% 59.38% -0.01%
==========================================
Files 134 134
Lines 8678 8679 +1
==========================================
Hits 5154 5154
- Misses 2977 2978 +1
Partials 547 547
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- Consider adding a brief code comment explaining that Kessel and RBAC middleware are mutually exclusive here so future changes don’t accidentally reintroduce both on the same route group.
- It may be clearer to construct the middleware slice conditionally (e.g.,
authMiddlewares := []gin.HandlerFunc{}then append either Kessel or RBAC) before applying it to the group, which can help avoid duplication if additional auth middlewares are added later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a brief code comment explaining that Kessel and RBAC middleware are mutually exclusive here so future changes don’t accidentally reintroduce both on the same route group.
- It may be clearer to construct the middleware slice conditionally (e.g., `authMiddlewares := []gin.HandlerFunc{}` then append either Kessel or RBAC) before applying it to the group, which can help avoid duplication if additional auth middlewares are added later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This is not expected to fix the issue from the ticket, I just noticed this bug during testing
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Enhancements: