659140 - Separate out reset codes into their own space

Status: RESOLVED FIXED

(Cloud Services :: Server: Core, defect)

Description

[Toby Elliott [:telliott]]

There's no reason that reset codes should be directly a part of the auth object - they can be used generically for a whole lot more if we want to. There's also no reason they need to be locked into sql for implementation - indeed, we should consider a membase version for sreg so as to remove all mysql dependencies on that box. It also seems strange that an app using ldap for auth and not reset codes should have sqlalchemy as a dependency.

Thus, I propose creating

services.resetcodes.[mysql|memcache|...]

and putting all reset-code related libraries in there. The application controllers can instantiate them as needed.

Comment 1

[Toby Elliott [:telliott]]

Attachment: base class for reset codes

Comment 2

[Toby Elliott [:telliott]]

Attachment: memcache implementation

Comment 3

[Toby Elliott [:telliott]]

Attachment: sql implementation

Comment 4

[Toby Elliott [:telliott]]

Attachment: Stuff that gets removed from util.py

Comment 5

[Tarek Ziadé (:tarek)]

Comment on attachment 534840 [details]
base class for reset codes

If you're doing a base class and not a plugin, you don't need to use abc.

@abc.abstractmethod is to be used to define meta class (see http://docs.python.org/library/abc.html).  That's what I do for the plugin system (see auth and storage)

So you have two paths here:

1/ do like the plugin system, by using interfaces. the concrete class does not have to inherit from the base class, just to implement the methods defined'

2/ use a 'base' class from which your concrete class inherits. In that case, abc is not useful and you add "raise NotImplementedError()" in the base class methods body.


1/ seems simpler to me, since implementations do not have to worry about inheritance.

Comment 6

[Tarek Ziadé (:tarek)]

Comment on attachment 534843 [details]
memcache implementation

># ***** 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 Sync Server
>#
># The Initial Developer of the Original Code is the Mozilla Foundation.
># Portions created by the Initial Developer are Copyright (C) 2010
># the Initial Developer. All Rights Reserved.
>#
># Contributor(s):
>#   Tarek Ziade ([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 *****
>""" Reset code manager.
>
>Stores the reset codes in a SQL Table, per user name.
>
>The storage can be overriden.
>"""
>import datetime
>
>from memcache import Client
>
>from services.resetcodes import ResetCode, NoUserIDError
>from services import logger
>
>class ResetCodeManager(ResetCode):
>    """ Implements the reset code methods for auth backends.
>    """

What's 21600 ? seconds ? minutes ? The expiration constant could be a constant like

_24HOURS = 21600  

to make it clear


>    def __init__(self, product='auth', nodes=None, debug=0,
>                 expiration=21600, **kw):
>        if nodes is None:
>            nodes = ['127.0.0.1:11211']
>
>        self._engine = Client(nodes, debug)
>        self.product = product
>        self.expiration = expiration
>
>    #
>    # Private methods
>    #
>    def _get_reset_code(self, user_id):
>        return self._engine.get(self._generate_key(user_id))
>
>    def _generate_key(self, user_id):
>        return "reset:%s:%s" % (user_id, self.product)
>
>    def _set_reset_code(self, user_id):

it seems that _get_reset_code already generates the key, so are you calling _generate_key twice here ?

>        code = self._generate_reset_code()
>        key = self._generate_key(user_id)
>        if not self._engine.set(key, code, self.expiration):
>            raise BackendError()
>
>        return code
>
>    #
>    # Public methods
>    #
>    def generate_reset_code(self, user, overwrite=False):
>        user_id = self._get_user_id(user)
>        if not overwrite:
>            stored_code = self._get_reset_code(user_id)
>            if stored_code is not None:
>                return stored_code
>
>        return self._set_reset_code(user_id)
>
>    def verify_reset_code(self, user, code):
>        user_id = self._get_user_id(user)
>        if not self._check_reset_code(code):
>            return False
>
>        stored_code = self._get_reset_code(user_id)
>        if stored_code is None:
>            return False
>
>        return stored_code == code
>
>    def clear_reset_code(self, user):
>        user_id = self._get_user_id(user)
>        return self._engine.delete(self._generate_key(user_id))

Comment 7

[Toby Elliott [:telliott]]

Attachment: base class for reset codes (v2)

I appear to have misunderstood the purpose of abc. raising NotImplementedError seems fine.

Comment 8

[Toby Elliott [:telliott]]

(In reply to comment #6)
> Comment on attachment 534843 [details]
> memcache implementation

> >class ResetCodeManager(ResetCode):
> >    """ Implements the reset code methods for auth backends.
> >    """
> 
> What's 21600 ? seconds ? minutes ? The expiration constant could be a
> constant like
> 
> _24HOURS = 21600  
> 
> to make it clear
> 

It's 6 hours in seconds. I can add a constant.

> >    #
> >    # Private methods
> >    #
> >    def _get_reset_code(self, user_id):
> >        return self._engine.get(self._generate_key(user_id))
> >
> >    def _generate_key(self, user_id):
> >        return "reset:%s:%s" % (user_id, self.product)
> >
> >    def _set_reset_code(self, user_id):
> 
> it seems that _get_reset_code already generates the key, so are you calling
> _generate_key twice here ?
> 

This is calling _generate_reset_code (which generates a new one), not _get_reset_code (which looks it up in memcache). I don't think there's a double call here.

There is a double-call if overwrite is set to true, as it does a get, then a set. But since it's just a string concatenation, that shouldn't matter.



> >        code = self._generate_reset_code()
> >        key = self._generate_key(user_id)
> >        if not self._engine.set(key, code, self.expiration):
> >            raise BackendError()

Comment 9

[Toby Elliott [:telliott]]

Attachment: The kludge to make sreg work

Comment 10

[Toby Elliott [:telliott]]

Attachment: The tests for everything

Comment 11

[Tarek Ziadé (:tarek)]

Would you mind generating a single unified diff against server-core ? that'd be way easier for me to review it 

Thanks

Comment 12

[Toby Elliott [:telliott]]

The only file that isn't completely new here is the removing of functions from util.py, so I don't know that it'll do much. I'll see about generating a diff.

Comment 13

[Toby Elliott [:telliott]]

Attachment: One big patch with all the changes

Comment 14

[Tarek Ziadé (:tarek)]

(In reply to comment #12)
> The only file that isn't completely new here is the removing of functions
> from util.py, so I don't know that it'll do much. I'll see about generating
> a diff.

the diff is still useful for the new files because I can apply it to my own repo to run tests etc, using a single "hg qimport http://the.bugzilla.url".

Having single files means I have to set them manually, dealing with the old files diffs etc.

Thanks for doing it

Comment 15

[Tarek Ziadé (:tarek)]

2 errors caught with flake8 (also applying the patch adds many warning you need to fix) :

  services/resetcodes/__init__.py:61: undefined name 'NoUserIdError'
  services/resetcodes/rc_memcache.py:76: undefined name 'BackendError'

First one seems a typo.

the test fails after applying the patch (do I have a previous pending patch to apply before this one ? ) :

tarek@tarek-laptop:~/dev/hg.mozilla.org/server-core$ make test
bin/nosetests -s --with-xunit services
......................................EE...............
======================================================================
ERROR: Failure: ImportError (cannot import name User)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/lib/python2.6/site-packages/nose-1.0.0-py2.6.egg/nose/loader.py", line 390, in loadTestsFromName
    addr.filename, addr.module)
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/lib/python2.6/site-packages/nose-1.0.0-py2.6.egg/nose/importer.py", line 39, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/lib/python2.6/site-packages/nose-1.0.0-py2.6.egg/nose/importer.py", line 86, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/home/tarek/dev/hg.mozilla.org/server-core/services/tests/test_resetcode.py", line 41, in <module>
    from services.auth import User
ImportError: cannot import name User

======================================================================
ERROR: Failure: ImportError (cannot import name generate_reset_code)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/lib/python2.6/site-packages/nose-1.0.0-py2.6.egg/nose/loader.py", line 390, in loadTestsFromName
    addr.filename, addr.module)
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/lib/python2.6/site-packages/nose-1.0.0-py2.6.egg/nose/importer.py", line 39, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/lib/python2.6/site-packages/nose-1.0.0-py2.6.egg/nose/importer.py", line 86, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/home/tarek/dev/hg.mozilla.org/server-core/services/tests/test_sqlauth.py", line 43, in <module>
    from services.auth.sql import SQLAuth
  File "/home/tarek/dev/hg.mozilla.org/server-core/services/auth/sql.py", line 47, in <module>
    from services.util import (validate_password, ssha256,
ImportError: cannot import name generate_reset_code

----------------------------------------------------------------------
Ran 55 tests in 1.689s

FAILED (errors=2)
make: *** [test] Error 1


Other remarks:

1/ you should raise objects, not classes. so "raise NoUserIdError()"

2/ on naming

resetcodes.rc_sql.ResetCodeManager 
resetcodes.rc_memcache.ResetCodeManager 
resetcodes.rc_regsql.ResetCodeManager 

That's a lot of "rc" and having the same name for every backend is troublesome.

I would suggest (maybe a shorther class name): 

resetcodes.sql.SQLResetCodeManager 
resetcodes.memcache.MemCacheResetCodeManager 
resetcodes.sreg.SRegResetCodeManager 

3/ shouldn't the auth backends use those new classes, and then auth/resetcode be gone ?

Comment 16

[Toby Elliott [:telliott]]

On 2/ I started that way and had immense problems because import memcache was importing that, not the memcache lib.

Comment 17

[Tarek Ziadé (:tarek)]

ah right, this is indeed an issue from within the package. What we usually do is add a _ suffix. _memcache

Comment 18

[Toby Elliott [:telliott]]

Attachment: Big patch v2

1/ is fixed

2/ I like the rc_ prefixes - we have a ton of files floating around named sql, and that's just not all that clear. Plus we avoid namespace issues. I did change the class names.

3/ Yes, but I'm trying to be minimally disruptive here. The auth classes still use resetcode (look what happened when I tried to pull the util function - sqlauth blew up giving us error #2 above). There will be some redundant code for a while until I get the other bits in place.

Also resolved the no-memcache-install issue, though I'm not thrilled by the idea that our tests will pass if memcache isn't there. We wouldn't have known memcache wasn't installed if it hadn't errored above.

This should Just Work on top of an hg tip, fingers crossed.

Comment 19

[Tarek Ziadé (:tarek)]

3/ I don't get this approach since the reset code is to be used by the auth class at first mainly. I think it's better to change everything at once, especially if the new code break something.

Although, if it's just an organization on your side, and at the end we get a single changeset for that feature, I am fine with this.

== 

For the tests there are several things really:

1 - some tests are covering core features that can be used without ldap or mysql or memcached 

2 - all the tests, with our execution environment, are covering sql/memcache/ldap

3 - you are talking about a test dependency, not a production dependency here. The test failed because memcache was missing.

4 - we are the onex setting up the test environment and we should simply make sure we test everything

5 - but we should allow the community to run tests w/ ldap/sql/memcached

6 - remember that some projects will use core without sql, ldap or memcache. While we garantuee that every server has memcached installed, one project might simply want to use redis. Why forcing the devs of that project to have memcahe ?

I solved this by adding a build_extras in the Makefile, that installs memcached/sql/ldap

Mozilla / Jenkins does:

 $ make build build_extras tests

The community does :

 $ make build test

So it boils down to skipping tests if memcache is not installed, and in the meantine making sure our test env. tests a memcache-enabled environment

Comment 20

[Toby Elliott [:telliott]]

Yes, I understand the test purposes. I'm just mildly annoyed that we lose a line of defense.

as for 3/, the final changeset for this is a massive blob that will cross multiple repos and affect big chunks of the core - we're stripping these from auth functions which are getting rewritten anyway. It seems like it would be easier to review in bite-sized backwards-compatible chunks. If you prefer a big lump, I can do that, but when everything is this intertwined, it seems like it would make sense to do the non-disruptive bits first.

Comment 21

[Toby Elliott [:telliott]]

Comment on attachment 535196 [details] [diff] [review]
Big patch v2

This ended up getting reviewed as part of the larger patch in bug 652175

Comment 22

[Toby Elliott [:telliott]]

fixed in http://hg.mozilla.org/services/server-core/rev/7b5955f4801e