feat(lightspeed): add DCR authentication support for MCP servers - #3347
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
gabemontero
left a comment
gabemontero
left a comment
There was a problem hiding this comment.
hey @maysunfaisal - so claude and I looked at two error scenarios:
- If the user's RBAC policy denies a specific tool invocation
for this, claude's analysis was that the rejection happens inside @backstage/plugin-mcp-actions-backend, not in this code. Lightspeed streams back whatever LCS returns — so the user would see whatever error or refusal the LCS/MCP Actions layer produces in the chat stream. The Lightspeed backend is fully opaque to per-tool authorization outcomes.
Assuming this is correct, I'm curious if it makes sense to catch the LCS/MCP actions layer error and display a potentially more user friendly indication of permission issues.
WDYT?
- Unexpected error minting a Backstage token (
getPluginRequestTokenthrows)
Claude claims the entire chat query fails (I'll post the details in a moment). It then goes on that is a chat query results in querying multiple MCP servers, none of those servers gets called.
I suppose a user prompt could result in multiple queries, but even if so, it seems unpredictable whether calling the other mcp servers is productive i.e. how dependent would the output of the backstage mcp server be.
I'm inclined to leave this be, but thought I would surface it in the review as due diligence to get your and everyone else's take.
Here is the code analysis from claude on gitPluginRequestToken
There are two call sites for getPluginRequestToken, and neither has a local try/catch around it:
A) During /v1/query (chat flow) — buildMcpHeaders() calls getPluginRequestToken at ~line 133. If it throws, the exception propagates unhandled out of buildMcpHeaders and is caught by the outer catch in the /v1/query handler (~line 775):
} catch (error) {
const errormsg = `Error fetching completions from ${provider}: ${error}`;
logger.error(errormsg);
response.status(500).json({ error: errormsg });
}
The entire chat query fails with a 500. Even if only one of several MCP servers uses auth: dcr, a token-minting failure for that one server aborts the whole request — the error is not isolated per-server. The other MCP servers (static-token or other DCR servers) never get their headers built.
B) During POST /mcp-servers/:name/validate (~line 393) — same pattern: no local catch, the outer handler returns 500 with "Validation failed".
Note: there is a guard for when authService/credentials are absent (logs a warning, skips the server), but that only handles the case where the services weren't injected — not a runtime failure in getPluginRequestToken itself.
763129e to
077a7a0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3347 +/- ##
==========================================
- Coverage 54.31% 54.29% -0.03%
==========================================
Files 2349 2345 -4
Lines 89679 89555 -124
Branches 25134 25067 -67
==========================================
- Hits 48707 48620 -87
- Misses 39406 40632 +1226
+ Partials 1566 303 -1263
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
077a7a0 to
4b468a5
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
3b2db5a to
5f5b935
Compare
@gabemontero Thanks for flagging this. You're correct, RBAC tool-level denials happen inside @backstage/plugin-mcp-actions-backend, and Lightspeed is opaque to those outcomes; it streams back whatever LCS returns. Surfacing a user-friendly permission error would require parsing the LCS response stream for authorization-specific error patterns and rendering them differently in the chat UI and that touches the frontend streaming logic and response parsing, which is a bigger scope than this PR. I'll capture it as a follow-up enhancement (parsing MCP/RBAC errors from LCS and rendering actionable messages in the Lightspeed chat). For now, the user will see whatever LCS surfaces in the streamed response. Addressed your second concern |
5f5b935 to
221f0f2
Compare
sounds good @maysunfaisal thanks |
|
@gabemontero Here is the issue to address surfacing user friendly MCP permission denial msgs https://redhat.atlassian.net/browse/RHIDP-14907 |
Screenshots - LocalRBAC Example - catalog.example.read = denyrbac policy file
Lightspeed (prior to rebrand) interacting with Backstage/RHDH MCP with DCR - does not return Catalog resources
MCP Settings for DCR auth server does not let you edit token from the UI
Cursor MCP Settings - backstage-actions pointing to Backstage/RHDH MCP with DCR
Redirected to Backstage/RHDH for DCR auth and successful auth
Cursor MCP backstage-actions now shows a successful connection
Query the Backstage/RHDH Catalog, denied due to rbac policy
RBAC Example - catalog.example.read = allowrbac policy file
Lightspeed query now passes for the same prompt using MCP with DCR
Cursor query now passes for the same prompt using MCP with DCR
|
4bd3a06 to
099f3fb
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
- Wrap getPluginRequestToken in per-server try/catch so one failing DCR server does not break all MCP integration - Return 502 with clear error on token mint failure in /validate endpoint - Bundle authService+credentials into dcrAuth object to prevent partial provision at compile time - Export McpServerAuth type and use it instead of raw string - Remove redundant per-request warning for dual auth+token config Co-authored-by: Cursor <cursoragent@cursor.com>
- Remove duplicate migrate(database) call in plugin.ts - Remove unused 'mcp.settings.status.autoManaged' translation key - Regenerate API report Co-authored-by: Cursor <cursoragent@cursor.com>
…me in tests Co-authored-by: Cursor <cursoragent@cursor.com>
099f3fb to
4d83df4
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 11:29 PM UTC · Completed 11:42 PM UTC |
ReviewFindingsMedium
Low
Labels: PR adds a new DCR authentication feature for MCP servers with security-relevant auth changes and test additions. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
requires-manual-review
- Tighten config.d.ts auth type from string to literal 'dcr' - Fix @visibility annotation to 'frontend' (field is used by UI) - Add case-insensitive comparison for auth config value - Add test for DCR validate endpoint path Co-authored-by: Cursor <cursoragent@cursor.com>
This is an internal plugin — coreServices.auth is always injected by the Backstage plugin framework. Making it optional would add unnecessary null checks for a code path that can't occur in practice.
Partially addressed. Added test for the DCR validate endpoint path. The 502 error case (auth service throws) requires overriding coreServices.auth mock internals which is non-trivial with the current test infrastructure.
Intentional design — graceful degradation. The warning log is sufficient; crashing the entire request when one server's auth is unavailable would be worse UX.
Addressed. Changed type from string to 'dcr'.
Addressed. Changed @visibility backend to @visibility frontend.
By design — all DCR-enabled MCP servers in RHDH are proxied through @backstage/plugin-mcp-actions-backend, so the target is always mcp-actions.
Addressed. Added .toLowerCase() so DCR, Dcr, etc. all work.
|
|
@its-mitesh-kumar Addressed the fullsend reviews |
|
New changes are detected. LGTM label has been removed. |
|
| isSendButtonDisabled={isSendButtonDisabled} | ||
| hasAttachButton | ||
| attachButtonPosition="start" | ||
| handleAttach={handleAttach} |
There was a problem hiding this comment.
@maysunfaisal Why did we removed it ?
@HusneShabbir We did missed while testing, due to this change attach button start appearing in the right side of message bar, while it should be on the left of the model selctor.
Lets be careful.
I will be fixing this as part of my new PR
…hat-developer#3347) * feat(lightspeed): add DCR authentication support for MCP servers Co-authored-by: Cursor <cursoragent@cursor.com> * feat(lightspeed): harden DCR token minting and improve type safety - Wrap getPluginRequestToken in per-server try/catch so one failing DCR server does not break all MCP integration - Return 502 with clear error on token mint failure in /validate endpoint - Bundle authService+credentials into dcrAuth object to prevent partial provision at compile time - Export McpServerAuth type and use it instead of raw string - Remove redundant per-request warning for dual auth+token config Co-authored-by: Cursor <cursoragent@cursor.com> * fix(lightspeed): address review comments from yangcao77 - Remove duplicate migrate(database) call in plugin.ts - Remove unused 'mcp.settings.status.autoManaged' translation key - Regenerate API report Co-authored-by: Cursor <cursoragent@cursor.com> * fix(lightspeed): add DCR localization strings and fix config key rename in tests Co-authored-by: Cursor <cursoragent@cursor.com> * test(lightspeed-e2e): add MCP DCR settings coverage Add DCR-specific MCP settings e2e scenarios and extract shared DCR test support helpers so behavior is validated consistently across display modes and mixed server types. Co-authored-by: Cursor <cursoragent@cursor.com> * test(lightspeed-e2e): tighten localized cancel button selector Use exact text matching for modal cancel button lookups in DCR MCP tests to avoid strict-mode collisions in locales where cancel label prefixes the clear-token button aria-label. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: HusneShabbir <husneshabbir447@gmail.com>
5 participants
Hey, I just made a Pull Request!
✔️ Checklist
Issue
https://redhat.atlassian.net/browse/RHIDP-11657
Summary
Adds Dynamic Client Registration (DCR) authentication for MCP servers in Lightspeed, allowing the backend to mint per-user plugin request tokens instead of requiring static tokens.
What changed
Backend (
lightspeed-backend)auth: dcrconfig option for MCP servers — when set, the backend mints a Backstage service token on behalf of the user viagetPluginRequestTokenAuthServiceinjection into the routerGET /mcp-serversandPATCH /mcp-servers/:nameresponses now include theauthfieldhasToken: truealways (tokens are minted per-request, no manual entry needed)buildMcpHeaderssends the minted token inMCP-HEADERSfor DCR serversFrontend (
lightspeed)Bug fixes along the way
@patternfly/react-coreversions (added resolution to force6.4.3)attachButtonPositionprop that no longer exists in@patternfly/chatbot@backstage/plugin-permission-backendregistered separately (breaking change from v5/v6)Other
@backstage/plugin-authto frontend for DCR OAuth2 consent pagedist-dynamicto.prettierignorerbac-policy.csvwith Lightspeed permissions for local RBAC testingHow to test
cd workspaces/lightspeed && yarn test:all(769 tests pass)@backstage/plugin-mcp-actions-backendadded toindex.tsexperimentalDynamicClientRegistration.enabled: trueinapp-config.yamlmcpServers: [{ name: "...", auth: dcr }]in lightspeed confighttp://localhost:7007/api/mcp-actions/v1— DCR OAuth consent should pop up in browserauth: dcrinvalues.yamlunderlightspeed.mcpServersConfig example