Conversation
…d Pydantic models
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces several new features to the application, including an admin panel for user management, image emotion analysis, image management endpoints, and authentication functionalities. It also sets up a SQLite database for storing user information and configures the necessary dependencies. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new admin panel with endpoints for user management and system statistics. It also adds the basic structure for authentication, image uploads, and database setup. While the new features are a good step, I've identified several critical security vulnerabilities related to authentication and authorization. The current implementation allows any user to perform admin actions and upload files on behalf of other users. My review focuses on fixing these security holes, improving code structure by reducing duplication, and addressing some correctness and style issues. Please pay close attention to the comments marked as 'critical'.
| def get_all_users(admin_id: int, db: Session = Depends(get_db)): | ||
| """Admin: Get all users.""" | ||
| admin = db.query(User).filter( | ||
| User.id == admin_id, | ||
| User.is_admin == True | ||
| ).first() | ||
| if not admin: | ||
| raise HTTPException(status_code=403, detail="Admin access required") |
There was a problem hiding this comment.
The current authorization mechanism is critically flawed. It relies on an admin_id passed as a request parameter to verify admin privileges. This allows any user to gain admin access by simply providing the ID of a known admin user. Authorization should be based on the identity of the currently authenticated user, which should be established via a secure token (e.g., JWT) provided in the request headers, not a parameter in the request body or query.
This insecure pattern is repeated across all endpoints in this file (get_all_users, update_user, delete_user, get_stats).
A recommended approach is to:
- Modify the
/auth/loginendpoint to issue a JWT token upon successful login. - Create a dependency that verifies the JWT token from the
Authorizationheader and decodes it to get the current user. - Create another dependency that depends on the first one and checks if the current user has
is_admin == True. - Protect all admin endpoints with this admin-check dependency.
| @router.post("/login") | ||
| def login(user: UserLogin, db: Session = Depends(get_db)): | ||
| """Login with email and password.""" | ||
| db_user = db.query(User).filter(User.email == user.email).first() | ||
| if not db_user or not verify_password(user.password, db_user.hashed_password): | ||
| raise HTTPException( | ||
| status_code=401, | ||
| detail="Invalid email or password" | ||
| ) | ||
| return { | ||
| "message": "Login successful", | ||
| "user": { | ||
| "id": db_user.id, | ||
| "email": db_user.email, | ||
| "full_name": db_user.full_name, | ||
| "is_admin": db_user.is_admin | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The /login endpoint successfully verifies user credentials but fails to provide a mechanism for maintaining an authenticated session. After logging in, the client has no way to prove its identity on subsequent requests. This is the root cause of the insecure admin_id and user_id parameters seen in other parts of the application.
Upon successful login, you should generate and return an access token (e.g., a JSON Web Token - JWT). This token can then be sent by the client in the Authorization header for all protected endpoints.
You can use a library like python-jose (which is already in requirements.txt) to create JWTs.
| async def upload_image( | ||
| user_id: int, | ||
| narrative: str = "", | ||
| file: UploadFile = File(...), | ||
| db: Session = Depends(get_db) | ||
| ): |
There was a problem hiding this comment.
This endpoint has a critical security vulnerability. It accepts a user_id as a parameter to associate the uploaded image with a user. This allows any person to upload images on behalf of any other user, simply by knowing their ID.
File uploads should be associated with the currently authenticated user, whose identity should be determined from a secure access token sent with the request, not from a parameter controlled by the client.
| raise HTTPException(status_code=403, detail="Admin access required") | ||
|
|
||
| total_users = db.query(User).count() | ||
| active_users No newline at end of file |
There was a problem hiding this comment.
The get_stats function is incomplete. The line active_users will raise a NameError at runtime because the variable is referenced but never assigned a value. The function also doesn't return anything. You should complete the implementation to calculate and return the desired statistics.
For example, to count active users and return stats:
total_users = db.query(User).count()
active_users = db.query(User).filter(User.is_active == True).count()
# You can add more stats here
return {"total_users": total_users, "active_users": active_users}| filepath = os.path.join(UPLOAD_DIR, str(user_id), filename) | ||
|
|
||
| if not os.path.exists(filepath): | ||
| raise HTTPException(status_code=404, detail="Image not found") |
There was a problem hiding this comment.
There is a potential path traversal vulnerability here. The filename is taken directly from the URL path and used to construct a file path for deletion. A malicious user could provide a filename like ../../some_other_file to attempt to delete files outside of the intended user directory.
While os.path.join provides some protection, you should explicitly validate that the filename does not contain path traversal characters (/, \, ..) and that the final resolved path is within the expected base directory for that user.
|
|
||
| ```bash | ||
|
|
||
|   git clone https://github.com/KathiraveluLab/BHV.git |
There was a problem hiding this comment.
The use of   for indentation is unconventional and makes the raw Markdown source difficult to read. You can use regular spaces for indentation, and they will be rendered correctly within a fenced code block. This applies to all similar code blocks in this file.
|   git clone https://github.com/KathiraveluLab/BHV.git | |
| git clone https://github.com/KathiraveluLab/BHV.git |
| try: | ||
| yield db | ||
| finally: | ||
| db.close() No newline at end of file |
There was a problem hiding this comment.
| from fastapi import FastAPI | ||
| from pydantic import BaseModel | ||
| from app.database.db import Base, engine | ||
| from app.routes import auth, images, emotion ,admin |
| if os.path.exists(user_dir): | ||
| for f in os.listdir(user_dir): | ||
| os.remove(os.path.join(user_dir, f)) | ||
| os.rmdir(user_dir) |
There was a problem hiding this comment.
The logic for deleting user images can be simplified. Instead of manually listing files, removing them one by one, and then removing the directory, you can use shutil.rmtree to recursively delete the directory and all its contents. This is more concise and robust.
Remember to add import shutil at the top of the file.
if os.path.exists(user_dir):
shutil.rmtree(user_dir)| ext = file.filename.split(".")[-1] | ||
| filename = f"{file_id}.{ext}" |
There was a problem hiding this comment.
Getting the file extension by splitting on . and taking the last part is not robust. It can fail for filenames without an extension or handle filenames with multiple dots (e.g., archive.tar.gz) incorrectly.
A safer approach is to use os.path.splitext. Note that with splitext, you should handle cases where a file might not have an extension. The best practice is to derive the extension from the validated Content-Type of the upload, not the filename.
| ext = file.filename.split(".")[-1] | |
| filename = f"{file_id}.{ext}" | |
| _, ext = os.path.splitext(file.filename) | |
| filename = f"{file_id}{ext}" |
|
@gemini-code-assist I have made the changes accordingly |
|
Thank you for letting me know! I appreciate you addressing the feedback. If you'd like me to perform another review to check the changes, please use the |
Added Admin Panel:
GET /admin/users — View all users
PUT /admin/users/{id} — Update user details
DELETE /admin/users/{id} — Delete user + their images
GET /admin/stats — System statistics
Admin access is role-based — only users with is_admin=True can access these endpoints.