Merge lp:~jamestait/django-openid-auth/revert-account-verified-model-change into lp:~django-openid-auth/django-openid-auth/trunk

[James Tait]

Revert the UserOpenID model change which added the account_verified flag. The intention was to allow for the support of multiple UserOpenIDs for a given User, but in reality the solution wouldn't have provided this, because we have no idea which OP provided the e-mail address associated with the account. In the meantime, allow for the simple case of handling account_verified for a single UserOpenID, while not ruling out future support for the complex case.

This branch reverts the addition of the account_verified flag to the
UserOpenID model.

The intention was to allow for the support of multiple UserOpenIDs for a
given User, but the solution as implemented would not have provided this.

If the OpenID response contains an e-mail address then we *do* know where
the e-mail address associated with the Django account came from - it came
from the same OP that's now making assertions as to the validity of the
address, because we're in the process of updating the user's details
with the ones in the OpenID response.

But this doesn't necessarily help us, because we don't maintain the link
between the e-mail address and the OpenID, so even if the same e-mail
address is used with two (trusted) OPs, if they differ in their opinion of
the validity of the address, we have no way of saying "but it's OK, because
the other OP has verified it."

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

=== modified file 'django_openid_auth/auth.py'
--- django_openid_auth/auth.py	2013-05-16 17:30:45 +0000
+++ django_openid_auth/auth.py	2013-06-21 17:58:35 +0000
@@ -80,8 +80,7 @@
                 claimed_id__exact=openid_response.identity_url)
         except UserOpenID.DoesNotExist:
             if getattr(settings, 'OPENID_CREATE_USERS', False):
-                user, user_openid = self.create_user_from_openid(
-                    openid_response)
+                user = self.create_user_from_openid(openid_response)
         else:
             user = user_openid.user
 
@@ -90,7 +89,7 @@
 
         if getattr(settings, 'OPENID_UPDATE_DETAILS_FROM_SREG', False):
             details = self._extract_user_details(openid_response)
-            self.update_user_details(user_openid, details, openid_response)
+            self.update_user_details(user, details, openid_response)
 
         if getattr(settings, 'OPENID_PHYSICAL_MULTIFACTOR_REQUIRED', False):
             pape_response = pape.Response.fromSuccessResponse(openid_response)
@@ -275,10 +274,10 @@
             openid_response.identity_url)
 
         user = User.objects.create_user(username, email, password=None)
-        user_openid = self.associate_openid(user, openid_response)
-        self.update_user_details(user_openid, details, openid_response)
+        self.associate_openid(user, openid_response)
+        self.update_user_details(user, details, openid_response)
 
-        return user, user_openid
+        return user
 
     def associate_openid(self, user, openid_response):
         """Associate an OpenID with a user account."""
@@ -300,8 +299,7 @@
 
         return user_openid
 
-    def update_user_details(self, user_openid, details, openid_response):
-        user = user_openid.user
+    def update_user_details(self, user, details, openid_response):
         updated = False
         if details['first_name']:
             user.first_name = details['first_name'][:30]
@@ -313,13 +311,18 @@
             user.email = details['email']
             updated = True
         if getattr(settings, 'OPENID_FOLLOW_RENAMES', False):
-            user.username = self._get_available_username(details['nickname'], openid_response.identity_url)
+            user.username = self._get_available_username(
+                details['nickname'], openid_response.identity_url)
             updated = True
         account_verified = details.get('account_verified', None)
-        if (account_verified is not None and
-                user_openid.account_verified != account_verified):
-            user_openid.account_verified = account_verified
-            user_openid.save()
+        if (account_verified is not None):
+            permission = Permission.objects.get(codename='account_verified')
+            perm_label = '%s.%s' % (permission.content_type.app_label,
+                                    permission.codename)
+            if account_verified and not user.has_perm(perm_label):
+                user.user_permissions.add(permission)
+            elif not account_verified and user.has_perm(perm_label):
+                user.user_permissions.remove(permission)
 
         if updated:
             user.save()

=== modified file 'django_openid_auth/models.py'
--- django_openid_auth/models.py	2013-05-24 23:33:30 +0000
+++ django_openid_auth/models.py	2013-06-21 17:58:35 +0000
@@ -59,27 +59,13 @@
     user = models.ForeignKey(User)
     claimed_id = models.TextField(max_length=2047, unique=True)
     display_id = models.TextField(max_length=2047)
-    account_verified = models.BooleanField(default=False)
 
     class Meta:
         permissions = (
             ('account_verified', 'The OpenID has been verified'),
         )
 
-    def _get_permission(self):
-        return Permission.objects.get(codename='account_verified')
-
-    def save(self, force_insert=False, force_update=False, using=None):
-        permission = self._get_permission()
-        perm_label = '%s.%s' % (permission.content_type.app_label,
-                                permission.codename)
-        if self.account_verified and not self.user.has_perm(perm_label):
-            self.user.user_permissions.add(permission)
-        elif not self.account_verified and self.user.has_perm(perm_label):
-            self.user.user_permissions.remove(permission)
-        super(UserOpenID, self).save(force_insert, force_update, using)
-
     def delete(self, using=None):
-        permission = self._get_permission()
+        permission = Permission.objects.get(codename='account_verified')
         self.user.user_permissions.remove(permission)
         super(UserOpenID, self).delete(using)

=== modified file 'django_openid_auth/tests/test_auth.py'
--- django_openid_auth/tests/test_auth.py	2013-06-18 14:10:24 +0000
+++ django_openid_auth/tests/test_auth.py	2013-06-21 17:58:35 +0000
@@ -29,7 +29,11 @@
 import unittest
 
 from django.conf import settings
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import (
+    Group,
+    Permission,
+    User,
+)
 from django.test import TestCase
 
 from django_openid_auth.auth import OpenIDBackend
@@ -181,13 +185,12 @@
         user_openid, created = UserOpenID.objects.get_or_create(
             user=user,
             claimed_id='http://example.com/existing_identity',
-            display_id='http://example.com/existing_identity',
-            account_verified=False)
+            display_id='http://example.com/existing_identity')
         data = dict(first_name=u"Some56789012345678901234567890123",
             last_name=u"User56789012345678901234567890123",
             email=u"[e-mail]", account_verified=False)
 
-        self.backend.update_user_details(user_openid, data, response)
+        self.backend.update_user_details(user, data, response)
 
         self.assertEqual("Some56789012345678901234567890",  user.first_name)
         self.assertEqual("User56789012345678901234567890",  user.last_name)
@@ -206,35 +209,44 @@
             user=user, claimed_id=claimed_id, display_id=display_id)
         return user_openid
 
-    def _test_account_verified(self, user_openid, verified, expected):
+    def _test_account_verified(self, user, initially_verified, expected):
         # set user's verification status
-        user_openid.account_verified = verified
+        permission = Permission.objects.get(codename='account_verified')
+        if initially_verified:
+            user.user_permissions.add(permission)
+        else:
+            user.user_permissions.remove(permission)
+
+        if hasattr(user, '_perm_cache'):
+            del user._perm_cache
 
         # get a response including verification status
         response = self.make_response_ax()
         data = dict(first_name=u"Some56789012345678901234567890123",
-            last_name=u"User56789012345678901234567890123",
-            email=u"[e-mail]", account_verified=expected)
-        self.backend.update_user_details(user_openid, data, response)
+                    last_name=u"User56789012345678901234567890123",
+                    email=u"[e-mail]",
+                    account_verified=expected)
+        self.backend.update_user_details(user, data, response)
 
         # refresh object from the database
-        user_openid = UserOpenID.objects.get(pk=user_openid.pk)
+        user = User.objects.get(pk=user.pk)
         # check the verification status
-        self.assertEqual(user_openid.account_verified, expected)
-        self.assertEqual(user_openid.user.has_perm(
-            'django_openid_auth.account_verified'), expected)
-
-    def test_update_user_openid_unverified(self):
-        user_openid = self.make_user_openid()
-
-        for verified in (False, True):
-            self._test_account_verified(user_openid, verified, expected=False)
-
-    def test_update_user_openid_verified(self):
-        user_openid = self.make_user_openid()
-
-        for verified in (False, True):
-            self._test_account_verified(user_openid, verified, expected=True)
+        self.assertEqual(user.has_perm('django_openid_auth.account_verified'),
+                         expected)
+
+    def test_update_user_perms_unverified(self):
+        user_openid = self.make_user_openid()
+
+        for initially_verified in (False, True):
+            self._test_account_verified(
+                user_openid.user, initially_verified, expected=False)
+
+    def test_update_user_perms_verified(self):
+        user_openid = self.make_user_openid()
+
+        for initially_verified in (False, True):
+            self._test_account_verified(
+                user_openid.user, initially_verified, expected=True)
 
     def test_extract_user_details_name_with_trailing_space(self):
         response = self.make_response_ax(fullname="SomeUser ")

=== modified file 'django_openid_auth/tests/test_models.py'
--- django_openid_auth/tests/test_models.py	2013-05-24 23:33:30 +0000
+++ django_openid_auth/tests/test_models.py	2013-06-21 17:58:35 +0000
@@ -31,7 +31,10 @@
 from django.contrib.auth.models import User
 from django.test import TestCase
 
-from django_openid_auth.models import UserOpenID
+from django_openid_auth.models import (
+    Permission,
+    UserOpenID,
+)
 
 
 class UserOpenIDModelTestCase(TestCase):
@@ -42,47 +45,26 @@
         user_openid, created = UserOpenID.objects.get_or_create(
             user=user,
             claimed_id='http://example.com/existing_identity',
-            display_id='http://example.com/existing_identity',
-            account_verified=False)
+            display_id='http://example.com/existing_identity')
 
         self.assertEqual('someuser', user_openid.user.username)
         self.assertEqual(
             user_openid.claimed_id, 'http://example.com/existing_identity')
         self.assertEqual(
             user_openid.display_id, 'http://example.com/existing_identity')
-        self.assertFalse(user_openid.account_verified)
         self.assertFalse(
             User.objects.get(username='someuser').has_perm(
                 'django_openid_auth.account_verified'))
 
-    def test_create_verified_useropenid(self):
-        user = User.objects.create_user('someuser', '[e-mail]',
-                                        password=None)
-        user_openid, created = UserOpenID.objects.get_or_create(
-            user=user,
-            claimed_id='http://example.com/existing_identity',
-            display_id='http://example.com/existing_identity',
-            account_verified=True)
-
-        self.assertEqual('someuser', user_openid.user.username)
-        self.assertEqual(
-            user_openid.claimed_id, 'http://example.com/existing_identity')
-        self.assertEqual(
-            user_openid.display_id, 'http://example.com/existing_identity')
-        self.assertTrue(user_openid.account_verified)
-        self.assertTrue(
-            User.objects.get(username='someuser').has_perm(
-                'django_openid_auth.account_verified'))
-
     def test_delete_verified_useropenid(self):
         user = User.objects.create_user('someuser', '[e-mail]',
                                         password=None)
         user_openid, created = UserOpenID.objects.get_or_create(
             user=user,
             claimed_id='http://example.com/existing_identity',
-            display_id='http://example.com/existing_identity',
-            account_verified=True)
-
+            display_id='http://example.com/existing_identity')
+        permission = Permission.objects.get(codename='account_verified')
+        user.user_permissions.add(permission)
         self.assertTrue(
             User.objects.get(username='someuser').has_perm(
                 'django_openid_auth.account_verified'))

=== modified file 'django_openid_auth/tests/test_views.py'
--- django_openid_auth/tests/test_views.py	2013-06-18 15:20:13 +0000
+++ django_openid_auth/tests/test_views.py	2013-06-21 17:58:35 +0000
@@ -259,8 +259,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         # The login form is displayed:
@@ -300,8 +299,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         settings.LOGIN_REDIRECT_URL = '/getuser/'
@@ -326,8 +324,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         # Requesting the login form immediately begins an
@@ -467,8 +464,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -512,8 +508,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -568,8 +563,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -723,8 +717,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -752,8 +745,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -780,16 +772,14 @@
         UserOpenID.objects.get_or_create(
             user=user,
             claimed_id='http://example.com/existing_identity',
-            display_id='http://example.com/existing_identity',
-            account_verified=False)
+            display_id='http://example.com/existing_identity')
 
         # Setup user who is going to try to change username to 'testuser'
         renamed_user = User.objects.create_user('renameuser', '[e-mail]')
         UserOpenID.objects.get_or_create(
             user=renamed_user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
 
         # identity url is for 'renameuser'
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -819,16 +809,14 @@
         UserOpenID.objects.get_or_create(
             user=user,
             claimed_id='http://example.com/existing_identity',
-            display_id='http://example.com/existing_identity',
-            account_verified=False)
+            display_id='http://example.com/existing_identity')
 
         # Setup user who is going to try to change username to 'testuser'
         renamed_user = User.objects.create_user('testuser2000eight', '[e-mail]')
         UserOpenID.objects.get_or_create(
             user=renamed_user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
 
         # identity url is for 'testuser2000eight'
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -861,16 +849,14 @@
         UserOpenID.objects.get_or_create(
             user=user,
             claimed_id='http://example.com/existing_identity',
-            display_id='http://example.com/existing_identity',
-            account_verified=False)
+            display_id='http://example.com/existing_identity')
 
         # Setup user who is going to try to change username to 'testuser'
         renamed_user = User.objects.create_user('testuser2000', '[e-mail]')
         UserOpenID.objects.get_or_create(
             user=renamed_user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
 
         # identity url is for 'testuser2000'
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -901,8 +887,7 @@
         UserOpenID.objects.get_or_create(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
 
         # identity url is for 'testuser2'
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -996,8 +981,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/different_identity',
-            display_id='http://example.com/different_identity',
-            account_verified=False)
+            display_id='http://example.com/different_identity')
         useropenid.save()
 
         # Posting in an identity URL begins the authentication request:
@@ -1042,8 +1026,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/different_identity',
-            display_id='http://example.com/different_identity',
-            account_verified=False)
+            display_id='http://example.com/different_identity')
         useropenid.save()
 
         # Posting in an identity URL begins the authentication request:
@@ -1102,8 +1085,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         openid_req = {'openid_identifier': 'http://example.com/identity',
@@ -1128,8 +1110,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         # Posting in an identity URL begins the authentication request:
@@ -1149,8 +1130,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         # Posting in an identity URL begins the authentication request:
@@ -1246,9 +1226,6 @@
         # So have the user's permissions
         self.assertEqual(
             user.has_perm('django_openid_auth.account_verified'), is_verified)
-        # And the verified status of their UserOpenID
-        user_openid = UserOpenID.objects.get(user=user)
-        self.assertEqual(user_openid.account_verified, is_verified)
 
     def test_login_attribute_exchange_with_verification(self):
         settings.OPENID_VALID_VERIFICATION_SCHEMES = {
@@ -1314,8 +1291,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=True)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         # Posting in an identity URL begins the authentication request:
@@ -1359,8 +1335,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         # Posting in an identity URL begins the authentication request:
@@ -1412,8 +1387,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
 
         # Posting in an identity URL begins the authentication request:
@@ -1437,8 +1411,7 @@
         useropenid = UserOpenID(
             user=user,
             claimed_id='http://example.com/identity',
-            display_id='http://example.com/identity',
-            account_verified=False)
+            display_id='http://example.com/identity')
         useropenid.save()
         response = self.client.post('/openid/login/',
             {'openid_identifier': 'http://example.com/identity'})