[web2py] LDAP with username=True can still login with email

[Richard]

Hello,

Is there a way to prevent user to log with there email? I set LDAP authentication, I create a username field on custom auth_user model and set auth.define_tables(username=True)

But I notice that I can still login with my_e...@dom.com. In this case, ldap_auth create a new user with first_name and username = my_e...@dom.com

So, I think there is a flaw here in ldap_auth :

if ldap_mode == 'ad':

                # Microsoft Active Directory

                if '@' not in username:

                    domain = []

                    for x in ldap_basedn.split(','):

                        if "DC=" in x.upper():

                            domain.append(x.split('=')[-1])

                    username = "%s@%s" % (username, '.'.join(domain))

                username_bare = username.split("@")[0]

Since it seems to recreate email as username...

Thanks

Richard

[Massimo Di Pierro]

How would you change this?

[Richard Vézina]

Hello,

I was about to post this (I think I answer your question) :

Hello,

I think I found a flaw in the interaction between Auth and LDAP contrib (web2py 2.4.7).

If I set LDAP as unique authentification method (auth.settings.login_methods = LDAP) as written in the book, web2py should leaves LDAP to create user... The things is web2py Auth seems to create user even if it LDAP that is responsible of doing it. I mean, I carefully read the code of LDAP and the only way it could create a new user is if manage_groups=True by calling do_manage_groups() since the other place where LDAP is instert new user it set email, first_name and last_name. In my case, if user use email instead of username (that should not be email, but I can't enforce this with the custom IS_NOT_EMAIL() validator I wrote) for login a new user get inserted like this : first_name = email (or the content of username input that is an email), username = email and registration_id = email. As far as I can see the only way LDAP could produce this result is if the do_manage_groups method is called, but it can't be call if manage_groups is set to False. So, the only remaining possibility is that Auth is creating the new user because it recieve a bad signal from LDAP.

I make a couples tests and found that the insert new user base on the credentials of already existing user that log with it email instead of it username occure at line 2147-2148. So I guess Auth recieve a True flag from LDAP mean the user exist in directory, since web2py can't match a existing user base on the wrong username (email) it insert a new user with wrong setting.

The origin of this is multifold. First, I think it could be prevent if there was a IS_NOT_EMAIL() validator on the username field, for some reason I can't get it to work properly with LDAP because of the way LDAP is working the validator seems to be skipped, and the username is first check against directory. Maybe using IS_NOT_EMAIL() inside ldap_auth contrib could solve this issue. Other possible origin is the way ldap_auth is written. I mean it seems that for saving a variable "username" is re-used... I think that the issue is coming from line 8 of code extract below : 

            if ldap_mode == 'ad':

                # Microsoft Active Directory

                if '@' not in username:

                    domain = []

                    for x in ldap_basedn.split(','):

                        if "DC=" in x.upper():

                            domain.append(x.split('=')[-1])

                    username = "%s@%s" % (username, '.'.join(domain))

                username_bare = username.split("@")[0]

                con.set_option(ldap.OPT_PROTOCOL_VERSION, 3)

                # In cases where ForestDnsZones and DomainDnsZones are found,

                # result will look like the following:

                # ['ldap://ForestDnsZones.domain.com/DC=ForestDnsZones,

                #    DC=domain,DC=com']

                if ldap_binddn:

                    # need to search directory with an admin account 1st

                    con.simple_bind_s(ldap_binddn, ldap_bindpw)

                else:

                    # credentials should be in the form of user...@domain.tld

                    con.simple_bind_s(username, password)

                # this will throw an index error if the account is not found

                # in the ldap_basedn

                requested_attrs = ['sAMAccountName']

                if manage_user:

                    requested_attrs.extend([user_firstname_attrib,

                                           user_lastname_attrib,

                                           user_mail_attrib])

                result = con.search_ext_s(

                    ldap_basedn, ldap.SCOPE_SUBTREE,

                    "(&(sAMAccountName=%s)(%s))" % (

                                ldap.filter.escape_filter_chars(username_bare),

                                filterstr),

                    requested_attrs)[0][1]

                if not isinstance(result, dict):

                    # result should be a dict in the form

                    # {'sAMAccountName': [username_bare]}

                    logger.warning('User [%s] not found!' % username)

                    return False

                if ldap_binddn:

                    # We know the user exists & is in the correct OU

                    # so now we just check the password

                    con.simple_bind_s(username, password)

                username = username_bare

This peace of code is pretty unreliable : It start by re-creating a email and store it in username vars if username it recieves from web2py is not a email before derive a username_bare from the altered username var and at the end it finally set username = username_bare... Why all this just to avoid create a var?!

I propose to refator this using creating a new ID or identifier var to store connection "identifier" var instead reusing the username for that. Then it will require to determine if the IS_NOT_EMAIL() should go at Auth level or ldap_auth. I don't know so much LDAP in general and even less the different implementation, so I don't know if some implementation use email as an identifier or not. Since, the Auth class as mechanism to create missing user I don't no if it intentional to allow the creation of user with email as username or not... So, maybe it a option in to use IS_NOT_EMAIL() on username field in this case it will require that IS_NOT_EMAIL be at level of Auth. Maybe, I didn't be able to make work my custom validator because of the order of validator (I had set multiple validator on username), I will try to set only IS_NOT_EMAIL and report here if it solve the problem I have with LDAP authentication.

Thanks

Richard

--
 
---
You received this message because you are subscribed to the Google Groups "web2py-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to web2py+un...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Richard Vézina]

No change... Auth seems to delegate entirely the validation on username input field in case ldap_auth is used as authentication method.

I guess this simple refactor (not tested) could do the tricks at least for Active directory :

                if not IS_EMAIL()(username)[1]:

                    domain = []

                    for x in ldap_basedn.split(','):

                        if "DC=" in x.upper():

                            domain.append(x.split('=')[-1])

                    identifier = "%s@%s" % (username, '.'.join(domain))

                else: return ERROR...

                username_bare = username.split("@")[0]

Richard

[Massimo Di Pierro]

Please send me a patch when you test it. ;-)

[Richard Vézina]

Ok!

:)

Richard

[Richard Vézina]

from gluon.validators import IS_EMAIL

            if ldap_mode == 'ad':

                # Microsoft Active Directory

                if IS_EMAIL()(username)[1] is not None:

                #if '@' not in username:

                    domain = []

                    for x in ldap_basedn.split(','):

                        if "DC=" in x.upper():

                            domain.append(x.split('=')[-1])

                    username = "%s@%s" % (username, '.'.join(domain))

                else:

                    return False

                username_bare = username.split("@")[0]

This prevent login to occure and new user to be inserted when email is used as username... however it not returning any advise to the user... I will try to figure out how to implement validation from ldap_auth and get back with a patch.

Richard 

[Richard Vézina]

Better then that I think :

In gluon/tools.py in Auth login() near line 2006 :

Replace :

tmpvalidator = IS_NOT_EMPTY(error_message=self.messages.is_empty)

With :

        if 'username' in table_user.fields or \

                not self.settings.login_email_validate:

            tmpvalidator = [IS_NOT_EMPTY(error_message=self.messages.is_empty), IS_NOT_EMAIL()]

Will require to add this new validator though :

class IS_NOT_EMAIL:

    def __init__(self, error_message='You can\'t use email as username'):

        self.e = error_message

    def __call__(self, value):

        if not IS_EMAIL()(value)[1]:

            return (value, self.e)

        return (value, None)

What you think about that??

Richard

[Richard Vézina]

I would also add this :

tmpvalidator = [IS_NOT_EMPTY(error_message=self.messages.is_empty), IS_NOT_IN_DB(db, 'auth_user.username'), IS_NOT_EMAIL()]

Since this line in the book :

auth_table.username.requires = IS_NOT_IN_DB(db, auth_table.username)

Doesn't seem to works and could be erase since who want a duplicated username in his auth_user??

Richard

[Richard Vézina]

Here the patch!!

I have not been able to use the IS_NOT_EMAIL() validator from validators.py didn't understand how validators are import in tools.py...

NOTE :

About my precedent mail... the "auth_table.username.requires = IS_NOT_IN_DB(db, auth_table.username" from the book is not require if the user use the "auth.define_tables(username=True)" and the recommended auth_tables customization mechanism. So, I think the book should be revised this way :

"In case you use old style customizing auth_tables. Make sure your username field definition looks like that :

Field('username', 'string',

          notnull=True,

          required=True,

          requires=[IS_NOT_EMPTY(error_message=T(auth.messages.is_empty)),

                    IS_NOT_IN_DB(db, 'auth_user.username'), IS_NOT_EMAIL()]

          ),

Where you make sure you use these validators in order to make sure email is not used as username and there is no duplicated username in your auth_user table."

:)

Richard

[Richard Vézina]

UP!

[Massimo Di Pierro]

I need to review this. I understand the problem but I am not convinced by the solution. I will need one week.

[Richard Vézina]

No problem!

As long as there is a solution in the next version I will be happy... Curious to know what is bugging you though, adding a new validator? Maybe the IS_MAIL() could be hack in order that it usage could be reversed, something like this : IS_MAIL(..., complement=True), then it will return true if the string is not a email... So don't need a new validator.

Richard

 

[Richard Vézina]

Massimo,

If you are concern about possible backward compatibility issue that this change could raise... Maybe we could find a way to let the ldap_auth return validator error to form (to user)... I could live with that too... I just didn't find a easy way to make it works from ldap_auth (mean a lot of refactoring could be required and I don't want to screw up ldap_auth for others, I am not equiped to test it properly over different ldap implementation). I just have to change a single line in tools.py + add a IS_NOT_EMAIL() validator, that was the easiest...

:)

Richard

[Massimo Di Pierro]

I have not forgotten. I opened issue 1645.

[Richard Vézina]

No problem, thank for the follow...

Richard

[Richard Vézina]

Hello,

I just push these change in production, then face some exception... Some of our users have email as username some don't... So the fix I found is not the proper solution... I think we will have to improve ldap_auth logic to get out of the mud with this... I will look throught this a bit more see what I can come up with. What ldap_auth should do is to check user credentials, if the user log with email but he already have a user with the same username and password it should use this user instead of creating a new user with email as username and missing first_name and many other field empty...

Richard

[Massimo Di Pierro]

i agree

[Camille Roussel]

Please excuse my ignorance on this as I am brand new to programming and web2py. For this issue what necessitates the need to store the bare username instead of potentially storing the full email or UPN? I know in our environment the sAMAccountName is not necessarily unique across the forest but the UPN is. In intial testing I have commented out line 255 from ldap_auth.py to resolve this issue but I am not sure what the full implications of that are elsewhere in the Auth module.

[Richard Vézina]

The issue come more from the way ldap_auth is written then anything else... My concern is that in case the user log with an email instead of username and there is no constraint (auth.define_tables(username=True)) when defining auth the ldap_auth contrib will create a new user with email as username just because it not well written and it can retrieve and reuse the proper record...

I was in holiday last week, but I want to have an other look now that I workaround all the issue I had migrating to LDAP/AD authentication.

If you want to help to improve ldap_auth it will be welcome... I would rewrite at least the part I can test, the active directory part...

Richard

[Camille Roussel]

I am brand new to programing so I am not sure how much i can contribute. That being said I am trying to get this running in a multi-domain active directory forest. I can help test out an AD changes you propose to ensure they remain compatible with a forest versus an individual domain. While you look at rewriting ldap_auth I have got it working to meet our needs by modifying the one line mentioned above and manually specifying the UPN as the username attribute.

[Richard Vézina]

Do you mean we could use the UPN as a mean to authenticate base on email address?

Here some informations :

http://social.technet.microsoft.com/Forums/windowsserver/en-US/9a739918-a9dd-4fea-acd4-387126343402/is-it-possible-to-have-active-directory-login-as-an-email-address

sAMAccountName is the default username for historical reason.

Since I switch on username based authentication and I educate my user on that, I don't want to get back. Also, ldap_auth as far as I remember authenticate base on the reconstitution of an email address even if a username or an email address is passed. I think the problem is more to make sure ldap_auth doesn't duplicate user if they log with username or email address and maybe enforce one or the other as desire.

I will try to give a go to that soon.

Richard

--
Resources:
- http://web2py.com
- http://web2py.com/book (Documentation)
- http://github.com/web2py/web2py (Source code)
- https://code.google.com/p/web2py/issues/list (Report Issues)

[Camille Roussel]

In taking a closer look at this I think we agree that AD authentication needs to be able to handle both sAMAccountName and UPN's. Although UPN's are not emails they share the same syntax.

This issue seems to stem from the following: 

1) When a user uses their UPN as a username ldap_auth ends up truncating the UPN to before the @ ending up with the equivalent of the sAMAccountName. This truncated username is used to register the user in Auth.

2) Later in the login processes get_or_create_user is called from gluon/tools.py . This gets passed the username from the original login form, in this case the UPN. It then uses the UPN to look up the user in auth_user. The look up fails so get_or_create_user creates a new user in auth_user with the UPN as the username.

3) On subsequent logins the user is authenticated through ldap_auth but again the final login returned by get_or_create_user is the duplicate (UPN) entry.

3a) Its important to note that this bug may be present in any alternative authentication methods that modify the username used in auth_user.

If we delete line 255 from ldap_auth (username = username_bare) then the username used to register users by ldap_auth and the username used by get_or_create_user match and everything works as expected. Still this posses a new issue. 

After the change any user that logs in once with their sAMAccountName and another time with their UPN will be treated as two distinct users within Auth. Still I think this is preferable than the current action.

[Richard Vézina]

Good analysis!!

I was so obsess by make it works that I didn't track down the problem at it source yet... That will help to fix the issue.

So if I understand preventing user to log with email or kind of monkey patch UPN login call to get_or_create_user and replace the user it contains with the proper username could solve the issue. This may be a security issue, I am not really versed in security. Also, I am not sure how it works and if get_or_create_user expose the ID inputed... I mean does the ID in get_or_create_user is passed from the input ID from login form or does get_or_create_user get the ID from ladp_auth... If it the later case we should just make sure we pass the proper ID.

Not sure what have change but there is change in get_or_create_user in web2py 2.6.1

Richard

On Fri, Sep 13, 2013 at 1:18 PM, Camille Roussel <cam...@rousselfamily.com> wrote:

In taking a closer look at this I think we agree that AD authentication needs to be able to handle both sAMAccountName and UPN's. Although UPN's are not emails they share the same syntax.

This issue seems to stem from the following: 

1) When a user uses their UPN as a username ldap_auth ends up truncating the UPN to before the @ ending up with the equivalent of the sAMAccountName. This truncated username is used to register the user in Auth.

2) Later in the login processes get_or_create_user is called from gluon/tools.py . This gets passed the username from the original login form, in this case the UPN. It then uses the UPN to look up the user in auth_user. The lookup fails so get_or_create_user creates a new user in auth_user with the UPN as the username.

3) On subsecuent logins the user is authenticated through ldap_auth but again the final login returned by get_or_create_user is the duplicate (UPN) entry.

3a) Its important to note that this bug may be present in any alternative authentication methods that modify the username used in auth_user.

If we delete line 255 from ldap_auth (username = username_bare) then the username used to register users by ldap_auth and the username used by get_or_create_user match and everything works as expected. Still this posses a new issue. 

After the change any user that logs in once with their sAMAccountName and another time with their UPN will be treated as two distinct users within Auth. Still I think this is preferable than the current action.

[Richard Vézina]

Ok, I just restart working on this and now it seems to me that the issue is coming form line the try and except where the except start at 417. To me it makes no sens at all that we try to search if user exist by looking at it by searching with username (username_bare set at line 255 : username = username_bare) if there is an user that as a username as email!!!

So, what about removing line 255 and changing the try like this :

                try:

                    #

                    # user as username

                    # #################

                    if username_bare:

                        user_in_db = db(db.auth_user.username == username_bare)

                        # 'ad' ldap_mode use username_bare the other mode not always define username_bare

                    else:

                        user_in_db = db(db.auth_user.username == username)

                        # for other mode than 'ad'

                    if user_in_db.count() > 0:

                        user_in_db.update(first_name=store_user_firstname,

                                          last_name=store_user_lastname,

                                          email=store_user_mail)

                    else:

                        db.auth_user.insert(first_name=store_user_firstname,

                                            last_name=store_user_lastname,

                                            email=store_user_mail,

                                            username=username)

                except:

                    #

                    # user as email

                    # ##############

                    user_in_db = db(db.auth_user.email == username)

                    if user_in_db.count() > 0:

                        user_in_db.update(first_name=store_user_firstname,

                                          last_name=store_user_lastname)

                    else:

                        db.auth_user.insert(first_name=store_user_firstname,

                                            last_name=store_user_lastname,

                                            email=username)

But as describe by Camille, the same quirks seems to occure in gluon/tools.py, since even with the change above, I still get new user created if I am login with email as username... I look  futher in tool.py to find where that occure.

Richard

[Richard Vézina]

Ok, found where the quirk is from, I think... These lines (gluon/tools.py line 2107 - 2108, web2py 2.4.7): 

                user = self.db(table_user[username]

                               == form.vars[username]).select().first()

In case of using "username" as authentication field, the credentials will be check against "username" and since the user as input his email the check failed and a new user is created later on by get_or_create_user().

So I guess we should manipulate form.vars[username] in order to make sure we compare appel with apple and pear with pear... Something like that should be be working :

                if username == 'username' and not IS_EMAIL()(form.vars[username])[1]:

                    form.vars[username] = form.vars[username].split('@')[0]

                user = self.db(table_user[username]

                           == form.vars[username]).select().first()

Also, I notice that login_bare() method use userfield in order to distinguish between username and "username". Maybe login() method redability could improve by using the same convention as in longin_bare()??

If you want to test that I can provide a patch against web2py 2.6.3 if you want...

Richard

[Camille Roussel]

Richard, While we are on the right track I think these changes do not address the deeper issue. From what I can tell this stems from the fact that there are effectively 2 ways in which an LDAP user get created. First there is code in ldap_auth (lines 403-428) where we effectively duplicate get_or_create_user from gluon tools. Second is the calls from login to get_or_create_user in the login method of gluon/tools.py. Perhaps we need to refactor the login code to remove calls to get_or_create_user in tools.py and then call that function directly in ldap_auth.py. For this to work though all other alternative login methods would have a dependence on creating or updating their own users. Once I have time I will try and verify the other methods in gluon/contrib/login_mehtods to see what the impact would be.

Hopefully this made sense if not let me know and I will try to clarify further.

[Richard Vézina]

Hmm... I am not sure bring get_or_create_user() in ldap_auth is the way to go... I am almost sure that Massimo will not agree to change tools.py login method that much. How I see that, tools.py login method need to call get_or_create_user() in case auth authentication is used in order to deliver an add user "on fly" feature. The same goes for ldap_auth if it want to deliver the feature too. Also, it seems that it is always tools.py that have create user in my test case, so the user creation in ldap_auth is going to occure very few, only the update part is going to occure on every authentication. Ok, wait, if the auth methods is declared first it does what I just wrote :

from gluon.contrib.login_methods.ldap_auth import ldap_auth

auth.settings.login_methods = [auth, ldap_auth(mode='ad', ...)]

I guess, if I remove the auth as the first methods, the responsability to create a new user will be dedicate to ldap_auth...

So, if the change (I proposed for tools.py is working well), I suggest that we just use get_or_create_user() in ldap_auth instead of "custom" update and insert. I think this could be a good compromise and make ldap_auth more integrated and standardized with web2py??

Waiting your reply...

Richard

[Richard Vézina]

If we bring get_or_create_user() in ldap_auth we elminate the horrible try and many lines of code that are a duplicate from get_or_add_user() for sure.

Richard

[Massimo Di Pierro]

+1

[Richard Vézina]

Here the change to ldap_auth in order to use get_or_create_user(), at line 403 add the uncommented lines below and comment the commented ones :

                if username_bare:

                    user_in_db = get_or_create_user({'username': username_bare, 'password': password}, 

                                                    self.settings.update_fields)

                else:

                    user_in_db = get_or_create_user({'username': username, 'password': password}, 

                                                    self.settings.update_fields)

#                try:

#                    #

#                    # user as username

#                    # #################

#                    user_in_db = db(db.auth_user.username == username_bare)

#                    if user_in_db.count() > 0:

#                        user_in_db.update(first_name=store_user_firstname,

#                                          last_name=store_user_lastname,

#                                          email=store_user_mail)

#                    else:

#                        db.auth_user.insert(first_name=store_user_firstname,

#                                            last_name=store_user_lastname,

#                                            email=store_user_mail,

#                                            username=username)

#                except:

#                    #

#                    # user as email

#                    # ##############

#                    user_in_db = db(db.auth_user.email == username)

#                    if user_in_db.count() > 0:

#                        user_in_db.update(first_name=store_user_firstname,

#                                          last_name=store_user_lastname)

#                    else:

#                        db.auth_user.insert(first_name=store_user_firstname,

#                                            last_name=store_user_lastname,

#                                            email=username)

Don't forget to import the get_or_create_user() function :

from gluon.tools import get_or_create_user

I am not pretty sure of this change... In tools.py get_or_create_user() get form.vars as keys argument, in case of ldap_auth we don't have access to form.vars and I don't think that having it is the write way to go. So, if ldap_auth only works with username I think the change is correct, if it could use email field there is a flaw in what is above. I am not sure I really have to pass username_bare and make the if username_bare check since I think that get_or_create_user() is just interrested in the keys and not verify credentials... Also, I am not sure it does the samething of the commented lines, since I don't see how get_or_create_user() could get the AD data to update the record of the found user... So, I guess, there is a bigger refactoring to do!

I am waiting a fake user in AD to be created in order to make further test, but if someone can test it and report here if it works as intent, I mean, create in case auth_user don't contains the user and the user credentials have been verify against AD or update and login user in case the user is already availble in auth_user and the credentials verification against AD pass...

Notice, I also add this in db.py :

auth.settings.update_fields = ['emails', 'username', 'first_name', 'last_name']

I attach ldap_auth.py

Richard