feat: Improve LDAP implementation to be standards compliant #13777

jon-nfc · 2021-07-26T09:26:07Z

Full proposal can be found in issue #13738

🔗 Related Items
Proposal: #13738 PR: #13777
📖 Documentation
ERPNext: PR frappe/erpnext_documentation#376 Frappe: PR frappe/frappe_docs#168

PR Tasks

All Tests Passing
Development complete (OP to tick as appropriate)
Documentation update - issues/PR submitted
All tasks in [proposal] Refactor/Improve LDAP implementation to be standards compliant #13738 complete

docs: frappe/frappe_docs#168

Add new fields for the new group feature issue #13738

New method to search for user group membership. Replaces old logic of using an ldap users attribute memberof which is not supported by all LDAP implementations issue #13738

as part of the search the login/user name is required to find the user issue #13738

adjusted to posixgroup as openldap groups use objectclass 'posixgroup' for both a posix group and a samba group. issue #13738

All LDAP operations should be done by ldap base dn user. This allows an administrator to lock down their directory to the user the LDAP operations are being conducted by. issue #13738

User needs to be able to conduct complex filtering. As long as the placeholder '{0}' for the username is included in the ldap search filter, the user can customize as required. searches must be enclosed in '()' i.e '(uid={0}) or '(&(objecttype=posixaccount)(uid={0}))' etc. issue #13738 close #6037

Validate the LDAP search filter including enclosing in '()'. Note: if a user has a complex filter that misses the last ')' it will not be added. i.e. (&(objectclass=posixgroup)(uid={0}) is invalid but will pass validation. issue #13738

issue #13738

ldap_group_field set for depreciation. issue #13738 fixes #10794

to confirm user credentials, use 'rebind' instead of re-connecting to ldap. This also enables unit testing of all functions except the connection to ldap. issue #13738

ldap search string is user input. validate to ensure is enclosed in '()', has the '{0}' placeholder and has the same number of brackets as used in complex ldap search strings. issue #13738

A blank password causes exception 'ldap3.core.exceptions.LDAPPasswordIsMandatoryError'. Validate the user input. Issue #13738

a user document is passed to the function. use this to derive user details issue #13738

frappe/integrations/doctype/ldap_settings/ldap_settings.py

frappe/integrations/doctype/ldap_settings/ldap_settings.json

frappe/integrations/doctype/ldap_settings/test_ldap_settings.py

frappe/integrations/doctype/ldap_settings/ldap_settings.py

sider #issue-5810499

A user object is passed to the function. Use this to derived the user details. PR #13777

As the user provides some of the ldap attributes, validate those entries when the 'LDAP Settings' editor clicks save. Provide an error message if validation fails stating what is incorrect. issue #13738 PR-#13777

Function requires attributes to be of type x, validate to ensure any changes will break function and to prevent further exceptions. Only output to console as it's only a developer who will generate this error. PR-#13777

jon-nfc · 2021-07-27T09:22:51Z

Docs are currently being worked on and will have PR done as soon as complete.

jon-nfc · 2021-08-11T06:41:36Z

now you can login with ldap. (posix.user1 - password1 or posix.user2 password2)

After trying it out on test servers, everything working as expected.

@revant

glad to hear.

do I need to create an additional PR for this to be added to version-13?

jon-nfc · 2021-08-11T23:32:33Z

Bump,

What is the process to have this PR added to version 12 and 13?

revant · 2021-08-12T09:09:13Z

https://discuss.erpnext.com/t/hotfix-version-xx-develop-issue-pr-and-frappe-policies/78952/2

@mergify backport version-13

should create a PR

A user object is passed to the function. Use this to derived the user details. PR #13777 (cherry picked from commit c0565ae)

As the user provides some of the ldap attributes, validate those entries when the 'LDAP Settings' editor clicks save. Provide an error message if validation fails stating what is incorrect. issue #13738 PR-#13777 (cherry picked from commit 1b2ec4f)

Function requires attributes to be of type x, validate to ensure any changes will break function and to prevent further exceptions. Only output to console as it's only a developer who will generate this error. PR-#13777 (cherry picked from commit 99b1410)

to prevent exceptions in other locations, validate the user and group search paths at the timeof input. issue #13738 PR-#13777 (cherry picked from commit c37e16b)

User can use any valid LDAP fdn to search. fields updated to refect. issue #13738 PR-#13777 (cherry picked from commit 26b0fe3)

Created unit tests for integration LDAP. These tests are designed to confirm that LDAP will continue to work after changes are made to frappe. PR-#13777 (cherry picked from commit 172d1b3) # Conflicts: # frappe/integrations/doctype/ldap_settings/test_ldap_settings.py

mergify · 2021-08-12T09:10:20Z

Command backport version-13: success

Backports have been created

#13923 feat: Improve LDAP implementation to be standards compliant (backport #13777) has been created for branch version-13

Hey, I reacted but my real name is @Mergifyio

ankush · 2021-08-12T09:16:42Z

@mergify backport version-13-hotfix

A user object is passed to the function. Use this to derived the user details. PR #13777 (cherry picked from commit c0565ae)

As the user provides some of the ldap attributes, validate those entries when the 'LDAP Settings' editor clicks save. Provide an error message if validation fails stating what is incorrect. issue #13738 PR-#13777 (cherry picked from commit 1b2ec4f)

Function requires attributes to be of type x, validate to ensure any changes will break function and to prevent further exceptions. Only output to console as it's only a developer who will generate this error. PR-#13777 (cherry picked from commit 99b1410)

to prevent exceptions in other locations, validate the user and group search paths at the timeof input. issue #13738 PR-#13777 (cherry picked from commit c37e16b)

User can use any valid LDAP fdn to search. fields updated to refect. issue #13738 PR-#13777 (cherry picked from commit 26b0fe3)

Created unit tests for integration LDAP. These tests are designed to confirm that LDAP will continue to work after changes are made to frappe. PR-#13777 (cherry picked from commit 172d1b3) # Conflicts: # frappe/integrations/doctype/ldap_settings/test_ldap_settings.py

mergify · 2021-08-12T09:17:28Z

Command backport version-13-hotfix: success

Backports have been created

#13924 feat: Improve LDAP implementation to be standards compliant (backport #13777) has been created for branch version-13-hotfix

Hey, I reacted but my real name is @Mergifyio

…13777) (#13924) Co-authored-by: jon <jon-nfc@users.noreply.github.com> Co-authored-by: Jon Lockwood <jonathon.lockwood@networkedweb.com> Co-authored-by: Ankush Menat <ankush@iwebnotes.com>

jon-nfc added 13 commits July 25, 2021 11:02

feat: update interface

1f0e0d2

Add new fields for the new group feature issue #13738

feat: new method to search for members ldap groups

6228004

New method to search for user group membership. Replaces old logic of using an ldap users attribute memberof which is not supported by all LDAP implementations issue #13738

fix: username required

68a5ac2

as part of the search the login/user name is required to find the user issue #13738

refactor(ldap): use posixgroup

0914d8a

adjusted to posixgroup as openldap groups use objectclass 'posixgroup' for both a posix group and a samba group. issue #13738

refactor(ldap): ldap operations under base dn user

a508569

All LDAP operations should be done by ldap base dn user. This allows an administrator to lock down their directory to the user the LDAP operations are being conducted by. issue #13738

feat(ldap): validate LDAP search filter

05e978c

Validate the LDAP search filter including enclosing in '()'. Note: if a user has a complex filter that misses the last ')' it will not be added. i.e. (&(objectclass=posixgroup)(uid={0}) is invalid but will pass validation. issue #13738

chore(ldap): typo

09cf8ee

issue #13738

feat(ldap): sync ldap groups to roles

9ff38de

ldap_group_field set for depreciation. issue #13738 fixes #10794

refactor(ldap): reuse existing connection

ba81929

to confirm user credentials, use 'rebind' instead of re-connecting to ldap. This also enables unit testing of all functions except the connection to ldap. issue #13738

feat(ldap): ldap search string validation

36c5b7f

ldap search string is user input. validate to ensure is enclosed in '()', has the '{0}' placeholder and has the same number of brackets as used in complex ldap search strings. issue #13738

fix(ldap): ldap3 exception

5f6f6a7

A blank password causes exception 'ldap3.core.exceptions.LDAPPasswordIsMandatoryError'. Validate the user input. Issue #13738

fix(ldap): Don't reach outside function for user details

7927af3

a user document is passed to the function. use this to derive user details issue #13738

jon-nfc commented Jul 26, 2021

View reviewed changes

jon-nfc changed the title ~~Draft: [Proposal - issue 13738] Refactor LDAP~~ feature: Improve LDAP implementation to be standards compliant Jul 26, 2021

jon-nfc changed the title ~~feature: Improve LDAP implementation to be standards compliant~~ feat: Improve LDAP implementation to be standards compliant Jul 26, 2021

jon-nfc commented Jul 26, 2021

View reviewed changes

frappe/integrations/doctype/ldap_settings/test_ldap_settings.py Outdated Show resolved Hide resolved

jon-nfc commented Jul 26, 2021

View reviewed changes

jon-nfc commented Jul 27, 2021

View reviewed changes

frappe/integrations/doctype/ldap_settings/ldap_settings.py Outdated Show resolved Hide resolved

jon-nfc added 4 commits July 27, 2021 10:16

chore(ldap): remove unused var

ac5f85f

sider #issue-5810499

fix(ldap): Don't reach outside of function for details supplied

c0565ae

A user object is passed to the function. Use this to derived the user details. PR #13777

feat(ldap): validate user fields that are ldap attributes

1b2ec4f

As the user provides some of the ldap attributes, validate those entries when the 'LDAP Settings' editor clicks save. Provide an error message if validation fails stating what is incorrect. issue #13738 PR-#13777

fix(ldap): fetch_ldap_groups attribute vallidation

99b1410

Function requires attributes to be of type x, validate to ensure any changes will break function and to prevent further exceptions. Only output to console as it's only a developer who will generate this error. PR-#13777

jon-nfc marked this pull request as ready for review July 27, 2021 09:21

jon-nfc requested a review from leela as a code owner July 27, 2021 09:21

mergify bot merged commit be5f712 into frappe:develop Aug 11, 2021

mergify bot mentioned this pull request Aug 12, 2021

feat: Improve LDAP implementation to be standards compliant (backport #13777) #13923

Closed

mergify bot pushed a commit that referenced this pull request Aug 12, 2021

fix(ldap): Don't reach outside of function for details supplied

d9bec9a

A user object is passed to the function. Use this to derived the user details. PR #13777 (cherry picked from commit c0565ae)

mergify bot pushed a commit that referenced this pull request Aug 12, 2021

docs(ldap): update field to be a fdn

2e7a4b3

User can use any valid LDAP fdn to search. fields updated to refect. issue #13738 PR-#13777 (cherry picked from commit 26b0fe3)

mergify bot pushed a commit that referenced this pull request Aug 12, 2021

fix(ldap): Don't reach outside of function for details supplied

6f9f03e

A user object is passed to the function. Use this to derived the user details. PR #13777 (cherry picked from commit c0565ae)

mergify bot pushed a commit that referenced this pull request Aug 12, 2021

docs(ldap): update field to be a fdn

103abda

User can use any valid LDAP fdn to search. fields updated to refect. issue #13738 PR-#13777 (cherry picked from commit 26b0fe3)

mergify bot mentioned this pull request Aug 12, 2021

feat: Improve LDAP implementation to be standards compliant (backport #13777) #13924

Merged

rohitwaghchaure mentioned this pull request Sep 1, 2021

chore: change log for v13.10.0 #14103

Merged

jon-nfc mentioned this pull request Dec 28, 2021

LDAP Search String needs to end with a placeholder #18530

Closed

mdujmovits mentioned this pull request Jan 5, 2022

fix(ldap): using ldap_group_objectclass instead of ldap_group_mappings_section field for validating custom config #15541

Closed

mdujmovits mentioned this pull request Jan 17, 2022

fix(ldap): using ldap_group_objectclass instead of ldap_group_mappings_section field for validating custom config #15630

Closed

surajshetty3416 mentioned this pull request Jan 18, 2022

v14 Features & Enhancements surajshetty3416/frappe#25

Closed

bene mentioned this pull request Feb 16, 2022

fix: broken validation for custom LDAP servers #16016

Merged

surajshetty3416 mentioned this pull request Jul 18, 2022

List of V14 features & enhancements #17532

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Improve LDAP implementation to be standards compliant #13777

feat: Improve LDAP implementation to be standards compliant #13777

jon-nfc commented Jul 26, 2021 •

edited by ankush

jon-nfc commented Jul 27, 2021

jon-nfc commented Aug 11, 2021

jon-nfc commented Aug 11, 2021

revant commented Aug 12, 2021

mergify bot commented Aug 12, 2021

ankush commented Aug 12, 2021

mergify bot commented Aug 12, 2021

feat: Improve LDAP implementation to be standards compliant #13777

feat: Improve LDAP implementation to be standards compliant #13777

Conversation

jon-nfc commented Jul 26, 2021 • edited by ankush

PR Tasks

jon-nfc commented Jul 27, 2021

jon-nfc commented Aug 11, 2021

jon-nfc commented Aug 11, 2021

revant commented Aug 12, 2021

mergify bot commented Aug 12, 2021

ankush commented Aug 12, 2021

mergify bot commented Aug 12, 2021

jon-nfc commented Jul 26, 2021 •

edited by ankush