iloveeyjafjalla opened a new pull request, #11407:
URL: https://github.com/apache/rocketmq/pull/11407
### Which Issue(s) This PR Fixes
- Fixes #11406
### Brief Description
Retry topics are authorized through the consumer group that owns them:
`DefaultAuthorizationContextBuilder` maps `%RETRY%<group>` to the
`Group:<group>` resource in every request path that reads a caller-supplied
topic, and #11009 extended that rule to `VIEW_MESSAGE_BY_ID` on purpose.
`QUERY_MESSAGE` was the remaining read path without that mapping — it built
a **topic** resource unconditionally:
```java
case RequestCode.QUERY_MESSAGE:
topic = Resource.ofTopic(requireResource(fields.get(TOPIC), "topic"));
result.add(DefaultAuthorizationContext.of(subject, topic,
Arrays.asList(Action.SUB, Action.GET), sourceIp));
break;
```
Because `Resource.isMatch` never matches two different resource types
(`auth/.../model/Resource.java:111-113`), a role granted `Group:<group>` with
`SUB`/`GET` — the grant that lets the same user pull, view and query offsets of
that group's retry messages (cf. the `GET_MAX_OFFSET` and
`SEARCH_OFFSET_BY_TIMESTAMP` cases, which use the same `SUB, GET` actions and
do map retry topics) — could not run `mqadmin queryMsgByKey -t %RETRY%<group>
-k <key>`: the request was rejected although the caller is authorized for the
retry topic's group.
The change gives `QUERY_MESSAGE` the same shape `VIEW_MESSAGE_BY_ID` already
uses, so a retry topic resolves to its group and everything else is untouched:
```java
case RequestCode.QUERY_MESSAGE:
String queryMessageTopic = requireResource(fields.get(TOPIC), "topic");
Resource queryMessageResource =
NamespaceUtil.isRetryTopic(queryMessageTopic)
? Resource.ofGroup(queryMessageTopic) :
Resource.ofTopic(queryMessageTopic);
result.add(DefaultAuthorizationContext.of(subject, queryMessageResource,
Arrays.asList(Action.SUB, Action.GET), sourceIp));
break;
```
Scope note (verified while preparing this change): every other request code
in the builder that reads a topic either already maps retry topics
(`GET_ROUTEINFO_BY_TOPIC`, `SEND_MESSAGE`,
`SEND_MESSAGE_V2`/`SEND_BATCH_MESSAGE`, `VIEW_MESSAGE_BY_ID`, `PULL_MESSAGE`,
`LITE_PULL_MESSAGE`, `QUERY_CONSUMER_OFFSET`, `UPDATE_CONSUMER_OFFSET`,
`LOCK_BATCH_MQ`, `UNLOCK_BATCH_MQ`, `HEART_BEAT`, the delete/list admin paths)
or cannot legitimately carry one (`SEND_REPLY_MESSAGE`,
`SEND_REPLY_MESSAGE_V2`, `RECALL_MESSAGE`, `END_TRANSACTION`,
`QUERY_ASSIGNMENT`, `SET_MESSAGE_REQUEST_MODE`,
`UPDATE_AND_CREATE_STATIC_TOPIC`); `BATCH_ACK_MESSAGE` and
`CHECK_CLIENT_CONFIG` additionally authorize the consumer group carried by the
same request, so they are not part of this change. No documentation change is
needed: the retry-topic-to-group rule is already the documented behaviour of
the sibling paths.
### How Did You Test This Change?
New test
`DefaultAuthorizationContextBuilderTest#buildQueryMessageRequestsForTopicAndRetryTopic`:
it builds a `QUERY_MESSAGE` request for a plain topic and for `%RETRY%group`
and asserts the resource and actions of each context (`Topic:topic` with `SUB,
GET`; `Group:group` with `SUB, GET`).
Results on Temurin JDK 8 (the JDK CI uses), base `a7edeecfb` plus this patch:
- before the fix: `org.junit.ComparisonFailure: expected:<[Group:]group> but
was:<[Topic:%RETRY%]group>` (`Tests run: 1, Failures: 1`)
- after the fix: `DefaultAuthorizationContextBuilderTest` — 24 tests, 0
failures
- `mvn -B -ntp -pl auth package` (module tests + checkstyle + spotbugs
enabled): `BUILD SUCCESS`, 94 tests / 0 failures, `You have 0 Checkstyle
violations.`, `BugInstance size is 0`
This change was prepared with AI assistance (analysis, patch and test
drafting); the diff and every result above were reproduced locally by running
the module's tests against the base commit and the patched tree.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
