From 6ae9ec07d9d12c7850b2718a0f3b612ef45a6b87 Mon Sep 17 00:00:00 2001
From: Ivan <45982459+ivntsng@users.noreply.github.com>
Date: Sun, 10 Nov 2024 22:48:34 -0800
Subject: [PATCH] enhance admin permission checks for moderator and content
 manager roles (#581)

* enhance admin permission checks for moderator and content manager roles

* update auth forms with dark theme styling

* updated button colors
---
 frontend/src/components/auth/AuthBlock.tsx    |  4 +-
 frontend/src/components/auth/LoginForm.tsx    |  7 +++-
 .../src/components/auth/SignupWithEmail.tsx   |  7 +++-
 frontend/src/components/pages/Login.tsx       |  2 +-
 frontend/src/components/pages/Logout.tsx      |  4 +-
 frontend/src/components/pages/Signup.tsx      |  5 ++-
 frontend/src/components/ui/Header.tsx         |  6 +--
 store/app/routers/users.py                    | 20 ++++------
 store/app/security/user.py                    | 37 ++++++++++++++++---
 9 files changed, 62 insertions(+), 30 deletions(-)

diff --git a/frontend/src/components/auth/AuthBlock.tsx b/frontend/src/components/auth/AuthBlock.tsx
index 9ecac6e5..a6dccf45 100644
--- a/frontend/src/components/auth/AuthBlock.tsx
+++ b/frontend/src/components/auth/AuthBlock.tsx
@@ -16,14 +16,14 @@ import { useAlertQueue } from "@/hooks/useAlertQueue";
 import { useAuthentication } from "@/hooks/useAuth";
 
 interface AuthBlockProps {
-  title?: string;
+  title?: string | React.ReactNode;
   onClosed?: () => void;
   signup?: boolean;
 }
 
 const AuthBlock: React.FC<AuthBlockProps> = ({ title, onClosed, signup }) => {
   return (
-    <Card className="w-[400px] bg-gray-2 text-gray-12 rounded-lg">
+    <Card className="w-[400px] bg-gray-12 text-gray-12 rounded-lg">
       <CardHeader>
         <Header title={title} onClosed={onClosed} />
       </CardHeader>
diff --git a/frontend/src/components/auth/LoginForm.tsx b/frontend/src/components/auth/LoginForm.tsx
index f135c1eb..4562e01b 100644
--- a/frontend/src/components/auth/LoginForm.tsx
+++ b/frontend/src/components/auth/LoginForm.tsx
@@ -60,7 +60,12 @@ const LoginForm = () => {
         showStrength={false} // Hide password strength bar
       />
       {/* Submit Button */}
-      <Button variant="default">Login</Button>
+      <Button
+        variant="outline"
+        className="bg-gray-12 text-gray-2 hover:bg-gray-9"
+      >
+        Login
+      </Button>
     </form>
   );
 };
diff --git a/frontend/src/components/auth/SignupWithEmail.tsx b/frontend/src/components/auth/SignupWithEmail.tsx
index 8a02bb47..25007037 100644
--- a/frontend/src/components/auth/SignupWithEmail.tsx
+++ b/frontend/src/components/auth/SignupWithEmail.tsx
@@ -56,7 +56,12 @@ const SignupWithEmail = () => {
         {errors?.email && <ErrorMessage>{errors?.email?.message}</ErrorMessage>}
       </div>
       {/* Signup Button */}
-      <Button variant="default">Sign up with email</Button>
+      <Button
+        variant="outline"
+        className="bg-gray-12 text-gray-2 hover:bg-gray-9"
+      >
+        Sign up with email
+      </Button>
     </form>
   );
 };
diff --git a/frontend/src/components/pages/Login.tsx b/frontend/src/components/pages/Login.tsx
index 41d727d6..1ebd9bb7 100644
--- a/frontend/src/components/pages/Login.tsx
+++ b/frontend/src/components/pages/Login.tsx
@@ -4,7 +4,7 @@ const Login = () => {
   return (
     <div className="mx-8">
       <div className="flex justify-center items-center">
-        <AuthBlock title="Welcome back!" />
+        <AuthBlock title={<span className="text-gray-2">Welcome back!</span>} />
       </div>
     </div>
   );
diff --git a/frontend/src/components/pages/Logout.tsx b/frontend/src/components/pages/Logout.tsx
index 0f4b2b02..6b1a1b64 100644
--- a/frontend/src/components/pages/Logout.tsx
+++ b/frontend/src/components/pages/Logout.tsx
@@ -24,9 +24,9 @@ const Logout = () => {
   return (
     <div className="mx-8">
       <div className="flex justify-center items-center">
-        <Card className="w-[400px] shadow-md bg-gray-2 text-gray-12 rounded-lg">
+        <Card className="w-[400px] shadow-md bg-gray-12 text-gray-12 rounded-lg">
           <CardHeader>
-            <Header title="Logout" />
+            <Header title={<span className="text-gray-2">Logout</span>} />
           </CardHeader>
           <CardContent className="flex justify-center">
             <Spinner />
diff --git a/frontend/src/components/pages/Signup.tsx b/frontend/src/components/pages/Signup.tsx
index da2fa14a..4aafaf23 100644
--- a/frontend/src/components/pages/Signup.tsx
+++ b/frontend/src/components/pages/Signup.tsx
@@ -4,7 +4,10 @@ const Signup = () => {
   return (
     <div className="mx-8">
       <div className="flex justify-center items-center">
-        <AuthBlock title="Welcome!" signup />
+        <AuthBlock
+          title={<span className="text-gray-2">Welcome!</span>}
+          signup
+        />
       </div>
     </div>
   );
diff --git a/frontend/src/components/ui/Header.tsx b/frontend/src/components/ui/Header.tsx
index 2a654c80..0b974866 100644
--- a/frontend/src/components/ui/Header.tsx
+++ b/frontend/src/components/ui/Header.tsx
@@ -1,7 +1,7 @@
 import { FaTimes } from "react-icons/fa";
 
 interface HeaderProps {
-  title?: string;
+  title: string | React.ReactNode;
   label?: string;
   onClosed?: () => void;
 }
@@ -9,9 +9,7 @@ interface HeaderProps {
 const Header = ({ title, label, onClosed }: HeaderProps) => {
   return (
     <div className="w-full flex flex-col items-center justify-center gap-y-4">
-      <h1 className="text-3xl font-semibold text-primary py-4">
-        {title ?? "K-Scale Labs"}
-      </h1>
+      <h1 className="text-3xl font-semibold text-primary py-4">{title}</h1>
       {label && <p className="text-muted-foreground text-s,">{label}</p>}
       {onClosed && (
         <button
diff --git a/store/app/routers/users.py b/store/app/routers/users.py
index 8014c8fe..746e195a 100644
--- a/store/app/routers/users.py
+++ b/store/app/routers/users.py
@@ -16,6 +16,8 @@
     get_session_user_with_admin_permission,
     get_session_user_with_read_permission,
     get_session_user_with_write_permission,
+    verify_admin_permission,
+    verify_target_not_admin,
 )
 from store.app.utils.email import send_delete_email
 
@@ -268,14 +270,10 @@ async def set_moderator(
     admin_user: Annotated[User, Depends(get_session_user_with_admin_permission)],
     crud: Annotated[Crud, Depends(Crud.get)],
 ) -> UserPublic:
-    # Get the target user first to check their permissions
-    target_user = await crud.get_user(request.user_id, throw_if_missing=True)
+    verify_admin_permission(admin_user, "modify moderator status")
 
-    # Prevent modifying admin users' moderator status
-    if target_user.permissions and "admin" in target_user.permissions:
-        raise HTTPException(
-            status_code=status.HTTP_403_FORBIDDEN, detail="Cannot modify moderator status of admin users"
-        )
+    target_user = await crud.get_user(request.user_id, throw_if_missing=True)
+    verify_target_not_admin(target_user, "modify moderator status")
 
     updated_user = await crud.set_moderator(request.user_id, request.is_mod)
     return UserPublic(**updated_user.model_dump())
@@ -302,12 +300,10 @@ async def set_content_manager(
     admin_user: Annotated[User, Depends(get_session_user_with_admin_permission)],
     crud: Annotated[Crud, Depends(Crud.get)],
 ) -> UserPublic:
-    target_user = await crud.get_user(request.user_id, throw_if_missing=True)
+    verify_admin_permission(admin_user, "modify content manager status")
 
-    if target_user.permissions and "admin" in target_user.permissions:
-        raise HTTPException(
-            status_code=status.HTTP_403_FORBIDDEN, detail="Cannot modify content manager status of admin users"
-        )
+    target_user = await crud.get_user(request.user_id, throw_if_missing=True)
+    verify_target_not_admin(target_user, "modify content manager status")
 
     try:
         updated_user = await crud.set_content_manager(request.user_id, request.is_content_manager)
diff --git a/store/app/security/user.py b/store/app/security/user.py
index 333fcdb8..d7fefd73 100644
--- a/store/app/security/user.py
+++ b/store/app/security/user.py
@@ -1,4 +1,4 @@
-"""Defines security-related functions for authenticating users with given permissions."""
+"""Defines security-related functions for authenticating users and verifying permissions."""
 
 from typing import Annotated
 
@@ -10,6 +10,33 @@
 from store.app.security.requests import get_request_api_key_id, maybe_get_request_api_key_id
 
 
+def verify_admin_permission(user: User, action_description: str = "perform this action") -> None:
+    """Verifies that a user has admin permissions."""
+    if not user.permissions or "is_admin" not in user.permissions:
+        raise HTTPException(
+            status_code=status.HTTP_403_FORBIDDEN, detail=f"Only administrators can {action_description}"
+        )
+
+
+def verify_target_not_admin(user: User, action_description: str = "modify this user") -> None:
+    """Verifies that a target user is not an admin."""
+    if user.permissions and "is_admin" in user.permissions:
+        raise HTTPException(
+            status_code=status.HTTP_403_FORBIDDEN, detail=f"Cannot {action_description} for admin users"
+        )
+
+
+async def get_session_user(
+    crud: Annotated[Crud, Depends(Crud.get)],
+    api_key_id: Annotated[str, Depends(get_request_api_key_id)],
+) -> User:
+    try:
+        api_key = await crud.get_api_key(api_key_id)
+        return await crud.get_user(api_key.user_id, throw_if_missing=True)
+    except ItemNotFoundError:
+        raise NotAuthenticatedError("Not authenticated")
+
+
 async def get_session_user_with_permission(
     permission: str,
     crud: Crud,
@@ -38,11 +65,9 @@ async def get_session_user_with_write_permission(
     return await get_session_user_with_permission("write", crud, api_key_id)
 
 
-async def get_session_user_with_admin_permission(
-    crud: Annotated[Crud, Depends(Crud.get)],
-    api_key_id: Annotated[str, Depends(get_request_api_key_id)],
-) -> User:
-    return await get_session_user_with_permission("admin", crud, api_key_id)
+async def get_session_user_with_admin_permission(user: Annotated[User, Depends(get_session_user)]) -> User:
+    verify_admin_permission(user)
+    return user
 
 
 async def maybe_get_user_from_api_key(