alex-rufous commented on issue #42: Qpid-8374: [Broker-J][ACL] Allow case insensitive mapping of group members to groups in existing GroupProvider URL: https://github.com/apache/qpid-broker-j/pull/42#issuecomment-558609977 > Hi Alex @alex-rufous, > I want to discuss the last point you have described, please. > If Case Sensitive only applied to GroupFile type it is better to replace new variable into FileBasedGroupProvider. > There will be 2 attributes: "path" & "caseSensitive". > And UI will be changed accordingly. Hi Stanislav, There are two implementations of GroupProvider interface where case insensitive group mapping can be used: - GroupFile (FileBasedGroupProviderImpl) - ManagedGroupProvider (GroupProviderImpl) Potentially, you can duplicate the changes in both implementations. Alternatively, you can implement changes in abstract class and extend corresponding implementations from that class. The latter seems a better approach from clean code point of view. You can start with a duplicate code and refactor the common code into a separate class later. I usually implemented my code in the way where the first implementation is "dirty"; I try to refactor the code to make it clean before commit. I would like to point out that FileBasedGroupProviderImpl should be deprecated (on some point in future) and removed from the Broker together with authentication providers implementing interface ExternalFileBasedAuthenticationManager (PlainPasswordFile, Base64MD5PasswordFile). Thus, FileBasedGroupProviderImpl can be left unchanged and the corresponding changes can be only added into GroupProviderImpl. As you have already added the relevant changes into both implementations, I though it would be easier just to keep the changes for FileBasedGroupProviderImpl. Regarding UI: for simplicity, you can duplicate the changes in UI for both providers. Otherwise, the UI might get more complicated than it should be. We can discuss the possible UI refactoring later. One review comment I have forgotten to mention previously: FileBasedGroupProviderImpl might need to have some logic around changing the value of caseSensitive attribute from managed interfaces (REST, AMQP management). With the implemented approach, the underlying database object needs to be recreated on changes to "caseSensitive". Alternatively, you can pass a reference to FileBasedGroupProvider interface into database implementation and call method isCaseSensitive() to perform the right look-up. Kind Regards, Alex
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
