Merge lp:~canonical-isd-hackers/canonical-identity-provider/redirect-url into lp:canonical-identity-provider/release

[Łukasz Czyżykowski]

Added return_to GET argument to /+logout, which enables people getting back to the original page from which they were logged out.

Added return_to GET argument to +logout view.

I created the view as class to easier handle logic in that view.

[Danny Tamez (zematynnad)]

Looks great - just some nit picks...
typo on line 12 of the diff withing
is the test_test_ thing on lines 181 and 185 on purpose?
please put line 259 after 261 and remove the unused import on line 11 of identityprovider/views/ui.py

review: Needs Fixing

[Łukasz Czyżykowski (lukasz-czyzykowski)]

Approving after David applied requested changes.

review: Approve

[Łukasz Czyżykowski (lukasz-czyzykowski)]

Attempt to merge lp:~canonical-isd-hackers/canonical-identity-provider/redirect-url into lp:canonical-identity-provider failed due to merge conflicts:

text conflict in identityprovider/views/ui.py

[Łukasz Czyżykowski (lukasz-czyzykowski)]

Approving after resolving merge conflicts.

review: Approve

=== modified file 'identityprovider/models/account.py'
--- identityprovider/models/account.py	2010-09-20 20:13:55 +0000
+++ identityprovider/models/account.py	2010-09-22 08:06:21 +0000
@@ -117,6 +117,19 @@
         sites = self.openidrpsummary_set.order_by('date_last_used')
         return sites.reverse()[:limit]
 
+    def sites_with_active_sessions(self, age_hours=24):
+        """
+        Return list of sites on which user *may* have active sessions
+
+        Current rule of thumb is to return all sites accessed within previous
+        24 hours.
+        """
+        max_date_last_used = (datetime.datetime.now() -
+                              datetime.timedelta(hours=age_hours))
+        sites = self.openidrpsummary_set.filter(
+            date_last_used__gte=max_date_last_used)
+        return sites.order_by('-date_last_used')
+
     def _get_preferredemail(self):
         try:
             return self.emailaddress_set.filter(

=== added file 'identityprovider/templates/registration/logout.html'
--- identityprovider/templates/registration/logout.html	1970-01-01 00:00:00 +0000
+++ identityprovider/templates/registration/logout.html	2010-09-22 08:06:21 +0000
@@ -0,0 +1,45 @@
+{% extends "base.html" %}
+{% load i18n %}
+
+{% comment %}
+Copyright 2010 Canonical Ltd.  This software is licensed under the
+GNU Affero General Public License version 3 (see the file  LICENSE).
+{% endcomment %}
+
+{% block title %}{% trans "You have been logged out" %}{% endblock %}
+
+{% block text_title %}
+  <h1 class="main">{% trans "You have been logged out" %}</h1>
+{% endblock %}
+
+{% block content %}
+    <p>{% trans "You have been logged out." %}
+      {% if return_to_url %}
+      {% blocktrans with return_to_url as url and return_to_site_name as site_name %}
+     <span class="big">Return to <a href="{{ return_to_url }}">{{ site_name }}</a></span>{% endblocktrans %}
+     {% endif %}
+    </p>
+
+    {% if other_sites %}
+     <div>
+       <h2 class="main">{% trans "Sites you may still be logged to" %}</h2>
+       <table class="listing">
+        <thead>
+          <tr>
+            <th class="description">{% trans "Site" %}</th>
+            <th class="date">{% trans "Last authenticated" %}</th>
+        </tr>
+        </thead>
+        <tbody>
+          {% for site in other_sites %}
+            <tr>
+              <td><a href="{{ site.trust_root }}">{{ site.trust_root }}</a></td>
+              <td>{{ site.date_last_used|date:"Y-m-d" }}</td>
+            </tr>
+          {% endfor %}
+        </tbody>
+      </table>
+    </div>
+    {% endif %}
+{% endblock %}
+

=== modified file 'identityprovider/tests/test_models_account.py'
--- identityprovider/tests/test_models_account.py	2010-07-13 18:05:31 +0000
+++ identityprovider/tests/test_models_account.py	2010-09-22 08:06:21 +0000
@@ -1,7 +1,7 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-from datetime import datetime
+from datetime import datetime, timedelta
 from django.conf import settings
 from django.contrib.auth.models import User
 from identityprovider.tests.utils import BasicAccountTestCase
@@ -292,6 +292,28 @@
         self.assertEqual(password.password,
             encrypt_launchpad_password('password', salt=salt))
 
+    def test_sites_with_active_sessions_when_returns_empty_result(self):
+        account = Account.objects.create_account(
+            'sites-account', '[e-mail]', 'password')
+        sites_count = account.sites_with_active_sessions().count()
+        self.assertEqual(sites_count, 0)
+
+    def test_sites_with_active_sessions_when_one_site_is_active(self):
+        account = Account.objects.create_account(
+            'sites-account', '[e-mail]', 'password')
+
+        account.openidrpsummary_set.create(
+            date_last_used=datetime.now(),
+            trust_root='http://trust-root.example.com',
+            openid_identifier='openid-identifier')
+
+        account.openidrpsummary_set.create(
+            date_last_used=datetime.now() - timedelta(days=3),
+            trust_root='http://trust-root-2.example.com')
+
+        sites_count = account.sites_with_active_sessions().count()
+        self.assertEqual(sites_count, 1)
+
 
 class AccountPasswordTestCase(BasicAccountTestCase):
     def test_unicode(self):

=== modified file 'identityprovider/tests/test_views_ui.py'
--- identityprovider/tests/test_views_ui.py	2010-09-21 22:04:10 +0000
+++ identityprovider/tests/test_views_ui.py	2010-09-22 08:06:21 +0000
@@ -20,7 +20,7 @@
 from identityprovider.models import EmailAddress, Person, OpenIDRPConfig
 from identityprovider.models.const import EmailStatus
 from identityprovider.views import ui, server
-from identityprovider.models import Account, AuthToken, EmailAddress
+from identityprovider.models import Account, AuthToken
 from identityprovider.models.authtoken import create_token
 from identityprovider.models.captcha import Captcha
 from identityprovider.models.const import (AccountStatus, EmailStatus,
@@ -166,7 +166,7 @@
     def test_logout(self):
         self.authenticate()
         r = self.client.get('/+logout')
-        self.assertRedirects(r, '/+login')
+        self.assertTemplateUsed(r, 'registration/logout.html')
 
     def test_logout_preserve_token(self):
         self.authenticate()
@@ -182,10 +182,6 @@
         self.assertEquals(self.client.session.get(token),
                           'raw_orequest content')
 
-    def test_logout_no_preferredlanguage_attribute(self):
-        r = self.client.get('/+logout')
-        self.assertRedirects(r, '/+login')
-
     def create_token(self, token_type, email=None, redirection_url=None):
         token = at.AuthToken.objects.create(
             token_type=token_type,

=== added file 'identityprovider/tests/test_views_ui_logout.py'
--- identityprovider/tests/test_views_ui_logout.py	1970-01-01 00:00:00 +0000
+++ identityprovider/tests/test_views_ui_logout.py	2010-09-22 08:06:21 +0000
@@ -0,0 +1,97 @@
+from django.test import TestCase
+from django.conf import settings
+from django.http import HttpResponse
+from django.contrib.sessions.backends.cache import SessionStore
+
+from identityprovider.views.ui import LogoutView
+from identityprovider.models.openidmodels import OpenIDRPConfig
+from identityprovider.models.const import AccountCreationRationale
+
+
+class LogoutViewTestCase(TestCase):
+
+    def setUp(self):
+        self.view = LogoutView()
+        self.cookies = {}
+
+    def test_get_language_succeeds_when_user_has_attribute(self):
+        self.preferredlanguage = "en"
+        self.assertEquals(self.view.get_language(self), "en")
+        del self.preferredlanguage
+
+    def test_get_language_fails_when_user_has_no_attribute(self):
+        self.assertEquals(self.view.get_language(object()), None)
+
+    def set_cookie(self, cookie_name, cookie_value):
+        self.cookies[cookie_name] = cookie_value
+
+    def test_set_language_with_language_equal_to_none(self):
+        self.view.set_language(self, None)
+        self.assertEquals(self.cookies, {})
+
+    def test_set_language_with_language_equal_to_en(self):
+        self.view.set_language(self, "en")
+        self.assertEquals(self.cookies[settings.LANGUAGE_COOKIE_NAME], "en")
+
+    def test_set_orequest_when_token_and_orequset_is_passed(self):
+        session = {}
+        self.view.set_orequest(session, 'token', 'orequest')
+        self.assertEquals(session['token'], 'orequest')
+
+    def test_set_orequest_when_token_is_none(self):
+        session = {}
+        self.view.set_orequest(session, None, 'orequest')
+        self.assertFalse('orequest' in session.values())
+
+    def test_set_orequest_when_orequest_is_none(self):
+        session = {}
+        self.view.set_orequest(session, 'toekn', None)
+        self.assertFalse('token' in session)
+
+    def create_openid_rp_config(self, trust_root):
+        OpenIDRPConfig.objects.create(
+            trust_root=trust_root,
+            displayname="Example",
+            creation_rationale=AccountCreationRationale.UNKNOWN)
+
+    def test_get_return_to_url_whithout_referer_and_url_is_recognized(self):
+        trust_root = 'http://example.com/r'
+        self.create_openid_rp_config(trust_root)
+        return_to = self.view.get_return_to_url(trust_root, None)
+        self.assertEquals(return_to, trust_root)
+
+    def test_get_return_to_url_when_referer_mismatches_known_url(self):
+        trust_root = 'http://example.com/r'
+        self.create_openid_rp_config(trust_root)
+        return_to = self.view.get_return_to_url(trust_root,
+                                                'http://r.example.com')
+        self.assertTrue(return_to is None)
+
+    def test_get_return_to_url_when_url_is_unknown(self):
+        return_to = self.view.get_return_to_url('http://unknown.example.com',
+                                                None)
+        self.assertTrue(return_to is None)
+
+    def test_get_return_to_url_when_return_to_is_none(self):
+        return_to = self.view.get_return_to_url(None, None)
+
+        self.assertTrue(return_to is None)
+
+
+    def sites_with_active_sessions(self):
+        return []
+
+    def test_call(self):
+        """Sanity check test, make sure that all code in __call__ is runable."""
+        self.user = self
+        self.session = SessionStore()
+        self.GET = {}
+        self.META = {}
+        response = self.view(self, None)
+
+        self.assertTrue(isinstance(response, HttpResponse))
+
+        del self.user
+        del self.session
+        del self.GET
+        del self.META

=== modified file 'identityprovider/views/ui.py'
--- identityprovider/views/ui.py	2010-09-21 22:13:44 +0000
+++ identityprovider/views/ui.py	2010-09-22 08:06:21 +0000
@@ -22,21 +22,21 @@
 from django.utils.translation import ugettext as _
 
 from identityprovider.branding import current_brand
-from identityprovider.decorators import (dont_cache, guest_required, limitlogin,
-    requires_cookies)
+from identityprovider.decorators import (check_readonly, dont_cache,
+    guest_required, limitlogin, requires_cookies)
 from identityprovider.forms import (LoginForm, ForgotPasswordForm,
     ResetPasswordForm, OldNewAccountForm, NewAccountForm, ConfirmNewAccountForm,
     TokenForm)
 from identityprovider.models import (Account, AccountPassword, AuthToken,
     AuthTokenFactory, EmailAddress, get_type_of_token, verify_token_string)
+from identityprovider.models.captcha import Captcha
 from identityprovider.models.const import (AccountStatus, EmailStatus,
     LoginTokenType)
+from identityprovider.models.openidmodels import OpenIDRPConfig
 import identityprovider.signed as signed
 from identityprovider.utils import (encrypt_launchpad_password,
     format_address, get_person_and_account_by_email, polite_form_errors,
     CannotResetPasswordException, PersonAndAccountNotFoundException)
-from identityprovider.decorators import check_readonly
-from identityprovider.models.captcha import Captcha
 from identityprovider.views.utils import get_rpconfig, set_session_token_info
 
 
@@ -98,23 +98,79 @@
     return True
 
 
-def logout(request, token=None):
-    request.token = token
-    try:
-        lang = request.user.preferredlanguage
-    except AttributeError:
-        lang = None
-
-    # We don't want to lose session[token] when we log the user out
-    raw_orequest = request.session.get(token, None)
-    auth.logout(request)
-    if token is not None and raw_orequest is not None:
-        request.session[token] = raw_orequest
-    request.session['message'] = _("You have successfully logged out")
-    response = HttpResponseRedirect('+login')
-    if lang:
-        response.set_cookie(settings.LANGUAGE_COOKIE_NAME, lang)
-    return response
+class LogoutView(object):
+
+    def get_language(self, user):
+        try:
+            return user.preferredlanguage
+        except AttributeError:
+            return None
+
+    def set_language(self, response, language):
+        if language:
+            response.set_cookie(settings.LANGUAGE_COOKIE_NAME, language)
+
+    def get_return_to_url(self, return_to, http_referer):
+        if not return_to:
+            return None
+        known = OpenIDRPConfig.objects.filter(
+            trust_root=return_to).count() == 1
+        if known:
+            if http_referer is not None and http_referer != return_to:
+                return None
+            return return_to
+        else:
+            return None
+
+    def set_orequest(self, session, token, raw_orequest):
+        if token is not None and raw_orequest is not None:
+            session[token] = raw_orequest
+
+    def get_other_sites(self, user, trust_root):
+        if not hasattr(user, 'sites_with_active_sessions'):
+            return []
+        return [site for site in user.sites_with_active_sessions()
+                if trust_root != site.trust_root]
+
+    def get_site_name(self, trust_root):
+        if not trust_root:
+            return None
+        sites = list(OpenIDRPConfig.objects.filter(trust_root=trust_root))
+        if sites:
+            return sites[0].displayname
+        else:
+            return trust_root
+
+    def compute_context(self, user, return_to, referrer):
+        return_to = self.get_return_to_url(return_to, referrer)
+
+        context = {
+            'return_to_url': return_to,
+            'return_to_site_name': self.get_site_name(return_to),
+            'other_sites': self.get_other_sites(user, return_to),
+        }
+        return context
+
+    def __call__(self, request, token):
+        lang = self.get_language(request.user)
+
+        context = self.compute_context(
+            request.user,
+            request.GET.get('return_to', None),
+            request.META.get('HTTP_REFERER', None))
+
+        # We don't want to lose session[token] when we log the user out
+        raw_orequest = request.session.get(token, None)
+        auth.logout(request)
+        self.set_orequest(request.session, token, raw_orequest)
+
+        response = render_to_response('registration/logout.html', context)
+
+        self.set_language(response, lang)
+        return response
+
+
+logout = LogoutView()
 
 
 def enter_token(request, token=None):