Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions backend/src/modules/account/account_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
AccountUpdateData,
PaginatedAccountsResponse,
)
from src.modules.account.account_service import AccountService
from src.modules.account.account_service import AccountService, CannotDeleteOwnAccountException

account_router = APIRouter(prefix="/api/accounts", tags=["accounts"])

Expand Down Expand Up @@ -59,6 +59,8 @@ async def update_account(
async def delete_account(
account_id: int,
account_service: AccountService = Depends(),
_=Depends(authenticate_admin),
current_admin: AccountDto = Depends(authenticate_admin),
) -> AccountDto:
if account_id == current_admin.id:
raise CannotDeleteOwnAccountException()
return await account_service.delete_account(account_id)
5 changes: 5 additions & 0 deletions backend/src/modules/account/account_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ def __init__(self, onyen: str):
super().__init__(f"Account with onyen {onyen} not found")


class CannotDeleteOwnAccountException(ForbiddenException):
def __init__(self):
super().__init__(detail="Admins cannot delete their own account")


class AccountService:
_ALLOWED_FIELDS: ClassVar[list[str]] = [
"id",
Expand Down
17 changes: 15 additions & 2 deletions backend/test/modules/account/account_router_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
from httpx import AsyncClient
from src.modules.account.account_entity import AccountEntity, AccountRole
from src.modules.account.account_model import AccountDto
from src.modules.account.account_service import AccountConflictException, AccountNotFoundException
from src.modules.account.account_service import (
AccountConflictException,
AccountNotFoundException,
CannotDeleteOwnAccountException,
)
from test.modules.account.account_utils import AccountTestUtils
from test.utils.http.assertions import (
assert_res_failure,
Expand Down Expand Up @@ -217,8 +221,17 @@ async def test_delete_account(self, accounts_two_per_role: list[AccountEntity]):
@pytest.mark.asyncio
async def test_delete_account_not_found(self):
"""Test deleting a non-existent account returns 404."""
response = await self.admin_client.delete("/api/accounts/88888")
assert_res_failure(response, AccountNotFoundException(88888))

@pytest.mark.asyncio
async def test_delete_own_account(self, accounts_two_per_role: list[AccountEntity]):
"""Test that an admin cannot delete their own account."""
response = await self.admin_client.delete("/api/accounts/99999")
assert_res_failure(response, AccountNotFoundException(99999))
assert_res_failure(response, CannotDeleteOwnAccountException())

accounts = await self.account_utils.get_all()
assert len(accounts) == len(accounts_two_per_role)


class TestAccountListSearch:
Expand Down
6 changes: 4 additions & 2 deletions frontend/src/app/api/auth/login/saml/route.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { AccountRole } from "@/lib/api/account/account.types";
import { exchangeToken, setAuthCookies } from "@/lib/api/auth/auth.service";
import { getSessionAccountIdFromAccessToken } from "@/lib/api/auth/sessionIdentity";
import { identityProvider, postAssert, serviceProvider } from "@/lib/saml";
import { AxiosError } from "axios";
import { NextRequest, NextResponse } from "next/server";
Expand Down Expand Up @@ -146,11 +147,12 @@ export async function POST(req: NextRequest) {
return errorUrl(status === 403 ? "AccessDenied" : "ExchangeFailed");
}

const accountId = getSessionAccountIdFromAccessToken(tokens.access_token);
const res = NextResponse.redirect(new URL(callbackUrl, origin));

await setAuthCookies(res, tokens, {
sub: samlUser.name_id,
id: samlUser.name_id,
sub: accountId,
id: accountId,
email,
name: `${firstName} ${lastName}`.trim(),
firstName,
Expand Down
25 changes: 25 additions & 0 deletions frontend/src/app/staff/_components/account/AccountTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import type {
AccountRole,
AccountTableRow,
} from "@/lib/api/account/account.types";
import {
canDeleteAccountRow,
getAccountDeleteBlockReason,
getCurrentAccountId,
} from "@/lib/api/account/accountDeleteGuard";
import type {
PoliceAccountUpdate,
PoliceRole,
Expand All @@ -27,6 +32,7 @@ import {
import { formatRoleLabel } from "@/lib/utils";
import { ColumnDef } from "@tanstack/react-table";
import { isAxiosError } from "axios";
import { useSession } from "next-auth/react";
import { useMemo, useState } from "react";
import * as z from "zod";
import { useSidebar } from "../shared/sidebar/SidebarContext";
Expand Down Expand Up @@ -86,6 +92,7 @@ const getErrorMessage = (error: Error): string => {
export const AccountTable = () => {
const { openSidebar, closeSidebar } = useSidebar();
const { openSnackbar } = useSnackbar();
const { data: session } = useSession();
const [editingAccount, setEditingAccount] = useState<AccountTableRow | null>(
null
);
Expand Down Expand Up @@ -229,6 +236,8 @@ export const AccountTable = () => {
},
});

const currentAccountId = getCurrentAccountId(session?.id);

const handleEdit = (row: AccountTableRow) => {
setEditingAccount(row);

Expand Down Expand Up @@ -265,6 +274,21 @@ export const AccountTable = () => {
};

const handleDelete = (row: AccountTableRow) => {
const deleteBlockReason = getAccountDeleteBlockReason(
row,
currentAccountId
);

if (deleteBlockReason === "self-delete") {
openSnackbar("You cannot delete your own account", "error");
return;
}

if (deleteBlockReason === "identity-unavailable") {
openSnackbar("Unable to confirm your account identity", "error");
return;
}

if (row._isPolice) {
deletePoliceAccountMutation.mutate(row.id);
} else {
Expand Down Expand Up @@ -387,6 +411,7 @@ export const AccountTable = () => {
deleteAccountMutation.isPending ||
deletePoliceAccountMutation.isPending
}
canDeleteRow={(row) => canDeleteAccountRow(row, currentAccountId)}
serverMeta={
accountsQuery.data
? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ export type TableProps<T> = {
error?: Error | null;
getDeleteDescription?: (row: T) => string;
isDeleting?: boolean;
isDeleteDisabled?: (row: T) => boolean;
initialSort?: SortingState;
sortBy?: (a: T, b: T) => number;
pageSize?: number;
Expand All @@ -108,6 +109,7 @@ export function TableTemplate<T extends object>({
error,
getDeleteDescription,
isDeleting,
isDeleteDisabled,
initialSort = [],
sortBy,
pageSize = 50,
Expand Down Expand Up @@ -342,7 +344,9 @@ export function TableTemplate<T extends object>({
</DropdownMenuItem>
)}
{onDelete &&
(canDeleteRow ? canDeleteRow(row.original) : true) && (
(canDeleteRow
? canDeleteRow(row.original)
: !isDeleteDisabled?.(row.original)) && (
<DropdownMenuItem
onClick={() => handleDeleteClick(row.original)}
variant="destructive"
Expand Down
70 changes: 70 additions & 0 deletions frontend/src/lib/api/account/accountDeleteGuard.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import assert from "node:assert/strict";
import { describe, it } from "node:test";
import type { AccountTableRow } from "./account.types";
import {
canDeleteAccountRow,
getAccountDeleteBlockReason,
getCurrentAccountId,
} from "./accountDeleteGuard";

function createRow(overrides: Partial<AccountTableRow> = {}): AccountTableRow {
return {
id: 7,
email: "admin@unc.edu",
first_name: "Admin",
last_name: "User",
pid: "123456789",
onyen: "adminuser",
role: "admin",
_isPolice: false,
...overrides,
};
}

describe("getCurrentAccountId", () => {
it("parses numeric session ids", () => {
assert.equal(getCurrentAccountId("42"), 42);
});

it("returns null for non-numeric session ids", () => {
assert.equal(getCurrentAccountId("saml-name-id"), null);
});
});

describe("account delete guards", () => {
it("blocks deleting the current admin row", () => {
const row = createRow({ id: 42 });

assert.equal(canDeleteAccountRow(row, 42), false);
assert.equal(getAccountDeleteBlockReason(row, 42), "self-delete");
});

it("allows deleting a different admin or staff row", () => {
const row = createRow({ id: 99, role: "staff" });

assert.equal(canDeleteAccountRow(row, 42), true);
assert.equal(getAccountDeleteBlockReason(row, 42), null);
});

it("keeps police rows deletable from the staff dashboard", () => {
const row = createRow({
id: 42,
email: "police@unc.edu",
role: "police_admin",
_isPolice: true,
});

assert.equal(canDeleteAccountRow(row, 42), true);
assert.equal(getAccountDeleteBlockReason(row, 42), null);
});

it("fails closed for account rows when session identity is unavailable", () => {
const row = createRow({ id: 42 });

assert.equal(canDeleteAccountRow(row, null), false);
assert.equal(
getAccountDeleteBlockReason(row, null),
"identity-unavailable"
);
});
});
36 changes: 36 additions & 0 deletions frontend/src/lib/api/account/accountDeleteGuard.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import type { AccountTableRow } from "./account.types";

type AccountDeleteBlockReason = "identity-unavailable" | "self-delete";

export function getCurrentAccountId(
sessionId: string | number | null | undefined
): number | null {
if (sessionId == null) {
return null;
}

const parsedId = Number(sessionId);
return Number.isInteger(parsedId) ? parsedId : null;
}

export function getAccountDeleteBlockReason(
row: AccountTableRow,
currentAccountId: number | null
): AccountDeleteBlockReason | null {
if (row._isPolice) {
return null;
}

if (currentAccountId === null) {
return "identity-unavailable";
}

return row.id === currentAccountId ? "self-delete" : null;
}

export function canDeleteAccountRow(
row: AccountTableRow,
currentAccountId: number | null
): boolean {
return getAccountDeleteBlockReason(row, currentAccountId) === null;
}
28 changes: 28 additions & 0 deletions frontend/src/lib/api/auth/sessionIdentity.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import assert from "node:assert/strict";
import { describe, it } from "node:test";
import { getSessionAccountIdFromAccessToken } from "./sessionIdentity";

function createJwt(payload: Record<string, unknown>): string {
const header = Buffer.from(
JSON.stringify({ alg: "none", typ: "JWT" })
).toString("base64url");
const body = Buffer.from(JSON.stringify(payload)).toString("base64url");
return `${header}.${body}.signature`;
}

describe("getSessionAccountIdFromAccessToken", () => {
it("uses the backend subject as the session account id", () => {
const accessToken = createJwt({ sub: 42, email: "admin@unc.edu" });

assert.equal(getSessionAccountIdFromAccessToken(accessToken), "42");
});

it("throws when the access token has no valid subject", () => {
const accessToken = createJwt({ email: "admin@unc.edu" });

assert.throws(
() => getSessionAccountIdFromAccessToken(accessToken),
/Backend access token is missing a valid subject/
);
});
});
14 changes: 14 additions & 0 deletions frontend/src/lib/api/auth/sessionIdentity.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { decodeJwtPayload } from "./auth.service";

export function getSessionAccountIdFromAccessToken(
accessToken: string
): string {
const payload = decodeJwtPayload(accessToken);
const subject = payload.sub;

if (typeof subject !== "string" && typeof subject !== "number") {
throw new Error("Backend access token is missing a valid subject");
}

return String(subject);
}
Loading