Skip to content

Conversation

andreaskienle
Copy link
Contributor

@andreaskienle andreaskienle commented Jun 5, 2025

Implements openmcp-project/backlog#151

Some notes:

  • Currently, we assume that the Onboarding API and MCPs can have different client IDs, but all MCPs within a single landscape must share the same client ID. This approach will need to be revised once we support multiple IdPs. I added a comment in the corresponding issue.
  • To test this PR, you need to add the env variable OIDC_CLIENT_ID_MCP (see env template)

Needs to be merged together with openmcp-project/ui-backend#9

@andreaskienle andreaskienle requested a review from Copilot June 13, 2025 12:12
Copilot

This comment was marked as outdated.

@andreaskienle andreaskienle marked this pull request as ready for review June 26, 2025 13:57
Copilot

This comment was marked as outdated.

@andreaskienle andreaskienle requested a review from Copilot June 26, 2025 14:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enables separate authentication tokens and flows for the Onboarding API and MCPs by introducing distinct client IDs, session keys, and context providers for each.

  • Front-end: Replaced the single useAuth context with useAuthOnboarding and added useAuthMcp for MCP-specific auth, updated routing and session handling.
  • Back-end: Split auth routes into /auth/onboarding/* and /auth/mcp/*, replaced the old secure-session plugin with an encrypted-session implementation, and extended environment validation.
  • Infrastructure: Introduced AuthCallbackHandler to centralize redirect handling, updated the HTTP proxy to select tokens based on request headers, and extended .env.template with OIDC_CLIENT_ID_MCP and SESSION_SECRET.

Reviewed Changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/spaces/onboarding/auth/AuthContextOnboarding.tsx Renamed onboarding auth context, updated endpoints and redirects.
src/spaces/mcp/auth/AuthContextMcp.tsx Added MCP-specific auth context and hooks.
src/common/auth/AuthCallbackHandler.tsx New component to handle post-login redirects for both flows.
src/lib/shared/McpContext.tsx Wrapped control-plane view in MCP auth to gate downstream login.
src/lib/api/fetch.ts Changed unauthorized redirect to the onboarding login route.
server/routes/auth-onboarding.js Updated onboarding login, callback, me, and logout endpoints.
server/routes/auth-mcp.js Introduced MCP login, callback, and me endpoints (no logout).
server/encrypted-session.js Replaced secure-session with per-user encrypted session plugin.
server/config/env.js Added OIDC_CLIENT_ID_MCP and SESSION_SECRET to env schema.
Comments suppressed due to low confidence (5)

server/plugins/http-proxy.js:89

  • Combining two tokens into a comma-separated string for the Authorization header is likely invalid; instead select or merge tokens according to the proxy target and format the header as Bearer <token>.
        const accessToken = useCrate ? req.encryptedSession.get("onboarding_accessToken") : `${req.encryptedSession.get("onboarding_accessToken")},${req.encryptedSession.get("mcp_accessToken")}`;

src/lib/api/fetch.ts:52

  • The redirect to the onboarding login endpoint no longer includes the original redirectTo or return URL; consider appending ?redirectTo= with the current location hash or pathname to preserve post-login navigation.
      window.location.replace('/api/auth/onboarding/login');

server/routes/auth-onboarding.js:63

  • The onboarding logout endpoint is registered at /auth/logout, which conflicts with the shared path and may collide with MCP routes; consider namespacing it under /auth/onboarding/logout for symmetry and clarity.
  fastify.post("/auth/logout", async (req, reply) => {

server/routes/auth-mcp.js:1

  • There is no /auth/mcp/logout endpoint to allow MCP users to clear their session; adding a corresponding logout route would improve consistency between flows.
import fp from "fastify-plugin";

server/plugins/http-proxy.js:87

  • The proxy rewrite logic trusts the x-use-crate header and directly reads session tokens; ensure that x-use-crate cannot be spoofed by clients and consider checking an internal flag or context instead of a client-controlled header.
      rewriteRequestHeaders: (req, headers) => {

Copy link
Contributor

@enrico-kaack-comp enrico-kaack-comp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for now. Still many things to improve but I think this is a valid first version to support two idp flows.

ValentinGerlach
ValentinGerlach previously approved these changes Jul 2, 2025
Copy link
Member

@ValentinGerlach ValentinGerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer: I did not check the frontend part.

Comment on lines +25 to +26
COOKIE_SECRET: { type: 'string' },
SESSION_SECRET: { type: 'string' },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential improvement in the future: Add validation to ensure these values cannot be empty (e.g. minLength = 16).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I moved it into the follow-up task: openmcp-project/backlog#162

Co-authored-by: Valentin Gerlach <valentin.gerlach@sap.com>
@andreaskienle andreaskienle merged commit 845dab5 into main Jul 2, 2025
5 checks passed
@andreaskienle andreaskienle deleted the mcp-auth branch July 2, 2025 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants