This is an automated email from the ASF dual-hosted git repository.
sijie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pulsar.git
The following commit(s) were added to refs/heads/master by this push:
new 2d197b0 Issue 2283: Improve error message if authorization is not
enabled (#2382)
2d197b0 is described below
commit 2d197b0e7e40106535d91ed6de82dd30241ff256
Author: Sijie Guo <[email protected]>
AuthorDate: Fri Aug 17 08:48:53 2018 -0700
Issue 2283: Improve error message if authorization is not enabled (#2382)
### Motivation
Fixes #2283.
If authorizationEnabled is not enabled, authorization service is not set.
All the accesses to this authorization service will throw NPE and clients will
receive Internal Server Error. The message is meaningless. It is better to
return a more meaningful message.
### Changes
Return `Not implemented` if authorization is not enabled.
---
.../pulsar/broker/admin/impl/NamespacesBase.java | 9 ++++++--
.../apache/pulsar/broker/admin/v1/Namespaces.java | 3 ++-
.../apache/pulsar/broker/admin/v2/Namespaces.java | 3 ++-
.../pulsar/client/admin/PulsarAdminException.java | 18 ++++++++++++----
.../pulsar/client/admin/internal/BaseResource.java | 5 ++---
.../pulsar/tests/integration/cli/CLITest.java | 24 ++++++++++++++++++++++
6 files changed, 51 insertions(+), 11 deletions(-)
diff --git
a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
index c4202c6..0d7e0ee 100644
---
a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
+++
b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
@@ -50,6 +50,7 @@ import org.apache.commons.lang3.StringUtils;
import org.apache.pulsar.broker.PulsarServerException;
import org.apache.pulsar.broker.ServiceConfiguration;
import org.apache.pulsar.broker.admin.AdminResource;
+import org.apache.pulsar.broker.authorization.AuthorizationService;
import
org.apache.pulsar.broker.service.BrokerServiceException.SubscriptionBusyException;
import org.apache.pulsar.broker.service.Subscription;
import org.apache.pulsar.broker.service.Topic;
@@ -297,9 +298,13 @@ public abstract class NamespacesBase extends AdminResource
{
validateAdminAccessForTenant(namespaceName.getTenant());
try {
- pulsar().getBrokerService().getAuthorizationService()
- .grantPermissionAsync(namespaceName, actions, role,
null/*additional auth-data json*/)
+ AuthorizationService authService =
pulsar().getBrokerService().getAuthorizationService();
+ if (null != authService) {
+ authService.grantPermissionAsync(namespaceName, actions, role,
null/*additional auth-data json*/)
.get();
+ } else {
+ throw new RestException(Status.NOT_IMPLEMENTED, "Authorization
is not enabled");
+ }
} catch (InterruptedException e) {
log.error("[{}] Failed to get permissions for namespace {}",
clientAppId(), namespaceName, e);
throw new RestException(e);
diff --git
a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/Namespaces.java
b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/Namespaces.java
index 6b607ab..b08bfb5 100644
---
a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/Namespaces.java
+++
b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/Namespaces.java
@@ -218,7 +218,8 @@ public class Namespaces extends NamespacesBase {
@ApiOperation(hidden = true, value = "Grant a new permission to a role on
a namespace.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have
admin permission"),
@ApiResponse(code = 404, message = "Property or cluster or
namespace doesn't exist"),
- @ApiResponse(code = 409, message = "Concurrent modification") })
+ @ApiResponse(code = 409, message = "Concurrent modification"),
+ @ApiResponse(code = 501, message = "Authorization is not
enabled")})
public void grantPermissionOnNamespace(@PathParam("property") String
property, @PathParam("cluster") String cluster,
@PathParam("namespace") String namespace, @PathParam("role")
String role, Set<AuthAction> actions) {
validateNamespaceName(property, cluster, namespace);
diff --git
a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
index 3a306e4..f58ec19 100644
---
a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
+++
b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
@@ -160,7 +160,8 @@ public class Namespaces extends NamespacesBase {
@ApiOperation(value = "Grant a new permission to a role on a namespace.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have
admin permission"),
@ApiResponse(code = 404, message = "Tenant or cluster or namespace
doesn't exist"),
- @ApiResponse(code = 409, message = "Concurrent modification") })
+ @ApiResponse(code = 409, message = "Concurrent modification"),
+ @ApiResponse(code = 501, message = "Authorization is not
enabled")})
public void grantPermissionOnNamespace(@PathParam("tenant") String tenant,
@PathParam("namespace") String namespace, @PathParam("role")
String role, Set<AuthAction> actions) {
validateNamespaceName(tenant, namespace);
diff --git
a/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/PulsarAdminException.java
b/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/PulsarAdminException.java
index 143e848..9961454 100644
---
a/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/PulsarAdminException.java
+++
b/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/PulsarAdminException.java
@@ -47,25 +47,25 @@ public class PulsarAdminException extends Exception {
public PulsarAdminException(ClientErrorException e) {
super(getReasonFromServer(e), e);
- this.httpError = e.getMessage();
+ this.httpError = getReasonFromServer(e);
this.statusCode = e.getResponse().getStatus();
}
public PulsarAdminException(ClientErrorException e, String message) {
super(message, e);
- this.httpError = e.getMessage();
+ this.httpError = getReasonFromServer(e);
this.statusCode = e.getResponse().getStatus();
}
public PulsarAdminException(ServerErrorException e) {
super(getReasonFromServer(e), e);
- this.httpError = e.getMessage();
+ this.httpError = getReasonFromServer(e);
this.statusCode = e.getResponse().getStatus();
}
public PulsarAdminException(ServerErrorException e, String message) {
super(message, e);
- this.httpError = e.getMessage();
+ this.httpError = getReasonFromServer(e);
this.statusCode = e.getResponse().getStatus();
}
@@ -75,6 +75,12 @@ public class PulsarAdminException extends Exception {
statusCode = DEFAULT_STATUS_CODE;
}
+ public PulsarAdminException(WebApplicationException e) {
+ super(getReasonFromServer(e), e);
+ this.httpError = getReasonFromServer(e);
+ this.statusCode = e.getResponse().getStatus();
+ }
+
public PulsarAdminException(String message, Throwable t) {
super(message, t);
httpError = null;
@@ -124,6 +130,10 @@ public class PulsarAdminException extends Exception {
}
public static class ServerSideErrorException extends PulsarAdminException {
+ public ServerSideErrorException(ServerErrorException e, String msg) {
+ super(e, msg);
+ }
+
public ServerSideErrorException(ServerErrorException e) {
super(e, "Some error occourred on the server");
}
diff --git
a/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java
b/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java
index 96671ec..25d622b 100644
---
a/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java
+++
b/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java
@@ -159,7 +159,7 @@ public abstract class BaseResource {
// Handle 5xx exceptions
if (e instanceof ServerErrorException) {
ServerErrorException see = (ServerErrorException) e;
- return new ServerSideErrorException(see);
+ return new ServerSideErrorException(see, e.getMessage());
} else if (e instanceof ClientErrorException) {
// Handle 4xx exceptions
ClientErrorException cee = (ClientErrorException) e;
@@ -176,12 +176,11 @@ public abstract class BaseResource {
return new ConflictException(cee);
case 412:
return new PreconditionFailedException(cee);
-
default:
return new PulsarAdminException(cee);
}
} else {
- return new PulsarAdminException(e);
+ return new PulsarAdminException((WebApplicationException) e);
}
} else {
return new PulsarAdminException(e);
diff --git
a/tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/CLITest.java
b/tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/CLITest.java
index e30a1e3..8526ded 100644
---
a/tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/CLITest.java
+++
b/tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/CLITest.java
@@ -18,6 +18,7 @@
*/
package org.apache.pulsar.tests.integration.cli;
+import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;
@@ -208,4 +209,27 @@ public class CLITest extends PulsarTestSuite {
result.getStdout());
}
+ // authorization related tests
+
+ @Test
+ public void testGrantPermissionsAuthorizationDisabled() throws Exception {
+ ContainerExecResult result;
+
+ String namespace = "grant-permissions-" + randomName(8);
+ result = pulsarCluster.createNamespace(namespace);
+ assertEquals(0, result.getExitCode());
+
+ String[] grantCommand = {
+ "namespaces", "grant-permission", "public/" + namespace,
+ "--actions", "produce",
+ "--role", "test-role"
+ };
+ try {
+ pulsarCluster.runAdminCommandOnAnyBroker(grantCommand);
+ } catch (ContainerExecException cee) {
+ result = cee.getResult();
+ assertTrue(result.getStderr().contains("HTTP 501 Not
Implemented"), result.getStderr());
+ }
+ }
+
}
