This is an automated email from the ASF dual-hosted git repository.
mmarshall pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pulsar.git
The following commit(s) were added to refs/heads/master by this push:
new 00dc7a0691b [revert] "[fix][broker] Fix tenant admin authorization
bug. (#20068)" (#20143)
00dc7a0691b is described below
commit 00dc7a0691b496065dba6af0c6de64af4973886e
Author: Michael Marshall <[email protected]>
AuthorDate: Thu Apr 20 00:17:22 2023 -0500
[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.
---
.../broker/authorization/AuthorizationService.java | 27 +++++++++++++++++-----
.../pulsar/broker/auth/AuthorizationTest.java | 22 ++++++++----------
.../api/AuthorizationProducerConsumerTest.java | 16 -------------
.../websocket/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 a9225f5e48f..0c61219b57a 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.commons.lang3.StringUtils;
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 class AuthorizationService {
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 class AuthorizationService {
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 class AuthorizationService {
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<Boolean> 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 4fce7c50e1c..58cf4ee418e 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 class AuthorizationTest extends
MockedPulsarServiceBaseTest {
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 class AuthorizationTest extends
MockedPulsarServiceBaseTest {
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 dd351286d2e..0ce3b7df07d 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 class AuthorizationProducerConsumerTest extends
ProducerConsumerBase {
return
CompletableFuture.completedFuture(grantRoles.contains(role));
}
- @Override
- public CompletableFuture<Boolean> 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<Void> grantPermissionAsync(NamespaceName
namespace, Set<AuthAction> 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 29327d3d4eb..a3b26a4a9d1 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.ArgumentMatchers.anyString;
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 @@ public class ProxyAuthorizationTest extends
MockedPulsarServiceBaseTest {
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 class ProxyAuthorizationTest extends
MockedPulsarServiceBaseTest {
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");
