Redirect flow for FxA (fixes #984)

apps/accounts/tests/test_views.py

@@ -1,11 +1,12 @@
 from django.core.urlresolvers import resolve, reverse
+from django.test import RequestFactory, TestCase
 from django.test.utils import override_settings
 
 import mock
 
 from rest_framework.test import APITestCase
 
-from accounts import verify
+from accounts import verify, views
 from amo.tests import create_switch
 from api.tests.utils import APIAuthTestCase
 from users.models import UserProfile
@@ -40,20 +41,95 @@ class TestFxALoginWaffle(APITestCase):
         assert response.status_code == 422
 
 
-@override_settings(FXA_CONFIG=FXA_CONFIG)
-class TestLoginView(APITestCase):
+class TestLoginUser(TestCase):
 
     def setUp(self):
-        self.url = reverse('accounts.login')
-        create_switch('fxa-auth', active=True)
-        patcher = mock.patch('accounts.views.verify.fxa_identify')
-        self.fxa_identify = patcher.start()
+        self.request = RequestFactory().get('/login')
+        self.user = UserProfile.objects.create(email='real@yeahoo.com')
+        self.identity = {'email': 'real@yeahoo.com', 'uid': '9001'}
+        patcher = mock.patch('accounts.views.login')
+        self.login = patcher.start()
         self.addCleanup(patcher.stop)
 
+    def test_user_gets_logged_in(self):
+        views.login_user(self.request, self.user, self.identity)
+        self.login.assert_called_with(self.request, self.user)
+
+    def test_identify_success_sets_fxa_data(self):
+        assert self.user.fxa_id is None
+        views.login_user(self.request, self.user, self.identity)
+        user = self.user.reload()
+        assert user.fxa_id == '9001'
+        assert not user.has_usable_password()
+
+    def test_identify_success_account_exists_migrated_different_email(self):
+        self.user.update(email='different@yeahoo.com')
+        views.login_user(self.request, self.user, self.identity)
+        user = self.user.reload()
+        assert user.fxa_id == '9001'
+        assert user.email == 'real@yeahoo.com'
+
+
+class TestRegisterUser(TestCase):
+
+    def setUp(self):
+        self.request = RequestFactory().get('/register')
+        self.identity = {'email': 'me@yeahoo.com', 'uid': '9005'}
+        patcher = mock.patch('accounts.views.login')
+        self.login = patcher.start()
+        self.addCleanup(patcher.stop)
+
+    def test_user_is_created(self):
+        user_qs = UserProfile.objects.filter(email='me@yeahoo.com')
+        assert not user_qs.exists()
+        views.register_user(self.request, self.identity)
+        assert user_qs.exists()
+        user = user_qs.get()
+        assert user.username == 'me@yeahoo.com'
+        assert user.fxa_id == '9005'
+        assert not user.has_usable_password()
+        self.login.assert_called_with(self.request, user)
+
+    def test_username_taken_creates_user(self):
+        UserProfile.objects.create(
+            email='you@yeahoo.com', username='me@yeahoo.com')
+        user_qs = UserProfile.objects.filter(email='me@yeahoo.com')
+        assert not user_qs.exists()
+        views.register_user(self.request, self.identity)
+        assert user_qs.exists()
+        user = user_qs.get()
+        assert user.username.startswith('me@yeahoo.com')
+        assert user.username != 'me@yeahoo.com'
+        assert user.fxa_id == '9005'
+        assert not user.has_usable_password()
+
+
+@override_settings(FXA_CONFIG=FXA_CONFIG)
+class BaseAuthenticationView(APITestCase):
+
+    def setUp(self):
+        self.url = reverse(self.view_name)
+        create_switch('fxa-auth', active=True)
+        self.fxa_identify = self.patch('accounts.views.verify.fxa_identify')
+
+    def patch(self, thing):
+        patcher = mock.patch(thing)
+        self.addCleanup(patcher.stop)
+        return patcher.start()
+
+
+class TestLoginView(BaseAuthenticationView):
+    view_name = 'accounts.login'
+
+    def setUp(self):
+        super(TestLoginView, self).setUp()
+        self.login_user = self.patch('accounts.views.login_user')
+
     def test_no_code_provided(self):
         response = self.client.post(self.url)
         assert response.status_code == 422
         assert response.data['error'] == 'No code provided.'
+        assert not self.login_user.called
 
     def test_identify_no_profile(self):
         self.fxa_identify.side_effect = verify.IdentificationError
@@ -61,6 +137,7 @@ class TestLoginView(APITestCase):
         assert response.status_code == 401
         assert response.data['error'] == 'Profile not found.'
         self.fxa_identify.assert_called_with('codes!!', config=FXA_CONFIG)
+        assert not self.login_user.called
 
     def test_identify_success_no_account(self):
         self.fxa_identify.return_value = {'email': 'me@yeahoo.com', 'uid': '5'}
@@ -68,48 +145,16 @@ class TestLoginView(APITestCase):
         assert response.status_code == 422
         assert response.data['error'] == 'User does not exist.'
         self.fxa_identify.assert_called_with('codes!!', config=FXA_CONFIG)
+        assert not self.login_user.called
 
-    def test_identify_success_logs_user_in(self):
+    def test_identify_success_account_exists(self):
         user = UserProfile.objects.create(email='real@yeahoo.com')
-        assert '_auth_user_id' not in self.client.session
-        self.fxa_identify.return_value = {'email': 'real@yeahoo.com',
-                                          'uid': '9001'}
-        response = self.client.post(self.url, {'code': 'code'})
-        assert response.status_code == 200
-        assert '_auth_user_id' in self.client.session
-        assert self.client.session['_auth_user_id'] == user.pk
-
-    def test_identify_success_sets_fxa_data(self):
-        user = UserProfile.objects.create(email='real@yeahoo.com')
-        assert user.fxa_id is None
-        self.fxa_identify.return_value = {'email': 'real@yeahoo.com',
-                                          'uid': '9001'}
-        response = self.client.post(self.url, {'code': 'code'})
-        assert response.status_code == 200
-        user = user.reload()
-        assert user.fxa_id == '9001'
-        assert not user.has_usable_password()
-
-    def test_identify_success_account_exists_migrated_different_email(self):
-        user = UserProfile.objects.create(email='different@yeahoo.com',
-                                          fxa_id='9005')
-        self.fxa_identify.return_value = {'email': 'real@yeahoo.com',
-                                          'uid': '9005'}
-        response = self.client.post(self.url, {'code': 'code'})
-        assert response.status_code == 200
-        self.fxa_identify.assert_called_with('code', config=FXA_CONFIG)
-        user = user.reload()
-        assert user.fxa_id == '9005'
-        assert user.email == 'real@yeahoo.com'
-
-    def test_identify_success_account_exists_not_migrated(self):
-        UserProfile.objects.create(email='real@yeahoo.com')
-        self.fxa_identify.return_value = {'email': 'real@yeahoo.com',
-                                          'uid': '9001'}
+        identity = {'email': 'real@yeahoo.com', 'uid': '9001'}
+        self.fxa_identify.return_value = identity
         response = self.client.post(self.url, {'code': 'code'})
         assert response.status_code == 200
         assert response.data['email'] == 'real@yeahoo.com'
-        self.fxa_identify.assert_called_with('code', config=FXA_CONFIG)
+        self.login_user.assert_called_with(mock.ANY, user, identity)
 
     def test_identify_success_account_exists_migrated_multiple(self):
         UserProfile.objects.create(email='real@yeahoo.com', username='foo')
@@ -119,22 +164,21 @@ class TestLoginView(APITestCase):
                                           'uid': '9005'}
         with self.assertRaises(UserProfile.MultipleObjectsReturned):
             self.client.post(self.url, {'code': 'code'})
+        assert not self.login_user.called
 
 
-@override_settings(FXA_CONFIG=FXA_CONFIG)
-class TestRegisterView(APITestCase):
+class TestRegisterView(BaseAuthenticationView):
+    view_name = 'accounts.register'
 
     def setUp(self):
-        self.url = reverse('accounts.register')
-        create_switch('fxa-auth', active=True)
-        patcher = mock.patch('accounts.views.verify.fxa_identify')
-        self.fxa_identify = patcher.start()
-        self.addCleanup(patcher.stop)
+        super(TestRegisterView, self).setUp()
+        self.register_user = self.patch('accounts.views.register_user')
 
     def test_no_code_provided(self):
         response = self.client.post(self.url)
         assert response.status_code == 422
         assert response.data['error'] == 'No code provided.'
+        assert not self.register_user.called
 
     def test_identify_no_profile(self):
         self.fxa_identify.side_effect = verify.IdentificationError
@@ -142,69 +186,62 @@ class TestRegisterView(APITestCase):
         assert response.status_code == 401
         assert response.data['error'] == 'Profile not found.'
         self.fxa_identify.assert_called_with('codes!!', config=FXA_CONFIG)
+        assert not self.register_user.called
+
+    def test_identify_success_no_account(self):
+        identity = {u'email': u'me@yeahoo.com', u'uid': u'e0b6f'}
+        self.register_user.return_value = UserProfile(email=identity['email'])
+        self.fxa_identify.return_value = identity
+        response = self.client.post(self.url, {'code': 'codes!!'})
+        assert response.status_code == 200
+        assert response.data['email'] == 'me@yeahoo.com'
+        self.fxa_identify.assert_called_with('codes!!', config=FXA_CONFIG)
+        self.register_user.assert_called_with(mock.ANY, identity)
+
+
+class TestAuthorizeView(BaseAuthenticationView):
+    view_name = 'accounts.authorize'
+
+    def setUp(self):
+        super(TestAuthorizeView, self).setUp()
+        self.login_user = self.patch('accounts.views.login_user')
+        self.register_user = self.patch('accounts.views.register_user')
+
+    def test_no_code_provided(self):
+        response = self.client.get(self.url)
+        assert response.status_code == 422
+        assert response.data['error'] == 'No code provided.'
+        assert not self.login_user.called
+        assert not self.register_user.called
+
+    def test_identify_no_profile(self):
+        self.fxa_identify.side_effect = verify.IdentificationError
+        response = self.client.get(self.url, {'code': 'codes!!'})
+        assert response.status_code == 401
+        assert response.data['error'] == 'Profile not found.'
+        self.fxa_identify.assert_called_with('codes!!', config=FXA_CONFIG)
+        assert not self.login_user.called
+        assert not self.register_user.called
 
     def test_identify_success_no_account(self):
         user_qs = UserProfile.objects.filter(email='me@yeahoo.com')
         assert not user_qs.exists()
-        self.fxa_identify.return_value = {u'email': u'me@yeahoo.com',
-                                          u'uid': u'e0b6f'}
-
-        response = self.client.post(self.url, {'code': 'codes!!'})
-        assert response.status_code == 200
-        assert response.data['email'] == 'me@yeahoo.com'
-        assert user_qs.exists()
-        user = user_qs.get()
-        assert user.username == 'me@yeahoo.com'
-        assert user.fxa_id == 'e0b6f'
-        assert not user.has_usable_password()
+        identity = {u'email': u'me@yeahoo.com', u'uid': u'e0b6f'}
+        self.fxa_identify.return_value = identity
+        response = self.client.get(self.url, {'code': 'codes!!'})
+        assert response.status_code == 302
         self.fxa_identify.assert_called_with('codes!!', config=FXA_CONFIG)
+        assert not self.login_user.called
+        self.register_user.assert_called_with(mock.ANY, identity)
 
-    def test_identify_success_logs_user_in(self):
-        assert '_auth_user_id' not in self.client.session
-        self.fxa_identify.return_value = {u'email': u'me@yeahoo.com',
-                                          u'uid': u'e0b6f'}
-        response = self.client.post(self.url, {'code': 'codes!!'})
-        assert response.status_code == 200
-        user = UserProfile.objects.get(email='me@yeahoo.com')
-        assert '_auth_user_id' in self.client.session
-        assert self.client.session['_auth_user_id'] == user.pk
-
-    def test_identify_success_no_account_username_taken(self):
-        UserProfile.objects.create(
-            email='you@yeahoo.com', username='me@yeahoo.com')
-        user_qs = UserProfile.objects.filter(email='me@yeahoo.com')
-        assert not user_qs.exists()
-        self.fxa_identify.return_value = {u'email': u'me@yeahoo.com',
-                                          u'uid': u'e0b6f'}
-
-        response = self.client.post(self.url, {'code': 'codes!!'})
-        assert response.status_code == 200
-        assert response.data['email'] == 'me@yeahoo.com'
-        assert user_qs.exists()
-        user = user_qs.get()
-        assert user.username.startswith('me@yeahoo.com')
-        assert user.username != 'me@yeahoo.com'
-        assert user.fxa_id == 'e0b6f'
-        assert not user.has_usable_password()
-        self.fxa_identify.assert_called_with('codes!!', config=FXA_CONFIG)
-
-    def test_identify_success_account_exists_email(self):
-        UserProfile.objects.create(email='real@yeahoo.com')
-        self.fxa_identify.return_value = {'email': 'real@yeahoo.com',
-                                          'uid': '8675'}
-        response = self.client.post(self.url, {'code': 'code'})
-        assert response.status_code == 422
-        assert response.data['error'] == 'That account already exists.'
-        self.fxa_identify.assert_called_with('code', config=FXA_CONFIG)
-
-    def test_identify_success_account_exists_uid(self):
-        UserProfile.objects.create(email='real@yeahoo.com', fxa_id='10')
-        self.fxa_identify.return_value = {'email': 'diff@yeahoo.com',
-                                          'uid': '10'}
-        response = self.client.post(self.url, {'code': 'code'})
-        assert response.status_code == 422
-        assert response.data['error'] == 'That account already exists.'
-        self.fxa_identify.assert_called_with('code', config=FXA_CONFIG)
+    def test_identify_success_exists_logs_user_in(self):
+        user = UserProfile.objects.create(email='real@yeahoo.com')
+        identity = {'email': 'real@yeahoo.com', 'uid': '9001'}
+        self.fxa_identify.return_value = identity
+        response = self.client.get(self.url, {'code': 'code'})
+        assert response.status_code == 302
+        self.login_user.assert_called_with(mock.ANY, user, identity)
+        assert not self.register_user.called
 
 
 class TestProfileView(APIAuthTestCase):

apps/accounts/urls.py

@@ -3,6 +3,8 @@ from django.conf.urls import url
 from . import views
 
 urlpatterns = [
+    url(r'^authorize/$', views.AuthorizeView.as_view(),
+        name='accounts.authorize'),
     url(r'^login/$', views.LoginView.as_view(), name='accounts.login'),
     url(r'^profile/$', views.ProfileView.as_view(), name='accounts.profile'),
     url(r'^register/$', views.RegisterView.as_view(),

apps/accounts/views.py

@@ -4,6 +4,7 @@ import logging
 from django.conf import settings
 from django.contrib.auth import login
 from django.db.models import Q
+from django.http import HttpResponseRedirect
 
 from rest_framework import generics
 from rest_framework.response import Response
@@ -37,14 +38,36 @@ def find_user(identity):
         raise
 
 
+def register_user(request, identity):
+    user = UserProfile.objects.create_user(
+        email=identity['email'], username=None, fxa_id=identity['uid'])
+    log.info('Created user {} from FxA'.format(user))
+    login(request, user)
+    return user
+
+
+def login_user(request, user, identity):
+    if (user.fxa_id != identity['uid'] or
+            user.email != identity['email']):
+        log.info(
+            'Updating user info from FxA. Old {old_email} {old_uid} '
+            'New {new_email} {new_uid}'.format(
+                old_email=user.email, old_uid=user.fxa_id,
+                new_email=identity['email'], new_uid=identity['uid']))
+        user.update(fxa_id=identity['uid'], email=identity['email'])
+    log.info('Logging in user {} from FxA'.format(user))
+    login(request, user)
+
+
 def with_user(fn):
     @functools.wraps(fn)
     def inner(self, request):
-        if 'code' not in request.DATA:
+        data = request.GET if request.method == 'GET' else request.DATA
+        if 'code' not in data:
             return Response({'error': 'No code provided.'}, status=422)
 
         try:
-            identity = verify.fxa_identify(request.DATA['code'],
+            identity = verify.fxa_identify(data['code'],
                                            config=settings.FXA_CONFIG)
         except verify.IdentificationError:
             return Response({'error': 'Profile not found.'}, status=401)
@@ -60,26 +83,10 @@ class LoginView(APIView):
         if user is None:
             return Response({'error': 'User does not exist.'}, status=422)
         else:
-            if (user.fxa_id != identity['uid'] or
-                    user.email != identity['email']):
-                log.info(
-                    'Updating user info from FxA. Old {old_email} {old_uid} '
-                    'New {new_email} {new_uid}'.format(
-                        old_email=user.email, old_uid=user.fxa_id,
-                        new_email=identity['email'], new_uid=identity['uid']))
-                user.update(fxa_id=identity['uid'], email=identity['email'])
-            log.info('Logging in user {} from FxA'.format(user))
-            login(request, user)
+            login_user(request, user, identity)
             return Response({'email': identity['email']})
 
 
-class ProfileView(JWTProtectedView, generics.RetrieveAPIView):
-    serializer_class = UserProfileSerializer
-
-    def retrieve(self, request, *args, **kw):
-        return Response(self.get_serializer(request.user).data)
-
-
 class RegisterView(APIView):
 
     @waffle_switch('fxa-auth')
@@ -89,8 +96,24 @@ class RegisterView(APIView):
             return Response({'error': 'That account already exists.'},
                             status=422)
         else:
-            user = UserProfile.objects.create_user(
-                email=identity['email'], username=None, fxa_id=identity['uid'])
-            log.info('Created user {} from FxA'.format(user))
-            login(request, user)
+            user = register_user(request, identity)
             return Response({'email': user.email})
+
+
+class AuthorizeView(APIView):
+
+    @waffle_switch('fxa-auth')
+    @with_user
+    def get(self, request, user, identity):
+        if user is None:
+            register_user(request, identity)
+        else:
+            login_user(request, user, identity)
+        return HttpResponseRedirect('/')
+
+
+class ProfileView(JWTProtectedView, generics.RetrieveAPIView):
+    serializer_class = UserProfileSerializer
+
+    def retrieve(self, request, *args, **kw):
+        return Response(self.get_serializer(request.user).data)

settings.py

@@ -114,11 +114,9 @@ FXA_CONFIG = {
     'client_secret':
         '4db6f78940c6653d5b0d2adced8caf6c6fd8fd4f2a3a448da927a54daba7d401',
     'content_host': 'https://stable.dev.lcip.org',
-    'login_url': 'http://olympia.dev/api/v3/accounts/login/',
     'oauth_host': 'https://oauth-stable.dev.lcip.org/v1',
     'profile_host': 'https://stable.dev.lcip.org/profile/v1',
-    'redirect_url': 'http://olympia.dev/fxa-authorize',
-    'register_url': 'http://olympia.dev/api/v3/accounts/register/',
+    'redirect_url': 'http://olympia.dev/api/v3/accounts/authorize/',
     'scope': 'profile',
 }
 

static/js/common/fxa-login.js

@@ -17,43 +17,24 @@
 
         opts = opts || {};
         var authConfig = {
-            ui: 'lightbox',
             state: 'foo',
             redirectUri: config.redirectUrl,
             scope: config.scope,
         };
         if (opts.signUp) {
             console.log('[FxA] Starting register');
-            return fxaClient.auth.signUp(authConfig).then(function(response) {
-                console.log('[FxA] Register success', response);
-                return $.post(config.registerUrl, postConfig(response));
-            });
+            return fxaClient.auth.signUp(authConfig);
         } else {
             console.log('[FxA] Starting login');
-            return fxaClient.auth.signIn(authConfig).then(function(response) {
-                console.log('[FxA] Login success', response);
-                return $.post(config.loginUrl, postConfig(response));
-            });
+            return fxaClient.auth.signIn(authConfig);
         }
     }
 
     $('body').on('click', '.fxa-login', function(e) {
         e.preventDefault();
-
-        fxaLogin().then(function(response) {
-            console.log('[FxA] Server login response', response);
-            window.location.reload();
-        }, function(error) {
-            console.log('[FxA] Login failed', error);
-        });
+        fxaLogin();
     }).on('click', '.fxa-register', function(e) {
         e.preventDefault();
-
-        fxaLogin({signUp: true}).then(function(response) {
-            console.log('[FxA] Server register response', response);
-            window.location.reload();
-        }, function(error) {
-            console.log('[FxA] Register failed', error);
-        });
+        fxaLogin({signUp: true});
     });
 })();