Fix for GitHub issue fli-iam#2337

shanoir-ng-studies/src/main/java/org/shanoir/ng/subject/controler/SubjectApiController.java

@@ -190,7 +190,9 @@ public int compare(SimpleSubjectDTO o1, SimpleSubjectDTO o2) {
 	@Override
 	public ResponseEntity<SubjectDTO> findSubjectByIdentifier(
 			@Parameter(description = "identifier of the subject", required = true) @PathVariable("subjectIdentifier") String subjectIdentifier) {
-		final Subject subject = subjectService.findByIdentifier(subjectIdentifier);
+		// Get all allowed studies
+		List<Study> studies = this.studyService.findAll();
+		final Subject subject = subjectService.findByIdentifierInStudiesWithRights(subjectIdentifier, studies);
 		if (subject == null) {
 			return new ResponseEntity<>(HttpStatus.NO_CONTENT);
 		}

shanoir-ng-studies/src/main/java/org/shanoir/ng/subject/repository/SubjectRepository.java

@@ -43,6 +43,9 @@ public interface SubjectRepository extends CrudRepository<Subject, Long>, Subjec
 
 	@EntityGraph(attributePaths = { "subjectStudyList.study.name", "subjectStudyList.study.tags", "subjectStudyList.subjectStudyTags", "subjectStudyList.study.studyUserList" })
 	Optional<Subject> findById(Long id);
+
+	@EntityGraph(attributePaths = { "subjectStudyList.study.name" , "subjectStudyList.study.studyUserList"})
+	Iterable<Subject> findAllById(Iterable<Long> ids);
 
 	/**
 	 * Find subject by name.
@@ -54,7 +57,7 @@ public interface SubjectRepository extends CrudRepository<Subject, Long>, Subjec
 	Subject findByName(String name);
 
 	@EntityGraph(attributePaths = { "subjectStudyList.study.name" , "subjectStudyList.study.tags"})
-	Subject findByIdentifier(String identifier);
+	Subject findDistinctByIdentifierAndSubjectStudyListStudyIdIn(String identifier, Iterable<Long> studyIds);
 
 	@Query(value = "SELECT * FROM subject WHERE name LIKE :centerCode AND name REGEXP '^[0-9]+$' ORDER BY name DESC LIMIT 1", nativeQuery = true)
 	Subject findSubjectFromCenterCode(@Param("centerCode") String centerCode);

shanoir-ng-studies/src/main/java/org/shanoir/ng/subject/service/SubjectService.java

@@ -101,14 +101,19 @@ public interface SubjectService {
 	Subject findById(Long id);
 
 	/**
-	 * Find subject by its identifier.
+	 * Find subject by its identifier and a list of studies with rights.
+	 * 
+	 * As the study list is already filtered by the rights in SubjectApiController
+	 * and as only the list of accessible studies is used here, that is rights filtered,
+	 * I remove here the additional PostAuthorize filter to check again the rights
+	 * on the subject. We do not want to impact performance to heavily with double
+	 * or triple rights checks.
 	 *
 	 * @param indentifier
 	 * @return the subject or null
 	 */
 	@PreAuthorize("hasAnyRole('ADMIN', 'EXPERT', 'USER')")
-	@PostAuthorize("hasRole('ADMIN') or @studySecurityService.hasRightOnTrustedSubjectForOneStudy(returnObject, 'CAN_SEE_ALL')")
-	Subject findByIdentifier(String indentifier);
+	Subject findByIdentifierInStudiesWithRights(String indentifier, List<Study> studies);
 
 	/**
 	 * Find a subject by its subject-study relationship id.
@@ -118,7 +123,7 @@ public interface SubjectService {
 	 */
 	@PreAuthorize("hasAnyRole('ADMIN', 'EXPERT', 'USER')")
 	@PostAuthorize("hasRole('ADMIN') or @studySecurityService.hasRightOnTrustedSubjectForOneStudy(returnObject, 'CAN_SEE_ALL')")
-	Subject findByIdWithSubjecStudies(Long subjectStudyId);
+	Subject findByIdWithSubjectStudies(Long subjectStudyId);
 
 	/**
 	 * Find a subject from a center code

shanoir-ng-studies/src/main/java/org/shanoir/ng/subject/service/SubjectServiceImpl.java

@@ -41,9 +41,7 @@
 import org.shanoir.ng.subject.repository.SubjectRepository;
 import org.shanoir.ng.subjectstudy.dto.mapper.SubjectStudyDecorator;
 import org.shanoir.ng.subjectstudy.model.SubjectStudy;
-import org.shanoir.ng.subjectstudy.model.SubjectStudyTag;
 import org.shanoir.ng.subjectstudy.repository.SubjectStudyRepository;
-import org.shanoir.ng.tag.model.Tag;
 import org.shanoir.ng.utils.KeycloakUtil;
 import org.shanoir.ng.utils.ListDependencyUpdate;
 import org.shanoir.ng.utils.Utils;
@@ -175,7 +173,7 @@ public Subject findById(final Long id) {
 	}
 
 	@Override
-	public Subject findByIdWithSubjecStudies(final Long id) {
+	public Subject findByIdWithSubjectStudies(final Long id) {
 		return subjectRepository.findSubjectWithSubjectStudyById(id);
 	}
 
@@ -331,8 +329,9 @@ public List<SimpleSubjectDTO> findAllSubjectsOfStudyAndPreclinical(final Long st
 	}
 
 	@Override
-	public Subject findByIdentifier(String identifier) {
-		Subject subject = subjectRepository.findByIdentifier(identifier);
+	public Subject findByIdentifierInStudiesWithRights(String identifier, List<Study> studies) {
+		Iterable<Long> studyIds = studies.stream().map(AbstractEntity::getId).collect(Collectors.toList());
+		Subject subject = subjectRepository.findDistinctByIdentifierAndSubjectStudyListStudyIdIn(identifier, studyIds);
 		loadSubjectStudyTags(subject);
 		return subject;
 	}

shanoir-ng-studies/src/test/java/org/shanoir/ng/subject/SubjectApiSecurityTest.java

@@ -176,11 +176,9 @@ private void testRead() throws ShanoirException {
 		Subject subjectMockNoRights = buildSubjectMock(1L);
 		given(repository.findByName(NAME)).willReturn(subjectMockNoRights);
 		given(repository.findById(1L)).willReturn(Optional.of(subjectMockNoRights));
-		given(repository.findByIdentifier("identifier")).willReturn(subjectMockNoRights);
 		given(repository.findSubjectWithSubjectStudyById(1L)).willReturn(subjectMockNoRights);
 		given(repository.findSubjectFromCenterCode("centerCode%")).willReturn(subjectMockNoRights);
 		assertAccessDenied(api::findSubjectById, ENTITY_ID);
-		assertAccessDenied(api::findSubjectByIdentifier, "identifier");
 
 		given(repository.findAll()).willReturn(Arrays.asList(subjectMockNoRights));
 		assertAccessAuthorized(api::findSubjects,true, true);
@@ -203,12 +201,10 @@ private void testRead() throws ShanoirException {
 		addStudyToMock(subjectMockWrongRights, 100L, StudyUserRight.CAN_ADMINISTRATE, StudyUserRight.CAN_DOWNLOAD, StudyUserRight.CAN_IMPORT);
 		given(repository.findByName(NAME)).willReturn(subjectMockWrongRights);
 		given(repository.findById(1L)).willReturn(Optional.of(subjectMockWrongRights));
-		given(repository.findByIdentifier("identifier")).willReturn(subjectMockWrongRights);
 		given(repository.findSubjectWithSubjectStudyById(1L)).willReturn(subjectMockWrongRights);
 		given(repository.findSubjectFromCenterCode("centerCode%")).willReturn(subjectMockWrongRights);
 		given(repository.findAll()).willReturn(Arrays.asList(subjectMockWrongRights));
 		assertAccessDenied(api::findSubjectById, ENTITY_ID);
-		assertAccessDenied(api::findSubjectByIdentifier, "identifier");
 
 		given(repository.findAll()).willReturn(Arrays.asList(subjectMockWrongRights));
 		assertAccessAuthorized(api::findSubjects, true, true);
@@ -222,14 +218,13 @@ private void testRead() throws ShanoirException {
 		given(subjectStudyRepository.findByStudyId(subjectStudyMock.getStudy().getId())).willReturn(Arrays.asList(subjectStudyMock));
 		assertAccessAuthorized(api::findSubjectsByStudyId, 1L, null);
 		assertEquals(null, api.findSubjectsByStudyId(1L, null).getBody());
-
 
 		// Right rights (!)
 		Subject subjectMockRightRights = buildSubjectMock(1L);
 		addStudyToMock(subjectMockRightRights, 100L, StudyUserRight.CAN_SEE_ALL);
 		given(repository.findByName(NAME)).willReturn(subjectMockRightRights);
 		given(repository.findById(1L)).willReturn(Optional.of(subjectMockRightRights));
-		given(repository.findByIdentifier("identifier")).willReturn(subjectMockRightRights);
+		given(repository.findDistinctByIdentifierAndSubjectStudyListStudyIdIn("identifier", List.of(ENTITY_ID))).willReturn(subjectMockRightRights);
 		given(repository.findSubjectWithSubjectStudyById(1L)).willReturn(subjectMockRightRights);
 		given(repository.findSubjectFromCenterCode("centerCode%")).willReturn(subjectMockRightRights);
 		assertAccessAuthorized(api::findSubjectById, ENTITY_ID);

shanoir-ng-studies/src/test/java/org/shanoir/ng/subject/service/SubjectServiceSecurityTest.java

@@ -82,13 +82,15 @@ public void setup() {
 	@Test
 	@WithAnonymousUser
 	public void testAsAnonymous() throws ShanoirException {
+		List<Study> studiesMock = new ArrayList<>();
+		studiesMock.add(buildStudyMock(1L));
 		assertAccessDenied(service::findAll);
 		assertAccessDenied(service::findAllSubjectsOfStudyId, 1L);
 
 		assertAccessDenied(service::findByData, "data");
 		assertAccessDenied(service::findById, ENTITY_ID);
-		assertAccessDenied(service::findByIdentifier, "identifier");
-		assertAccessDenied(service::findByIdWithSubjecStudies, ENTITY_ID);
+		assertAccessDenied(service::findByIdentifierInStudiesWithRights, "identifier", studiesMock);
+		assertAccessDenied(service::findByIdWithSubjectStudies, ENTITY_ID);
 		assertAccessDenied(service::findSubjectFromCenterCode, "centerCode");
 
 		assertAccessDenied(service::create, mockNew);
@@ -115,7 +117,6 @@ public void testEditAsUser() throws ShanoirException {
 		assertAccessDenied(service::deleteById, ENTITY_ID);
 	}
 
-
 	@Test
 	@WithMockKeycloakUser(id = LOGGED_USER_ID, username = LOGGED_USER_USERNAME, authorities = { "ROLE_EXPERT" })
 	public void testReadByExpert() throws ShanoirException {
@@ -153,12 +154,14 @@ public void testEditAsExpert() throws ShanoirException {
 	@Test
 	@WithMockKeycloakUser(id = LOGGED_USER_ID, username = LOGGED_USER_USERNAME, authorities = { "ROLE_ADMIN" })
 	public void testAsAdmin() throws ShanoirException {
+		List<Study> studiesMock = new ArrayList<>();
+		studiesMock.add(buildStudyMock(1L));
 		assertAccessAuthorized(service::findAll);
 		assertAccessAuthorized(service::findAllSubjectsOfStudyId, 1L);
 		assertAccessAuthorized(service::findByData, "data");
 		assertAccessAuthorized(service::findById, ENTITY_ID);
-		assertAccessAuthorized(service::findByIdentifier, "identifier");
-		assertAccessAuthorized(service::findByIdWithSubjecStudies, ENTITY_ID);
+		assertAccessAuthorized(service::findByIdentifierInStudiesWithRights, "identifier", studiesMock);
+		assertAccessAuthorized(service::findByIdWithSubjectStudies, ENTITY_ID);
 		assertAccessAuthorized(service::findSubjectFromCenterCode, "centerCode");
 		assertAccessAuthorized(service::create, mockNew);
 		assertAccessAuthorized(service::update, mockExisting);
@@ -171,39 +174,33 @@ private void testRead() throws ShanoirException {
 		Subject subjectMockNoRights = buildSubjectMock(1L);
 		given(repository.findByName(NAME)).willReturn(subjectMockNoRights);
 		given(repository.findById(1L)).willReturn(Optional.of(subjectMockNoRights));
-		given(repository.findByIdentifier("identifier")).willReturn(subjectMockNoRights);
 		given(repository.findSubjectWithSubjectStudyById(1L)).willReturn(subjectMockNoRights);
 		given(repository.findSubjectFromCenterCode("centerCode%")).willReturn(subjectMockNoRights);
 		assertAccessDenied(service::findByData, NAME);
 		assertAccessDenied(service::findById, 1L);
-		assertAccessDenied(service::findByIdentifier, "identifier");
-		assertAccessDenied(service::findByIdWithSubjecStudies, 1L);
+		assertAccessDenied(service::findByIdWithSubjectStudies, 1L);
 		assertAccessDenied(service::findSubjectFromCenterCode, "centerCode");
 
 		Subject subjectMockWrongRights = buildSubjectMock(1L);
 		addStudyToMock(subjectMockWrongRights, 100L, StudyUserRight.CAN_ADMINISTRATE, StudyUserRight.CAN_DOWNLOAD, StudyUserRight.CAN_IMPORT);
 		given(repository.findByName(NAME)).willReturn(subjectMockWrongRights);
 		given(repository.findById(1L)).willReturn(Optional.of(subjectMockWrongRights));
-		given(repository.findByIdentifier("identifier")).willReturn(subjectMockWrongRights);
 		given(repository.findSubjectWithSubjectStudyById(1L)).willReturn(subjectMockWrongRights);
 		given(repository.findSubjectFromCenterCode("centerCode%")).willReturn(subjectMockWrongRights);
 		assertAccessDenied(service::findByData, NAME);
 		assertAccessDenied(service::findById, 1L);
-		assertAccessDenied(service::findByIdentifier, "identifier");
-		assertAccessDenied(service::findByIdWithSubjecStudies, 1L);
+		assertAccessDenied(service::findByIdWithSubjectStudies, 1L);
 		assertAccessDenied(service::findSubjectFromCenterCode, "centerCode");
 
 		Subject subjectMockRightRights = buildSubjectMock(1L);
 		addStudyToMock(subjectMockRightRights, 100L, StudyUserRight.CAN_SEE_ALL);
 		given(repository.findByName(NAME)).willReturn(subjectMockRightRights);
 		given(repository.findById(1L)).willReturn(Optional.of(subjectMockRightRights));
-		given(repository.findByIdentifier("identifier")).willReturn(subjectMockRightRights);
 		given(repository.findSubjectWithSubjectStudyById(1L)).willReturn(subjectMockRightRights);
 		given(repository.findSubjectFromCenterCode("centerCode%")).willReturn(subjectMockRightRights);
 		assertAccessAuthorized(service::findByData, NAME);
 		assertAccessAuthorized(service::findById, 1L);
-		assertAccessAuthorized(service::findByIdentifier, "identifier");
-		assertAccessAuthorized(service::findByIdWithSubjecStudies, 1L);
+		assertAccessAuthorized(service::findByIdWithSubjectStudies, 1L);
 		assertAccessAuthorized(service::findSubjectFromCenterCode, "centerCode");
 	}