RBAC middleware support #2144
RBAC middleware support #2144
Conversation
coolwednesday
left a comment
coolwednesday
left a comment
There was a problem hiding this comment.
We would like to keep RBAC as a separate module. Hence add go.mod in it. So that it does not get added in the binary of the gofr framework.
@coolwednesday What's the advantage of having RBAC in another module, unless we move the auth middlewares into a separate module as well? |
|
@goginenibhavani2000 please go ahead and resolve the review comments given by the reviewer. While we decide whether to keep it inside or outside like a separate module i think we can focus on completing the implementation, testing, documentation etc. Please let us know if you know any more help. |
@akshat-kumar-singhal We see RBAC as a layer beneath Auth—more specialized and not universally required. Auth is core to GoFr and widely used, which is why its middlewares are bundled. RBAC, on the other hand, is optional and domain-specific, so we’re keeping it modular to avoid unnecessary coupling and binary weight. |
|
Won’t it be taken care of by the dead code elimination done during build?
I agree that RBAC shouldn’t be a default, but an easy to include
module/package. What we could consider is having the interface for RBAC
within gofr and the external module implementing that interface.
…On Wed, 20 Aug 2025 at 16:58, Umang Mundhra ***@***.***> wrote:
*Umang01-hash* left a comment (gofr-dev/gofr#2144)
<#2144 (comment)>
We would like to keep RBAC as a separate module. Hence add go.mod in it.
So that it does not get added in the binary of the gofr framework.
@coolwednesday <https://github.com/coolwednesday> What's the advantage of
having RBAC in another module, unless we move the auth middlewares into a
separate module as well?
@akshat-kumar-singhal <https://github.com/akshat-kumar-singhal> We see
RBAC as a layer beneath Auth—more specialized and not universally required.
Auth is core to GoFr and widely used, which is why its middlewares are
bundled. RBAC, on the other hand, is optional and domain-specific, so we’re
keeping it modular to avoid unnecessary coupling and binary weight.
—
Reply to this email directly, view it on GitHub
<#2144 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/APUGM5WLZAMQTTVY4YHXRRD3ORLXVAVCNFSM6AAAAACDUPRBZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEMBVG44DMNJTHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes that makes sense. WE can have the interface in GoFr and implementation outside. |
|
@goginenibhavani2000 Are you still working on the issue. Please let us know in case you need any further help for the same. |
Yes, i have completed the suggested changes except the change in having interface. Would like to confirm if the RBAC interface is like below type RBAC interface { |
|
@goginenibhavani2000, So Sorry for the delay. I will be prioritising this issue review for the next week's release. Let me know if you are interested to tale this further if not, I myself will pick up this issue. |
|
@coolwednesday I’d be happy to take this further and help with the changes. Let me know if there's anything specific you'd like me to focus on. |
…BAC-middleware-support
Umang01-hash
left a comment
Umang01-hash
left a comment
There was a problem hiding this comment.
- In all the examples added, if the handlers are not using the ctx so we can keep the signature like :
func handlerName(_ *gofr.Context){}
- Files like
adapter.go,jwt_extractor.gohave very less coverage. Can we please try and improve it?
aryanmehrotra
left a comment
aryanmehrotra
left a comment
There was a problem hiding this comment.
code needs to be simplified, once the structural fixes are done code needs to be reviewed again.
| config, err := rbac.LoadPermissions("configs/rbac.json") | ||
| if err != nil { | ||
| app.Logger().Error("Failed to load RBAC config: ", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
we can do this internally by just taking the file path and also have a default file path so user need to give path unless he gives something different
| "users:read": {"admin", "editor", "viewer"}, | ||
| "users:write": {"admin", "editor"}, | ||
| "users:delete": {"admin"}, | ||
| "posts:read": {"admin", "author", "viewer"}, | ||
| "posts:write": {"admin", "author"}, |
There was a problem hiding this comment.
example doesnt seem correct, why author??
also should this not be
admin : users:read, users:write etc etc
to figure out what an admin can do need to look in multiple places
need to think this through what can be a better way
| gofr.WithRoleExtractor(func(req *http.Request, args ...any) (string, error) { | ||
| role := req.Header.Get("X-User-Role") | ||
| if role == "" { | ||
| return "", fmt.Errorf("role header not found") | ||
| } | ||
| return role, nil |
There was a problem hiding this comment.
why not just take the key? rather than giving user access the complete request? should the role come from anywhere apart from X-User-Role
Also is there any standardised guide which we are following for rbac support? if so we should mention that
| GoFr's RBAC middleware implements a **two-tier authorization system**: | ||
|
|
||
| 1. **Permission-Based Check** (Primary) - Checks if user's role has the required permission | ||
| 2. **Role-Based Check** (Fallback) - Falls back to route-based role checking if permission check fails | ||
|
|
||
| ```go | ||
| // Check permission-based access if enabled | ||
| if config.EnablePermissions && config.PermissionConfig != nil { | ||
| if err := CheckPermission(reqWithRole, config.PermissionConfig); err == nil { | ||
| authorized = true | ||
| authReason = "permission-based" | ||
| } | ||
| } | ||
|
|
||
| // Check role-based access (if not already authorized by permissions) | ||
| if !authorized { | ||
| if isRoleAllowed(role, route, config) { | ||
| authorized = true | ||
| authReason = "role-based" | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
can the user make any changes to it?? if cannot what benefit do we get by keeping this in documentation? how is it beneficial to the users of the framework?
| 2. **Permission check** → Checks if route has a permission mapping | ||
| 3. **Role validation** → Verifies if user's role has the required permission | ||
| 4. **Fallback** → If no permission mapping exists, falls back to role-based check |
There was a problem hiding this comment.
what is the benefit of having the fallback? can't we already do the role-based check in just single operation?
| // Array notation - extract first role | ||
| app.EnableRBAC( | ||
| gofr.WithPermissionsFile("configs/rbac.json"), | ||
| gofr.WithJWT("roles[0]"), |
There was a problem hiding this comment.
user role would either be one or many ideally based on implementation, in which case would the user want to get only one role? instead of all and then validate, if required role is there or not.
| | **Security** | ⚠️ Low | ✅ High | ⚠️ Low | ✅ High | ✅ High | | ||
| | **Flexibility** | ⚠️ Low | ⚠️ Low | ✅ High | ✅ High | ✅✅ Very High | | ||
| | **Performance** | ✅ Fast | ✅ Fast | ✅ Fast | ✅ Fast | ⚠️ Slower* | | ||
| | **Dynamic Roles** | ❌ No | ❌ No | ❌ No | ❌ No | ✅ Yes | |
There was a problem hiding this comment.
even without giving db access dynamic roles can still be set, depends on how its implmented
| } | ||
|
|
||
| // RBACOption is a function that configures RBACOptions. | ||
| type RBACOption func(*RBACOptions) |
There was a problem hiding this comment.
why not use an interface, similar to options in http service
| func (*auditLogger) logAccess(logger Logger, req *http.Request, role, route string, allowed bool, reason string) { | ||
| if logger == nil { | ||
| return // Skip logging if no logger provided | ||
| } | ||
|
|
||
| status := "denied" | ||
| if allowed { | ||
| status = "allowed" | ||
| } | ||
|
|
||
| logger.Infof("[RBAC Audit] %s %s - Role: %s - Route: %s - %s - Reason: %s", | ||
| req.Method, req.URL.Path, role, route, status, reason) | ||
| } |
There was a problem hiding this comment.
why not just keep it by default? without creating this function, what benefit it gives inspite of not logging? and should we not by default log it may be debug level?
|
|
||
| // _ is a package-level variable that triggers registration when the package is imported. | ||
| // This avoids using init() while maintaining automatic registration behavior. | ||
| var _ = registerRBAC() |
5 participants
Description:
Breaking Changes (if applicable):
Additional Information:
Checklist:
goimportandgolangci-lint.