Skip to content

Commit

Permalink
Pass content-IDs of all ContentKey elements to Checks (#6859)
Browse files Browse the repository at this point in the history
Introduces a new type `IdentifiedContentKey` associating a content-ID to each content-key element, and also holds the content-type.

The change became bigger than originally expected, because:
* the return type of `ContentApi.getContent(s)` had to be changed
* the `Diff` type had to be changed to provide the "from" _and_ "to" keys as `IdentifiedContentKey`s

The authz check tests had to be changed as well, because with this change, the checks need the `IdentifiedContentKey`, which is only available _after_ storage interactions. The existing test case has been split into three test cases.

New vs old storage model: the new storage model returns _all_ content IDs in `IdentifiedContentKey` - so for the content and all its namespace elements. The old storage model only returns the content ID for the content itself but not for the namespace elements.
  • Loading branch information
snazy authored May 25, 2023
1 parent 1969298 commit 49d9266
Show file tree
Hide file tree
Showing 51 changed files with 1,185 additions and 558 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* Copyright (C) 2023 Dremio
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.projectnessie.model;

import static org.projectnessie.model.IdentifiedContentKey.IdentifiedElement.identifiedElement;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Function;
import javax.annotation.Nullable;
import javax.validation.constraints.NotNull;
import javax.validation.constraints.Size;
import org.immutables.value.Value;

@Value.Immutable
@JsonSerialize(as = ImmutableIdentifiedContentKey.class)
@JsonDeserialize(as = ImmutableIdentifiedContentKey.class)
public interface IdentifiedContentKey {
@NotNull
@jakarta.validation.constraints.NotNull
@Size
@jakarta.validation.constraints.Size(min = 1)
List<IdentifiedElement> elements();

@NotNull
@jakarta.validation.constraints.NotNull
ContentKey contentKey();

@JsonIgnore
@Value.Redacted
default IdentifiedElement lastElement() {
List<IdentifiedElement> el = elements();
return el.get(el.size() - 1);
}

@Nullable
@jakarta.annotation.Nullable
Content.Type type();

@Value.Check
default void check() {
if (contentKey().getElementCount() != elements().size()) {
throw new IllegalArgumentException(
"Number of content-key elements must match the identified-elements");
}
}

@Value.Immutable
@JsonSerialize(as = ImmutableIdentifiedElement.class)
@JsonDeserialize(as = ImmutableIdentifiedElement.class)
interface IdentifiedElement {
@Value.Parameter(order = 1)
String element();

@Value.Parameter(order = 2)
@Nullable
@jakarta.annotation.Nullable
String contentId();

static IdentifiedElement identifiedElement(String element, String contentId) {
return ImmutableIdentifiedElement.of(element, contentId);
}
}

static ImmutableIdentifiedContentKey.Builder builder() {
return ImmutableIdentifiedContentKey.builder();
}

static IdentifiedContentKey identifiedContentKeyFromContent(
ContentKey key, Content content, Function<List<String>, String> keyToContentId) {
return identifiedContentKeyFromContent(key, content.getType(), content.getId(), keyToContentId);
}

static IdentifiedContentKey identifiedContentKeyFromContent(
ContentKey key,
Content.Type type,
String contentId,
Function<List<String>, String> keyToContentId) {
ImmutableIdentifiedContentKey.Builder identifiedKey =
IdentifiedContentKey.builder().contentKey(key).type(type);
List<String> path = new ArrayList<>();
List<String> keyElements = key.getElements();
int elementCount = keyElements.size();
for (int i = 0; i < elementCount - 1; i++) {
String element = keyElements.get(i);
path.add(element);
String id = keyToContentId.apply(path);
identifiedKey.addElements(identifiedElement(element, id));
}
identifiedKey.addElements(identifiedElement(keyElements.get(elementCount - 1), contentId));
return identifiedKey.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ private void checkValues(Persist persist, String ref, ContentKey key, Content va
ReferenceInfo<CommitMeta> main = store.getNamedRef(ref, GetNamedRefsParams.DEFAULT);
soft.assertThat(store.getKeys(main.getHash(), null, true, null, null, null, null))
.toIterable()
.extracting(KeyEntry::getKey, KeyEntry::getContent)
.extracting(e -> e.getKey().contentKey(), KeyEntry::getContent)
.containsExactly(tuple(key, value));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Map;
import java.util.function.Supplier;
import org.projectnessie.cel.tools.ScriptException;
import org.projectnessie.model.Content;
import org.projectnessie.services.authz.AbstractBatchAccessChecker;
import org.projectnessie.services.authz.AccessContext;
import org.projectnessie.services.authz.BatchAccessChecker;
Expand Down Expand Up @@ -72,10 +73,11 @@ private void canPerformOp(Check check, Map<Check, String> failed) {

private void canPerformOpOnReference(Check check, Map<Check, String> failed) {
String roleName = getRoleName();
String refName = check.ref().getName();
ImmutableMap<String, Object> arguments =
ImmutableMap.of(
"ref",
check.ref().getName(),
refName,
"role",
roleName,
"op",
Expand All @@ -89,30 +91,32 @@ private void canPerformOpOnReference(Check check, Map<Check, String> failed) {
() ->
String.format(
"'%s' is not allowed for role '%s' on reference '%s'",
check.type(), roleName, check.ref().getName());
check.type(), roleName, refName);
canPerformOp(arguments, check, errorMsgSupplier, failed);
}

private void canPerformOpOnPath(Check check, Map<Check, String> failed) {
String roleName = getRoleName();
Content.Type contentType = check.contentType();
String contentKeyPathString = check.key().toPathString();
ImmutableMap<String, Object> arguments =
ImmutableMap.of(
"ref",
check.ref().getName(),
"path",
check.key().toPathString(),
contentKeyPathString,
"role",
roleName,
"op",
check.type().name(),
"contentType",
check.contentType() != null ? check.contentType().name() : "");
contentType != null ? contentType.name() : "");

Supplier<String> errorMsgSupplier =
() ->
String.format(
"'%s' is not allowed for role '%s' on content '%s'",
check.type(), roleName, check.key().toPathString());
check.type(), roleName, contentKeyPathString);

canPerformOp(arguments, check, errorMsgSupplier, failed);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ void testCommitDisallowed() {
api()
.commitMultipleOperations()
.branchName("main")
.hash("11223344556677")
.hash("2e1cfa82b035c26cbbbdae632cea070514eb8b773f616aaeaf668e2f0be8f10d")
.operation(Delete.of(ContentKey.of("testKey")))
.commitMeta(CommitMeta.fromMessage("test commit"))
::commit)
Expand Down Expand Up @@ -223,38 +223,44 @@ void testRefLogDisallowed() {
}

@Test
// test_user2 has all permissions on a Branch, but not permissions on a Key
// test_user2 has all permissions on a Branch, but no permissions to READ_ENTITY_VALUE
@TestSecurity(user = "test_user2")
void testCanCommitButNotUpdateOrDeleteEntity() throws BaseNessieClientServerException {
void testCanCommitButNotReadEntity() throws BaseNessieClientServerException {
String role = "test_user2";
ContentKey key = ContentKey.of("allowed-some");
String branchName = "allowedBranchForTestUser2";
createBranch(Branch.of(branchName, null));

final Branch branch = retrieveBranch(branchName);
Branch branch = createBranch(Branch.of(branchName, null));

assertThatThrownBy(
() ->
api()
.commitMultipleOperations()
.branch(branch)
.commitMeta(CommitMeta.fromMessage("add stuff"))
.operation(Put.of(key, IcebergTable.of("foo", 42, 42, 42, 42, "cid-foo")))
.commit())
.isInstanceOf(NessieForbiddenException.class)
.hasMessageContaining(
String.format(
"'UPDATE_ENTITY' is not allowed for role '%s' on content '%s'",
role, key.toPathString()));
api()
.commitMultipleOperations()
.branch(branch)
.commitMeta(CommitMeta.fromMessage("add stuff"))
.operation(Put.of(key, IcebergTable.of("foo", 42, 42, 42, 42)))
.commit();

assertThatThrownBy(() -> api().getContent().refName(branchName).key(key).get())
.isInstanceOf(NessieForbiddenException.class)
.hasMessageContaining(
String.format(
"'READ_ENTITY_VALUE' is not allowed for role '%s' on content '%s'",
role, key.toPathString()));
}

final Branch b = retrieveBranch(branchName);
@Test
// test_user3 has all permissions on a Branch, but not permissions to DELETE_ENTITY
@TestSecurity(user = "test_user3")
void testCanCommitButNotDeleteEntity() throws BaseNessieClientServerException {
String role = "test_user3";
ContentKey key = ContentKey.of("allowed-some");
Branch branch = createBranch(Branch.of("allowedBranchForTestUser3", null));

Branch b =
api()
.commitMultipleOperations()
.branch(branch)
.commitMeta(CommitMeta.fromMessage("add stuff"))
.operation(Put.of(key, IcebergTable.of("foo", 42, 42, 42, 42)))
.commit();

assertThatThrownBy(
() ->
Expand All @@ -271,16 +277,37 @@ void testCanCommitButNotUpdateOrDeleteEntity() throws BaseNessieClientServerExce
role, key.toPathString()));
}

@Test
// test_user3 has all permissions on a Branch, but not permissions to DELETE_ENTITY
@TestSecurity(user = "test_user4")
void testCanCommitButNotUpdateEntity() throws BaseNessieClientServerException {
String role = "test_user4";
ContentKey key = ContentKey.of("allowed-some");
Branch branch = createBranch(Branch.of("allowedBranchForTestUser4", null));

assertThatThrownBy(
() ->
api()
.commitMultipleOperations()
.branch(branch)
.commitMeta(CommitMeta.fromMessage("add stuff"))
.operation(Put.of(key, IcebergTable.of("foo", 42, 42, 42, 42)))
.commit())
.isInstanceOf(NessieForbiddenException.class)
.hasMessageContaining(
String.format(
"'UPDATE_ENTITY' is not allowed for role '%s' on content '%s'",
role, key.toPathString()));
}

@Test
@TestSecurity(user = "admin_user")
void testCanReadTargetBranchDuringAssign() throws BaseNessieClientServerException {
String branchName = "adminCanReadWhenAssigning";
String targetBranchName = "targetBranchForAssign";
createBranch(Branch.of(branchName, null));
Branch branch = retrieveBranch(branchName);
Branch branch = createBranch(Branch.of(branchName, null));

createBranch(Branch.of(targetBranchName, null));
Branch targetBranch = retrieveBranch(targetBranchName);
Branch targetBranch = createBranch(Branch.of(targetBranchName, null));

addContent(
targetBranch, Put.of(ContentKey.of("allowed-x"), IcebergTable.of("foo", 42, 42, 42, 42)));
Expand All @@ -302,11 +329,9 @@ void testCanReadTargetBranchDuringMerge() throws BaseNessieClientServerException

String branchName = "adminCanReadWhenMerging";
String targetBranchName = "targetBranchForMerge";
createBranch(Branch.of(branchName, main.getHash()));
Branch branch = retrieveBranch(branchName);
Branch branch = createBranch(Branch.of(branchName, main.getHash()));

createBranch(Branch.of(targetBranchName, main.getHash()));
Branch targetBranch = retrieveBranch(targetBranchName);
Branch targetBranch = createBranch(Branch.of(targetBranchName, main.getHash()));

addContent(branch, Put.of(ContentKey.of("allowed-x"), IcebergTable.of("foo", 42, 42, 42, 42)));
branch = retrieveBranch(branchName);
Expand All @@ -323,11 +348,9 @@ void testCanReadTargetBranchDuringMerge() throws BaseNessieClientServerException
void testCanReadTargetBranchDuringTransplant() throws BaseNessieClientServerException {
String branchName = "adminCanReadWhenTransplanting";
String targetBranchName = "targetBranchForTransplant";
createBranch(Branch.of(branchName, null));
Branch branch = retrieveBranch(branchName);
Branch branch = createBranch(Branch.of(branchName, null));

createBranch(Branch.of(targetBranchName, null));
Branch targetBranch = retrieveBranch(targetBranchName);
Branch targetBranch = createBranch(Branch.of(targetBranchName, null));

addContent(branch, Put.of(ContentKey.of("allowed-x"), IcebergTable.of("foo", 42, 42, 42, 42)));
branch = retrieveBranch(branchName);
Expand All @@ -352,12 +375,10 @@ void testCanReadTargetBranchDuringTransplant() throws BaseNessieClientServerExce
void testCannotReadTargetBranch() throws BaseNessieClientServerException {
String role = "user1";
String branchName = "allowedBranchForUser1";
createBranch(Branch.of(branchName, null));
Branch branch = createBranch(Branch.of(branchName, null));
String disallowedBranch = "disallowedBranchForUser1";
createBranch(Branch.of(disallowedBranch, null));

final Branch branch = retrieveBranch(branchName);

String errorMessage =
String.format(
"'VIEW_REFERENCE' is not allowed for role '%s' on reference '%s'",
Expand Down Expand Up @@ -398,8 +419,8 @@ private Branch retrieveBranch(String branchName) throws NessieNotFoundException
return (Branch) api().getReference().refName(branchName).get();
}

private void createBranch(Branch branch) throws BaseNessieClientServerException {
api().createReference().sourceRefName("main").reference(branch).create();
private Branch createBranch(Branch branch) throws BaseNessieClientServerException {
return (Branch) api().createReference().sourceRefName("main").reference(branch).create();
}

private void addContent(Branch branch, Put put) throws BaseNessieClientServerException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,17 @@ public class NessieAuthorizationTestProfile extends AuthenticationEnabledProfile
("op=='COMMIT_CHANGE_AGAINST_REFERENCE' && role.startsWith('test_user') && ref.startsWith"
+ "('allowedBranch')"))
.put(
"nessie.server.authorization.rules.allow_create_or_delete_entity",
"op in ['VIEW_REFERENCE', 'READ_ENTITY_VALUE', 'UPDATE_ENTITY', 'DELETE_ENTITY'] "
+ "&& role=='test_user' && path.startsWith('allowed-') && ref.startsWith('allowedBranch')")
"nessie.server.authorization.rules.allow_create_not_read_entity",
"op in ['VIEW_REFERENCE', 'UPDATE_ENTITY'] "
+ "&& role=='test_user2' && path.startsWith('allowed-') && ref.startsWith('allowedBranch')")
.put(
"nessie.server.authorization.rules.allow_create_not_delete_entity",
"op in ['VIEW_REFERENCE', 'DELETE_ENTITY_VALUE', 'UPDATE_ENTITY'] "
+ "&& role=='test_user3' && path.startsWith('allowed-') && ref.startsWith('allowedBranch')")
.put(
"nessie.server.authorization.rules.allow_no_create_entity",
"op in ['VIEW_REFERENCE', 'READ_ENTITY_VALUE', 'DELETE_ENTITY'] "
+ "&& role=='test_user4' && path.startsWith('allowed-') && ref.startsWith('allowedBranch')")
.put(
"nessie.server.authorization.rules.allow_commits_without_entity_changes",
"op=='COMMIT_CHANGE_AGAINST_REFERENCE' && role=='test_user2' && ref.startsWith('allowedBranch')")
Expand Down
Loading

0 comments on commit 49d9266

Please sign in to comment.