New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ldap): Support for handling DN based multiloaded roles #1058
feat(ldap): Support for handling DN based multiloaded roles #1058
Conversation
Minor refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. FWIW, we've been running this code internally at Salesforce for ~2 years(with enablePagingForGroupMembershipQueries: true
).
@kirangodishala would you add a release note for this please? Note that I believe the branches for 1.31 have already been cut, but it hasn't been released yet, so the release notes still mention 1.31, though I believe this will first appear in 1.32. |
* Fiat when logging in the user, eg. employee email. This attribute is used for creating a map | ||
* of the user dn to user id. | ||
*/ | ||
String userIdAttribute = "employeeEmail"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: the typical LDAP attribute for this is mail
, but the default probably doesn't matter that much.
public Pair<String, String> mapFromContext(Object ctx) { | ||
DirContextAdapter context = (DirContextAdapter) ctx; | ||
String userDN = | ||
LdapNameBuilder.newInstance(LdapUtils.parseRootDnFromUrl(configProps.getUrl())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for not using the deprecated DistinguishedName
here! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but the existing code still uses it!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's why I even noticed this change 😁
|
||
// This creates an "OR" filter of this form: | ||
// (|(employeeEmail=foo@mycompany.com)(employeeEmail=bar@mycompany.com)(employeeEmail=bax@mycompany.com)...) | ||
String userDNsFilter = String.format("(|%s)", String.join("", idFilters)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to refactor, but there's an LdapQueryBuilder
DSL in Spring that makes dynamic queries like this easier to assemble (and handles escaping strings properly which may or may not be relevant depending on the email addressed used here): https://docs.spring.io/spring-ldap/docs/2.4.1/api/org/springframework/ldap/query/LdapQueryBuilder.html
@Mergifyio update |
✅ Branch has been successfully updated |
…r#1058) * feat(ldap): Support for handling DN based multiloaded roles * feat(ldap): Support for handling DN based multiloaded roles. Minor refactor. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Addresses spinnaker/spinnaker#6841
And to enable pagination, below configuration is also needed: