-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(frontend)_Fetch_user_groups_via_LDAP_login_API #14918
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
base: master
Are you sure you want to change the base?
feat(frontend)_Fetch_user_groups_via_LDAP_login_API #14918
Conversation
Bundle ReportChanges will decrease total bundle size by 81 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
|
4dee461
to
b975849
Compare
/build |
/build pr |
b975849
to
5233d38
Compare
5233d38
to
f7bd56a
Compare
Hi @deepgarg760 the test case is not related to new test cases which are done in AuthenticationManagerTest but from ApplicationTest STANDARD_OUT
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
public static Hashtable<String, Object> createLdapEnvironment( | ||
@Nonnull Map<String, String> options, @Nonnull String username, @Nonnull String password) { | ||
|
||
Hashtable<String, Object> env = new Hashtable<>(); |
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.
any specific reason using hashTable instead of hashmap ?
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.
public static DirContext createLdapContext(Hashtable<String, Object> env) throws NamingException {
return new InitialDirContext(env);
}
Java's LDAP API (InitialDirContext) requires a Hashtable for environment properties hence used it instead of hashMap.
fdc6934
to
6a4cd05
Compare
6a4cd05
to
a5d7bb8
Compare
@Test | ||
public void testCreateLdapEnvironmentWithBasicOptions() { | ||
String username = "testuser"; | ||
String password = "testpass"; |
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.
Exposed secret in datahub-frontend/test/auth/ldap/LdapConnectionUtilTest.java - low severity
Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
View details in Aikido Security
|
||
testUserDN = "CN=TestUser,OU=Users,DC=example,DC=com"; | ||
testUsername = "testuser"; | ||
testPassword = "testpass"; |
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.
Exposed secret in datahub-frontend/test/auth/ldap/LdapUserAttributeExtractorTest.java - low severity
Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
View details in Aikido Security
new LdapProvisioningLogic(mockSystemEntityClient, mockSystemOperationContext); | ||
|
||
testUsername = "testuser"; | ||
testPassword = "testpass"; |
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.
Exposed secret in datahub-frontend/test/auth/ldap/LdapProvisioningLogicTest.java - low severity
Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
View details in Aikido Security
I would expect similar configuration options for enabling group provisioning and extraction. Something like this to match the OIDC versions: AUTH_LDAP_JIT_PROVISIONING_ENABLED and AUTH_LDAP_EXTRACT_GROUPS_ENABLED (maybe take a look at the other ones to see if any apply) |
We should also update the documentation around ldap+jaas, maybe adding to this document. |
The changes in AuthenticationManager.java are too LDAP specific which do not make sense when the specific configuration with LDAP you used for testing this is not being used. This may be better addressed if you reorganise the code into wrapper Custom LoginModule that delegates calls to an LDAPLoginModule and additionally did the group lookup and provision groups/user membership if needed. The LoginModule can receive some additional object references required to create groups/user membership by calling [Configuration.setConfiguration]((https://docs.oracle.com/javase/8/docs/api/javax/security/auth/login/Configuration.html#setConfiguration-javax.security.auth.login.Configuration-) and adding SystemEntityClient and OperationContext there will give the login module access to these objects and be able to create groups from custom login module. The custom login module can access these merged configuration options by implementing the initialize method, the last param |
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.
See the comment on alternate approach that keeps the LDAP groups sync code separate into a module and used only when this custom module is explicitly configured.
It was observed that LDAP groups were not getting fetched for user via /login API.
This PR involves changes for /login API via LDAP authentication. The changes includes validating user credentials for /login API via LDAP provider and then fetching groups associated for user, using groupProvider.
Post this validation, it will provision user, groups and then create relationship between user and groups.
Sample frontend jaas.conf
WHZ-Authentication {
security.PropertyFileLoginModule sufficient
debug="true"
file="user.props";
com.sun.security.auth.module.LdapLoginModule sufficient
userProvider={}
authIdentity="{USERNAME}@Domain"
userFilter="(&(userPrincipalName={USERNAME}@Domain)(objectClass=person))"
groupProvider={}
groupNameAttribute="cn"
java.naming.security.authentication="simple"
debug="true"
useSSL="true";
};