From b654bfae274481d3ad38b45528f1488246edfc0d Mon Sep 17 00:00:00 2001 From: Sebastian Becker Date: Tue, 19 Nov 2024 15:28:06 +0100 Subject: [PATCH] feat(service): undo authentication for use Signed-off-by: Sebastian Becker --- .../amphora/service/opa/OpaService.java | 14 --------- .../persistence/metadata/StorageService.java | 7 +---- .../service/rest/IntraVcpController.java | 10 ++----- .../amphora/service/opa/OpaServiceTest.java | 30 +++++-------------- .../metadata/StorageServiceTest.java | 26 ++++------------ .../service/rest/IntraVcpControllerTest.java | 9 ++---- 6 files changed, 19 insertions(+), 77 deletions(-) diff --git a/amphora-service/src/main/java/io/carbynestack/amphora/service/opa/OpaService.java b/amphora-service/src/main/java/io/carbynestack/amphora/service/opa/OpaService.java index d8c5618..2ee6770 100644 --- a/amphora-service/src/main/java/io/carbynestack/amphora/service/opa/OpaService.java +++ b/amphora-service/src/main/java/io/carbynestack/amphora/service/opa/OpaService.java @@ -20,7 +20,6 @@ public class OpaService { public static final String OWNER_TAG_KEY = "owner"; static final String READ_SECRET_ACTION_NAME = "read"; - static final String USE_SECRET_ACTION_NAME = "use"; static final String DELETE_SECRET_ACTION_NAME = "delete"; static final String CREATE_TAG_ACTION_NAME = "tag/create"; static final String READ_TAG_ACTION_NAME = "tag/read"; @@ -47,19 +46,6 @@ public boolean canReadSecret(String subject, List tags) throws CsOpaExcepti return isAllowed(subject, READ_SECRET_ACTION_NAME, tags); } - /** - * Check if the subject can use the secret described by the given tags evaluating the OPA policy package. - * The policy package is extracted from the tags if present. If not present, the default policy package is used. - * - * @param subject The subject attempting to use the secret. - * @param tags The tags describing the referenced secret. - * @return True if the subject can use the secret, false otherwise. - * @throws CsOpaException If an error occurred while evaluating the policy. - */ - public boolean canUseSecret(String subject, List tags) throws CsOpaException { - return isAllowed(subject, USE_SECRET_ACTION_NAME, tags); - } - /** * Check if the subject can delete the secret described by the given tags evaluating the OPA policy package. * The policy package is extracted from the tags if present. If not present, the default policy package is used. diff --git a/amphora-service/src/main/java/io/carbynestack/amphora/service/persistence/metadata/StorageService.java b/amphora-service/src/main/java/io/carbynestack/amphora/service/persistence/metadata/StorageService.java index 4997b2d..9c4ca5d 100644 --- a/amphora-service/src/main/java/io/carbynestack/amphora/service/persistence/metadata/StorageService.java +++ b/amphora-service/src/main/java/io/carbynestack/amphora/service/persistence/metadata/StorageService.java @@ -219,20 +219,15 @@ public Page getSecretList(List tagFilters, Sort sort) { * @return an {@link Option} containing the requested {@link SecretShare} of {@link Option#none()} * if no secret with the given id exists. * @throws AmphoraServiceException if an {@link SecretShare} exists but could not be retrieved. - * @throws NotFoundException if no {@link SecretShare} with the given id exists - * @throws UnauthorizedException if the requesting program is not authorized to access the {@link SecretShare} */ @Transactional(readOnly = true) - public SecretShare useSecretShare(UUID secretId, String programId) throws UnauthorizedException, CsOpaException { + public SecretShare useSecretShare(UUID secretId){ SecretEntity secretEntity = secretEntityRepository .findById(secretId.toString()) .orElseThrow( () -> new NotFoundException( String.format(NO_SECRET_WITH_ID_EXISTS_EXCEPTION_MSG, secretId))); - if (!opaService.canUseSecret(programId, setToTagList(secretEntity.getTags()))) { - throw new UnauthorizedException("Requesting program is not authorized to access the secret"); - } return getSecretShareForEntity(secretEntity); } diff --git a/amphora-service/src/main/java/io/carbynestack/amphora/service/rest/IntraVcpController.java b/amphora-service/src/main/java/io/carbynestack/amphora/service/rest/IntraVcpController.java index 7120826..b4318f6 100644 --- a/amphora-service/src/main/java/io/carbynestack/amphora/service/rest/IntraVcpController.java +++ b/amphora-service/src/main/java/io/carbynestack/amphora/service/rest/IntraVcpController.java @@ -65,18 +65,12 @@ public ResponseEntity uploadSecretShare(@RequestBody SecretShare secretShar * Retrieves an {@link SecretShare} with a given id. * * @param secretId id of the {@link SecretShare} to retrieve - * @param programId id of the Program requesting the {@link SecretShare} * @return {@link HttpStatus#OK} with the {@link SecretShare} if successful. * @throws AmphoraServiceException if an {@link SecretShare} exists but could not be retrieved. * @throws NotFoundException if no {@link SecretShare} with the given id exists - * @throws UnauthorizedException if the requesting Program is not authorized to access the {@link SecretShare} - * @throws CsOpaException if an error occurred while evaluating the OPA policy */ @GetMapping(path = "/{" + SECRET_ID_PARAMETER + "}") - public ResponseEntity downloadSecretShare(@PathVariable UUID secretId, - @RequestParam String programId) throws UnauthorizedException, CsOpaException { - - notNull(programId, "ProgramId must not be null"); - return new ResponseEntity<>(storageService.useSecretShare(secretId, programId), HttpStatus.OK); + public ResponseEntity downloadSecretShare(@PathVariable UUID secretId) { + return new ResponseEntity<>(storageService.useSecretShare(secretId), HttpStatus.OK); } } diff --git a/amphora-service/src/test/java/io/carbynestack/amphora/service/opa/OpaServiceTest.java b/amphora-service/src/test/java/io/carbynestack/amphora/service/opa/OpaServiceTest.java index 6dad392..26434b4 100644 --- a/amphora-service/src/test/java/io/carbynestack/amphora/service/opa/OpaServiceTest.java +++ b/amphora-service/src/test/java/io/carbynestack/amphora/service/opa/OpaServiceTest.java @@ -20,7 +20,6 @@ import java.util.List; import static io.carbynestack.amphora.service.opa.OpaService.READ_SECRET_ACTION_NAME; -import static io.carbynestack.amphora.service.opa.OpaService.USE_SECRET_ACTION_NAME; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; @@ -81,7 +80,7 @@ void givenPolicyDefinedInTag_whenIsAllowed_thenUsePolicyPackageProvided() throws } @Test - public void givenNoPolicyDefinedInTag_whenIsAllowed_thenUseDefaultPolicyPackage() throws CsOpaException { + void givenNoPolicyDefinedInTag_whenIsAllowed_thenUseDefaultPolicyPackage() throws CsOpaException { ArgumentCaptor packageCaptor = ArgumentCaptor.forClass(String.class); ArgumentCaptor> tagsCaptor = ArgumentCaptor.forClass(List.class); when(opaClientMock.isAllowed( @@ -99,7 +98,7 @@ public void givenNoPolicyDefinedInTag_whenIsAllowed_thenUseDefaultPolicyPackage( } @Test - public void whenCanReadSecret_thenUseProperAction() throws CsOpaException { + void whenCanReadSecret_thenUseProperAction() throws CsOpaException { ArgumentCaptor actionCaptor = ArgumentCaptor.forClass(String.class); when(opaClientMock.isAllowed( any(), actionCaptor.capture(), any(), any())) @@ -114,22 +113,7 @@ public void whenCanReadSecret_thenUseProperAction() throws CsOpaException { } @Test - public void whenCanUseSecret_thenUseProperAction() throws CsOpaException { - ArgumentCaptor actionCaptor = ArgumentCaptor.forClass(String.class); - when(opaClientMock.isAllowed( - any(), actionCaptor.capture(), any(), any())) - .thenReturn(true); - - OpaService service = new OpaService(opaClientMock); - service.canUseSecret(SUBJECT, TAGS); - - String actualAction = actionCaptor.getValue(); - assertEquals(USE_SECRET_ACTION_NAME, actualAction); - - } - - @Test - public void whenCanDeleteSecret_thenUseProperAction() throws CsOpaException { + void whenCanDeleteSecret_thenUseProperAction() throws CsOpaException { ArgumentCaptor actionCaptor = ArgumentCaptor.forClass(String.class); when(opaClientMock.isAllowed( any(), actionCaptor.capture(), any(), any())) @@ -143,7 +127,7 @@ public void whenCanDeleteSecret_thenUseProperAction() throws CsOpaException { } @Test - public void whenCanCreateTags_thenUseProperAction() throws CsOpaException { + void whenCanCreateTags_thenUseProperAction() throws CsOpaException { ArgumentCaptor actionCaptor = ArgumentCaptor.forClass(String.class); when(opaClientMock.isAllowed( any(), actionCaptor.capture(), any(), any())) @@ -157,7 +141,7 @@ public void whenCanCreateTags_thenUseProperAction() throws CsOpaException { } @Test - public void whenCanReadTags_thenUseProperAction() throws CsOpaException { + void whenCanReadTags_thenUseProperAction() throws CsOpaException { ArgumentCaptor actionCaptor = ArgumentCaptor.forClass(String.class); when(opaClientMock.isAllowed( any(), actionCaptor.capture(), any(), any())) @@ -171,7 +155,7 @@ public void whenCanReadTags_thenUseProperAction() throws CsOpaException { } @Test - public void whenCanUpdateTags_thenUseProperAction() throws CsOpaException { + void whenCanUpdateTags_thenUseProperAction() throws CsOpaException { ArgumentCaptor actionCaptor = ArgumentCaptor.forClass(String.class); when(opaClientMock.isAllowed( any(), actionCaptor.capture(), any(), any())) @@ -185,7 +169,7 @@ public void whenCanUpdateTags_thenUseProperAction() throws CsOpaException { } @Test - public void whenCanDeleteTags_thenUseProperAction() throws CsOpaException { + void whenCanDeleteTags_thenUseProperAction() throws CsOpaException { ArgumentCaptor actionCaptor = ArgumentCaptor.forClass(String.class); when(opaClientMock.isAllowed( any(), actionCaptor.capture(), any(), any())) diff --git a/amphora-service/src/test/java/io/carbynestack/amphora/service/persistence/metadata/StorageServiceTest.java b/amphora-service/src/test/java/io/carbynestack/amphora/service/persistence/metadata/StorageServiceTest.java index 66b7d4f..f11f20f 100644 --- a/amphora-service/src/test/java/io/carbynestack/amphora/service/persistence/metadata/StorageServiceTest.java +++ b/amphora-service/src/test/java/io/carbynestack/amphora/service/persistence/metadata/StorageServiceTest.java @@ -451,40 +451,28 @@ void givenNoSecretShareWithGivenIdInDatabase_whenUseSecretShare_thenThrowNotFoun when(secretEntityRepository.findById(testSecretId.toString())).thenReturn(Optional.empty()); NotFoundException nfe = - assertThrows(NotFoundException.class, () -> storageService.useSecretShare(testSecretId, authorizedSubject)); + assertThrows(NotFoundException.class, () -> storageService.useSecretShare(testSecretId)); assertEquals( String.format(NO_SECRET_WITH_ID_EXISTS_EXCEPTION_MSG, testSecretId), nfe.getMessage()); } @Test - void givenSubjectIsNotAuthorized_whenUseSecretShare_thenThrowUnauthorizedException() { - TagEntity tagEntity = TagEntity.fromTag(testTag); - SecretEntity secretEntity = new SecretEntity(testSecretId.toString(), singleton(tagEntity)); - when(secretEntityRepository.findById(testSecretId.toString())) - .thenReturn(Optional.of(secretEntity)); - - assertThrows(UnauthorizedException.class, () -> storageService.useSecretShare(testSecretId, authorizedSubject)); - } - - @Test - void givenDataCannotBeRetrieved_whenUseSecretShare_thenThrowAmphoraServiceException() throws CsOpaException { + void givenDataCannotBeRetrieved_whenUseSecretShare_thenThrowAmphoraServiceException() { AmphoraServiceException expectedAse = new AmphoraServiceException("Expected this one"); TagEntity tagEntity = TagEntity.fromTag(testTag); SecretEntity secretEntity = new SecretEntity(testSecretId.toString(), singleton(tagEntity)); when(secretEntityRepository.findById(testSecretId.toString())) .thenReturn(Optional.of(secretEntity)); - when(opaService.canUseSecret(authorizedSubject, setToTagList(secretEntity.getTags()))) - .thenReturn(true); when(secretShareDataStore.getSecretShareData(testSecretId)).thenThrow(expectedAse); assertThrows( AmphoraServiceException.class, - () -> storageService.useSecretShare(testSecretId, authorizedSubject), + () -> storageService.useSecretShare(testSecretId), expectedAse.getMessage()); } @Test - void givenSuccessfulRequest_whenUseSecretShare_thenReturnContent() throws CsOpaException, UnauthorizedException { + void givenSuccessfulRequest_whenUseSecretShare_thenReturnContent() { List expectedTags = singletonList(testTag); byte[] expectedData = RandomUtils.nextBytes(MpSpdzIntegrationUtils.SHARE_WIDTH); SecretEntity existingSecretEntity = @@ -492,14 +480,12 @@ void givenSuccessfulRequest_whenUseSecretShare_thenReturnContent() throws CsOpaE when(secretEntityRepository.findById(existingSecretEntity.getSecretId())) .thenReturn(Optional.of(existingSecretEntity)); - when(opaService.canUseSecret(authorizedSubject, setToTagList(existingSecretEntity.getTags()))) - .thenReturn(true); when(secretShareDataStore.getSecretShareData( UUID.fromString(existingSecretEntity.getSecretId()))) .thenReturn(expectedData); SecretShare actualSecretShare = - storageService.useSecretShare(UUID.fromString(existingSecretEntity.getSecretId()), authorizedSubject); + storageService.useSecretShare(UUID.fromString(existingSecretEntity.getSecretId())); assertEquals( UUID.fromString(existingSecretEntity.getSecretId()), actualSecretShare.getSecretId()); assertEquals(expectedTags, actualSecretShare.getTags()); @@ -514,7 +500,7 @@ void givenNoSecretShareWithGivenIdInDatabase_whenDeleteObject_thenThrowNotFoundE } @Test - void givenSubjectHasNoAccess_whenDeleteObject_thenThrowUnauthorizedException() throws CsOpaException { + void givenSubjectHasNoAccess_whenDeleteObject_thenThrowUnauthorizedException() { SecretEntity secretEntity = new SecretEntity(testSecretId.toString(), emptySet()); when(secretEntityRepository.findById(testSecretId.toString())) .thenReturn(Optional.of(secretEntity)); diff --git a/amphora-service/src/test/java/io/carbynestack/amphora/service/rest/IntraVcpControllerTest.java b/amphora-service/src/test/java/io/carbynestack/amphora/service/rest/IntraVcpControllerTest.java index 272fac8..82b5d2e 100644 --- a/amphora-service/src/test/java/io/carbynestack/amphora/service/rest/IntraVcpControllerTest.java +++ b/amphora-service/src/test/java/io/carbynestack/amphora/service/rest/IntraVcpControllerTest.java @@ -7,8 +7,6 @@ package io.carbynestack.amphora.service.rest; import io.carbynestack.amphora.common.SecretShare; -import io.carbynestack.amphora.service.exceptions.CsOpaException; -import io.carbynestack.amphora.service.exceptions.UnauthorizedException; import io.carbynestack.amphora.service.persistence.metadata.StorageService; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -29,7 +27,6 @@ @ExtendWith(MockitoExtension.class) class IntraVcpControllerTest { - private final String authorizedSubjectId ="afc0117f-c9cd-4d8c-acee-fa1433ca0fdd"; @Mock private StorageService storageService; @@ -63,14 +60,14 @@ void givenSuccessfulRequest_whenUploadSecretShare_thenReturnCreatedWithExpectedC } @Test - void givenSuccessfulRequest_whenDownloadSecretShare_thenReturnOkWithExpectedContent() throws CsOpaException, UnauthorizedException { + void givenSuccessfulRequest_whenDownloadSecretShare_thenReturnOkWithExpectedContent() { UUID secretShareId = UUID.fromString("3bcf8308-8f50-4d24-a37b-b0075bb5e779"); SecretShare expectedSecretShare = SecretShare.builder().secretId(secretShareId).build(); - when(storageService.useSecretShare(secretShareId, authorizedSubjectId)).thenReturn(expectedSecretShare); + when(storageService.useSecretShare(secretShareId)).thenReturn(expectedSecretShare); ResponseEntity actualResponse = - intraVcpController.downloadSecretShare(secretShareId, authorizedSubjectId); + intraVcpController.downloadSecretShare(secretShareId); assertEquals(HttpStatus.OK, actualResponse.getStatusCode()); assertEquals(expectedSecretShare, actualResponse.getBody());