This is an automated email from the ASF dual-hosted git repository. mattisonchao pushed a commit to branch branch-2.9 in repository https://gitbox.apache.org/repos/asf/pulsar.git
commit e1b207e698e1bc09fc08656f36dc2cd7fbda323a Author: Jiwei Guo <[email protected]> AuthorDate: Sat Mar 12 11:20:57 2022 +0800 Fix wrong prompt exception when get non-persistent topic list without GET_BUDNLE permission (#14638) Fixes #14191 ### Motivation We have some big issues with the permission part. We only have the permission with [doc](https://pulsar.apache.org/docs/en/admin-api-permissions/) mentioned. But if user do it according to the doc, they will face the same issue that #14191 described. We don't have GET_BUNDLE in the grant interface but given the prompt message to the user. And currently, only the admin role could have the permission. This pr is not solving the permission issue but fixing the prompt message first, not giving 500 error to the user. Then I open an issue https://github.com/apache/pulsar/issues/14639 to discuss refactoring the permission part. ### Modification - Return 403 to the user when permission is denied. (cherry picked from commit ca6e82422ecbc5272d3dda2c90873c5a493764e1) --- .../broker/admin/v1/NonPersistentTopics.java | 25 +++++++------- .../broker/admin/v2/NonPersistentTopics.java | 31 ++++++++--------- .../pulsar/broker/auth/AuthorizationTest.java | 39 ++++++++++++++++++++-- 3 files changed, 62 insertions(+), 33 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/NonPersistentTopics.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/NonPersistentTopics.java index 2a5ab779097..68a6c719942 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/NonPersistentTopics.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/NonPersistentTopics.java @@ -198,6 +198,7 @@ public class NonPersistentTopics extends PersistentTopics { Policies policies = null; NamespaceName nsName = null; try { + validateNamespaceName(property, cluster, namespace); validateNamespaceOperation(namespaceName, NamespaceOperation.GET_TOPICS); policies = getNamespacePolicies(property, cluster, namespace); nsName = NamespaceName.get(property, cluster, namespace); @@ -232,22 +233,19 @@ public class NonPersistentTopics extends PersistentTopics { } } - final List<String> topics = Lists.newArrayList(); - FutureUtil.waitForAll(futures).handle((result, exception) -> { - for (int i = 0; i < futures.size(); i++) { - try { - if (futures.get(i).isDone() && futures.get(i).get() != null) { - topics.addAll(futures.get(i).get()); + FutureUtil.waitForAll(futures).whenComplete((result, ex) -> { + if (ex != null) { + resumeAsyncResponseExceptionally(asyncResponse, ex); + } else { + final List<String> topics = Lists.newArrayList(); + for (int i = 0; i < futures.size(); i++) { + List<String> topicList = futures.get(i).join(); + if (topicList != null) { + topics.addAll(topicList); } - } catch (InterruptedException | ExecutionException e) { - log.error("[{}] Failed to get list of topics under namespace {}/{}/{}", clientAppId(), property, - cluster, namespace, e); - asyncResponse.resume(new RestException(e instanceof ExecutionException ? e.getCause() : e)); - return null; } + asyncResponse.resume(topics); } - asyncResponse.resume(topics); - return null; }); } @@ -264,6 +262,7 @@ public class NonPersistentTopics extends PersistentTopics { @PathParam("bundle") String bundleRange) { log.info("[{}] list of topics on namespace bundle {}/{}/{}/{}", clientAppId(), property, cluster, namespace, bundleRange); + validateNamespaceName(property, cluster, namespace); validateNamespaceOperation(namespaceName, NamespaceOperation.GET_BUNDLE); Policies policies = getNamespacePolicies(property, cluster, namespace); if (!cluster.equals(Constants.GLOBAL_CLUSTER)) { diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/NonPersistentTopics.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/NonPersistentTopics.java index fa52cb45997..38b1d725a70 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/NonPersistentTopics.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/NonPersistentTopics.java @@ -403,26 +403,23 @@ public class NonPersistentTopics extends PersistentTopics { } } - final List<String> topics = Lists.newArrayList(); - FutureUtil.waitForAll(futures).handle((result, exception) -> { - for (int i = 0; i < futures.size(); i++) { - try { - if (futures.get(i).isDone() && futures.get(i).get() != null) { - topics.addAll(futures.get(i).get()); + FutureUtil.waitForAll(futures).whenComplete((result, ex) -> { + if (ex != null) { + resumeAsyncResponseExceptionally(asyncResponse, ex); + } else { + final List<String> topics = Lists.newArrayList(); + for (int i = 0; i < futures.size(); i++) { + List<String> topicList = futures.get(i).join(); + if (topicList != null) { + topics.addAll(topicList); } - } catch (InterruptedException | ExecutionException e) { - log.error("[{}] Failed to get list of topics under namespace {}", clientAppId(), namespaceName, e); - asyncResponse.resume(new RestException(e instanceof ExecutionException ? e.getCause() : e)); - return null; } + final List<String> nonPersistentTopics = + topics.stream() + .filter(name -> !TopicName.get(name).isPersistent()) + .collect(Collectors.toList()); + asyncResponse.resume(nonPersistentTopics); } - - final List<String> nonPersistentTopics = - topics.stream() - .filter(name -> !TopicName.get(name).isPersistent()) - .collect(Collectors.toList()); - asyncResponse.resume(nonPersistentTopics); - return null; }); } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthorizationTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthorizationTest.java index b7a54d137fd..4b18791fce0 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthorizationTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthorizationTest.java @@ -18,14 +18,17 @@ */ package org.apache.pulsar.broker.auth; +import static org.mockito.Mockito.when; +import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; import static org.testng.Assert.fail; - import java.util.EnumSet; - import org.apache.pulsar.broker.authorization.AuthorizationService; +import org.apache.pulsar.client.admin.PulsarAdmin; import org.apache.pulsar.client.admin.PulsarAdminBuilder; +import org.apache.pulsar.client.admin.PulsarAdminException; +import org.apache.pulsar.common.naming.TopicDomain; import org.apache.pulsar.common.naming.TopicName; import org.apache.pulsar.common.policies.data.AuthAction; import org.apache.pulsar.common.policies.data.ClusterData; @@ -36,7 +39,6 @@ import org.testng.Assert; import org.testng.annotations.AfterClass; import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; - import com.google.common.collect.Sets; @Test(groups = "flaky") @@ -230,6 +232,37 @@ public class AuthorizationTest extends MockedPulsarServiceBaseTest { admin.clusters().deleteCluster("c1"); } + @Test + public void testGetListWithoutGetBundleOp() throws Exception { + String tenant = "p1"; + String namespaceV1 = "p1/global/ns1"; + String namespaceV2 = "p1/ns2"; + admin.clusters().createCluster("c1", ClusterData.builder().build()); + admin.tenants().createTenant(tenant, new TenantInfoImpl(Sets.newHashSet("role1"), Sets.newHashSet("c1"))); + admin.namespaces().createNamespace(namespaceV1, Sets.newHashSet("c1")); + admin.namespaces().grantPermissionOnNamespace(namespaceV1, "pass.pass2", EnumSet.of(AuthAction.produce)); + admin.namespaces().createNamespace(namespaceV2, Sets.newHashSet("c1")); + admin.namespaces().grantPermissionOnNamespace(namespaceV2, "pass.pass2", EnumSet.of(AuthAction.produce)); + PulsarAdmin admin2 = PulsarAdmin.builder().serviceHttpUrl(brokerUrl != null + ? brokerUrl.toString() + : brokerUrlTls.toString()) + .authentication(new MockAuthentication("pass.pass2")) + .build(); + when(pulsar.getAdminClient()).thenReturn(admin2); + try { + admin2.topics().getList(namespaceV1, TopicDomain.non_persistent); + } catch (Exception ex) { + assertTrue(ex instanceof PulsarAdminException.NotAuthorizedException); + assertEquals(ex.getMessage(), "Unauthorized to validateNamespaceOperation for operation [GET_BUNDLE] on namespace [p1/global/ns1]"); + } + try { + admin2.topics().getList(namespaceV2, TopicDomain.non_persistent); + } catch (Exception ex) { + assertTrue(ex instanceof PulsarAdminException.NotAuthorizedException); + assertEquals(ex.getMessage(), "Unauthorized to validateNamespaceOperation for operation [GET_BUNDLE] on namespace [p1/ns2]"); + } + } + private static void waitForChange() { try { Thread.sleep(100);
