-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue#301 + Issue#300 #305
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename this file server/src/main/resources/db/migration/V2__add_userPhoto_to_users.sql:
V1.0.011_add_userPhoto_to_users.sql
Not quiet ready to move our application to V2 yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I love what you have done. I know there is a lot to review but I don't want that to discourage you.
Coming into the project there is no possible way for you to know all the details like UserContext to get the id from user's request.
public ResponseEntity<?> uploadProfilePicture(@RequestParam("file") MultipartFile file, | ||
@RequestParam("userId") int userId) { | ||
// File size validation | ||
if (file.getSize() > MAX_FILE_SIZE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do the size check in the method parameter, also helps keep our end point from getting dsod with big files.
@@ -154,4 +168,85 @@ public ResponseEntity<String> refreshToken( | |||
return ResponseEntity.ok().header(HttpHeaders.SET_COOKIE, cookie.toString()).body(token); | |||
}).orElseThrow(() -> new TokenRefreshException(jwt, "Refresh token is not in database!")); | |||
} | |||
|
|||
@PostMapping("/profile-picture") | |||
public ResponseEntity<?> uploadProfilePicture(@RequestParam("file") MultipartFile file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After at @RequestParm
use the size annotation @Size(max = MAX_FILE_SIZE)
|
||
@PostMapping("/profile-picture") | ||
public ResponseEntity<?> uploadProfilePicture(@RequestParam("file") MultipartFile file, | ||
@RequestParam("userId") int userId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this because it makes it so we can not validate the user by their jwt credentials.
I would move all the internal logic from 186 - 213 into the userService.
Ideally the controller logic should mostly be handling errors if any occur, the parameters and different types of parameters then letting the Service level handle the doing.
try { | ||
User user; | ||
try { | ||
user = userService.getUserById(userId).orElseThrow(() -> new NoUserFoundException()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clear up a little bit above:
Add this to the userManagement service:
https://github.com/R-Sandor/FindFirst/blob/75ada43b5d5aa7c6ea3c808519e361e45280e5fe/server/src/main/java/dev/findfirst/core/service/BookmarkService.java#L61C1-L61C38
UserContext userContext
that allows us to get the user from the request to server rather than telling the server who you are, which could lead to me putting my photo on your profile since there is not any validation.
@@ -60,6 +70,10 @@ public class UserController { | |||
@Value("${findfirst.app.domain}") | |||
private String domain; | |||
|
|||
private static final long MAX_FILE_SIZE = 2 * 1024 * 1024; // 2 MB | |||
private static final String[] ALLOWED_TYPES = {"image/jpeg", "image/png"}; | |||
private static final String UPLOAD_DIR = "uploads/profile-pictures/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind using an application properties for that @Value
@Value("${findfirst.screenshot.uploads.profile-pictures}")
private String uploadProfileLoc;
Or something similar.
For example:
findfirst.local.upload.profile-pictures=../data/uploads/profile-pictures/
findfirst.upload.location=${FINDFIRST_UPLOAD_LOCATION:${findfirst.local.upload.profile-pictures}}
Then in the docker files do something like I have for the screenshot location:
environment:
- FINDFIRST_UPLOAD_LOCATION=/app/profile-pictures/
volumes:
- ./data/uploads/profile-pictures:/app/profile-pictures
Issue number: resolves #300 and #301
/users/profile-picture
to upload profile pictures.JPG
andPNG
)./users/profile-picture
to retrieve profile pictures.