Merge lp:~jamestait/canonical-identity-provider/if-you-dont-ask-you-dont-get into lp:canonical-identity-provider/release

[James Tait]

Ensure that the attributes returned in the responses for AX and SReg were actually requested via that method, rather than returning all approved attributes in both responses.

Ensure that the attributes returned in the responses for AX and SReg were
actually requested via that method, rather than returning all approved
attributes in both responses.

[Ricardo Kirkner (ricardokirkner)]

l. 92-97: this could be rolled up in a single loop instead

approving anyway, as it's not critical to fix.

review: Approve

=== modified file 'identityprovider/tests/test_views_server.py'
--- identityprovider/tests/test_views_server.py	2013-05-06 08:52:33 +0000
+++ identityprovider/tests/test_views_server.py	2013-05-15 11:50:44 +0000
@@ -31,6 +31,7 @@
 
 import identityprovider.signed as signed
 from identityprovider.const import (
+    AX_DATA_FIELDS,
     AX_URI_ACCOUNT_VERIFIED,
     AX_URI_EMAIL,
     AX_URI_FULL_NAME,
@@ -192,37 +193,84 @@
         self.assertEqual(query['openid.ax.value.account_verified.1'],
                          'token_via_email')
 
-    def test_handle_user_response_auto_auth_large_response(self):
+    def test_handle_user_response_auto_auth_large_ax_sreg_response(self):
         # Make sure we get a large response
         self.account.displayname = 'a' * OPENID1_URL_LIMIT
         self.account.save()
-        self._test_auto_auth()
+        self._test_auto_auth(ax=['email',
+                                 'account_verified',
+                                 'language'],
+                             sreg=['fullname'])
 
-    def test_handle_user_response_auto_auth_borderline_response(self):
+    def test_handle_user_response_auto_auth_borderline_ax_sreg_response(self):
         # Make a response that's small enough that it fits in a redirect before
         # signing, but large enough that it will need a POST after signing. We
         # might want to come up with a more robust method of determining the
         # required length, but this works for now.
         self.account.displayname = 'a' * (OPENID1_URL_LIMIT / 2)
         self.account.save()
-        self._test_auto_auth()
-
-    def _test_auto_auth(self):
+        self._test_auto_auth(ax=['email',
+                                 'account_verified',
+                                 'language'],
+                             sreg=['fullname'])
+
+    def test_handle_user_response_auto_auth_large_ax_only_response(self):
+        self.account.displayname = 'a' * OPENID1_URL_LIMIT
+        self.account.save()
+        self._test_auto_auth(ax=['fullname'])
+
+    def test_handle_user_response_auto_auth_large_sreg_only_response(self):
+        self.account.displayname = 'a' * OPENID1_URL_LIMIT
+        self.account.save()
+        self._test_auto_auth(sreg=['fullname'])
+
+    def _test_auto_auth(self, ax=None, sreg=None):
         # update rp to auto authorize
         self.rpconfig.auto_authorize = True
         self.rpconfig.allowed_user_attribs = 'fullname,email,account_verified'
         self.rpconfig.save()
 
+        # Define expected values for each attribute
+        expected_values = {
+            'email': self.email,
+            'account_verified': 'token_via_email',
+            'language': None,  # disallowed by rpconfig
+            'fullname': self.account.displayname,
+        }
+        expected_fields = [
+            ('openid.claimed_id', self.account.openid_identity_url),
+            ('openid.identity', self.account.openid_identity_url),
+        ]
+        unexpected_fields = []
+
         self.client.login(username=self.email, password=DEFAULT_USER_PASSWORD)
-        self.params.update({
-            'openid.ns.ax': AXMessage.ns_uri,
-            'openid.ax.mode': FetchRequest.mode,
-            'openid.ax.type.fullname': AX_URI_FULL_NAME,
-            'openid.ax.type.email': AX_URI_EMAIL,
-            'openid.ax.type.account_verified': AX_URI_ACCOUNT_VERIFIED,
-            'openid.ax.type.language': AX_URI_LANGUAGE,
-            'openid.ax.required': 'fullname,email,account_verified,language',
-        })
+        if ax:
+            self.params.update({
+                'openid.ns.ax': AXMessage.ns_uri,
+                'openid.ax.mode': FetchRequest.mode,
+                'openid.ax.required': ','.join(ax)
+            })
+            self.params.update(
+                [['openid.ax.type.%s' % alias,
+                  AX_DATA_FIELDS.getNamespaceURI(alias)] for alias in ax])
+            expected_fields += [('openid.ax.mode', 'fetch_response')]
+            expected_fields += [('openid.ax.value.%s.1' % k, v)
+                                for k, v in expected_values.iteritems()
+                                if k in ax and v is not None]
+            unexpected_fields += [('openid.ax.value.%s.1' % k)
+                                  for k, v in expected_values.iteritems()
+                                  if k not in ax or v is None]
+        if sreg:
+            self.params.update({
+                'openid.ns.sreg': 'http://openid.net/sreg/1.0',
+                'openid.sreg.required': ','.join(sreg),
+            })
+            expected_fields += [('openid.sreg.%s' % k, v)
+                                for k, v in expected_values.iteritems()
+                                if k in sreg and v is not None]
+            unexpected_fields += ['openid.sreg.%s' % k
+                                  for k, v in expected_values.iteritems()
+                                  if k not in sreg or v is None]
         response = self.client.post(self.url, self.params)
         self.assertEqual('text/html', response['Content-type'].split(';')[0])
         dom = PyQuery(response.content)
@@ -232,16 +280,10 @@
         self.assertEqual('document.forms[0].submit();', body.get('onload'))
         forms = dom.find('form')
         self.assertEqual(len(forms), 1)
-        expected_fields = (
-            ('openid.claimed_id', self.account.openid_identity_url),
-            ('openid.identity', self.account.openid_identity_url),
-            ('openid.ax.mode', 'fetch_response'),
-            ('openid.ax.value.email.1', self.email),
-            ('openid.ax.value.fullname.1', self.account.displayname),
-            ('openid.ax.value.account_verified.1', 'token_via_email'),
-        )
         for k, v in expected_fields:
             self.assertEqual(v, forms[0].fields[k])
+        for k in unexpected_fields:
+            self.assertNotIn(k, forms[0].fields)
         for k in ('openid.assoc_handle', 'openid.sig'):
             self.assertIn(k, forms[0].fields)
 

=== modified file 'identityprovider/views/server.py'
--- identityprovider/views/server.py	2013-04-30 16:49:09 +0000
+++ identityprovider/views/server.py	2013-05-15 11:50:44 +0000
@@ -607,10 +607,12 @@
         sreg_response = SRegResponse.extractResponse(
             sreg_request, form.data_approved_for_request)
         openid_response.addExtension(sreg_response)
-        ax_response = ax.FetchResponse(ax_request)
-        for k, v in form.data_approved_for_request.iteritems():
-            ax_response.addValue(AX_DATA_FIELDS.getNamespaceURI(k), v)
-        openid_response.addExtension(ax_response)
+        if ax_request is not None:
+            ax_response = ax.FetchResponse(ax_request)
+            for k, v in form.data_approved_for_request.iteritems():
+                if AX_DATA_FIELDS.getNamespaceURI(k) in ax_request:
+                    ax_response.addValue(AX_DATA_FIELDS.getNamespaceURI(k), v)
+            openid_response.addExtension(ax_response)
 
 
 def _process_decide(request, orequest, decision):