diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index 7a5d96c86a..6f2a6445ec 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -206,6 +206,89 @@ For endpoints that need to match multiple paths, use mux patterns: - **Middle variable**: Use `"/api/{category}/posts"` instead of `"/api/*/posts"` - Matches: `/api/tech/posts`, `/api/news/posts` +### Rule Resolution + +More than one endpoint entry can match the same request — a broad pattern and a narrow one, for +example. GoFr resolves this by **most specific wins**, rather than by the order the entries appear +in the config file. + +Specificity is compared segment by segment, and the first segment where two patterns differ decides: + +1. A literal segment (`users`) is more specific than +2. a constrained variable (`{id:[0-9]+}`), which is more specific than +3. a free variable (`{id}`), which is more specific than +4. a multi-segment catch-all (`{path:.*}`). + +If the paths are equally specific, an entry that names its methods explicitly wins over one +declared with `["*"]`. + +```json +{ + "endpoints": [ + { + "path": "/admin/{path:.*}", + "methods": ["*"], + "requiredPermissions": ["admin:read"] + }, + { + "path": "/admin/orgs/{org_id}", + "methods": ["DELETE"], + "requiredPermissions": ["admin:write"] + } + ] +} +``` + +`DELETE /admin/orgs/123` matches both entries, and the second one governs it — so `admin:write` is +required. `GET /admin/settings` matches only the first, so `admin:read` is required. + +`GET /admin/orgs/123` is the case to watch. The narrow entry is `["DELETE"]`-only, so it does not +cover a GET at all and drops out on method before specificity is ever considered — the catch-all +governs, and the request needs only `admin:read`. Writing a strict rule for one method does not +protect the other methods on that path; each method needs its own entry, or the broad entry has to +be strict enough to stand on its own. + +> **Note**: `"methods": ["*"]` — and omitting `methods` entirely, which means the same thing — +> matches **every** HTTP method, including methods GoFr does not otherwise know about. Since an +> entry states what a caller must have in order to be let through, covering an unrecognized method +> tightens enforcement rather than relaxing it. + +#### Where the ordering is not decided + +**Two patterns that score identically** — `/{a}/{b}` and `/{x}/{y}` are the same shape, so nothing +about the patterns separates them. One thing still does: an entry that **requires permissions wins +over a public one**, because a tie is a config that did not express an intent either way and +enforcing is the recoverable half of that mistake. Past that, the first entry declared wins. Avoid +writing two entries that overlap without one being plainly narrower than the other. + +**A constraint that can span segments** — a constraint containing `/`, such as `{path:[a-z/]+}`, +matches `/files/a/b/c` the way a catch-all does, so it is scored as one: least specific, losing to +any narrower entry it overlaps. That is decided by the `/` appearing in the constraint at all, not +by whether the regex can really produce one — `{id:[0-9]+/}` is scored as a catch-all even though +it is anchored to a single segment. The effect is only ever to move an entry *down* the ordering, +so it can lose to a narrower rule but never shadow one. Write multi-segment matches as `{path:.*}` +or `{path:.+}` and the scoring is exact. + +#### Declaring the same path twice + +Two entries with the same method and path are one entry: **the last one declared wins, in full**. +That includes the `public` flag — a public entry followed by a protected one for the same key is +protected, not public. Duplicates are not rejected, so it is worth checking for them in a config +assembled from more than one source. + +#### A pattern that cannot compile + +If a pattern's constraint is not valid regex — `{id:[}`, for example — the config still loads and +the application still starts, but the pattern can never match a request, so **that endpoint is not +enforced**. GoFr logs the pattern at error level on startup: + +``` +RBAC: endpoint[2]: invalid mux pattern: "/api/{id:[}": ... This endpoint will never match a +request, so it is NOT enforced - any route it was meant to govern is currently unguarded. +``` + +Treat that line as an open route, not a warning about a typo. + ## JWT-Based RBAC For production/public APIs, use JWT-based role extraction: @@ -465,9 +548,18 @@ Or use role inheritance to avoid duplication: **Route not being protected by RBAC** - Verify the route is explicitly configured in `endpoints[]` array - Check that the path pattern matches exactly (case-sensitive) -- Ensure HTTP method matches (or use `["*"]` for all methods) +- Ensure HTTP method matches (or use `["*"]` for all methods) — a rule declared for one method does + not cover the others on that path +- Check the startup logs for `invalid mux pattern` — a pattern whose constraint is not valid regex + loads but never matches, leaving that endpoint unenforced - Remember: Routes not in RBAC config are allowed to proceed (not blocked) +**The wrong permission is being required on a path two entries match** +- The more specific pattern governs — see [Rule Resolution](#rule-resolution) +- Check whether the narrower entry actually covers the request's method; if it does not, it drops + out and the broader entry applies +- If both patterns are equally specific, declaration order decides — rewrite one to be narrower + ## How It Works 1. **Role Extraction**: Extracts user role from header (`X-User-Role`) or JWT claims diff --git a/pkg/gofr/rbac/config.go b/pkg/gofr/rbac/config.go index 8edad15402..9bb8538b24 100644 --- a/pkg/gofr/rbac/config.go +++ b/pkg/gofr/rbac/config.go @@ -125,6 +125,10 @@ type Config struct { publicEndpointsMap map[string]bool `json:"-" yaml:"-"` // Key: "METHOD:/path", Value: true if public endpointMap map[string]*EndpointMapping `json:"-" yaml:"-"` // Key: "METHOD:/path", Value: endpoint object muxRouter *mux.Router `json:"-" yaml:"-"` // Used for mux pattern matching + + // rules holds every (method, path) rule ordered most-specific-first, and is the + // single source of truth for resolving a request that no exact key matches. + rules []endpointRule `json:"-" yaml:"-"` } // LoadPermissions loads RBAC configuration from a JSON or YAML file. @@ -221,6 +225,9 @@ func (c *Config) validateEndpointPath(path string, index int) error { if err := validateMuxPattern(path); err != nil { return fmt.Errorf("endpoint[%d]: invalid mux pattern: %w", index, err) } + + // Non-fatal: a pattern mux cannot compile loads, but never matches, so say so loudly. + c.logUncompilablePattern(path, index) } return nil @@ -289,19 +296,24 @@ func (c *Config) buildRolePermissionsMap() { } } -// buildEndpointPermissionMap builds the endpoint permission map from Endpoints. +// buildEndpointPermissionMap builds the endpoint permission map and the ordered rule list +// from Endpoints. func (c *Config) buildEndpointPermissionMap() error { - for _, endpoint := range c.Endpoints { + for i := range c.Endpoints { + endpoint := &c.Endpoints[i] + methods := endpoint.Methods if len(methods) == 0 { methods = []string{"*"} } - if err := c.processEndpointMethods(&endpoint, methods); err != nil { + if err := c.processEndpointMethods(endpoint, methods); err != nil { return err } } + c.rules = buildEndpointRules(c.Endpoints) + return nil } @@ -326,16 +338,24 @@ func buildEndpointKey(endpoint *EndpointMapping, methodUpper string) string { return fmt.Sprintf("%s:%s", methodUpper, pattern) } -// storeEndpointMapping stores an endpoint mapping. +// storeEndpointMapping stores an endpoint mapping. A duplicate (method, path) declaration +// overwrites the earlier one entirely - including the public flag and the permission list, which +// live in separate maps. Leaving either behind would mix the two declarations together: a public +// entry followed by a protected one for the same key would keep serving the route unauthenticated +// while reporting the protected endpoint. func (c *Config) storeEndpointMapping(endpoint *EndpointMapping, key, methodUpper string) error { // Store endpoint object for fast lookup c.endpointMap[key] = endpoint if endpoint.Public { c.publicEndpointsMap[key] = true + delete(c.endpointPermissionMap, key) + return nil } + delete(c.publicEndpointsMap, key) + if len(endpoint.RequiredPermissions) == 0 { return fmt.Errorf("%w: %s %s", ErrEndpointMissingPermissions, methodUpper, endpoint.Path) } @@ -393,16 +413,15 @@ func (c *Config) GetRolePermissions(role string) []string { // Returns all required permissions (user needs ANY of them - OR logic). // Config is read-only after initialization, so no mutex is needed. func (c *Config) GetEndpointPermission(method, path string) ([]string, bool) { - methodUpper := strings.ToUpper(method) - key := fmt.Sprintf("%s:%s", methodUpper, path) - - // Try exact match first - if perms, isPublic := c.checkExactMatch(key); isPublic || len(perms) > 0 { - return perms, isPublic + endpoint, isPublic := c.resolve(strings.ToUpper(method), path) + if endpoint == nil || isPublic { + return nil, isPublic } - // Try pattern and regex matching - return c.checkPatternMatch(methodUpper, path) + permissions := make([]string, len(endpoint.RequiredPermissions)) + copy(permissions, endpoint.RequiredPermissions) + + return permissions, false } // getExactEndpoint returns the endpoint for an exact key match (O(1) lookup). @@ -415,71 +434,3 @@ func (c *Config) getExactEndpoint(key string) (*EndpointMapping, bool) { return nil, false } - -// checkExactMatch checks for an exact endpoint match. -func (c *Config) checkExactMatch(key string) (permissions []string, isPublic bool) { - if public, ok := c.publicEndpointsMap[key]; ok && public { - return nil, true - } - - if perms, ok := c.endpointPermissionMap[key]; ok { - return perms, false - } - - return nil, false -} - -// findEndpointByPattern finds an endpoint by pattern matching (wildcards/regex). -// Only used when exact match fails, so this is O(n) but only for patterns. -// Config is read-only after initialization, so no mutex is needed. -func (c *Config) findEndpointByPattern(methodUpper, path string) (*EndpointMapping, bool) { - // Try pattern matching for wildcards/regex - // Iterate over endpointMap to find matching patterns - for key, endpoint := range c.endpointMap { - if c.matchesKey(key, methodUpper, path) { - isPublic := c.publicEndpointsMap[key] - return endpoint, isPublic - } - } - - return nil, false -} - -// checkPatternMatch checks for pattern and regex matches. -// Config is read-only after initialization, so no mutex is needed. -func (c *Config) checkPatternMatch(methodUpper, path string) (permissions []string, isPublic bool) { - // Try pattern matching for wildcards - for key, perms := range c.endpointPermissionMap { - if c.matchesKey(key, methodUpper, path) { - return perms, false - } - } - - // Check public endpoints with pattern/regex - for key := range c.publicEndpointsMap { - if c.matchesKey(key, methodUpper, path) { - return nil, true - } - } - - return nil, false -} - -// matchesKey checks if a key matches the given method and path. -// Keys are built by buildEndpointKey which uses Path (may contain mux patterns). -// Uses mux Route.Match() for mux patterns, exact match for non-pattern paths. -func (c *Config) matchesKey(key, methodUpper, path string) bool { - if !strings.HasPrefix(key, methodUpper+":") { - return false - } - - pattern := strings.TrimPrefix(key, methodUpper+":") - - // For exact paths (no variables), use string comparison - if !isMuxPattern(pattern) { - return pattern == path - } - - // For mux patterns, use Route.Match() from endpoint_matcher - return matchMuxPattern(pattern, methodUpper, path, c.muxRouter) -} diff --git a/pkg/gofr/rbac/config_test.go b/pkg/gofr/rbac/config_test.go index 26c79f69e3..437efb553a 100644 --- a/pkg/gofr/rbac/config_test.go +++ b/pkg/gofr/rbac/config_test.go @@ -384,7 +384,7 @@ func createTestConfigFile(filename, content string) (string, error) { return filename, err } -func TestConfig_FindEndpointByPattern(t *testing.T) { +func TestConfig_Resolve(t *testing.T) { t.Run("finds endpoint with mux pattern", func(t *testing.T) { config := &Config{ Endpoints: []EndpointMapping{ @@ -394,7 +394,7 @@ func TestConfig_FindEndpointByPattern(t *testing.T) { err := config.processUnifiedConfig() require.NoError(t, err) - endpoint, isPublic := config.findEndpointByPattern("GET", "/api/users") + endpoint, isPublic := config.resolve("GET", "/api/users") assert.NotNil(t, endpoint) assert.Equal(t, "/api/{resource}", endpoint.Path) assert.False(t, isPublic) @@ -409,7 +409,7 @@ func TestConfig_FindEndpointByPattern(t *testing.T) { err := config.processUnifiedConfig() require.NoError(t, err) - endpoint, isPublic := config.findEndpointByPattern("GET", "/api/users/123") + endpoint, isPublic := config.resolve("GET", "/api/users/123") assert.NotNil(t, endpoint) assert.False(t, isPublic) }) @@ -423,7 +423,7 @@ func TestConfig_FindEndpointByPattern(t *testing.T) { err := config.processUnifiedConfig() require.NoError(t, err) - endpoint, isPublic := config.findEndpointByPattern("GET", "/public/files") + endpoint, isPublic := config.resolve("GET", "/public/files") assert.NotNil(t, endpoint) assert.True(t, isPublic) }) @@ -437,7 +437,7 @@ func TestConfig_FindEndpointByPattern(t *testing.T) { err := config.processUnifiedConfig() require.NoError(t, err) - endpoint, isPublic := config.findEndpointByPattern("GET", "/other/path") + endpoint, isPublic := config.resolve("GET", "/other/path") assert.Nil(t, endpoint) assert.False(t, isPublic) }) @@ -451,92 +451,12 @@ func TestConfig_FindEndpointByPattern(t *testing.T) { err := config.processUnifiedConfig() require.NoError(t, err) - endpoint, isPublic := config.findEndpointByPattern("POST", "/api/users") + endpoint, isPublic := config.resolve("POST", "/api/users") assert.Nil(t, endpoint) assert.False(t, isPublic) }) } -func TestConfig_MatchesKey(t *testing.T) { - t.Run("matches exact path", func(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/users", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}, - }, - } - err := config.processUnifiedConfig() - require.NoError(t, err) - - result := config.matchesKey("GET:/api/users", "GET", "/api/users") - assert.True(t, result) - }) - - t.Run("matches mux pattern", func(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/{resource}", Methods: []string{"GET"}, RequiredPermissions: []string{"api:read"}}, - }, - } - err := config.processUnifiedConfig() - require.NoError(t, err) - - result := config.matchesKey("GET:/api/{resource}", "GET", "/api/users") - assert.True(t, result) - }) - - t.Run("matches mux pattern with constraint", func(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/users/{id:[0-9]+}", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}, - }, - } - err := config.processUnifiedConfig() - require.NoError(t, err) - - result := config.matchesKey("GET:/api/users/{id:[0-9]+}", "GET", "/api/users/123") - assert.True(t, result) - }) - - t.Run("matches mux pattern with constraint", func(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/test/{id:[0-9]+}", Methods: []string{"GET"}, RequiredPermissions: []string{"test:read"}}, - }, - } - err := config.processUnifiedConfig() - require.NoError(t, err) - - result := config.matchesKey("GET:/test/{id:[0-9]+}", "GET", "/test/456") - assert.True(t, result) - }) - - t.Run("returns false when method doesn't match", func(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/users", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}, - }, - } - err := config.processUnifiedConfig() - require.NoError(t, err) - - result := config.matchesKey("GET:/api/users", "POST", "/api/users") - assert.False(t, result) - }) - - t.Run("returns false for invalid mux pattern", func(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/invalid{", Methods: []string{"GET"}, RequiredPermissions: []string{"test:read"}}, - }, - } - err := config.processUnifiedConfig() - require.NoError(t, err) - - result := config.matchesKey("GET:/api/invalid{", "GET", "/test") - assert.False(t, result) - }) -} - func TestConfig_getEffectivePermissions(t *testing.T) { testCases := []struct { desc string diff --git a/pkg/gofr/rbac/endpoint_matcher.go b/pkg/gofr/rbac/endpoint_matcher.go index 2bd3833d52..d788660801 100644 --- a/pkg/gofr/rbac/endpoint_matcher.go +++ b/pkg/gofr/rbac/endpoint_matcher.go @@ -25,37 +25,10 @@ const ( var ( // errUnbalancedBraces is returned when a mux pattern has unbalanced braces. errUnbalancedBraces = errors.New("unbalanced braces in pattern") -) - -// matchEndpoint checks if the request matches an endpoint configuration. -// This is the primary authorization check using the unified Endpoints configuration. -// Returns the matched endpoint and whether it's public. -func matchEndpoint(method, route string, endpoints []EndpointMapping, config *Config) (*EndpointMapping, bool) { - for i := range endpoints { - endpoint := &endpoints[i] - - // Check if endpoint is public - if endpoint.Public { - if matchesEndpointPattern(endpoint, route, config) { - return endpoint, true - } - - continue - } - // Check method match - if !matchesHTTPMethod(method, endpoint.Methods) { - continue - } - - // Check route match - if matchesEndpointPattern(endpoint, route, config) { - return endpoint, false - } - } - - return nil, false -} + // errInvalidPattern is returned when mux cannot compile a pattern. + errInvalidPattern = errors.New("invalid mux pattern") +) // matchesHTTPMethod checks if the HTTP method matches the endpoint's allowed methods. func matchesHTTPMethod(method string, allowedMethods []string) bool { @@ -111,6 +84,9 @@ func matchMuxPattern(pattern, method, path string, router *mux.Router) bool { // validateMuxPattern validates mux pattern syntax. // Ensures balanced braces and validates regex constraints format. +// +// Whether mux can actually compile the pattern is checked separately and non-fatally by +// logUncompilablePattern - see the note there. func validateMuxPattern(pattern string) error { // Check for balanced braces openCount := strings.Count(pattern, "{") @@ -127,13 +103,32 @@ func validateMuxPattern(pattern string) error { return fmt.Errorf("%w: %s", errUnbalancedBraces, pattern) } - // Basic validation: check that braces are properly formatted - // More detailed validation would require parsing, which mux will do anyway return nil } +// logUncompilablePattern reports, at error level, an endpoint pattern that mux cannot compile - +// an unterminated character class such as "{id:[}", for example. +// +// It does not fail the load. A pattern like that passes the brace-balance check, loads cleanly, +// and then never matches any request, so the endpoint it was written to govern is left unguarded: +// the same failure shape as an unreachable wildcard-method rule, reached a different way. Refusing +// to start would close that hole, but GoFr's position is to log and stay up rather than abort on a +// config defect (gofr-dev/gofr#2378), and reversing that is not something a bugfix should do. +// Closing it properly needs the fail-closed default for unmatched routes tracked in #3935; until +// then the operator gets a loud line naming the pattern instead of silence. +func (c *Config) logUncompilablePattern(pattern string, index int) { + err := mux.NewRouter().NewRoute().Path(pattern).GetError() + if err == nil || c.Logger == nil { + return + } + + c.Logger.Errorf("RBAC: endpoint[%d]: %v: %q: %v. This endpoint will never match a request, "+ + "so it is NOT enforced - any route it was meant to govern is currently unguarded.", + index, errInvalidPattern, pattern, err) +} + // matchesEndpointPattern checks if the route matches the endpoint pattern. -// Method matching is handled separately in matchEndpoint before this function is called. +// Method matching is handled separately by endpointRule.matchesMethod before this is called. // Uses mux Route.Match() for mux patterns, exact match for non-pattern paths. func matchesEndpointPattern(endpoint *EndpointMapping, route string, config *Config) bool { if endpoint.Path == "" { @@ -197,17 +192,7 @@ func getEndpointForRequest(r *http.Request, config *Config) (*EndpointMapping, b return nil, false } - method := strings.ToUpper(r.Method) - path := r.URL.Path - key := fmt.Sprintf("%s:%s", method, path) - - // Try exact match first (O(1) lookup) - if endpoint, isPublic := config.getExactEndpoint(key); endpoint != nil { - return endpoint, isPublic - } - - // Try pattern matching (O(n) but only for patterns, not exact matches) - return config.findEndpointByPattern(method, path) + return config.resolve(strings.ToUpper(r.Method), r.URL.Path) } // ResolveRBACConfigPath resolves the RBAC config file path. diff --git a/pkg/gofr/rbac/endpoint_matcher_test.go b/pkg/gofr/rbac/endpoint_matcher_test.go index 773b660de7..c338a459c6 100644 --- a/pkg/gofr/rbac/endpoint_matcher_test.go +++ b/pkg/gofr/rbac/endpoint_matcher_test.go @@ -12,108 +12,92 @@ import ( "github.com/stretchr/testify/require" ) -func TestMatchEndpoint_ExactMatch(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/users", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}, +// TestConfig_resolve_Cases covers the resolution cases that used to be exercised through the +// now-deleted matchEndpoint helper. They run against config.resolve, which is the single entry +// point the middleware and GetEndpointPermission both use. +func TestConfig_resolve_Cases(t *testing.T) { + testCases := []struct { + desc string + endpoints []EndpointMapping + method string + path string + expectedPath string + expectedPublic bool + }{ + { + desc: "exact match", + endpoints: []EndpointMapping{{Path: "/api/users", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}}, + method: http.MethodGet, + path: "/api/users", + expectedPath: "/api/users", }, - } - _ = config.processUnifiedConfig() - endpoints := config.Endpoints - endpoint, isPublic := matchEndpoint("GET", "/api/users", endpoints, config) - require.NotNil(t, endpoint) - assert.False(t, isPublic) -} - -func TestMatchEndpoint_PublicEndpoint(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/health", Methods: []string{"GET"}, Public: true}, + { + desc: "public endpoint", + endpoints: []EndpointMapping{{Path: "/health", Methods: []string{"GET"}, Public: true}}, + method: http.MethodGet, + path: "/health", + expectedPath: "/health", + expectedPublic: true, }, - } - _ = config.processUnifiedConfig() - endpoints := config.Endpoints - endpoint, isPublic := matchEndpoint("GET", "/health", endpoints, config) - require.NotNil(t, endpoint) - assert.True(t, isPublic) -} - -func TestMatchEndpoint_DifferentMethod(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/users", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}, + { + desc: "declared method does not cover the request method", + endpoints: []EndpointMapping{{Path: "/api/users", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}}, + method: http.MethodPost, + path: "/api/users", }, - } - _ = config.processUnifiedConfig() - endpoints := config.Endpoints - endpoint, isPublic := matchEndpoint("POST", "/api/users", endpoints, config) - require.Nil(t, endpoint) - assert.False(t, isPublic) -} - -func TestMatchEndpoint_WildcardMethod(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api", Methods: []string{"*"}, RequiredPermissions: []string{"api:*"}}, + { + desc: "wildcard method", + endpoints: []EndpointMapping{{Path: "/api", Methods: []string{"*"}, RequiredPermissions: []string{"api:read"}}}, + method: http.MethodPost, + path: "/api", + expectedPath: "/api", }, - } - _ = config.processUnifiedConfig() - endpoints := config.Endpoints - endpoint, isPublic := matchEndpoint("POST", "/api", endpoints, config) - require.NotNil(t, endpoint) - assert.False(t, isPublic) -} - -func TestMatchEndpoint_MuxPatternPath(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/{resource}", Methods: []string{"GET"}, RequiredPermissions: []string{"api:read"}}, + { + desc: "omitted methods behaves as wildcard", + endpoints: []EndpointMapping{{Path: "/api", Methods: []string{}, RequiredPermissions: []string{"api:read"}}}, + method: http.MethodPost, + path: "/api", + expectedPath: "/api", }, - } - _ = config.processUnifiedConfig() - endpoints := config.Endpoints - endpoint, isPublic := matchEndpoint("GET", "/api/users", endpoints, config) - require.NotNil(t, endpoint) - assert.False(t, isPublic) -} - -func TestMatchEndpoint_MuxPatternWithConstraint(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/users/{id:[0-9]+}", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}, + { + desc: "mux pattern path", + endpoints: []EndpointMapping{{Path: "/api/{resource}", Methods: []string{"GET"}, RequiredPermissions: []string{"api:read"}}}, + method: http.MethodGet, + path: "/api/users", + expectedPath: "/api/{resource}", }, - } - _ = config.processUnifiedConfig() - endpoints := config.Endpoints - endpoint, isPublic := matchEndpoint("GET", "/api/users/123", endpoints, config) - require.NotNil(t, endpoint) - assert.False(t, isPublic) -} - -func TestMatchEndpoint_NotFound(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api/users", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}, + { + desc: "mux pattern with constraint", + endpoints: []EndpointMapping{{Path: "/api/users/{id:[0-9]+}", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}}, + method: http.MethodGet, + path: "/api/users/123", + expectedPath: "/api/users/{id:[0-9]+}", + }, + { + desc: "no configured endpoint matches", + endpoints: []EndpointMapping{{Path: "/api/users", Methods: []string{"GET"}, RequiredPermissions: []string{"users:read"}}}, + method: http.MethodGet, + path: "/api/posts", }, } - _ = config.processUnifiedConfig() - endpoints := config.Endpoints - endpoint, isPublic := matchEndpoint("GET", "/api/posts", endpoints, config) - require.Nil(t, endpoint) - assert.False(t, isPublic) -} -func TestMatchEndpoint_EmptyMethods(t *testing.T) { - config := &Config{ - Endpoints: []EndpointMapping{ - {Path: "/api", Methods: []string{}, RequiredPermissions: []string{"api:*"}}, - }, + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + config := newTestConfig(t, tc.endpoints, nil) + + endpoint, isPublic := config.resolve(tc.method, tc.path) + + assert.Equal(t, tc.expectedPublic, isPublic) + + if tc.expectedPath == "" { + assert.Nil(t, endpoint) + return + } + + require.NotNil(t, endpoint) + assert.Equal(t, tc.expectedPath, endpoint.Path) + }) } - _ = config.processUnifiedConfig() - endpoints := config.Endpoints - endpoint, isPublic := matchEndpoint("POST", "/api", endpoints, config) - require.NotNil(t, endpoint) - assert.False(t, isPublic) } func TestMatchesHTTPMethod(t *testing.T) { diff --git a/pkg/gofr/rbac/resolver.go b/pkg/gofr/rbac/resolver.go new file mode 100644 index 0000000000..645b0720ad --- /dev/null +++ b/pkg/gofr/rbac/resolver.go @@ -0,0 +1,239 @@ +package rbac + +import ( + "sort" + "strings" +) + +// Segment specificity scores, compared segment by segment to order overlapping rules. +// Each level constrains the set of paths a segment can match more tightly than the one below it: +// a literal matches one string, a constrained variable matches one segment drawn from a regex, +// a free variable matches any one segment, and a catch-all matches any number of them. +const ( + segCatchAll = iota + 1 // {path:.*} - matches any number of segments + segVariable // {id} - matches exactly one segment, unconstrained + segConstrained // {id:[0-9]+} - matches exactly one segment, and only some of them + segLiteral // users - matches itself +) + +// endpointRule is one (path, method) pair from the config, pre-scored so that overlapping +// rules resolve deterministically. Rules are sorted once at load time. +type endpointRule struct { + endpoint *EndpointMapping + + // pattern is the endpoint path, which may contain mux variables. + pattern string + + // method is the upper-cased declared method, or "*" for all methods. + method string + + // isPublic mirrors endpoint.Public. + isPublic bool + + // pathScore is the per-segment specificity of pattern. + pathScore []int + + // methodScore is 1 for an explicitly declared method and 0 for "*", so that an explicit + // method wins over a wildcard on an otherwise equally specific path. + methodScore int +} + +// matchesMethod reports whether the rule covers the given upper-cased request method. +// A rule declared with "*" covers every method, including methods GoFr does not know about +// (WebDAV verbs, for example). That is the safe direction here: because a rule imposes a +// permission requirement rather than granting access, covering an unknown verb tightens +// enforcement rather than loosening it. +func (r *endpointRule) matchesMethod(methodUpper string) bool { + return matchesHTTPMethod(methodUpper, []string{r.method}) +} + +// matches reports whether the rule covers the given request. +func (r *endpointRule) matches(methodUpper, path string, config *Config) bool { + return r.matchesMethod(methodUpper) && matchesEndpointPattern(r.endpoint, path, config) +} + +// buildEndpointRules expands endpoints into one rule per declared method and orders them +// most-specific-first, so that a narrower rule governs a request even when a broader one is +// declared ahead of it. Rules that score identically - two patterns of the same shape, such as +// "/{a}/{b}" and "/{x}/{y}" - keep their declaration order, which is the one case where the +// order entries are written in still decides the outcome. +// +// Duplicate (method, path) declarations collapse to the last one, matching how the lookup +// maps are built, so both resolution paths always agree. +func buildEndpointRules(endpoints []EndpointMapping) []endpointRule { + byKey := make(map[string]endpointRule, len(endpoints)) + order := make([]string, 0, len(endpoints)) + + for i := range endpoints { + endpoint := &endpoints[i] + + methods := endpoint.Methods + if len(methods) == 0 { + methods = []string{"*"} + } + + for _, method := range methods { + methodUpper := strings.ToUpper(method) + key := buildEndpointKey(endpoint, methodUpper) + + methodScore := 1 + if methodUpper == "*" { + methodScore = 0 + } + + if _, seen := byKey[key]; !seen { + order = append(order, key) + } + + byKey[key] = endpointRule{ + endpoint: endpoint, + pattern: endpoint.Path, + method: methodUpper, + isPublic: endpoint.Public, + pathScore: pathSpecificity(endpoint.Path), + methodScore: methodScore, + } + } + } + + rules := make([]endpointRule, 0, len(order)) + for _, key := range order { + rules = append(rules, byKey[key]) + } + + sort.SliceStable(rules, func(i, j int) bool { + if cmp := compareSpecificity(rules[i].pathScore, rules[j].pathScore); cmp != 0 { + return cmp > 0 + } + + if rules[i].methodScore != rules[j].methodScore { + return rules[i].methodScore > rules[j].methodScore + } + + // Nothing about the patterns separates them, so fall back on the safer outcome: a rule + // that requires permissions outranks a public one. A tie is a config the operator did not + // intend either way, and enforcing is the recoverable half of that mistake. + return !rules[i].isPublic && rules[j].isPublic + }) + + return rules +} + +// pathSpecificity scores each segment of a path pattern. +func pathSpecificity(pattern string) []int { + if pattern == "" { + return nil + } + + segments := splitPatternSegments(strings.Trim(pattern, "/")) + scores := make([]int, 0, len(segments)) + + for _, segment := range segments { + switch { + case !strings.HasPrefix(segment, "{") || !strings.HasSuffix(segment, "}"): + scores = append(scores, segLiteral) + case isCatchAllVariable(segment): + scores = append(scores, segCatchAll) + case strings.Contains(segment, ":"): + scores = append(scores, segConstrained) + default: + scores = append(scores, segVariable) + } + } + + return scores +} + +// splitPatternSegments splits a path pattern on "/", ignoring separators that sit inside a +// "{name:regex}" constraint - "{path:[a-z/]+}" is one segment, not three. Splitting naively +// would shatter such a constraint into fragments that each look like a literal, scoring the +// loosest pattern in the config as the most specific one. +func splitPatternSegments(pattern string) []string { + var ( + segments []string + depth int + start int + ) + + for i, r := range pattern { + switch r { + case '{': + depth++ + case '}': + if depth > 0 { + depth-- + } + case '/': + if depth == 0 { + segments = append(segments, pattern[start:i]) + start = i + 1 + } + } + } + + return append(segments, pattern[start:]) +} + +// isCatchAllVariable reports whether a "{name:regex}" segment can span multiple path +// segments. Two kinds qualify: the documented ".*" and ".+" forms, and any constraint that +// admits "/" itself, such as "{path:[a-z/]+}" - it matches "/files/a/b/c" just as a +// catch-all would, so scoring it as a single-segment variable would let the loosest pattern in +// the config outrank a narrower one it fully contains. +// +// Whether the constraint can *actually* produce a "/" is not decided here; a "/" appearing +// anywhere in it is enough. Deciding it properly means parsing the regex, and the conservative +// answer only ever moves a pattern down the ordering, which is the safe direction: it can lose +// to a narrower rule, never shadow one. +func isCatchAllVariable(segment string) bool { + inner := segment[1 : len(segment)-1] + + idx := strings.Index(inner, ":") + if idx < 0 { + return false + } + + constraint := strings.TrimSpace(inner[idx+1:]) + + return constraint == ".*" || constraint == ".+" || strings.Contains(constraint, "/") +} + +// compareSpecificity orders two segment score vectors, most specific first. +// The first differing segment decides; if one vector is a prefix of the other, the longer +// (more constrained) pattern wins. Returns >0 when a is more specific than b. +func compareSpecificity(a, b []int) int { + for i := 0; i < len(a) && i < len(b); i++ { + if a[i] != b[i] { + return a[i] - b[i] + } + } + + return len(a) - len(b) +} + +// resolveEndpoint returns the most specific rule covering the request, and whether it is public. +func resolveEndpoint(methodUpper, path string, rules []endpointRule, config *Config) (*EndpointMapping, bool) { + for i := range rules { + if rules[i].matches(methodUpper, path, config) { + return rules[i].endpoint, rules[i].isPublic + } + } + + return nil, false +} + +// resolve finds the endpoint governing a request, preferring the O(1) exact-path lookups. +// +// The exact lookups stay consistent with the ordered scan because a literal path is always +// more specific than any pattern, and an explicitly declared method is always preferred over +// a wildcard one - which is why the request's own method is probed before "*". +func (c *Config) resolve(methodUpper, path string) (*EndpointMapping, bool) { + if endpoint, isPublic := c.getExactEndpoint(methodUpper + ":" + path); endpoint != nil { + return endpoint, isPublic + } + + if endpoint, isPublic := c.getExactEndpoint("*:" + path); endpoint != nil { + return endpoint, isPublic + } + + return resolveEndpoint(methodUpper, path, c.rules, c) +} diff --git a/pkg/gofr/rbac/resolver_test.go b/pkg/gofr/rbac/resolver_test.go new file mode 100644 index 0000000000..c8151e6cb4 --- /dev/null +++ b/pkg/gofr/rbac/resolver_test.go @@ -0,0 +1,415 @@ +package rbac + +import ( + "net/http" + "net/http/httptest" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newTestConfig builds a Config from endpoints and processes it, failing the test on error. +func newTestConfig(tb testing.TB, endpoints []EndpointMapping, roles []RoleDefinition) *Config { + tb.Helper() + + config := &Config{Endpoints: endpoints, Roles: roles, RoleHeader: "X-User-Role"} + require.NoError(tb, config.processUnifiedConfig()) + + return config +} + +func TestGetEndpointForRequest_WildcardMethod(t *testing.T) { + testCases := []struct { + desc string + methods []string + requestMethod string + expectMatch bool + }{ + {"wildcard matches GET", []string{"*"}, http.MethodGet, true}, + {"wildcard matches DELETE", []string{"*"}, http.MethodDelete, true}, + {"wildcard matches custom method", []string{"*"}, "PROPFIND", true}, + {"omitted methods matches DELETE", nil, http.MethodDelete, true}, + {"empty methods matches DELETE", []string{}, http.MethodDelete, true}, + {"explicit method matches itself", []string{"GET"}, http.MethodGet, true}, + {"explicit method does not match other", []string{"GET"}, http.MethodDelete, false}, + {"lowercase declaration matches", []string{"get"}, http.MethodGet, true}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + config := newTestConfig(t, []EndpointMapping{ + {Path: "/admin/{path:.*}", Methods: tc.methods, RequiredPermissions: []string{"admin:read"}}, + }, nil) + + req := httptest.NewRequestWithContext(t.Context(), tc.requestMethod, "/admin/orgs/123", http.NoBody) + endpoint, isPublic := getEndpointForRequest(req, config) + + assert.False(t, isPublic) + + if tc.expectMatch { + require.NotNil(t, endpoint, "expected the rule to match") + assert.Equal(t, "/admin/{path:.*}", endpoint.Path) + } else { + assert.Nil(t, endpoint) + } + }) + } +} + +func TestGetEndpointForRequest_MostSpecificWins(t *testing.T) { + broad := EndpointMapping{Path: "/admin/{path:.*}", Methods: []string{"*"}, RequiredPermissions: []string{"admin:read"}} + narrow := EndpointMapping{Path: "/admin/orgs/{org_id}", Methods: []string{"DELETE"}, RequiredPermissions: []string{"admin:write"}} + literal := EndpointMapping{Path: "/admin/orgs/global", Methods: []string{"DELETE"}, RequiredPermissions: []string{"admin:super"}} + + // A constrained variable admits fewer paths than a free one, so it has to outrank it - + // otherwise these two score equally and the sort falls through to declaration order. + free := EndpointMapping{Path: "/users/{id}", Methods: []string{"*"}, RequiredPermissions: []string{"users:read"}} + constrained := EndpointMapping{Path: "/users/{id:[0-9]+}", Methods: []string{"*"}, RequiredPermissions: []string{"users:write"}} + + // Same depth, differing only in one segment, and the more specific one is declared last: these + // go red if segments stop being compared position by position, where a case whose patterns + // differ in length or that a literal path resolves by exact lookup would still pass. + varPrefix := EndpointMapping{Path: "/{scope}/orgs/{org_id}", Methods: []string{"*"}, RequiredPermissions: []string{"scope:read"}} + varTail := EndpointMapping{Path: "/admin/{section}/{org_id}", Methods: []string{"*"}, RequiredPermissions: []string{"admin:list"}} + litTail := EndpointMapping{Path: "/admin/orgs/{org_id}", Methods: []string{"*"}, RequiredPermissions: []string{"orgs:read"}} + + testCases := []struct { + desc string + endpoints []EndpointMapping + path string + expected string + }{ + {"narrow wins over broad", []EndpointMapping{broad, narrow}, "/admin/orgs/123", "/admin/orgs/{org_id}"}, + {"declaration order is irrelevant", []EndpointMapping{narrow, broad}, "/admin/orgs/123", "/admin/orgs/{org_id}"}, + {"literal wins over param", []EndpointMapping{broad, narrow, literal}, "/admin/orgs/global", "/admin/orgs/global"}, + {"literal segment wins over a variable in the same position", []EndpointMapping{varPrefix, litTail}, + "/admin/orgs/123", "/admin/orgs/{org_id}"}, + {"the first differing segment decides", []EndpointMapping{varPrefix, varTail}, "/admin/orgs/123", "/admin/{section}/{org_id}"}, + {"broad wins when it is the only match", []EndpointMapping{broad, narrow}, "/admin/settings", "/admin/{path:.*}"}, + {"constrained variable wins over free one", []EndpointMapping{free, constrained}, "/users/42", "/users/{id:[0-9]+}"}, + {"constrained variable wins in either declaration order", []EndpointMapping{constrained, free}, "/users/42", "/users/{id:[0-9]+}"}, + {"free variable still matches what the constraint rejects", []EndpointMapping{constrained, free}, "/users/me", "/users/{id}"}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + config := newTestConfig(t, tc.endpoints, nil) + + // Repeat to catch map-iteration nondeterminism: Go randomizes range order per range. + for range 100 { + req := httptest.NewRequestWithContext(t.Context(), http.MethodDelete, tc.path, http.NoBody) + endpoint, _ := getEndpointForRequest(req, config) + + require.NotNil(t, endpoint) + require.Equal(t, tc.expected, endpoint.Path) + } + }) + } +} + +func TestGetEndpointForRequest_ExplicitMethodBeatsWildcard(t *testing.T) { + config := newTestConfig(t, []EndpointMapping{ + {Path: "/admin/reports", Methods: []string{"*"}, RequiredPermissions: []string{"admin:read"}}, + {Path: "/admin/reports", Methods: []string{"DELETE"}, RequiredPermissions: []string{"admin:write"}}, + }, nil) + + for range 100 { + req := httptest.NewRequestWithContext(t.Context(), http.MethodDelete, "/admin/reports", http.NoBody) + endpoint, _ := getEndpointForRequest(req, config) + + require.NotNil(t, endpoint) + require.Equal(t, []string{"admin:write"}, endpoint.RequiredPermissions) + } +} + +func TestConfig_GetEndpointPermission_WildcardMethod(t *testing.T) { + testCases := []struct { + desc string + method string + path string + expectedPerms []string + expectedPublic bool + }{ + {"wildcard rule is reachable", http.MethodDelete, "/admin/orgs/123", []string{"admin:read"}, false}, + {"most specific rule wins", http.MethodGet, "/team/reports", []string{"team:read"}, false}, + {"public rule reported as public", http.MethodGet, "/health", nil, true}, + {"unconfigured path returns nothing", http.MethodGet, "/nope", nil, false}, + } + + config := newTestConfig(t, []EndpointMapping{ + {Path: "/admin/{path:.*}", Methods: []string{"*"}, RequiredPermissions: []string{"admin:read"}}, + {Path: "/team/reports", Methods: []string{"GET"}, RequiredPermissions: []string{"team:read"}}, + {Path: "/health", Methods: []string{"GET"}, Public: true}, + }, nil) + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + perms, isPublic := config.GetEndpointPermission(tc.method, tc.path) + + assert.Equal(t, tc.expectedPublic, isPublic) + assert.Equal(t, tc.expectedPerms, perms) + }) + } +} + +func TestMiddleware_WildcardMethodEnforcement(t *testing.T) { + testCases := []struct { + desc string + role string + expectedCode int + }{ + {"no role is rejected", "", http.StatusUnauthorized}, + {"insufficient role is rejected", "viewer", http.StatusForbidden}, + {"authorized role is allowed", "admin", http.StatusOK}, + } + + config := newTestConfig(t, + []EndpointMapping{ + {Path: "/admin/{path:.*}", Methods: []string{"*"}, RequiredPermissions: []string{"admin:write"}}, + }, + []RoleDefinition{ + {Name: "admin", Permissions: []string{"admin:write"}}, + {Name: "viewer", Permissions: []string{"admin:read"}}, + }, + ) + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + reached := false + handler := Middleware(config)(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + reached = true + })) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodDelete, "/admin/orgs/123", http.NoBody) + if tc.role != "" { + req.Header.Set("X-User-Role", tc.role) + } + + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, tc.expectedCode, rec.Code) + assert.Equal(t, tc.expectedCode == http.StatusOK, reached) + }) + } +} + +func TestLoadPermissions_UncompilablePatternIsNonFatal(t *testing.T) { + testCases := []struct { + desc string + path string + expectLog bool + }{ + {"unparsable regex constraint", "/api/{id:[}", true}, + {"unbalanced parenthesis in constraint", "/api/{id:(}", true}, + {"valid numeric constraint", "/api/{id:[0-9]+}", false}, + {"valid catch-all", "/api/{path:.*}", false}, + {"plain path", "/api/users", false}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + fileContent := `{ + "roles": [{"name": "admin", "permissions": ["admin:read"]}], + "endpoints": [{"path": "` + tc.path + `", "methods": ["GET"], "requiredPermissions": ["admin:read"]}] + }` + + path, err := createTestConfigFile("test_pattern_config.json", fileContent) + require.NoError(t, err) + + defer os.Remove(path) + + logger := &mockLogger{} + + config, err := LoadPermissions("test_pattern_config.json", logger, nil, nil) + + // A pattern mux cannot compile does not stop the app from booting - it is logged loudly + // instead, because the endpoint it governs will silently never match. + require.NoError(t, err) + require.NotNil(t, config) + + if tc.expectLog { + require.Len(t, logger.errorLogs, 1) + assert.Contains(t, logger.errorLogs[0], tc.path) + assert.Contains(t, logger.errorLogs[0], "NOT enforced") + } else { + assert.Empty(t, logger.errorLogs) + } + }) + } +} + +func TestLoadPermissions_UncompilablePatternWithoutLogger(t *testing.T) { + fileContent := `{ + "roles": [{"name": "admin", "permissions": ["admin:read"]}], + "endpoints": [{"path": "/api/{id:[}", "methods": ["GET"], "requiredPermissions": ["admin:read"]}] + }` + + path, err := createTestConfigFile("test_pattern_nolog_config.json", fileContent) + require.NoError(t, err) + + defer os.Remove(path) + + config, err := LoadPermissions("test_pattern_nolog_config.json", nil, nil, nil) + + require.NoError(t, err) + assert.NotNil(t, config) +} + +func TestLoadPermissions_UnbalancedBracesStillFails(t *testing.T) { + fileContent := `{ + "roles": [{"name": "admin", "permissions": ["admin:read"]}], + "endpoints": [{"path": "/api/{id}}", "methods": ["GET"], "requiredPermissions": ["admin:read"]}] + }` + + path, err := createTestConfigFile("test_pattern_braces_config.json", fileContent) + require.NoError(t, err) + + defer os.Remove(path) + + config, err := LoadPermissions("test_pattern_braces_config.json", nil, nil, nil) + + require.ErrorIs(t, err, errUnbalancedBraces) + assert.Nil(t, config) +} + +func TestPathSpecificity(t *testing.T) { + testCases := []struct { + desc string + pattern string + expected []int + }{ + {"empty pattern has no segments", "", nil}, + {"literal segments", "/admin/orgs/global", []int{segLiteral, segLiteral, segLiteral}}, + {"free variable", "/users/{id}", []int{segLiteral, segVariable}}, + {"constrained variable", "/users/{id:[0-9]+}", []int{segLiteral, segConstrained}}, + {"constrained variable with a quantifier", "/users/{id:[0-9]{2,3}}", []int{segLiteral, segConstrained}}, + {"documented catch-all", "/admin/{path:.*}", []int{segLiteral, segCatchAll}}, + {"one-or-more catch-all", "/admin/{path:.+}", []int{segLiteral, segCatchAll}}, + + // A "/" inside the constraint must not be treated as a segment separator: splitting on it + // would leave three fragments that each look literal, scoring the loosest pattern in the + // config as the most specific one. + {"constraint admitting a slash spans segments", "/files/{path:[a-z/]+}", []int{segLiteral, segCatchAll}}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + assert.Equal(t, tc.expected, pathSpecificity(tc.pattern)) + }) + } +} + +func TestCompareSpecificity(t *testing.T) { + testCases := []struct { + desc string + a, b []int + expectAFirst bool + expectTie bool + }{ + {"literal beats constrained in the same position", []int{segLiteral, segLiteral}, []int{segLiteral, segConstrained}, true, false}, + {"constrained beats free", []int{segLiteral, segConstrained}, []int{segLiteral, segVariable}, true, false}, + {"free beats catch-all", []int{segLiteral, segVariable}, []int{segLiteral, segCatchAll}, true, false}, + {"the first differing segment decides, not later ones", []int{segCatchAll, segLiteral}, []int{segLiteral, segCatchAll}, false, false}, + {"a longer path wins when one is a prefix of the other", []int{segLiteral, segLiteral}, []int{segLiteral}, true, false}, + {"identical vectors tie", []int{segLiteral, segVariable}, []int{segLiteral, segVariable}, false, true}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + got := compareSpecificity(tc.a, tc.b) + + if tc.expectTie { + assert.Equal(t, 0, got) + return + } + + assert.Equal(t, tc.expectAFirst, got > 0) + // The comparison has to be antisymmetric, or sort.SliceStable's ordering is undefined. + assert.Equal(t, tc.expectAFirst, compareSpecificity(tc.b, tc.a) < 0) + }) + } +} + +func TestGetEndpointForRequest_DuplicateDeclarationLastWins(t *testing.T) { + public := EndpointMapping{Path: "/admin/reports", Methods: []string{"GET"}, Public: true} + protected := EndpointMapping{Path: "/admin/reports", Methods: []string{"GET"}, RequiredPermissions: []string{"admin:read"}} + + testCases := []struct { + desc string + endpoints []EndpointMapping + expectedPublic bool + expectedPerms []string + }{ + // The public flag and the permission list live in maps of their own, so overwriting the + // endpoint alone would leave the earlier declaration's flag behind - and a stale public + // flag serves the route unauthenticated. + {"protected declared last is enforced", []EndpointMapping{public, protected}, false, []string{"admin:read"}}, + {"public declared last is public", []EndpointMapping{protected, public}, true, nil}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + config := newTestConfig(t, tc.endpoints, nil) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/admin/reports", http.NoBody) + endpoint, isPublic := getEndpointForRequest(req, config) + + require.NotNil(t, endpoint) + assert.Equal(t, tc.expectedPublic, isPublic) + assert.Equal(t, tc.expectedPerms, endpoint.RequiredPermissions) + + perms, isPublic := config.GetEndpointPermission(http.MethodGet, "/admin/reports") + assert.Equal(t, tc.expectedPublic, isPublic) + assert.Equal(t, tc.expectedPerms, perms) + }) + } +} + +func TestGetEndpointForRequest_ProtectedBeatsPublicOnTie(t *testing.T) { + public := EndpointMapping{Path: "/{a}/{b}", Methods: []string{"GET"}, Public: true} + protected := EndpointMapping{Path: "/{x}/{y}", Methods: []string{"GET"}, RequiredPermissions: []string{"admin:read"}} + + testCases := []struct { + desc string + endpoints []EndpointMapping + }{ + {"public declared first", []EndpointMapping{public, protected}}, + {"protected declared first", []EndpointMapping{protected, public}}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + config := newTestConfig(t, tc.endpoints, nil) + + for range 100 { + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/foo/bar", http.NoBody) + endpoint, isPublic := getEndpointForRequest(req, config) + + require.NotNil(t, endpoint) + assert.False(t, isPublic, "a tie must not resolve to the public rule") + assert.Equal(t, "/{x}/{y}", endpoint.Path) + } + }) + } +} + +func TestGetEndpointForRequest_SlashConstraintDoesNotOutrankNarrowRule(t *testing.T) { + // "{path:[a-z/]+}" spans segments exactly as "{path:.*}" does. Scored naively it reads as + // three literal segments and outranks the narrow rule below, which is the fail-open direction + // when the broad rule is the more permissive one. + broad := EndpointMapping{Path: "/files/{path:[a-z/]+}", Methods: []string{"*"}, RequiredPermissions: []string{"files:read"}} + narrow := EndpointMapping{Path: "/files/private/{name}", Methods: []string{"*"}, RequiredPermissions: []string{"files:admin"}} + + for _, endpoints := range [][]EndpointMapping{{broad, narrow}, {narrow, broad}} { + config := newTestConfig(t, endpoints, nil) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/files/private/secret", http.NoBody) + endpoint, _ := getEndpointForRequest(req, config) + + require.NotNil(t, endpoint) + assert.Equal(t, "/files/private/{name}", endpoint.Path) + } +}