diff --git a/servers/services/src/main/java/org/projectnessie/services/authz/RetriableAccessChecker.java b/servers/services/src/main/java/org/projectnessie/services/authz/RetriableAccessChecker.java new file mode 100644 index 00000000000..ed3dc8f9cc8 --- /dev/null +++ b/servers/services/src/main/java/org/projectnessie/services/authz/RetriableAccessChecker.java @@ -0,0 +1,63 @@ +/* + * 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.services.authz; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Map; +import java.util.function.Supplier; + +/** + * A utility class for performing access check in contexts where operations may have to be re-tried + * due to optimistic locking or similar mechanisms. Each {@link #newAttempt() attempt} forms a new + * batch of access checks, but they are not re-validated on subsequent attempts unless the new batch + * of checks is different from the already validated one. The order of checks matters. + */ +public final class RetriableAccessChecker { + private final Supplier validator; + private Collection validatedChecks; + private Map result; + + public RetriableAccessChecker(Supplier validator) { + Preconditions.checkNotNull(validator); + this.validator = validator; + } + + public BatchAccessChecker newAttempt() { + return new Attempt(); + } + + private class Attempt extends AbstractBatchAccessChecker { + @Override + public Map check() { + // Shallow collection copy to ensure that we use what was current at the time of check + // in the equals call below (in case checks are added to this instance later, for whatever + // reason). Note that elements are immutable. + Collection currentChecks = new ArrayList<>(getChecks()); + + if (validatedChecks != null && result != null && validatedChecks.equals(currentChecks)) { + return result; + } + + BatchAccessChecker checker = validator.get(); + currentChecks.forEach(checker::can); + result = checker.check(); + validatedChecks = currentChecks; + return result; + } + } +} diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java index a60de562c18..c6410af9d7f 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java @@ -101,6 +101,7 @@ import org.projectnessie.services.authz.AuthzPaginationIterator; import org.projectnessie.services.authz.BatchAccessChecker; import org.projectnessie.services.authz.Check; +import org.projectnessie.services.authz.RetriableAccessChecker; import org.projectnessie.services.cel.CELUtil; import org.projectnessie.services.config.ServerConfig; import org.projectnessie.services.hash.HashValidator; @@ -1065,8 +1066,16 @@ public CommitResponse commitMultipleOperations( } private CommitValidator createCommitValidator(BranchName branchName) { + // Commits routinely run retries due to collisions on updating the HEAD of the branch. + // Authorization is not dependent on the commit history, only on the collection of access + // checks, which reflect the current commit. On retries, the commit data relevant to access + // checks almost never changes. Therefore, we use RetriableAccessChecker to avoid re-validating + // access checks (which could be a time-consuming operation) on subsequent retries, unless + // authorization input data changes. + RetriableAccessChecker accessChecker = new RetriableAccessChecker(this::startAccessCheck); return validation -> { - BatchAccessChecker check = startAccessCheck().canCommitChangeAgainstReference(branchName); + BatchAccessChecker check = accessChecker.newAttempt(); + check.canCommitChangeAgainstReference(branchName); validation .operations() .forEach( diff --git a/servers/services/src/test/java/org/projectnessie/services/authz/TestRetriableAccessChecker.java b/servers/services/src/test/java/org/projectnessie/services/authz/TestRetriableAccessChecker.java new file mode 100644 index 00000000000..b8bb7e660bc --- /dev/null +++ b/servers/services/src/test/java/org/projectnessie/services/authz/TestRetriableAccessChecker.java @@ -0,0 +1,117 @@ +/* + * 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.services.authz; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.projectnessie.model.IdentifiedContentKey.IdentifiedElement.identifiedElement; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.function.Supplier; +import org.junit.jupiter.api.Test; +import org.projectnessie.model.Content; +import org.projectnessie.model.ContentKey; +import org.projectnessie.model.IdentifiedContentKey; +import org.projectnessie.versioned.BranchName; + +class TestRetriableAccessChecker { + + private int checkCount; + private final List checked = new ArrayList<>(); + private final Map result = new HashMap<>(); + + private final Supplier validator = + () -> + new AbstractBatchAccessChecker() { + @Override + public Map check() { + checkCount++; + checked.clear(); + checked.addAll(getChecks()); + return result; + } + }; + + @Test + void checkAndThrow() { + RetriableAccessChecker checker = new RetriableAccessChecker(validator); + Check check = Check.check(Check.CheckType.CREATE_ENTITY); + result.put(check, "test123"); + assertThatThrownBy(() -> checker.newAttempt().can(check).checkAndThrow()) + .isInstanceOf(AccessCheckException.class) + .hasMessage("test123"); + assertThat(checked).containsExactly(check); + assertThat(checkCount).isEqualTo(1); + } + + @Test + void repeatedCheck() { + RetriableAccessChecker checker = new RetriableAccessChecker(validator); + Check c1 = Check.check(Check.CheckType.CREATE_ENTITY); + Check c2 = Check.check(Check.CheckType.CREATE_REFERENCE); + assertThat(checker.newAttempt().can(c1).can(c2).check()).isEmpty(); + assertThat(checked).containsExactly(c1, c2); + assertThat(checkCount).isEqualTo(1); + + // repeating the same checks in same order does not re-trigger validation + assertThat(checker.newAttempt().can(c1).can(c2).check()).isEmpty(); + assertThat(checked).containsExactly(c1, c2); + assertThat(checkCount).isEqualTo(1); + + // repeating the same checks in different order triggers re-validation + assertThat(checker.newAttempt().can(c2).can(c1).check()).isEmpty(); + assertThat(checked).containsExactly(c2, c1); + assertThat(checkCount).isEqualTo(2); + } + + @Test + void dataChangeBetweenAttempts() { + IdentifiedContentKey.IdentifiedElement ns1 = identifiedElement("namespace", "id1"); + IdentifiedContentKey.IdentifiedElement ns2 = identifiedElement("namespace", "id2"); + IdentifiedContentKey.IdentifiedElement tableElement = identifiedElement("table", "id3"); + + IdentifiedContentKey t1 = + IdentifiedContentKey.builder() + .type(Content.Type.ICEBERG_TABLE) + .contentKey(ContentKey.of(ns1.element(), tableElement.element())) + .addElements(ns1, tableElement) + .build(); + + // Note: only the namespace ID is different between `t1` and `t2` + IdentifiedContentKey t2 = + IdentifiedContentKey.builder() + .type(Content.Type.ICEBERG_TABLE) + .contentKey(ContentKey.of(ns2.element(), tableElement.element())) + .addElements(ns2, tableElement) + .build(); + + RetriableAccessChecker checker = new RetriableAccessChecker(validator); + BranchName ref = BranchName.of("test"); + assertThat(checker.newAttempt().canCreateEntity(ref, t1).check()).isEmpty(); + assertThat(checked) + .containsExactly(Check.canViewReference(ref), Check.canCreateEntity(ref, t1)); + assertThat(checkCount).isEqualTo(1); + + // repeating the attempt with different checks' data results in re-validation + assertThat(checker.newAttempt().canCreateEntity(ref, t2).check()).isEmpty(); + assertThat(checked) + .containsExactly(Check.canViewReference(ref), Check.canCreateEntity(ref, t2)); + assertThat(checkCount).isEqualTo(2); + } +}