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,