[
https://issues.apache.org/jira/browse/ARTEMIS-3068?focusedWorklogId=536394&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-536394
]
ASF GitHub Bot logged work on ARTEMIS-3068:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 15/Jan/21 09:27
Start Date: 15/Jan/21 09:27
Worklog Time Spent: 10m
Work Description: brusdev commented on a change in pull request #3408:
URL: https://github.com/apache/activemq-artemis/pull/3408#discussion_r558117860
##########
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/settings/impl/HierarchicalObjectRepository.java
##########
@@ -484,17 +482,10 @@ public int compare(final String o1, final String o2) {
} else if (!o1.contains(singleWord) && o2.contains(singleWord)) {
return -1;
} else if (o1.contains(singleWord) && o2.contains(singleWord)) {
- String[] leftSplits = o1.split(quotedDelimiter);
- String[] rightSplits = o2.split(quotedDelimiter);
- for (int i = 0; i < leftSplits.length; i++) {
- String left = leftSplits[i];
- if (left.equals(singleWord)) {
- if (rightSplits.length < i ||
!rightSplits[i].equals(singleWord)) {
- return -1;
- } else {
- return +1;
- }
- }
+ int o1Count = StringUtils.countMatches(o1, singleWord);
+ int o2Count = StringUtils.countMatches(o2, singleWord);
+ if (o1Count != o2Count) {
+ return o1Count - o2Count;
Review comment:
The MatchComparator should take into account the position the
`singleWord` wild card too, ie `foo.*.too.*` is more specific than `*.too` in a
hierarchical sense.
----------------------------------------------------------------
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:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 536394)
Time Spent: 50m (was: 40m)
> Comparator implementation of HierarchicalObjectRepository issue
> ---------------------------------------------------------------
>
> Key: ARTEMIS-3068
> URL: https://issues.apache.org/jira/browse/ARTEMIS-3068
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Affects Versions: 2.9.0, 2.10.0, 2.10.1, 2.11.0, 2.12.0, 2.13.0, 2.14.0,
> 2.15.0, 2.16.0
> Reporter: Luis Miguel De Bello
> Priority: Major
> Fix For: 2.17.0
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> Hi, I am having an issue with the MatchComparator implemented in the
> HierarchicalObjectRepository class.
> Let me put an example on what's going on right now:
> I have created a class called DynamicRolesSecuritySettingPlugin implementing
> the interface SecuritySettingPlugin so that I can implement the
> setSecurityRepository method. There, I am adding some roles to the
> HierarchicalRepository based on a certificate. The HierarchicalRepository
> type T is Set<Role>. To summarize, the HierarchicalObjectRepository ends up
> having two different strings as key to the internal map (they are added with
> the addMatch method):
> - *.Provider.*.Agent.*.State
> - uswest.Provider.RR.Agent.*.State
> For the string "*.Provider.*.Agent.*.State", i end up setting a Role with
> permissions to publish but not to consume.
> For the string "uswest.Provider.RR.Agent.*.State", i end up setting a Role
> with permissions to consume.
> Now, when I create a consumer for a queue named
> uswest.Provider.RR.Agent.1.State (there is already a queue created with that
> name and with messages on it), I get an error on the consumer saying that I
> don't have the permission 'CONSUME' for queue
> uswest.Provider.RR.Agent.1.State.
> So I went ahead and debugged the code. I ended up in the class
> HierarchicalObjectRepository reading the getMatch method:
> @Override
> public T getMatch(final String match) {
> String modifiedMatch = matchModifier.modify(match);
> T cacheResult = cache.get(modifiedMatch);
> if (cacheResult != null) {
> return cacheResult;
> }
> lock.readLock().lock();
> try {
> T actualMatch;
> Map<String, Match<T>> possibleMatches =
> getPossibleMatches(modifiedMatch);
> Collection<Match<T>> orderedMatches = sort(possibleMatches);
> actualMatch = merge(orderedMatches);
> T value = actualMatch != null ? actualMatch : defaultmatch;
> if (value != null) {
> cache.put(modifiedMatch, value);
> }
> return value;
> } finally {
> lock.readLock().unlock();
> }
> }
> When I set a breakpoint in the getPossibleMatches I get the two entries
> correctly (both "*.Provider.*.Agent.*.State" and
> "uswest.Provider.RR.Agent.*.State"). Then when i see the sort method, i see
> that I do get first the "*.Provider.*.Agent.*.State" instead of the
> "uswest.Provider.RR.Agent.*State". As I said before, the role attached to
> "*.Provider.*.Agent.*.State" does not have permission to consume on the
> queue "uswest.Provider.RR.Agent.1.State" and that is the reason why I can't
> consume.
> When I looked at the comparator (MatchComparator) that decides which one
> should be first in the list, I see the following code:
> if (o1.contains(anyWords) && !o2.contains(anyWords)) {
> return +1;
> } else if (!o1.contains(anyWords) && o2.contains(anyWords)) {
> return -1;
> } else if (o1.contains(anyWords) && o2.contains(anyWords)) {
> return o2.length() - o1.length();
> } else if (o1.contains(singleWord) && !o2.contains(singleWord)) {
> return +1;
> } else if (!o1.contains(singleWord) && o2.contains(singleWord)) {
> return -1;
> } else if (o1.contains(singleWord) && o2.contains(singleWord)) {
> String[] leftSplits = o1.split(quotedDelimiter);
> String[] rightSplits = o2.split(quotedDelimiter);
> for (int i = 0; i < leftSplits.length; i++) {
> String left = leftSplits[i];
> if (left.equals(singleWord)) {
> if (rightSplits.length < i ||
> !rightSplits[i].equals(singleWord)) {
> return -1;
> } else {
> return +1;
> }
> }
> }
> }
> return o1.length() - o2.length();
> Since in this case both strings contains the "*" it goes through the last
> else if and this is what ends up doing the comparation:
>
> String[] leftSplits = o1.split(quotedDelimiter);
> String[] rightSplits = o2.split(quotedDelimiter);
> for (int i = 0; i < leftSplits.length; i++) {
> String left = leftSplits[i];
> if (left.equals(singleWord)) {
> if (rightSplits.length < i ||
> !rightSplits[i].equals(singleWord)) {
> return -1;
> } else {
> return +1;
> }
> }
> }
> This ends up returning the "*.Provider.*.Agent.*.State" when it shouldn't.
> There is a more specific string for the queue
> "uswest.Provider.RR.Agent.1.State".
> It's not taking into account the amount of "*" present in the string.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
