You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/08/17 09:49:02 UTC
[GitHub] [druid] nikhil-ddu opened a new pull request #11606: Make ldap group attribute id configurable
nikhil-ddu opened a new pull request #11606:
URL: https://github.com/apache/druid/pull/11606
Fixes #11605 .
### Description
Added two new properties in ```LDAPCredentialsValidator``` and ```LDAPRoleProvider```
These two properties can be set in runtime properties in following way
```druid.auth.authenticator.<ldapauthenticatorname>.credentialsValidator.ldapGroupAttribute```
```druid.auth.authorizer.<ldapauthorizername>.roleProvider.ldapGroupAttribute```
Both properties will have default value of ```membeOf```.
If these properties are set by users explicitly, it should have same values.
<hr>
##### Key changed/added classes in this PR
* `BasicAuthLDAPConfig`
* `BasicAuthUtils`
* `LDAPCredentialsValidator`
* `LDAPRoleProvider`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [x] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [x] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s edited a comment on pull request #11606: Make ldap group attribute id configurable
Posted by GitBox <gi...@apache.org>.
suneet-s edited a comment on pull request #11606:
URL: https://github.com/apache/druid/pull/11606#issuecomment-916555363
Thanks for the contribution @nikhil-ddu !
Have you considered adding an integration test for these new parameters? I think `ITBasicAuthLdapConfigurationTest` is a potential place to add the test.
The docker compose file to set up the cluster for ldap tests appears to be in `docker-compose.ldap-security.yml`
There are some instructions on how to run / add integration tests here - https://github.com/apache/druid/blob/master/integration-tests/README.md
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #11606: Make ldap group attribute id configurable
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #11606:
URL: https://github.com/apache/druid/pull/11606#discussion_r705824174
##########
File path: docs/development/extensions-core/druid-basic-security.md
##########
@@ -181,6 +182,7 @@ The authorizer configuration examples in the rest of this document will use "MyB
|`druid.auth.authorizer.MyBasicLDAPAuthorizer.initialAdminGroupMapping`|The initial admin group mapping with role defined in initialAdminRole property if specified, otherwise the default admin role will be assigned. The name of this initial admin group mapping will be set to adminGroupMapping|null|No|
|`druid.auth.authorizer.MyBasicLDAPAuthorizer.roleProvider.type`|The type of role provider (ldap) to authorize requests credentials.|metadata|No
|`druid.auth.authorizer.MyBasicLDAPAuthorizer.roleProvider.groupFilters`|Array of LDAP group filters used to filter out the allowed set of groups returned from LDAP search. Filters can be begin with *, or end with ,* to provide configurational flexibility to limit or filter allowed set of groups available to LDAP Authorizer.|null|No|
+|`druid.auth.authorizer.MyBasicLDAPAuthorizer.roleProvider.ldapGroupAttribute`|The attribute id identifying the attribute used for LDAP groups by LDAP server. It should be same as druid.auth.authenticator.<ldap-authenticator-name>.ldapGroupAttribute|memberOf|No|
Review comment:
> It should be same as druid.auth.authenticator.<ldap-authenticator-name>.ldapGroupAttribute
Can we enforce this in the code to prevent someone setting this incorrectly? What happens if they are different?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nikhil-ddu commented on pull request #11606: Make ldap group attribute id configurable
Posted by GitBox <gi...@apache.org>.
nikhil-ddu commented on pull request #11606:
URL: https://github.com/apache/druid/pull/11606#issuecomment-916505049
> Do you consider pass ldapGroupAttribute using DynamicConfigProvider which is mentioned in [9351](https://github.com/apache/druid/issues/9351)? It can allow users pass ldapGroupAttribute by plain text or a use defined provider.
Since ldapGroupAttribute is not sensitive information and very similar to [userAttribute](https://github.com/apache/druid/blob/master/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/BasicAuthLDAPConfig.java#L31), I have followed implementation of [userAttribute](https://github.com/apache/druid/blob/master/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/BasicAuthLDAPConfig.java#L31).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11606: Make ldap group attribute id configurable
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11606:
URL: https://github.com/apache/druid/pull/11606#issuecomment-902454781
Do you consider pass ldapGroupAttribute using DynamicConfigProvider which is mentioned in [9351](https://github.com/apache/druid/issues/9351)? It can allow users pass ldapGroupAttribute by plain text or a use defined provider.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #11606: Make ldap group attribute id configurable
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #11606:
URL: https://github.com/apache/druid/pull/11606#issuecomment-916555363
Thanks for the contribution @nikhil-ddu !
Have you considered adding an integration test for these new parameters? I think `ITBasicAuthLdapConfigurationTest` is a potential place to add the test.
The docker compose file to set up the cluster for ldap tests appears to be in `docker-compose.ldap-security.yml`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org