You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by ma...@apache.org on 2022/06/13 03:14:15 UTC
[pulsar] 01/02: Fix wrong prompt exception when get non-persistent topic list without GET_BUDNLE permission (#14638)
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 <te...@apache.org>
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);