fix(auth): return an error when RBAC, basic auth or OAuth cannot be enabled

[akshat-kumar-singhal]

Description:

Fixes #3763. EnableRBAC logs and returns when its config cannot be loaded, so the app starts and serves every route with no role checks. A config path that resolves differently inside a container is enough to trigger it, and the only signal is one error line at startup. EnableBasicAuth (no credentials, or an odd number of arguments) and EnableOAuth (JWKS URL that does not parse, has no scheme or host, or is not http/https) have the same shape, so this PR covers all three.

The three methods now return an error:

• EnableRBAC(configPath ...string) error
• EnableBasicAuth(credentials ...string) error
• EnableOAuth(jwksEndpoint string, refreshInterval int, options ...jwt.ParserOption) error

On bad input a method installs no middleware, logs <reason>. <Authorization | Basic authentication | OAuth authentication> is DISABLED: all routes are served without it. Check the error returned by <method> to stop startup instead, and returns a wrapped error (path or reason included). The caller decides what to do, typically app.Logger().Fatalf("%v", err). The framework does not exit on its own, so apps that genuinely want to start without auth still can.

One gap closed along the way: EnableRBAC() with no argument and none of the default files present resolved to an empty path and failed on reading "". It now returns a clear "no RBAC config file found at configs/rbac.json, configs/rbac.yaml or configs/rbac.yml".

Breaking Changes (if applicable):

Existing call statements such as app.EnableRBAC("configs/rbac.json") compile unchanged. golangci-lint's errcheck flags the unchecked result, and apps that ignore it behave as before but with the louder DISABLED log. Code that declares these methods in its own interface (for example a mock of App) or stores one as a func(...string) value must add the error result. There are no such uses inside gofr.

Additional Information:

• Tests now assert behaviour, not just that a router exists. For each method, an unauthenticated request through the router must get 401 when the middleware is installed and 200 when it is not, alongside the returned error (or nil) and the DISABLED log line. Previously Test_EnableBasicAuth asserted 200 for odd arguments and the RBAC failure cases only checked router != nil, which is how the fail-open went unnoticed.
• RBAC cases: valid JSON, valid YAML, role inheritance, default path, no default file, missing file, invalid JSON, unsupported extension.
• Docs updated to the if err := …; err != nil form: RBAC guide (plus a "Config Load Failures" section and troubleshooting entry), authentication guide, auth-in-Kubernetes guide, and the using-http-auth-middleware example.
• The docs use Logger().Fatalf("%v", err) rather than Logger().Fatal(err): the latter logs "message":{} for an error value, dropping the reason.
• Out of scope: EnableAPIKeyAuth() with zero keys already fails closed; nil validators in the …WithValidator variants; routes missing from the RBAC config pass through (documented behaviour); RBAC does not cover gRPC.

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.

[aryanmehrotra]

Thanks for digging into this, and for the production write-up in #3763. The fail-open is real, and the gap is on our side: nothing in the docs shows how to avoid it.

The capability already exists in the public API, though. rbac.LoadPermissions, rbac.Middleware and app.UseMiddleware have been exported since the RBAC middleware landed, so an app can already fail closed without any new methods on App:

cfg, err := rbac.LoadPermissions("configs/rbac.json", app.Logger(), app.Metrics(), otel.Tracer("gofr-rbac"))
if err != nil {
	app.Logger().Fatalf("rbac: %v", err)
}

app.UseMiddleware(rbac.Middleware(cfg))

I ran this against development (a091e65c9):

Config	Existing API above	EnableRBAC(path)
missing file	exits 1, FATAL rbac: failed to read RBAC config file …	starts; /admin returns 200 with no role, salesman and admin
malformed JSON	exits 1, failed to parse JSON config file …	—
fails validation	exits 1, invalid RBAC config: endpoint[0]: …	—
valid	401 no role / 403 salesman / 200 admin	401 / 403 / 200

For basic auth and OAuth, the inputs that currently disable auth are arguments the caller writes, so they can be checked before the call.

So rather than adding …WithError twins and deprecating three methods, could we keep this PR to:

1. Docs: a "Failing closed on a bad config" section in the RBAC guide with the snippet above, and a pointer to it from the troubleshooting entry.
2. Example: a fail-closed RBAC example under examples/.
3. Optional: keep the louder Authorization is DISABLED … log line in EnableRBAC, pointing at the new docs section. It's a small change and it makes the failure hard to miss.

Two reasons for not adding the twins:

• Setup methods on App log and continue. 3 of the 62 exported methods return an error.
• Where gofr does add configurability, it uses functional options (EnableMCP, AddLLM, AddReadinessCheck). A WithError twin for each method would be a second pattern that we'd have to maintain.

With the scope reduced to docs and an example, this is a docs: change rather than fix(auth).

[akshat-kumar-singhal]

Thanks for running it against development. The table is useful, and I'm happy to add the "Failing closed" docs section and the example you suggested. I'd still like to keep a code fix in this PR, though, because docs alone leave the default where it is.

The default is the bug. Your table's EnableRBAC(path) row doesn't change after a docs-only PR: a missing file still serves /admin with 200 to no role, salesman and admin. That is what happened to us in production (#3763): the config path resolved differently inside the container, one error line went by at startup, and a salesman token called an admin-only endpoint. Every app already calling EnableRBAC stays exposed to that, and the RBAC guide itself calls EnableRBAC in 4 places. The people who hit this are the ones who didn't read the troubleshooting page.

The workaround isn't the same as EnableRBAC. EnableRBAC() with no argument looks for configs/rbac.json, rbac.yaml and rbac.yml. The snippet hard-codes one path, so to match it an app has to call rbac.ResolveRBACConfigPath(""), which returns "" when none of the three exists, and LoadPermissions then fails reading "". The app also has to pass in the logger, metrics and a tracer itself. That is framework plumbing moving into every app, when the point of EnableRBAC is to hide it. It also cuts those apps off from anything the app does with the RBAC config later. I have a draft (#4390, for #3935) that checks the RBAC rules against the registered routes at startup. It needs the App to hold the config, and apps wired by hand through UseMiddleware would not get it.

On "setup methods log and continue": I went through the 61 exported App methods on development, and it holds for about half the setup methods, but what "continue" leads to differs a lot:

• Most of those fail loudly later. When AddMongo, AddCassandra and the other datasources can't connect, they log and carry on (several retry in the background), and requests that use them get errors. AddStaticFiles with a missing directory gives 404s. The failure shows up the first time the feature is used.
• Several already stop the app. AddDBResolver without a primary SQL connection calls Logger().Fatal (external_db.go:232), RegisterService calls Fatalf when container injection fails (grpc.go:260), and route registration calls Fatalf when the HTTP port is blocked (rest.go:58).
• Some already return an error: AddRESTHandlers and AddWSService.
• Only the three auth methods fail open. When EnableRBAC, EnableBasicAuth or EnableOAuth can't apply their config, the app starts and every request succeeds, and nothing at request time shows that anything is wrong. EnableAPIKeyAuth with zero keys already fails closed (every request gets 401).

So log-and-continue is safe for most setup methods because the failure shows up later. For these three it doesn't, and that's the case the error return is for.

On functional options: I'd prefer them too, but none of the three signatures can take one without breaking callers. EnableRBAC(configPath ...string) and EnableBasicAuth(credentials ...string) are variadic strings, and EnableOAuth is already variadic in jwt.ParserOption. Changing any of them breaks every app that calls it today, which I want to avoid.

The twin-plus-deprecation isn't a new pattern either; gofr already uses it. The Cassandra interface kept Query, Exec, ExecCAS, NewBatch, BatchQuery and ExecuteBatch, marked them // Deprecated: … users must use QueryWithCtx (and so on), and added the …WithCtx versions next to them. EnableBasicAuthWithFunc and EnableAPIKeyAuthWithFunc are deprecated the same way, pointing at …WithValidator. This PR does the same. Nothing breaks, existing apps keep compiling and behaving as they do now, and the // Deprecated: line names the replacement, so gopls and staticcheck (SA1019) flag every call site and the migration is a rename plus handling the returned error. The old methods go away at the next major release, like the others.

I'll add the docs section and the example you asked for to this PR as well.

[aryanmehrotra]

Thanks, you're right on the points I checked against development: these three are the only setup methods that fail silently open, and docs alone leave that default in place.

I ran the branch against development:

Bad config (missing/invalid RBAC file, odd/zero basic-auth args, bad JWKS URL)	Result
development	app starts, /admin → 200 for everyone
this PR, existing methods	app starts, /admin → 200 for everyone, louder log
this PR, …WithError	error returned, app exits ✅

The twins work, but existing callers stay open until they find the new method, and App goes from 9 to 12 Enable* methods.

Could we return the error from the existing methods instead?

func (a *App) EnableRBAC(configPath ...string) error
func (a *App) EnableBasicAuth(credentials ...string) error
func (a *App) EnableOAuth(jwksEndpoint string, refreshInterval int, options ...jwt.ParserOption) error

existing call   app.EnableRBAC("x.json")   → still compiles
golangci-lint   (errcheck, on by default)  → "Error return value of app.EnableRBAC is not checked"
new usage       if err := app.EnableRBAC(); err != nil { app.Logger().Fatalf("%v", err) }

• No new methods and no deprecations: your validation, error messages, the no RBAC config file found at … fix and the behavioural tests all carry over.
• Keep the … is DISABLED error log as well, so callers who ignore the return value still see it.
• The caller still decides, so apps that want to start without auth can.
• Same shape as AddRESTHandlers, casbin.NewEnforcer and echo-jwt's ToMiddleware.

Could you also update the docs and the example to the if err != nil form, and retitle to fix(auth): return an error when RBAC, basic auth or OAuth cannot be enabled?

Out of scope here, to track separately: routes missing from the RBAC config pass through (documented behaviour), and RBAC doesn't cover gRPC.

[akshat-kumar-singhal]

Thanks, done in edcf26a. EnableRBAC, EnableBasicAuth and EnableOAuth now return error, the …WithError methods and deprecations are gone, the DISABLED log stays, the docs and example use the if err != nil form, and the PR is retitled.

I checked the errcheck point: a bare a.EnableRBAC() gives Error return value of a.EnableRBAC is not checked (errcheck).

One thing worth a line in the release notes: adding a return value keeps plain calls compiling, but it does break code that declares these methods in its own interface (for example, a mock of App with EnableRBAC(...string)) or stores one as a func(...string) value. There are no such uses inside gofr, so this only affects apps that do that.

Agreed on the out-of-scope items. #4390 (for #3935) is where unlisted routes come up, and I'll rework it on top of this once it merges.

[aryanmehrotra]

Thanks for the quick turnaround. I ran edcf26ad9 locally against development (da4653594):

Bad input	development	this PR, error checked	this PR, error ignored
RBAC: no default file / missing / bad JSON / .txt / fails validation	starts, /admin → 200 for everyone	exits 1 with the reason	starts, 200, Authorization is DISABLED logged
Basic auth: zero or odd args	starts, 200	exits 1	starts, 200, DISABLED logged
OAuth: ftp:// or path-only URL	starts, 200	exits 1	starts, 200, DISABLED logged

• Valid RBAC: 401 no role / 403 salesman / 200 admin, JSON and YAML, default path too.
• OAuth against a live JWKS with real RS256 tokens: valid → 200; wrong key, expired, missing exp, garbage → 401.
• errcheck flags all three bare calls, and the interface / func(...string) breakage behaves as the PR description says.
• -race clean; changing EnableBasicAuth to swallow its error fails TestEnableBasicAuth_Errors, so the new tests catch it; 0 new lint issues.

One docs nit inline, non-blocking. Please also mention the interface / func(...string) breakage in the release notes as you suggested.

[Umang01-hash]

Verified independently at d6e6c23, including a live server E2E: configured basic auth gives 401/401/200 (no/wrong/correct creds), all four misconfig cases return the expected non-nil errors, and an ignored error serves 200 unprotected + logs the DISABLED warning — fail-closed when handled, detectable when not. Coverage 95.9→96.0, golangci-lint --new-from-rev clean, tests are load-bearing (revert-to-red confirmed). Scoping is right — only the three fail-open methods return an error; EnableAPIKeyAuth and the Func/Validator variants already install middleware (fail-closed).

Adding an error return is source-compatible for the usual call-as-statement usage (confirmed it still compiles), but it's an apidiff-incompatible signature change (a method-value assignment breaks), so worth a release-note line calling it an API change.

One coordination flag, not a code issue: #4168 (slim builds) also rewrites auth.go — it splits the gRPC interceptors out so auth.go no longer imports grpc, whereas this PR keeps them inline. Whichever merges second needs a hand-merge; taking this auth.go wholesale after #4168 would reintroduce the grpc import and silently break the gofr_nogrpc build (only the slim-tag CI job catches it).

LGTM.

[aryanmehrotra]

Re-verified at d6e6c23: the k8s-guide nit is fixed, and the merged-in change touches no PR file. Targeted tests pass with -race, 0 new lint issues. The live matrix is unchanged from edcf26ad9: every bad input exits 1 when the error is checked, and valid configs give 401/403/200. LGTM.