Skip to content
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

Fix access control for modifying filters and contingency lists #117

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AAJELLAL
Copy link
Contributor

No description provided.

AAJELLAL and others added 3 commits January 17, 2025 11:35
…to handle_access_rights

# Conflicts:
#	src/main/java/org/gridsuite/explore/server/services/ExploreService.java
@souissimai souissimai self-requested a review January 22, 2025 13:18
} catch (HttpStatusCodeException e) {
handleException(e);
}
if (response != null && HttpStatus.NO_CONTENT.equals(response.getStatusCode())) {
Copy link
Contributor

@souissimai souissimai Jan 22, 2025

Choose a reason for hiding this comment

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

response
!= null is already true

headers.add(HEADER_USER_ID, userId);

String path = UriComponentsBuilder.fromPath(ELEMENTS_SERVER_ROOT_PATH)
.queryParam(isUpdate ? PARAM_FOR_UPDATE : PARAM_FOR_DELETION, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

use one param for both (deletion and update)

if (HttpStatus.NO_CONTENT.equals(response.getStatusCode())) {
throw new ExploreException(NOT_ALLOWED);
}
checkPermission(elementUuids, userId, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

areElementsAccessible instead checkPermission

@souissimai
Copy link
Contributor

souissimai commented Jan 22, 2025

TEST OK, small remarks

@@ -350,6 +354,8 @@ public void notifyCasesThresholdReached(int userCasesCount, int userMaxAllowedSt
}

public void updateElement(UUID id, ElementAttributes elementAttributes, String userId) {
// check if the user have the right to update the element
directoryService.areDirectoryElementsUpdatable(List.of(id), userId);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's already done in the directory-server when calling updateElement and you don't have another service called than the directory-server here. Maybe keep a comment here but do not check multiple times I think ?

Copy link
Contributor

@sBouzols sBouzols left a comment

Choose a reason for hiding this comment

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

I think you should manage updateCompositeModification, updateParameters, updateSpreadsheetConfig, replaceFilterWithScript and replaceFormContingencyListWithScript as well with your code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants