-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[Improve] Add LDAP user not exsitst action config #10451
[Improve] Add LDAP user not exsitst action config #10451
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #10451 +/- ##
============================================
+ Coverage 40.59% 40.86% +0.27%
- Complexity 4778 4848 +70
============================================
Files 878 886 +8
Lines 35747 35990 +243
Branches 3970 3993 +23
============================================
+ Hits 14512 14709 +197
- Misses 19789 19822 +33
- Partials 1446 1459 +13
Continue to review full report at Codecov.
|
PTAL @zhongjiajie @caishunfeng |
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.
please find my tow cents
...-api/src/main/java/org/apache/dolphinscheduler/api/security/impl/ldap/LdapAuthenticator.java
Outdated
Show resolved
Hide resolved
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. Thanks.
Currently, we only support LDAP. I hope that LDAPS and LDAPS without ssl can be supported in the future.
Kudos, SonarCloud Quality Gate passed! |
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.
thanks for the works of LDAP
Purpose of the pull request
When you log in with LDAP and are not a ds user, it will create a new user automatedly.
But in my option, LDAP login is the same as user-password log-in, if you are not a ds user, your login will be denied.
I think we can add a new config field, user can choose if they log in with LDAP and are not a ds user, which will create a new user or be denied.
Brief change log
Verify this pull request
This change added tests and can be verified as follows:
modified: dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/security/SecurityConfigLDAPTest.java
modified: dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/security/impl/ldap/LdapAuthenticatorTest.java
part of #10425