[ 
https://issues.apache.org/jira/browse/QPID-7318?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15399814#comment-15399814
 ] 

Rob Godfrey commented on QPID-7318:
-----------------------------------

h4. Patch 4
{quote}
* 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?
{quote}
Yes it is intentional - BindingImpls are not created in the "proper" way... 
there is much wrong in Bindings...
{quote}
* 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.
{quote}
Yeah - the way priority is being used is consistent with our general approach.  
"Number 1 priority" means very high priority.  This is just English being 
illogical.

{quote}
* AccessControlSource seems to be only used in tests. Why was it split from 
AccessControlProvider? Should CommonAccessControlProvider extend 
AccessControlSource?
{quote}
It is only used in tests and is a way of allowing mocks to implement functions 
that otherwise would require us to expose private methods as part of the public 
interface of the objects.  Having these interfaces is ugly, but I consider it 
way less offensive than making private implementation details public simply to 
allow for allowing the tests to mock objects.  Our non-test classes should 
never need to implement these interfaces as they inherit from 
AbstractConfiguredObject and thus the actual (private/protected) methods can be 
used.
{quote}
* 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
{quote}
Think this was actually what I fixed in the previous commit.

> [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]

Reply via email to