Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Broker] Remove tenant permission verification when list partitioned-topic #13138

Merged
merged 3 commits into from Dec 14, 2021
Merged

[Broker] Remove tenant permission verification when list partitioned-topic #13138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
269b024
[Broker] Remove tenant permission verification when list partitioned-…
yuruguo Dec 5, 2021
fe7ac30
Improve test
yuruguo Dec 6, 2021
12c3d5e
Update annotation
yuruguo Dec 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
View file
Open in desktop
Expand Up @@ -172,7 +172,6 @@ protected List<String> internalGetList() {
}

protected List<String> internalGetPartitionedTopicList() {
validateAdminAccessForTenant(namespaceName.getTenant());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to remove this line, we should update the annotations on the two methods that call internalGetPartitionedTopicList().

Copy link
Contributor Author

@yuruguo yuruguo Dec 8, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
In addition, we should also update the annotations on the method that call internalGetList().

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

Lines 149 to 150 in 01d73aa

protected List<String> internalGetList() {
validateNamespaceOperation(namespaceName, NamespaceOperation.GET_TOPICS);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

validateNamespaceOperation(namespaceName, NamespaceOperation.GET_TOPICS);
// Validate that namespace exists, throws 404 if it doesn't exist
try {
Expand Down
4 changes: 2 additions & 2 deletions pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/PersistentTopics.java
View file
Open in desktop
Expand Up @@ -69,7 +69,7 @@ public class PersistentTopics extends PersistentTopicsBase {
@Path("/{property}/{cluster}/{namespace}")
@ApiOperation(hidden = true, value = "Get the list of topics under a namespace.",
response = String.class, responseContainer = "List")
@ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin or consume permission on namespace"),
@ApiResponse(code = 404, message = "Namespace doesn't exist")})
public void getList(@Suspended final AsyncResponse asyncResponse, @PathParam("property") String property,
@PathParam("cluster") String cluster, @PathParam("namespace") String namespace) {
Expand All @@ -87,7 +87,7 @@ public void getList(@Suspended final AsyncResponse asyncResponse, @PathParam("pr
@Path("/{property}/{cluster}/{namespace}/partitioned")
@ApiOperation(hidden = true, value = "Get the list of partitioned topics under a namespace.",
response = String.class, responseContainer = "List")
@ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin or consume permission on namespace"),
@ApiResponse(code = 404, message = "Namespace doesn't exist")})
public List<String> getPartitionedTopicList(@PathParam("property") String property,
@PathParam("cluster") String cluster, @PathParam("namespace") String namespace) {
Expand Down
4 changes: 2 additions & 2 deletions pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
View file
Open in desktop
Expand Up @@ -85,7 +85,7 @@ public class PersistentTopics extends PersistentTopicsBase {
response = String.class, responseContainer = "List")
@ApiResponses(value = {
@ApiResponse(code = 401, message = "Don't have permission to administrate resources on this tenant"),
@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponse(code = 403, message = "Don't have admin or consume permission on namespace"),
@ApiResponse(code = 404, message = "tenant/namespace/topic doesn't exit"),
@ApiResponse(code = 412, message = "Namespace name is not valid"),
@ApiResponse(code = 500, message = "Internal server error")})
Expand All @@ -111,7 +111,7 @@ public void getList(
response = String.class, responseContainer = "List")
@ApiResponses(value = {
@ApiResponse(code = 401, message = "Don't have permission to administrate resources on this tenant"),
@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponse(code = 403, message = "Don't have admin or consume permission on namespace"),
@ApiResponse(code = 404, message = "tenant/namespace/topic doesn't exit"),
@ApiResponse(code = 412, message = "Namespace name is not valid"),
@ApiResponse(code = 500, message = "Internal server error")})
Expand Down
20 changes: 19 additions & 1 deletion ...-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import java.io.IOException;
Expand Down Expand Up @@ -365,6 +366,7 @@ public void testClearBacklogPermission() throws Exception {
conf.setAuthorizationProvider(PulsarAuthorizationProvider.class.getName());
setup();

final String tenantRole = "tenant-role";
final String subscriptionRole = "sub-role";
final String subscriptionName = "sub1";
final String namespace = "my-property/my-ns-sub-auth";
Expand All @@ -377,6 +379,11 @@ public void testClearBacklogPermission() throws Exception {
PulsarAdmin superAdmin = spy(PulsarAdmin.builder().serviceHttpUrl(brokerUrl.toString())
.authentication(adminAuthentication).build());

Authentication tenantAdminAuthentication = new ClientAuthentication(tenantRole);
@Cleanup
PulsarAdmin tenantAdmin = spy(PulsarAdmin.builder().serviceHttpUrl(brokerUrl.toString())
.authentication(tenantAdminAuthentication).build());

Authentication subAdminAuthentication = new ClientAuthentication(subscriptionRole);
@Cleanup
PulsarAdmin sub1Admin = spy(PulsarAdmin.builder().serviceHttpUrl(brokerUrl.toString())
Expand All @@ -385,9 +392,11 @@ public void testClearBacklogPermission() throws Exception {
superAdmin.clusters().createCluster("test",
ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
superAdmin.tenants().createTenant("my-property",
new TenantInfoImpl(Sets.newHashSet(), Sets.newHashSet("test")));
new TenantInfoImpl(Sets.newHashSet(tenantRole), Sets.newHashSet("test")));
superAdmin.namespaces().createNamespace(namespace, Sets.newHashSet("test"));
superAdmin.topics().createPartitionedTopic(topicName, 1);
assertEquals(tenantAdmin.topics().getPartitionedTopicList(namespace),
Lists.newArrayList(topicName));

// grant topic consume&produce authorization to the subscriptionRole
superAdmin.topics().grantPermission(topicName, subscriptionRole,
Expand Down Expand Up @@ -416,6 +425,13 @@ public void testClearBacklogPermission() throws Exception {
.get(subscriptionName).getMsgBacklog(), 10);

// subscriptionRole doesn't have namespace-level authorization, so it will fail to clear backlog
try {
sub1Admin.topics().getPartitionedTopicList(namespace);
fail("should have failed with authorization exception");
} catch (Exception e) {
assertTrue(e.getMessage().startsWith(
"Unauthorized to validateNamespaceOperation for operation [GET_TOPICS]"));
}
try {
sub1Admin.namespaces().clearNamespaceBundleBacklog(namespace, "0x00000000_0xffffffff");
fail("should have failed with authorization exception");
Expand All @@ -427,6 +443,8 @@ public void testClearBacklogPermission() throws Exception {
superAdmin.namespaces().grantPermissionOnNamespace(namespace, subscriptionRole,
Sets.newHashSet(AuthAction.consume));
// now, subscriptionRole have consume authorization on namespace, so it will successfully clear backlog
assertEquals(sub1Admin.topics().getPartitionedTopicList(namespace),
yuruguo marked this conversation as resolved.
Show resolved Hide resolved
Lists.newArrayList(topicName));
sub1Admin.namespaces().clearNamespaceBundleBacklog(namespace, "0x00000000_0xffffffff");
assertEquals(sub1Admin.topics().getStats(topicName + "-partition-0").getSubscriptions()
.get(subscriptionName).getMsgBacklog(), 0);
Expand Down