michaeljmarshall commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073983457
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java:
##########
@@ -435,6 +435,55 @@ public void testConnectCommandWithAuthenticationPositive()
throws Exception {
channel.finish();
}
+ @Test(timeOut = 30000)
+ public void testConnectCommandWithInvalidRoleCombinations() throws
Exception {
+ AuthenticationService authenticationService =
mock(AuthenticationService.class);
+ AuthenticationProvider authenticationProvider = new
BasicCommandAuthenticationProvider();
+ String authMethodName = authenticationProvider.getAuthMethodName();
+
+
when(brokerService.getAuthenticationService()).thenReturn(authenticationService);
+
when(authenticationService.getAuthenticationProvider(authMethodName)).thenReturn(authenticationProvider);
+ svcConfig.setAuthenticationEnabled(true);
+ svcConfig.setAuthorizationEnabled(true);
+ svcConfig.setProxyRoles(Collections.singleton("proxy"));
+
+ resetChannel();
+ assertTrue(channel.isActive());
+ assertEquals(serverCnx.getState(), State.Start);
+
+ // Invalid combinations where authData is proxy role
+ verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy",
"proxy");
+ verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy",
"");
+ verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy",
null);
+ // Invalid combinations where original principal is set to a proxy role
+ verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "",
"proxy");
+ verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, null,
"proxy");
+ verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "client",
"proxy");
+ // Invalid combinations where the original principal is set to a
non-proxy role
+ verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "",
"some_user");
+ verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, null,
"some_user");
+ verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "client",
"some_user");
+ }
+
+ private void verifyAuthRoleAndOriginalPrincipalBehavior(String
authMethodName, String authData,
+ String
originalPrincipal) throws Exception {
+ resetChannel();
+ assertTrue(channel.isActive());
+ assertEquals(serverCnx.getState(), State.Start);
+
+ ByteBuf clientCommand = Commands.newConnect(authMethodName, authData,
1,null,
+ null, originalPrincipal, null, null);
+ channel.writeInbound(clientCommand);
+
+ Object response1 = getResponse();
+ assertTrue(response1 instanceof CommandConnected);
Review Comment:
In order to get this test to pass, I needed to add this verification because
we send the response first. It'd be ideal to only send the `CommandError`, if
possible.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -965,15 +961,23 @@ protected void handleConnect(CommandConnect connect) {
remoteAddress, originalPrincipal);
}
}
+ if (service.isAuthorizationEnabled()) {
+ validateRoleAndOriginalPrincipal();
+ }
Review Comment:
It's not always correct to verify this here. Specifically, the
authentication might not be complete yet in the case of SASL.
--
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]
