Backport to branch(3.14) : Add Cassandra permission test #2836
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @feeblefakie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces Cassandra permission tests to the branch(3.14). It includes new integration tests, configuration changes to run these tests, and utility classes to manage Cassandra user permissions during testing. The tests verify that users with appropriate permissions can perform administrative tasks.
Highlights
- Backport: This PR backports the Cassandra permission test functionality to the branch(3.14).
- Integration Tests: Adds new integration tests for Cassandra permissions, including tests for both normal users and admin users.
- Configuration: Introduces a new Gradle task,
integrationTestCassandraPermission, to run the Cassandra permission integration tests. - Properties: Adds support for specifying normal user credentials for Cassandra in the test properties.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request is an automated backport to add Cassandra permission tests. The changes include updates to the Gradle build configuration and the addition of several new integration test classes and utilities.
My review has identified some critical and high-severity issues:
- Security: There are several CQL injection vulnerabilities in
CassandraPermissionTestUtils.javawhere user and password strings are not properly escaped or quoted. - Correctness: The new
waitFor...methods in the test classes have potential resource leaks because they don't correctly handle closing resources when exceptions occur.
I've provided specific code suggestions to fix these issues.
| .execute( | ||
| String.format( | ||
| "CREATE ROLE %s WITH PASSWORD = '%s' AND LOGIN = true", userName, password)); |
There was a problem hiding this comment.
There is a critical CQL injection vulnerability here. Both userName and password are used directly in a formatted string to construct a CQL query.
- Username: If
userNamecontains spaces or special characters, it should be quoted. You can usecom.datastax.driver.core.Metadata.quoteIfNecessary()for this. - Password: If
passwordcontains a single quote, it will break the CQL query and can lead to injection. Single quotes in passwords should be escaped by doubling them (e.g.,pa'ssbecomespa''ss).
.execute(
String.format(
"CREATE ROLE %s WITH PASSWORD = '%s' AND LOGIN = true",
com.datastax.driver.core.Metadata.quoteIfNecessary(userName),
password.replace("'", "''")));| protected void waitForNamespaceCreation() { | ||
| try { | ||
| AdminTestUtils utils = getAdminTestUtils(TEST_NAME); | ||
| int retryCount = 0; | ||
| while (retryCount < MAX_RETRY_COUNT) { | ||
| if (utils.namespaceExists(NAMESPACE)) { | ||
| utils.close(); | ||
| return; | ||
| } | ||
| Uninterruptibles.sleepUninterruptibly( | ||
| SLEEP_BETWEEN_RETRIES_SECONDS, java.util.concurrent.TimeUnit.SECONDS); | ||
| retryCount++; | ||
| } | ||
| utils.close(); | ||
| throw new RuntimeException("Namespace was not created after " + MAX_RETRY_COUNT + " retries"); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to wait for namespace creation", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method has a potential resource leak. If an exception is thrown from within the while loop (e.g., from utils.namespaceExists()), the catch block will be executed without utils.close() being called. This could leave resources open. Use a finally block to ensure utils.close() is always called.
Additionally, the four waitFor... methods in this class are nearly identical. Consider refactoring them into a single private helper method to reduce code duplication and improve maintainability.
| protected void waitForNamespaceCreation() { | |
| try { | |
| AdminTestUtils utils = getAdminTestUtils(TEST_NAME); | |
| int retryCount = 0; | |
| while (retryCount < MAX_RETRY_COUNT) { | |
| if (utils.namespaceExists(NAMESPACE)) { | |
| utils.close(); | |
| return; | |
| } | |
| Uninterruptibles.sleepUninterruptibly( | |
| SLEEP_BETWEEN_RETRIES_SECONDS, java.util.concurrent.TimeUnit.SECONDS); | |
| retryCount++; | |
| } | |
| utils.close(); | |
| throw new RuntimeException("Namespace was not created after " + MAX_RETRY_COUNT + " retries"); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to wait for namespace creation", e); | |
| } | |
| } | |
| protected void waitForNamespaceCreation() { | |
| AdminTestUtils utils = getAdminTestUtils(TEST_NAME); | |
| try { | |
| int retryCount = 0; | |
| while (retryCount < MAX_RETRY_COUNT) { | |
| if (utils.namespaceExists(NAMESPACE)) { | |
| return; | |
| } | |
| Uninterruptibles.sleepUninterruptibly( | |
| SLEEP_BETWEEN_RETRIES_SECONDS, java.util.concurrent.TimeUnit.SECONDS); | |
| retryCount++; | |
| } | |
| throw new RuntimeException("Namespace was not created after " + MAX_RETRY_COUNT + " retries"); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to wait for namespace creation", e); | |
| } finally { | |
| utils.close(); | |
| } | |
| } |
| protected void waitForTableCreation() { | ||
| try { | ||
| AdminTestUtils utils = getAdminTestUtils(TEST_NAME); | ||
| int retryCount = 0; | ||
| while (retryCount < MAX_RETRY_COUNT) { | ||
| if (utils.tableExists(NAMESPACE, TABLE)) { | ||
| utils.close(); | ||
| return; | ||
| } | ||
| Uninterruptibles.sleepUninterruptibly(SLEEP_BETWEEN_RETRIES_SECONDS, TimeUnit.SECONDS); | ||
| retryCount++; | ||
| } | ||
| utils.close(); | ||
| throw new RuntimeException("Table was not created after " + MAX_RETRY_COUNT + " retries"); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to wait for table creation", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method has a potential resource leak. If an exception is thrown from within the while loop (e.g., from utils.tableExists()), the catch block will be executed without utils.close() being called, leaving resources open.
A finally block should be used to ensure utils.close() is always called, regardless of whether an exception occurs.
| protected void waitForTableCreation() { | |
| try { | |
| AdminTestUtils utils = getAdminTestUtils(TEST_NAME); | |
| int retryCount = 0; | |
| while (retryCount < MAX_RETRY_COUNT) { | |
| if (utils.tableExists(NAMESPACE, TABLE)) { | |
| utils.close(); | |
| return; | |
| } | |
| Uninterruptibles.sleepUninterruptibly(SLEEP_BETWEEN_RETRIES_SECONDS, TimeUnit.SECONDS); | |
| retryCount++; | |
| } | |
| utils.close(); | |
| throw new RuntimeException("Table was not created after " + MAX_RETRY_COUNT + " retries"); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to wait for table creation", e); | |
| } | |
| } | |
| protected void waitForTableCreation() { | |
| AdminTestUtils utils = getAdminTestUtils(TEST_NAME); | |
| try { | |
| int retryCount = 0; | |
| while (retryCount < MAX_RETRY_COUNT) { | |
| if (utils.tableExists(NAMESPACE, TABLE)) { | |
| return; | |
| } | |
| Uninterruptibles.sleepUninterruptibly(SLEEP_BETWEEN_RETRIES_SECONDS, TimeUnit.SECONDS); | |
| retryCount++; | |
| } | |
| throw new RuntimeException("Table was not created after " + MAX_RETRY_COUNT + " retries"); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to wait for table creation", e); | |
| } finally { | |
| utils.close(); | |
| } | |
| } |
|
|
||
| @Override | ||
| public void dropNormalUser(String userName) { | ||
| clusterManager.getSession().execute(String.format("DROP ROLE %s", userName)); |
There was a problem hiding this comment.
The userName is not quoted, which can lead to CQL injection if the username contains special characters. Please use com.datastax.driver.core.Metadata.quoteIfNecessary() to properly quote the username.
clusterManager.getSession().execute(String.format("DROP ROLE %s", com.datastax.driver.core.Metadata.quoteIfNecessary(userName)));| return new String[] { | ||
| String.format("GRANT CREATE ON ALL KEYSPACES TO %s", userName), | ||
| String.format("GRANT DROP ON ALL KEYSPACES TO %s", userName), | ||
| String.format("GRANT ALTER ON ALL KEYSPACES TO %s", userName), | ||
| String.format("GRANT SELECT ON ALL KEYSPACES TO %s", userName), | ||
| String.format("GRANT MODIFY ON ALL KEYSPACES TO %s", userName) | ||
| }; |
There was a problem hiding this comment.
The userName parameter is used directly in String.format to build GRANT statements, which is a potential CQL injection vulnerability. To prevent this, you should quote the username using com.datastax.driver.core.Metadata.quoteIfNecessary().
return new String[] {
String.format("GRANT CREATE ON ALL KEYSPACES TO %s", com.datastax.driver.core.Metadata.quoteIfNecessary(userName)),
String.format("GRANT DROP ON ALL KEYSPACES TO %s", com.datastax.driver.core.Metadata.quoteIfNecessary(userName)),
String.format("GRANT ALTER ON ALL KEYSPACES TO %s", com.datastax.driver.core.Metadata.quoteIfNecessary(userName)),
String.format("GRANT SELECT ON ALL KEYSPACES TO %s", com.datastax.driver.core.Metadata.quoteIfNecessary(userName)),
String.format("GRANT MODIFY ON ALL KEYSPACES TO %s", com.datastax.driver.core.Metadata.quoteIfNecessary(userName))
};
3 participants
This is an automated backport of the following:
Please merge this PR after all checks have passed.