[jira] [Commented] (QPID-7318) [Java Broker] Refactor existing ACL plugin code

["Lorenz Quack (JIRA)" <ji...@apache.org>]

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