From 86c8079fb1c31276fbddaf3387d313ef466ca0eb Mon Sep 17 00:00:00 2001 From: Louis King Date: Fri, 24 Jul 2026 22:41:32 +0100 Subject: [PATCH] fix(profiles): restore admin ability to edit other users' profiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The update_profile endpoint used RequireUserOwner which returns only the caller's user_id — no role information. The ownership check blocked ALL non-owner edits with 403, including admins. This regressed in d37b30a when the old Member model (RequireAdmin) was replaced with UserProfile. Backend: read X-User-Roles header directly in update_profile (same pattern as node_tags.py / routes.py) and bypass the ownership check when the admin role is present. Regular members editing their own profiles are unaffected — RequireUserOwner stays as the dependency. Frontend: extract ProfileEditForm component (with data-testids) from OwnProfileView. PublicProfileView now shows an inline edit form when an admin views another user's profile. Owner still gets the existing edit link; non-admins see nothing. Tests: - Backend: test_update_profile_admin_can_edit_other (admin edits other user's profile, asserts 200 + all fields updated) - Vitest: 4 new tests — admin button visibility, non-admin hidden, owner link vs admin button, form submission to correct endpoint - E2E: admin-profile-edit.spec.ts (admin edits Mem South's profile, verifies persistence; admin on own profile sees no admin button); members.spec.ts negative assertion (member sees no admin button) --- e2e/tests/admin-profile-edit.spec.ts | 51 +++ e2e/tests/members.spec.ts | 4 + src/meshcore_hub/api/routes/user_profiles.py | 19 +- .../js/spa-react/pages/Profile.test.tsx | 108 +++++- .../web/static/js/spa-react/pages/Profile.tsx | 319 +++++++++++------- tests/test_api/test_user_profiles.py | 25 ++ 6 files changed, 400 insertions(+), 126 deletions(-) create mode 100644 e2e/tests/admin-profile-edit.spec.ts diff --git a/e2e/tests/admin-profile-edit.spec.ts b/e2e/tests/admin-profile-edit.spec.ts new file mode 100644 index 0000000..e73f252 --- /dev/null +++ b/e2e/tests/admin-profile-edit.spec.ts @@ -0,0 +1,51 @@ +import { expect, test } from "@playwright/test"; +import { ADMIN_STATE } from "../utils/helpers"; + +test.use({ storageState: ADMIN_STATE }); + +test.describe.serial("admin profile edit", () => { + test("admin can edit another user's profile", async ({ page }) => { + await page.goto("/members"); + + // Navigate to Mem South's profile (not the admin's own). + await page + .getByTestId("member-card") + .filter({ hasText: "Mem South" }) + .first() + .click(); + await expect(page).toHaveURL(/\/profile\//); + + // Admin sees the edit button (owner does NOT — sub mismatch). + await expect(page.getByTestId("profile-admin-edit")).toBeVisible(); + + // Click edit — form appears pre-filled with the target profile's values. + await page.getByTestId("profile-admin-edit").click(); + await expect(page.getByTestId("profile-form")).toBeVisible(); + await expect(page.getByTestId("profile-name")).toHaveValue("Mem South"); + + // Edit callsign and save. + await page.getByTestId("profile-callsign").fill("ADMEDIT"); + await page.getByTestId("profile-save").click(); + + // Read-only view returns with updated callsign badge. + await expect(page.getByTestId("profile-admin-edit")).toBeVisible(); + await expect(page.getByText("ADMEDIT")).toBeVisible(); + }); + + test("admin does not see admin edit button on own profile", async ({ + page, + }) => { + // Navigate to own profile via members page. + await page.goto("/members"); + await page + .getByTestId("member-card") + .filter({ hasText: "PW Admin" }) + .first() + .click(); + await expect(page).toHaveURL(/\/profile\//); + + // Admin IS the owner here — should see the owner edit link, not the + // admin edit button. + await expect(page.getByTestId("profile-admin-edit")).toHaveCount(0); + }); +}); diff --git a/e2e/tests/members.spec.ts b/e2e/tests/members.spec.ts index 304d09b..13fb211 100644 --- a/e2e/tests/members.spec.ts +++ b/e2e/tests/members.spec.ts @@ -32,6 +32,10 @@ test.describe("members", () => { await expect(page).toHaveURL(/\/profile\/[0-9a-f-]{36}/); await expect(page.getByText("Mem South").first()).toBeVisible(); await expect(page.locator('nav[aria-label="Breadcrumb"]')).toBeVisible(); + + // A member viewing another user's profile must NOT see the admin edit + // button. + await expect(page.getByTestId("profile-admin-edit")).toHaveCount(0); }); test("clicking a member node shows the node detail page", async ({ page }) => { diff --git a/src/meshcore_hub/api/routes/user_profiles.py b/src/meshcore_hub/api/routes/user_profiles.py index 7be0c31..baea953 100644 --- a/src/meshcore_hub/api/routes/user_profiles.py +++ b/src/meshcore_hub/api/routes/user_profiles.py @@ -7,7 +7,12 @@ from pydantic import AnyUrl from sqlalchemy import func, or_, select from sqlalchemy.orm import selectinload -from meshcore_hub.api.auth import RequireRead, RequireUserOwner, X_USER_ID_HEADER +from meshcore_hub.api.auth import ( + RequireRead, + RequireUserOwner, + X_USER_ID_HEADER, + X_USER_ROLES_HEADER, +) from meshcore_hub.api.cache import cached from meshcore_hub.api.cache_invalidation import ( invalidate_dashboard, @@ -223,10 +228,14 @@ def update_profile( ) if profile.user_id != caller_id: - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Access denied: cannot modify another user's profile", - ) + roles_header = request.headers.get(X_USER_ROLES_HEADER, "") + roles = [r.strip() for r in roles_header.split(",") if r.strip()] + admin_role = getattr(request.app.state, "oidc_role_admin", "admin") + if admin_role not in roles: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Access denied: cannot modify another user's profile", + ) if profile_update.name is not None: profile.name = profile_update.name.strip() diff --git a/src/meshcore_hub/web/static/js/spa-react/pages/Profile.test.tsx b/src/meshcore_hub/web/static/js/spa-react/pages/Profile.test.tsx index 87f2dec..8411e09 100644 --- a/src/meshcore_hub/web/static/js/spa-react/pages/Profile.test.tsx +++ b/src/meshcore_hub/web/static/js/spa-react/pages/Profile.test.tsx @@ -1,5 +1,5 @@ -import { screen, waitFor } from "@testing-library/react"; -import { describe, expect, it, vi } from "vitest"; +import { fireEvent, screen, waitFor } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { Profile } from "@/pages/Profile"; import { renderWithProviders } from "@/test/renderWithProviders"; @@ -52,6 +52,110 @@ describe("Profile (public view)", () => { }); }); +describe("Profile (admin edit)", () => { + afterEach(() => { + window.__APP_CONFIG__ = makeConfig(); + }); + + function setAdminConfig() { + const config = makeConfig({ + oidc_enabled: true, + user: { sub: "admin-user", name: "Admin" }, + roles: ["admin"], + role_names: { admin: "admin", operator: "operator", member: "member" }, + }); + window.__APP_CONFIG__ = config; + return config; + } + + function setMemberConfig() { + const config = makeConfig({ + oidc_enabled: true, + user: { sub: "other-user", name: "Member" }, + roles: ["member"], + role_names: { admin: "admin", operator: "operator", member: "member" }, + }); + window.__APP_CONFIG__ = config; + return config; + } + + it("admin sees edit button on another user's profile", async () => { + vi.spyOn(api, "apiGet").mockResolvedValue(PROFILE_DATA); + const config = setAdminConfig(); + renderWithProviders(, { + route: "/profile/p1", + routePath: "/profile/:id", + config, + }); + await waitFor(() => { + expect(screen.getByTestId("profile-admin-edit")).toBeInTheDocument(); + }); + }); + + it("non-admin does not see edit button on another user's profile", async () => { + vi.spyOn(api, "apiGet").mockResolvedValue(PROFILE_DATA); + const config = setMemberConfig(); + renderWithProviders(, { + route: "/profile/p1", + routePath: "/profile/:id", + config, + }); + await waitFor(() => { + expect(screen.getAllByText("Jane Operator").length).toBeGreaterThanOrEqual(1); + }); + expect(screen.queryByTestId("profile-admin-edit")).toBeNull(); + }); + + it("owner sees edit link, not admin edit button", async () => { + vi.spyOn(api, "apiGet").mockResolvedValue(PROFILE_DATA); + const config = makeConfig({ + oidc_enabled: true, + user: { sub: "user-123", name: "Jane" }, + roles: ["admin"], + role_names: { admin: "admin", operator: "operator", member: "member" }, + }); + window.__APP_CONFIG__ = config; + renderWithProviders(, { + route: "/profile/p1", + routePath: "/profile/:id", + config, + }); + await waitFor(() => { + expect(screen.getAllByText("Jane Operator").length).toBeGreaterThanOrEqual(1); + }); + expect(screen.queryByTestId("profile-admin-edit")).toBeNull(); + }); + + it("admin edit form submits to correct endpoint", async () => { + vi.spyOn(api, "apiGet").mockResolvedValue(PROFILE_DATA); + const apiPutSpy = vi.spyOn(api, "apiPut").mockResolvedValue(undefined); + const config = setAdminConfig(); + renderWithProviders(, { + route: "/profile/p1", + routePath: "/profile/:id", + config, + }); + await waitFor(() => { + expect(screen.getByTestId("profile-admin-edit")).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByTestId("profile-admin-edit")); + + const nameInput = await screen.findByTestId("profile-name"); + fireEvent.change(nameInput, { target: { value: "Admin Set Name" } }); + fireEvent.click(screen.getByTestId("profile-save")); + + await waitFor(() => { + expect(apiPutSpy).toHaveBeenCalledWith("/api/v1/user/profile/p1", { + name: "Admin Set Name", + callsign: "AB1CDE", + description: "Mesh enthusiast", + url: "https://example.com", + }); + }); + }); +}); + describe("Profile (own view)", () => { it("shows a login prompt when OIDC is disabled", async () => { renderWithProviders(, { route: "/profile" }); diff --git a/src/meshcore_hub/web/static/js/spa-react/pages/Profile.tsx b/src/meshcore_hub/web/static/js/spa-react/pages/Profile.tsx index fd02c17..7906d93 100644 --- a/src/meshcore_hub/web/static/js/spa-react/pages/Profile.tsx +++ b/src/meshcore_hub/web/static/js/spa-react/pages/Profile.tsx @@ -1,8 +1,8 @@ -import { type FormEvent } from "react"; +import { type FormEvent, useState } from "react"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { Link, useNavigate, useParams, useSearchParams } from "react-router"; import { useTranslation } from "react-i18next"; -import { useAppConfig } from "@/context/AppConfigContext"; +import { useAppConfig, hasRole } from "@/context/AppConfigContext"; import { apiGet, apiPut } from "@/utils/api"; import { qk, invalidate } from "@/utils/queryKeys"; import { resolveNodeName, useFormatDateTime } from "@/utils/format"; @@ -116,9 +116,120 @@ function AdoptedNodesCard({ ); } +function ProfileEditForm({ + profile, + onSuccess, + onError, +}: { + profile: UserProfileData; + onSuccess?: () => void; + onError?: (msg: string) => void; +}) { + const { t } = useTranslation(); + const queryClient = useQueryClient(); + + const updateMutation = useMutation({ + mutationFn: ({ + id, + body, + }: { + id: string; + body: Record; + }) => apiPut(`/api/v1/user/profile/${id}`, body), + onSuccess: () => invalidate.profiles(queryClient), + }); + + const handleSubmit = async (e: FormEvent) => { + e.preventDefault(); + const data = new FormData(e.currentTarget); + const body = { + name: String(data.get("name") ?? "").trim() || null, + callsign: String(data.get("callsign") ?? "").trim() || null, + description: String(data.get("description") ?? "").trim() || null, + url: String(data.get("url") ?? "").trim() || null, + }; + try { + await updateMutation.mutateAsync({ id: profile.id, body }); + onSuccess?.(); + } catch (err) { + onError?.((err as Error).message); + } + }; + + return ( +
+ + + + + +
+ ); +} + function PublicProfileView({ id }: { id: string }) { const { t } = useTranslation(); const config = useAppConfig(); + const [editMode, setEditMode] = useState(false); + const [editError, setEditError] = useState(null); const { data: profile, error: queryError } = useQuery({ queryKey: qk.profiles.detail(id), queryFn: ({ signal }) => @@ -131,6 +242,7 @@ function PublicProfileView({ id }: { id: string }) { const isOwner = !!config.user && !!profile.user_id && config.user.sub === profile.user_id; + const isAdmin = hasRole("admin"); return ( <> @@ -147,34 +259,79 @@ function PublicProfileView({ id }: { id: string }) { {t("user_profile.edit_profile")} )} + {!isOwner && isAdmin && !editMode && ( + + )} + {editMode && ( + + )} -
-
-

- {profile.name || t("common.unnamed")} - {profile.callsign && } -

- - {profile.description && ( -

{profile.description}

- )} - {profile.url && ( - - {profile.url} - - )} - - {hasOperatorOrAdmin(profile.roles, config) && ( - - )} + {editMode && isAdmin ? ( +
+
+

{t("user_profile.edit_profile")}

+ + {editError && } + { + setEditMode(false); + setEditError(null); + }} + onError={(msg) => setEditError(msg)} + /> + +
-
+ ) : ( +
+
+

+ {profile.name || t("common.unnamed")} + {profile.callsign && ( + + )} +

+ + {profile.description && ( +

{profile.description}

+ )} + {profile.url && ( + + {profile.url} + + )} + + {hasOperatorOrAdmin(profile.roles, config) && ( + + )} +
+
+ )} ); } @@ -184,7 +341,6 @@ function OwnProfileView() { const config = useAppConfig(); const navigate = useNavigate(); const [searchParams] = useSearchParams(); - const queryClient = useQueryClient(); const { data: profile, error: queryError } = useQuery({ queryKey: qk.profiles.me(), queryFn: ({ signal }) => @@ -192,17 +348,6 @@ function OwnProfileView() { }); const error = queryError ? queryError.message : null; - const updateMutation = useMutation({ - mutationFn: ({ - id, - body, - }: { - id: string; - body: Record; - }) => apiPut(`/api/v1/user/profile/${id}`, body), - onSuccess: () => invalidate.profiles(queryClient), - }); - if (!config.oidc_enabled || !config.user) { return (
@@ -221,29 +366,6 @@ function OwnProfileView() { const flashMessage = searchParams.get("message") || ""; const flashError = searchParams.get("error") || ""; - const handleSubmit = async (e: FormEvent) => { - e.preventDefault(); - const data = new FormData(e.currentTarget); - const body = { - name: String(data.get("name") ?? "").trim() || null, - callsign: String(data.get("callsign") ?? "").trim() || null, - description: String(data.get("description") ?? "").trim() || null, - url: String(data.get("url") ?? "").trim() || null, - }; - try { - await updateMutation.mutateAsync({ id: profile.id, body }); - navigate( - "/profile?message=" + encodeURIComponent(t("user_profile.profile_updated")), - { replace: true }, - ); - } catch (err) { - navigate( - "/profile?error=" + encodeURIComponent((err as Error).message), - { replace: true }, - ); - } - }; - return ( <>

{t("user_profile.your_profile")}

-
- - - - - -
+ + navigate( + "/profile?message=" + + encodeURIComponent(t("user_profile.profile_updated")), + { replace: true }, + ) + } + onError={(msg) => + navigate( + "/profile?error=" + encodeURIComponent(msg), + { replace: true }, + ) + } + />
diff --git a/tests/test_api/test_user_profiles.py b/tests/test_api/test_user_profiles.py index 65cd10a..641cac6 100644 --- a/tests/test_api/test_user_profiles.py +++ b/tests/test_api/test_user_profiles.py @@ -28,6 +28,10 @@ NO_ROLES_HEADERS = { "X-User-Id": TEST_USER_ID, "X-User-Roles": "", } +ADMIN_HEADERS = { + "X-User-Id": OTHER_USER_ID, + "X-User-Roles": "admin", +} class TestListProfiles: @@ -383,6 +387,27 @@ class TestUpdateProfile: ) assert response.status_code == 403 + def test_update_profile_admin_can_edit_other( + self, client_no_auth, sample_user_profile + ): + """Test that an admin can update another user's profile.""" + response = client_no_auth.put( + f"/api/v1/user/profile/{sample_user_profile.id}", + json={ + "name": "Admin Edited", + "callsign": "ADM1", + "description": "Admin revised this", + "url": "https://example.com/admin", + }, + headers=ADMIN_HEADERS, + ) + assert response.status_code == 200 + data = response.json() + assert data["name"] == "Admin Edited" + assert data["callsign"] == "ADM1" + assert data["description"] == "Admin revised this" + assert data["url"] == "https://example.com/admin" + def test_update_profile_rejects_missing_user_id( self, client_no_auth, sample_user_profile ):