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

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

{quote}
Further comments
 * There were to many style guide violations to mention. It's not even funny :(
{quote}
As discussed offline, the style file is not an agreed / voted standard of which 
deviations can be considered "violations".  I'll update my IntelliJ to use the 
style file, but other than brace conventions, naming and standard indentation, 
I don't think we have ever considered the style as a rule.
{quote}
 * {{BDBVirtualHostImpl}} still uses calls to {{authorize(Operation.UPDATE)}}. 
I believe these are covered by the automatic codegen and can die.
{quote}
Fixed
{quote} 
 * Obsolete imports in : BDBVirtualHostNodeImpl, BDBVirtualHostImpl, 
BDBHAVirtualHostNodeImpl, ConfiguredObjectFactoryGenerator, 
BrokerFileLoggerImpl, BrokerMemoryLoggerImpl, VirtualHostFileLoggerImpl, 
BrokerImpl, ... (I stopped tracking it)
{quote}
I used IntelliJ to fix all redundant imports and checked this in as a NO-JIRA 
change.
{quote}
 * We could add a test for the broker upgrader (adding AllowAll 
[iff|https://en.wikipedia.org/wiki/If_and_only_if] no ACL present)
{quote}
We could, however actually the broker works perfectly well without any Acl 
implementations :-) Having the AllowAll there is really just to highlight the 
default operation
{quote}
 * In various {{authorizePublish}} methods in the 1.0 protocol layer you create 
ad hoc maps instead of reusing the FixMapCreator (also not passing the 
"immediate" argument)
{quote}
I use a singletonMap as it is as efficient (space and performance wise) as a 
fixed map.  The reason the immediate argument is not passed is because there is 
no such thing as "immediate" in 1.0.
{quote}
 * To me it is a bit unclear (it should probably be documented somewhere or at 
least the knowledge should be widely distributed in the team) when you need to 
call {{authorize}} explicitly and when it is handled automatically
{quote}
For 6.2 I think we are going to need to do more work here - particularly in 
refining exactly how operations might map to coarser grained permissions, and 
whether we want to ad annotations for methods that are called not by REST but 
by the connection and require permissions.  Right now the basic CRUD operations 
on configured objects are automatically authorized (this was the case before 
this change too).  WIth this change, calls to ManagedOperations result in calls 
to the ACL modules.  Explicit calls to authorize are still required for 
publishing messages, etc, because these come from the connection
{quote} 
* The WMC lacks the ability to edit AccessControlProvider (priority, ACL path, 
rules). I understand this is not strictly part of this JIRA since it was mainly 
about refactoring existing functionality but we might want to capture this lack 
somehow.
{quote}
Yeah - that was deliberately out of scope for this JIRA / 6.1.  We'll look 
again at this aspect for 6.2
{quote}
 * We might want to consider renaming {{ACLFileAccessControlProvider(-Impl)}} 
to {{AclFileAccessControlProvider(-Impl)}} (i.e., lower-casing the "cl" in 
"Acl") to be more in line with the rest of the code.
{quote} 
Agreed (and done)
{quote}
* The name {{RuleBasedAccessControlProvider}} is IMHO unfortunate. Yes, it is 
based on rules but so is {{ACLFileAccessControlProvider}}
{quote}
I'd agree but thinking up names is hard :-)
{quote} 
 * I think {{AbstractCommonRulesBasedAccessControlProvider}} should implement 
{{RuleBasedAccessControlProvider}}
{quote}
nope - {{RuleBasedAccessControlProvider}} is a (Broker)AccessControlProvider, 
whereas  {{AbstractCommonRulesBasedAccessControlProvider}} is a base class for 
VHost and Broker access control providers.

{quote}
 * The class hierarchy seems a bit messy.
 ** Make CommonAccessControlProvider#getPriority a  ManagedAttribute and remove 
them from VHACP and ACP at which point they are simple marker interfaces.
 ** rename ACP to BrokerACP
 ** rename CommonACP to ACP
 ** It is a bit odd that {{RuleBasedAccessControlProvider}} and 
{{RuleBasesVirtualHostAccessControlProvider}} are virtually identical except 
for annotation content.
 ** {{AbstractCommonRuleBasedAccessControlProvider}} contains the 
implementation of all the methods of the {{RuleBased(VH)AccessControlProvider}} 
but does not inherit either.  I guess I understand the reason for it 
(semantically the children of ACRBACP are disjunct but it clouds the 
implementation (not semantic) relationship a little bit since we cannot use 
@Override
 ** Which part of {{AbstractLegacyAccessControlProvider}} is Legacy specific? 
The non-legacy {{AbstractCommonRulesBasedAccessControlProvider}} also extends 
this.
{quote}
** I don't think this works.  I don't think you can define a ManagedAttribute 
on a class that isn't inheriting from a managed object category - and COACP 
isn't - it's an interface extended by both Broker and VHost access control 
providers
** Renaming ACP to BrokerACP will change the category name which would break 
(badly) the REST API, which I don't think we should do in 6.1 (maybe for 7... 
or at a point where we have some backwards compatibility layer).
** The oddness with "identical but for annotation content" is just how it needs 
to be without a bigger change to how all the configured object meta data stuff 
works.
** Yeah - again this is just an artefact of the fact that the definitions of 
the managed object categories needs to be distinct.  The real answer is to 
allow the same categiry type at different places in the model hierarchy.  
** Rules based is also Legacy (all our ACL providers except AllowAll are Legacy

{quote}
* {{Rule}} and {{AclRule}}. Are they really distinct? {{Rule}} seems to be 
coupled to Acl by it having {{AclAction}} in it's public interface.
{quote}
It's all related to the legacy implementations, yes.  AclRule is in a form 
which can be used by the REST API.  It looked sufficiently annoying to try to 
refactor the legacy code to use AclRule rather than Rule+AclAction that I 
didn't bother.
{quote}
 * In the most parts of the code the Acl* names refer to the legacy code. Here 
it seems to be reversed, the {{Rule}} and {{RuleSet}} classes are from the 
legacy world whereas the {{AclRule}} is from the "RuleBased" world.
{quote}
RuleBased is still legacy.  It is essentially using the same logic, only 
storing the rules in the config file and not externally (and thus better able 
to manage change).  For 6.2 we will be looking at new (non-Legacy) ways of 
expressing ACL intentions.
{quote}
 * {{AbstractCommonRulesBasedAccessControlProvider}} should be renamed to 
{{AbstractCommonRuleBasedAccessControlProvider}} (i.e., "Rule" instead of 
"Rules")
{quote}
Agreed
{quote}
 * I would put "Legacy" into the name of the ManagedOperation 
{{AbstractCommonRulesBasedAccessControlProvider#extractRules}}
{quote}
I disagree - as above both these providers are legacy, extracting the rules as 
a file doesn't change their legacy-ness
{quote} 
* IMHO, {{AbstractCommonRulesBasedAccessControlProvider#getRules}} should 
return a unmodifiableList
{quote}
We've generally implemented the get methods on configured objects as simple 
getters (see {{AbstractPort}} for example).  Obviously callers shouldn't be 
mutating the returned list (nor assuming that the returned value is mutable).  
We perhaps need a more general policy on what the getter methods should do 
(and, indeed, if we should actually just start code generating these things).


> [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: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

Reply via email to