[
https://issues.apache.org/jira/browse/QPID-7318?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15399559#comment-15399559
]
Lorenz Quack commented on QPID-7318:
------------------------------------
I am slowly working myself through these changes. I am not finished yet but
here are my WIP comments so far so that they don't get lost in case I get hit
by that infamous bus:
Patch 1 (Refactor ACL implementation to separate concept of ruls \[sic\] based
ACL from parsing files)
* AclFileParser#getLine can die?
* en-/disableing of rules functionality removed?
* ACLFileAccessControlProviderImpl#onOpen can die
* RuleSetCreator#grant is an unfortunate name. #addRule seems more appropriate
Patch 2
* You don't seem to be using our IntelliJ style file :(
* should AbstractRuleBasedAccessControlProvider have a validateChange method
preventing changing the durability?
* ManagedAttributes/-Operations in RuleBasedAccessControlProvider don't have a
description
* There is Result.ALLOWED, Result.DENIED but Result.DEFER. Also there is
RuleOutcome.ALLOW and RuleOutcome.DENY. All a bit ugly and unobvious (for
example in RuleBasedAccessControlProvider you have DENIED as default value for
getDefaltResult and ALLOW in the defaultValue of getRule.
Patch 3
* The refactoring from AbstractAMQPConnection#isAuthorizedMessagePrincipal to
AbstractAMQPConnection#checkAuthorizedMessagePrincipal changed behaviour. e.g,
in the case where userId = null; authPrincipal = "";
* I am surprised the while-loop in CachingSecurityToken#authoriseMethod
terminates. I would have thought that cache contains a reference to the current
AccessControlCache instance. calls to CACHE_UPDATE.compareAndSet will (if
anything) change the _cache reference but leave the local cache reference
untouched. So why would the while-loop ever terminate? I would expect it to
look something like this {code}AccessControlCache cache;
while((cache = CACHE_UPDATE.get(this)).getAccessControl() !=
ruleBasedAccessControl)
{
CACHE_UPDATE.compareAndSet(this, cache, new
AccessControlCache(ruleBasedAccessControl));
}{code}
* comment in LegacyAccessControlAdapter#authorise seems out of place
Patch 4
* In ACO child creation is authorised in createChildAsync. In a couple of
places (e.g., BindingImpl) it is again checked in validateOnCreate. Is this by
design?
* we had this elsewhere but the way "priority" in AccessControlProvider and
ACCESS_CONTROL_PROVIDER_COMPARATOR work a low "priority" means it will take
precedence over a high "priority". common but not very intuitive. But I now see
that you mention this in the UI which is good.
* AccessControlSource seems to be only used in tests. Why was it split from
AccessControlProvider? Should CommonAccessControlProvider extend
AccessControlSource?
* Beside all the formating being of this instance deserves special mention:
The closing brace of CachingSecurityToken#authorise is on the wrong line.
* I have the same concern about the while-loop in
CachingSecurityToken#authorise as about the one in
CachingSecurityToken#authoriseMethod mentioned above
Patch 5
* SystemPrincipalSource is currently only used by tests. Should the
SystemConfig not be a SystemPrincipalSource?
Patch 6
* ConfigurationSecretEncrypterSource is not used.
Patch 7 (Show priority in web console, order children according to priority in
output)
* currently all AccessControlProvider have the default priority so they will
be ordered by name.
> [Java Broker] Refactor existing ACL plugin code
> -----------------------------------------------
>
> Key: QPID-7318
> URL: https://issues.apache.org/jira/browse/QPID-7318
> Project: Qpid
> Issue Type: Improvement
> Components: Java Broker
> Reporter: Rob Godfrey
> Assignee: Rob Godfrey
> Fix For: qpid-java-6.1
>
>
> While the aim is to redesign the ACL implementation in the v6.2 or v7.0
> timeframe, there is still utility in tidying up the existing ACL
> implementation a bit. In particular by separating out functions and
> providing a better encapsulation, we will make the job of writing automated
> upgraders to any new ACL implementation substantially easier.
> As a first step we can separate out the parsing of the ACL file, from the
> "rule based" implementation of ACLs.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
