Remove URL upload of avatar

chriscarrollsmith · chriscarrollsmith · commit f947dc98db2d · 2024-12-15T16:24:29.000-05:00
Only get avatar if user is authorized
diff --git a/routers/user.py b/routers/user.py
@@ -1,10 +1,10 @@
-from fastapi import APIRouter, Depends, HTTPException, Form, UploadFile, File
+from fastapi import APIRouter, Depends, Form, UploadFile, File
 from fastapi.responses import RedirectResponse, Response
 from pydantic import BaseModel, EmailStr
-from sqlmodel import Session, select
+from sqlmodel import Session
 from typing import Optional
-from utils.models import User
-from utils.auth import get_session, get_authenticated_user, verify_password
+from utils.models import User, DataIntegrityError
+from utils.auth import get_session, get_authenticated_user, verify_password, PasswordValidationError
 
 router = APIRouter(prefix="/user", tags=["user"])
 
@@ -16,7 +16,6 @@ class UpdateProfile(BaseModel):
     """Request model for updating user profile information"""
     name: str
     email: EmailStr
-    avatar_url: Optional[str] = None
     avatar_file: Optional[bytes] = None
     avatar_content_type: Optional[str] = None
 
@@ -25,21 +24,18 @@ async def as_form(
         cls,
         name: str = Form(...),
         email: EmailStr = Form(...),
-        avatar_url: Optional[str] = Form(None),
         avatar_file: Optional[UploadFile] = File(None),
     ):
         avatar_data = None
         avatar_content_type = None
 
         if avatar_file:
-            # Read the file content
             avatar_data = await avatar_file.read()
             avatar_content_type = avatar_file.content_type
 
         return cls(
             name=name,
             email=email,
-            avatar_url=avatar_url if not avatar_file else None,
             avatar_file=avatar_data,
             avatar_content_type=avatar_content_type
         )
@@ -62,74 +58,64 @@ async def as_form(
 @router.post("/update_profile", response_class=RedirectResponse)
 async def update_profile(
     user_profile: UpdateProfile = Depends(UpdateProfile.as_form),
-    current_user: User = Depends(get_authenticated_user),
+    user: User = Depends(get_authenticated_user),
     session: Session = Depends(get_session)
 ):
     # Update user details
-    current_user.name = user_profile.name
-    current_user.email = user_profile.email
+    user.name = user_profile.name
+    user.email = user_profile.email
 
     # Handle avatar update
     if user_profile.avatar_file:
-        current_user.avatar_url = None
-        current_user.avatar_data = user_profile.avatar_file
-        current_user.avatar_content_type = user_profile.avatar_content_type
-    else:
-        current_user.avatar_url = user_profile.avatar_url
-        current_user.avatar_data = None
-        current_user.avatar_content_type = None
+        user.avatar_data = user_profile.avatar_file
+        user.avatar_content_type = user_profile.avatar_content_type
 
     session.commit()
-    session.refresh(current_user)
+    session.refresh(user)
     return RedirectResponse(url="/profile", status_code=303)
 
 
 @router.post("/delete_account", response_class=RedirectResponse)
 async def delete_account(
     user_delete_account: UserDeleteAccount = Depends(
         UserDeleteAccount.as_form),
-    current_user: User = Depends(get_authenticated_user),
+    user: User = Depends(get_authenticated_user),
     session: Session = Depends(get_session)
 ):
-    if not current_user.password:
-        raise HTTPException(
-            status_code=500,
-            detail="User password not found in database; please contact a system administrator"
+    if not user.password:
+        raise DataIntegrityError(
+            resource="User password"
         )
 
     if not verify_password(
         user_delete_account.confirm_delete_password,
-        current_user.password.hashed_password
+        user.password.hashed_password
     ):
-        raise HTTPException(
-            status_code=400,
-            detail="Password is incorrect"
+        raise PasswordValidationError(
+            field="confirm_delete_password",
+            message="Password is incorrect"
         )
 
     # Delete the user
-    session.delete(current_user)
+    session.delete(user)
     session.commit()
 
     # Log out the user
     return RedirectResponse(url="/auth/logout", status_code=303)
 
 
-@router.get("/avatar/{user_id}")
+@router.get("/avatar")
 async def get_avatar(
-    user_id: int,
+    user: User = Depends(get_authenticated_user),
     session: Session = Depends(get_session)
 ):
     """Serve avatar image from database"""
-    user = session.exec(select(User).where(User.id == user_id)).first()
-    if not user:
-        raise HTTPException(status_code=404, detail="User not found")
-
-    if user.avatar_data:
-        return Response(
-            content=user.avatar_data,
-            media_type=user.avatar_content_type
+    if not user.avatar_data:
+        raise DataIntegrityError(
+            resource="User avatar"
         )
-    elif user.avatar_url:
-        return RedirectResponse(url=user.avatar_url)
-    else:
-        raise HTTPException(status_code=404, detail="Avatar not found")
+
+    return Response(
+        content=user.avatar_data,
+        media_type=user.avatar_content_type
+    )
diff --git a/templates/users/organization.html b/templates/users/organization.html
@@ -63,11 +63,7 @@ <h1 class="mb-4">{{ organization.name }}</h1>
                             {% for user in role.users %}
                             <tr>
                                 <td class="text-center" style="width: 50px;">
-                                    {% if user.avatar_url %}
-                                        <img src="{{ user.avatar_url }}" alt="User Avatar" class="rounded-circle" width="40" height="40">
-                                    {% else %}
-                                        {{ render_silhouette(width=40, height=40) }}
-                                    {% endif %}
+                                    {{ render_silhouette(width=40, height=40) }}
                                 </td>
                                 <td>{{ user.name }}</td>
                                 <td>{{ user.email }}</td>
diff --git a/templates/users/profile.html b/templates/users/profile.html
@@ -18,8 +18,8 @@ <h1 class="mb-4">User Profile</h1>
             <p><strong>Email:</strong> {{ user.email }}</p>
             <!-- Display user avatar or silhouette if no avatar is available -->
             <div class="mb-3">
-                {% if user.avatar_url or user.avatar_data %}
-                    <img src="{{ url_for('get_avatar', user_id=user.id) }}" alt="User Avatar" class="img-thumbnail" width="150">
+                {% if user.avatar_data %}
+                    <img src="{{ url_for('get_avatar') }}" alt="User Avatar" class="img-thumbnail" width="150">
                 {% else %}
                     {{ render_silhouette(width=150, height=150) }}
                 {% endif %}
@@ -45,20 +45,8 @@ <h1 class="mb-4">User Profile</h1>
                     <input type="email" class="form-control" id="email" name="email" value="{{ user.email }}">
                 </div>
                 <div class="mb-3">
-                    <label class="form-label">Avatar</label>
-                    <div class="mb-2">
-                        <label for="avatar_type" class="form-label">Choose avatar type:</label>
-                        <select class="form-select" id="avatar_type" onchange="toggleAvatarInput()">
-                            <option value="url">URL</option>
-                            <option value="file">Upload File</option>
-                        </select>
-                    </div>
-                    <div id="url_input" class="mb-2">
-                        <input type="url" class="form-control" id="avatar_url" name="avatar_url" value="{{ user.avatar_url or '' }}">
-                    </div>
-                    <div id="file_input" class="mb-2" style="display: none;">
-                        <input type="file" class="form-control" id="avatar_file" name="avatar_file" accept="image/*">
-                    </div>
+                    <label for="avatar_file" class="form-label">Avatar</label>
+                    <input type="file" class="form-control" id="avatar_file" name="avatar_file" accept="image/*">
                 </div>
                 <button type="submit" class="btn btn-primary">Save Changes</button>
             </form>
@@ -115,39 +103,5 @@ <h1 class="mb-4">User Profile</h1>
             editProfile.style.display = 'block';
         }
     }
-
-    // Function to toggle between URL and file upload inputs
-    function toggleAvatarInput() {
-        var avatarType = document.getElementById('avatar_type').value;
-        var urlInput = document.getElementById('url_input');
-        var fileInput = document.getElementById('file_input');
-        var urlField = document.getElementById('avatar_url');
-        
-        if (avatarType === 'url') {
-            urlInput.style.display = 'block';
-            fileInput.style.display = 'none';
-            // Clear file input
-            document.getElementById('avatar_file').value = '';
-        } else {
-            urlInput.style.display = 'none';
-            fileInput.style.display = 'block';
-            // Clear URL input
-            urlField.value = '';
-        }
-    }
-
-    // Add form submission validation
-    document.querySelector('form[action="{{ url_for("update_profile") }}"]').addEventListener('submit', function(e) {
-        var avatarType = document.getElementById('avatar_type').value;
-        var urlField = document.getElementById('avatar_url');
-        var fileField = document.getElementById('avatar_file');
-
-        // Clear the unused field before submission
-        if (avatarType === 'url') {
-            fileField.value = '';
-        } else {
-            urlField.value = '';
-        }
-    });
 </script>
 {% endblock %}
diff --git a/tests/test_user.py b/tests/test_user.py
@@ -13,7 +13,9 @@ def test_update_profile_unauthorized(unauth_client: TestClient):
         data={
             "name": "New Name",
             "email": "new@example.com",
-            "avatar_url": "https://example.com/avatar.jpg"
+        },
+        files={
+            "avatar_file": ("test_avatar.jpg", b"fake image data", "image/jpeg")
         },
         follow_redirects=False
     )
@@ -23,14 +25,40 @@ def test_update_profile_unauthorized(unauth_client: TestClient):
 
 def test_update_profile_authorized(auth_client: TestClient, test_user: User, session: Session):
     """Test that authorized users can edit their profile"""
-
+    
+    # Create test image data
+    test_image_data = b"fake image data"
+    
     # Update profile
     response: Response = auth_client.post(
         app.url_path_for("update_profile"),
         data={
             "name": "Updated Name",
             "email": "updated@example.com",
-            "avatar_url": "https://example.com/new-avatar.jpg"
+        },
+        files={
+            "avatar_file": ("test_avatar.jpg", test_image_data, "image/jpeg")
+        },
+        follow_redirects=False
+    )
+    assert response.status_code == 303
+    assert response.headers["location"] == app.url_path_for("read_profile")
+
+    # Verify changes in database
+    session.refresh(test_user)
+    assert test_user.name == "Updated Name"
+    assert test_user.email == "updated@example.com"
+    assert test_user.avatar_data == test_image_data
+    assert test_user.avatar_content_type == "image/jpeg"
+
+
+def test_update_profile_without_avatar(auth_client: TestClient, test_user: User, session: Session):
+    """Test that profile can be updated without changing the avatar"""
+    response: Response = auth_client.post(
+        app.url_path_for("update_profile"),
+        data={
+            "name": "Updated Name",
+            "email": "updated@example.com",
         },
         follow_redirects=False
     )
@@ -41,7 +69,6 @@ def test_update_profile_authorized(auth_client: TestClient, test_user: User, ses
     session.refresh(test_user)
     assert test_user.name == "Updated Name"
     assert test_user.email == "updated@example.com"
-    assert test_user.avatar_url == "https://example.com/new-avatar.jpg"
 
 
 def test_delete_account_unauthorized(unauth_client: TestClient):
@@ -62,7 +89,7 @@ def test_delete_account_wrong_password(auth_client: TestClient, test_user: User)
         data={"confirm_delete_password": "WrongPassword123!"},
         follow_redirects=False
     )
-    assert response.status_code == 400
+    assert response.status_code == 422
     assert "Password is incorrect" in response.text.strip()
 
 
@@ -81,3 +108,37 @@ def test_delete_account_success(auth_client: TestClient, test_user: User, sessio
     # Verify user is deleted from database
     user = session.get(User, test_user.id)
     assert user is None
+
+
+def test_get_avatar_authorized(auth_client: TestClient, test_user: User):
+    """Test getting user avatar"""
+    # First upload an avatar
+    test_image_data = b"fake image data"
+    auth_client.post(
+        app.url_path_for("update_profile"),
+        data={
+            "name": test_user.name,
+            "email": test_user.email,
+        },
+        files={
+            "avatar_file": ("test_avatar.jpg", test_image_data, "image/jpeg")
+        },
+    )
+
+    # Then try to retrieve it
+    response = auth_client.get(
+        app.url_path_for("get_avatar")
+    )
+    assert response.status_code == 200
+    assert response.content == test_image_data
+    assert response.headers["content-type"] == "image/jpeg"
+
+
+def test_get_avatar_unauthorized(unauth_client: TestClient):
+    """Test getting avatar for non-existent user"""
+    response = unauth_client.get(
+        app.url_path_for("get_avatar"),
+        follow_redirects=False
+    )
+    assert response.status_code == 303
+    assert response.headers["location"] == app.url_path_for("read_login")
diff --git a/utils/models.py b/utils/models.py
@@ -180,7 +180,6 @@ class User(SQLModel, table=True):
     id: Optional[int] = Field(default=None, primary_key=True)
     name: str
     email: str = Field(index=True, unique=True)
-    avatar_url: Optional[str] = None
     avatar_data: Optional[bytes] = Field(
         default=None, sa_column=Column(LargeBinary))
     avatar_content_type: Optional[str] = None
