Skip to content

Conversation

goginenibhavani2000
Copy link

@goginenibhavani2000 goginenibhavani2000 commented Aug 11, 2025

Description:

  • Added a declarative RBAC middleware with JSON‑defined roles, flexible role extraction, pattern matching, and helper functions enables consistent, reusable, and easy‑to‑maintain authorization across routes.
  • Addresses RBAC(Role-Based Access Control) Support #2000
  • GoFr has no native RBAC, forcing manual, repetitive route‑level authorization that risks inconsistency and increases maintenance. Expected Benefits include centralized, configurable, and reusable access control that’s easier to maintain, more flexible, and improves security.

Breaking Changes (if applicable):

  • None

Additional Information:

  • No relevant dependencies effected or external libraries used.

Checklist:

  • I have formatted my code using goimport and golangci-lint.
  • All new code is covered by unit tests.
  • This PR does not decrease the overall code coverage.
  • I have reviewed the code comments and documentation for clarity.

Copy link
Member

@coolwednesday coolwednesday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 1 to 7
{
"roles": {
"admin": ["*"],
"editor": ["/posts/*", "/dashboard"],
"user": ["/profile", "/home", "/sayhello*","/greet"]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roles should be defined at route level, not the other way around. That would allow clarity on who all can access a given route.

"strings"
)

func isPathAllowed(role, route string, config *Config) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't reinvent the path matching, utilise existing packages.

return expRole == role
}

func IsAdmin(ctx *gofr.Context) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roles should not be hardcoded into the framework. They are just strings, should not have a literal meaning defined in the framework.

return HasRole(ctx, "admin")
}

func GetUserRole(ctx *gofr.Context) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors should not be silently ignored

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it returns a bool, so have ignored it

gofrCtx := &gofr.Context{Context: baseCtx}

got := HasRole(gofrCtx, tt.checkRole)
if got != tt.expectedRes {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using assert.Equal/Equalf to make it more concise

}

func extractor(req *http.Request, _ ...any) (string, error) {
return req.Header.Get("X-USER-ROLE"), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User role passed in the header cannot be trusted as is. While this may have been added as an example only, it'd be better to have a proper example.

app.UseMiddleware(rbac.Middleware(rbacConfigs))

app.GET("/sayhello/123", handler)
app.GET("/greet", rbac.RequireRole("user1", handler))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is defeating the purpose of middleware if every route needs to add this RequireRole

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requireRole is not needed for every route, can be directly used as app.GET("/sayhello/123", handler)

@akshat-kumar-singhal
Copy link
Contributor

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?

@Umang01-hash
Copy link
Member

@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.

@Umang01-hash
Copy link
Member

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?

@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.

@akshat-kumar-singhal
Copy link
Contributor

akshat-kumar-singhal commented Aug 20, 2025 via email

@Umang01-hash
Copy link
Member

Umang01-hash commented Aug 20, 2025

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

Yes that makes sense. WE can have the interface in GoFr and implementation outside.

@Umang01-hash
Copy link
Member

@goginenibhavani2000 Are you still working on the issue. Please let us know in case you need any further help for the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants