From a878adfc895b2e70b058f90ad5d207c710788c4d Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Thu, 4 Jul 2024 16:53:49 +0200 Subject: [PATCH] Do not create a role if ALTER ROLE IF EXISTS operates on non-existing role patch by Stefan Miklosovic; reviewed by Brandon Williams for CASSANDRA-19749 --- CHANGES.txt | 1 + pylib/cqlshlib/cql3handling.py | 2 +- pylib/cqlshlib/test/test_cqlsh_completion.py | 9 +++++ .../cql3/statements/AlterRoleStatement.java | 3 ++ .../auth/CreateAndAlterRoleTest.java | 40 ++++++++++++++++++- 5 files changed, 53 insertions(+), 2 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index cdceaa802d..1ade2c00bf 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.1.6 + * Do not create a role if ALTER ROLE IF EXISTS operates on non-existing role (CASSANDRA-19749) * Use OpOrder in repairIterator to ensure we don't lose memtables mid-paxos repair (Cassandra-19668) * Refresh stale paxos commit (CASSANDRA-19617) * Reduce info logging from automatic paxos repair (CASSANDRA-19445) diff --git a/pylib/cqlshlib/cql3handling.py b/pylib/cqlshlib/cql3handling.py index 03e06d8098..bbaea48325 100644 --- a/pylib/cqlshlib/cql3handling.py +++ b/pylib/cqlshlib/cql3handling.py @@ -1483,7 +1483,7 @@ syntax_rules += r''' | ; - ::= "CREATE" "ROLE" + ::= "CREATE" "ROLE" ( "IF" "NOT" "EXISTS" )? ( "WITH" ("AND" )*)? ; diff --git a/pylib/cqlshlib/test/test_cqlsh_completion.py b/pylib/cqlshlib/test/test_cqlsh_completion.py index af9d05ecbc..731b987d27 100644 --- a/pylib/cqlshlib/test/test_cqlsh_completion.py +++ b/pylib/cqlshlib/test/test_cqlsh_completion.py @@ -952,3 +952,12 @@ class TestCqlshCompletion(CqlshCompletionCase): def test_complete_in_alter_role(self): self.trycompletions('ALTER ROLE ', choices=['', 'IF', '']) + self.trycompletions('ALTER ROLE IF ', immediate='EXISTS ') + + def test_complete_in_create_user(self): + self.trycompletions('CREATE USER ', choices=['', 'IF']) + self.trycompletions('CREATE USER IF ', immediate='NOT EXISTS ') + + def test_complete_in_create_role(self): + self.trycompletions('CREATE ROLE ', choices=['', '', 'IF']) + self.trycompletions('CREATE ROLE IF ', immediate='NOT EXISTS ') diff --git a/src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java b/src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java index eb0e3e01f2..ccbaeb20a5 100644 --- a/src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java @@ -105,6 +105,9 @@ public class AlterRoleStatement extends AuthenticationStatement public ResultMessage execute(ClientState state) throws RequestValidationException, RequestExecutionException { + if (ifExists && !DatabaseDescriptor.getRoleManager().isExistingRole(role)) + return null; + if (!opts.isEmpty()) DatabaseDescriptor.getRoleManager().alterRole(state.getUser(), role, opts); if (dcPermissions != null) diff --git a/test/unit/org/apache/cassandra/auth/CreateAndAlterRoleTest.java b/test/unit/org/apache/cassandra/auth/CreateAndAlterRoleTest.java index 33ae129b7e..5b079b4525 100644 --- a/test/unit/org/apache/cassandra/auth/CreateAndAlterRoleTest.java +++ b/test/unit/org/apache/cassandra/auth/CreateAndAlterRoleTest.java @@ -18,13 +18,20 @@ package org.apache.cassandra.auth; +import java.util.HashSet; +import java.util.Set; + import org.junit.BeforeClass; import org.junit.Test; +import com.datastax.driver.core.ResultSet; import com.datastax.driver.core.exceptions.AuthenticationException; +import com.datastax.driver.core.exceptions.InvalidQueryException; import org.apache.cassandra.Util; import org.apache.cassandra.cql3.CQLTester; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mindrot.jbcrypt.BCrypt.gensalt; import static org.mindrot.jbcrypt.BCrypt.hashpw; @@ -101,7 +108,7 @@ public class CreateAndAlterRoleTest extends CQLTester String.format("CREATE USER %s WITH hashed password '%s'", user1, "this_is_an_invalid_hash")); executeNet(String.format("CREATE USER %s WITH hashed password '%s'", user1, hashedPassword)); - executeNet(String.format("CREATE USER %s WITH password '%s'", user2, plainTextPwd)); + executeNet(String.format("CREATE USER %s WITH password '%s'", user2, plainTextPwd)); useUser(user1, plainTextPwd); @@ -125,6 +132,37 @@ public class CreateAndAlterRoleTest extends CQLTester executeNetWithAuthSpin("SELECT key FROM system.local"); } + @Test + public void createAlterRoleIfExists() throws Throwable + { + useSuperUser(); + + executeNet("CREATE ROLE IF NOT EXISTS does_not_exist_yet"); + assertTrue(getAllRoles().contains("does_not_exist_yet")); + + // execute one more time + executeNet("CREATE ROLE IF NOT EXISTS does_not_exist_yet"); + + assertThatThrownBy(() -> executeNet("CREATE ROLE does_not_exist_yet")) + .isInstanceOf(InvalidQueryException.class) + .hasMessageContaining("does_not_exist_yet already exists"); + + // alter non-existing is no-op when "if exists" is specified + executeNet("ALTER ROLE IF EXISTS also_does_not_exist_yet WITH LOGIN = true"); + Set roles = getAllRoles(); + assertTrue(roles.contains("does_not_exist_yet")); + // not created - CASSANDRA-19749 + assertFalse(roles.contains("also_does_not_exist_yet")); + } + + private Set getAllRoles() throws Throwable + { + ResultSet rows = executeNet("SELECT role FROM system_auth.roles"); + Set roles = new HashSet<>(); + rows.forEach(row -> roles.add(row.getString(0))); + return roles; + } + /** * Altering or creating auth may take some time to be effective *