From commits-return-12946-archive-asf-public=cust-asf.ponee.io@pulsar.incubator.apache.org Fri Aug 17 17:48:58 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id BFFAF180627 for ; Fri, 17 Aug 2018 17:48:57 +0200 (CEST) Received: (qmail 28975 invoked by uid 500); 17 Aug 2018 15:48:56 -0000 Mailing-List: contact commits-help@pulsar.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@pulsar.incubator.apache.org Delivered-To: mailing list commits@pulsar.incubator.apache.org Received: (qmail 28966 invoked by uid 99); 17 Aug 2018 15:48:56 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 17 Aug 2018 15:48:56 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 46F748060D; Fri, 17 Aug 2018 15:48:56 +0000 (UTC) Date: Fri, 17 Aug 2018 15:48:56 +0000 To: "commits@pulsar.apache.org" Subject: [incubator-pulsar] branch master updated: Issue 2283: Improve error message if authorization is not enabled (#2382) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <153452093612.10034.16995743779152939319@gitbox.apache.org> From: sijie@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: incubator-pulsar X-Git-Refname: refs/heads/master X-Git-Reftype: branch X-Git-Oldrev: 086cbbba0e8854ecbafc55bff9e93cb9a84ce797 X-Git-Newrev: 2d197b0e7e40106535d91ed6de82dd30241ff256 X-Git-Rev: 2d197b0e7e40106535d91ed6de82dd30241ff256 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated 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 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 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 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()); + } + } + }