feat(rbac): accept role arrays and authorize from a scope/permissions claim by akshat-kumar-singhal · Pull Request #4434 · gofr-dev/gofr

akshat-kumar-singhal · 2026-10-05T06:16:12Z

Description:

Implements the multi-value claims part of RBAC: authorize on a token's scope/permissions claim, and add the role/permission extractor hook agreed in #2144 #4388 (item 1, as agreed in the issue on 10-01). The extractor hook stays open there and will get its own issue. Also covers RBAC: per-request pattern matching races and grows the router without bound; multi-role JWTs authorize nothing #3979 item 3.
Role arrays: with jwtClaimPath: "roles" and a token "roles": ["admin", "viewer"], the request holds the permissions of both roles. Before this, the array was formatted as "[admin viewer]", which matched no role, so every request got a 403. A role string such as "Org Admin" is still one role and is never split, roles[0] is unchanged, and a number or boolean role is still read as its text (123 is role "123"), whether it is the claim itself or an entry of the array ([123]).
Permissions mode: the new permissionsClaimPath (e.g. "scope") authorizes straight from the permissions in the token. A string claim is split on spaces, and an array claim is held entry by entry. RBAC does not validate the token itself: the docs and the example refuse to start with an empty TOKEN_ISSUER (it would disable the issuer check) and pass jwt.WithAudience, jwt.WithIssuer and jwt.WithExpirationRequired to EnableOAuth, which checks them on every request that carries a token, not only on routes with an RBAC rule.
Statuses: a verified token that holds nothing gets a 403: in permissions mode a missing, null, "" or [] claim, and in roles mode a null claim (as before). Roles mode keeps its 401 for a missing or "" claim. The 401/403 split follows RFC 6750 §3.1; no WWW-Authenticate header is set.
Errors unchanged: a custom ErrorHandler receives the bare ErrRoleNotFound or ErrAccessDenied, as on development (err == ErrRoleNotFound holds). The cause goes to the metric label and the logs only.
Load errors: EnableRBAC returns an error when both claim paths are set, or when permissionsClaimPath cannot be parsed (e.g. scope[x]). In permissions mode a roles section or a roleHeader is never read, and each logs a WARN at load.
No merged list per request: roles are checked one by one against a permission set built at load time, and a scope string is walked with strings.FieldsSeq. A unit test pins that check at 0 allocations.
Behavior change for a flat dotted key: when the dotted walk fails, development returns a 401, while this branch then reads the whole key. With jwtClaimPath: "a.b", a token {"a": "x", "a.b": "admin"} is a 401 on development and role admin here; the same holds for {"realm_access.roles": ["admin"]} with path realm_access.roles. The identity provider must put a flat key with dots into a signed token for this to happen. Documented in the claim-path paragraph of the RBAC docs and pinned by two TestExtractClaimValue cases.
Dotted claim names: a claim path is walked as a.b.c first, as before; only when the walk finds nothing is it tried as one whole top-level key, so https://example.com/roles works without changing what any existing config reads.
Unreadable claim (an object, or in permissions mode a number or bool): 403, plus one WARN per config naming the claim path (never its value); the metric counts every request.
Metric: rbac_role_extraction_failures carries a reason label: no_jwt_claims, missing_role or unreadable_claim. The counter was incremented before but never registered; it is now registered.
Call order: if a JWT mode finds no claims in the request, RBAC warns once: "call EnableOAuth before EnableRBAC". This is skipped on /.well-known/*, which OAuth never verifies.
Privacy: with a role array or permissions mode, an allowed request logs only the role or permission that granted access (a permission goes in the new permission field, not role). A denied request logs only held_count, and the role passed to ErrorHandler is empty. held_count is always logged (it has no omitempty), so a denial that held nothing shows "held_count":0; this also means the field appears on single-role lines. The single-role path otherwise logs as before.
Startup line: RBAC enabled: mode=permissions, source=scope. The field is source= in every mode (a header is not a claim).
Metric registration: EnableRBAC registers the counter the first time it runs on an app, the way GraphQL, gRPC, cron and LLM register theirs, so calling it twice does not log a duplicate-metric ERROR. LoadPermissions registers nothing, and no package-level state is kept.
Request context: the value RBAC stores in the request context (under its internal userRole key) is now what granted access: the role when the request holds one, the granting role when the claim is an array of roles, and the granting permission in permissions mode. On development it was the extracted role. Public routes store nothing, as before. The key is unexported, so no handler could read it.

Breaking Changes (if applicable):

None. Every claim shape that worked on development behaves the same, the ErrorHandler gets the same errors, and a dotted claim path reads the same claim (one exception: a flat key with dots now grants where it was a 401, see Behavior change above); a role array, which used to match no role, now grants.
No public API is removed or renamed. AuditLog gains HeldCount and Permission, and Config gains PermissionsClaimPath.

Additional Information:

No new dependencies.
Docs: docs/advanced-guide/rbac/page.md gains a config reference, "Role Arrays", "Permissions Mode" (with token validation through EnableOAuth, and the issuer and identity-provider caveats), a rule table, a call-order note, and privacy notes.
New example: examples/using-rbac-permissions. It passes jwt.WithIssuer, jwt.WithAudience and jwt.WithExpirationRequired to EnableOAuth. Its test runs the whole app against an in-process JWKS server with tokens it signs itself (allow, deny, wrong audience, no exp, wrong or missing issuer, no scope).
Tests run through the real middleware.OAuth with RSA-signed tokens, and both JSON and YAML configs are loaded.
Benchmark (go test -bench BenchmarkMiddleware -benchmem ./pkg/gofr/rbac/, arm64 macOS). The multi-value modes add no allocations: the 3 allocs/op are the existing request-context store, the same as the single-role baseline.

Case	ns/op	B/op	allocs/op
header mode, one role (baseline)	140	384	3
roles mode, 10-role array	322	384	3
permissions mode, 10-entry scope	257	384	3

This branch is based on development, not feat(rbac): check rules against registered routes at startup #4390, as requested. The two touch rbac/middleware.go and rbac.go, so whichever merges second will be rebased.

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.

… claim A JWT role array now holds every role in it (union of their permissions); before, ["admin","viewer"] was formatted as "[admin viewer]" and matched no role. A new permissions mode (permissionsClaimPath, e.g. "scope") authorizes straight from the permissions in the token, split on spaces for a string claim, and requires an audience and an exp claim. - Config: permissionsClaimPath and audience; both claim paths, or a permissions path without audience, or an audience without a JWT path, fail at load. - No merged list per request: roles are checked one by one against a precomputed permission set, a scope string with strings.FieldsSeq (0 allocs, pinned by a test; benchmark added). - Claim paths try the whole path as a top-level key first, so namespaced claims such as https://example.com/roles work. - An unreadable claim type gets a 403 and a WARN naming the path, never the value; a missing OAuth middleware is warned about once. - With multiple values, the deny audit log carries only held_count and the ErrorHandler role is empty; the single-role path is unchanged. - rbac_role_extraction_failures is now registered. - Startup line: RBAC enabled: mode=..., claim=..., audience=[...]. - Docs, config reference, and examples/using-rbac-permissions. Refs gofr-dev#4388

aryanmehrotra

Thanks, this covers everything we agreed in #4388. I ran a22c42362 end to end: the example app and a roles-mode copy against a local JWKS server, with signed tokens for each row of the table in the issue. All of them behave as described, and the benchmark holds at 3 allocs/op in every mode.

A few things before this merges. The main one is that we do not want any breaking change, so the number/bool role case has to keep working. That one is on me: my table in the issue asked for a 403 on any unreadable shape.

Kind	Count
Behaviour	5
Config and logging	3
Docs and example	3
Optional	1

Audience in roles mode: keep it. It is opt-in, and a key that is silently ignored would be worse.

aryanmehrotra · 2026-10-05T15:32:39Z

+	case nil:
+		return held{}, ErrRoleNotFound
+	default:
+		return held{}, errUnreadableClaim


No breaking change here, please. On development a number or bool role claim is formatted into a role name (123 becomes "123"), and an app that defines such a role is allowed through. With this branch the same token gets a 403.

In roles mode, could a scalar keep formatting to a role name as before? Permissions mode is new, so it can stay strict.

This corrects my own table in #4388, which asked for a 403 on any unreadable shape. For roles mode that should now mean objects only.

Done. In roles mode a number or boolean is read as its text again (123 is role "123"), as on development; only an object is unreadable (403 + WARN). Permissions mode stays strict. I also restored the development test converts non-string role to string.

aryanmehrotra · 2026-10-05T15:32:39Z

+	case []any:
+		return held{list: v, perms: split}, nil
+	case nil:
+		return held{}, ErrRoleNotFound


A null role claim changes status too. On development it formats to "<nil>", matches no role and gets a 403. Here it gets a 401.

403 is also what RFC 6750 §3.1 asks for: the token is valid and the caller simply holds nothing (insufficient_scope). 401 is for a token that is itself not acceptable. Could null go back to 403?

Done: null holds no role and gets a 403 again. It is denied through the normal path (no role, so nothing grants), not as an extraction failure.

aryanmehrotra · 2026-10-05T15:32:39Z

 	if err != nil {
-		return "", fmt.Errorf("failed to extract role from JWT: %w", err)
+		// Do not fall back to the header: JWT is the only method when configured
+		return held{}, ErrRoleNotFound


Same reasoning for permissions mode. A verified token whose scope is missing, null or "" currently gets a 401, while "scope": [] gets a 403:

scope Now RFC 6750

[] 403 403

missing 401 403

null 401 403

"" 401 403

Permissions mode is new, so all four can be 403 from the start. If it ships as 401, changing it later is a breaking change.

Roles mode should keep its 401 for a missing or empty claim. That is existing behaviour.

Done. In permissions mode a missing, null, "" or [] claim holds nothing and gets a 403, so all four rows in your table are now 403. Roles mode keeps its 401 for a missing or "" claim.

aryanmehrotra · 2026-10-05T15:32:39Z

+	errAudienceMismatch = fmt.Errorf("%w: token audience not accepted", ErrRoleNotFound)
+
+	// errMissingExpiry is returned when a permissions-mode token carries no exp claim.
+	errMissingExpiry = fmt.Errorf("%w: token has no expiry", ErrRoleNotFound)


401 is the right status for both of these. The body is not: because they wrap ErrRoleNotFound, the client is told Unauthorized: Missing or invalid role when the real cause is the audience or a missing exp. Could they get their own message?

Also, both errors are unexported, so a custom ErrorHandler cannot tell them apart from a missing role. Worth exporting them, or do you see a reason not to?

Done. They now have their own bodies, Unauthorized: Token audience not accepted and Unauthorized: Token has no expiry, and are exported as ErrAudienceMismatch and ErrMissingExpiry. I kept them wrapping ErrRoleNotFound, so a custom ErrorHandler that checks errors.Is(err, rbac.ErrRoleNotFound) still returns 401 for them.

aryanmehrotra · 2026-10-05T15:32:39Z

+// its value), and RBAC running without the OAuth middleware in front of it (warned once).
+func reportExtractionFailure(r *http.Request, config *Config, err error, claimsWarning *sync.Once) {
+	if config.Metrics != nil {
+		config.Metrics.IncrementCounter(r.Context(), extractionFailuresMetric)


This counter now covers several different causes under one number. In my permissions-mode run it read 5, and only 1 of those was an unreadable claim:

Cause What it tells an operator

audience mismatch a token for another API, or a wrong audience

no claims in the request RBAC is not behind OAuth

no exp, missing or unreadable claim an IdP or client problem

IncrementCounter already takes labels. Could this carry a reason label so the first row can be alerted on separately?

Done. The counter now carries a reason label: audience_mismatch, no_jwt_claims, missing_expiry, missing_role or unreadable_claim. A permissions-mode claim that holds nothing is now a plain 403 and is no longer counted here.

aryanmehrotra · 2026-10-05T15:32:39Z

+
+	if logger != nil {
+		mode, source := config.mode()
+		logger.Infof("RBAC enabled: mode=%s, claim=%s, audience=%v", mode, source, config.Audience)


A config with both permissionsClaimPath and a roles section loads without a word, and the roles are never consulted. Someone moving from roles to scopes could leave the section in and assume it still applies. A warning at load would be enough.

Done: a WARN at load says the roles section is ignored when permissionsClaimPath is set.

aryanmehrotra · 2026-10-05T15:32:39Z

+Because the token itself is the grant here, permissions mode also checks two claims:
+
+- **`aud`**: the token's audience (a string or an array) must share at least one value with
+  `audience`. A token minted for a different API gets a **401**.


This holds only for routes listed in rbac.json. Routes that are not listed pass straight through, so:

Request Result

aud: billing-api to /orders (listed) 401

aud: billing-api to /admin (not listed) 200

The pass-through is existing behaviour and not something to change here. Could the docs say that audience and exp are checked only on routes with a rule?

Done: Permissions Mode now says aud and exp are checked only on routes with a rule, and links to Unmatched Routes Behavior.

aryanmehrotra · 2026-10-05T15:32:39Z

+
+	// OAuth verifies the token and puts its claims in the request; RBAC then reads the "scope"
+	// claim named in configs/rbac.json. OAuth must be enabled first.
+	if err := app.EnableOAuth(app.Config.Get("JWKS_URL"), 10); err != nil {


Permissions mode trusts what the token says, and nothing here checks who issued it. A token with iss: https://evil-idp, signed by a key in the same JWKS, gets a 200.

EnableOAuth already takes parser options, and jwt.WithIssuer(...) closes it. I tried it:

Token iss Result

the configured issuer 200

another issuer 401

none 401

Could the example pass it, and the Permissions Mode docs mention it? No RBAC change needed.

Done. The example passes jwt.WithIssuer(app.Config.Get("TOKEN_ISSUER")), and its test covers the matching issuer (200), another issuer (401) and no iss (401). Permissions Mode says RBAC does not check iss and shows the same option.

aryanmehrotra · 2026-10-05T15:32:39Z

+
+### Permissions Mode (scope claim)
+
+Many identity providers put what a token may do straight into the token, in a `scope` claim


Worth one caveat here: this mode is only as strict as the identity provider. If the provider grants a client any scope it asks for, every user holds every permission. Some need per-user RBAC or a policy turned on before scopes are restricted. A sentence telling readers to check that would do.

Done: added a paragraph telling readers to check how their provider decides which scopes a token gets.

aryanmehrotra · 2026-10-05T15:32:39Z

+	// Role is the role (or, in permissions mode, the permission) that granted access. On a denial
+	// it is the role only when a single role was held; with a role array or permissions claim it is
+	// empty and HeldCount says how many were held instead.
+	Role      string `json:"role,omitempty"`


Optional. In permissions mode this field holds a permission ("role":"orders:read"), which a log consumer could read as a role name. Fine to leave as is if you would rather not add a field.

Taken: in permissions mode the granting permission now goes in a new permission field, and role stays empty. Moving it later would break log consumers, so it seemed better to do now.

aryanmehrotra

Requesting changes for the comments in my review above. The ones that must land before merge are the behaviour changes: a number or bool role claim has to keep working as it does on development, and the 401/403 cases should follow RFC 6750. The rest are small.

Review round 1 on gofr-dev#4434: - Roles mode reads a number or boolean claim as its text again (123 is role "123"), as on development; only an object is unreadable. A null role claim is a 403 again. - Permissions mode: a missing, null, "" or [] claim holds nothing and gets a 403, not a 401. - Export ErrAudienceMismatch and ErrMissingExpiry, each with its own 401 body; both still wrap ErrRoleNotFound. - rbac_role_extraction_failures gets a reason label. - Reject an empty audience entry at load; warn when permissions mode has a roles section. - No call-order warning for /.well-known/* paths, which OAuth skips. - AuditLog.Permission holds the granting permission in permissions mode. - Example checks the issuer with jwt.WithIssuer; docs cover the rule-only scope of aud/exp, the issuer and the IdP caveat.

aryanmehrotra · 2026-10-13T06:34:29Z

 	claims, ok := r.Context().Value(middleware.JWTClaim).(jwt.MapClaims)
 	if !ok || claims == nil {
-		return "", fmt.Errorf("%w", errJWTClaimsNotFound)
+		return held{}, fmt.Errorf("%w: %w", ErrRoleNotFound, errJWTClaimsNotFound)


This changes the error that a custom ErrorHandler receives, so it is not backward compatible. The description says there are no breaking changes.

I ran the same test on development (6ce72f9f8) and on this branch (76bcd1f9d):

Case development This branch

JWT mode, no claims in the context err == ErrRoleNotFound is true false

Role claim is an object err == ErrAccessDenied is true false

errors.Is is still true in both cases. But a handler that uses == or switch err now goes to its default branch.

The text also changes. err.Error() is now unauthorized: role not found: JWT claims not found in request context. A handler that writes err.Error() to the response now shows that internal detail to the client.

Please keep backward compatibility here: return the bare ErrRoleNotFound and ErrAccessDenied on the paths that existed before, and carry the cause for the reason label in a different way. Please also add a test that asserts err == ErrRoleNotFound and err == ErrAccessDenied for these two cases, so a later change cannot break it again.

Done in 9388024. A custom ErrorHandler gets the bare ErrRoleNotFound / ErrAccessDenied again on every path that exists on development, so == and switch err work, and err.Error() carries no internal detail. The cause and the reason label travel in an internal struct that never reaches the handler or the client. TestMiddleware_ClaimModes now asserts assert.Same against the sentinel in every denied case, including your two (no claims in the context, and an object role claim). Both failed before the fix.

aryanmehrotra · 2026-10-13T06:34:31Z

 		return nil, fmt.Errorf("%w", errEmptyClaimPath)
 	}

+	if value, ok := claims[path]; ok {


This lookup runs before the dotted walk, so an existing config can now read a different claim. That is not backward compatible, and here it changes an access decision.

I ran this on development (6ce72f9f8) and on this branch (76bcd1f9d), with jwtClaimPath: "a.b" and this token:

{ "a.b": "admin", "a": { "b": "viewer" } }

Claim read Status

development nested a.b = viewer 403

This branch top-level "a.b" = admin 200

The same token and the same config get a grant where they got a denial. The description says every claim shape that worked on development behaves the same.

Please keep backward compatibility: do the dotted walk first, and use the whole-key lookup only when the walk finds nothing. That still supports a namespaced claim name, and it does not change the result for any config that works today. Please add a test for a token that has both keys.

Done. The dotted walk runs first, as on development; the whole-key lookup is only a fallback when the walk finds nothing. Your token ({"a.b": "admin", "a": {"b": "viewer"}} with jwtClaimPath: "a.b") reads viewer and gets the 403 again. There's a case for it in TestExtractClaimValue and one in TestMiddleware_ClaimModes.

aryanmehrotra · 2026-10-13T06:37:04Z

+		return held{}, fmt.Errorf("%w: %w", ErrRoleNotFound, errJWTClaimsNotFound)
+	}
+
+	if len(config.Audience) > 0 && !audienceAccepted(claims["aud"], config.Audience) {


Why does the aud and exp check need to be in RBAC?

EnableOAuth already accepts jwt.ParserOption (pkg/gofr/auth.go:124), and the pinned golang-jwt/jwt/v5 v5.3.1 has jwt.WithAudience(...) and jwt.WithExpirationRequired(). Those options run on every request that has a token.

This check runs only on a route that has an RBAC rule. I tested it with audience: ["api"] and a token with aud: "other-api":

Request Status

GET /x (has a rule) 401

GET /norule (no rule) 200

So a token for a different audience is still accepted on each route without a rule. A user who sets audience in rbac.json will expect the opposite.

The example also ends with iss in Go code and aud in rbac.json, so token validation is in two places.

If the reason is to make audience mandatory at load in permissions mode, please say that in the description. Otherwise I prefer to document the two parser options, and to drop Audience, ErrAudienceMismatch, and ErrMissingExpiry. That removes three exported identifiers and one config key.

There was no good reason; you're right. I removed the check: Audience, ErrAudienceMismatch and ErrMissingExpiry are gone, with the config key and the load-time audience rules. The docs' permissions-mode section and the example now validate the token in EnableOAuth with jwt.WithAudience, jwt.WithIssuer and jwt.WithExpirationRequired, and say that these run on every request that carries a token. The example's test still covers a wrong audience and a missing exp; they are now rejected by OAuth.

aryanmehrotra · 2026-10-13T06:37:06Z

+
+	// ErrAudienceMismatch is returned when the token's aud shares no value with the configured
+	// audience. It wraps ErrRoleNotFound, so it is a 401.
+	ErrAudienceMismatch = fmt.Errorf("%w: token audience not accepted", ErrRoleNotFound)


These two errors wrap ErrRoleNotFound to get the 401. The result is a wrong message:

unauthorized: role not found: token audience not accepted unauthorized: role not found: token has no expiry

An audience mismatch is not a missing role. sanitizeErrorForTrace (line 513) also matches them with errors.Is(err, ErrRoleNotFound), so the span records "role not found" for both.

If these errors stay, please use a neutral unauthorized sentinel that all three share, and keep ErrRoleNotFound for a role that is missing.

Resolved by removing the two errors (see the aud/exp thread). ErrRoleNotFound is back to meaning a missing role only.

aryanmehrotra · 2026-10-13T06:37:07Z

-	if errors.Is(err, ErrRoleNotFound) {
+	// Default error handling. A 401 means the token itself is not acceptable; a 403 means it is, but
+	// holds nothing that grants access (RFC 6750 §3.1).
+	switch {


The comment here, claims.go:27, and docs/advanced-guide/rbac/page.md:246 say these responses follow RFC 6750 §3.1. Only the status codes do.

That section also requires a WWW-Authenticate: Bearer header, with error="invalid_token" on the 401 and error="insufficient_scope" on the 403. I checked both responses:

Response WWW-Authenticate

401, audience mismatch empty

403, role without the permission empty

Please set the header, or change the docs to say that only the status codes follow the RFC.

I changed the claim rather than adding the header. The comment here, claims.go and the docs now say that only the status codes follow RFC 6750 §3.1, and that no WWW-Authenticate header is set.

aryanmehrotra · 2026-10-13T06:37:08Z

+	switch {
+	case errors.Is(err, errUnreadableClaim):
+		_, path := config.mode()
+		config.Logger.Warnf("RBAC: claim %q is not a string or an array of strings; request denied", path)


This warns on every request. I sent 1000 requests with a role claim that is an object, and the logger received 1000 WARN lines.

An identity provider that sends the claim in the wrong shape then fills the log at request rate. The metric already counts these with reason=unreadable_claim.

Please warn one time, as the call-order warning in the next case does.

Done. The unreadable-claim warning is logged once per config, like the call-order warning, and the metric still counts every request with reason=unreadable_claim. Covered by TestMiddleware_UnreadableClaimWarnsOnce.

aryanmehrotra · 2026-10-13T06:37:10Z

+		return errAudienceRequired
+	case len(c.Audience) > 0 && c.JWTClaimPath == "" && c.PermissionsClaimPath == "":
+		return errAudienceWithoutJWT
+	case slices.ContainsFunc(c.Audience, func(a string) bool { return strings.TrimSpace(a) == "" }):


This rejects an entry that is empty after a trim, but audienceAccepted compares the untrimmed value. I tested three configs that load with no error:

Config Result

"audience": [" orders-api "] A token with aud: "orders-api" gets a 401 on every request

"permissionsClaimPath": "scope[x]" Every request gets a 403, with no warning

permissionsClaimPath together with roleHeader The header is ignored, with no warning

The comment on this function says it rejects settings that are "silently ignored", and the third row is one.

Please trim the audience entries at load, or reject an entry that has leading or trailing space. Please also reject a claim path that cannot be parsed, and warn when roleHeader is set in permissions mode.

Audience: moot now, since the setting is gone.

"permissionsClaimPath": "scope[x]" now fails at load with an error that says the path cannot be parsed. I left jwtClaimPath alone, because rejecting it at load would stop configs from loading that load on development.

roleHeader together with permissionsClaimPath now logs a WARN at load saying the header is ignored.

There are tests for both in TestLoadPermissions_ClaimModes.

aryanmehrotra · 2026-10-13T06:37:11Z

+// grantFromList checks an array claim entry by entry, skipping empty and non-string entries.
+func (h held) grantFromList(required []string, roles map[string]map[string]struct{}) (string, bool) {
+	for _, entry := range h.list {
+		name, ok := entry.(string)


Not a regression, because development does the same, but the two rules disagree:

Claim Status

"roles": 123 200 (read as role "123")

"roles": [123] 403 (the entry is skipped)

This PR rewrites the array path, so it is a good time to make the two the same. If the scalar rule must stay for compatibility, please say in the docs why an array entry is different.

Made them the same for roles mode: a number or boolean entry of a role array is read as its text, like a scalar claim, so "roles": [123] holds role "123". Arrays never matched a role on development, so this changes nothing that works today. Objects and null entries are still skipped. In permissions mode a number is unreadable as a scalar, so array entries stay skipped there too, which keeps that mode consistent. The docs say this, and TestHeldFromClaim covers both shapes.

aryanmehrotra · 2026-10-13T06:37:12Z

-func extractRoleFromJWT(r *http.Request, claimPath string) (string, error) {
+// extractHeldFromJWT reads the claim at claimPath from the JWT claims in the request context, after
+// checking the token's audience (when configured) and, in permissions mode, that it expires.
+func extractHeldFromJWT(r *http.Request, config *Config, claimPath string, permissionsMode bool) (held, error) {


Some smaller points on the internal signatures in this file:

claimPath and permissionsMode both come from config, and config.mode() already returns them. Passing them again lets the two disagree.

The same flag has three names: permissionsMode here, split in heldFromClaim (claims.go:29), and perms in held (claims.go:20). Please use one name.

handleAuthError (line 261) now has 7 parameters, with role, route string next to each other. A swap of the two compiles. A small struct for the denial details is safer.

Done. The claim path and mode come from config.mode() only, the flag is permissionsMode in every file, and handleAuthError takes a *denial struct instead of 7 parameters.

aryanmehrotra · 2026-10-13T06:37:14Z

Please rename this file to README.md. That is the spelling in 20 of the 27 readme files in the repo, and in 17 of the 24 under examples/.

The rename changes only the letter case, so use git mv ReadMe.md README.md. A plain rename can fail on a file system that ignores case.

Renamed with git mv to README.md.

aryanmehrotra

The feature works and the tests pass. Role arrays and permissions mode gave the expected results in my tests. Please make these changes before the merge. The details are in the inline comments.

Backward compatibility (blocking). The description says there are no breaking changes, but I found two:

The error given to a custom ErrorHandler is now wrapped. err == ErrRoleNotFound and err == ErrAccessDenied were true on development and are false here.
A dotted jwtClaimPath now reads the top-level key first. With jwtClaimPath: "a.b" and a token that has both keys, development returns 403 and this branch returns 200.

Please keep the old behavior in both cases and add a test for each.

Design.

The aud and exp checks repeat what jwt.WithAudience and jwt.WithExpirationRequired do in EnableOAuth, and they do not run on a route without a rule.
ErrAudienceMismatch and ErrMissingExpiry wrap ErrRoleNotFound, so the message and the span say "role not found".

Smaller points.

The RFC 6750 claim: no response sets WWW-Authenticate.
One WARN line per request for an unreadable claim.
Three config check gaps, including an untrimmed audience entry that gives a 401 on every request.
"roles": 123 grants and "roles": [123] does not.
Internal signatures in middleware.go.
Rename examples/using-rbac-permissions/ReadMe.md to README.md.

…to EnableOAuth Review round 2 on gofr-dev#4434: - A custom ErrorHandler gets the bare ErrRoleNotFound / ErrAccessDenied again; the cause and metric reason travel separately. - A dotted claim path is walked first; the literal key is a fallback. - Drop the audience/expiry checks (Audience, ErrAudienceMismatch, ErrMissingExpiry). Docs and example validate the token in EnableOAuth with jwt.WithAudience, WithIssuer and WithExpirationRequired instead. - Docs: only the status codes follow RFC 6750. - Warn once for an unreadable claim; reject an unparsable permissionsClaimPath; warn when roleHeader is set in permissions mode. - Read number/bool entries of a role array as text, like a scalar claim. - Internal: mode from config.mode(), one flag name, a denial struct for handleAuthError. - Rename the example's ReadMe.md to README.md.

aryanmehrotra

Thanks, every thread from the last round is addressed at 9388024a5. I read the code for each one, and I also changed the code back in 11 places (wrapped error to the ErrorHandler, whole-key lookup first, null as 401, the privacy rules, the /.well-known skip, numbers in permissions mode) to see if a test fails. A test failed every time.

Check at `9388024a5`	Result
`go test -race`, `rbac` and the example	pass
Coverage, `pkg/gofr/rbac`	94.4%
`golangci-lint`	0 issues
Benchmark	3 allocs/op in all 3 modes

One change is needed before the merge, and it is in the example and the docs, not in the RBAC code:

jwt.WithIssuer with an empty value does not check the issuer. The example and the docs read the issuer from config, so a missing TOKEN_ISSUER turns the check off with no error. Details are in the inline comment.

One point for the description:

The whole-key fallback can change a 401 on development into a grant. I think the behaviour is fine. Please say it in the description and the docs.

The other three comments are small and optional.

aryanmehrotra · 2026-10-13T10:32:33Z

+	// belongs here, where it runs on every request that carries a token: a token from another issuer,
+	// minted for another API, or with no expiry gets a 401 even if a key in the same JWKS signed it.
+	if err := app.EnableOAuth(app.Config.Get("JWKS_URL"), 10,
+		jwt.WithIssuer(app.Config.Get("TOKEN_ISSUER")),


If TOKEN_ISSUER is not set, this passes "", and the issuer is then not checked at all.

The pinned golang-jwt/jwt/v5 v5.3.1 runs the check only for a non-empty value (validator.go:138, if v.expectedIss != ""). I tested it with a token that has iss: https://evil-idp/:

Option Result

jwt.WithIssuer("https://auth.example.com/") token has invalid issuer

jwt.WithIssuer("") no error

jwt.WithAudience("") token has invalid audience

So a missing or misspelled env var turns off the check that the docs call necessary for permissions mode, and the app starts with no warning. The audience option does not have this problem.

Please stop at startup when the issuer is empty:

issuer := app.Config.Get("TOKEN_ISSUER") if issuer == "" { app.Logger().Fatalf("TOKEN_ISSUER is not set") }

People will copy this example, so it should fail closed.

Good catch, thanks for testing it. The example now reads TOKEN_ISSUER first and calls Fatalf("TOKEN_ISSUER is not set") when it is empty, then passes it to jwt.WithIssuer. Fixed in c20456b.

aryanmehrotra · 2026-10-13T10:32:33Z

+
+```go
+app.EnableOAuth(app.Config.Get("JWKS_URL"), 10,
+    jwt.WithIssuer(app.Config.Get("TOKEN_ISSUER")),


Same point as in the example: with an unset TOKEN_ISSUER this is jwt.WithIssuer(""), which checks nothing. Please add one sentence that an empty issuer disables the check, and that the app should refuse to start without it.

Small: the snippet drops the error that EnableOAuth returns. The example handles it, so the snippet could do the same.

Added a sentence that an empty issuer disables the check and the app should refuse to start without it. The snippet now has the same empty-issuer guard and handles the error from EnableOAuth, like the example. Fixed in c20456b.

aryanmehrotra · 2026-10-13T10:32:33Z

+		return value, nil
+	}
+
+	if value, ok := claims[path]; ok {


The order is right now, and the token from my last comment gets its 403 again. One case still changes, and I think it is acceptable, but the description does not say it.

When the dotted walk fails, development returns a 401. This branch then reads the whole key. I tested with jwtClaimPath: "a.b":

Token development This branch

{"a": "x", "a.b": "admin"} 401 role admin

{"realm_access.roles": ["admin"]} with path realm_access.roles 401 role admin

The description says the fallback works "without changing what any existing config reads". That is true for a config that reads a claim today. It is not true for a request that got a 401.

The identity provider must put a flat key with dots into a signed token for this to happen, so the risk is low. No code change needed. Please add this case to the description and to the claim-path paragraph in the docs.

Agreed, no code change. I added both of your cases to the claim-path paragraph in the docs and to the PR description (behavior changes). TestExtractClaimValue now has two cases that pin it: {"a": "x", "a.b": "admin"} with a.b, and {"realm_access.roles": ["admin"]} with realm_access.roles. In c20456b.
I also checked that the cases are not vacuous: with the whole-key fallback temporarily disabled, both new cases fail (and so does the existing "a top-level key with dots is used when the walk finds nothing" case). After restoring it they pass again.

aryanmehrotra · 2026-10-13T10:32:33Z

+// logStartup logs the mode RBAC runs in, and warns about a roles section permissions mode never reads.
+func (c *Config) logStartup(logger datasource.Logger) {
+	mode, source := c.mode()
+	logger.Infof("RBAC enabled: mode=%s, claim=%s", mode, source)


Optional. In header mode this prints mode=header, claim=X-User-Role, and a header is not a claim. With no source set it prints mode=none, claim=, and then every route with a rule gets a 401.

A neutral name such as source= fits all modes. A WARN for mode=none would also help, but that part is existing behaviour and can be a separate change.

Done in 49f94be: the startup line is now RBAC enabled: mode=<mode>, source=<path or header> in every mode. I left the WARN for mode=none out, as you suggested, for a separate change. The existing startup-line table in TestLoadPermissions_ClaimModes covers it, and the docs example is updated.

aryanmehrotra · 2026-10-13T10:32:33Z

 	}

+	if metrics != nil {
+		metrics.NewCounter(extractionFailuresMetric, "Number of requests whose RBAC role or permissions could not be read")


Optional. If LoadPermissions runs two times with the same metrics manager (for example EnableRBAC called twice), the second NewCounter logs an ERROR for a duplicate metric. The counter still works. Fine to leave as is.

Done in 49f94be: the counter is registered once per metrics manager, so loading RBAC twice with the same manager no longer logs the duplicate-metric ERROR. The manager gives the caller no signal for an existing name, so a small unexported registry remembers which managers already hold the counter. TestLoadPermissions_RegistersMetricOncePerManager covers it.

aryanmehrotra · 2026-10-13T10:32:33Z

+	Role string `json:"role,omitempty"`
+	// Permission is, in permissions mode, the permission that granted access.
+	Permission string `json:"permission,omitempty"`
+	HeldCount  int    `json:"held_count,omitempty"`


Optional. Because of omitempty, a permissions-mode denial for a token that holds nothing logs no role and no held_count. In the log it then looks the same as a denial where the role was not read. Without omitempty the entry would show "held_count":0. Fine to leave as is if you prefer the shorter line.

Done in 49f94be: held_count no longer has omitempty, so a permissions-mode denial that held nothing logs "held_count":0. As you noted, the field now also shows on single-role lines. TestAuditLog_HeldCountIsAlwaysLogged covers it.

…-key claim fallback An empty TOKEN_ISSUER turns jwt.WithIssuer into a no-op, so the example and the docs snippet now stop at startup without it. The whole-key fallback can allow a request that got a 401 on development; the docs say so and two table cases pin it.

…manager, always log held_count claim= is wrong for header mode, so the startup line names the source in every mode. Loading RBAC twice with one metrics manager logged a duplicate-metric ERROR; the manager gives the caller no signal, so remember which managers already hold the counter. held_count loses omitempty so a denial that held nothing is distinguishable from one where nothing was read.

akshat-kumar-singhal · 2026-10-13T12:55:45Z

Thanks for the careful review. The empty-issuer case is fixed in the example and the docs (c20456b), and the flat dotted key behavior is documented and pinned by tests. I also took the three optional points in 49f94be: source= in the startup line, one counter registration per metrics manager, and an always-present held_count. go test -race ./pkg/gofr/rbac/... passes and lint reports 0 issues.

aryanmehrotra

The earlier comments are all fixed. I ran go test -race -count=1 on pkg/gofr/rbac and the new example, and the lint reports 0 issues. I also compared 14 roles-mode claim shapes against development (3b84b01c3). Only the two documented cases differ: a role array now grants, and a flat dotted key now grants.

Two small changes remain. The details are in the inline comments.

The global counterRegistered map keeps every metrics manager for the life of the process.
The value that handlers read for the role has changed, and no text says so.

aryanmehrotra · 2026-10-14T09:32:07Z

+// counterRegistered holds the metrics managers that already registered extractionFailuresMetric.
+//
+//nolint:gochecknoglobals // registry of managers, safe for concurrent use; the manager API cannot report a duplicate.
+var counterRegistered sync.Map


This package-level sync.Map has two problems.

It keeps a reference to every metrics manager for the life of the process, so a manager can never be freed.

A map key must be comparable. If a container.Metrics implementation has a non-comparable dynamic type, such as a struct holding a slice or map and passed by value, LoadOrStore panics inside EnableRBAC. I found no such implementation today, and I did not test this.

Could the registry live on something with the manager's own lifetime? One option is a field on Config, which would still cover the EnableRBAC called twice case if the second load reuses the first config. Another is a weak.Pointer key. If you keep the global map, please say in the comment why that is safe.

Removed in 112c8bd7. I looked at how the other opt-in features do it: GraphQL, gRPC, cron and LLM all register their metrics from the App method that enables them, with App state as the "done once" check (a.cron == nil, a.graphqlManager == nil, !a.container.HasLLM()). RBAC now does the same. EnableRBAC registers rbac_role_extraction_failures on its first call on an app, guarded by a private rbacEnabled field on App. LoadPermissions registers nothing. So there is no package-level map, nothing outlives its app, and no manager is ever used as a map key.

Tests: TestLoadPermissions_RegistersNoMetric (a strict mock, so any registration fails) and TestEnableRBAC_RegistersExtractionCounterOncePerApp (one call, two calls on one app, two apps). Without the guard, the two-call cases go red.

aryanmehrotra · 2026-10-14T09:32:07Z

-			// Store role in context and continue
-			ctx := context.WithValue(r.Context(), userRole, role)
+			// Store the granting role in context and continue
+			ctx := context.WithValue(r.Context(), userRole, granted)


The value stored under userRole is now the role or permission that granted access. On development it was the extracted role. With a single role the two are the same. With a role array it is the one role that granted. In permissions mode it is a permission name, not a role.

I found no reader of this key outside the package. Still, the PR description says nothing changed here. Please add one sentence to the description and to the docs: what is stored in each mode.

Agreed, that should have been stated. Added in 112c8bd7: a sentence in the RBAC docs ("Accessing Role in Handlers") and a bullet in the description. The value is the role when the request holds one, the granting role when the claim is a role array, and the granting permission in permissions mode. Public routes store nothing, as on development. I also updated the code comment above the context.WithValue call.

…a global EnableRBAC now registers rbac_role_extraction_failures the first time it runs on an app, guarded by a field on App, the way GraphQL, gRPC, cron and LLM register their metrics. LoadPermissions registers nothing, and the package-level registry of metrics managers is gone: it held every manager for the life of the process and would panic on a non-comparable one. docs: say what RBAC stores in the request context in each mode.

aryanmehrotra

Both comments from the last round are fixed at 112c8bd7c. One change is needed before the merge: the branch conflicts with development.

Last round                    At 112c8bd7c
────────────────────────────  ─────────────────────────────────────────────
global counterRegistered map  Gone. App.rbacEnabled guards NewCounter once per app
userRole value not stated     Stated in docs (page.md) and in the code comment
Merge state                   CONFLICTING

I ran go test -race -count=1 on pkg/gofr/rbac and examples/using-rbac-permissions, and go test -run RBAC ./pkg/gofr/. All pass. The new registration in pkg/gofr/rbac.go follows the same pattern as GraphQL, gRPC, cron and LLM.

Required

Rebase on development. #4437 (09379dd06, "log an inheritsFrom role that matches no defined role") changed pkg/gofr/rbac.go, pkg/gofr/rbac_test.go, pkg/gofr/rbac/config.go and pkg/gofr/rbac/config_test.go. Please resolve the conflicts and run the same tests again on the new head. I will review the resolved head.

Small, optional

pkg/gofr/rbac.go:79 writes the counter name as a string. The middleware increments the unexported constant extractionFailuresMetric (rbac/config.go:45, used at middleware.go:236). If the two names differ, IncrementCounter finds no counter (metrics/register.go:175-181). It then logs an ERROR on every failed request and counts nothing. Please use one exported name in both places, or add a test that fails when they differ.
LoadPermissions no longer registers the counter. A caller who uses rbac.LoadPermissions and rbac.Middleware directly (both are public) gets that ERROR on each extraction failure. This was also the case on development. Please add one sentence to the LoadPermissions comment or the docs: App.EnableRBAC registers the counter.

aryanmehrotra · 2026-10-14T13:53:33Z

also resolve the merge conflicts

…lti-value-claims-4388 # Conflicts: # pkg/gofr/rbac.go # pkg/gofr/rbac_test.go

…ffer The extraction-failure counter is registered in App.EnableRBAC by name and incremented in rbac.Middleware through extractionFailuresMetric. Compare the two names in the test instead of pinning each to a literal, so renaming either side alone fails. Note on LoadPermissions that it does not register the counter; App.EnableRBAC does.

akshat-kumar-singhal · 2026-10-14T14:29:14Z

Thanks, all three are done.

Conflicts. I merged development (09379dd0, fix(rbac): log an inheritsFrom role that matches no defined role #4437) in a70e2315. The only conflicts were in pkg/gofr/rbac.go and pkg/gofr/rbac_test.go. In rbac.go, fix(rbac): log an inheritsFrom role that matches no defined role #4437 moved loading into loadRBACConfig, which now logs the "Loaded RBAC config" line, so I dropped this branch's copy of that line and kept the once-per-app counter registration after it. In rbac_test.go both tests are kept. On the new head, go test -race -count=1 on pkg/gofr/rbac/... and examples/using-rbac-permissions, go test -race -run RBAC ./pkg/gofr/, the full ./pkg/gofr/ package and golangci-lint all pass.
Counter name. I kept the constant unexported and added the test instead (9a05edb9). TestEnableRBAC_RegistersExtractionCounterOncePerApp now records the name passed to NewCounter and the name the middleware passes to IncrementCounter on a rejected request, then checks they are equal. It compares the two names rather than pinning either one to a string, so renaming either side alone fails it. I checked this both ways: renaming the string in rbac.go fails it, and so does renaming extractionFailuresMetric.
LoadPermissions. Its comment now says it does not register the counter, that App.EnableRBAC does, and that a caller using LoadPermissions and Middleware directly must register it.

While testing item 2 I also saw the "not registered" error log as "message":{}, which is the second defect in #4460. This PR covers the first part of #4460, registering rbac_role_extraction_failures in EnableRBAC; the empty log message in pkg/gofr/metrics is left for #4460.

aryanmehrotra

Approving at 9a05edb92.

Last round (112c8bd7c)               Now (9a05edb92)
───────────────────────────────────  ──────────────────────────────────────────
CONFLICTING with development         Merged at a70e23151, clean
counter name: string vs constant     Test compares the two names
LoadPermissions: no doc on counter   Comment says EnableRBAC registers it

The merge with #4437 is correct. loadRBACConfig logs the "Loaded RBAC config" line once, and the counter is still registered once per app. If I rename the registered counter, TestEnableRBAC_RegistersExtractionCounterOncePerApp fails.

I ran go test -race -count=1 on pkg/gofr/rbac and examples/using-rbac-permissions, and -run RBAC ./pkg/gofr/. All pass. golangci-lint reports 0 issues.

CONTRIBUTING check: no mutable globals (only sentinel errors, as on development), no init, no new dependency, and American English is used in the added lines. In my benchmark run, allocations stay at 3 allocs/op in all three modes. Roles mode costs about 2.1x the header baseline in ns/op.

One small, optional point. CONTRIBUTING asks for tests on all new lines. AuditLog.PrettyPrint (middleware.go:47, new Permission and held_count branches) and extractionFailure.Error and .Unwrap (middleware.go:212, :214) show 0.0% coverage. Please add one PrettyPrint test with a role, a permission and a held count. Unwrap may be unused, because bareError uses errors.AsType.

Covers each branch of the held column (role, permission, held count, nothing held). Drops extractionFailure.Unwrap: the one path that sees an extractionFailure strips it with bareError first, so nothing unwraps it.

akshat-kumar-singhal · 2026-10-15T07:02:30Z

Thanks. Added in 4e9f0b5: TestAuditLog_PrettyPrint covers the role, permission, held-count and nothing-held cases, and TestExtractionFailure_Error covers Error. Both are 100% covered now.

You were right about Unwrap: the only path that sees an extractionFailure strips it with bareError first (middleware.go:154), so I removed it.

aryanmehrotra

Re-reviewed the new head d44a04a3f. Every open point from my last review is fixed.

Last round	At `d44a04a3f`
Conflicts with `development` (#4437)	Resolved in `a70e23151`. `loadRBACConfig` keeps the "Loaded RBAC config" line, and the once-per-app counter registration follows it. The later merges are clean.
Counter name as a string in `rbac.go`	`TestEnableRBAC_RegistersExtractionCounterOncePerApp` now compares the name given to `NewCounter` with the name given to `IncrementCounter`. I renamed the string in `rbac.go`: the test fails.
`LoadPermissions` does not register the counter	The `LoadPermissions` comment now says so (`config.go`).
`extractionFailure.Unwrap` (author's own removal in `4e9f0b56e`)	Safe. `bareError` strips the failure before `handleAuthError`, the trace sanitizer and `ErrorHandler`. Nothing calls `errors.Is` on a raw failure.

Checks at this head: go test -race -count=1 on pkg/gofr/rbac, examples/using-rbac-permissions and go test -run RBAC ./pkg/gofr/ pass. Coverage on pkg/gofr/rbac is 96.1%, go vet is clean and golangci-lint reports 0 issues.

akshat-kumar-singhal mentioned this pull request Oct 5, 2026

RBAC: authorize on a token's scope/permissions claim, and add the role/permission extractor hook agreed in #2144 #4388

Open

fix(rbac): name the RBAC modes as constants for goconst

a22c423

aryanmehrotra reviewed Oct 5, 2026

View reviewed changes

aryanmehrotra requested changes Oct 5, 2026

View reviewed changes

akshat-kumar-singhal requested a review from aryanmehrotra October 6, 2026 06:23

Merge branch 'development' into feat/rbac-multi-value-claims-4388

76bcd1f

aryanmehrotra reviewed Oct 13, 2026

View reviewed changes

aryanmehrotra requested changes Oct 13, 2026

View reviewed changes

akshat-kumar-singhal requested a review from aryanmehrotra October 13, 2026 07:24

aryanmehrotra requested changes Oct 13, 2026

View reviewed changes

akshat-kumar-singhal added 2 commits October 13, 2026 17:46

Merge branch 'development' into feat/rbac-multi-value-claims-4388

3340360

aryanmehrotra requested changes Oct 14, 2026

View reviewed changes

akshat-kumar-singhal added 2 commits October 14, 2026 19:53

Merge remote-tracking branch 'upstream/development' into feat/rbac-mu…

a70e231

…lti-value-claims-4388 # Conflicts: # pkg/gofr/rbac.go # pkg/gofr/rbac_test.go

aryanmehrotra previously approved these changes Oct 15, 2026

View reviewed changes

aryanmehrotra and others added 2 commits October 15, 2026 11:45

Merge branch 'development' into feat/rbac-multi-value-claims-4388

3654b21

test(rbac): cover AuditLog.PrettyPrint and extractionFailure.Error

4e9f0b5

Covers each branch of the held column (role, permission, held count, nothing held). Drops extractionFailure.Unwrap: the one path that sees an extractionFailure strips it with bareError first, so nothing unwraps it.

akshat-kumar-singhal dismissed aryanmehrotra’s stale review via 4e9f0b5 October 15, 2026 07:02

Merge branch 'development' into feat/rbac-multi-value-claims-4388

d44a04a

aryanmehrotra approved these changes Oct 15, 2026

View reviewed changes

Cause	What it tells an operator
audience mismatch	a token for another API, or a wrong `audience`
no claims in the request	RBAC is not behind OAuth
no `exp`, missing or unreadable claim	an IdP or client problem

Request	Result
`aud: billing-api` to `/orders` (listed)	401
`aud: billing-api` to `/admin` (not listed)	200


		### Permissions Mode (scope claim)

		Many identity providers put what a token may do straight into the token, in a `scope` claim

Case	`development`	This branch
JWT mode, no claims in the context	`err == ErrRoleNotFound` is true	false
Role claim is an object	`err == ErrAccessDenied` is true	false

	Claim read	Status
`development`	nested `a.b` = `viewer`	403
This branch	top-level `"a.b"` = `admin`	200

Response	`WWW-Authenticate`
401, audience mismatch	empty
403, role without the permission	empty

Config	Result
`"audience": [" orders-api "]`	A token with `aud: "orders-api"` gets a 401 on every request
`"permissionsClaimPath": "scope[x]"`	Every request gets a 403, with no warning
`permissionsClaimPath` together with `roleHeader`	The header is ignored, with no warning

Claim	Status
`"roles": 123`	200 (read as role `"123"`)
`"roles": [123]`	403 (the entry is skipped)

Option	Result
`jwt.WithIssuer("https://auth.example.com/")`	`token has invalid issuer`
`jwt.WithIssuer("")`	no error
`jwt.WithAudience("")`	`token has invalid audience`

Token	`development`	This branch
`{"a": "x", "a.b": "admin"}`	401	role `admin`
`{"realm_access.roles": ["admin"]}` with path `realm_access.roles`	401	role `admin`

Conversation

akshat-kumar-singhal commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aryanmehrotra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aryanmehrotra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akshat-kumar-singhal commented Oct 5, 2026 •

edited

Loading