dlmarion commented on code in PR #96: URL: https://github.com/apache/accumulo-access/pull/96#discussion_r2677542896
########## core/src/main/java/org/apache/accumulo/access/impl/AuthorizationsImpl.java: ########## @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * https://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.apache.accumulo.access.impl; + +import java.util.Iterator; +import java.util.Set; + +import org.apache.accumulo.access.Authorizations; + +public final class AuthorizationsImpl implements Authorizations { Review Comment: I wonder if this could simplify the code? Records are implicitly `final` and it takes care of the `equals`, `hashcode` and `toString` methods. ``` public record AuthorizationsImpl(Set<String> authorizations) implements Authorizations ``` ########## core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java: ########## @@ -107,31 +120,32 @@ public static String unescape(BytesWrapper auth) { * @param shouldQuote true to wrap escaped authorization in quotes * @return escaped authorization string */ - public static byte[] escape(byte[] auth, boolean shouldQuote) { + public static CharSequence escape(CharSequence auth, boolean shouldQuote) { Review Comment: Same comment about two passes. ########## core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java: ########## @@ -143,20 +157,29 @@ public boolean canAccess(AccessExpression expression) { @Override public boolean canAccess(String expression) throws InvalidAccessExpressionException { - return evaluate(expression.getBytes(UTF_8)); - } - - @Override - public boolean canAccess(byte[] expression) throws InvalidAccessExpressionException { return evaluate(expression); } - boolean evaluate(byte[] accessExpression) throws InvalidAccessExpressionException { - var bytesWrapper = ParserEvaluator.lookupWrappers.get(); + boolean evaluate(String accessExpression) throws InvalidAccessExpressionException { + var charsWrapper = ParserEvaluator.lookupWrappers.get(); Predicate<Tokenizer.AuthorizationToken> atp = authToken -> { - bytesWrapper.set(authToken.data, authToken.start, authToken.len); - return authorizedPredicate.test(bytesWrapper); + var authorization = ParserEvaluator.unescape(authToken, charsWrapper); + if (!authorizationValidator.test(authorization, authToken.quoting)) { Review Comment: This will throw an NPE if the validator is null. ########## core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java: ########## Review Comment: I wonder if this can be a Record and if there is any benefit to it. ########## core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java: ########## @@ -30,42 +29,56 @@ import org.apache.accumulo.access.AccessEvaluator; import org.apache.accumulo.access.AccessExpression; +import org.apache.accumulo.access.AuthorizationValidator; import org.apache.accumulo.access.Authorizations; import org.apache.accumulo.access.InvalidAccessExpressionException; +import org.apache.accumulo.access.InvalidAuthorizationException; public final class AccessEvaluatorImpl implements AccessEvaluator { - private final Predicate<BytesWrapper> authorizedPredicate; + private final Predicate<CharSequence> authorizedPredicate; + // TODO set + private final AuthorizationValidator authorizationValidator; /** * Create an AccessEvaluatorImpl using an Authorizer object */ - public AccessEvaluatorImpl(Authorizer authorizationChecker) { - this.authorizedPredicate = auth -> authorizationChecker.isAuthorized(unescape(auth)); + public AccessEvaluatorImpl(Authorizer authorizationChecker, + AuthorizationValidator authorizationValidator) { + this.authorizedPredicate = auth -> authorizationChecker.isAuthorized(auth.toString()); + this.authorizationValidator = authorizationValidator; Review Comment: Can the validator be null? ########## core/src/main/java/org/apache/accumulo/access/impl/CharsWrapper.java: ########## @@ -18,46 +18,59 @@ */ package org.apache.accumulo.access.impl; -import static java.nio.charset.StandardCharsets.UTF_8; -import static java.util.Objects.checkFromIndexSize; -import static java.util.Objects.checkIndex; - import java.util.Arrays; +import java.util.Objects; -public final class BytesWrapper implements Comparable<BytesWrapper> { - - private byte[] data; +public final class CharsWrapper implements CharSequence { Review Comment: Can this benefit from being a Record? ########## core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java: ########## @@ -143,20 +157,29 @@ public boolean canAccess(AccessExpression expression) { @Override public boolean canAccess(String expression) throws InvalidAccessExpressionException { - return evaluate(expression.getBytes(UTF_8)); - } - - @Override - public boolean canAccess(byte[] expression) throws InvalidAccessExpressionException { return evaluate(expression); } - boolean evaluate(byte[] accessExpression) throws InvalidAccessExpressionException { - var bytesWrapper = ParserEvaluator.lookupWrappers.get(); + boolean evaluate(String accessExpression) throws InvalidAccessExpressionException { + var charsWrapper = ParserEvaluator.lookupWrappers.get(); Predicate<Tokenizer.AuthorizationToken> atp = authToken -> { - bytesWrapper.set(authToken.data, authToken.start, authToken.len); - return authorizedPredicate.test(bytesWrapper); + var authorization = ParserEvaluator.unescape(authToken, charsWrapper); + if (!authorizationValidator.test(authorization, authToken.quoting)) { + throw new InvalidAuthorizationException(authorization.toString()); + } + return authorizedPredicate.test(authorization); + }; + + // This is used once the expression is known to always be true or false. For this case only need + // to validate authorizations, do not need to look them up in a set. + Predicate<Tokenizer.AuthorizationToken> shortCircuit = authToken -> { + var authorization = ParserEvaluator.unescape(authToken, charsWrapper); + if (!authorizationValidator.test(authorization, authToken.quoting)) { Review Comment: This will throw an NPE if the validator is null. ########## core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java: ########## @@ -30,42 +29,56 @@ import org.apache.accumulo.access.AccessEvaluator; import org.apache.accumulo.access.AccessExpression; +import org.apache.accumulo.access.AuthorizationValidator; import org.apache.accumulo.access.Authorizations; import org.apache.accumulo.access.InvalidAccessExpressionException; +import org.apache.accumulo.access.InvalidAuthorizationException; public final class AccessEvaluatorImpl implements AccessEvaluator { - private final Predicate<BytesWrapper> authorizedPredicate; + private final Predicate<CharSequence> authorizedPredicate; + // TODO set + private final AuthorizationValidator authorizationValidator; /** * Create an AccessEvaluatorImpl using an Authorizer object */ - public AccessEvaluatorImpl(Authorizer authorizationChecker) { - this.authorizedPredicate = auth -> authorizationChecker.isAuthorized(unescape(auth)); + public AccessEvaluatorImpl(Authorizer authorizationChecker, + AuthorizationValidator authorizationValidator) { + this.authorizedPredicate = auth -> authorizationChecker.isAuthorized(auth.toString()); + this.authorizationValidator = authorizationValidator; } /** * Create an AccessEvaluatorImpl using a collection of authorizations */ - public AccessEvaluatorImpl(Authorizations authorizations) { + public AccessEvaluatorImpl(Authorizations authorizations, + AuthorizationValidator authorizationValidator) { var authsSet = authorizations.asSet(); - final Set<BytesWrapper> authBytes = new HashSet<>(authsSet.size()); + final Set<CharsWrapper> wrappedAuths = new HashSet<>(authsSet.size()); for (String authorization : authsSet) { - var auth = authorization.getBytes(UTF_8); - if (auth.length == 0) { + if (authorization.isEmpty()) { throw new IllegalArgumentException("Empty authorization"); } - authBytes.add(new BytesWrapper(AccessEvaluatorImpl.escape(auth, false))); + + wrappedAuths.add(new CharsWrapper(authorization.toCharArray())); } - authorizedPredicate = authBytes::contains; + this.authorizedPredicate = auth -> { + if (auth instanceof CharsWrapper) { + return wrappedAuths.contains(auth); + } else { + return wrappedAuths.contains(new CharsWrapper(auth.toString().toCharArray())); + } + }; + this.authorizationValidator = authorizationValidator; } - public static String unescape(BytesWrapper auth) { + public static CharSequence unescape(CharSequence auth) { Review Comment: This makes two passes over the auth. Do you think it can be done in one pass? Maybe an issue for a different time. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
