Skip to content

Conversation

@ngholiza
Copy link
Collaborator

Why

In production, dt-web-secret had ENABLE_MCP_HTTP_ENDPOINT=true and MCP_TEST_USER_ROLE=admin. /api/mcp and /api/tools/* are reachable from the internet and accept one shared bearer token, so anyone holding that token acted as ngholiza with admin rights. The production setting is being fixed separately: the endpoint is turned off and the token deleted. This PR makes the code refuse that setup, and closes two other ways to end up as an admin.

Changes

  • MCP bearer token (web/lib/mcp/bearer-identity.ts, dt-call.ts):
    • The token now acts only as an identity set explicitly with MCP_TEST_USER_ID and MCP_TEST_USER_EMAIL. There is no default account anymore.
    • The default role is researcher.
    • If the role is admin (or Administrator), the token is refused, and the refusal is logged.
  • Role requests (api/role_requests.py, role-request-card.tsx):
    • Users can no longer request the admin role, and a pending admin request can't be approved.
    • The request form no longer offers admin.
    • Admins can still assign roles directly on the Users page (require_admin).
  • SAML identity collision (web/lib/saml/profile.ts, web/auth.ts, api/user_identity.py, api/routers/admin.py):
    • The problem: when the identity provider sends no username, the id is taken from the part of the email before the @. So ngholiza@other.example became ngholiza, was merged into that account, and kept its admin role.
    • The fix: the sign-in now records whether the id came from the identity provider (id_source=asserted) or was derived from the email (id_source=email). An email-derived id that matches an account with a different email is refused with a 409, before any update or merge.
    • Usernames sent by the identity provider work as before, so a legitimate email change still signs in.

Tests

  • API: 19 tests pass, in test_user_identity.py and test_platform_notifications.py. New tests cover:
    • an email-derived id being refused when it matches another account;
    • the same email still signing in;
    • an IdP-sent username still signing in after an email change;
    • an admin request being refused at creation and at approval.
  • Web: the new node --test lib/mcp/bearer-identity.test.mjs passes (3 tests: admin refused, explicit identity required, default researcher). The 18 assistant tests, tsc and eslint pass.
  • Codex review: APPROVE, after it raised the SAML collision.

Note

DT_SYSTEM_TOKEN can still claim any identity. It's the internal credential between dt-web and dt-api and should only be in those two secrets.

🤖 Generated with Claude Code

Nobody should be able to act as a platform admin except through an
admin's own sign-in.

- External MCP endpoints (/api/mcp, /api/tools): the shared bearer
  token used to default to ngholiza, and production had it set to admin,
  so anyone holding the token was an admin. The token now needs an
  explicit MCP_TEST_USER_ID and MCP_TEST_USER_EMAIL, defaults to
  researcher, and is refused if the identity is admin. The check lives
  in lib/mcp/bearer-identity.ts, which has tests.
- Role requests: users can no longer request the admin role, and a
  pending admin request can't be approved. Admins still assign admin
  directly on the Users page. The request form no longer offers admin.
- SAML sign-in: when the identity provider sends no username, the id
  comes from the email's local part, so ngholiza@other.example became
  "ngholiza" and was merged into that account, keeping its role. The
  sign-in now records whether the id came from the identity provider or
  from the email. An email-derived id that matches an account with a
  different email is refused with a 409 instead of being merged.
  Usernames sent by the identity provider work as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign in to join this conversation on GitHub.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant