-
Notifications
You must be signed in to change notification settings - Fork 304
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
HPCC-32711 Define LDAP security manager abstraction #19292
base: master
Are you sure you want to change the base?
HPCC-32711 Define LDAP security manager abstraction #19292
Conversation
Replace platform references to CLdapSecManager with references to the ILdapSecManager abstraction. Signed-off-by: Tim Klemm <[email protected]>
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32711 Jirabot Action Result: |
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.
Unfortunate that it has to be done this way. But looks reasonable to convert to an interface.
@ghalliday please merge. |
@ghalliday per our offline discussion today, this change is not necessary for any forthcoming changes. It may still be viewed as a code improvement that decouples the platform from a specific implementation of LDAP. Any security manager plugin that implements the interface can be used instead. Or it can just be closed. |
@timothyklemm do you think it is a worthwhile change to keep - I am happy either way. It does not seem right that anything outside the ldap implementation knows it is an ldap manager, but I suspect that is a far bigger factoring that is required. |
@ghalliday unifying the interfaces is a large undertaking. Russ wanted to do it. Ken and I would both like to see it happen. It's never been a high enough priority. As long as casting is necessary, I think casting to an interface is preferable to a single implementation. Also, the potential for use of the decorator pattern to control/enforce trace settings remains a possibility. I would keep this change. |
Replace platform references to CLdapSecManager with references to the ILdapSecManager abstraction.
Type of change:
Checklist:
Smoketest:
Testing: