chia7712 commented on code in PR #18683:
URL: https://github.com/apache/kafka/pull/18683#discussion_r2152310283
##########
clients/src/test/java/org/apache/kafka/common/security/JaasContextTest.java:
##########
@@ -252,6 +254,32 @@ public void testDisallowedLoginModulesSystemProperty()
throws Exception {
checkEntry(context.configurationEntries().get(0),
"com.sun.security.auth.module.LdapLoginModule",
LoginModuleControlFlag.REQUISITE, Collections.emptyMap());
}
+
+ @Test
+ void testAllowedLoginModulesSystemProperty() {
+
+ // default
+ String jaasConfigProp1 = "com.ibm.security.auth.module.LdapLoginModule
required;";
+ configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp1);
+
+ String jaasConfigProp2 = "com.sun.security.auth.module.JndiLoginModule
required;";
+ // set allowed list, but not set disallowed list
+ System.setProperty(JaasUtils.ALLOWED_LOGIN_MODULES_CONFIG,
"com.ibm.security.auth.module.LdapLoginModule");
+ assertDoesNotThrow(() -> configurationEntry(JaasContext.Type.CLIENT,
jaasConfigProp1));
Review Comment:
we don't need `assertDoesNotThrow`, right?
##########
clients/src/test/java/org/apache/kafka/common/security/JaasContextTest.java:
##########
@@ -252,6 +254,32 @@ public void testDisallowedLoginModulesSystemProperty()
throws Exception {
checkEntry(context.configurationEntries().get(0),
"com.sun.security.auth.module.LdapLoginModule",
LoginModuleControlFlag.REQUISITE, Collections.emptyMap());
}
+
+ @Test
+ void testAllowedLoginModulesSystemProperty() {
+
+ // default
+ String jaasConfigProp1 = "com.ibm.security.auth.module.LdapLoginModule
required;";
+ configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp1);
+
+ String jaasConfigProp2 = "com.sun.security.auth.module.JndiLoginModule
required;";
+ // set allowed list, but not set disallowed list
+ System.setProperty(JaasUtils.ALLOWED_LOGIN_MODULES_CONFIG,
"com.ibm.security.auth.module.LdapLoginModule");
+ assertDoesNotThrow(() -> configurationEntry(JaasContext.Type.CLIENT,
jaasConfigProp1));
+ assertThrows(IllegalArgumentException.class, () ->
configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp2));
+
+ // set both allowed list and disallowed list
+ System.setProperty(JaasUtils.DISALLOWED_LOGIN_MODULES_CONFIG,
"com.ibm.security.auth.module.LdapLoginModule");
+ assertDoesNotThrow(() -> configurationEntry(JaasContext.Type.CLIENT,
jaasConfigProp1));
Review Comment:
ditto
##########
clients/src/test/java/org/apache/kafka/common/security/JaasContextTest.java:
##########
@@ -252,6 +254,32 @@ public void testDisallowedLoginModulesSystemProperty()
throws Exception {
checkEntry(context.configurationEntries().get(0),
"com.sun.security.auth.module.LdapLoginModule",
LoginModuleControlFlag.REQUISITE, Collections.emptyMap());
}
+
+ @Test
+ void testAllowedLoginModulesSystemProperty() {
+
+ // default
+ String jaasConfigProp1 = "com.ibm.security.auth.module.LdapLoginModule
required;";
+ configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp1);
Review Comment:
Should we test `throwIfLoginModuleIsNotAllowed` directly?
##########
clients/src/test/java/org/apache/kafka/common/security/JaasContextTest.java:
##########
@@ -252,6 +254,32 @@ public void testDisallowedLoginModulesSystemProperty()
throws Exception {
checkEntry(context.configurationEntries().get(0),
"com.sun.security.auth.module.LdapLoginModule",
LoginModuleControlFlag.REQUISITE, Collections.emptyMap());
}
+
+ @Test
+ void testAllowedLoginModulesSystemProperty() {
+
+ // default
+ String jaasConfigProp1 = "com.ibm.security.auth.module.LdapLoginModule
required;";
+ configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp1);
+
+ String jaasConfigProp2 = "com.sun.security.auth.module.JndiLoginModule
required;";
+ // set allowed list, but not set disallowed list
+ System.setProperty(JaasUtils.ALLOWED_LOGIN_MODULES_CONFIG,
"com.ibm.security.auth.module.LdapLoginModule");
+ assertDoesNotThrow(() -> configurationEntry(JaasContext.Type.CLIENT,
jaasConfigProp1));
+ assertThrows(IllegalArgumentException.class, () ->
configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp2));
+
+ // set both allowed list and disallowed list
+ System.setProperty(JaasUtils.DISALLOWED_LOGIN_MODULES_CONFIG,
"com.ibm.security.auth.module.LdapLoginModule");
+ assertDoesNotThrow(() -> configurationEntry(JaasContext.Type.CLIENT,
jaasConfigProp1));
+ assertThrows(IllegalArgumentException.class, () ->
configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp2));
+
+ // set disallowed list, but not set allowed list
+ System.clearProperty(JaasUtils.ALLOWED_LOGIN_MODULES_CONFIG);
+ IllegalArgumentException error =
assertThrows(IllegalArgumentException.class, () ->
configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp1));
+ // Ensure the exception message includes the deprecation warning for
the disallowed login modules config
+ assertTrue(error.getMessage().contains("The system property '" +
DISALLOWED_LOGIN_MODULES_CONFIG + "' is deprecated."));
+ assertDoesNotThrow(() -> configurationEntry(JaasContext.Type.CLIENT,
jaasConfigProp2));
Review Comment:
ditto
--
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]
