adityamparikh commented on PR #160:
URL: https://github.com/apache/solr-mcp/pull/160#issuecomment-5341525826
@epugh — re "should we merge this then?": I re-verified the document's
checkable claims against `main` (`a84033b`) rather than against the earlier
review thread. **Everything I can mechanically check is accurate, and it merges
clean. No blockers from me.**
What I verified this pass:
**Config surface (§5a, §13).** Every environment variable the model names
exists in the code exactly as spelled — `HTTP_SECURITY_ENABLED`,
`MCP_CORS_ALLOWED_ORIGINS`, `OAUTH2_ISSUER_URI`, `PROFILES`, `SOLR_URL`,
`SOLR_USERNAME`, `SOLR_PASSWORD`. Worth calling out because our own docs have
repeatedly invented a `SECURITY_ENABLED` toggle with the wrong default; this
document does not.
**"HTTP is secured by default" (§5a).** Matches
`application-http.properties`:
```properties
http.security.enabled=${HTTP_SECURITY_ENABLED:true}
```
Default `true`, so the secured filter chain is the default posture, as the
§5a table states.
**"`@PreAuthorize` on every tool" (§8.1).** Confirmed — 11 `@McpTool`
methods across `CollectionService`, `IndexingService`, `SchemaService` and
`SearchService`, and 26 `@PreAuthorize` annotations covering those tools plus
the prompt/resource methods. No unguarded tool.
**JWT audience validation (§8.1, §9).** This is the claim I most expected to
be wrong, because stock Spring Security validates `iss`/`exp` but *not* `aud`.
It is correct — we explicitly opt in:
```java
.authorizationServer(issuerUrl).resourcePath("/mcp").validateAudienceClaim(true)
```
So "validates sig/iss/exp/aud" is accurate rather than aspirational.
**Provenance (§1).** "No *(inferred)* claims remain" holds — all 13
previously-inferred tags now read *(maintainer)*.
**Tenancy open question.** Resolved in the meantime: #169 landed as
`a57f4d9`, documenting the single-tenant deployment model. §2/§10/§11a already
carry that decision, so nothing here goes stale on merge.
One non-blocking note for future readers, not a change request: the overview
describes HTTP as "streamable-HTTP", which is the right MCP transport name, but
`application-http.properties` also sets
`spring.ai.mcp.server.protocol=stateless`. Those are orthogonal (transport type
vs. mode within it), so the text is not wrong — it just does not mention that
per-request context features (sampling, progress, server→client logging) are
off in our configuration. Fine as a follow-up if the scan ever cares.
LGTM from my side.
--
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]
