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

Duplicates in automatically generated usernames #7

Closed
mk2301 opened this issue Sep 20, 2022 · 3 comments · Fixed by #160
Closed

Duplicates in automatically generated usernames #7

mk2301 opened this issue Sep 20, 2022 · 3 comments · Fixed by #160
Assignees
Labels
bug Something isn't working

Comments

@mk2301
Copy link
Collaborator

mk2301 commented Sep 20, 2022

At the moment, the username is generated <firstname>.<lastname>.
So if 2 people have the same name, the second one can't register as the username must be unique (LDAP uid).

As the username is only really used for login, I would propose to use email for login instead. It better for the user as he/she doesn't need to remember a seperate username.

We need to allow change of the email address though, so probably we can't just use the email as unique id because it might be referenced somewhere. Maybe something like hash(birthday, creationDate) as uid, as those fields will never change.

@mk2301 mk2301 added the bug Something isn't working label Sep 20, 2022
@Theophile-Madet
Copy link
Contributor

I'm not really convinced that users remember their email addresses more than their usernames, but I don't have a strong opinion on that.

The main difficulty I think is that updating a username in LDAP is not straightforward. I don't think we can just update the username field of TapirUser.

The firstname.lastname is only the default suggestion, it is just an editable string when creating the account with CreateUserFromShareOwnerView. That view also gives an error if the username already exists.
We'd need to check uniqueness for email addresses as well.

Having something readable as UID makes it easier for example to give someone extra rights on the LDAP admin page, though I guess relying on names can be error prone when several people have the same or similar names.

I would not use the birthday in the LDAP UID, as there could be typing errors when creating the account and it could change later. What about just "account_[id]" with the TapirUser.id?

@mk2301
Copy link
Collaborator Author

mk2301 commented Sep 26, 2022

Yes TapirUser.pk makes more sense, I will look into this soon and see how easy it is to do email login.

oliveratfoodcoopx added a commit that referenced this issue Oct 30, 2022
- adds small REST app within ldap to create volatile LDAP servers
  (for testing purposes) - by no mean this is production ready (it
  was added as a proof of concept first - improvements come later)
- defines LdapEnabledTestCase so it can test LDAP connections in
  a isolated environment

Ref: #7
oliveratfoodcoopx added a commit that referenced this issue Oct 30, 2022
@gkirchner
Copy link
Collaborator

I just discussed with @oliveratfoodcoopx to follow a new approach for authentication with Keycloak and therefore at least put this issue on hold.

@mk2301 mk2301 linked a pull request Feb 15, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants