[ 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