[
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: [email protected]
For additional commands, e-mail: [email protected]
