RongtongJin commented on a change in pull request #1985:
URL: https://github.com/apache/rocketmq/pull/1985#discussion_r431088551
##########
File path:
broker/src/main/java/org/apache/rocketmq/broker/processor/AbstractSendMessageProcessor.java
##########
@@ -173,6 +173,12 @@ protected RemotingCommand msgCheck(final
ChannelHandlerContext ctx,
return response;
}
+ if
(TopicValidator.RMQ_SYS_SCHEDULE_TOPIC.equals(requestHeader.getTopic())) {
+ response.setCode(ResponseCode.NO_PERMISSION);
+ response.setRemark("sending message to topic[" +
requestHeader.getTopic() + "] is forbidden");
+ return response;
+ }
+
Review comment:
Is this piece of code necessary? The following code will check system
topic again.
##########
File path:
common/src/main/java/org/apache/rocketmq/common/topic/TopicValidator.java
##########
@@ -57,13 +85,24 @@ public static boolean validateTopic(String topic,
RemotingCommand response) {
return false;
}
- //whether the same with system reserved keyword
- if (topic.equals(MixAll.AUTO_CREATE_TOPIC_KEY_TOPIC)) {
+ if (isSystemTopic(topic)) {
response.setCode(ResponseCode.SYSTEM_ERROR);
- response.setRemark("The specified topic is conflict with
AUTO_CREATE_TOPIC_KEY_TOPIC.");
+ response.setRemark("The topic[" + topic + "] is conflict with
system topic.");
return false;
}
Review comment:
[Important] There is a serious problem. msgCheck call this method and
check all system topics, this will cause the client to fail to send messages to
**all system topics**, but it is necessary for the client to send messages to
certain system topics, such as BenchmarkTest, OFFSET_MOVED_EVENT. IMO, We need
a blacklist to prohibit sending to certain system topics, but not all system
topics.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
