From 38360d06ba16fbf6339d325ae200d603850b47ec Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Thu, 20 Apr 2023 00:17:22 -0500 Subject: [PATCH] [revert] "[fix][broker] Fix tenant admin authorization bug. (#20068)" (#20143) This reverts commit fc17c1d98a3c1edd975c131d174a9ef69887d9cd. ### Motivation In https://github.com/apache/pulsar/pull/20068 we changed the way that the `AuthorizationService` is implemented. I think this approach could have unintended consequences. Instead, I think we should change the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method. I propose an update to the `Consumer` and `Producer` logic here #20142. Given that our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, I think we should not change their implementations. ### Modifications * Revert https://github.com/apache/pulsar/pull/20068 ### Verifying this change This change is trivial. It removes certain test changes that were only made to make the previous PR work. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: Skipping PR as I ran tests locally. (cherry picked from commit 00dc7a0691b496065dba6af0c6de64af4973886e) --- .../authorization/AuthorizationService.java | 27 ++++++++++++++----- .../pulsar/broker/auth/AuthorizationTest.java | 22 +++++++-------- .../AuthorizationProducerConsumerTest.java | 16 ----------- .../proxy/ProxyAuthorizationTest.java | 14 +++------- 4 files changed, 33 insertions(+), 46 deletions(-) diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java index a9225f5e48fa9..0c61219b57a50 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java @@ -28,7 +28,6 @@ import org.apache.pulsar.broker.PulsarServerException; import org.apache.pulsar.broker.ServiceConfiguration; import org.apache.pulsar.broker.authentication.AuthenticationDataSource; -import org.apache.pulsar.broker.authentication.AuthenticationDataSubscription; import org.apache.pulsar.broker.authentication.AuthenticationParameters; import org.apache.pulsar.broker.resources.PulsarResources; import org.apache.pulsar.common.naming.NamespaceName; @@ -183,7 +182,13 @@ public CompletableFuture canProduceAsync(TopicName topicName, String ro if (!this.conf.isAuthorizationEnabled()) { return CompletableFuture.completedFuture(true); } - return provider.allowTopicOperationAsync(topicName, role, TopicOperation.PRODUCE, authenticationData); + return provider.isSuperUser(role, authenticationData, conf).thenComposeAsync(isSuperUser -> { + if (isSuperUser) { + return CompletableFuture.completedFuture(true); + } else { + return provider.canProduceAsync(topicName, role, authenticationData); + } + }); } /** @@ -202,9 +207,13 @@ public CompletableFuture canConsumeAsync(TopicName topicName, String ro if (!this.conf.isAuthorizationEnabled()) { return CompletableFuture.completedFuture(true); } - - return provider.allowTopicOperationAsync(topicName, role, TopicOperation.CONSUME, - new AuthenticationDataSubscription(authenticationData, subscription)); + return provider.isSuperUser(role, authenticationData, conf).thenComposeAsync(isSuperUser -> { + if (isSuperUser) { + return CompletableFuture.completedFuture(true); + } else { + return provider.canConsumeAsync(topicName, role, authenticationData, subscription); + } + }); } public boolean canProduce(TopicName topicName, String role, AuthenticationDataSource authenticationData) @@ -280,7 +289,13 @@ public CompletableFuture canLookupAsync(TopicName topicName, String rol if (!this.conf.isAuthorizationEnabled()) { return CompletableFuture.completedFuture(true); } - return provider.allowTopicOperationAsync(topicName, role, TopicOperation.LOOKUP, authenticationData); + return provider.isSuperUser(role, authenticationData, conf).thenComposeAsync(isSuperUser -> { + if (isSuperUser) { + return CompletableFuture.completedFuture(true); + } else { + return provider.canLookupAsync(topicName, role, authenticationData); + } + }); } public CompletableFuture allowFunctionOpsAsync(NamespaceName namespaceName, String role, 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 4fce7c50e1c44..58cf4ee418ea4 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 @@ -79,13 +79,10 @@ public void cleanup() throws Exception { public void simple() throws Exception { AuthorizationService auth = pulsar.getBrokerService().getAuthorizationService(); - try { - assertFalse(auth.canLookup(TopicName.get("persistent://p1/c1/ns1/ds1"), "my-role", null)); - fail("Should throw exception when tenant not exist"); - } catch (Exception ignored) {} + assertFalse(auth.canLookup(TopicName.get("persistent://p1/c1/ns1/ds1"), "my-role", null)); + admin.clusters().createCluster("c1", ClusterData.builder().build()); - String tenantAdmin = "role1"; - admin.tenants().createTenant("p1", new TenantInfoImpl(Sets.newHashSet(tenantAdmin), Sets.newHashSet("c1"))); + admin.tenants().createTenant("p1", new TenantInfoImpl(Sets.newHashSet("role1"), Sets.newHashSet("c1"))); waitForChange(); admin.namespaces().createNamespace("p1/c1/ns1"); waitForChange(); @@ -218,22 +215,21 @@ public void simple() throws Exception { SubscriptionAuthMode.Prefix); waitForChange(); - assertTrue(auth.canLookup(TopicName.get("persistent://p1/c1/ns1/ds1"), tenantAdmin, null)); + assertTrue(auth.canLookup(TopicName.get("persistent://p1/c1/ns1/ds1"), "role1", null)); assertTrue(auth.canLookup(TopicName.get("persistent://p1/c1/ns1/ds1"), "role2", null)); - // tenant admin can consume all topics, even if SubscriptionAuthMode.Prefix mode - assertTrue(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), tenantAdmin, null, "sub1")); + try { + assertFalse(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), "role1", null, "sub1")); + fail(); + } catch (Exception ignored) {} try { assertFalse(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), "role2", null, "sub2")); fail(); } catch (Exception ignored) {} - assertTrue(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), tenantAdmin, null, "role1-sub1")); + assertTrue(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), "role1", null, "role1-sub1")); assertTrue(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), "role2", null, "role2-sub2")); assertTrue(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), "pulsar.super_user", null, "role3-sub1")); - // tenant admin can produce all topics - assertTrue(auth.canProduce(TopicName.get("persistent://p1/c1/ns1/ds1"), tenantAdmin, null)); - admin.namespaces().deleteNamespace("p1/c1/ns1"); admin.tenants().deleteTenant("p1"); admin.clusters().deleteCluster("c1"); diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java index dd351286d2e5e..0ce3b7df07d1f 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java @@ -1035,22 +1035,6 @@ public CompletableFuture canLookupAsync(TopicName topicName, String rol return CompletableFuture.completedFuture(grantRoles.contains(role)); } - @Override - public CompletableFuture allowTopicOperationAsync(TopicName topic, String role, - TopicOperation operation, - AuthenticationDataSource authData) { - switch (operation) { - - case PRODUCE: - return canProduceAsync(topic, role, authData); - case CONSUME: - return canConsumeAsync(topic, role, authData, authData.getSubscription()); - case LOOKUP: - return canLookupAsync(topic, role, authData); - } - return super.allowTopicOperationAsync(topic, role, operation, authData); - } - @Override public CompletableFuture grantPermissionAsync(NamespaceName namespace, Set actions, String role, String authData) { diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/websocket/proxy/ProxyAuthorizationTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/websocket/proxy/ProxyAuthorizationTest.java index 29327d3d4ebe1..a3b26a4a9d122 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/websocket/proxy/ProxyAuthorizationTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/websocket/proxy/ProxyAuthorizationTest.java @@ -25,7 +25,6 @@ import static org.mockito.Mockito.doReturn; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; -import static org.testng.Assert.fail; import com.google.common.collect.Sets; import java.util.EnumSet; import java.util.Optional; @@ -84,13 +83,10 @@ protected void cleanup() throws Exception { public void test() throws Exception { AuthorizationService auth = service.getAuthorizationService(); - try { - assertFalse(auth.canLookup(TopicName.get("persistent://p1/c1/ns1/ds1"), "my-role", null)); - fail("Should throw exception when tenant not exist"); - } catch (Exception ignored) {} + assertFalse(auth.canLookup(TopicName.get("persistent://p1/c1/ns1/ds1"), "my-role", null)); + admin.clusters().createCluster(configClusterName, ClusterData.builder().build()); - String tenantAdmin = "role1"; - admin.tenants().createTenant("p1", new TenantInfoImpl(Sets.newHashSet(tenantAdmin), Sets.newHashSet("c1"))); + admin.tenants().createTenant("p1", new TenantInfoImpl(Sets.newHashSet("role1"), Sets.newHashSet("c1"))); waitForChange(); admin.namespaces().createNamespace("p1/c1/ns1"); waitForChange(); @@ -121,10 +117,6 @@ public void test() throws Exception { assertTrue(auth.canProduce(TopicName.get("persistent://p1/c1/ns1/ds1"), "my-role", null)); assertTrue(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), "my-role", null, null)); - // tenant admin can produce/consume all topics, even if SubscriptionAuthMode.Prefix mode - assertTrue(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), tenantAdmin, null, "sub1")); - assertTrue(auth.canProduce(TopicName.get("persistent://p1/c1/ns1/ds1"), tenantAdmin, null)); - admin.namespaces().deleteNamespace("p1/c1/ns1"); admin.tenants().deleteTenant("p1"); admin.clusters().deleteCluster("c1");