Conversation
Review Summary by QodoAdd MCP server tools and OAuth2 consent flow
WalkthroughsDescription• Implement MCP server tools for project status checking • Add OAuth2 consent page for authorization flow • Register x2a:project:list MCP action for listing projects • Extract listProjects logic for reuse across HTTP and MCP • Configure MCP actions plugin and dynamic client registration Diagramflowchart LR
MCP["MCP Inspector"]
Backend["x2a Backend Plugin"]
Actions["MCP Actions Registry"]
ProjectList["x2a:project:list Action"]
ListProjects["listProjects Function"]
OAuth2["OAuth2 Consent Page"]
MCP -->|"calls"| Actions
Actions -->|"registers"| ProjectList
ProjectList -->|"executes"| ListProjects
ListProjects -->|"checks permissions"| Backend
Backend -->|"requires auth"| OAuth2
File Changes1. workspaces/x2a/plugins/x2a-backend/src/mcp/projects.ts
|
Code Review by Qodo
1. Placeholder AAP token active
|
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
| config: coreServices.rootConfig, | ||
| x2aDatabase: x2aDatabaseServiceRef, | ||
| kubeService: kubeServiceRef, | ||
| actionsRegistry: actionsRegistryServiceRef, | ||
| }, |
There was a problem hiding this comment.
2. Unprovided actionsregistry service 🐞 Bug ⛯ Reliability
x2APlugin now requires actionsRegistryServiceRef and always registers MCP actions at startup. Existing x2a-backend integration tests/backends shown in-repo do not provide an ActionsRegistryService instance, which can prevent the backend from starting in those environments.
Agent Prompt
### Issue description
`x2APlugin` now requires `actionsRegistryServiceRef`, but existing integration tests/backends do not inject a provider for that service. This can cause backend startup failures in test environments (and in any consuming backend that doesn’t include an MCP actions registry implementation).
### Issue Context
The plugin declares `actionsRegistry: actionsRegistryServiceRef` as a required init dependency and immediately calls `registerProjectActions({ actionsRegistry, ... })`.
### Fix Focus Areas
- workspaces/x2a/plugins/x2a-backend/src/plugin.ts[37-49]
- workspaces/x2a/plugins/x2a-backend/src/plugin.test.ts[145-171]
- workspaces/x2a/plugins/x2a-backend/src/plugin.test.ts[233-251]
### Suggested fix options
1) **Update tests to provide a registry service**
- Add a `createServiceFactory` for `actionsRegistryServiceRef` returning a registry mock/implementation.
- You can base the instance on `actionsRegistryServiceMock()` from `@backstage/backend-test-utils/alpha`.
2) **Make MCP integration optional**
- Register the MCP actions only if the registry service is available (e.g., use an optional service ref if supported in your Backstage version), otherwise skip registration and log a debug/info message.
3) **Ensure runtime backends include the provider**
- If MCP is mandatory, ensure every backend composition (including test backends) includes the plugin/service that provides `actionsRegistryServiceRef`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This enables a way to check the current status of the project using MCP tools, and can be used inside any project that needs to have AI tooling on top of it. Because this uses OAuth2, I need to implement a quick page, because `plugin-auth` is not that easy to run within the current auth implementation. To test, I use MCP Inspector, like this: ``` DANGEROUSLY_OMIT_AUTH=true npx @modelcontextprotocol/inspector ``` The auth env var is important, because without it, it messes up the OAuth2 token that we set[0]. I can see room for improvements where: - The auth could maybe be moved to https://www.npmjs.com/package/@backstage/plugin-auth - The router is highly coupled; maybe we need to have an HTTP router and services that can be consumed by MCP and by the router. - The MCP actions could be expanded (follow-up PR): - Run module - Get project - Get module So the idea is just to get the basic terms of the MCP server in place, and finish all the implementation correctly by the end of the quarter. [0] modelcontextprotocol/inspector#633 Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
eloycoto
force-pushed
the
wt-auth-plugins
branch
from
1ff9522 to
2d6efc4
Compare
|
mareklibra
left a comment
mareklibra
left a comment
There was a problem hiding this comment.
How do we plan to distribute the ConsentPage? Anything under the packages is dev-only and not included in the production build. The RHDH can consume plugins only (after their conversion to the dynamic ones).
| }, | ||
| callbackUrl: { | ||
| fontFamily: 'monospace', | ||
| fontSize: '0.85rem', |
There was a problem hiding this comment.
nit: this should be taken from theme
| {state.status === 'completed' && ( | ||
| <Typography variant="h5" align="center" style={{ marginTop: 32 }}> | ||
| {state.action === 'approve' | ||
| ? `You have successfully authorized the application to access your ${appTitle} account. Redirecting...` |
There was a problem hiding this comment.
All strings must be internationalized
| <Route path="/settings" element={<UserSettingsPage />} /> | ||
| <Route path="/catalog-graph" element={<CatalogGraphPage />} /> | ||
| <Route path="/notifications" element={<NotificationsPage />} /> | ||
| <Route path="/oauth2/authorize/:sessionId" element={<ConsentPage />} /> |
There was a problem hiding this comment.
This will be difficult to distribute within RHDH. Easy with Backstage upstream deployments.
| action: async ({ input, credentials, logger }) => { | ||
| logger.info('Running x2a:project:list MCP action'); | ||
| const response = await listProjects( | ||
| input, |
There was a problem hiding this comment.
This is good. TypeScript can help here with maintenance in the future.
In addition, we will need unit tests covering
- functionality of the action
- guarding the contract between our router/services and the MCP tools.
This is simple to achieve at this scale but a headache when the tool list grows.
| 'List conversion projects with optional pagination and sorting.', | ||
| attributes: { readOnly: true }, | ||
| schema: { | ||
| input: z => |
There was a problem hiding this comment.
I can see a lot of similarity with the projectsGetRequestSchema from the router/project.ts.
Can we reuse it?
The tools and router will evolve together. At certain point we will need to introduce API versioning anyway.
| ): Promise<ProjectsGet['response']> { | ||
| const { permissionsSvc, catalog, x2aDatabase } = deps; | ||
|
|
||
| const [userResult, adminViewResult] = await Promise.all([ |
There was a problem hiding this comment.
This is reinventing the wheel, let's use the useEnforceX2APermissions which is well tested and consistent already. If not, we will eventually implement the same logic from scratch and end up in a similar solution.
| const credentials = await httpAuth.credentials(req, { allow: ['user'] }); | ||
|
|
||
| // parse request query | ||
| const projectsGetRequestSchema = z.object({ |
| }); | ||
|
|
||
| const { projects, totalCount } = await x2aDatabase.listProjects(query, { | ||
| const response = await listProjects( |
There was a problem hiding this comment.
Instead of introducing yet another layer (we have a lot of them already), the MCP tools can act as API clients.
This will simplify the flow, testing and maintenance.
The overhead of calls is very small.
We we will still benefit from all the decorators which Backstage provides for the calls.
There was a problem hiding this comment.
The main problem for this is the token, the token is different and you cannot use the token for the HTTP request.
| config: coreServices.rootConfig, | ||
| x2aDatabase: x2aDatabaseServiceRef, | ||
| kubeService: kubeServiceRef, | ||
| actionsRegistry: actionsRegistryServiceRef, |
There was a problem hiding this comment.
This QoDo finding makes sense. We should not rely on the RHDH having the MCP server configured. Let's make it optional.
|
The use of the I would be more for starting simple by reusing existing Backstage auth mechanisms: https://backstage.io/docs/auth/service-to-service-auth In case of the static tokens listed there, the Some of the issues I can see with the
We can evolve to the |
|
We will need unit tests covering:
|
Yes, supported.
The problem with a simple service-to-service auth, is that all RBAC system is broken in so many ways, like you will lost all fine-grained RBAC, and other users can see projects that they are not allowed to it. The MCP is just another way to consume the service, and we should take that primitive to that point. So what you're suggesting is like, from the HTML we render one thing, and the user in the API can see all the things, do you accept that? If not, why the MCP is different? MCP is just another presentation layer, and the RBAC and the authN/Z should use the same primitives rules. It's just doing it right now, and growth into the less headache in the future. The experimentalDynamicClientRegistration is a risk that we need to take, doing it right now, and evolve from it. And we can also help RHDH teams to discover issues, so it's a win for the company :) |
| auth: | ||
| experimentalDynamicClientRegistration: | ||
| # enable the feature | ||
| enabled: true |
There was a problem hiding this comment.
should this be auth enabled?
There was a problem hiding this comment.
yes, it is what allows the oauth flow in MCP
There was a problem hiding this comment.
oops sorry. I meant "Auto enabled". as in do we want this enabled by default
I'm not sure about this to be honest, I think that we should embrace the hexagonal architecture here, so we can have the layers of inputs, because calling the same API, it also add problems because the token is different, and it's just complicated in some ways. For me the architecture should be like this: HTTP Router - Schema/Validation - Service
Mainly, becase we can reuse the following and have something like: MCP Server action - Schema/Validation - Service The Schema/Validation- Service will be reused across. And we ackonoledge that the Auth on MCP is/might be different, so we have the correct abstraction layer. For me calling the API internally, it's just an overhead, something that it's not that great, and it'll add compromise the architecture in the future. |
3 participants
This enables a way to check the current status of the project using MCP tools, and can be used inside any project that needs to have AI tooling on top of it.
Because this uses OAuth2, I need to implement a quick page, because
plugin-authis not that easy to run within the current auth implementation.To test, I use MCP Inspector, like this:
The auth env var is important, because without it, it messes up the OAuth2 token that we set[0].
I can see room for improvements where:
So the idea is just to get the basic terms of the MCP server in place, and finish all the implementation correctly by the end of the quarter.
[0] modelcontextprotocol/inspector#633
Hey, I just made a Pull Request!
✔️ Checklist