[broker] Fix topic produced through REST not support Authorization #13771

liudezhi2098 · 2022-01-16T06:24:08Z

Master Issue: #13766

Motivation

fix topic produced through REST not support Authorization

Documentation

no-need-doc

mattisonchao · 2022-01-16T11:12:27Z

Hi , @liudezhi2098

Could you add some test for this change ?

liudezhi2098 · 2022-01-17T08:03:32Z

/pulsarbot run-failure-checks

…_support_Authorization

liudezhi2098 · 2022-01-17T11:16:45Z

/pulsarbot run-failure-checks

liudezhi2098 · 2022-01-17T23:46:03Z

/pulsarbot run-failure-checks

liudezhi2098 · 2022-01-18T09:13:41Z

/pulsarbot run-failure-checks

thomasechen · 2022-01-18T22:41:41Z

Hi @codelipenghui , The fix is critical to the existing community users who want to use the REST function. I suggest to add the fix as part of the release 2.9.2.

thomasechen · 2022-01-18T23:18:15Z

Hi @codelipenghui , The fix is critical to the existing community users who want to use the REST function. I suggest to add the fix as part of the release 2.9.2.

Sorry , I mean the users that already implemented the Pulsar in their environment

…_support_Authorization

codelipenghui · 2022-01-19T14:28:26Z

@thomasechen Yes, we can contain it in 2.9.2

Demogorgon314

Overall LGTM, just left a few comments about code style.

Demogorgon314 · 2022-01-20T01:56:17Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicsAuthTest.java

+    public void testProduceToNonPartitionedTopic(String token, int status) throws Exception {
+        admin.topics().createNonPartitionedTopic("persistent://" + testTenant + "/"
+                + testNamespace + "/" + testTopicName);
+        AsyncResponse asyncResponse = PowerMockito.mock(AsyncResponse.class);


The asyncResponse is never used, we can remove it.

Others asyncResponse can be removed either.

Demogorgon314 · 2022-01-20T02:01:40Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicsAuthTest.java

+        topics = spy(new Topics());
+        topics.setPulsar(pulsar);


topics never used, we can remove it.

gaoran10 · 2022-01-20T02:43:11Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicsAuthTest.java

+        admin.topics().createNonPartitionedTopic("persistent://" + testTenant + "/"
+                + testNamespace + "/" + testTopicName);
+        Schema<String> schema = StringSchema.utf8();
+        ProducerMessages producerMessages = new ProducerMessages();
+        producerMessages.setKeySchema(ObjectMapperFactory.getThreadLocal().
+                writeValueAsString(schema.getSchemaInfo()));
+        producerMessages.setValueSchema(ObjectMapperFactory.getThreadLocal().
+                writeValueAsString(schema.getSchemaInfo()));
+        String message = "[" +
+                "{\"key\":\"my-key\",\"payload\":\"RestProducer:1\",\"eventTime\":1603045262772,\"sequenceId\":1}," +
+                "{\"key\":\"my-key\",\"payload\":\"RestProducer:2\",\"eventTime\":1603045262772,\"sequenceId\":2}]";
+        producerMessages.setMessages(ObjectMapperFactory.getThreadLocal().readValue(message,
+                new TypeReference<List<ProducerMessage>>() {
+                }));
+
+        WebTarget root = buildWebClient();
+        Response response = root.path("/topics/persistent/" + testTenant + "/" + testNamespace + "/"
+                + testTopicName).request(MediaType.APPLICATION_JSON)
+                .header("Authorization", "Bearer " + token)
+                .post(Entity.json(producerMessages));
+        Assert.assertEquals(response.getStatus(), status);


There are some repeated code blocks, maybe we could try to reuse the same code block. It seems that only topic names are different.

liudezhi2098 · 2022-01-20T06:04:59Z

/pulsarbot run-failure-checks

…13771) (cherry picked from commit f551fdc)

thomasechen · 2022-01-25T08:04:15Z

@liudezhi2098

I have done the testing about producing the message through REST client. The HTTP client could produce the message successfully if I granted the authorization in the namespace or topic level. However, if I use the token which belongs to the tenant admin and there are no any permissions settings in the namespace and the topics level ,the system will refuse the HTTP client to produce the message to the topic and show the message under below.

"Unauthorized to produce to topic persistent://tls/demo4/test with clientAppId [tls] and authdata org.apache.pulsar.broker.authentication.AuthenticationDataHttps@318c6550"}"

Actually, if we use the client library (Java, Node), we can produce the message to the topic using the tenant admin token (the tenant admin role was set ) without granting additional permissions to the namespace or topics.

Reproduce :

I have a tenant called tls which admin role is also called "tls"
There is no permission settings related to the namespace tls/demo4
We use the token of the role "tls" to produce a message and succeed
We use "POSTMAN" to produce a message to the same topic
4.1 Authorization Settings: The token is the role "tls'

4.2 Message Body Detail
The system will give me the unauthorized message
After we grant the permission allowing producing message to the namespace tls/demo4, then we can successfully produce the message
6.1 Grant the permission to the role tls

6.2 Produce the message successfully

Conclusion:
REST authorization mechanism should be as same as the other clients (JAVA, NODEJS ......), We should be able to produce the message to the topic with the tenant admin token without any settings in the namespace and topics

fix_topic_produced_through_rest_not_support_Authorization

fab65f5

github-actions bot assigned liudezhi2098 Jan 16, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 16, 2022

add test for this TopicsAuthTest

c48e2a1

Merge branch 'apache:master' into fix_topic_produced_through_rest_not…

a1e4f80

…_support_Authorization

codelipenghui approved these changes Jan 18, 2022

View reviewed changes

codelipenghui added the release/2.9.2 label Jan 18, 2022

codelipenghui added this to the 2.10.0 milestone Jan 18, 2022

codelipenghui added release/2.9.3 and removed release/2.9.2 labels Jan 18, 2022

Merge branch 'apache:master' into fix_topic_produced_through_rest_not…

d462244

…_support_Authorization

codelipenghui added release/2.9.2 and removed release/2.9.3 labels Jan 19, 2022

codelipenghui requested review from MarvinCai, 315157973, gaoran10, hangc0276, merlimat, hezhangjian, Technoboy-, tuteng and zymap January 19, 2022 14:29

zymap approved these changes Jan 20, 2022

View reviewed changes

MarvinCai approved these changes Jan 20, 2022

View reviewed changes

Demogorgon314 reviewed Jan 20, 2022

View reviewed changes

hangc0276 approved these changes Jan 20, 2022

View reviewed changes

Remove unneeded code

72e7f44

hezhangjian approved these changes Jan 20, 2022

View reviewed changes

Remove unneeded code

1cd968a

Demogorgon314 approved these changes Jan 20, 2022

View reviewed changes

gaoran10 reviewed Jan 20, 2022

View reviewed changes

reuse the same code block

8cfbafd

gaoran10 approved these changes Jan 20, 2022

View reviewed changes

codelipenghui merged commit f551fdc into apache:master Jan 20, 2022

codelipenghui pushed a commit that referenced this pull request Jan 21, 2022

[broker] Fix topic produced through REST not support Authorization (#…

2dac261

…13771) (cherry picked from commit f551fdc)

codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jan 21, 2022

gaoran10 added the area/broker label Jan 25, 2022

tisonkun mentioned this pull request Jun 30, 2023

Pulsar 2.9 topic produced through REST not support Authorization #13766

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[broker] Fix topic produced through REST not support Authorization #13771

[broker] Fix topic produced through REST not support Authorization #13771

liudezhi2098 commented Jan 16, 2022

mattisonchao commented Jan 16, 2022

liudezhi2098 commented Jan 17, 2022

liudezhi2098 commented Jan 17, 2022

liudezhi2098 commented Jan 17, 2022

liudezhi2098 commented Jan 18, 2022

thomasechen commented Jan 18, 2022

thomasechen commented Jan 18, 2022

codelipenghui commented Jan 19, 2022

Demogorgon314 left a comment

Demogorgon314 Jan 20, 2022

Demogorgon314 Jan 20, 2022

Demogorgon314 Jan 20, 2022

gaoran10 Jan 20, 2022

liudezhi2098 commented Jan 20, 2022

thomasechen commented Jan 25, 2022 •

edited

Loading

[broker] Fix topic produced through REST not support Authorization #13771

[broker] Fix topic produced through REST not support Authorization #13771

Conversation

liudezhi2098 commented Jan 16, 2022

Motivation

Documentation

mattisonchao commented Jan 16, 2022

liudezhi2098 commented Jan 17, 2022

liudezhi2098 commented Jan 17, 2022

liudezhi2098 commented Jan 17, 2022

liudezhi2098 commented Jan 18, 2022

thomasechen commented Jan 18, 2022

thomasechen commented Jan 18, 2022

codelipenghui commented Jan 19, 2022

Demogorgon314 left a comment

Choose a reason for hiding this comment

Demogorgon314 Jan 20, 2022

Choose a reason for hiding this comment

Demogorgon314 Jan 20, 2022

Choose a reason for hiding this comment

Demogorgon314 Jan 20, 2022

Choose a reason for hiding this comment

gaoran10 Jan 20, 2022

Choose a reason for hiding this comment

liudezhi2098 commented Jan 20, 2022

thomasechen commented Jan 25, 2022 • edited Loading

thomasechen commented Jan 25, 2022 •

edited

Loading