Skip to content

fix: added authorization for admins to remove resources from negotiation #1237

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.

Sign up for GitHub

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

Open
ChrisiSailer wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: added authorization for admins to remove resources from negotiation #1237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions backend/src/main/java/eu/bbmri_eric/negotiator/negotiation/NegotiationServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,10 @@ public void removeResourceFromNegotiation(String negotiationId, Long resourceId)
}

private void verifyRemoveResourcePreconditions(String negotiationId, Negotiation negotiation) {
if (!isNegotiationCreator(negotiationId)) {
if (!isNegotiationCreator(negotiationId)

@stetsche stetsche Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We might need to queue the PR, but would be nice to use isNegotiationEditor here as well. 1) to have it defined in one place and 2) so collaborators can remove resources.

&& !AuthenticatedUserContext.isCurrentlyAuthenticatedUserAdmin()) {
throw new ForbiddenRequestException(
"Only the negotiation author can remove resources from a draft negotiation");
"Only the negotiation author and admins can remove resources from a draft negotiation");
Comment thread
ChrisiSailer marked this conversation as resolved.
}
if (negotiation.getCurrentState() != NegotiationState.DRAFT) {
throw new IllegalStateException(
Expand Down
49 changes: 49 additions & 0 deletions backend/src/test/java/eu/bbmri_eric/negotiator/negotiation/NegotiationControllerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1635,6 +1635,34 @@
"Resource should be removed from the negotiation");
}

@Test
@WithUserDetails("admin")
@Transactional
void removeResource_draftStatus_multipleResources_adminCanRemoveResource() throws Exception {

Check warning on line 1641 in backend/src/test/java/eu/bbmri_eric/negotiator/negotiation/NegotiationControllerTests.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Update this method so that its implementation is not identical to "removeResource_draftStatus_multipleResources_userCanRemoveResource" on line 1613. 


See more on https://sonarcloud.io/project/issues?id=BBMRI-ERIC_negotiator&issues=AZ9vHKZjUZUOIdR7jvXB&open=AZ9vHKZjUZUOIdR7jvXB&pullRequest=1237
// negotiation-5 belongs to TheResearcher and has 2 resources (ids 5 and 7)
Negotiation negotiation = negotiationRepository.findById(NEGOTIATION_5_ID).get();
negotiation.setCurrentState(NegotiationState.DRAFT);
negotiationRepository.saveAndFlush(negotiation);

int initialResourceCount = negotiation.getResources().size();
Assertions.assertTrue(initialResourceCount > 1, "Negotiation should have more than 1 resource");

Long resourceIdToRemove = negotiation.getResources().iterator().next().getId();

mockMvc
.perform(
MockMvcRequestBuilders.delete(
"%s/%s/resources/%s"
.formatted(NEGOTIATIONS_URL, NEGOTIATION_5_ID, resourceIdToRemove)))
.andExpect(status().isNoContent());

Negotiation updatedNegotiation = negotiationRepository.findById(NEGOTIATION_5_ID).get();
assertEquals(
initialResourceCount - 1,
updatedNegotiation.getResources().size(),
"Resource should be removed from the negotiation");
}

@Test
@WithUserDetails("TheResearcher")
@Transactional
Expand All @@ -1656,6 +1684,27 @@
.andExpect(status().isBadRequest());
}

@Test
@WithUserDetails("admin")
@Transactional
void removeResource_draftStatus_onlyOneResourceLeft_asAdmin_throwsBadRequest() throws Exception {

Check warning on line 1690 in backend/src/test/java/eu/bbmri_eric/negotiator/negotiation/NegotiationControllerTests.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Update this method so that its implementation is not identical to "removeResource_draftStatus_onlyOneResourceLeft_throwsBadRequest" on line 1669. 


See more on https://sonarcloud.io/project/issues?id=BBMRI-ERIC_negotiator&issues=AZ9vHKZjUZUOIdR7jvXC&open=AZ9vHKZjUZUOIdR7jvXC&pullRequest=1237
Negotiation negotiation = negotiationRepository.findById(NEGOTIATION_5_ID).get();
negotiation.setCurrentState(NegotiationState.DRAFT);
negotiationRepository.saveAndFlush(negotiation);
Long resourceIdToRemove = negotiation.getResources().iterator().next().getId();
mockMvc.perform(
MockMvcRequestBuilders.delete(
"%s/%s/resources/%s"
.formatted(NEGOTIATIONS_URL, NEGOTIATION_5_ID, resourceIdToRemove)));
resourceIdToRemove = negotiation.getResources().iterator().next().getId();
mockMvc
.perform(
MockMvcRequestBuilders.delete(
"%s/%s/resources/%s"
.formatted(NEGOTIATIONS_URL, NEGOTIATION_5_ID, resourceIdToRemove)))
.andExpect(status().isBadRequest());
}

@Test
@WithUserDetails("admin")
void getLifecycleEvents() throws Exception {
Expand Down
Loading