frankgh commented on code in PR #250:
URL: https://github.com/apache/cassandra-sidecar/pull/250#discussion_r2279910789
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/authentication/AuthenticationHandlerFactory.java:
##########
@@ -45,4 +46,14 @@ public interface AuthenticationHandlerFactory
AuthenticationHandlerInternal create(Vertx vertx,
AccessControlConfiguration
accessControlConfiguration,
Map<String, String> parameters)
throws ConfigurationException;
+
+ /**
+ * Validates that this authentication handler factory can be used with the
given configuration.
Review Comment:
```suggestion
* Validates that this authentication handler factory can be used with
the given configuration.
* This gives the implementation flexibility to establish pre-requisites
in order to properly function
* correctly.
```
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/authentication/MutualTlsAuthenticationHandlerFactory.java:
##########
@@ -106,4 +107,16 @@ private MutualTlsAuthenticationHandler
createInternal(Vertx vertx,
MutualTlsAuthentication mTLSAuthProvider = new
MutualTlsAuthenticationImpl(vertx, certificateValidator,
certificateIdentityExtractor);
return new MutualTlsAuthenticationHandler(mTLSAuthProvider,
identityToRoleCache);
}
+
+ @Override
+ public void validatePrerequisites(SidecarConfiguration
sidecarConfiguration) throws ConfigurationException
+ {
+ boolean isSidecarSchemaEnabled =
sidecarConfiguration.serviceConfiguration()
+
.schemaKeyspaceConfiguration()
+ .isEnabled();
+ if (!isSidecarSchemaEnabled)
+ {
+ throw new ConfigurationException("mTLS auth requires sidecar
schema to be enabled for role processing");
Review Comment:
The user facing configuration that we document in sidecar.yaml is
MutualTlsAuthenticationHandlerFactory, so I think we should say that instead.
```suggestion
throw new
ConfigurationException("MutualTlsAuthenticationHandlerFactory requires Sidecar
schema to be enabled for role processing");
```
##########
server/src/main/java/org/apache/cassandra/sidecar/modules/AuthModule.java:
##########
@@ -180,6 +181,10 @@ AuthorizationProvider
authorizationProvider(SidecarConfiguration sidecarConfigur
}
if
(config.className().equalsIgnoreCase(RoleBasedAuthorizationProvider.class.getName()))
{
+ if
(!sidecarConfiguration.serviceConfiguration().schemaKeyspaceConfiguration().isEnabled())
+ {
+ throw new ConfigurationException(config.className() + "
requires sidecar schema to be enabled for role permissions storage");
Review Comment:
```suggestion
throw new ConfigurationException(config.className() + "
requires Sidecar schema to be enabled for role permissions used by Sidecar");
```
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/authentication/JwtAuthenticationHandlerFactory.java:
##########
@@ -65,4 +66,16 @@ protected JwtParameters parameterParser(Map<String, String>
parameters)
{
return new JwtParameterExtractor(parameters);
}
+
+ @Override
+ public void validatePrerequisites(SidecarConfiguration
sidecarConfiguration) throws ConfigurationException
+ {
+ boolean isSidecarSchemaEnabled =
sidecarConfiguration.serviceConfiguration()
+
.schemaKeyspaceConfiguration()
+ .isEnabled();
+ if (!isSidecarSchemaEnabled)
+ {
+ throw new ConfigurationException("JWT auth requires sidecar schema
to be enabled for role processing");
Review Comment:
```suggestion
throw new
ConfigurationException("JwtAuthenticationHandlerFactory requires Sidecar schema
to be enabled for role processing");
```
--
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]
