pulsar-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From si...@apache.org
Subject [incubator-pulsar] branch master updated: Issue 2283: Improve error message if authorization is not enabled (#2382)
Date Fri, 17 Aug 2018 15:48:56 GMT
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 <guosijie@gmail.com>
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());
+        }
+    }
+
 }


Mime
View raw message