allura-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jetm...@apache.org
Subject [07/28] git commit: [#7527] Allow different users to claim the same email address + tests
Date Fri, 12 Sep 2014 10:29:21 GMT
[#7527] Allow different users to claim the same email address + tests


Project: http://git-wip-us.apache.org/repos/asf/allura/repo
Commit: http://git-wip-us.apache.org/repos/asf/allura/commit/3e6fdda3
Tree: http://git-wip-us.apache.org/repos/asf/allura/tree/3e6fdda3
Diff: http://git-wip-us.apache.org/repos/asf/allura/diff/3e6fdda3

Branch: refs/heads/je/42cc_4905
Commit: 3e6fdda33a95a69a0fe68655e802db2afbf52c06
Parents: 7c30568
Author: Alexander Luberg <aluberg@slashdotmedia.com>
Authored: Tue Jul 22 18:15:06 2014 +0000
Committer: Dave Brondsema <dbrondsema@slashdotmedia.com>
Committed: Thu Aug 28 20:26:59 2014 +0000

----------------------------------------------------------------------
 Allura/allura/controllers/auth.py           | 24 +++++++--
 Allura/allura/lib/mail_util.py              |  4 +-
 Allura/allura/model/auth.py                 | 25 ++++-----
 Allura/allura/tests/functional/test_auth.py | 65 +++++++++++++++++++++---
 Allura/allura/tests/model/test_auth.py      | 12 +++--
 Allura/allura/tests/test_mail_util.py       |  8 +--
 6 files changed, 104 insertions(+), 34 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/allura/blob/3e6fdda3/Allura/allura/controllers/auth.py
----------------------------------------------------------------------
diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py
index 247a94b..dc3a177 100644
--- a/Allura/allura/controllers/auth.py
+++ b/Allura/allura/controllers/auth.py
@@ -179,7 +179,7 @@ class AuthController(BaseController):
         if not email:
             redirect('/')
 
-        email_record = M.EmailAddress.query.find({'_id': email}).first()
+        email_record = M.EmailAddress.query.find({'email': email}).first()
         user_record = M.User.by_email_address(email)
 
         if user_record and email_record.confirmed:
@@ -225,7 +225,7 @@ class AuthController(BaseController):
 
     @expose()
     def send_verification_link(self, a):
-        addr = M.EmailAddress.query.get(_id=a)
+        addr = M.EmailAddress.query.get(email=a, claimed_by_user_id=c.user._id)
         if addr:
             addr.send_verification_link()
             flash('Verification link sent')
@@ -236,8 +236,25 @@ class AuthController(BaseController):
     @expose()
     def verify_addr(self, a):
         addr = M.EmailAddress.query.get(nonce=a)
+        # import pydevd
+        # pydevd.settrace('localhost', port=14000, stdoutToServer=True, stderrToServer=True)
+
         if addr:
             addr.confirmed = True
+            # Remove other non-confirmed emails claimed by other users
+            claimed_by_others = M.EmailAddress.query.find({
+                'email': addr.email,
+                'claimed_by_user_id': {"$ne": addr.claimed_by_user_id}
+            }).all()
+
+            users = [email.claimed_by_user() for email in claimed_by_others]
+            for user in users:
+                user.email_addresses.remove(addr.email)
+
+            M.EmailAddress.query.remove({
+                'email': addr.email,
+                'claimed_by_user_id': {'$ne': addr.claimed_by_user_id}
+            })
             flash('Email address confirmed')
             M.AuditLog.log_user('Email address verified: %s', addr._id)
         else:
@@ -442,7 +459,8 @@ class PreferencesController(BaseController):
                 if not kw.get('password') or not provider.validate_password(c.user, kw.get('password')):
                     flash('You must provide your current password to claim new email', 'error')
                     redirect('.')
-                if M.EmailAddress.query.get(_id=new_addr['addr']):
+                if M.EmailAddress.query.get(email=new_addr['addr'], confirmed=True) \
+                        or M.EmailAddress.query.get(email=new_addr['addr'], claimed_by_user_id=c.user._id):
                     flash('Email address already claimed', 'error')
                 elif mail_util.isvalid(new_addr['addr']):
                     c.user.email_addresses.append(new_addr['addr'])

http://git-wip-us.apache.org/repos/asf/allura/blob/3e6fdda3/Allura/allura/lib/mail_util.py
----------------------------------------------------------------------
diff --git a/Allura/allura/lib/mail_util.py b/Allura/allura/lib/mail_util.py
index 4da1819..a932f13 100644
--- a/Allura/allura/lib/mail_util.py
+++ b/Allura/allura/lib/mail_util.py
@@ -159,13 +159,13 @@ def identify_sender(peer, email_address, headers, msg):
     from allura import model as M
     # Dumb ID -- just look for email address claimed by a particular user
     addr = M.EmailAddress.query.get(
-        _id=M.EmailAddress.canonical(email_address))
+        email=M.EmailAddress.canonical(email_address))
     if addr and addr.claimed_by_user_id:
         return addr.claimed_by_user()
     from_address = headers.get('From', '').strip()
     if not from_address:
         return M.User.anonymous()
-    addr = M.EmailAddress.query.get(_id=M.EmailAddress.canonical(from_address))
+    addr = M.EmailAddress.query.get(email=M.EmailAddress.canonical(from_address))
     if addr and addr.claimed_by_user_id:
         return addr.claimed_by_user()
     return M.User.anonymous()

http://git-wip-us.apache.org/repos/asf/allura/blob/3e6fdda3/Allura/allura/model/auth.py
----------------------------------------------------------------------
diff --git a/Allura/allura/model/auth.py b/Allura/allura/model/auth.py
index 5d7900b..1c753d5 100644
--- a/Allura/allura/model/auth.py
+++ b/Allura/allura/model/auth.py
@@ -115,12 +115,12 @@ class EmailAddress(MappedClass):
     class __mongometa__:
         name = 'email_address'
         session = main_orm_session
-        indexes = [
-            'claimed_by_user_id']
+        unique_indexes = [('email', 'claimed_by_user_id'),]
 
-    _id = FieldProperty(str)
+    _id = FieldProperty(S.ObjectId)
+    email = FieldProperty(str)
     claimed_by_user_id = FieldProperty(S.ObjectId, if_missing=None)
-    confirmed = FieldProperty(bool)
+    confirmed = FieldProperty(bool, if_missing=False)
     nonce = FieldProperty(str)
 
     def claimed_by_user(self):
@@ -129,10 +129,7 @@ class EmailAddress(MappedClass):
     @classmethod
     def upsert(cls, addr):
         addr = cls.canonical(addr)
-        result = cls.query.get(_id=addr)
-        if not result:
-            result = cls(_id=addr)
-        return result
+        return cls(email=addr)
 
     @classmethod
     def canonical(cls, addr):
@@ -147,13 +144,13 @@ class EmailAddress(MappedClass):
 
     def send_verification_link(self):
         self.nonce = sha256(os.urandom(10)).hexdigest()
-        log.info('Sending verification link to %s', self._id)
+        log.info('Sending verification link to %s', self.email)
         text = '''
 To verify the email address %s belongs to the user %s,
 please visit the following URL:
 
     %s
-''' % (self._id, self.claimed_by_user().username, g.url('/auth/verify_addr', a=self.nonce))
+''' % (self.email, self.claimed_by_user().username, g.url('/auth/verify_addr', a=self.nonce))
         log.info('Verification email:\n%s', text)
         allura.tasks.mail_tasks.sendsimplemail.post(
             fromaddr=g.noreply,
@@ -550,7 +547,7 @@ class User(MappedClass, ActivityNode, ActivityObject):
 
     @classmethod
     def by_email_address(cls, addr):
-        ea = EmailAddress.query.get(_id=addr)
+        ea = EmailAddress.query.get(email=addr)
         if ea is None:
             return None
         return ea.claimed_by_user()
@@ -573,7 +570,7 @@ class User(MappedClass, ActivityNode, ActivityObject):
         state(self).soil()
 
     def address_object(self, addr):
-        return EmailAddress.query.get(_id=addr, claimed_by_user_id=self._id)
+        return EmailAddress.query.get(email=addr, claimed_by_user_id=self._id)
 
     def claim_address(self, email_address):
         addr = EmailAddress.canonical(email_address)
@@ -592,10 +589,10 @@ class User(MappedClass, ActivityNode, ActivityObject):
         addresses = set(self.email_addresses)
         for addr in EmailAddress.query.find(
                 dict(claimed_by_user_id=self._id)):
-            if addr._id in addresses:
+            if addr.email in addresses:
                 if not addr.confirmed:
                     addr.confirmed = True
-                addresses.remove(addr._id)
+                addresses.remove(addr.email)
             else:
                 addr.delete()
         for a in addresses:

http://git-wip-us.apache.org/repos/asf/allura/blob/3e6fdda3/Allura/allura/tests/functional/test_auth.py
----------------------------------------------------------------------
diff --git a/Allura/allura/tests/functional/test_auth.py b/Allura/allura/tests/functional/test_auth.py
index 8af23ca..65d3a50 100644
--- a/Allura/allura/tests/functional/test_auth.py
+++ b/Allura/allura/tests/functional/test_auth.py
@@ -57,8 +57,7 @@ class TestAuth(TestController):
 
     def test_login(self):
         self.app.get('/auth/')
-        r = self.app.post('/auth/send_verification_link',
-                          params=dict(a='test@example.com'))
+        r = self.app.post('/auth/send_verification_link', params=dict(a='test@example.com'))
         email = M.User.query.get(username='test-admin').email_addresses[0]
         r = self.app.post('/auth/send_verification_link', params=dict(a=email))
         ThreadLocalORMSession.flush_all()
@@ -115,6 +114,60 @@ class TestAuth(TestController):
             if header == 'Set-cookie':
                 assert_in('expires', contents)
 
+    def test_claimed_duplicate_emails_by_different_users(self):
+        email_address = 'test-email@domain.com'
+
+        user1 = M.User.query.get(username='test-user-0')
+        user2 = M.User.query.get(username='test-user-1')
+
+        user1.claim_address(email_address)
+        user2.claim_address(email_address)
+
+        ThreadLocalORMSession.flush_all()
+        r = self.app.post('/auth/send_verification_link',
+                          params=dict(a=email_address),
+                          extra_environ={'username': 'test-user-0'})
+        email1 = M.EmailAddress.query.find(dict(email=email_address, claimed_by_user_id=user1._id)).first()
+        email2 = M.EmailAddress.query.find(dict(email=email_address, claimed_by_user_id=user2._id)).first()
+
+        # User1 verifies claimed address. All other duplicates should be removed once it
is confirmed
+        r = self.app.get('/auth/verify_addr', params=dict(a=email1.nonce))
+
+        assert email1.confirmed == True
+        assert M.EmailAddress.query.find(dict(email=email_address)).count() == 1
+        assert email_address in M.User.query.get(username='test-user-0').email_addresses
+        assert email_address not in M.User.query.get(username='test-user-1').email_addresses
+
+    @td.with_user_project('test-admin')
+    def test_user_can_not_claim_duplicate_emails(self):
+        email_address = 'test_abcd_123@domain.net'
+        user = M.User.query.get(username='test-admin')
+        addresses_number = len(user.email_addresses)
+        self.app.post('/auth/preferences/update',
+                      params={
+                          'preferences.display_name': 'Test Admin',
+                          'new_addr.addr': email_address,
+                          'new_addr.claim': 'Claim Address',
+                          'primary_addr': 'test-admin@users.localhost',
+                          'preferences.email_format': 'plain'
+                      },
+                      extra_environ=dict(username='test-admin'))
+
+        assert M.EmailAddress.query.find(dict(email=email_address, claimed_by_user_id=user._id)).count()
== 1
+        r = self.app.post('/auth/preferences/update',
+                          params={
+                              'preferences.display_name': 'Test Admin',
+                              'new_addr.addr': email_address,
+                              'new_addr.claim': 'Claim Address',
+                              'primary_addr': 'test-admin@users.localhost',
+                              'preferences.email_format': 'plain'
+                          },
+                          extra_environ=dict(username='test-admin'))
+
+        assert json.loads(self.webflash(r))['status'] == 'error', self.webflash(r)
+        assert M.EmailAddress.query.find(dict(email=email_address, claimed_by_user_id=user._id)).count()
== 1
+        assert len(M.User.query.get(username='test-admin').email_addresses) == addresses_number
+ 1
+
     @td.with_user_project('test-admin')
     def test_prefs(self):
         r = self.app.get('/auth/preferences/',
@@ -792,7 +845,7 @@ class TestPasswordReset(TestController):
             {'claimed_by_user_id': user._id}).first()
         email.confirmed = False
         ThreadLocalORMSession.flush_all()
-        self.app.post('/auth/password_recovery_hash', {'email': email._id})
+        self.app.post('/auth/password_recovery_hash', {'email': email.email})
         hash = user.get_tool_data('AuthPasswordReset', 'hash')
         assert hash is None
 
@@ -804,7 +857,7 @@ class TestPasswordReset(TestController):
             {'claimed_by_user_id': user._id}).first()
         user.disabled = True
         ThreadLocalORMSession.flush_all()
-        self.app.post('/auth/password_recovery_hash', {'email': email._id})
+        self.app.post('/auth/password_recovery_hash', {'email': email.email})
         hash = user.get_tool_data('AuthPasswordReset', 'hash')
         assert hash is None
 
@@ -818,7 +871,7 @@ class TestPasswordReset(TestController):
         ThreadLocalORMSession.flush_all()
         old_pw_hash = user.password
         with td.audits('Password recovery link sent to: test-admin@users.localhost'):
-            r = self.app.post('/auth/password_recovery_hash', {'email': email._id})
+            r = self.app.post('/auth/password_recovery_hash', {'email': email.email})
         hash = user.get_tool_data('AuthPasswordReset', 'hash')
         hash_expiry = user.get_tool_data('AuthPasswordReset', 'hash_expiry')
         assert hash is not None
@@ -865,7 +918,7 @@ To reset your password on %s, please visit the following URL:
             {'claimed_by_user_id': user._id}).first()
         email.confirmed = True
         ThreadLocalORMSession.flush_all()
-        r = self.app.post('/auth/password_recovery_hash', {'email': email._id})
+        r = self.app.post('/auth/password_recovery_hash', {'email': email.email})
         user = M.User.by_username('test-admin')
         hash = user.get_tool_data('AuthPasswordReset', 'hash')
         user.set_tool_data('AuthPasswordReset',

http://git-wip-us.apache.org/repos/asf/allura/blob/3e6fdda3/Allura/allura/tests/model/test_auth.py
----------------------------------------------------------------------
diff --git a/Allura/allura/tests/model/test_auth.py b/Allura/allura/tests/model/test_auth.py
index f3869ee..165e331 100644
--- a/Allura/allura/tests/model/test_auth.py
+++ b/Allura/allura/tests/model/test_auth.py
@@ -51,24 +51,26 @@ def setUp():
 
 @with_setup(setUp)
 def test_email_address():
-    addr = M.EmailAddress(_id='test_admin@domain.net',
+    addr = M.EmailAddress(email='test_admin@domain.net',
                           claimed_by_user_id=c.user._id)
     ThreadLocalORMSession.flush_all()
     assert addr.claimed_by_user() == c.user
     addr2 = M.EmailAddress.upsert('test@domain.net')
     addr3 = M.EmailAddress.upsert('test_admin@domain.net')
-    assert addr3 is addr
+
+    # Duplicate emails are allowed, until the email is confirmed
+    assert addr3 is not addr
+
     assert addr2 is not addr
     assert addr2
     addr4 = M.EmailAddress.upsert('test@DOMAIN.NET')
-    assert addr4 is addr2
+    assert addr4 is not addr2
     with patch('allura.lib.app_globals.request', Request.blank('/')):
         addr.send_verification_link()
     assert addr is c.user.address_object('test_admin@domain.net')
     c.user.claim_address('test@DOMAIN.NET')
     assert 'test@domain.net' in c.user.email_addresses
 
-
 @td.with_user_project('test-admin')
 @with_setup(setUp)
 def test_user():
@@ -173,7 +175,7 @@ def test_default_project_roles():
 
 @with_setup(setUp)
 def test_email_address_claimed_by_user():
-    addr = M.EmailAddress(_id='test_admin@domain.net',
+    addr = M.EmailAddress(email='test_admin@domain.net',
                           claimed_by_user_id=c.user._id)
     c.user.disabled = True
     ThreadLocalORMSession.flush_all()

http://git-wip-us.apache.org/repos/asf/allura/blob/3e6fdda3/Allura/allura/tests/test_mail_util.py
----------------------------------------------------------------------
diff --git a/Allura/allura/tests/test_mail_util.py b/Allura/allura/tests/test_mail_util.py
index 64a1c92..c2fe363 100644
--- a/Allura/allura/tests/test_mail_util.py
+++ b/Allura/allura/tests/test_mail_util.py
@@ -193,7 +193,7 @@ class TestIdentifySender(object):
         EA.query.get.side_effect = [
             mock.Mock(claimed_by_user_id=True, claimed_by_user=lambda:'user')]
         assert_equal(identify_sender(None, 'arg', None, None), 'user')
-        EA.query.get.assert_called_once_with(_id='arg')
+        EA.query.get.assert_called_once_with(email='arg')
 
     @mock.patch('allura.model.EmailAddress')
     def test_header(self, EA):
@@ -203,7 +203,7 @@ class TestIdentifySender(object):
         assert_equal(
             identify_sender(None, 'arg', {'From': 'from'}, None), 'user')
         assert_equal(EA.query.get.call_args_list,
-                     [mock.call(_id='arg'), mock.call(_id='from')])
+                     [mock.call(email='arg'), mock.call(email='from')])
 
     @mock.patch('allura.model.User')
     @mock.patch('allura.model.EmailAddress')
@@ -213,7 +213,7 @@ class TestIdentifySender(object):
         EA.query.get.side_effect = [
             None, mock.Mock(claimed_by_user_id=True, claimed_by_user=lambda:'user')]
         assert_equal(identify_sender(None, 'arg', {}, None), anon)
-        assert_equal(EA.query.get.call_args_list, [mock.call(_id='arg')])
+        assert_equal(EA.query.get.call_args_list, [mock.call(email='arg')])
 
     @mock.patch('allura.model.User')
     @mock.patch('allura.model.EmailAddress')
@@ -224,7 +224,7 @@ class TestIdentifySender(object):
         assert_equal(
             identify_sender(None, 'arg', {'From': 'from'}, None), anon)
         assert_equal(EA.query.get.call_args_list,
-                     [mock.call(_id='arg'), mock.call(_id='from')])
+                     [mock.call(email='arg'), mock.call(email='from')])
 
 
 def test_parse_message_id():


Mime
View raw message