646945 - Reconfigure LDAP to use uidNumber as the primary key

Status: RESOLVED FIXED

(Cloud Services :: Server: Registration, defect)

Description

[Toby Elliott [:telliott]]

Proposal is described in https://intranet.mozilla.org/Services/Ops/LDAP_Reorganization

Basically, we want to move away from using uid (a mutable field that should match the email in most cases) and to uidnumber (an internal record the user never knows about). We've figured out how to make the sync and reg servers backwards compatible (search to get the dn, then operate on that), which means we can do the change with no downtime.

Comment 1

[Toby Elliott [:telliott]]

Attachment: The new user object for sync authentication

Not attaching the diff here, since the diff would be longer than the file. Basically, the authenticate_user method has been ripped out and replaced with the new process.

Comment 2

[Toby Elliott [:telliott]]

Attachment: patches the secure server to provide new dn functionality

This one is a patch. Unfortunately, there's a lot of spacing fixes picked up, so it looks a lot bigger than it is. The big changes are in update_email (plus the new library at the bottom), authenticate_user, create_user and constructUserDN

Comment 3

[Toby Elliott [:telliott]]

Note that the second attachment (for sreg) also applies to reg.

Comment 4

[Tarek Ziadé (:tarek)]

what I suggest to do is to fix the spaces fixes first (I am +r on this ;) )
Then provide a clean patch.

Comment 5

[Toby Elliott [:telliott]]

Attachment: better patch for sreg (and reg is basically the same changes)

Comment 6

[Toby Elliott [:telliott]]

Attachment: Update for server core to do the python ldapsql

This one can almost certainly be optimized, but not in the same way as the php (which uses a per-request class). This version is good enough for account-portal, though.

Comment 7

[Toby Elliott [:telliott]]

Attachment: Revised version for python

Made some changes after dev testing. Turns out ou=logins has been chosen rather than ou=users

Comment 8

[Toby Elliott [:telliott]]

Attachment: Version of code for python

Pete decided to use ou=users rather than ou=logins, so reverting that chunk

Comment 9

[Tarek Ziadé (:tarek)]

Comment on attachment 525523 [details] [diff] [review]
Version of code for python

>diff -r 93540bfa4998 services/auth/ldapsql.py
...

btw, what happens when the user changes the e-mail and that e-mail already exists for another user ?
that changes the mail and the uid field here, but do we have something in place to prevent this collision ? 
if so, do we have a test for it ?

>-        user = [(ldap.MOD_REPLACE, 'mail', [email])]
>+        user = [(ldap.MOD_REPLACE, 'mail', [email]),
>+                (ldap.MOD_REPLACE, 'uid', [extract_username(email)])
>+               ]

Comment 10

[Tarek Ziadé (:tarek)]

Comment on attachment 523380 [details]
The new user object for sync authentication

><?php
>
># ***** BEGIN LICENSE BLOCK *****
># Version: MPL 1.1/GPL 2.0/LGPL 2.1
>#
># The contents of this file are subject to the Mozilla Public License Version
># 1.1 (the "License"); you may not use this file except in compliance with
># the License. You may obtain a copy of the License at
># http://www.mozilla.org/MPL/
>#
># Software distributed under the License is distributed on an "AS IS" basis,
># WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
># for the specific language governing rights and limitations under the
># License.
>#
># The Original Code is Weave Basic Object Server
>#
># The Initial Developer of the Original Code is
># Mozilla Labs.
># Portions created by the Initial Developer are Copyright (C) 2010
># the Initial Developer. All Rights Reserved.
>#
># Contributor(s):
>#    Toby Elliott ([e-mail])
>#    Anant Narayanan ([e-mail])
>#
># Alternatively, the contents of this file may be used under the terms of
># either the GNU General Public License Version 2 or later (the "GPL"), or
># the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
># in which case the provisions of the GPL or the LGPL are applicable instead
># of those above. If you wish to allow use of your version of this file only
># under the terms of either the GPL or the LGPL, and not to allow others to
># use your version of this file under the terms of the MPL, indicate your
># decision by deleting the provisions above and replace them with the notice
># and other provisions required by the GPL or the LGPL. If you do not delete
># the provisions above, a recipient may use your version of this file under
># the terms of any one of the MPL, the GPL or the LGPL.
>#
># ***** END LICENSE BLOCK *****
>
>require_once 'weave_user/base.php';
>require_once 'weave_constants.php';
>
># Mozilla version of Authentication
>class WeaveAuthentication implements WeaveAuthenticationBase
>{
>    var $_conn;
>    var $_username = null;
>    var $_alert;
>
>
>    function __construct($username)
>    {
>        $this->open_connection();
>        $this->_username = $username;
>        $this->_alert = null;
>    }
>
>    function open_connection()
>    {
>        $this->_conn = ldap_connect(WEAVE_LDAP_AUTH_HOST);
>        if (!$this->_conn)
>            throw new Exception("Cannot contact LDAP server", 503);
>
>        ldap_set_option($this->_conn, LDAP_OPT_PROTOCOL_VERSION, 3);
>
>        return true;
>    }
>
>    function get_connection()
>    {
>        return $this->_conn;
>    }
>
>    function authenticate_user($password)
>    {
>        #bind as read admin to get the user data
>        if (!ldap_bind($this->_conn,
>                       WEAVE_LDAP_AUTH_USER, WEAVE_LDAP_AUTH_PASS))
>        {
>            error_log("Failed to bind as auth user");
>            throw new Exception("Authentication Server failure", 503);
>        }
>
>        $search = @ldap_search(
>            $this->_conn, WEAVE_LDAP_AUTH_DN,
>            "(&(" . WEAVE_LDAP_AUTH_USER_PARAM_NAME . "=" . $this->_username
>            . ")(account-enabled=Yes))",
>            array("dn", "primarynode", "uidnumber")
>        );
>
>        $res = ldap_get_entries($this->_conn, $search);
>        if ($res['count'] < 1)
>            return false;
>        if ($res['count'] > 1)
>        {
>            error_log("Got more than one ldap result for a search on username" .
>                      $this->_username);
>            throw new Exception("Authentication Server failure", 503);
>        }
>
>        $attrs = $res[0];
>        #bind as user to verify the password
>        if (!ldap_bind($this->_conn, $attrs['dn'], $password))
>            return false;
>
>        for ($i = 0; $i < $attrs["primarynode"]["count"]; $i++)
>        {
>            $node = $attrs["primarynode"][$i];
>            if (substr($node, 0, 6) == "weave:") {
>                $nd = substr($node, 6);
>                break;
>            }
>        }
>
>        if (trim($nd) != $_SERVER['HTTP_HOST'])
>            return false;
>
>        return $attrs['uidnumber'][0];
>    }


maybe that's not important, but if this is a new object, do we really need to keep these deprecated user alerts ?

>
>    function get_user_alert()
>    {
>        return $this->_alert;
>    }
>
>}
>
>?>

Comment 11

[Toby Elliott [:telliott]]

(In reply to comment #9)

> btw, what happens when the user changes the e-mail and that e-mail already
> exists for another user ?

Since email and username are now the "same" value, it fails on the "username already exists" test.

Comment 12

[Toby Elliott [:telliott]]

(In reply to comment #10)

> 
> maybe that's not important, but if this is a new object, do we really need to
> keep these deprecated user alerts ?
> 
> >
> >    function get_user_alert()
> >    {
> >        return $this->_alert;
> >    }


Possibly not, though that's beyond the scope of this bug. It would be an interface change, and probably not worth the effort.

Comment 13

[Toby Elliott [:telliott]]

Attachment: Adds two new exceptions for catching two problems

Comment 14

[Tarek Ziadé (:tarek)]

Comment on attachment 525523 [details] [diff] [review]
Version of code for python

Review of attachment 525523 [details] [diff] [review]:

looks good

Comment 15

[Tarek Ziadé (:tarek)]

Comment on attachment 528349 [details] [diff] [review]
Adds two new exceptions for catching two problems

Review of attachment 528349 [details] [diff] [review]:

Looks good. I guess the usage of those exception is related to the other patch so +r ing this one.

Comment 16

[Toby Elliott [:telliott]]

Attachment: Fix to make the tests work, and reverting a behavior change for now

Comment 17

[Tarek Ziadé (:tarek)]

Comment on attachment 528933 [details] [diff] [review]
Fix to make the tests work, and reverting a behavior change for now

Review of attachment 528933 [details] [diff] [review]:

looks good