search

ensky / taiga-contrib-ldap-auth

Taiga plugin for LDAP authentication
http://taiga.io
GNU Affero General Public License v3.0
54 stars 37 forks source link

services: slugify before DB lookup #48

Closed veox closed 7 years ago

veox commented 7 years ago

In ldap_register(), LDAP binding has already been performed, and the user is allowed to authenticate. However, the current logic is to look up the user entry in local DB using the string from the login form. If the latter fails, creating a new user is attempted - which might have been already created!

This still does not allow special characters (as in #43, which can be unsafe), but does address the underlying issue.

Should fix #15, #17, #45, and make #43 obsolete.

This PR conflicts with #47 due to whitespace changes in the latter. If either gets merged first, I can rebase the other one.

pstreef commented 7 years ago

This does not seem to work for me.

It looks like slugify_uniquely() creates a unique instance of an object for a model. Meaning it ads "-1" or probably something similar when the name exists for the model (user_model).

peter.streef becomes peterstreef the first time. but the second it becomes peterstreef-1. which does not exist (when getting) so it still tries to create a new one.

As a fix I now just remove the known characters in the ldap server ("." and "-") with a replace from the username.

veox commented 7 years ago

Looks like I opened this PR without sufficient testing. :( Thanks @pstreef - I'll close this and reopen if fixed.

veox commented 7 years ago

@pstreef Well, there seems to be a general disconnect between LDAP and Taiga in this case, arising from the need to map usernames, determined by the LDAP schema and dependent on the schema used, to Taiga/Django unique user identifiers, currently determined by unique_slugify() implementation.

The best I've come up with is a "pre-slugify" _slugify(), which is ugly, but might work.

It's different from your approach in constraning at the time of LDAP registration.

I won't be submitting a PR for this, seeing how development here is stalled. I've moved my own development to the organisation that I'm doing this work for instead (if relevant, see link above).

veox commented 7 years ago

Slept on it. Understood that this might as well just be a config option, e.g. LDAP_SLUGIFY or the like, with a function. Will rework.