adityamparikh opened a new pull request, #187:
URL: https://github.com/apache/solr-mcp/pull/187
The security configuration currently has **no test that exercises it**. This
adds two, both
validated by mutation.
## The gap
`McpToolRegistrationTest#everyMcpEndpointIsPreAuthorized` reflects over the
service classes and
asserts `@PreAuthorize` is *present* on every MCP entry point. That is a
good guard against
forgetting it on a new tool — but it is static, and cannot tell whether the
annotation has any
runtime effect.
Demonstrated by commenting out `@EnableMethodSecurity` in
`MethodSecurityConfiguration`, which
neuters all 24 `@PreAuthorize` annotations and makes every MCP tool callable
unauthenticated:
| | existing guard | this PR |
|---|---|---|
| `@EnableMethodSecurity` disabled | **BUILD SUCCESSFUL** | **FAILS** |
| `/actuator/**` → `permitAll()` | **BUILD SUCCESSFUL** | **FAILS** |
So today, if the profile gate or the `http.security.enabled` property
condition ever stopped
matching, every tool would be open and CI would stay green.
## What's added
**`MethodSecurityEnforcementTest`** — calls a secured tool through the
Spring proxy with an
empty `SecurityContext` and asserts rejection. Note the exception type: with
no `Authentication`
at all, Spring raises `AuthenticationCredentialsNotFoundException`, not
`AccessDeniedException`
(the latter is for an authenticated principal lacking authority). I got that
wrong on the first
run and the failure output corrected it.
**`HttpSecurityFilterChainTest`** — pins the anonymous-access boundary:
`/actuator/health` open
for probes, `/actuator/sbom/application` and `/actuator/metrics` closed.
That split is a single
`requestMatchers` rule whose justification currently lives only in a code
comment — widening it
to `permitAll()` would expose the dependency tree and the metrics that map
the tool surface, and
would break no existing test.
Denial is asserted as **401-or-403 rather than a fixed code**. With no
issuer configured there
is no authentication entry point, so Spring rejects with 403; wiring an
issuer turns the same
request into a 401 with `WWW-Authenticate: Bearer`. Both are correct denials
— the property
worth pinning is that neither is a 200. Asserting 401 exactly would fail the
day someone
configures an issuer, which is a *good* change.
## Why this went unnoticed
`OtlpExportIntegrationTest` is the only test that activates the `http`
profile without setting
`http.security.enabled=false` — and it is `@Disabled` over an unrelated
Jetty/LGTM container
issue. Every other http-profile test disables security. So no executing test
has ever run with
the security configuration active.
## Notes
- Applies to `main` as-is; the security classes are identical on the `sb4`
branch, so this
flows there on the next rebase.
- 376 tests, 0 failures (baseline 372).
- Both tests are `@Tag("integration")` + `@DisabledInNativeImage`, matching
the convention for
Testcontainers-backed and proxy-dependent tests.
## Not covered here
Deliberately out of scope, worth separate issues if wanted: OAuth2 wiring
when an issuer *is*
configured (the Nimbus decoder is eager, so it needs a reachable issuer or a
mock), the
`validateAudienceClaim(true)` behaviour the MCP Authorization spec requires,
and the CORS
configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
