Skip to content
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

1775 LDAP login and user creation fix #1777

Conversation

ERLKDev
Copy link
Contributor

@ERLKDev ERLKDev commented Oct 26, 2022

What type of PR is this?

  • bug

What this PR does / why we need it:

This PR fixes two bugs in the code:

  • LDAP login not possible due to a programming error (wrong if statement)

  • Fullname and email adres in newly created users using LDAP had the binary values instead of the string values of the corresponding attributes

The full description of the bugs is available in #1775

Which issue(s) this PR fixes:

Fixes #1775

@ERLKDev
Copy link
Contributor Author

ERLKDev commented Oct 27, 2022

In #1487 the unit test has been changed from (see comment for an explanation):

    assert result is not False
    assert result.username == user

to

assert result is False

The result should however not be false, since an array with an empty tuple is still a result since it is not an empty array, so the search was "successful" and an user is created. I will revert the asserts back in the unit test.

Since an empty tuple is still a result, an user is created and the result should not be false.
@hay-kot
Copy link
Collaborator

hay-kot commented Nov 11, 2022

FYI, I tried to run formatting to get this through, but the test is still failing. You can run both of these locally using the make file.

@hay-kot hay-kot marked this pull request as draft November 11, 2022 00:25
@hay-kot
Copy link
Collaborator

hay-kot commented Dec 1, 2022

This good to go now?


if not user:
user_bind = settings.LDAP_BIND_TEMPLATE.format(username)
user_bind = "cn={}, {}".format(username, settings.LDAP_BASE_DN)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might break some LDAP setups, I suggest to leave the LDAP_BIND_TEMPLATE as it is.
For example on my server I need uid={},..., not cn={},....

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you get LDAP to work with v1? It seems that you are using FreeIPA.
Mine was working with v0.5, but v1 broke it.
I am trying

LDAP_BIND_TEMPLATE=uid={},cn=users,cn=accounts,dc=...,dc=...
LDAP_BASE_DN=cn=users,cn=accounts,dc=...,dc=...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not using FreeIPA, I use my own custom setup with OpenLDAP. But yes, it worked with v1.

@Zackptg5
Copy link

@ERLKDev, I attempted to build with this but got: NameError: name 'Optional' is not defined from settings.py. Optional needs imported at the top of it: from typing import Optional

@ERLKDev
Copy link
Contributor Author

ERLKDev commented Dec 31, 2022

This good to go now?

Sorry for the late response, have been quite busy last month and kind of forgot about it.

Next week I have time to finish the PR.

@michael-genson
Copy link
Collaborator

michael-genson commented Dec 31, 2022

FYI in Python >= 3.10 you can use str | None rather than Optional[str] which doesn't require an import, and looks a bit nicer

@hay-kot
Copy link
Collaborator

hay-kot commented Feb 11, 2023

Merged in #2107

@hay-kot hay-kot closed this Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v1.0.0b] - LDAP login/user creation not possible due to an incorrect if statement in the code
6 participants