bbotella commented on code in PR #4379:
URL: https://github.com/apache/cassandra/pull/4379#discussion_r2433487899
##########
conf/cassandra.yaml:
##########
@@ -2507,20 +2507,41 @@ drop_compact_storage_enabled: false
# This would also apply to system keyspaces.
# maximum_replication_factor_warn_threshold: -1
# maximum_replication_factor_fail_threshold: -1
-#
+
+#role_name_policy:
+# # Implementation class of a validator. When not in form of FQCN, the
+# # package name org.apache.cassandra.db.guardrails is prepended.
+# # By default, there is no validator.
+# #validator_class_name: YourValidatorOfRoleNames
+# # Implementation class of related generator which generates values which
are valid when
+# # tested against this validator. When not in form of FQCN, the
+# # package name org.apache.cassandra.db.guardrails is prepended.
Review Comment:
This got me thinking. Is `org.apache.cassandra.db.guardrails` the right
package for a default? Should we add a new `org.apache.cassandra.db.policies`
package?
##########
doc/cql3/CQL.textile:
##########
@@ -1522,7 +1528,6 @@ bc(syntax)..
<create-user-statement> ::= CREATE USER ( IF NOT EXISTS )? <identifier> ( WITH
<option> ( AND <option> )* )?
Review Comment:
Shouldn't `CREATE (GENEARTED)? USER` be a thing here according to the CEP?
##########
src/antlr/Parser.g:
##########
@@ -1535,10 +1528,11 @@ listUsersStatement returns [ListRolesStatement stmt]
;
/**
- * CREATE ROLE [IF NOT EXISTS] <rolename> [ [WITH] option [ [AND] option ]* ]
+ * CREATE [GENERATED] ROLE [IF NOT EXISTS] <rolename> [ [WITH] option [ [AND]
option ]* ]
Review Comment:
Does it make sense for `[GENERATED]` and `[IF NOT EXISTS]` to coexist?
##########
pylib/cqlshlib/test/test_cqlsh_completion.py:
##########
@@ -1187,6 +1187,9 @@ def test_complete_in_alter_type(self):
def test_complete_in_alter_user(self):
self.trycompletions('ALTER USER ', choices=['<identifier>', 'IF',
'<pgStringLiteral>', '<quotedStringLiteral>'])
+ def test_complete_in_generated(self):
+ self.trycompletions('CREATE GENERATED ', immediate='ROLE ')
Review Comment:
And `USER`?
##########
src/antlr/Parser.g:
##########
@@ -1453,15 +1453,8 @@ createUserStatement returns [CreateRoleStatement stmt]
{
throw new SyntaxException("Options 'password' and 'hashed password'
are mutually exclusive");
}
- if (opts.getPassword().isPresent() && opts.isGeneratedPassword())
Review Comment:
Oh, this answers some of my comments around the missing USER. Thanks!
##########
src/java/org/apache/cassandra/cql3/statements/CreateRoleStatement.java:
##########
@@ -52,7 +54,11 @@ public class CreateRoleStatement extends
AuthenticationStatement
public CreateRoleStatement(RoleName name, RoleOptions options,
DCPermissions dcPermissions,
CIDRPermissions cidrPermissions, boolean
ifNotExists)
{
- this.role = RoleResource.role(name.getName());
+ if (name == null || name.getName() == null)
Review Comment:
Also, what about checking for `options. isGeneratedName()` instead? I think
it is a more robust approach?
##########
doc/cql3/CQL.textile:
##########
@@ -1370,9 +1372,13 @@ Use the @WITH PASSWORD@ clause to set a password for
internal authentication, en
If internal authentication has not been set up or the role does not have
@LOGIN@ privileges, the @WITH PASSWORD@ clause is not necessary.
When @WITH GENERATED PASSWORD@ is used, Cassandra provides out-of-the-box
CassandraPasswordValidator and CassandraPasswordGenerator
-under "password_validator" configuration property in cassandra.yaml. The usage
of this clause will generate a password for a given password strength policy,
as configured,
+under "password_policy" configuration property in cassandra.yaml. The usage of
this clause will generate a password for a given password strength policy, as
configured,
and such password is returned to a client in CQL shell after query is
executed. @GENERATED PASSWORD@ can not be used together with @HASHED PASSWORD@
nor with @PASSWORD@ alone.
+When @GENERATED@ is used in front of @ROLE@, it is possible to generate a role
name automatically. This is possible if CEP-55 is enabled in the configuration.
Review Comment:
Instead of CEP-55 reference, we should add the actual property name.
##########
src/antlr/Parser.g:
##########
@@ -1578,6 +1573,14 @@ createRoleStatement returns [CreateRoleStatement stmt]
{
throw new SyntaxException("Options 'hashed password' and 'generated
password' are mutually exclusive");
}
+ if (isGeneratedName)
Review Comment:
Let's add a
```
if (isGeneratedName && ifNotExists)
{
throw new SyntaxException("IF EXISTS can not be specified together with
GENERATED keyword.");
}
```
##########
src/java/org/apache/cassandra/cql3/statements/CreateRoleStatement.java:
##########
@@ -52,7 +54,11 @@ public class CreateRoleStatement extends
AuthenticationStatement
public CreateRoleStatement(RoleName name, RoleOptions options,
DCPermissions dcPermissions,
CIDRPermissions cidrPermissions, boolean
ifNotExists)
{
- this.role = RoleResource.role(name.getName());
+ if (name == null || name.getName() == null)
Review Comment:
For my education, can either of these two happen? What are the different
sources for these two different approaches to a name not being present?
##########
conf/cassandra_latest.yaml:
##########
@@ -2290,6 +2290,92 @@ drop_compact_storage_enabled: false
# maximum_replication_factor_warn_threshold: -1
# maximum_replication_factor_fail_threshold: -1
+#role_name_policy:
Review Comment:
Good catch. I'm fine with it being part of this PR, but I also see that
being part of its own low hanging fruit jira.
##########
src/java/org/apache/cassandra/auth/CassandraRoleManager.java:
##########
@@ -827,4 +853,52 @@ public void setInvalidClientDisconnectMaxJitterMillis(long
duration)
this.invalidClientDisconnectMaxJitterMillis = duration;
scheduleDisconnectInvalidRoleTask();
}
+
+ private static final ColumnSpecification GENERATED_PASSWORD_METADATA = new
ColumnSpecification(SchemaConstants.AUTH_KEYSPACE_NAME,
+
"generated_password",
+
new ColumnIdentifier("generated_password", true),
+
UTF8Type.instance);
+
+ private static final ColumnSpecification GENERATED_ROLE_NAME_METADATA =
new ColumnSpecification(SchemaConstants.AUTH_KEYSPACE_NAME,
+
"generated_role_name",
+
new ColumnIdentifier("generated_role_name", true),
Review Comment:
For consistency, I'd leave this as `generated_name` or change the previous
one to `generated_role_password`
--
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]
