iloveeyjafjalla opened a new pull request, #11419:
URL: https://github.com/apache/rocketmq/pull/11419
### Which Issue(s) This PR Fixes
- Fixes #11418
### Brief Description
`AuthMigrator` migrated every permission string it did not recognize into an
**allow-all** entry, because the decision only distinguished the literal `DENY`
from everything else:
```java
private Decision parseDecision(String str) {
if (StringUtils.isBlank(str)) {
return Decision.DENY;
}
return StringUtils.equals(str, AclConstants.DENY) ? Decision.DENY :
Decision.ALLOW;
}
```
ACL 1.0 — the implementation this migration replaces — was fail-closed for
the same input: `Permission.parsePermFromString` returned `Permission.DENY` for
`null` and in its `default` branch for every string outside `PUB`, `SUB`,
`PUB|SUB`, `SUB|PUB`, `DENY`, and its validator `checkResourcePerms` refused
such a config outright (`The expected permissions are 'SUB' or 'PUB' or
'SUB|PUB' or 'PUB|SUB'.`). So a v1 config entry that denied a resource (for
example `topicA=ANY`, a value that appears in the permission table of
`docs/en/acl/Operations_ACL.md`) was migrated into `Topic:topicA` with
`Action.ALL` and `Decision.ALLOW` — a fail-open escalation.
The change accepts only the four recognized allow values and denies
everything else, which reproduces ACL 1.0 semantics:
```java
private Decision parseDecision(String str) {
String perm = StringUtils.trim(str);
if (AclConstants.PUB.equals(perm) || AclConstants.SUB.equals(perm)
|| AclConstants.PUB_SUB.equals(perm) ||
AclConstants.SUB_PUB.equals(perm)) {
return Decision.ALLOW;
}
return Decision.DENY;
}
```
`parseActions` is intentionally left as it is: its default `Action.ALL` is
the list a *deny* entry carries (the same shape the existing
`AclConstants.DENY` case builds), so an unrecognized value now produces a
deny-everything entry for that resource — exactly what ACL 1.0 did with it.
`DENY` and blank values keep their current behaviour.
### How Did You Test This Change?
New test
`AuthMigratorTest#testMigrateUnrecognizedPermDoesNotAuthorizeTheResource`: it
migrates a `PlainAccessConfig` whose `topicPerms` contains `topicA=ANY`,
captures the `Acl` handed to `AuthorizationMetadataManager#createAcl` and
asserts that no policy entry authorizes anything.
Results on Temurin JDK 8 (the JDK CI uses), base `a7edeecfb` plus this patch:
- before the fix: `java.lang.AssertionError: an unrecognized v1 permission
must not authorize anything expected:<DENY> but was:<ALLOW>`
- after the fix: `AuthMigratorTest` — `Tests run: 4, Failures: 0, Errors: 0`
- `mvn -B -ntp -pl auth clean 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]
