You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/05/25 17:19:47 UTC
[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639000237
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,33 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
Review comment:
We should check multi is implemented and then call the appropriated method. This change will cause users who have custom implementations to always throw an exception
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -63,7 +66,11 @@
* if the credentials are not valid
*/
default String authenticate(AuthenticationDataSource authData) throws AuthenticationException {
- throw new AuthenticationException("Not supported");
+ return authenticate(authData, false).get(0);
Review comment:
Keep the AuthenticationException("Not supported").
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,33 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
+ } catch (AuthenticationException e) {
+ if (e.getMessage().equals(MULTI_ROLE_NOT_SUPPORTED)) {
+ this.authRoles = Lists.newArrayList(provider.authenticate(authenticationDataSource));
+ } else {
+ throw e;
+ }
+ }
}
@Override
public String getAuthRole() {
- return authRole;
+ return authRoles.get(0);
Review comment:
What is the list is empty or null?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -346,8 +348,18 @@ private boolean invalidOriginalPrincipal(String originalPrincipal) {
} else {
isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
}
- isAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, authRole, authenticationData);
+ isAuthorizedFuture = CompletableFuture.supplyAsync(() -> {
+ for (String authRole : authRoles) {
+ try {
+ if (service.getAuthorizationService().allowTopicOperationAsync(
+ topicName, operation, authRole, authenticationData).get()) {
+ return true;
+ }
+ } catch (InterruptedException | ExecutionException e) {
Review comment:
Will this hide any errors we could be interested in?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java
##########
@@ -37,6 +39,10 @@
*/
String getAuthRole() throws AuthenticationException;
+ default List<String> getAuthRoles() throws AuthenticationException {
+ return Collections.singletonList(getAuthRole());
Review comment:
What if the getAuthRole returns null or empty string I think this should return an empty list
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org