nvharikrishna commented on code in PR #165: URL: https://github.com/apache/cassandra-sidecar/pull/165#discussion_r1892965606
########## server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/Action.java: ########## @@ -0,0 +1,57 @@ +/* + * 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 + * + * 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.apache.cassandra.sidecar.acl.authorization; + +import io.vertx.ext.auth.authorization.Authorization; +import org.jetbrains.annotations.NotNull; + +import static org.apache.cassandra.sidecar.acl.authorization.WildcardAction.WILDCARD_PART_DIVIDER_TOKEN; +import static org.apache.cassandra.sidecar.acl.authorization.WildcardAction.WILDCARD_TOKEN; + +/** + * Represents an action that can be granted to a user on a resource or across resources. + */ +public interface Action +{ + /** + * @return {@link Authorization}. + */ + default Authorization toAuthorization() + { + return toAuthorization(null); Review Comment: What does `null` as a resource represent? All resources or no resources? ########## server-common/src/main/java/org/apache/cassandra/sidecar/db/schema/AbstractSchema.java: ########## @@ -69,13 +81,16 @@ protected boolean initializeInternal(@NotNull Session session) } prepareStatements(session); + logger.info("{} is initialized!", this.getClass().getSimpleName()); return true; } protected abstract String keyspaceName(); protected abstract void prepareStatements(@NotNull Session session); + protected abstract void unprepareStatements(); Review Comment: Looks like it intended to reset prepared statements. Does it sound better if it is called as `resetPreparedStatements()`? `unprepareStatements` looks like a getter but it is not returning a value and not a setter as it is not accepting a value to set. ########## server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/Action.java: ########## @@ -0,0 +1,57 @@ +/* + * 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 + * + * 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.apache.cassandra.sidecar.acl.authorization; + +import io.vertx.ext.auth.authorization.Authorization; +import org.jetbrains.annotations.NotNull; + +import static org.apache.cassandra.sidecar.acl.authorization.WildcardAction.WILDCARD_PART_DIVIDER_TOKEN; +import static org.apache.cassandra.sidecar.acl.authorization.WildcardAction.WILDCARD_TOKEN; + +/** + * Represents an action that can be granted to a user on a resource or across resources. + */ +public interface Action +{ + /** + * @return {@link Authorization}. + */ + default Authorization toAuthorization() + { + return toAuthorization(null); + } + + /** + * @return {@link Authorization} created for a resource + */ + Authorization toAuthorization(String resource); + + /** + * @return a new instance of {@link Action} + */ + static Action fromName(@NotNull String name) + { + boolean isWildCard = name.equals(WILDCARD_TOKEN) || name.contains(WILDCARD_PART_DIVIDER_TOKEN); Review Comment: WILDCARD_TOKEN and WILDCARD_PART_DIVIDER_TOKEN are from the implementation class of this interface. Ideally the interface should not be aware of implementation details. ########## server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/AdminIdentityResolver.java: ########## @@ -0,0 +1,55 @@ +/* + * 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 + * + * 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.apache.cassandra.sidecar.acl.authorization; + +import java.util.Set; + +import com.google.inject.Inject; +import com.google.inject.Singleton; +import org.apache.cassandra.sidecar.acl.IdentityToRoleCache; +import org.apache.cassandra.sidecar.config.SidecarConfiguration; + +/** + * Evaluates if provided identity is an admin identity. + */ +@Singleton +public class AdminIdentityResolver +{ + private final IdentityToRoleCache identityToRoleCache; + private final SuperUserCache superUserCache; + private final Set<String> adminIdentities; + + @Inject + public AdminIdentityResolver(IdentityToRoleCache identityToRoleCache, + SuperUserCache superUserCache, + SidecarConfiguration sidecarConfiguration) + { + this.identityToRoleCache = identityToRoleCache; + this.superUserCache = superUserCache; + this.adminIdentities = sidecarConfiguration.accessControlConfiguration().adminIdentities(); + } + + public Boolean isAdmin(String identity) + { + String role = identityToRoleCache.get(identity); + Boolean isSuperuser = superUserCache.isSuperUser(role); + // Cassandra superusers have admin privileges. For fallback, sidecar configured admin identities are honored + return isSuperuser || adminIdentities.contains(identity); Review Comment: If the `superUserCache` doesn't have the given role, then it returns null, right? Unboxing null Boolean result NPE. How does SuperUserCache behaves if the role is null? ########## server/src/main/java/org/apache/cassandra/sidecar/acl/AuthCache.java: ########## @@ -164,6 +165,10 @@ protected void warmUp(int availableRetries) { cache.putAll(bulkLoadFunction.get()); } + catch (SchemaUnavailableException sue) Review Comment: Looks like an implementation specific exception. Is it the right place to handle it? It is also silently avoiding retries in the next catch statement. Is it intentional? ########## server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/AllowAllAuthorizationProvider.java: ########## @@ -0,0 +1,60 @@ +/* + * 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 + * + * 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.apache.cassandra.sidecar.acl.authorization; + +import io.vertx.core.AsyncResult; +import io.vertx.core.Future; +import io.vertx.core.Handler; +import io.vertx.ext.auth.User; +import io.vertx.ext.auth.authorization.AuthorizationProvider; + +/** + * Authorizer to allow all requests. + */ +public class AllowAllAuthorizationProvider implements AuthorizationProvider +{ + public static final AllowAllAuthorizationProvider INSTANCE = new AllowAllAuthorizationProvider(); Review Comment: Make sense to have a private default constructor as only `INSTANCE` is used? ########## server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/AllowAllAuthorization.java: ########## @@ -0,0 +1,45 @@ +/* + * 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 + * + * 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.apache.cassandra.sidecar.acl.authorization; + +import io.vertx.ext.auth.authorization.Authorization; +import io.vertx.ext.auth.authorization.AuthorizationContext; + +/** + * {@code Authorization} implementation to allow access for all users regardless of their authorizations. + */ +public class AllowAllAuthorization implements Authorization +{ + public static final AllowAllAuthorization INSTANCE = new AllowAllAuthorization(); Review Comment: Intended to have only one instance? If yes, then makes sense to have a private default constructor? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
