[GitHub] [qpid-broker-j] alex-rufous edited a comment on issue #42: Qpid-8374: [Broker-J][ACL] Allow case insensitive mapping of group members to groups in existing GroupProvider

[GitBox <gi...@apache.org>]

alex-rufous edited a comment 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-558125705
 
 
   Hi Stanislav,
   
   I looked through the suggested changes and got the following review comments and questions:
   * FileBasedGroupProviderImpl does not extend AbstractGroupProvider. It seems for consistency sake it has to follow the rest of group provider implementations and extend AbstractGroupProvider
   * The default value for caseSensitive attribute in GroupProvider interface can be derived from context variable. I would like to add the context variable in GroupProvider interface as illustrated in a code snippet below:
   `String GROUP_PROVIDER_CASE_SENSITIVE = "qpid.groupProvider.caseSensitive";
   @SuppressWarnings("unused")
   @ManagedContextDefault(name = GROUP_PROVIDER_CASE_SENSITIVE)
   String DEFAULT_GROUP_PROVIDER_CASE_SENSITIVE = "true";
   @ManagedAttribute(defaultValue = "${" + GROUP_PROVIDER_CASE_SENSITIVE + "}", description = "Allow to choose CaseSensitive or CaseInsensitive search of Groups and Users")
   boolean isCaseSensitive();
   `
   * In FileGroupDatabaseCaseInsensitiveTest the test fields are initialized in static block. Is there any reason not to use junit @BeforeClass and @AfterClass methods to perform the initialization and clean up of test suite resources?
   * The same applies to FileGroupDatabaseTest; it seems initialization can be done @BeforeClass method and clean up in @AfterClass method.
   * It seems that case sensitive search seems is not applicable to CloudFoundryDashboardManagementGroupProvider. I am actually thinking that adding caseSensitive attribute there would be a mistake, as it will have no meaning there. I think we need an intermediate interface for GroupProvider supporting case insensitive group mapping. I think that it would be better to move attribute caseSensitive into special GroupProvider interface and extend FileBasedGroupProvider and GroupProviderImpl from that interface. Whilst CloudFoundryDashboardManagementGroupProvider can continue to extend original GroupProvider interface. That means that we need to change the UI as attribute "caseSensitive" will no longer be a common attribute. What do you think?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org