Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,37 +14,41 @@
import share.fare.backend.dto.response.ActivityResponse;
import share.fare.backend.entity.User;
import share.fare.backend.service.ActivityService;
import share.fare.backend.service.SecurityService;
import share.fare.backend.util.PaginatedResponse;

@RestController
@RequestMapping("/api/v1/groups/{groupId}/activities")
@RequiredArgsConstructor
public class ActivityController {
private final ActivityService activityService;
private final SecurityService securityService;

@PostMapping
public ResponseEntity<ActivityResponse> createActivity(
@PathVariable Long groupId,
@Valid @RequestBody ActivityRequest request,
@AuthenticationPrincipal User user) {
ActivityResponse response = activityService.createActivity(request, user.getId(), groupId);
ActivityResponse response = activityService.createActivity(request, user, groupId);
return ResponseEntity.status(HttpStatus.CREATED).body(response);
}

@PutMapping("/{activityId}")
public ResponseEntity<ActivityResponse> updateActivity(
@PathVariable Long groupId,
@PathVariable Long activityId,
@Valid @RequestBody ActivityRequest request) {
ActivityResponse response = activityService.updateActivity(request, activityId);
@Valid @RequestBody ActivityRequest request,
@AuthenticationPrincipal User user) {
ActivityResponse response = activityService.updateActivity(request, activityId, user);
return ResponseEntity.ok(response);
}

@DeleteMapping("/{activityId}")
public ResponseEntity<Void> deleteActivity(
@PathVariable Long groupId,
@PathVariable Long activityId) {
activityService.deleteActivity(activityId);
@PathVariable Long activityId,
@AuthenticationPrincipal User user) {
activityService.deleteActivity(activityId, user);
return ResponseEntity.noContent().build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,15 @@ public ResponseEntity<PaginatedResponse<GroupResponse>> getAllGroups(
@PutMapping("/{groupId}")
public ResponseEntity<GroupResponse> updateGroup(
@PathVariable Long groupId,
@Valid @RequestBody GroupRequest groupRequest) {
return ResponseEntity.ok(groupService.updateGroup(groupId, groupRequest));
@Valid @RequestBody GroupRequest groupRequest,
@AuthenticationPrincipal User user) {
return ResponseEntity.ok(groupService.updateGroup(groupId, groupRequest, user));
}

@DeleteMapping("/{groupId}")
public ResponseEntity<Void> deleteGroup(@PathVariable Long groupId) {
groupService.deleteGroup(groupId);
public ResponseEntity<Void> deleteGroup(@PathVariable Long groupId,
@AuthenticationPrincipal User user) {
groupService.deleteGroup(groupId, user);
return ResponseEntity.noContent().build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,26 @@ public ResponseEntity<GroupMembershipResponse> addMember(
@RequestParam Long userId,
@RequestParam(defaultValue = "MEMBER") GroupRole role,
@AuthenticationPrincipal User currentUser) {
GroupMembershipResponse response = groupMembershipService.addMemberToGroup(groupId, userId, role, currentUser.getId());
GroupMembershipResponse response = groupMembershipService.addMemberToGroup(groupId, userId, role, currentUser);
return ResponseEntity.status(HttpStatus.CREATED).body(response);
}

@DeleteMapping("/{userId}")
public ResponseEntity<Void> removeMember(
@PathVariable Long groupId,
@PathVariable Long userId) {
groupMembershipService.removeMemberFromGroup(groupId, userId);
@PathVariable Long userId,
@AuthenticationPrincipal User currentUser) {
groupMembershipService.removeMemberFromGroup(groupId, userId, currentUser);
return ResponseEntity.noContent().build();
}

@PutMapping("/{userId}/role")
public ResponseEntity<GroupMembershipResponse> updateMemberRole(
@PathVariable Long groupId,
@PathVariable Long userId,
@RequestParam GroupRole role) {
GroupMembershipResponse response = groupMembershipService.updateMemberRole(groupId, userId, role);
@RequestParam GroupRole role,
@AuthenticationPrincipal User currentUser) {
GroupMembershipResponse response = groupMembershipService.updateMemberRole(groupId, userId, role, currentUser);
return ResponseEntity.ok(response);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,37 @@
import lombok.RequiredArgsConstructor;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageRequest;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.web.bind.annotation.*;
import share.fare.backend.entity.ChatMessage;
import share.fare.backend.entity.Group;
import share.fare.backend.entity.User;
import share.fare.backend.exception.GroupNotFoundException;
import share.fare.backend.repository.ChatMessageRepository;
import share.fare.backend.repository.GroupRepository;
import share.fare.backend.service.SecurityService;

@RestController
@RequestMapping("/api/v1/group")
@RequiredArgsConstructor
public class GroupChatController {

private final ChatMessageRepository chatMessageRepository;
private final GroupRepository groupRepository;
private final SecurityService securityService;

@GetMapping("/{groupId}/chat")
public Page<ChatMessage> getRecentMessages(
@PathVariable Long groupId,
@RequestParam(defaultValue = "0") int page,
@RequestParam(defaultValue = "20") int size
@RequestParam(defaultValue = "20") int size,
@AuthenticationPrincipal User user
) {
Group group = groupRepository.findById(groupId)
.orElseThrow(() -> new GroupNotFoundException(groupId));

securityService.checkIfUserIsInGroup(user, group);
Comment on lines +32 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The retrieval of the Group entity followed by a security check securityService.checkIfUserIsInGroup(user, group) is a good practice for ensuring the user has access to the group chat. This explicit check enhances security before accessing sensitive chat data.


return chatMessageRepository.findByGroupIdOrderByTimestampDesc(groupId, PageRequest.of(page, size));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ public final ResponseEntity<ErrorDetails> userIsNotInGroupException(Exception ex
return new ResponseEntity<>(errorDetails, HttpStatus.CONFLICT);
}

@ExceptionHandler(UserIsNotGroupOwner.class)
public final ResponseEntity<ErrorDetails> userIsNotInGroupOwnerException(Exception ex, WebRequest request) {
ErrorDetails errorDetails = new ErrorDetails(LocalDateTime.now(), ex.getMessage(), request.getDescription(false), ex.getStackTrace().length);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The ErrorDetails constructor is called with ex.getStackTrace().length as the argument for what appears to be an error count field. The length of a stack trace does not typically represent a count of errors. This pattern is present in the newly added userIsNotInGroupOwnerException handler. It's advisable to use a more semantically correct value for this field or clarify its purpose in ErrorDetails. For validation errors, ex.getBindingResult().getErrorCount() is used, which is appropriate.

return new ResponseEntity<>(errorDetails, HttpStatus.FORBIDDEN);
}

@ExceptionHandler(OwnerDeletionException.class)
public final ResponseEntity<ErrorDetails> ownerDeletionException(Exception ex, WebRequest request) {
ErrorDetails errorDetails = new ErrorDetails(LocalDateTime.now(), ex.getMessage(), request.getDescription(false), ex.getStackTrace().length);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package share.fare.backend.exception;

public class UserIsNotGroupOwner extends RuntimeException {
public UserIsNotGroupOwner(Long userId, Long groupId) {
super("User " + userId + " is not group owner of " + groupId);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,18 @@
import share.fare.backend.entity.User;
import share.fare.backend.exception.ActivityNotFoundException;
import share.fare.backend.exception.GroupNotFoundException;
import share.fare.backend.exception.UserNotFoundException;
import share.fare.backend.mapper.ActivityMapper;
import share.fare.backend.repository.ActivityRepository;
import share.fare.backend.repository.GroupRepository;
import share.fare.backend.repository.UserRepository;

@Service
@RequiredArgsConstructor
public class ActivityService {
private final ActivityRepository activityRepository;
private final GroupRepository groupRepository;
private final UserRepository userRepository;
private final SecurityService securityService;

public ActivityResponse createActivity(ActivityRequest activityRequest, Long createdByUserId, Long groupId) {
public ActivityResponse createActivity(ActivityRequest activityRequest, User currentUser, Long groupId) {
if (activityRequest.getStartDate() != null && activityRequest.getEndDate() != null) {
if (activityRequest.getStartDate().isAfter(activityRequest.getEndDate())) {
throw new IllegalArgumentException("Start date must be before end date.");
Expand All @@ -35,28 +33,33 @@ public ActivityResponse createActivity(ActivityRequest activityRequest, Long cre
.findById(groupId)
.orElseThrow(() -> new GroupNotFoundException(groupId));

User user = userRepository
.findById(createdByUserId)
.orElseThrow(() -> new UserNotFoundException(createdByUserId));
securityService.checkIfUserIsInGroup(currentUser, group);

Activity activity = ActivityMapper.toEntity(activityRequest, group);

return ActivityMapper.toResponse(activityRepository.save(activity));
}

public ActivityResponse updateActivity(ActivityRequest request, Long activityId) {
public ActivityResponse updateActivity(ActivityRequest request, Long activityId, User currentUser) {
Activity activity = activityRepository
.findById(activityId)
.orElseThrow(() -> new ActivityNotFoundException("Activity with ID: " + activityId + " not found"));

securityService.checkIfUserIsInGroup(currentUser, activity.getGroup());

activity.setName(request.getName());
activity.setDescription(request.getDescription());
activity.setLink(request.getLink());

return ActivityMapper.toResponse(activityRepository.save(activity));
}

public void deleteActivity(Long activityId) {
public void deleteActivity(Long activityId, User currentUser) {
Activity activity = activityRepository.findById(activityId)
.orElseThrow(() -> new ActivityNotFoundException("Activity with ID: " + activityId + " not found"));

securityService.checkIfUserIsInGroup(currentUser, activity.getGroup());
Comment on lines +58 to +61

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Fetching the Activity entity before deletion to perform securityService.checkIfUserIsInGroup(currentUser, activity.getGroup()) is a good security practice. This ensures that the authorization check is performed against the correct group associated with the activity being deleted.


activityRepository.deleteById(activityId);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,14 @@ public class GroupMembershipService {
private final GroupMembershipRepository groupMembershipRepository;
private final GroupRepository groupRepository;
private final UserRepository userRepository;
private final SecurityService securityService;

public GroupMembershipResponse addMemberToGroup(Long groupId, Long userId, GroupRole role, Long currentUserId) {
public GroupMembershipResponse addMemberToGroup(Long groupId, Long userId, GroupRole role, User currentUser) {
Group group = groupRepository.findById(groupId)
.orElseThrow(() -> new GroupNotFoundException(groupId));

securityService.checkIfUserIsInGroup(currentUser, group);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The authorization check securityService.checkIfUserIsInGroup(currentUser, group) ensures the currentUser is a member of the group before they can add another user. If the design intends for any group member to add new users, this is sufficient. However, if adding members should be restricted to specific roles (e.g., ADMIN, OWNER), this check might need to be augmented with a role-specific verification to ensure the currentUser has the necessary privileges.


User user = userRepository.findById(userId)
.orElseThrow(() -> new UserNotFoundException(userId));

Expand All @@ -47,7 +50,7 @@ public GroupMembershipResponse addMemberToGroup(Long groupId, Long userId, Group
return GroupMembershipMapper.toResponse(savedMembership);
}

public void removeMemberFromGroup(Long groupId, Long userId) {
public void removeMemberFromGroup(Long groupId, Long userId, User currentUser) {
Group group = groupRepository.findById(groupId)
.orElseThrow(() -> new GroupNotFoundException(groupId));

Expand All @@ -57,17 +60,21 @@ public void removeMemberFromGroup(Long groupId, Long userId) {
GroupMembership membership = groupMembershipRepository.findByGroupAndUser(group, user)
.orElseThrow(() -> new UserIsNotInGroupException("User is not a member of the group"));

securityService.checkIfUserCanRemoveMember(currentUser, user, group);

if (membership.getRole() == GroupRole.OWNER) {
throw new IllegalArgumentException("Owner cannot be removed from the group");
}

groupMembershipRepository.delete(membership);
}

public GroupMembershipResponse updateMemberRole(Long groupId, Long userId, GroupRole newRole) {
public GroupMembershipResponse updateMemberRole(Long groupId, Long userId, GroupRole newRole, User currentUser) {
Group group = groupRepository.findById(groupId)
.orElseThrow(() -> new GroupNotFoundException(groupId));

securityService.checkIfUserIsGroupOwner(currentUser, group);

User user = userRepository.findById(userId)
.orElseThrow(() -> new UserNotFoundException(userId));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
public class GroupService {
private final GroupRepository groupRepository;
private final UserRepository userRepository;
private final SecurityService securityService;

public GroupResponse createGroup(GroupRequest groupRequest, Long createdByUserId) {
validateTripDates(groupRequest.getTripStartDate(), groupRequest.getTripEndDate());
Expand Down Expand Up @@ -50,19 +51,21 @@ public Page<GroupResponse> getGroupsForUser(Long userId, Pageable pageable) {
return groups.map(GroupMapper::toResponse);
}

public void deleteGroup(Long groupId) {
if (!groupRepository.existsById(groupId)) {
throw new GroupNotFoundException(groupId);
}
public void deleteGroup(Long groupId, User currentUser) {
Group group = groupRepository.findById(groupId)
.orElseThrow(() -> new GroupNotFoundException(groupId));
securityService.checkIfUserIsGroupOwner(currentUser, group);
groupRepository.deleteById(groupId);
}

public GroupResponse updateGroup(Long groupId, GroupRequest groupRequest) {
public GroupResponse updateGroup(Long groupId, GroupRequest groupRequest, User currentUser) {
validateTripDates(groupRequest.getTripStartDate(), groupRequest.getTripEndDate());

Group existingGroup = groupRepository.findById(groupId)
.orElseThrow(() -> new GroupNotFoundException(groupId));

securityService.checkIfUserIsGroupOwner(currentUser, existingGroup);

updateGroupFields(existingGroup, groupRequest);

Group updatedGroup = groupRepository.save(existingGroup);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package share.fare.backend.service;

import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import share.fare.backend.entity.Group;
import share.fare.backend.entity.GroupMembership;
import share.fare.backend.entity.GroupRole;
import share.fare.backend.entity.User;
import share.fare.backend.exception.UserIsNotGroupOwner;
import share.fare.backend.exception.UserIsNotInGroupException;
import share.fare.backend.repository.GroupMembershipRepository;

@Service
@RequiredArgsConstructor
public class SecurityService {
private final GroupMembershipRepository membershipRepository;

public void checkIfUserIsInGroup(User user, Group group) {
findMembershipOrThrow(user, group);
}

public void checkIfUserIsGroupOwner(User user, Group group) {
GroupMembership membership = findMembershipOrThrow(user, group);

if (membership.getRole() != GroupRole.OWNER) {
throw new UserIsNotGroupOwner(user.getId(), group.getId());
}
}

public void checkIfUserCanRemoveMember(User currentUser, User userToRemove, Group group) {
GroupMembership membership = findMembershipOrThrow(currentUser, group);
if (membership.getRole() != GroupRole.OWNER && !currentUser.getId().equals(userToRemove.getId())) {
throw new UserIsNotGroupOwner(currentUser.getId(), group.getId());
}
}
Comment on lines +30 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic in checkIfUserCanRemoveMember correctly allows a group owner to remove members and also allows any member to remove themselves from the group. This is a common and sensible policy. The use of findMembershipOrThrow for the currentUser ensures they are part of the group before attempting role checks or self-removal logic.


private GroupMembership findMembershipOrThrow(User user, Group group) {
return membershipRepository.findByGroupAndUser(group, user)
.orElseThrow(() ->
new UserIsNotInGroupException("User " + user.getId() + " is not in group " + group.getName()));
}
}
Loading
Loading