Merge lp:~mikemc/canonical-identity-provider/fix-1169632 into lp:canonical-identity-provider/release

[Mike McCracken]

save return_to url from openid request for redirect after email validation (LP: #1169632)

save return_to url from openid request for redirect after email validation (LP: #1169632)

If the openid request includes a return_to URL, this change will save it into the AuthToken that the email validation URL refers to, so that the user is redirected to the RP's requested page instead of to the /+emails page.

TO TEST:
Use an openid RP or use the /consumer/ openid test endpoint to register a new account, and click on the validate link in the resulting email. You should be redirected to the appropriate place after choosing to validate the email.

[Mike McCracken (mikemc)]

Thanks for the review, and my apologies for taking so long to get back to this.

[Mike McCracken (mikemc)]

One more concern here - moving the logic for getting the return_to URL out of the orequest into redirection_url_for_token will require importing get_orequest (and probably get_rpconfig_from_request for checking against phishing) from identityprovider.views.utils into identityprovider.utils, which will be an import loop.

Being unfamiliar with the code, I'm not sure which things to move where to resolve that loop - both moving get_* to .utils and moving redirection_url_for_token to .views.utils will require changing some other files too. Your guidance on what to do there would be useful. Thanks!

[Natalia Bidart (nataliabidart)]

Ricardo is on parenting leave so he will not be around for a few weeks, but I understand you got his request for the redirection_url_for_token correctly.

Thanks!

[Mike McCracken (mikemc)]

I just had another chance to look at this today.

The approach in this branch was incorrect, as it only resulted in a simple redirect to the return_to URL from the openid request. This redirected the browser but did not complete the openid auth flow.

What was necessary instead was using the openid request object to create the correct response using orequest.answer(), and then to wrap that in a django response object, as done in identityprovider.views.server._django_response().

I've verified that this approach works in the new-user flow, and am working on cleaning up the branch and adding tests now.

[Maximiliano Bertacchini (maxiberta)]

Hi, I'm sorry I could not review this before. I've just run this branch and it worked as expected :-D. That said:

* There's a couple of hidden code conflicts (see comments inline). Please merge trunk, fix and run all tests again.
* I think the new "OpenID + mail verification" flow may be a little bit confusing for users as it redirects from the email verification view directly back to the RP. It'd be great if we could show the /+decide view as usual at the end of the OpenID dance instead of merging it into the email verification view.
* I think the email verification view should make that redirection more explicit. Maybe just show a warning (eg. see how it is handled in the OpenID username required case: https://bazaar.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/trunk/view/head:/src/webui/views/account.py#L197).

Haven't checked the tests yet.

review: Needs Fixing

[Maximiliano Bertacchini (maxiberta)]

This needs some work, but the original developer is not an SSO maintainer and has likely moved on since then. It'd be quicker if someone related to SSO picked it up.

[Joe Talbott (joetalbott)]

I've pushed an MP with this branch merged with trunk. I'm not sure if I addressed the mail verification view to /+decide view quite the way @maxiberta mentioned.

https://code.launchpad.net/~joetalbott/canonical-identity-provider/bugs_1169632/+merge/312308

[Maximiliano Bertacchini (maxiberta)]

I'm closing this merge proposal as it's been superseded by https://code.launchpad.net/~joetalbott/canonical-identity-provider/bugs_1169632/+merge/312308.

review: Needs Resubmitting

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2016-06-08 00:55:13 +0000
+++ src/api/v20/handlers.py	2016-07-14 21:39:02 +0000
@@ -304,6 +304,7 @@
             account = registration.register(
                 root_url, email, password, displayname,
                 creation_source=data.get('creation_source'),
+                oid_token=data.get('oid_token')
             )
         except ValidationError as e:
             return errors.INVALID_DATA(**e.message_dict)

=== modified file 'src/api/v20/registration.py'
--- src/api/v20/registration.py	2015-07-01 19:42:58 +0000
+++ src/api/v20/registration.py	2016-07-14 21:39:02 +0000
@@ -28,7 +28,7 @@
 
 def register(
         root_url, email, password, displayname, email_validated=False,
-        send_email=True, creation_source=None):
+        send_email=True, creation_source=None, oid_token=None):
     """Register a new account"""
     emails = EmailAddress.objects.filter(email__iexact=email)
     if emails.count() > 0:
@@ -67,6 +67,7 @@
     account_created.send('api', openid_identifier=account.openid_identifier)
 
     if send_email:
-        send_new_user_email(root_url=root_url, account=account, email=email)
+        send_new_user_email(root_url=root_url, account=account, email=email,
+                            oid_token=oid_token)
 
     return account

=== modified file 'src/api/v20/tests/test_registration.py'
--- src/api/v20/tests/test_registration.py	2015-11-24 14:38:04 +0000
+++ src/api/v20/tests/test_registration.py	2016-07-14 21:39:02 +0000
@@ -50,7 +50,8 @@
         assert Account.objects.all().count() == 0
 
         registration.register(
-            self.sso_root_url, self.email, 'SecretPassword1', 'displayname')
+            self.sso_root_url, self.email, 'SecretPassword1', 'displayname',
+            oid_token="fake_oid_token")
 
         self.assertEqual(Account.objects.all().count(), 1)
         account = Account.objects.get()
@@ -62,7 +63,8 @@
             self.client.login(username=self.email, password='SecretPassword1'),
             'Password was not properly set on new account.')
         self.mock_send_new_user_email.assert_called_once_with(
-            root_url=self.sso_root_url, account=account, email=self.email)
+            root_url=self.sso_root_url, oid_token="fake_oid_token",
+            account=account, email=self.email)
         self.assertFalse(self.mock_send_impersonation_email.called)
 
     def test_invalid_email_and_pw(self):

=== modified file 'src/identityprovider/emailutils.py'
--- src/identityprovider/emailutils.py	2016-05-09 21:41:37 +0000
+++ src/identityprovider/emailutils.py	2016-07-14 21:39:02 +0000
@@ -101,6 +101,7 @@
 
 def _context_for_email_request(root_url, account, email, token_type,
                                redirection_url, requester_email=None,
+                               oid_token=None,
                                **kwargs):
     """
     Build and return a context (dictionary) for rendering an e-mail template.
@@ -116,7 +117,10 @@
         redirection_url=redirection_url, **kwargs)
 
     name = getattr(account, 'displayname', kwargs.get('displayname'))
-    token_url = urljoin(root_url, token.get_absolute_url())
+    token_path = token.get_absolute_url()
+    if oid_token:
+        token_path = oid_token + token_path
+    token_url = urljoin(root_url, token_path)
     context = {
         'requester': name,
         'requester_email': token.requester_email,
@@ -152,7 +156,7 @@
 
 
 def send_new_user_email(root_url, account, email, redirection_url=None,
-                        platform='all'):
+                        platform='all', oid_token=None):
     """
     Send an email to a new user.
 
@@ -164,7 +168,8 @@
     """
 
     context, token, invalidate_email_token = _context_for_email_request(
-        root_url, account, email, AuthTokenType.VALIDATEEMAIL, redirection_url)
+        root_url, account, email, AuthTokenType.VALIDATEEMAIL, redirection_url,
+        oid_token=oid_token)
 
     if platform != 'desktop':
         template = 'email/welcome.txt'

=== modified file 'src/identityprovider/tests/test_emailutils.py'
--- src/identityprovider/tests/test_emailutils.py	2016-05-09 02:47:48 +0000
+++ src/identityprovider/tests/test_emailutils.py	2016-07-14 21:39:02 +0000
@@ -5,6 +5,7 @@
 
 import re
 import unittest
+from urlparse import urlparse, urlunparse
 
 from django.core import mail
 from django.core.urlresolvers import reverse
@@ -255,6 +256,13 @@
             self.assertEqual(db_token, token)
         requester = getattr(self.account, 'displayname', token.displayname)
         token_url = self.request.build_absolute_uri(token.get_absolute_url())
+
+        if kwargs.get('oid_token'):
+            p = urlparse(token_url)
+            token_url = urlunparse((p.scheme, p.netloc,
+                                    kwargs['oid_token'] + p.path,
+                                    p.params, p.query, p.fragment))
+        
         context = {
             'requester': requester,
             'requester_email': token.requester_email,
@@ -376,6 +384,19 @@
             token_type=AuthTokenType.VALIDATEEMAIL, token=token,
             invalidate_token=invalidate_token)
 
+    def test_send_new_user_email_with_oid_token(self):
+        oid_token = "iama_oid_token"
+        token, invalidate_token = send_new_user_email(
+            self.sso_root_url, self.account, self.email, self.redirection_url,
+            oid_token=oid_token)
+
+        self.assert_tokens()
+        self.assert_email_properly_formatted(
+            'Finish your registration', 'email/welcome.txt',
+            token_type=AuthTokenType.VALIDATEEMAIL, token=token,
+            invalidate_token=invalidate_token,
+            oid_token=oid_token)
+
     def test_send_password_reset_email_with_redirection_url(self):
         token, invalidate_token = send_password_reset_email(
             self.sso_root_url, self.account, self.email, self.redirection_url)

=== modified file 'src/identityprovider/views/server.py'
--- src/identityprovider/views/server.py	2016-07-12 14:21:03 +0000
+++ src/identityprovider/views/server.py	2016-07-14 21:39:02 +0000
@@ -293,6 +293,29 @@
             (rpconfig is not None and rpconfig.auto_authorize)):
         return _process_decide(request, orequest, decision=True)
 
+    user_attribs_form, teams_form, macaroon_form = get_extension_forms(
+        request, orequest, rpconfig
+    )
+
+    context = {
+        'account': request.user,
+        'trust_root': orequest.trust_root,
+        'rpconfig': rpconfig,
+        'user_attribs_form': user_attribs_form,
+        'teams_form': teams_form,
+        'macaroon_form': macaroon_form,
+        'token': token,
+        'sane_trust_root': _request_has_sane_trust_root(orequest)
+    }
+    # Log existing authentication request (i.e. checked via pre-verified
+    # cookie).
+    login_succeeded.send(
+        sender=request.user, user=request.user, request=request,
+        authlogtype=AuthLogType.OPENID_EXISTING)
+    return render(request, 'server/decide.html', context)
+
+
+def get_extension_forms(request, orequest, rpconfig):
     sreg_request = SRegRequest.fromOpenIDRequest(orequest)
     ax_request = ax.FetchRequest.fromOpenIDRequest(orequest)
     teams_request = TeamsRequest.fromOpenIDRequest(orequest)
@@ -324,22 +347,8 @@
     macaroon_form = MacaroonRequestForm(
         request, macaroon_request, rpconfig, orequest.trust_root,
         approved_data=approved_data.get('macaroon'))
-    context = {
-        'account': request.user,
-        'trust_root': orequest.trust_root,
-        'rpconfig': rpconfig,
-        'user_attribs_form': user_attribs_form,
-        'teams_form': teams_form,
-        'macaroon_form': macaroon_form,
-        'token': token,
-        'sane_trust_root': _request_has_sane_trust_root(orequest)
-    }
-    # Log existing authentication request (i.e. checked via pre-verified
-    # cookie).
-    login_succeeded.send(
-        sender=request.user, user=request.user, request=request,
-        authlogtype=AuthLogType.OPENID_EXISTING)
-    return render(request, 'server/decide.html', context)
+
+    return user_attribs_form, teams_form, macaroon_form
 
 
 def _request_has_sane_trust_root(openid_request):

=== modified file 'src/identityprovider/views/utils.py'
--- src/identityprovider/views/utils.py	2016-06-24 23:04:37 +0000
+++ src/identityprovider/views/utils.py	2016-07-14 21:39:02 +0000
@@ -48,6 +48,14 @@
         return rpconfig[0]
 
 
+def get_orequest(request, token):
+    """Returns the OpenID request for the given request from the
+    user's browser.  May throw an exception if there is no OpenID
+    request, or if it is invalid."""
+    raw_orequest = request.session.get(token, None)
+    return signed.loads(raw_orequest, settings.SECRET_KEY)
+
+
 def get_rpconfig_from_request(request, token):
     rpconfig = None
     if token:  # won't have an rpconfig without a token

=== modified file 'src/webui/templates/account/confirm_new_email.html'
--- src/webui/templates/account/confirm_new_email.html	2016-03-10 17:30:19 +0000
+++ src/webui/templates/account/confirm_new_email.html	2016-07-14 21:39:02 +0000
@@ -20,6 +20,40 @@
 <div class="actions">
   <form action="" method="post">
     {% csrf_token %}
+    <div data-qa-id="info-items" class="info-items">
+        {% if user_attribs_form.has_data or teams_form.has_data or macaroon_form.has_data %}
+        <ul class="list">
+            {% if user_attribs_form.has_data %}
+                {% for field in user_attribs_form %}
+                <li class="user_attribs"><div class=radio-label-row>{{ field|safe }} {{ field.label_tag }}</div></li>
+                {% endfor %}
+            {% endif %}
+            <div class=radio-label-row>
+            {% if teams_form.has_data %}
+                {% ifequal teams_form.fields|length 1 %}
+                    {% for field in teams_form %}
+                    <li><div class=radio-label-row>{{ field|safe }} {{ field.label_tag }}</div></li>
+                    {% endfor %}
+                {% else %}
+            <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>
+                    {% endfor %}
+                </ul>
+            </li>
+            {% endifequal %}
+            {% endif %}
+            </div>
+            {% if macaroon_form.has_data %}
+                {% for field in macaroon_form %}
+                <li class="macaroon"><div class="radio-label-row">{{ field|safe }} {{ field.label_tag }}</div></li>
+                {% endfor %}
+            {% endif %}
+        </ul>
+        {% endif %}
+    </div>
 
     {% if captcha_required %}
     <p class="captcha" id="captcha">

=== modified file 'src/webui/tests/test_views_registration.py'
--- src/webui/tests/test_views_registration.py	2016-05-10 15:13:36 +0000
+++ src/webui/tests/test_views_registration.py	2016-07-14 21:39:02 +0000
@@ -192,6 +192,15 @@
             response['Location'],
             reverse('server-decide', kwargs=dict(token=token))
         )
+        expected_args = dict(email=self.TESTDATA['email'],
+                             password=self.TESTDATA['password'],
+                             displayname=self.TESTDATA['displayname'],
+                             create_captcha=False,
+                             captcha_solution=None,
+                             captcha_id=None,
+                             creation_source='web-flow',
+                             oid_token=token)
+        self.mock_api_register.assert_called_once_with(**expected_args)
 
     def test_post_already_registered(self):
         exc = api_errors.AlreadyRegistered(Mock())

=== modified file 'src/webui/tests/test_views_ui.py'
--- src/webui/tests/test_views_ui.py	2016-06-02 04:15:06 +0000
+++ src/webui/tests/test_views_ui.py	2016-07-14 21:39:02 +0000
@@ -15,7 +15,7 @@
 from functools import partial
 from StringIO import StringIO
 from textwrap import dedent
-from urlparse import urlsplit
+from urlparse import urlsplit, urlparse, urlunparse
 from urllib import quote
 
 from django.conf import settings

=== modified file 'src/webui/views/registration.py'
--- src/webui/views/registration.py	2016-04-29 14:17:15 +0000
+++ src/webui/views/registration.py	2016-07-14 21:39:02 +0000
@@ -14,6 +14,7 @@
 from django.http import (
     HttpResponseNotAllowed,
     HttpResponseRedirect,
+    HttpResponseServerError
 )
 from django.utils.decorators import method_decorator
 from django.utils.translation import ugettext_lazy as _
@@ -115,6 +116,8 @@
             # we'll handle our own capture generation
             data['create_captcha'] = False
             data['creation_source'] = WEB_CREATION_SOURCE
+            if token:
+                data['oid_token'] = token
 
             url = redirection_url_for_token(token)
 
@@ -141,7 +144,8 @@
                 captcha_error = '&error=' + e.extra.get('captcha_message', '')
                 captcha_error_message = _('Incorrect captcha solution')
                 collect_stats('error.captcha')
-
+            except Exception as e:
+                return HttpResponseServerError("exception: " + str(e))
             else:
                 collect_stats('success')
 

=== modified file 'src/webui/views/ui.py'
--- src/webui/views/ui.py	2016-06-14 21:03:43 +0000
+++ src/webui/views/ui.py	2016-07-14 21:39:02 +0000
@@ -32,6 +32,8 @@
 from django.views.decorators.cache import cache_control
 from django.views.decorators.gzip import gzip_page
 from gargoyle import gargoyle
+from openid.extensions import ax
+from openid.extensions.sreg import SRegRequest
 
 from identityprovider import emailutils
 from identityprovider.forms import (
@@ -40,6 +42,7 @@
     ResetPasswordForm,
     TwoFactorForm,
     TwoFactorLoginForm,
+    UserAttribsRequestForm,
 )
 from identityprovider.login import (
     AuthenticationError,
@@ -70,8 +73,16 @@
 from identityprovider.validators import PASSWORD_LEAKED
 from identityprovider.views.utils import (
     get_rpconfig_from_request,
+    get_orequest,
     is_safe_redirect_url,
 )
+from identityprovider.views.server import (
+    _django_response,
+    _add_user_attribs,
+    _check_team_membership,
+    _add_macaroon,
+    get_extension_forms
+)
 from webui.decorators import (
     check_readonly,
     dont_cache,
@@ -543,6 +554,9 @@
         # the TransactionMiddleware will not rollback the dirty transaction
         return HttpResponseNotFound()
 
+    if token:
+        orequest = get_orequest(request, token)
+    
     if request.method == 'POST':
         captcha_solution = request.POST.get('g-recaptcha-response')
         verified = False
@@ -575,6 +589,23 @@
                 email=email, token_type=AuthTokenType.INVALIDATEEMAIL)
             for t in tokens:
                 t.consume()
+
+            if token:
+                if orequest.idSelect():
+                    openid_identity_url = request.build_absolute_uri(
+                        request.user.get_absolute_url())
+                    oresponse = orequest.answer(True,
+                                                identity=openid_identity_url)
+                else:
+                    oresponse = orequest.answer(True)
+                _add_user_attribs(request, orequest, oresponse)
+                _check_team_membership(
+                    request, orequest, oresponse, immediate=True)
+                _add_macaroon(request, orequest, oresponse)
+                    
+                return _django_response(
+                    request, oresponse, auth_success=True, orequest=orequest)
+
             return HttpResponseRedirect(
                 atrequest.redirection_url or reverse('account-index')
             )
@@ -588,6 +619,18 @@
                'captcha_required': captcha_switch_active,
                'captcha_error_message': captcha_error_message,
                'CAPTCHA_PUBLIC_KEY': settings.CAPTCHA_PUBLIC_KEY}
+    
+    if token:
+        rpconfig = get_rpconfig_from_request(request, token)
+        user_attribs_form, teams_form, macaroon_form = get_extension_forms(
+            request, orequest, rpconfig
+        )
+        context.update({
+            'user_attribs_form': user_attribs_form,
+            'teams_form': teams_form,
+            'macaroon_form': macaroon_form
+        })
+
     return render(request, 'account/confirm_new_email.html', context)