CEP-50/CASS-20834: Enable IAuthenticator to declare supported and alterable role options #4427
Conversation
|
|
||
| /** | ||
| * Resets the initialized flag, enabling AuthConfig to be reconfigured multiple times within a single | ||
| * test case. |
There was a problem hiding this comment.
Specifically, see AuthConfigTest in this same PR.
|
|
||
| /** | ||
| * Returns the authenticator configured for this node. | ||
| */ |
There was a problem hiding this comment.
In the future this will become getDefaultAuthenticator(). Additional methods will be added to enable/disable negotiation and provide access to the node's supported authenticators.
|
|
||
| /** | ||
| * Indicates if this node uses an authenticator that requires authentication. | ||
| */ |
There was a problem hiding this comment.
In the future, the semantics will change from "'The' authenticator requires authentication" to "Any supported authenticator requires authentication." Existing callers to this method should be unaffected by that change.
| } | ||
|
|
||
| @After | ||
| public void teardown() { |
There was a problem hiding this comment.
brace on new line please
|
|
||
| public CassandraRoleManager(Map<String, String> parameters) | ||
| { | ||
| supportedOptions = DatabaseDescriptor.getAuthenticator() instanceof PasswordAuthenticator |
There was a problem hiding this comment.
can you please rebase this patch on top of current trunk? This constructor was changed in CEP-55 by conditionally adding Option.OPTIONS among supportedOptions if there is some specific role name generator.
|
|
||
| IAuthenticator authenticator = DatabaseDescriptor.getAuthenticator(); | ||
| if (authenticator instanceof PasswordAuthenticator) | ||
| this.passwordAuthenticatorOptional = Optional.of((PasswordAuthenticator) authenticator); |
There was a problem hiding this comment.
I think now we have the opportunity to change name of this variable to maybePasswordAuthenticator. If you check the codebase, naming like this is more prevalent and reads better.
We also do not need this. here. There is no ambiguity as in what field we are setting.
jcshepherd
changed the title
…RA-20834) With negotiated authentication (CEP-50), nodes may be configured with multiple authenticators. Prior to this change, a number of areas in the code assumed that there was a single configured authenticator and contained logic that switched depending on the authenticator type. This logic won't work when multiple authenticators can be configured. This change eliminates most calls to DataDescriptor.getAuthenticator(), by either requiring dependencies to specify the type of authenticator they're looking for, or (in the case of authenticator-specific role attributes) enabling individual authenticators to declare the role attributes they need. patch by jcshepherd; reviewed by <Reviewers> for CASSANDRA-20834
jcshepherd
force-pushed
the
jcshepherd/20834/trunk
branch
from
83f14f4 to
b192ada
Compare
… (CASSANDRA-20834 for CEP-50) With negotiated authentication (CEP-50), nodes may be configured with multiple authenticators. Prior to this change, a number of areas in the code assumed that there was a single configured authenticator and contained logic that switched depending on the authenticator type. This logic won't work when multiple authenticators can be configured. This change eliminates most calls to DataDescriptor.getAuthenticator(), by enabling individual authenticators to declare the role attributes they support, requiring callers to specify the type of authenticator they're looking for, and directly returning whether the node can enforce authn or not. Testing done: Unit tests for auth and config packages; d-tests for auth-related functionality (e.g. ColumnMasks). patch by jcshepherd; reviewed by TBD for CASSANDRA-20834
jcshepherd
force-pushed
the
jcshepherd/20834/trunk
branch
from
b192ada to
9aa01c9
Compare
|
Rebased and revised per Stefan's feedback. |
| } | ||
|
|
||
| private static void applyGuardrails() | ||
| public static void applyGuardrails() |
There was a problem hiding this comment.
I'm not thrilled with this, but I didn't see a cleaner way of initializing or defaulting guardrails config to enable CassandraRoleManager to construct in a unit test environment, without fully initializing DatabaseDescriptor. (If guardrails aren't explicitly initialized, CassandraRoleManager NPEs in the constructor.) Open to other suggestions.
There was a problem hiding this comment.
@jcshepherd what about calling DatabaseDescriptor.daemonInitialization() ? If you grep its usage, it is widely used in tests for these reasons.
You can also mock static methods like this, similar technique for DD you would be doing here
2 participants
Factoring out assumption of a single node-wide authenticator (CASSANDRA-20834 for CEP-50)
With negotiated authentication (CEP-50), nodes may be configured with multiple authenticators. Prior to this change, a number of areas in the code assumed that there was a single configured authenticator and contained logic that switched depending on the authenticator type. This logic won't work when multiple authenticators can be configured. This change eliminates most calls to DataDescriptor.getAuthenticator(), by either directly returning whether the node can enforce authn or not, requiring dependencies to specify the type of authenticator they're looking for, or (in the case of authenticator-specific role attributes) enabling individual authenticators to declare the role attributes they need.
Testing done: Unit tests for auth and config packages; d-tests for auth-related functionality (e.g. ColumnMasks).
patch by jcshepherd; reviewed by for CASSANDRA-20834