[
https://issues.apache.org/jira/browse/QPID-7318?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15403984#comment-15403984
]
Lorenz Quack commented on QPID-7318:
------------------------------------
Patch 8
* I think I am missing the point of {{AbstractConfiguredObjectProxy}}. I
cannot find is being used (extended, implemented, or mocked) anywhere. So how
would any of the {{if (source instanceof AbstractConfiguredObjectProxy)}}
checks ever trigger?
* {{FixedKeyMap#get}} and {{#containsKey}} have O(n ) time complexity which
certainly breaks my expectation of a Map implementation. I see that they are
only used for very small maps so it probabaly does not matter but still
surprising.
* {{FixedKeyMap$EntrySetIterator#next}} has a bug in the assignment of the
value.
Patch 9
* obsolete imports in {{CompoundAccessControl}}
* {{CompoundAccessControl#authorise}} returns {{DEFER}} instead of
{{_defaultResult}} if no underlying providers decide.
* The CompoundAccessControl in AbstractVirtualHost has at least 2
AccessControls - {{_systemUserAllowed}} and {{getParentAccessControl()}} -
which both will have default priority. Would it make sense to give
{{_systemUserAllowed}} a different default priority?
Patch 10 (Auto-generate acl checking for managed operations)
* {{ConfiguredObjectFactoryGenerator#processManagedOperation}} starts with a
semicolon
* It might be just me but I find this idiom ugly (and it is all over
{{ConfiguredObjectFactoryGenerator}}:{code}boolean first = true;
for(VariableElement param : methodElement.getParameters())
{
if (first)
{
first = false;
}
else
{
pw.print("\", \"");
}
pw.print(getParamName(param));
}{code} Unfortunately the only alternative I see in Java 7 is something like
this:{code}String s = Joiner.on("\",
\"").join(Iterables.transform(methodElement.getParameters(), new
Function<String, String>()
{
@Override
public String apply(final String input)
{
return input.toLowerCase();
}
}));
pw.print(s);{code} Not sure it is worth changing. Probably not.
Patch 11, 12
* No comment
Patch 13
* (VH-)AccessControllProvider#getPriority is missing description
Further comments
* There were to many style guide violations to mention. It's not even funny :(
* {{BDBVirtualHostImpl}} still uses calls to {{authorize(Operation.UPDATE)}}.
I believe these are covered by the automatic codegen and can die.
* Obsolete imports in : BDBVirtualHostNodeImpl, BDBVirtualHostImpl,
BDBHAVirtualHostNodeImpl, ConfiguredObjectFactoryGenerator,
BrokerFileLoggerImpl, BrokerMemoryLoggerImpl, VirtualHostFileLoggerImpl,
BrokerImpl, ... (I stopped tracking it)
* We could add a test for the broker upgrader (adding AllowAll
[iff|https://en.wikipedia.org/wiki/If_and_only_if] no ACL present)
* 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)
* 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
* 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.
* 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.
* The name {{RuleBasedAccessControlProvider}} is IMHO unfortunate. Yes, it is
based on rules but so is {{ACLFileAccessControlProvider}}
* I think {{AbstractCommonRulesBasedAccessControlProvider}} should implement
{{RuleBasedAccessControlProvider}}
* 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.
* {{Rule}} and {{AclRule}}. Are they really distinct? {{Rule}} seems to be
coupled to Acl by it having {{AclAction}} in it's public interface.
* 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.
* {{AbstractCommonRulesBasedAccessControlProvider}} should be renamed to
{{AbstractCommonRuleBasedAccessControlProvider}} (i.e., "Rule" instead of
"Rules")
* I would put "Legacy" into the name of the ManagedOperation
{{AbstractCommonRulesBasedAccessControlProvider#extractRules}}
* IMHO, {{AbstractCommonRulesBasedAccessControlProvider#getRules}} should
return a unmodifiableList
> [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]
