Skip to content

Commit

Permalink
gchqgh-2476 Stop defaulting access roles to empty arrays. Read and Wr…
Browse files Browse the repository at this point in the history 
…ite access role properties are mutually exclusive with read and write AccessPredicate properties.
  • Loading branch information
@m29827
m29827 committed Jul 14, 2021
1 parent 1fb6823 commit 3109a71
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 34 deletions.
10 changes: 0 additions & 10 deletions ...test/java/uk/gov/gchq/gaffer/integration/operation/named/cache/NamedOperationCacheIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,6 @@ private void shouldBeAbleToAddNamedOperationToCache() throws OperationException
.operationName(add.getOperationName())
.operationChain(add.getOperationChainAsString())
.creatorId(user.getUserId())
.readers(new ArrayList<>())
.writers(new ArrayList<>())
.description(add.getDescription())
.score(0)
.parameters(null)
Expand Down Expand Up @@ -197,8 +195,6 @@ private void shouldAllowUpdatingOfNamedOperations() throws OperationException {
.operationChain(update.getOperationChainAsString())
.description(update.getDescription())
.creatorId(user.getUserId())
.readers(new ArrayList<>())
.writers(new ArrayList<>())
.score(0)
.parameters(null)
.build();
Expand Down Expand Up @@ -237,8 +233,6 @@ private void shouldAllowUpdatingOfNamedOperationsWithAllowedUsers() throws Opera
.operationChain(update.getOperationChainAsString())
.description(update.getDescription())
.creatorId(user.getUserId())
.readers(new ArrayList<>())
.writers(new ArrayList<>())
.score(0)
.parameters(null)
.build();
Expand All @@ -259,8 +253,6 @@ private void shouldAllowReadingOfNamedOperationsUsingAdminAuth() throws Operatio
.operationChain(add.getOperationChainAsString())
.description(add.getDescription())
.creatorId(authorisedUser.getUserId())
.readers(new ArrayList<>())
.writers(new ArrayList<>())
.score(0)
.parameters(null)
.build();
Expand Down Expand Up @@ -301,8 +293,6 @@ private void shouldAllowUpdatingOfNamedOperationsUsingAdminAuth() throws Operati
.operationChain(update.getOperationChainAsString())
.description(update.getDescription())
.creatorId(adminAuthUser.getUserId())
.readers(new ArrayList<>())
.writers(new ArrayList<>())
.score(0)
.parameters(null)
.build();
Expand Down
27 changes: 20 additions & 7 deletions core/operation/src/main/java/uk/gov/gchq/gaffer/named/operation/AddNamedOperation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.io.UnsupportedEncodingException;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
Expand All @@ -51,7 +52,7 @@
* A {@code AddNamedOperation} is an {@link Operation} for creating a new {@link NamedOperation}
* and adding it to a Gaffer graph.
*/
@JsonPropertyOrder(value = {"class", "operationName", "description", "score", "operations"}, alphabetic = true)
@JsonPropertyOrder(value = {"class", "operationName", "description", "score", "labels", "operationChain", "input", "overwriteFlag", "parameters", "readAccessRoles", "writeAccessRoles", "readAccessPredicate", "writeAccessPredicate"}, alphabetic = true)
@Since("1.0.0")
@Summary("Adds a new named operation")
public class AddNamedOperation implements Operation, Operations<Operation> {
Expand All @@ -61,8 +62,8 @@ public class AddNamedOperation implements Operation, Operations<Operation> {
private String operationName;
private List<String> labels;
private String description;
private List<String> readAccessRoles = new ArrayList<>();
private List<String> writeAccessRoles = new ArrayList<>();
private List<String> readAccessRoles;
private List<String> writeAccessRoles;
private boolean overwriteFlag = false;
private Map<String, ParameterDetail> parameters;
private Map<String, String> options;
Expand Down Expand Up @@ -173,8 +174,8 @@ public AddNamedOperation shallowClone() {
.name(operationName)
.labels(labels)
.description(description)
.readAccessRoles(readAccessRoles.toArray(new String[readAccessRoles.size()]))
.writeAccessRoles(writeAccessRoles.toArray(new String[writeAccessRoles.size()]))
.readAccessRoles(readAccessRoles == null ? null : readAccessRoles.toArray(new String[readAccessRoles.size()]))
.writeAccessRoles(writeAccessRoles == null ? null : writeAccessRoles.toArray(new String[writeAccessRoles.size()]))
.overwrite(overwriteFlag)
.parameters(parameters)
.options(options)
Expand Down Expand Up @@ -301,12 +302,24 @@ public Builder description(final String description) {
}

public Builder readAccessRoles(final String... roles) {
Collections.addAll(_getOp().getReadAccessRoles(), roles);
if (null == roles) {
_getOp().setReadAccessRoles(null);
} else if (null == _getOp().getReadAccessRoles()) {
_getOp().setReadAccessRoles(Arrays.asList(roles));
} else {
Collections.addAll(_getOp().getReadAccessRoles(), roles);
}
return _self();
}

public Builder writeAccessRoles(final String... roles) {
Collections.addAll(_getOp().getWriteAccessRoles(), roles);
if (null == roles) {
_getOp().setWriteAccessRoles(null);
} else if (null == _getOp().getWriteAccessRoles()) {
_getOp().setWriteAccessRoles(Arrays.asList(roles));
} else {
Collections.addAll(_getOp().getWriteAccessRoles(), roles);
}
return _self();
}

Expand Down
55 changes: 43 additions & 12 deletions core/operation/src/test/java/uk/gov/gchq/gaffer/named/operation/AddNamedOperationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.Mockito.mock;

Expand All @@ -55,6 +56,7 @@ public class AddNamedOperationTest extends OperationTest<AddNamedOperation> {
private static final AccessPredicate READ_ACCESS_PREDICATE = new AccessPredicate(new CustomUserPredicate());
private static final AccessPredicate WRITE_ACCESS_PREDICATE = new AccessPredicate(new CustomUserPredicate());

@Test
@Override
public void shouldJsonSerialiseAndDeserialise() {
//Given
Expand Down Expand Up @@ -94,20 +96,16 @@ public void shouldJsonSerialiseAndDeserialise() {
" \"readAccessRoles\" : [ \"User\" ],%n" +
" \"writeAccessRoles\" : [ \"User\" ],%n" +
" \"readAccessPredicate\" : {%n" +
" \"class\" : \"uk.gov.gchq.gaffer.access.predicate.CustomAccessPredicate\",%n" +
" \"userId\" : \"CreatingUserId\",%n" +
" \"map\" : {%n" +
" \"ReadKey\": \"ReadValue\"%n" +
" },%n" +
" \"auths\" : [ \"CustomReadAuth1\", \"CustomReadAuth2\" ]%n" +
" \"class\" : \"uk.gov.gchq.gaffer.access.predicate.AccessPredicate\",%n" +
" \"userPredicate\" : {%n" +
" \"class\" : \"uk.gov.gchq.gaffer.access.predicate.user.CustomUserPredicate\"%n" +
" }%n" +
"},%n" +
"\"writeAccessPredicate\" : {%n" +
" \"class\" : \"uk.gov.gchq.gaffer.access.predicate.CustomAccessPredicate\",%n" +
" \"userId\" : \"CreatingUserId\",%n" +
" \"map\" : {%n" +
" \"WriteKey\": \"WriteValue\"%n" +
" },%n" +
" \"auths\" : [ \"CustomWriteAuth1\", \"CustomWriteAuth2\" ]%n" +
" \"class\" : \"uk.gov.gchq.gaffer.access.predicate.AccessPredicate\",%n" +
" \"userPredicate\" : {%n" +
" \"class\" : \"uk.gov.gchq.gaffer.access.predicate.user.CustomUserPredicate\"%n" +
" }%n" +
"}%n" +
"}"), new String(json));
assertNotNull(deserialisedObj);
Expand Down Expand Up @@ -152,6 +150,7 @@ public void shouldJsonSerialiseAndDeserialiseWithNoOptions() {
assertNotNull(deserialisedObj);
}

@Test
@Override
public void builderShouldCreatePopulatedOperation() {
AddNamedOperation addNamedOperation = new AddNamedOperation.Builder()
Expand Down Expand Up @@ -181,6 +180,7 @@ public void builderShouldCreatePopulatedOperation() {
assertEquals(WRITE_ACCESS_PREDICATE, addNamedOperation.getWriteAccessPredicate());
}

@Test
@Override
public void shouldShallowCloneOperation() {
// Given
Expand Down Expand Up @@ -227,6 +227,23 @@ public void shouldShallowCloneOperation() {
assertEquals(WRITE_ACCESS_PREDICATE, clone.getWriteAccessPredicate());
}

@Test
public void shouldShallowCloneOperationWithNoAccessRoles() {
// Given
AddNamedOperation addNamedOperation = new AddNamedOperation.Builder()
.operationChain(OPERATION_CHAIN)
.description("Test Named Operation")
.name("Test")
.build();

// When
AddNamedOperation clone = addNamedOperation.shallowClone();

// Then
assertNull(clone.getReadAccessRoles());
assertNull(clone.getWriteAccessRoles());
}

@Test
public void shouldGetOperationsWithDefaultParameters() {
// Given
Expand Down Expand Up @@ -301,4 +318,18 @@ protected AddNamedOperation getTestObject() {
protected Set<String> getRequiredFields() {
return Sets.newHashSet("operations");
}

@Test
public void shouldNotDefaultAnyAccessControlConfiguration() {
final AddNamedOperation addNamedOperation = new AddNamedOperation.Builder()
.operationChain("{\"operations\":[{\"class\": \"uk.gov.gchq.gaffer.operation.impl.get.GetAdjacentIds\", \"input\": [{\"vertex\": \"${testParameter}\", \"class\": \"uk.gov.gchq.gaffer.operation.data.EntitySeed\"}]}]}")
.description("Test Named Operation")
.name("Test")
.build();

assertNull(addNamedOperation.getReadAccessRoles());
assertNull(addNamedOperation.getWriteAccessRoles());
assertNull(addNamedOperation.getReadAccessPredicate());
assertNull(addNamedOperation.getWriteAccessPredicate());
}
}
5 changes: 0 additions & 5 deletions ...va/uk/gov/gchq/gaffer/store/operation/handler/named/GetAllNamedOperationsHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import uk.gov.gchq.gaffer.store.operation.handler.named.cache.NamedOperationCache;
import uk.gov.gchq.gaffer.user.User;

import java.util.ArrayList;
import java.util.Arrays;

import static org.junit.jupiter.api.Assertions.assertEquals;
Expand All @@ -53,8 +52,6 @@ public class GetAllNamedOperationsHandlerTest {
.inputType("uk.gov.gchq.gaffer.data.element.Element[]")
.creatorId(User.UNKNOWN_USER_ID)
.operationChain("{\"operations\":[{\"class\":\"uk.gov.gchq.gaffer.operation.impl.add.AddElements\",\"skipInvalidElements\":false,\"validate\":true}]}")
.readers(new ArrayList<>())
.writers(new ArrayList<>())
.parameters(null)
.build();

Expand All @@ -63,8 +60,6 @@ public class GetAllNamedOperationsHandlerTest {
.inputType(null)
.creatorId(User.UNKNOWN_USER_ID)
.operationChain("{\"operations\":[{\"class\":\"uk.gov.gchq.gaffer.store.operation.GetSchema\",\"compact\":false}]}")
.readers(new ArrayList<>())
.writers(new ArrayList<>())
.parameters(null)
.build();

Expand Down

0 comments on commit 3109a71

Please sign in to comment.