Merge lp:~jamestait/canonical-identity-provider/lp1167645 into lp:canonical-identity-provider/release

[James Tait]

Ensure that for trusted, auto-authorize RPs, AX data is still returned.

Ensure that for trusted, auto-authorize RPs, AX data is still returned.

[Ricardo Kirkner (ricardokirkner)]

<pindonga> 1. you added the ax extension to the request in the server views... this means this wasn't handled at all previously?
<jayteeuk> It wasn't handled in a specific situation - namely, when the RP was trusted and configured to auto-authorize.
<pindonga> so this addition is only in that case?
<pindonga> (can't get that from the diff context)
<jayteeuk> Correct - it's not obvious from the code, but we end up in that block specifically under those circumstances.

<pindonga> 2. care to explain why the data_approved_for_request also needs to be returned on GET (I'm sure it's correct, just want the context)
<pindonga> jayteeuk, did you see question 2 up there?
<jayteeuk> I was about to answer 2) but I think what I was about to say might not be the reason I changed that - let me go get my notebook. :)
<pindonga> :)
<jayteeuk> pindonga, all I have scribbled is "This will never have data for GET", with no explanation. :-(
<pindonga> I recall you saying something about auto-authorize not posting but getting, but don't recall the specifics
<jayteeuk> I'll have another look and make sure, because I'm not totally convinced that's true either.

<pindonga> ok, let me ask other stuff
<pindonga> 3. test_fields_for_trusted_auto_authorize_site looks good, but triggers the question, do we have a test that shows that optional attrs are also returned for trusted/auto-authorized sites?
<jayteeuk> pindonga, lines 86-87 request an optional attribute - maybe that could use a couple of comments.
<pindonga> ah, no, I missed the required=False
<pindonga> sorry
<pindonga> why not using assertIn instead of assertTrue in that test?
<jayteeuk> pindonga, good point - I made the same mistake last time.
<pindonga> not a mistake, really :)
<jayteeuk> No, but good style.

<pindonga> 4. in test_handle_user_response_ax_openid_is_authorized_idselect, why do you only assert the email? what's the intent of this test?
<pindonga> that's all I have
<jayteeuk> 4) I should add the other two attributes to the tests - language isn't allowed in the rpconfig, so won't be returned.
<jayteeuk> The test was to ensure the AX attributes were being returned.

<jayteeuk> pindonga, I need to reboot quickly before the weekly call, could you drop those questions in the MP, and I'll address them afterwards?
<pindonga> kk
<jayteeuk> Thanks!
<jayteeuk> Good review!
<jayteeuk> brb!

review: Needs Information

[James Tait (jamestait)]

17:20 < jayteeuk> Indirect communication passes messages through the user-agent, can be initiated by either the RP or the OP, and is
                  used for authentication requests and authentication responses.
17:21 < jayteeuk> "There are two methods for indirect communication: HTTP redirects and HTML form submission. Both form submission and
                  redirection require that the sender know a recipient URL and that the recipient URL expect indirect messages, as
                  specified in Section 4.1.2. The initiator of the communication chooses which method of indirect communication is
                  appropriate depending on capabilities, message size, or other external factors."
17:22 < jayteeuk> So, pretty much, if we're going to interact with RPs outside of our control, they can decide whether to use GETs or
                  POSTs.
17:23 < jayteeuk> A GET request is perfectly valid.

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

=== modified file 'identityprovider/forms.py'
--- identityprovider/forms.py	2013-04-09 14:24:53 +0000
+++ identityprovider/forms.py	2013-04-15 16:55:33 +0000
@@ -361,7 +361,7 @@
     callback = forms.CharField(error_messages=default_errors)
 
 
-def _get_data_for_user(request, fields, with_verified=False):
+def _get_data_for_user(request, fields, for_ax=False):
     """Get the data to ask about in the form based on the user's
     account record.
     """
@@ -370,9 +370,6 @@
     values['fullname'] = user.displayname
     if user.preferredemail is not None:
         values['email'] = user.preferredemail.email
-        if with_verified:
-            values['account_verified'] = (
-                'token_via_email' if user.is_verified else 'no')
     if user.person is not None:
         values['nickname'] = user.person.name
         if user.person.time_zone is not None:
@@ -381,7 +378,11 @@
         values['language'] = user.preferredlanguage
     else:
         values['language'] = translation.get_language_from_request(request)
-    logger.debug("values (sreg_fields) = " + str(values))
+    if for_ax:
+        values['account_verified'] = (
+            'token_via_email' if user.is_verified else 'no')
+    logger.debug('values (%s_fields) = %s',
+                 'ax' if for_ax else 'sreg', str(values))
 
     return dict([(f, values[f]) for f in fields if f in values])
 
@@ -419,7 +420,7 @@
                 fields = set()
         else:
             fields = sreg_fields
-        self.data = _get_data_for_user(request, fields, with_verified=False)
+        self.data = _get_data_for_user(request, fields, for_ax=False)
 
         super(SRegRequestForm, self).__init__(self.data)
         self._init_fields(self.data)
@@ -486,7 +487,7 @@
         ax_fields = self._get_requested_field_aliases()
         if rpconfig is not None:
             ax_fields = self._filter_allowed_fields(ax_fields)
-        self.data = _get_data_for_user(request, ax_fields, with_verified=True)
+        self.data = _get_data_for_user(request, ax_fields, for_ax=True)
 
         super(AXFetchRequestForm, self).__init__(self.data)
         self._init_fields(self.data)
@@ -569,11 +570,9 @@
     @property
     def data_approved_for_request(self):
         """Return the list of ax data approved for the request."""
-        if self.request_method == 'POST':
-            return dict(
-                [(f, self.data[f]) for f in self.data
-                 if self.field_approved(AX_DATA_FIELDS.getNamespaceURI(f))])
-        return {}
+        return dict(
+            [(f, self.data[f]) for f in self.data
+             if self.field_approved(AX_DATA_FIELDS.getNamespaceURI(f))])
 
     @property
     def has_data(self):

=== modified file 'identityprovider/tests/test_forms.py'
--- identityprovider/tests/test_forms.py	2013-04-02 09:26:22 +0000
+++ identityprovider/tests/test_forms.py	2013-04-15 16:55:33 +0000
@@ -435,8 +435,27 @@
             AttrInfo(AX_URI_EMAIL, alias='email', required=False))
         form = AXFetchRequestForm(self._get_request_with_post_args(),
                                   fetch_request, self.rpconfig)
-        self.assertTrue('fullname' in form.data_approved_for_request)
-        self.assertFalse('email' in form.data_approved_for_request)
+        self.assertIn('fullname', form.data_approved_for_request)
+        self.assertNotIn('email', form.data_approved_for_request)
+
+    def test_fields_for_trusted_auto_authorize_site(self):
+        """The server should always return values for requested fields to
+        trusted sites configured to auto-authorize.
+        """
+        self.rpconfig.allowed_ax = 'fullname,email'
+        self.rpconfig.auto_authorize = True
+        fetch_request = FetchRequest()
+        # One required attribute
+        fetch_request.add(
+            AttrInfo(AX_URI_FULL_NAME, alias='fullname', required=True))
+        # One optional attribute
+        fetch_request.add(
+            AttrInfo(AX_URI_EMAIL, alias='email', required=False))
+        form = AXFetchRequestForm(self._get_request_with_post_args(),
+                                  fetch_request, self.rpconfig)
+        # Both attributes should be returned
+        self.assertIn('fullname', form.data_approved_for_request)
+        self.assertIn('email', form.data_approved_for_request)
 
     def test_optional_fields_for_trusted_site(self):
         """The server should return values for optional fields to trusted

=== modified file 'identityprovider/tests/test_views_server.py'
--- identityprovider/tests/test_views_server.py	2013-04-02 09:26:22 +0000
+++ identityprovider/tests/test_views_server.py	2013-04-15 16:55:33 +0000
@@ -167,6 +167,31 @@
         self.assertEqual(r.status_code, 200)
         self.assertTemplateUsed(r, 'server/invalid_identifier.html')
 
+    def test_handle_user_response_ax_openid_is_authorized_idselect(self):
+        # update rp to auto authorize
+        self.rpconfig.auto_authorize = True
+        self.rpconfig.allowed_ax = 'fullname,email,account_verified'
+        self.rpconfig.save()
+
+        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',
+        })
+        r = self.client.get(self.url, self.params)
+        query = self.get_query(r)
+        self.assertEqual(query['openid.ax.value.email.1'],
+                         quote_plus(self.email))
+        self.assertEqual(query['openid.ax.value.fullname.1'],
+                         quote_plus(self.account.get_full_name()))
+        self.assertEqual(query['openid.ax.value.account_verified.1'],
+                         'token_via_email')
+
     def test_handle_user_response_openid_is_authorized_idselect(self):
         # update rp to auto authorize
         self.rpconfig.auto_authorize = True

=== modified file 'identityprovider/views/server.py'
--- identityprovider/views/server.py	2013-04-02 23:01:33 +0000
+++ identityprovider/views/server.py	2013-04-15 16:55:33 +0000
@@ -176,6 +176,7 @@
             else:
                 oresponse = orequest.answer(True)
             _add_sreg(request, orequest, oresponse)
+            _add_ax(request, orequest, oresponse)
             _check_team_membership(request, orequest, oresponse,
                                    immediate=True)
             response = _django_response(request, oresponse, True)
@@ -197,6 +198,7 @@
         else:
             oresponse = orequest.answer(True)
         _add_sreg(request, orequest, oresponse)
+        _add_ax(request, orequest, oresponse)
         _check_team_membership(request, orequest, oresponse, immediate=True)
         response = _django_response(request, oresponse, True)
     elif (twofactor.is_authenticated(request) and not