From 23e974634d17afb0df3dbaf4625d10c1adffe1f3 Mon Sep 17 00:00:00 2001 From: navidgh67 Date: Thu, 15 Oct 2026 22:43:07 -0400 Subject: [PATCH] Stop anyone from acting as an admin through the MCP token or sign-in 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 --- api/role_requests.py | 8 ++++ api/routers/admin.py | 7 ++- api/tests/test_platform_notifications.py | 23 +++++++++- api/tests/test_user_identity.py | 56 +++++++++++++++++++++++- api/user_identity.py | 23 ++++++++++ web/auth.ts | 5 +++ web/components/role-request-card.tsx | 3 +- web/lib/mcp/bearer-identity.test.mjs | 26 +++++++++++ web/lib/mcp/bearer-identity.ts | 38 ++++++++++++++++ web/lib/mcp/dt-call.ts | 26 +++++------ web/lib/saml/profile.ts | 3 ++ 11 files changed, 200 insertions(+), 18 deletions(-) create mode 100644 web/lib/mcp/bearer-identity.test.mjs create mode 100644 web/lib/mcp/bearer-identity.ts diff --git a/api/role_requests.py b/api/role_requests.py index 6e95369..862dd6b 100644 --- a/api/role_requests.py +++ b/api/role_requests.py @@ -20,6 +20,9 @@ from security import PlatformUser PLATFORM_ROLES = ("researcher", "pi", "equipment_owner", "admin") +# The administrator role is never requested: an admin assigns it directly on +# the Users page, so a request (or approving one) can't grant it. +REQUESTABLE_ROLES = ("researcher", "pi", "equipment_owner") ROLE_LABELS = { "researcher": "Researcher", "pi": "Principal Investigator", @@ -207,6 +210,8 @@ def create_role_request_pg( justification = (justification or "").strip() if requested_role not in PLATFORM_ROLES: raise RoleChangeError(400, "Invalid platform role") + if requested_role not in REQUESTABLE_ROLES: + raise RoleChangeError(400, "The administrator role can't be requested") if not justification: raise RoleChangeError(400, "Please explain why you need this role") if len(justification) > MAX_JUSTIFICATION_LENGTH: @@ -400,6 +405,9 @@ def decide_role_request_pg( raise RoleChangeError(409, f"This request is already {request['status']}") new_status = "approved" if decision == "approve" else "rejected" + if decision == "approve" and request["requested_role"] not in REQUESTABLE_ROLES: + # Requests made before admin stopped being requestable. + raise RoleChangeError(400, "The administrator role can't be granted through a request") if decision == "approve": apply_user_role_change( cur, diff --git a/api/routers/admin.py b/api/routers/admin.py index b02395f..0bbbeef 100644 --- a/api/routers/admin.py +++ b/api/routers/admin.py @@ -32,7 +32,7 @@ list_my_role_requests_pg, list_role_requests_pg, ) -from user_identity import sync_canonical_user +from user_identity import IdentityConflictError, sync_canonical_user logger = logging.getLogger(__name__) router = APIRouter(prefix="/admin", tags=["admin"]) @@ -45,6 +45,7 @@ def get_or_create_user_role( name: str | None = None, organization: str | None = None, user_id: str | None = None, + id_source: str | None = None, x_user_id: str | None = Header(None, alias="X-User-Id"), _: None = Depends(require_system_token), ): @@ -69,6 +70,7 @@ def get_or_create_user_role( name=name, organization=organization, requested_status="active", + id_from_email=id_source == "email", ) notified = False if user.get("first_sign_in"): @@ -104,6 +106,9 @@ def get_or_create_user_role( if user.get("is_new"): response["is_new"] = True return response + except IdentityConflictError as e: + logger.error(f"Refused sign-in for {email}: {e}") + raise HTTPException(status_code=409, detail="This sign-in conflicts with an existing account") except Exception as e: logger.error(f"Error syncing user role for {email}: {e}") raise HTTPException(status_code=500, detail="Database lookup failed") diff --git a/api/tests/test_platform_notifications.py b/api/tests/test_platform_notifications.py index 5f0fec6..c72bb18 100644 --- a/api/tests/test_platform_notifications.py +++ b/api/tests/test_platform_notifications.py @@ -4,7 +4,7 @@ import sys import unittest from pathlib import Path -from unittest.mock import patch +from unittest.mock import MagicMock, patch API_ROOT = Path(__file__).resolve().parents[1] if str(API_ROOT) not in sys.path: @@ -128,6 +128,7 @@ def test_invalid_role_and_missing_or_long_justification_are_rejected_before_data with patch.object(role_requests, "get_pg_connection") as connect: for kwargs, message in ( ({"requested_role": "superuser", "justification": "x"}, "Invalid platform role"), + ({"requested_role": "admin", "justification": "x"}, "can't be requested"), ({"requested_role": "pi", "justification": " "}, "explain"), ({"requested_role": "pi", "justification": "x" * 1001}, "at most"), ): @@ -137,6 +138,26 @@ def test_invalid_role_and_missing_or_long_justification_are_rejected_before_data self.assertEqual(caught.exception.status_code, 400) connect.assert_not_called() + def test_pending_admin_request_cannot_be_approved(self): + cursor = MagicMock() + cursor.__enter__.return_value = cursor + cursor.fetchone.return_value = { + "id": "r1", + "user_id": "someone", + "requested_role": "admin", + "status": "pending", + } + conn = MagicMock() + conn.cursor.return_value = cursor + with patch.object(role_requests, "get_pg_connection", return_value=conn), patch.object( + role_requests, "apply_user_role_change" + ) as change: + with self.assertRaises(role_requests.RoleChangeError) as caught: + role_requests.decide_role_request_pg(admin_user=self.USER, request_id="r1", decision="approve") + self.assertIn("can't be granted", caught.exception.detail) + change.assert_not_called() + conn.commit.assert_not_called() + def test_decision_must_be_approve_or_reject(self): with patch.object(role_requests, "get_pg_connection") as connect: with self.assertRaises(role_requests.RoleChangeError): diff --git a/api/tests/test_user_identity.py b/api/tests/test_user_identity.py index 4384cb0..69233ba 100644 --- a/api/tests/test_user_identity.py +++ b/api/tests/test_user_identity.py @@ -7,7 +7,12 @@ if str(API_DIR) not in sys.path: sys.path.insert(0, str(API_DIR)) -from user_identity import canonical_user_id, highest_role, sync_canonical_user +from user_identity import ( + IdentityConflictError, + canonical_user_id, + highest_role, + sync_canonical_user, +) class FakeCursor: @@ -118,6 +123,55 @@ def test_duplicate_identity_references_move_to_canonical_user(self): ) ) + def _admin_cursor(self): + return FakeCursor( + users=[ + { + "id": "ngholiza", + "name": "Navid", + "email": "ngholiza@purdue.edu", + "role": "admin", + "organization": "Purdue University", + "status": "active", + } + ], + available_columns=set(), + ) + + def test_email_derived_id_cannot_take_over_another_account(self): + cursor = self._admin_cursor() + with self.assertRaises(IdentityConflictError): + sync_canonical_user( + cursor, + email="ngholiza@other.example", + requested_user_id="ngholiza", + name="Someone else", + organization="Elsewhere", + id_from_email=True, + ) + self.assertFalse(any("UPDATE users" in query for query, _ in cursor.executions)) + + def test_email_derived_id_with_the_same_email_signs_in(self): + result = sync_canonical_user( + self._admin_cursor(), + email="NGHOLIZA@purdue.edu", + requested_user_id="ngholiza", + name="Navid", + organization="Purdue University", + id_from_email=True, + ) + self.assertEqual(result["role"], "admin") + + def test_asserted_username_keeps_its_account_after_an_email_change(self): + result = sync_canonical_user( + self._admin_cursor(), + email="navid@new.example", + requested_user_id="ngholiza", + name="Navid", + organization="Purdue University", + ) + self.assertEqual(result["id"], "ngholiza") + if __name__ == "__main__": unittest.main() diff --git a/api/user_identity.py b/api/user_identity.py index 5c2524c..f87cdb3 100644 --- a/api/user_identity.py +++ b/api/user_identity.py @@ -36,6 +36,10 @@ ) +class IdentityConflictError(Exception): + """A sign-in whose id was derived from its email matches another person's account.""" + + def canonical_user_id(email: str, requested_user_id: str | None = None) -> str: requested = (requested_user_id or "").strip() if requested: @@ -184,7 +188,18 @@ def sync_canonical_user( organization: str | None, requested_role: str = "researcher", requested_status: str = "active", + id_from_email: bool = False, ) -> dict[str, Any]: + """ + Create or update the user for a sign-in or invitation, merging duplicate + rows that share the email or id. + + id_from_email: the id was derived from the email's local part because the + identity provider sent no username. Such an id is not proof of identity + (ngholiza@other.example also yields "ngholiza"), so if it matches an + account with a different email, the sign-in is refused instead of merged, + which would hand over that account and its role. + """ normalized_email = email.strip().lower() resolved_user_id = canonical_user_id(normalized_email, requested_user_id) @@ -211,6 +226,14 @@ def sync_canonical_user( (normalized_email, resolved_user_id), ) rows = [dict(row) for row in cur.fetchall()] + if id_from_email and any( + row["id"] == resolved_user_id + and (row.get("email") or "").strip().lower() not in ("", normalized_email) + for row in rows + ): + raise IdentityConflictError( + f"User id {resolved_user_id!r} belongs to an account with a different email" + ) is_new_user = not rows canonical = next( (row for row in rows if row["id"] == resolved_user_id), diff --git a/web/auth.ts b/web/auth.ts index 9599d25..78f1c49 100644 --- a/web/auth.ts +++ b/web/auth.ts @@ -24,6 +24,8 @@ export interface PlatformIdentity { samlNameId?: string; samlNameIdFormat?: string; samlSessionIndex?: string; + /** "email" when the id was derived from the email (no username from the IdP). */ + idSource?: "asserted" | "email"; } interface RoleSyncResponse { @@ -61,6 +63,7 @@ function buildRoleSyncUrl(params: { name?: string; organization?: string; userId?: string; + idSource?: "asserted" | "email"; }): string { const baseUrl = getDtApiBaseUrl(); const url = new URL( @@ -73,6 +76,7 @@ function buildRoleSyncUrl(params: { url.searchParams.set("organization", params.organization); } if (params.userId) url.searchParams.set("user_id", params.userId); + if (params.idSource) url.searchParams.set("id_source", params.idSource); return url.toString(); } @@ -86,6 +90,7 @@ export async function syncPlatformIdentity( name: identity.name, organization: identity.organization, userId: identity.id, + idSource: identity.idSource, }), { headers: { "X-System-Token": requireServerEnv("DT_SYSTEM_TOKEN") }, diff --git a/web/components/role-request-card.tsx b/web/components/role-request-card.tsx index 250c062..498ecd3 100644 --- a/web/components/role-request-card.tsx +++ b/web/components/role-request-card.tsx @@ -11,7 +11,8 @@ import { } from "@/lib/api-client"; import { ArrowRightLeft, Clock, CheckCircle2, XCircle, Ban, Loader2 } from "lucide-react"; -const REQUESTABLE_ROLES: UserRole[] = ["pi", "equipment_owner", "researcher", "admin"]; +// Admin is assigned only by an administrator on the Users page, never requested. +const REQUESTABLE_ROLES: UserRole[] = ["pi", "equipment_owner", "researcher"]; const MAX_JUSTIFICATION = 1000; const STATUS_STYLES: Record = { diff --git a/web/lib/mcp/bearer-identity.test.mjs b/web/lib/mcp/bearer-identity.test.mjs new file mode 100644 index 0000000..600068c --- /dev/null +++ b/web/lib/mcp/bearer-identity.test.mjs @@ -0,0 +1,26 @@ +// Run from web/: node --test lib/mcp/bearer-identity.test.mjs +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { bearerIdentityFromEnv } from "./bearer-identity.ts"; + +const BASE = { MCP_TEST_USER_ID: "tester", MCP_TEST_USER_EMAIL: "tester@purdue.edu" }; + +test("the bearer token never acts as an admin", () => { + for (const role of ["admin", "Administrator", " ADMIN "]) { + assert.ok("refused" in bearerIdentityFromEnv({ ...BASE, MCP_TEST_USER_ROLE: role })); + } +}); + +test("the bearer token needs an explicit identity", () => { + assert.ok("refused" in bearerIdentityFromEnv({})); + assert.ok("refused" in bearerIdentityFromEnv({ MCP_TEST_USER_ID: "tester" })); + assert.ok("refused" in bearerIdentityFromEnv({ MCP_TEST_USER_EMAIL: "tester@purdue.edu" })); +}); + +test("the default role is researcher", () => { + const identity = bearerIdentityFromEnv(BASE); + assert.equal(identity.role, "researcher"); + assert.equal(identity.id, "tester"); + assert.equal(bearerIdentityFromEnv({ ...BASE, MCP_TEST_USER_ROLE: "pi" }).role, "pi"); +}); diff --git a/web/lib/mcp/bearer-identity.ts b/web/lib/mcp/bearer-identity.ts new file mode 100644 index 0000000..920956c --- /dev/null +++ b/web/lib/mcp/bearer-identity.ts @@ -0,0 +1,38 @@ +/** + * bearer-identity.ts — who the shared MCP bearer token acts as (pure, no + * imports, so it can be unit-tested). Whoever holds the token acts as this + * user, so it must be set explicitly (no default account) and can never be an + * administrator. + */ + +export interface BearerIdentity { + id: string; + email: string; + name: string; + role: string; + organization: string; +} + +function normalizePlatformRole(role: string): string { + const normalized = role.trim().toLowerCase(); + if (normalized === "administrator") return "admin"; + return normalized || "researcher"; +} + +/** The configured identity, or the reason the token may not be used. */ +export function bearerIdentityFromEnv( + env: Record, +): BearerIdentity | { refused: string } { + const id = env.MCP_TEST_USER_ID?.trim() ?? ""; + const email = env.MCP_TEST_USER_EMAIL?.trim() ?? ""; + const role = normalizePlatformRole(env.MCP_TEST_USER_ROLE ?? "researcher"); + if (!id || !email) return { refused: "MCP_TEST_USER_ID and MCP_TEST_USER_EMAIL must be set" }; + if (role === "admin") return { refused: "the MCP bearer token may not act as an admin" }; + return { + id, + email, + name: env.MCP_TEST_USER_NAME?.trim() || id, + role, + organization: env.MCP_TEST_USER_ORG?.trim() || "Birck Nanotechnology Center", + }; +} diff --git a/web/lib/mcp/dt-call.ts b/web/lib/mcp/dt-call.ts index 1eb83a3..96982d1 100644 --- a/web/lib/mcp/dt-call.ts +++ b/web/lib/mcp/dt-call.ts @@ -1,6 +1,7 @@ import "server-only"; import { auth } from "@/auth"; +import { bearerIdentityFromEnv } from "@/lib/mcp/bearer-identity"; import { requireServerEnv } from "@/lib/server-env"; import { timingSafeEqual } from "crypto"; @@ -75,26 +76,23 @@ function constantTimeEquals(left: string, right: string): boolean { return timingSafeEqual(leftBuffer, rightBuffer); } -function normalizePlatformRole(role: string): string { - const normalized = role.trim().toLowerCase(); - if (normalized === "administrator") return "admin"; - return normalized || "researcher"; -} - +/** + * The identity behind the shared MCP bearer token (external MCP / OpenAPI + * clients); see bearer-identity.ts. An admin identity, or a missing id/email, + * makes the token unusable. + */ export function resolveMcpBearerSession(authorization: string | null): PlatformSession | null { const expectedToken = process.env.MCP_BEARER_TOKEN?.trim() ?? ""; const receivedToken = bearerTokenFromHeader(authorization); if (!expectedToken || !receivedToken) return null; if (!constantTimeEquals(receivedToken, expectedToken)) return null; - return { - id: process.env.MCP_TEST_USER_ID?.trim() || "ngholiza", - email: process.env.MCP_TEST_USER_EMAIL?.trim() || "ngholiza@purdue.edu", - name: process.env.MCP_TEST_USER_NAME?.trim() || "N. Gholiza", - role: normalizePlatformRole(process.env.MCP_TEST_USER_ROLE ?? "pi"), - organization: - process.env.MCP_TEST_USER_ORG?.trim() || "Birck Nanotechnology Center", - }; + const identity = bearerIdentityFromEnv(process.env); + if ("refused" in identity) { + console.error(`[MCP_AUTH] token refused: ${identity.refused}`); + return null; + } + return identity; } export interface DtCallResult { diff --git a/web/lib/saml/profile.ts b/web/lib/saml/profile.ts index 0445b78..57a1762 100644 --- a/web/lib/saml/profile.ts +++ b/web/lib/saml/profile.ts @@ -165,6 +165,9 @@ export function mapSamlProfile(profile: Profile): Omit return { id: username, + // An id derived from the email is not proof of identity; the role sync + // refuses it when it matches an account with a different email. + idSource: explicitUsername || nonEmailNameId ? "asserted" : "email", nanohubUsername: username, email, name: displayName || username,