refactor(auth): unify authorization patterns with MANAGE_OWN_PROFILE permission
Issue #2: The profile route used a custom role-based check instead of the permission-based pattern used everywhere else. Changes: - Add MANAGE_OWN_PROFILE permission to backend Permission enum - Add permission to ROLE_REGULAR role definition - Update profile routes to use require_permission(MANAGE_OWN_PROFILE) - Remove custom require_regular_user dependency - Update frontend Permission constant and profile page - Update invites page to use permission instead of role check - Update profile tests with proper permission mocking This ensures consistent authorization patterns across all routes.
This commit is contained in:
parent
81cd34b0e7
commit
21698203fe
7 changed files with 40 additions and 23 deletions
|
|
@ -40,6 +40,9 @@ class Permission(str, PyEnum):
|
||||||
# Audit permissions
|
# Audit permissions
|
||||||
VIEW_AUDIT = "view_audit"
|
VIEW_AUDIT = "view_audit"
|
||||||
|
|
||||||
|
# Profile permissions
|
||||||
|
MANAGE_OWN_PROFILE = "manage_own_profile"
|
||||||
|
|
||||||
# Invite permissions
|
# Invite permissions
|
||||||
MANAGE_INVITES = "manage_invites"
|
MANAGE_INVITES = "manage_invites"
|
||||||
VIEW_OWN_INVITES = "view_own_invites"
|
VIEW_OWN_INVITES = "view_own_invites"
|
||||||
|
|
@ -93,6 +96,7 @@ ROLE_DEFINITIONS: dict[str, RoleConfig] = {
|
||||||
Permission.VIEW_COUNTER,
|
Permission.VIEW_COUNTER,
|
||||||
Permission.INCREMENT_COUNTER,
|
Permission.INCREMENT_COUNTER,
|
||||||
Permission.USE_SUM,
|
Permission.USE_SUM,
|
||||||
|
Permission.MANAGE_OWN_PROFILE,
|
||||||
Permission.VIEW_OWN_INVITES,
|
Permission.VIEW_OWN_INVITES,
|
||||||
Permission.BOOK_APPOINTMENT,
|
Permission.BOOK_APPOINTMENT,
|
||||||
Permission.VIEW_OWN_APPOINTMENTS,
|
Permission.VIEW_OWN_APPOINTMENTS,
|
||||||
|
|
|
||||||
|
|
@ -1,30 +1,18 @@
|
||||||
"""Profile routes for user contact details."""
|
"""Profile routes for user contact details."""
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends, HTTPException, status
|
from fastapi import APIRouter, Depends, HTTPException
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
|
|
||||||
from auth import get_current_user
|
from auth import require_permission
|
||||||
from database import get_db
|
from database import get_db
|
||||||
from models import ROLE_REGULAR, User
|
from models import Permission, User
|
||||||
from schemas import ProfileResponse, ProfileUpdate
|
from schemas import ProfileResponse, ProfileUpdate
|
||||||
from validation import validate_profile_fields
|
from validation import validate_profile_fields
|
||||||
|
|
||||||
router = APIRouter(prefix="/api/profile", tags=["profile"])
|
router = APIRouter(prefix="/api/profile", tags=["profile"])
|
||||||
|
|
||||||
|
|
||||||
async def require_regular_user(
|
|
||||||
current_user: User = Depends(get_current_user),
|
|
||||||
) -> User:
|
|
||||||
"""Dependency that requires the user to have the 'regular' role."""
|
|
||||||
if ROLE_REGULAR not in current_user.role_names:
|
|
||||||
raise HTTPException(
|
|
||||||
status_code=status.HTTP_403_FORBIDDEN,
|
|
||||||
detail="Profile access is only available to regular users",
|
|
||||||
)
|
|
||||||
return current_user
|
|
||||||
|
|
||||||
|
|
||||||
async def get_godfather_email(db: AsyncSession, godfather_id: int | None) -> str | None:
|
async def get_godfather_email(db: AsyncSession, godfather_id: int | None) -> str | None:
|
||||||
"""Get the email of a godfather user by ID."""
|
"""Get the email of a godfather user by ID."""
|
||||||
if not godfather_id:
|
if not godfather_id:
|
||||||
|
|
@ -35,7 +23,7 @@ async def get_godfather_email(db: AsyncSession, godfather_id: int | None) -> str
|
||||||
|
|
||||||
@router.get("", response_model=ProfileResponse)
|
@router.get("", response_model=ProfileResponse)
|
||||||
async def get_profile(
|
async def get_profile(
|
||||||
current_user: User = Depends(require_regular_user),
|
current_user: User = Depends(require_permission(Permission.MANAGE_OWN_PROFILE)),
|
||||||
db: AsyncSession = Depends(get_db),
|
db: AsyncSession = Depends(get_db),
|
||||||
) -> ProfileResponse:
|
) -> ProfileResponse:
|
||||||
"""Get the current user's profile (contact details and godfather)."""
|
"""Get the current user's profile (contact details and godfather)."""
|
||||||
|
|
@ -54,7 +42,7 @@ async def get_profile(
|
||||||
async def update_profile(
|
async def update_profile(
|
||||||
data: ProfileUpdate,
|
data: ProfileUpdate,
|
||||||
db: AsyncSession = Depends(get_db),
|
db: AsyncSession = Depends(get_db),
|
||||||
current_user: User = Depends(require_regular_user),
|
current_user: User = Depends(require_permission(Permission.MANAGE_OWN_PROFILE)),
|
||||||
) -> ProfileResponse:
|
) -> ProfileResponse:
|
||||||
"""Update the current user's profile (contact details)."""
|
"""Update the current user's profile (contact details)."""
|
||||||
# Validate all fields
|
# Validate all fields
|
||||||
|
|
|
||||||
|
|
@ -166,12 +166,12 @@ class TestGetProfileEndpoint:
|
||||||
assert data["nostr_npub"] is None
|
assert data["nostr_npub"] is None
|
||||||
|
|
||||||
async def test_admin_user_cannot_access_profile(self, client_factory, admin_user):
|
async def test_admin_user_cannot_access_profile(self, client_factory, admin_user):
|
||||||
"""Admin user gets 403 when trying to access profile."""
|
"""Admin user gets 403 when trying to access profile (lacks MANAGE_OWN_PROFILE)."""
|
||||||
async with client_factory.create(cookies=admin_user["cookies"]) as client:
|
async with client_factory.create(cookies=admin_user["cookies"]) as client:
|
||||||
response = await client.get("/api/profile")
|
response = await client.get("/api/profile")
|
||||||
|
|
||||||
assert response.status_code == 403
|
assert response.status_code == 403
|
||||||
assert "regular users" in response.json()["detail"].lower()
|
assert "manage_own_profile" in response.json()["detail"].lower()
|
||||||
|
|
||||||
async def test_unauthenticated_user_gets_401(self, client_factory):
|
async def test_unauthenticated_user_gets_401(self, client_factory):
|
||||||
"""Unauthenticated user gets 401."""
|
"""Unauthenticated user gets 401."""
|
||||||
|
|
|
||||||
|
|
@ -13,6 +13,7 @@ export const Permission = {
|
||||||
INCREMENT_COUNTER: "increment_counter",
|
INCREMENT_COUNTER: "increment_counter",
|
||||||
USE_SUM: "use_sum",
|
USE_SUM: "use_sum",
|
||||||
VIEW_AUDIT: "view_audit",
|
VIEW_AUDIT: "view_audit",
|
||||||
|
MANAGE_OWN_PROFILE: "manage_own_profile",
|
||||||
MANAGE_INVITES: "manage_invites",
|
MANAGE_INVITES: "manage_invites",
|
||||||
VIEW_OWN_INVITES: "view_own_invites",
|
VIEW_OWN_INVITES: "view_own_invites",
|
||||||
// Booking permissions (regular users)
|
// Booking permissions (regular users)
|
||||||
|
|
|
||||||
|
|
@ -6,6 +6,7 @@ import { Header } from "../components/Header";
|
||||||
import { useRequireAuth } from "../hooks/useRequireAuth";
|
import { useRequireAuth } from "../hooks/useRequireAuth";
|
||||||
import { components } from "../generated/api";
|
import { components } from "../generated/api";
|
||||||
import constants from "../../../shared/constants.json";
|
import constants from "../../../shared/constants.json";
|
||||||
|
import { Permission } from "../auth-context";
|
||||||
import {
|
import {
|
||||||
layoutStyles,
|
layoutStyles,
|
||||||
cardStyles,
|
cardStyles,
|
||||||
|
|
@ -19,7 +20,7 @@ type Invite = components["schemas"]["UserInviteResponse"];
|
||||||
|
|
||||||
export default function InvitesPage() {
|
export default function InvitesPage() {
|
||||||
const { user, isLoading, isAuthorized } = useRequireAuth({
|
const { user, isLoading, isAuthorized } = useRequireAuth({
|
||||||
requiredRole: constants.roles.REGULAR,
|
requiredPermission: Permission.VIEW_OWN_INVITES,
|
||||||
fallbackRedirect: "/audit",
|
fallbackRedirect: "/audit",
|
||||||
});
|
});
|
||||||
const [invites, setInvites] = useState<Invite[]>([]);
|
const [invites, setInvites] = useState<Invite[]>([]);
|
||||||
|
|
|
||||||
|
|
@ -15,11 +15,14 @@ let mockUser: { id: number; email: string; roles: string[]; permissions: string[
|
||||||
id: 1,
|
id: 1,
|
||||||
email: "test@example.com",
|
email: "test@example.com",
|
||||||
roles: ["regular"],
|
roles: ["regular"],
|
||||||
permissions: ["view_counter", "increment_counter", "use_sum"],
|
permissions: ["view_counter", "increment_counter", "use_sum", "manage_own_profile"],
|
||||||
};
|
};
|
||||||
let mockIsLoading = false;
|
let mockIsLoading = false;
|
||||||
const mockLogout = vi.fn();
|
const mockLogout = vi.fn();
|
||||||
const mockHasRole = vi.fn((role: string) => mockUser?.roles.includes(role) ?? false);
|
const mockHasRole = vi.fn((role: string) => mockUser?.roles.includes(role) ?? false);
|
||||||
|
const mockHasPermission = vi.fn(
|
||||||
|
(permission: string) => mockUser?.permissions.includes(permission) ?? false
|
||||||
|
);
|
||||||
|
|
||||||
vi.mock("../auth-context", () => ({
|
vi.mock("../auth-context", () => ({
|
||||||
useAuth: () => ({
|
useAuth: () => ({
|
||||||
|
|
@ -27,7 +30,23 @@ vi.mock("../auth-context", () => ({
|
||||||
isLoading: mockIsLoading,
|
isLoading: mockIsLoading,
|
||||||
logout: mockLogout,
|
logout: mockLogout,
|
||||||
hasRole: mockHasRole,
|
hasRole: mockHasRole,
|
||||||
|
hasPermission: mockHasPermission,
|
||||||
}),
|
}),
|
||||||
|
Permission: {
|
||||||
|
VIEW_COUNTER: "view_counter",
|
||||||
|
INCREMENT_COUNTER: "increment_counter",
|
||||||
|
USE_SUM: "use_sum",
|
||||||
|
VIEW_AUDIT: "view_audit",
|
||||||
|
MANAGE_OWN_PROFILE: "manage_own_profile",
|
||||||
|
MANAGE_INVITES: "manage_invites",
|
||||||
|
VIEW_OWN_INVITES: "view_own_invites",
|
||||||
|
BOOK_APPOINTMENT: "book_appointment",
|
||||||
|
VIEW_OWN_APPOINTMENTS: "view_own_appointments",
|
||||||
|
CANCEL_OWN_APPOINTMENT: "cancel_own_appointment",
|
||||||
|
MANAGE_AVAILABILITY: "manage_availability",
|
||||||
|
VIEW_ALL_APPOINTMENTS: "view_all_appointments",
|
||||||
|
CANCEL_ANY_APPOINTMENT: "cancel_any_appointment",
|
||||||
|
},
|
||||||
}));
|
}));
|
||||||
|
|
||||||
// Mock profile data
|
// Mock profile data
|
||||||
|
|
@ -45,10 +64,13 @@ beforeEach(() => {
|
||||||
id: 1,
|
id: 1,
|
||||||
email: "test@example.com",
|
email: "test@example.com",
|
||||||
roles: ["regular"],
|
roles: ["regular"],
|
||||||
permissions: ["view_counter", "increment_counter", "use_sum"],
|
permissions: ["view_counter", "increment_counter", "use_sum", "manage_own_profile"],
|
||||||
};
|
};
|
||||||
mockIsLoading = false;
|
mockIsLoading = false;
|
||||||
mockHasRole.mockImplementation((role: string) => mockUser?.roles.includes(role) ?? false);
|
mockHasRole.mockImplementation((role: string) => mockUser?.roles.includes(role) ?? false);
|
||||||
|
mockHasPermission.mockImplementation(
|
||||||
|
(permission: string) => mockUser?.permissions.includes(permission) ?? false
|
||||||
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
afterEach(() => {
|
afterEach(() => {
|
||||||
|
|
|
||||||
|
|
@ -7,6 +7,7 @@ import { Header } from "../components/Header";
|
||||||
import { useRequireAuth } from "../hooks/useRequireAuth";
|
import { useRequireAuth } from "../hooks/useRequireAuth";
|
||||||
import { components } from "../generated/api";
|
import { components } from "../generated/api";
|
||||||
import constants from "../../../shared/constants.json";
|
import constants from "../../../shared/constants.json";
|
||||||
|
import { Permission } from "../auth-context";
|
||||||
import {
|
import {
|
||||||
layoutStyles,
|
layoutStyles,
|
||||||
cardStyles,
|
cardStyles,
|
||||||
|
|
@ -121,7 +122,7 @@ function toFormData(data: ProfileData): FormData {
|
||||||
|
|
||||||
export default function ProfilePage() {
|
export default function ProfilePage() {
|
||||||
const { user, isLoading, isAuthorized } = useRequireAuth({
|
const { user, isLoading, isAuthorized } = useRequireAuth({
|
||||||
requiredRole: constants.roles.REGULAR,
|
requiredPermission: Permission.MANAGE_OWN_PROFILE,
|
||||||
fallbackRedirect: "/audit",
|
fallbackRedirect: "/audit",
|
||||||
});
|
});
|
||||||
const [originalData, setOriginalData] = useState<FormData | null>(null);
|
const [originalData, setOriginalData] = useState<FormData | null>(null);
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue