azotcsit commented on a change in pull request #1208:
URL: https://github.com/apache/cassandra/pull/1208#discussion_r729867298
##########
File path: src/java/org/apache/cassandra/auth/Roles.java
##########
@@ -38,26 +35,8 @@
private static final Role NO_ROLE = new Role("", false, false,
Collections.emptyMap(), Collections.emptySet());
- private static RolesCache cache;
- static
- {
- initRolesCache(DatabaseDescriptor.getRoleManager(),
- () ->
DatabaseDescriptor.getAuthenticator().requireAuthentication());
- }
-
- @VisibleForTesting
- public static void initRolesCache(IRoleManager roleManager,
BooleanSupplier enableCache)
- {
- if (cache != null)
- cache.unregisterMBean();
- cache = new RolesCache(roleManager, enableCache);
- }
-
- @VisibleForTesting
- public static void clearCache()
- {
- cache.invalidate();
- }
+ public static final RolesCache cache = new
RolesCache(DatabaseDescriptor.getRoleManager(),
Review comment:
Correct. I was a reviewer for CASSANDRA-17016, so I handled the
overlapping with taking those changes into account.
##########
File path:
src/java/org/apache/cassandra/db/virtual/JmxPermissionsCacheTable.java
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.db.virtual;
+
+import java.util.Optional;
+import java.util.Set;
+
+import org.apache.cassandra.auth.IResource;
+import org.apache.cassandra.auth.Permission;
+import org.apache.cassandra.auth.PermissionDetails;
+import org.apache.cassandra.auth.Resources;
+import org.apache.cassandra.auth.RoleResource;
+import org.apache.cassandra.auth.jmx.AuthorizationProxy;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.dht.LocalPartitioner;
+import org.apache.cassandra.schema.TableMetadata;
+
+final class JmxPermissionsCacheTable extends AbstractMutableVirtualTable
+{
+ private static final String ROLE = "role";
+ private static final String GRANTEE = "grantee";
+ private static final String RESOURCE = "resource";
+ private static final String PERMISSION = "permission";
+
+ JmxPermissionsCacheTable(String keyspace)
+ {
+ super(TableMetadata.builder(keyspace, "jmx_permissions_cache")
+ .comment("JMX permissions cache")
+ .kind(TableMetadata.Kind.VIRTUAL)
+ .partitioner(new LocalPartitioner(UTF8Type.instance))
+ .addPartitionKeyColumn(ROLE, UTF8Type.instance)
+ .addClusteringColumn(GRANTEE, UTF8Type.instance)
+ .addClusteringColumn(RESOURCE, UTF8Type.instance)
+ .addClusteringColumn(PERMISSION, UTF8Type.instance)
Review comment:
This class does not exist anymore. The latest version has
`.addPartitionKeyColumn(ROLE, UTF8Type.instance)` only.
It seems to be an old comment that you added to review some time ago and
posted now.
##########
File path: src/java/org/apache/cassandra/auth/Roles.java
##########
@@ -38,26 +35,8 @@
private static final Role NO_ROLE = new Role("", false, false,
Collections.emptyMap(), Collections.emptySet());
- private static RolesCache cache;
- static
- {
- initRolesCache(DatabaseDescriptor.getRoleManager(),
- () ->
DatabaseDescriptor.getAuthenticator().requireAuthentication());
- }
-
- @VisibleForTesting
- public static void initRolesCache(IRoleManager roleManager,
BooleanSupplier enableCache)
- {
- if (cache != null)
- cache.unregisterMBean();
- cache = new RolesCache(roleManager, enableCache);
- }
-
- @VisibleForTesting
- public static void clearCache()
- {
- cache.invalidate();
- }
+ public static final RolesCache cache = new
RolesCache(DatabaseDescriptor.getRoleManager(),
Review comment:
Good catch! Again rebase did not highlight the issue. Fixed now.
##########
File path: src/java/org/apache/cassandra/auth/Roles.java
##########
@@ -38,26 +35,8 @@
private static final Role NO_ROLE = new Role("", false, false,
Collections.emptyMap(), Collections.emptySet());
- private static RolesCache cache;
- static
- {
- initRolesCache(DatabaseDescriptor.getRoleManager(),
- () ->
DatabaseDescriptor.getAuthenticator().requireAuthentication());
- }
-
- @VisibleForTesting
- public static void initRolesCache(IRoleManager roleManager,
BooleanSupplier enableCache)
- {
- if (cache != null)
- cache.unregisterMBean();
- cache = new RolesCache(roleManager, enableCache);
- }
-
- @VisibleForTesting
- public static void clearCache()
- {
- cache.invalidate();
- }
+ public static final RolesCache cache = new
RolesCache(DatabaseDescriptor.getRoleManager(),
Review comment:
Good catch! Again rebase did not highlight the issue, I'll make sure to
run the build locally next time. Fixed now.
##########
File path:
test/unit/org/apache/cassandra/db/virtual/CredentialsCacheKeysTableTest.java
##########
@@ -0,0 +1,180 @@
+/*
+ * 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.db.virtual;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.datastax.driver.core.EndPoint;
+import com.datastax.driver.core.PlainTextAuthProvider;
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.auth.AuthTestUtils;
+import org.apache.cassandra.auth.AuthenticatedUser;
+import org.apache.cassandra.auth.IAuthenticator;
+import org.apache.cassandra.auth.RoleResource;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.cql3.UntypedResultSet;
+
+import static org.apache.cassandra.auth.AuthTestUtils.ROLE_A;
+import static org.apache.cassandra.auth.AuthTestUtils.ROLE_B;
+
+public class CredentialsCacheKeysTableTest extends CQLTester
+{
+ private static final String KS_NAME = "vts";
+ private static AuthTestUtils.LocalPasswordAuthenticator
passwordAuthenticator;
+
+ @SuppressWarnings("FieldCanBeLocal")
+ private CredentialsCacheKeysTable table;
+
+ @BeforeClass
+ public static void setUpClass()
+ {
+ // high value is used for convenient debugging
+ DatabaseDescriptor.setPermissionsValidity(20_000);
+
+ CQLTester.setUpClass();
+ AuthTestUtils.LocalCassandraRoleManager roleManager = new
AuthTestUtils.LocalCassandraRoleManager();
+ passwordAuthenticator = new AuthTestUtils.LocalPasswordAuthenticator();
+ SchemaLoader.setupAuth(roleManager,
+ passwordAuthenticator,
+ new AuthTestUtils.LocalCassandraAuthorizer(),
+ new AuthTestUtils.LocalCassandraNetworkAuthorizer());
+
+ roleManager.createRole(AuthenticatedUser.SYSTEM_USER, ROLE_A,
AuthTestUtils.getLoginRoleOprions());
Review comment:
Oh ok, it is a rebase issue. After rebasing onto the latest trunk the
method name got fixed (CASSANDRA-16926), but since there was no conflict here,
I missed it. Fixed everywhere.
##########
File path:
test/unit/org/apache/cassandra/auth/CassandraNetworkAuthorizerTest.java
##########
@@ -63,13 +62,12 @@ public static void setupSuperUser()
public static void defineSchema() throws ConfigurationException
{
SchemaLoader.prepareServer();
- SchemaLoader.setupAuth(new LocalCassandraRoleManager(),
- new PasswordAuthenticator(),
- new AuthTestUtils.LocalCassandraAuthorizer(),
- new
AuthTestUtils.LocalCassandraNetworkAuthorizer());
+ SchemaLoader.setupAuth(new AuthTestUtils.LocalCassandraRoleManager(),
Review comment:
Yes, it was automatically formatted by Intellij. I fixed it everywhere,
but generally I'm not sure how to deal with this problem. We either need to
document some project-wide stylistic preferences or define some checkstyle
rules. Otherwise, people will keep changing formatting based on their
subjective preferences.
--
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]
