Skip to content

Commit 9e6a64d

Browse files
CelineLahnorCelineLahnor
committed
resolved commets
1 parent b6601f4 commit 9e6a64d

10 files changed

Lines changed: 222 additions & 7 deletions

File tree

‎src/main/java/de/tum/cit/aet/core/repository/ImageRepository.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,16 @@ default List<DepartmentImage> findDefaultJobBannersBySchool(UUID schoolId) {
9999
@Query("SELECT pi FROM ProfileImage pi WHERE pi.uploadedBy.userId = :userId")
100100
List<ProfileImage> findProfileImagesByUserId(@Param("userId") UUID userId);
101101

102+
/**
103+
* Checks whether the given user owns a persisted profile image with the provided URL.
104+
*
105+
* @param userId the owner of the profile image
106+
* @param url the exact profile image URL
107+
* @return {@code true} when the URL belongs to a stored profile image of that user
108+
*/
109+
@Query("SELECT COUNT(pi) > 0 FROM ProfileImage pi WHERE pi.uploadedBy.userId = :userId AND pi.url = :url")
110+
boolean existsProfileImageByUserIdAndUrl(@Param("userId") UUID userId, @Param("url") String url);
111+
102112
/**
103113
* Updates all {@link Image} records uploaded by the given {@code user} to associate them with the
104114
* provided {@code deletedUser} instead of the original user.

‎src/main/java/de/tum/cit/aet/core/service/ImageService.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,12 @@
77
import de.tum.cit.aet.core.domain.ResearchGroupImage;
88
import de.tum.cit.aet.core.dto.ImageDTO;
99
import de.tum.cit.aet.core.exception.AccessDeniedException;
10+
import de.tum.cit.aet.core.exception.BadRequestException;
1011
import de.tum.cit.aet.core.exception.EntityNotFoundException;
1112
import de.tum.cit.aet.core.exception.InternalServerException;
1213
import de.tum.cit.aet.core.exception.UploadException;
1314
import de.tum.cit.aet.core.repository.ImageRepository;
15+
import de.tum.cit.aet.core.util.StringUtil;
1416
import de.tum.cit.aet.job.repository.JobRepository;
1517
import de.tum.cit.aet.usermanagement.domain.Department;
1618
import de.tum.cit.aet.usermanagement.domain.ResearchGroup;
@@ -192,6 +194,34 @@ public void deleteProfilePicturesByUserId(UUID userId) {
192194
deleteProfilePicturesForUserId(userId);
193195
}
194196

197+
/**
198+
* Ensures the provided avatar URL references a persisted profile picture owned by the given user.
199+
*
200+
* @param userId the owner that must match the stored profile picture
201+
* @param avatarUrl the avatar URL to validate
202+
* @throws BadRequestException when the URL is not a stored profile image of the user
203+
*/
204+
public void assertUserOwnsProfilePictureUrl(UUID userId, String avatarUrl) {
205+
String normalizedAvatarUrl = StringUtil.normalize(avatarUrl, false);
206+
if (normalizedAvatarUrl == null || normalizedAvatarUrl.isBlank() || !normalizedAvatarUrl.startsWith("/images/profiles/")) {
207+
throw new BadRequestException("Avatar URL must reference an existing profile picture owned by the current user");
208+
}
209+
210+
if (!imageRepository.existsProfileImageByUserIdAndUrl(userId, normalizedAvatarUrl)) {
211+
throw new BadRequestException("Avatar URL must reference an existing profile picture owned by the current user");
212+
}
213+
}
214+
215+
/**
216+
* Ensures the provided avatar URL references a persisted profile picture owned by the current user.
217+
*
218+
* @param avatarUrl the avatar URL to validate
219+
* @throws BadRequestException when the URL is not a stored profile image of the current user
220+
*/
221+
public void assertCurrentUserOwnsProfilePictureUrl(String avatarUrl) {
222+
assertUserOwnsProfilePictureUrl(currentUserService.getUserId(), avatarUrl);
223+
}
224+
195225
/**
196226
* Sets common properties for all image types.
197227
* Extracted to avoid code duplication across upload methods.

‎src/main/java/de/tum/cit/aet/usermanagement/service/UserService.java‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import de.tum.cit.aet.core.dto.PageDTO;
44
import de.tum.cit.aet.core.dto.PageResponseDTO;
55
import de.tum.cit.aet.core.exception.EntityNotFoundException;
6+
import de.tum.cit.aet.core.service.ImageService;
67
import de.tum.cit.aet.core.util.StringUtil;
78
import de.tum.cit.aet.usermanagement.constants.UserRole;
89
import de.tum.cit.aet.usermanagement.domain.User;
@@ -30,11 +31,17 @@ public class UserService {
3031

3132
private final UserRepository userRepository;
3233
private final UserResearchGroupRoleRepository userResearchGroupRoleRepository;
34+
private final ImageService imageService;
3335
private static final Duration LAST_ACTIVITY_UPDATE_THRESHOLD = Duration.ofHours(24);
3436

35-
public UserService(UserRepository userRepository, UserResearchGroupRoleRepository userResearchGroupRoleRepository) {
37+
public UserService(
38+
UserRepository userRepository,
39+
UserResearchGroupRoleRepository userResearchGroupRoleRepository,
40+
ImageService imageService
41+
) {
3642
this.userRepository = userRepository;
3743
this.userResearchGroupRoleRepository = userResearchGroupRoleRepository;
44+
this.imageService = imageService;
3845
}
3946

4047
/**
@@ -137,7 +144,12 @@ public void updateNames(String userId, String firstName, String lastName) {
137144
public void updateAvatar(String userId, String avatarUrl) {
138145
User user = userRepository.findById(UUID.fromString(userId)).orElseThrow(() -> EntityNotFoundException.forId("User", userId));
139146
String normalizedAvatarUrl = StringUtil.normalize(avatarUrl, false);
140-
user.setAvatar(normalizedAvatarUrl != null && !normalizedAvatarUrl.isBlank() ? normalizedAvatarUrl : null);
147+
if (normalizedAvatarUrl == null || normalizedAvatarUrl.isBlank()) {
148+
user.setAvatar(null);
149+
} else {
150+
imageService.assertUserOwnsProfilePictureUrl(user.getUserId(), normalizedAvatarUrl);
151+
user.setAvatar(normalizedAvatarUrl);
152+
}
141153
userRepository.save(user);
142154
}
143155

‎src/main/java/de/tum/cit/aet/usermanagement/web/UserResource.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ public ResponseEntity<Void> updatePassword(@AuthenticationPrincipal Jwt jwt, @Va
8484

8585
/**
8686
* Allows the currently authenticated user to update their avatar URL.
87+
* Non-empty values must point to a stored profile picture owned by the current user.
8788
*
8889
* @param jwt of the authenticated user
8990
* @param dto contains the new avatar URL (or null/blank to remove)
@@ -96,6 +97,7 @@ public ResponseEntity<Void> updateAvatar(@AuthenticationPrincipal Jwt jwt, @Requ
9697
if (normalizedAvatarUrl == null || normalizedAvatarUrl.isBlank()) {
9798
imageService.deleteCurrentUserProfilePicture();
9899
} else {
100+
imageService.assertCurrentUserOwnsProfilePictureUrl(normalizedAvatarUrl);
99101
userService.updateAvatar(jwt.getSubject(), normalizedAvatarUrl);
100102
}
101103
return ResponseEntity.noContent().build();

‎src/main/webapp/app/job/my-positions/my-positions-page.component.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ <h1>
9696
<div class="flex items-center gap-2">
9797
<jhi-user-avatar [fullName]="job.professorName ?? ''" [avatarUrl]="job.avatar" size="lg" />
9898
<span class="truncate">
99-
{{ job.professorName ? (job.professorName.startsWith('Prof.') ? job.professorName : 'Prof. ' + job.professorName) : '-' }}
99+
{{ job.professorName ? (job.professorName.startsWith('Prof.') ? job.professorName : job.professorName) : '-' }}
100100
</span>
101101
</div>
102102
</ng-template>

‎src/main/webapp/app/shared/components/atoms/user-avatar/user-avatar.component.html‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,13 @@
99
<span
1010
class="inline-flex shrink-0 items-center justify-center rounded-full border-[0.5px] border-solid border-transparent font-semibold uppercase leading-none select-none"
1111
[class]="sizeClass()"
12+
role="img"
1213
[attr.aria-label]="ariaLabel()"
1314
[style.background-color]="backgroundColor()"
1415
[style.color]="textColor()"
1516
[style.border-color]="borderColor()"
1617
[style.text-shadow]="textShadow()"
1718
>
18-
{{ initials() }}
19+
<span aria-hidden="true">{{ initials() }}</span>
1920
</span>
2021
}

‎src/main/webapp/app/usermanagement/research-group/research-group-add-members/research-group-add-members.component.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,8 +229,10 @@ export class ResearchGroupAddMembersComponent {
229229
} finally {
230230
// only touch loading/timeout if this is the latest request
231231
if (requestId === this.latestRequestId) {
232-
clearTimeout(this.loaderTimeout);
233-
this.loaderTimeout = null;
232+
if (this.loaderTimeout !== null) {
233+
clearTimeout(this.loaderTimeout);
234+
this.loaderTimeout = null;
235+
}
234236
this.loading.set(false);
235237
}
236238
}

‎src/test/java/de/tum/cit/aet/core/service/ImageServiceTest.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import de.tum.cit.aet.core.domain.ProfileImage;
1010
import de.tum.cit.aet.core.domain.ResearchGroupImage;
1111
import de.tum.cit.aet.core.exception.AccessDeniedException;
12+
import de.tum.cit.aet.core.exception.BadRequestException;
1213
import de.tum.cit.aet.core.exception.EntityNotFoundException;
1314
import de.tum.cit.aet.core.exception.UploadException;
1415
import de.tum.cit.aet.core.repository.ImageRepository;
@@ -294,6 +295,41 @@ void shouldDeleteCurrentUsersProfilePictureAndClearAvatar() throws IOException {
294295
}
295296
}
296297

298+
@Nested
299+
class AssertCurrentUserOwnsProfilePictureUrl {
300+
301+
@Test
302+
void shouldAcceptStoredProfilePictureOwnedByCurrentUser() {
303+
String avatarUrl = "/images/profiles/avatar.jpg";
304+
when(currentUserService.getUserId()).thenReturn(TEST_USER_ID);
305+
when(imageRepository.existsProfileImageByUserIdAndUrl(TEST_USER_ID, avatarUrl)).thenReturn(true);
306+
307+
assertThatCode(() -> imageService.assertCurrentUserOwnsProfilePictureUrl(avatarUrl)).doesNotThrowAnyException();
308+
309+
verify(imageRepository).existsProfileImageByUserIdAndUrl(TEST_USER_ID, avatarUrl);
310+
}
311+
312+
@Test
313+
void shouldRejectNonProfileImageUrls() {
314+
assertThatThrownBy(() -> imageService.assertCurrentUserOwnsProfilePictureUrl("https://example.com/avatar.png"))
315+
.isInstanceOf(BadRequestException.class)
316+
.hasMessage("Avatar URL must reference an existing profile picture owned by the current user");
317+
318+
verify(imageRepository, never()).existsProfileImageByUserIdAndUrl(any(UUID.class), anyString());
319+
}
320+
321+
@Test
322+
void shouldRejectProfileImageUrlsNotOwnedByCurrentUser() {
323+
String avatarUrl = "/images/profiles/other-user.jpg";
324+
when(currentUserService.getUserId()).thenReturn(TEST_USER_ID);
325+
when(imageRepository.existsProfileImageByUserIdAndUrl(TEST_USER_ID, avatarUrl)).thenReturn(false);
326+
327+
assertThatThrownBy(() -> imageService.assertCurrentUserOwnsProfilePictureUrl(avatarUrl))
328+
.isInstanceOf(BadRequestException.class)
329+
.hasMessage("Avatar URL must reference an existing profile picture owned by the current user");
330+
}
331+
}
332+
297333
@Nested
298334
class GetDefaultJobBanners {
299335

‎src/test/java/de/tum/cit/aet/usermanagement/service/UserServiceTest.java‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
package de.tum.cit.aet.usermanagement.service;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
5+
import static org.mockito.Mockito.doThrow;
6+
import static org.mockito.Mockito.never;
7+
import static org.mockito.Mockito.verify;
8+
import static org.mockito.Mockito.when;
9+
10+
import de.tum.cit.aet.core.exception.BadRequestException;
11+
import de.tum.cit.aet.core.service.ImageService;
12+
import de.tum.cit.aet.usermanagement.domain.User;
13+
import de.tum.cit.aet.usermanagement.repository.UserRepository;
14+
import de.tum.cit.aet.usermanagement.repository.UserResearchGroupRoleRepository;
15+
import de.tum.cit.aet.utility.testdata.UserTestData;
16+
import java.util.Optional;
17+
import java.util.UUID;
18+
import org.junit.jupiter.api.BeforeEach;
19+
import org.junit.jupiter.api.Nested;
20+
import org.junit.jupiter.api.Test;
21+
import org.junit.jupiter.api.extension.ExtendWith;
22+
import org.mockito.InjectMocks;
23+
import org.mockito.Mock;
24+
import org.mockito.junit.jupiter.MockitoExtension;
25+
26+
@ExtendWith(MockitoExtension.class)
27+
class UserServiceTest {
28+
29+
private static final UUID TEST_USER_ID = UUID.randomUUID();
30+
31+
@Mock
32+
private UserRepository userRepository;
33+
34+
@Mock
35+
private UserResearchGroupRoleRepository userResearchGroupRoleRepository;
36+
37+
@Mock
38+
private ImageService imageService;
39+
40+
@InjectMocks
41+
private UserService userService;
42+
43+
private User testUser;
44+
45+
@BeforeEach
46+
void setUp() {
47+
testUser = UserTestData.newUserAll(TEST_USER_ID, "test@example.com", "Test", "User");
48+
when(userRepository.findById(TEST_USER_ID)).thenReturn(Optional.of(testUser));
49+
}
50+
51+
@Nested
52+
class UpdateAvatar {
53+
54+
@Test
55+
void shouldClearAvatarWithoutOwnershipCheckWhenAvatarUrlIsBlank() {
56+
testUser.setAvatar("/images/profiles/existing.jpg");
57+
58+
userService.updateAvatar(TEST_USER_ID.toString(), " ");
59+
60+
assertThat(testUser.getAvatar()).isNull();
61+
verify(imageService, never()).assertUserOwnsProfilePictureUrl(TEST_USER_ID, " ");
62+
verify(userRepository).save(testUser);
63+
}
64+
65+
@Test
66+
void shouldNormalizeAndValidateAvatarUrlBeforePersisting() {
67+
userService.updateAvatar(TEST_USER_ID.toString(), " /images/profiles/avatar.jpg ");
68+
69+
assertThat(testUser.getAvatar()).isEqualTo("/images/profiles/avatar.jpg");
70+
verify(imageService).assertUserOwnsProfilePictureUrl(TEST_USER_ID, "/images/profiles/avatar.jpg");
71+
verify(userRepository).save(testUser);
72+
}
73+
74+
@Test
75+
void shouldRejectUnsafeAvatarUrlWithoutPersistingIt() {
76+
doThrow(new BadRequestException("Avatar URL must reference an existing profile picture owned by the current user"))
77+
.when(imageService)
78+
.assertUserOwnsProfilePictureUrl(TEST_USER_ID, "https://example.com/tracker.png");
79+
80+
assertThatThrownBy(() -> userService.updateAvatar(TEST_USER_ID.toString(), "https://example.com/tracker.png"))
81+
.isInstanceOf(BadRequestException.class)
82+
.hasMessage("Avatar URL must reference an existing profile picture owned by the current user");
83+
84+
assertThat(testUser.getAvatar()).isNull();
85+
verify(userRepository, never()).save(testUser);
86+
}
87+
}
88+
}

‎src/test/java/de/tum/cit/aet/usermanagement/web/rest/UserResourceTest.java‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@
66
import static org.mockito.Mockito.when;
77

88
import de.tum.cit.aet.AbstractResourceTest;
9+
import de.tum.cit.aet.core.domain.ProfileImage;
10+
import de.tum.cit.aet.core.dto.ApiError;
11+
import de.tum.cit.aet.core.repository.ImageRepository;
912
import de.tum.cit.aet.core.service.AuthenticationService;
1013
import de.tum.cit.aet.core.service.ImageService;
1114
import de.tum.cit.aet.usermanagement.domain.User;
@@ -19,6 +22,7 @@
1922
import de.tum.cit.aet.utility.DatabaseCleaner;
2023
import de.tum.cit.aet.utility.MvcTestClient;
2124
import de.tum.cit.aet.utility.security.JwtPostProcessors;
25+
import de.tum.cit.aet.utility.testdata.ImageTestData;
2226
import de.tum.cit.aet.utility.testdata.UserTestData;
2327
import java.util.Map;
2428
import org.junit.jupiter.api.BeforeEach;
@@ -45,6 +49,9 @@ public class UserResourceTest extends AbstractResourceTest {
4549
@Autowired
4650
UserRepository userRepository;
4751

52+
@Autowired
53+
ImageRepository imageRepository;
54+
4855
@Autowired
4956
MvcTestClient api;
5057

@@ -206,13 +213,40 @@ void returnsNoContentAndDeletesProfilePictureWhenAvatarUrlIsBlank() {
206213

207214
@Test
208215
void returnsNoContentAndUpdatesAvatarWhenAvatarUrlIsPresent() {
209-
UpdateAvatarDTO dto = new UpdateAvatarDTO("/images/profiles/avatar.jpg");
216+
ProfileImage profileImage = imageRepository.save(ImageTestData.newProfilePicture(currentUser));
217+
UpdateAvatarDTO dto = new UpdateAvatarDTO(profileImage.getUrl());
210218

211219
api
212220
.with(JwtPostProcessors.jwtUser(currentUser.getUserId(), "ROLE_APPLICANT"))
213221
.putAndRead(API_BASE_PATH + "/avatar", dto, Void.class, 204);
214222

215223
verify(userService).updateAvatar(currentUser.getUserId().toString(), dto.avatarUrl());
216224
}
225+
226+
@Test
227+
void returns400WhenAvatarUrlIsExternal() {
228+
UpdateAvatarDTO dto = new UpdateAvatarDTO("https://example.com/tracker.png");
229+
230+
ApiError error = api
231+
.with(JwtPostProcessors.jwtUser(currentUser.getUserId(), "ROLE_APPLICANT"))
232+
.putAndRead(API_BASE_PATH + "/avatar", dto, ApiError.class, 400);
233+
234+
assertThat(error.message()).isEqualTo("Avatar URL must reference an existing profile picture owned by the current user");
235+
assertThat(userRepository.findById(currentUser.getUserId()).orElseThrow().getAvatar()).isNull();
236+
}
237+
238+
@Test
239+
void returns400WhenAvatarUrlReferencesAnotherUsersProfilePicture() {
240+
User otherUser = UserTestData.createUserWithoutResearchGroup(userRepository, "other.user@tum.de", "Other", "User", "xy12zzz");
241+
ProfileImage otherUsersProfileImage = imageRepository.save(ImageTestData.newProfilePicture(otherUser));
242+
UpdateAvatarDTO dto = new UpdateAvatarDTO(otherUsersProfileImage.getUrl());
243+
244+
ApiError error = api
245+
.with(JwtPostProcessors.jwtUser(currentUser.getUserId(), "ROLE_APPLICANT"))
246+
.putAndRead(API_BASE_PATH + "/avatar", dto, ApiError.class, 400);
247+
248+
assertThat(error.message()).isEqualTo("Avatar URL must reference an existing profile picture owned by the current user");
249+
assertThat(userRepository.findById(currentUser.getUserId()).orElseThrow().getAvatar()).isNull();
250+
}
217251
}
218252
}

0 commit comments

Comments
 (0)