Merge lp:~canonical-isd-hackers/canonical-identity-provider/reconfirm_sreg_fields into lp:~canonical-isd-hackers/canonical-identity-provider/stable

[Anthony Lenton]

Require the user to re-confirm they want to sign in every time, for untrusted RPs.

Overview
========
This branch requires the user to re-confirm which details to send to the RP each time they sign in, for untrusted RPs

Details
=======
Three small improvements related to this:
 - OpenIDAuthorizations expire immediately, so that you always need to reconfirm that you wish to sign in to an untrusted RP, and which details to send.
 - Fixed a small bug so that even for untrusted RPs the form is prepopulated with defaults matching the details you sent last time.
 - Made the "Team membership" label work as a label for the team membership checkbox.

[Ricardo Kirkner (ricardokirkner)]

Looks really good. However I still have one question.

If we're ignoring the expiration date, because we're automatically expiring the authorizations, why not get rid of the column at all? (and all relevant checks)

[Ricardo Kirkner (ricardokirkner)]

After talking via IRC, we cannot remove the expiration date field yet, because it's being used by the pre-authorize functionality.

=== modified file 'identityprovider/const.py'
--- identityprovider/const.py	2010-09-14 22:32:05 +0000
+++ identityprovider/const.py	2011-07-26 00:11:18 +0000
@@ -5,8 +5,6 @@
 
 LAUNCHPAD_TEAMS_NS = 'http://ns.launchpad.net/2007/openid-teams'
 
-NEVER_EXPIRES = datetime.date(datetime.MAXYEAR, 12, 31)
-
 PERSON_VISIBILITY_PUBLIC = 1
 PERSON_VISIBILITY_PRIVATE_MEMBERSHIP = 20
 PERSON_VISIBILITY_PRIVATE = 30

=== modified file 'identityprovider/models/openidmodels.py'
--- identityprovider/models/openidmodels.py	2011-07-08 16:51:05 +0000
+++ identityprovider/models/openidmodels.py	2011-07-26 00:11:18 +0000
@@ -2,9 +2,8 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import base64
-import datetime
 import time
-
+from datetime import datetime
 from openid.association import Association
 import openid.store
 import openid.store.nonce
@@ -15,7 +14,7 @@
 from django.utils import simplejson as json
 from django.utils.translation import ugettext_lazy as _
 
-from identityprovider.const import NEVER_EXPIRES, SREG_LABELS
+from identityprovider.const import SREG_LABELS
 from identityprovider.models import Account
 from identityprovider.models.const import AccountCreationRationale
 
@@ -45,17 +44,17 @@
 
 class OpenIDAuthorizationManager(models.Manager):
 
-    def authorize(self, account, trust_root, expires, client_id=None):
+    def authorize(self, account, trust_root, expires=None, client_id=None):
         if settings.READ_ONLY_MODE:
             return
         if expires is None:
-            expires = NEVER_EXPIRES
+            expires = datetime.utcnow()
         try:
             existing = OpenIDAuthorization.objects.get(
                 account=account,
                 client_id=client_id,
                 trust_root=trust_root)
-            existing.date_created = datetime.datetime.utcnow()
+            existing.date_created = datetime.utcnow()
             existing.date_expires = expires
             existing.save()
         except OpenIDAuthorization.DoesNotExist:
@@ -70,7 +69,7 @@
             account__exact=account,
             trust_root__exact=trust_root,
             client_id__exact=None,
-            date_expires__gte=datetime.datetime.utcnow()).count()
+            date_expires__gte=datetime.utcnow()).count()
         if client_none > 0:
             return True
         else:
@@ -78,7 +77,7 @@
                 account__exact=account,
                 trust_root__exact=trust_root,
                 client_id__exact=client_id,
-                date_expires__gte=datetime.datetime.utcnow()).count()
+                date_expires__gte=datetime.utcnow()).count()
             if client_match > 0:
                 return True
         return False
@@ -87,7 +86,7 @@
 class OpenIDAuthorization(models.Model):
     account = models.ForeignKey(Account, db_column='account')
     client_id = models.TextField(blank=True, null=True)
-    date_created = models.DateTimeField(default=datetime.datetime.utcnow,
+    date_created = models.DateTimeField(default=datetime.utcnow,
         blank=True, editable=False)
     date_expires = models.DateTimeField()
     trust_root = models.TextField()
@@ -204,9 +203,9 @@
     account = models.ForeignKey(Account, db_column='account')
     openid_identifier = models.TextField()
     trust_root = models.TextField()
-    date_created = models.DateTimeField(default=datetime.datetime.utcnow,
+    date_created = models.DateTimeField(default=datetime.utcnow,
         blank=True, editable=False)
-    date_last_used = models.DateTimeField(default=datetime.datetime.utcnow,
+    date_last_used = models.DateTimeField(default=datetime.utcnow,
         blank=True, editable=False)
     total_logins = models.IntegerField(default=1)
     approved_data = models.TextField(blank=True, null=True, default='')
@@ -220,7 +219,7 @@
 
     def increment(self):
         self.total_logins += 1
-        self.date_last_used = datetime.datetime.utcnow()
+        self.date_last_used = datetime.utcnow()
         self.save()
 
     def get_approved_data(self):

=== modified file 'identityprovider/templates/decide.html'
--- identityprovider/templates/decide.html	2011-07-01 19:47:04 +0000
+++ identityprovider/templates/decide.html	2011-07-26 00:11:18 +0000
@@ -44,10 +44,11 @@
                 {% if teams_form.should_display %}
                     {% ifequal teams_form.fields|length 1 %}
                         {% for field in teams_form %}
-                <li>{{ field|safe }} {% trans "Team membership:"%} {{ field.label_tag }}</li>
+                <li>{{ field|safe }} <label for="id_{{ field.html_name }}">{% trans "Team membership:"%}</label> {{ field.label_tag }}</li>
                         {% endfor %}
                     {% else %}
-                <li id="teamslist_item">{% trans "Team membership:"%}
+                <li id="teamslist_item">
+                    <span id="teamslist_label">{% trans "Team membership:"%}</span>
                     <ul class="teams-list" id="teamslist">
                         {% for field in teams_form %}
                         <li>{{ field|safe }} {{ field.label_tag }}</li>
@@ -114,6 +115,7 @@
                     return;
                 }
                 Y.one('#teamslist_item').prepend('<input type="checkbox" id="checkme" /> ');
+                Y.one('#teamslist_label').wrap('<label for="checkme"></label>');
                 Y.one('#checkme').on('change', setTeamStates);
                 Y.all('#teamslist li').on('change', refreshTeamStates);
                 refreshTeamStates();
@@ -135,7 +137,9 @@
                     for (i = 0; i < c.size(); i++) {
                         buf.push(c.item(i).get('text'));
                     }
-                    node.insert('<span id="reveal_wrapper">' + buf.join(', ') + ' <small style="font-size: 80%;">(<a href="#" id="reveal">details</a>)</small></span>', "before");
+                    node.insert('<span id="reveal_wrapper"><label for="checkme">' +
+                        buf.join(', ') + '</label> <small style="font-size: 80%;">' +
+                        '(<a href="#" id="reveal">details</a>)</small></span>', "before");
                     Y.one('#reveal').on('click', onClick);
                 }
             });

=== modified file 'identityprovider/tests/functional/openid_/per_version/test_restricted_sreg.py'
--- identityprovider/tests/functional/openid_/per_version/test_restricted_sreg.py	2011-07-19 14:33:08 +0000
+++ identityprovider/tests/functional/openid_/per_version/test_restricted_sreg.py	2011-07-26 00:11:18 +0000
@@ -3,7 +3,11 @@
 from openid.store.memstore import MemoryStore
 from openid.consumer.discover import OPENID_2_0_TYPE as PROTOCOL_URI
 
-from identityprovider.models.openidmodels import OpenIDRPConfig
+from identityprovider.models.openidmodels import (
+    OpenIDAuthorization,
+    OpenIDRPConfig,
+    OpenIDRPSummary,
+    )
 
 from ...openidhelpers import complete_from_browser, make_endpoint
 from ...helpers import FunctionalTestCase
@@ -29,10 +33,11 @@
 
         # If a relying party attempts to request user details via the
         # openid.sreg extension and Launchpad does not have a particular policy
-        # configured, then only the user's approved fields are returned in the response.
+        # configured, then only the user's approved fields are returned in the
+        # response.
 
-        # We will perform an OpenID authentication request asking for a few user
-        # details:
+        # We will perform an OpenID authentication request asking for a few
+        # user details:
         openid_store = MemoryStore()
         consumer = Consumer(session={}, store=openid_store)
 
@@ -59,8 +64,10 @@
         # authorize data
         # required fields are checked by default. don't authorize
         # anything
-        self.assertEqual(self.browser.getControl(name='email').disabled, False)
-        self.browser.getControl(name='email').value = False
+        email = self.browser.getControl(name='email')
+        self.assertEqual(email.disabled, False)
+        self.assertTrue(email.mech_control.items[0].selected)
+        email.value = False
         self.browser.getControl(name='yes', index=0).click()
 
         self.assertUrl('http://launchpad.dev/\+openid-consumer\?.*?')
@@ -79,11 +86,31 @@
 
         self.assertEquals(sreg_response, None)
 
+        # If we attempt to authenticate again, we will be prompted to
+        # confirm which fields we want to provide to the RP again,
+        # but the defaults will be what we provided last time:
+
+        request = consumer.beginWithoutDiscovery(endpoint)
+        request.addExtension(SRegRequest(
+            required=['email', 'country'],
+            optional=['fullname', 'nickname']))
+        redirect_url = request.redirectURL(
+            'http://launchpad.dev/', 'http://launchpad.dev/+openid-consumer')
+        self.assertRegexpMatches(redirect_url,
+                                 'http://openid.launchpad.dev/\+openid\?.*?')
+        self.browser.open(redirect_url)
+        # No log in needed this time, we're directed straight to the confirm
+        # screen.  Check that email is *not* selected by default this time:
+        email = self.browser.getControl(name='email')
+        self.assertFalse(email.mech_control.items[0].selected)
+
         # == Behaviour for known trust roots ==
 
         # If we create a Relying Party configuration for the trust root, things
         # play out a bit differently:
 
+        OpenIDRPSummary.objects.all().delete()
+        OpenIDAuthorization.objects.all().delete()
 
         self.create_openid_rp_config(
             trust_root='http://launchpad.dev/',

=== modified file 'identityprovider/tests/test_models_openidmodels.py'
--- identityprovider/tests/test_models_openidmodels.py	2011-07-25 16:47:59 +0000
+++ identityprovider/tests/test_models_openidmodels.py	2011-07-26 00:11:18 +0000
@@ -3,11 +3,11 @@
 
 import base64
 from datetime import datetime, timedelta
+from mock import patch
 from time import time
 from openid.association import Association
 from openid.store.nonce import SKEW
 
-from identityprovider.const import NEVER_EXPIRES
 from identityprovider.models.account import Account
 from identityprovider.models.openidmodels import (DjangoOpenIDStore,
     OpenIDAssociation, OpenIDAuthorization, OpenIDNonce, OpenIDRPConfig,
@@ -176,29 +176,35 @@
         finally:
             rm.clear_readonly()
 
-    def test_authorize_expires(self):
-        OpenIDAuthorization.objects.authorize(self.account, self.trust_root,
-                                              None)
+    @patch('identityprovider.models.openidmodels.datetime')
+    def test_authorize_expires(self, mock_datetime):
+        now = datetime.utcnow()
+        mock_datetime.utcnow.return_value = now
+
+        OpenIDAuthorization.objects.authorize(self.account, self.trust_root)
+
         auth = OpenIDAuthorization.objects.get(account=self.account,
             client_id=None, trust_root=self.trust_root)
-        self.assertEqual(auth.date_expires,
-            datetime.fromordinal(NEVER_EXPIRES.toordinal()))
+        self.assertEqual(auth.date_expires, now)
 
-    def test_authorize_existing(self):
+    @patch('identityprovider.models.openidmodels.datetime')
+    def test_authorize_existing(self, mock_datetime):
+        now = datetime.utcnow()
+        mock_datetime.utcnow.return_value = now
         auth1 = self.create_auth()
-        OpenIDAuthorization.objects.authorize(self.account, self.trust_root,
-                                              None)
+        OpenIDAuthorization.objects.authorize(self.account, self.trust_root)
         auth2 = OpenIDAuthorization.objects.get(pk=auth1.id)
-        self.assertEqual(auth2.date_expires,
-            datetime.fromordinal(NEVER_EXPIRES.toordinal()))
+        self.assertEqual(auth2.date_expires, now)
 
-    def test_authorize_not_existing(self):
+    @patch('identityprovider.models.openidmodels.datetime')
+    def test_authorize_not_existing(self, mock_datetime):
+        now = datetime.utcnow()
+        mock_datetime.utcnow.return_value = now
         OpenIDAuthorization.objects.authorize(self.account, self.trust_root,
                                               None)
         auth = OpenIDAuthorization.objects.get(account=self.account,
             client_id=None, trust_root=self.trust_root)
-        self.assertEqual(auth.date_expires,
-            datetime.fromordinal(NEVER_EXPIRES.toordinal()))
+        self.assertEqual(auth.date_expires, now)
 
     def test_is_authorized_generic(self):
         self.create_auth()
@@ -209,7 +215,7 @@
         self.assertFalse(OpenIDAuthorization.objects.is_authorized(self.account,
             self.trust_root, 'client'))
         OpenIDAuthorization.objects.authorize(self.account, self.trust_root,
-            None, client_id='client')
+            expires=self.expires, client_id='client')
         self.assertTrue(OpenIDAuthorization.objects.is_authorized(self.account,
             self.trust_root, 'client'))
 

=== modified file 'identityprovider/tests/test_views_server.py'
--- identityprovider/tests/test_views_server.py	2011-07-25 17:53:04 +0000
+++ identityprovider/tests/test_views_server.py	2011-07-26 00:11:18 +0000
@@ -13,8 +13,6 @@
 from openid.extensions import pape
 from openid.extensions.sreg import SRegRequest
 
-from mock import patch
-
 from openid.message import (Message, IDENTIFIER_SELECT, OPENID1_URL_LIMIT,
     OPENID2_NS)
 from openid.yadis.constants import YADIS_HEADER_NAME
@@ -324,18 +322,6 @@
         self.assertEqual(r.status_code, 302)
         self.assertEqual(query['openid.mode'], 'id_res')
 
-    @patch('identityprovider.views.server.datetime')
-    def test_process_decide_use_utc(self, mock_datetime):
-        mock_datetime.utcnow.return_value = datetime.datetime.utcnow()
-        mock_datetime.side_effect = lambda *args, **kw: datetime(*args, **kw)
-
-        r = self.client.post("/%s/+decide" % self.token, {'yes': 'yes'})
-        query = self.get_query(r)
-
-        self.assertEqual(r.status_code, 302)
-        self.assertEqual(query['openid.mode'], 'id_res')
-        self.assertEqual(mock_datetime.utcnow.called, True)
-
     def test_decide_authenticated_with_post(self):
         # Using a return-to URL that is more than OPENID1_URL_LIMIT
         # characters long will force the assertion to be sent as a
@@ -539,10 +525,9 @@
                          'Preferred language: en</label></li>')
         # team data is enabled and *not checked*, the label is plain
         team_html = ('<li><input type="checkbox" name="ubuntu-team" '
-                     'value="ubuntu-team" id="id_ubuntu-team" /> Team '
-                     'membership: <label for="id_ubuntu-team">ubuntu-team'
-                     '</label></li>')
-
+                     'value="ubuntu-team" id="id_ubuntu-team" /> '
+                     '<label for="id_ubuntu-team">Team membership:</label> '
+                     '<label for="id_ubuntu-team">ubuntu-team</label></li>')
         self.assertContains(response, username_html)
         self.assertContains(response, email_html)
         self.assertContains(response, language_html)

=== modified file 'identityprovider/views/server.py'
--- identityprovider/views/server.py	2011-07-25 17:53:04 +0000
+++ identityprovider/views/server.py	2011-07-26 00:11:18 +0000
@@ -234,7 +234,7 @@
             try:
                 summary = OpenIDRPSummary.objects.get(
                     account=request.user, trust_root=orequest.trust_root,
-                    openid_identifier=request.user.openid_identifier)
+                    openid_identifier=request.user.openid_identity_url)
                 approved_data = summary.get_approved_data()
             except OpenIDRPSummary.DoesNotExist:
                 approved_data = {}
@@ -552,8 +552,7 @@
         OpenIDAuthorization.objects.authorize(
             request.user,
             orequest.trust_root,
-            datetime.utcnow(),
-            request.session.session_key)
+            client_id=request.session.session_key)
         _add_sreg(request, orequest, oresponse)
         # if there's no submitted POST data, this is an auto-authorized
         # (immediate) request