-
Notifications
You must be signed in to change notification settings - Fork 1.7k
RBAC middleware support #2144
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
base: development
Are you sure you want to change the base?
RBAC middleware support #2144
Changes from 6 commits
9170451
a123a03
e59fd18
993dd2e
23073f7
0a0334f
2cb75bf
21b6309
c57a2e1
0f9226d
03b7ab9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"roles": { | ||
"admin": ["*"], | ||
"editor": ["/posts/*", "/dashboard"], | ||
"user": ["/profile", "/home", "/sayhello*","/greet"] | ||
} | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package main | ||
|
||
import ( | ||
"net/http" | ||
|
||
"gofr.dev/pkg/gofr" | ||
"gofr.dev/pkg/gofr/rbac" | ||
) | ||
|
||
func main() { | ||
app := gofr.New() | ||
|
||
// json config path file is required | ||
rbacConfigs, err := rbac.LoadPermissions("config.json") | ||
if err != nil { | ||
return | ||
} | ||
|
||
overrides := map[string]bool{"user1": true} | ||
|
||
rbacConfigs.OverRides = overrides | ||
|
||
rbacConfigs.RoleExtractorFunc = extractor | ||
|
||
app.UseMiddleware(rbac.Middleware(rbacConfigs)) | ||
|
||
app.GET("/sayhello/123", handler) | ||
app.GET("/greet", rbac.RequireRole("user1", handler)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
||
|
||
app.Run() // listens and serves on localhost:8000 | ||
} | ||
|
||
func extractor(req *http.Request, _ ...any) (string, error) { | ||
return req.Header.Get("X-USER-ROLE"), nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
func handler(ctx *gofr.Context) (any, error) { | ||
return "Hello World!", nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package rbac | ||
|
||
import ( | ||
"encoding/json" | ||
"net/http" | ||
"os" | ||
) | ||
|
||
type Config struct { | ||
RoleWithPermissions map[string][]string `json:"roles"` // Role: [Allowed routes] | ||
RoleExtractorFunc func(req *http.Request, args ...any) (string, error) | ||
OverRides map[string]bool // role: [override bool] | ||
} | ||
|
||
func LoadPermissions(path string) (*Config, error) { | ||
data, err := os.ReadFile(path) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
var config Config | ||
|
||
if err := json.Unmarshal(data, &config); err != nil { | ||
return nil, err | ||
} | ||
|
||
return &config, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
package rbac | ||
|
||
import ( | ||
"encoding/json" | ||
"os" | ||
"path/filepath" | ||
"reflect" | ||
"testing" | ||
) | ||
|
||
// Helper function to create a temporary JSON file for testing. | ||
func createTempJSONFile(t *testing.T, data any) string { | ||
t.Helper() | ||
dir := t.TempDir() | ||
file := filepath.Join(dir, "test.json") | ||
|
||
jsonBytes, err := json.Marshal(data) | ||
if err != nil { | ||
t.Fatalf("error marshaling: %v", err) | ||
} | ||
|
||
if err := os.WriteFile(file, jsonBytes, 0600); err != nil { | ||
t.Fatalf("error writing file: %v", err) | ||
} | ||
|
||
return file | ||
} | ||
|
||
func TestLoadPermissions_Success(t *testing.T) { | ||
expected := Config{ | ||
RoleWithPermissions: map[string][]string{ | ||
"admin": {"/admin", "/dashboard"}, | ||
"viewer": {"/dashboard"}, | ||
}, | ||
} | ||
file := createTempJSONFile(t, struct { | ||
RoleWithPermissions map[string][]string `json:"roles"` | ||
}{ | ||
RoleWithPermissions: expected.RoleWithPermissions, | ||
}) | ||
|
||
got, err := LoadPermissions(file) | ||
|
||
if err != nil { | ||
t.Fatalf("LoadPermissions returned error: %v", err) | ||
} | ||
|
||
if !reflect.DeepEqual(got.RoleWithPermissions, expected.RoleWithPermissions) { | ||
t.Errorf("RoleWithPermissions mismatch: got %v, want %v", got.RoleWithPermissions, expected.RoleWithPermissions) | ||
} | ||
|
||
if !reflect.DeepEqual(got.OverRides, expected.OverRides) { | ||
t.Errorf("OverRides mismatch: got %v, want %v", got.OverRides, expected.OverRides) | ||
} | ||
} | ||
|
||
func TestLoadPermissions_FileNotFound(t *testing.T) { | ||
// Act | ||
_, err := LoadPermissions("nonexistentpath.json") | ||
// Assert | ||
if err == nil { | ||
t.Fatalf("expected error for missing file, got nil") | ||
} | ||
} | ||
|
||
func TestLoadPermissions_InvalidJSON(t *testing.T) { | ||
dir := t.TempDir() | ||
file := filepath.Join(dir, "bad.json") | ||
|
||
// Write invalid JSON | ||
if err := os.WriteFile(file, []byte("{invalid json"), 0600); err != nil { | ||
t.Fatalf("could not write test file: %v", err) | ||
} | ||
|
||
_, err := LoadPermissions(file) | ||
if err == nil { | ||
t.Fatalf("expected JSON unmarshal error, got nil") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package rbac | ||
|
||
import "gofr.dev/pkg/gofr" | ||
|
||
func HasRole(ctx *gofr.Context, role string) bool { | ||
expRole, _ := ctx.Context.Value(userRole).(string) | ||
return expRole == role | ||
} | ||
|
||
func IsAdmin(ctx *gofr.Context) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errors should not be silently ignored There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it returns a bool, so have ignored it |
||
role, _ := ctx.Context.Value(userRole).(string) | ||
return role | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
package rbac | ||
|
||
import ( | ||
"context" | ||
"testing" | ||
|
||
"gofr.dev/pkg/gofr" | ||
) | ||
|
||
func TestHasRole(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
ctxRoleVal string | ||
checkRole string | ||
expectedRes bool | ||
}{ | ||
{"matching role", "admin", "admin", true}, | ||
{"non-matching role", "viewer", "admin", false}, | ||
{"empty role in context", "", "admin", false}, | ||
{"nil role in context", "", "", true}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
// Create base context with the userRole value | ||
baseCtx := context.WithValue(t.Context(), userRole, tt.ctxRoleVal) | ||
|
||
// Wrap baseCtx in gofr.Context | ||
gofrCtx := &gofr.Context{Context: baseCtx} | ||
|
||
got := HasRole(gofrCtx, tt.checkRole) | ||
if got != tt.expectedRes { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using assert.Equal/Equalf to make it more concise |
||
t.Errorf("HasRole() = %v, want %v", got, tt.expectedRes) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestIsAdmin(t *testing.T) { | ||
baseCtx := context.WithValue(t.Context(), userRole, "admin") | ||
gofrCtx := &gofr.Context{Context: baseCtx} | ||
|
||
if !IsAdmin(gofrCtx) { | ||
t.Errorf("IsAdmin() = false, want true") | ||
} | ||
|
||
nonAdminCtx := &gofr.Context{Context: context.WithValue(t.Context(), userRole, "viewer")} | ||
if IsAdmin(nonAdminCtx) { | ||
t.Errorf("IsAdmin() = true, want false") | ||
} | ||
} | ||
|
||
func TestGetUserRole(t *testing.T) { | ||
expectedRole := "editor" | ||
baseCtx := context.WithValue(t.Context(), userRole, expectedRole) | ||
gofrCtx := &gofr.Context{Context: baseCtx} | ||
|
||
if role := GetUserRole(gofrCtx); role != expectedRole { | ||
t.Errorf("GetUserRole() = %v, want %v", role, expectedRole) | ||
} | ||
|
||
// Test no role set should return "" | ||
emptyCtx := &gofr.Context{Context: t.Context()} | ||
if role := GetUserRole(emptyCtx); role != "" { | ||
t.Errorf("GetUserRole() with no role = %v, want empty string", role) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
package rbac | ||
|
||
import ( | ||
"path" | ||
"strings" | ||
) | ||
|
||
func isPathAllowed(role, route string, config *Config) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't reinvent the path matching, utilise existing packages. |
||
allowedPaths := config.RoleWithPermissions[role] | ||
|
||
for _, pattern := range allowedPaths { | ||
// Allow simple wildcard "*" | ||
if pattern == "*" { | ||
return true | ||
} | ||
|
||
// Ensure pattern ends with * if it's a prefix match | ||
if pattern == route { | ||
return true | ||
} | ||
// Normalize pattern and path to avoid trailing slash issues | ||
normalizedPattern := strings.TrimSuffix(pattern, "/") | ||
normalizedPath := strings.TrimSuffix(route, "/") | ||
|
||
// Allow matching wildcard like /admin/* or /users/* | ||
if ok, _ := path.Match(normalizedPattern, normalizedPath); ok { | ||
return true | ||
} | ||
|
||
// Support prefix match with * (e.g. /users/* should match /users/123) | ||
if strings.HasSuffix(pattern, "*") { | ||
prefix := strings.TrimSuffix(pattern, "*") | ||
if strings.HasPrefix(route, prefix) { | ||
return true | ||
} | ||
} | ||
} | ||
|
||
// override with role match | ||
if config.OverRides[role] { | ||
return true | ||
} | ||
|
||
return false | ||
} |
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.
Roles should be defined at route level, not the other way around. That would allow clarity on who all can access a given route.