accounts tests fixes (#10485)

.travis.yml

@@ -16,20 +16,18 @@ jobs:
     - { stage: python2, env: TOXENV=addons }
     - { stage: python2, env: TOXENV=devhub }
     - { stage: python2, env: TOXENV=reviewers-and-zadmin }
-    - { stage: python2, env: TOXENV=users-and-ratings }
+    - { stage: python2, env: TOXENV=accounts-users-and-ratings }
     - { stage: python2, env: TOXENV=amo-locales-and-signing }
-    - { stage: python2, env: TOXENV=accounts }
     - { stage: python2, env: TOXENV=main }
     - { stage: python3working, python: 3.6, env: TOXENV=codestyle}
     - { stage: python3working, python: 3.6, env: TOXENV=docs }
     - { stage: python3working, python: 3.6, env: TOXENV=assets }
     - { stage: python3working, python: 3.6, env: TOXENV=es }
     - { stage: python3working, python: 3.6, env: TOXENV=addons }
-    - { stage: python3working, python: 3.6, env: TOXENV=users-and-ratings }
+    - { stage: python3working, python: 3.6, env: TOXENV=accounts-users-and-ratings }
     - { stage: python3, python: 3.6, env: TOXENV=devhub }
     - { stage: python3, python: 3.6, env: TOXENV=reviewers-and-zadmin }
     - { stage: python3, python: 3.6, env: TOXENV=amo-locales-and-signing }
-    - { stage: python3, python: 3.6, env: TOXENV=accounts }
     - { stage: python3, python: 3.6, env: TOXENV=main }
 
 env:

src/olympia/accounts/serializers.py

@@ -174,7 +174,7 @@ class AccountSuperCreateSerializer(serializers.Serializer):
     username = serializers.CharField(required=False)
     email = serializers.EmailField(required=False)
     fxa_id = serializers.CharField(required=False)
-    group = serializers.ChoiceField(choices=group_rules.items(),
+    group = serializers.ChoiceField(choices=list(group_rules.items()),
                                     required=False)
 
     def validate_email(self, email):

src/olympia/accounts/tests/test_utils.py

@@ -10,6 +10,7 @@ from six.moves.urllib_parse import parse_qs, urlparse
 from django.contrib.auth.models import AnonymousUser
 from django.test import RequestFactory
 from django.test.utils import override_settings
+from django.utils.encoding import force_text
 
 from olympia.accounts import utils
 from olympia.accounts.utils import process_fxa_event
@@ -59,7 +60,7 @@ def test_fxa_config_logged_in():
 
 @override_settings(FXA_CONFIG=FXA_CONFIG)
 def test_default_fxa_login_url_with_state():
-    path = '/en-US/addons/abp/?source=ddg'
+    path = b'/en-US/addons/abp/?source=ddg'
     request = RequestFactory().get(path)
     request.session = {'fxa_state': 'myfxastate'}
     raw_url = utils.default_fxa_login_url(request)
@@ -68,19 +69,20 @@ def test_default_fxa_login_url_with_state():
         scheme=url.scheme, netloc=url.netloc, path=url.path)
     assert base == 'https://accounts.firefox.com/oauth/authorization'
     query = parse_qs(url.query)
-    next_path = urlsafe_b64encode(path).rstrip('=')
+    next_path = urlsafe_b64encode(path).rstrip(b'=')
     assert query == {
         'action': ['signin'],
         'client_id': ['foo'],
         'redirect_url': ['https://testserver/fxa'],
         'scope': ['profile'],
-        'state': ['myfxastate:{next_path}'.format(next_path=next_path)],
+        'state': ['myfxastate:{next_path}'.format(
+            next_path=force_text(next_path))],
     }
 
 
 @override_settings(FXA_CONFIG=FXA_CONFIG)
 def test_default_fxa_register_url_with_state():
-    path = '/en-US/addons/abp/?source=ddg'
+    path = b'/en-US/addons/abp/?source=ddg'
     request = RequestFactory().get(path)
     request.session = {'fxa_state': 'myfxastate'}
     raw_url = utils.default_fxa_register_url(request)
@@ -89,13 +91,14 @@ def test_default_fxa_register_url_with_state():
         scheme=url.scheme, netloc=url.netloc, path=url.path)
     assert base == 'https://accounts.firefox.com/oauth/authorization'
     query = parse_qs(url.query)
-    next_path = urlsafe_b64encode(path).rstrip('=')
+    next_path = urlsafe_b64encode(path).rstrip(b'=')
     assert query == {
         'action': ['signup'],
         'client_id': ['foo'],
         'redirect_url': ['https://testserver/fxa'],
         'scope': ['profile'],
-        'state': ['myfxastate:{next_path}'.format(next_path=next_path)],
+        'state': ['myfxastate:{next_path}'.format(
+            next_path=force_text(next_path))],
     }
 
 
@@ -115,19 +118,20 @@ def test_fxa_login_url_without_requiring_two_factor_auth():
         scheme=url.scheme, netloc=url.netloc, path=url.path)
     assert base == 'https://accounts.firefox.com/oauth/authorization'
     query = parse_qs(url.query)
-    next_path = urlsafe_b64encode(path).rstrip('=')
+    next_path = urlsafe_b64encode(path.encode('utf-8')).rstrip(b'=')
     assert query == {
         'action': ['signin'],
         'client_id': ['foo'],
         'redirect_url': ['https://testserver/fxa'],
         'scope': ['profile'],
-        'state': ['myfxastate:{next_path}'.format(next_path=next_path)],
+        'state': ['myfxastate:{next_path}'.format(
+            next_path=force_text(next_path))],
     }
 
 
 @override_settings(FXA_CONFIG=FXA_CONFIG)
 def test_fxa_login_url_requiring_two_factor_auth():
-    path = '/en-US/addons/abp/?source=ddg'
+    path = u'/en-US/addons/abp/?source=ddg'
     request = RequestFactory().get(path)
     request.session = {'fxa_state': 'myfxastate'}
 
@@ -137,18 +141,19 @@ def test_fxa_login_url_requiring_two_factor_auth():
         force_two_factor=True)
 
     url = urlparse(raw_url)
-    base = '{scheme}://{netloc}{path}'.format(
+    base = u'{scheme}://{netloc}{path}'.format(
         scheme=url.scheme, netloc=url.netloc, path=url.path)
     assert base == 'https://accounts.firefox.com/oauth/authorization'
     query = parse_qs(url.query)
-    next_path = urlsafe_b64encode(path).rstrip('=')
+    next_path = urlsafe_b64encode(path.encode('utf-8')).rstrip(b'=')
     assert query == {
         'acr_values': ['AAL2'],
         'action': ['signin'],
         'client_id': ['foo'],
         'redirect_url': ['https://testserver/fxa'],
         'scope': ['profile'],
-        'state': ['myfxastate:{next_path}'.format(next_path=next_path)],
+        'state': ['myfxastate:{next_path}'.format(
+            next_path=force_text(next_path))],
     }
 
 

src/olympia/accounts/tests/test_views.py

@@ -13,6 +13,7 @@ from django.contrib.messages import get_messages
 from django.urls import reverse
 from django.test import RequestFactory
 from django.test.utils import override_settings
+from django.utils.encoding import force_text
 
 from rest_framework.exceptions import PermissionDenied
 from rest_framework.settings import api_settings
@@ -111,9 +112,9 @@ class TestLoginStartBaseView(WithDynamicEndpoints, TestCase):
         assert self.client.session['fxa_state'] == 'thisisthestate'
 
     def test_to_is_included_in_redirect_state(self):
-        path = '/addons/unlisted-addon/'
+        path = b'/addons/unlisted-addon/'
         # The =s will be stripped from the URL.
-        assert '=' in base64.urlsafe_b64encode(path)
+        assert b'=' in base64.urlsafe_b64encode(path)
         state = 'somenewstatestring'
         self.initialize_session({})
         with mock.patch('olympia.accounts.views.generate_fxa_state',
@@ -397,7 +398,7 @@ class TestWithUser(TestCase):
         self.request.data = {
             'code': 'foo',
             'state': u'some-blob:{next_path}'.format(
-                next_path=base64.urlsafe_b64encode('/a/path/?')),
+                next_path=force_text(base64.urlsafe_b64encode(b'/a/path/?'))),
         }
         args, kwargs = self.fn(self.request)
         assert args == (self, self.request)
@@ -463,7 +464,8 @@ class TestWithUser(TestCase):
         self.request.data = {
             'code': 'foo',
             'state': u'some-blob:{next_path}'.format(
-                next_path=base64.urlsafe_b64encode('https://www.google.com')),
+                next_path=force_text(
+                    base64.urlsafe_b64encode(b'https://www.google.com'))),
         }
         args, kwargs = self.fn(self.request)
         assert args == (self, self.request)
@@ -507,7 +509,8 @@ class TestWithUser(TestCase):
     def test_logged_in_disallows_login(self, generate_api_token_mock):
         self.request.data = {
             'code': 'foo',
-            'state': 'some-blob:{}'.format(base64.urlsafe_b64encode('/next')),
+            'state': 'some-blob:{}'.format(
+                force_text(base64.urlsafe_b64encode(b'/next'))),
         }
         self.user = UserProfile()
         self.request.user = self.user
@@ -525,7 +528,8 @@ class TestWithUser(TestCase):
     def test_already_logged_in_add_api_token_cookie_if_missing(self):
         self.request.data = {
             'code': 'foo',
-            'state': 'some-blob:{}'.format(base64.urlsafe_b64encode('/next')),
+            'state': 'some-blob:{}'.format(
+                force_text(base64.urlsafe_b64encode(b'/next'))),
         }
         self.user = UserProfile()
         self.request.user = self.user
@@ -552,7 +556,8 @@ class TestWithUser(TestCase):
         self.find_user.return_value = self.user
         self.request.data = {
             'code': 'foo',
-            'state': 'other-blob:{}'.format(base64.urlsafe_b64encode('/next')),
+            'state': 'other-blob:{}'.format(
+                force_text(base64.urlsafe_b64encode(b'/next'))),
         }
         self.fn(self.request)
         self.render_error.assert_called_with(
@@ -591,7 +596,7 @@ class TestWithUser(TestCase):
         self.request.data = {
             'code': 'foo',
             'state': u'some-blob:{next_path}'.format(
-                next_path=base64.urlsafe_b64encode('/a/path/?')),
+                next_path=force_text(base64.urlsafe_b64encode(b'/a/path/?'))),
         }
         # @with_user should return a redirect response directly in that case.
         response = self.fn(self.request)
@@ -607,14 +612,15 @@ class TestWithUser(TestCase):
             host=fxa_config['oauth_host'],
             path='/authorization')
         query = parse_qs(url.query)
-        next_path = base64.urlsafe_b64encode('/a/path/?').rstrip('=')
+        next_path = base64.urlsafe_b64encode(b'/a/path/?').rstrip(b'=')
         assert query == {
             'acr_values': ['AAL2'],
             'action': ['signin'],
             'client_id': [fxa_config['client_id']],
             'redirect_url': [fxa_config['redirect_url']],
             'scope': [fxa_config['scope']],
-            'state': ['some-blob:{next_path}'.format(next_path=next_path)],
+            'state': ['some-blob:{next_path}'.format(
+                next_path=force_text(next_path))],
         }
 
     def test_theme_developer_should_not_redirect_for_two_factor_auth(self):
@@ -628,7 +634,7 @@ class TestWithUser(TestCase):
         self.request.data = {
             'code': 'foo',
             'state': u'some-blob:{next_path}'.format(
-                next_path=base64.urlsafe_b64encode('/a/path/?')),
+                next_path=force_text(base64.urlsafe_b64encode(b'/a/path/?'))),
         }
         args, kwargs = self.fn(self.request)
         assert args == (self, self.request)
@@ -652,7 +658,7 @@ class TestWithUser(TestCase):
         self.request.data = {
             'code': 'foo',
             'state': u'some-blob:{next_path}'.format(
-                next_path=base64.urlsafe_b64encode('/a/path/?')),
+                next_path=force_text(base64.urlsafe_b64encode(b'/a/path/?'))),
         }
         args, kwargs = self.fn(self.request)
         assert args == (self, self.request)
@@ -672,7 +678,7 @@ class TestWithUser(TestCase):
         self.request.data = {
             'code': 'foo',
             'state': u'some-blob:{next_path}'.format(
-                next_path=base64.urlsafe_b64encode('/a/path/?')),
+                next_path=force_text(base64.urlsafe_b64encode(b'/a/path/?'))),
         }
         args, kwargs = self.fn(self.request)
         assert args == (self, self.request)
@@ -808,7 +814,8 @@ class TestAuthenticateView(BaseAuthenticationView):
         response = self.client.get(self.url, {
             'code': 'codes!!',
             'state': ':'.join(
-                [self.fxa_state, base64.urlsafe_b64encode('/go/here')]),
+                [self.fxa_state,
+                 force_text(base64.urlsafe_b64encode(b'/go/here'))]),
         })
         # This 302s because the user isn't logged in due to mocking.
         self.assertRedirects(
@@ -830,7 +837,8 @@ class TestAuthenticateView(BaseAuthenticationView):
         response = self.client.get(self.url, {
             'code': 'codes!!',
             'state': ':'.join(
-                [self.fxa_state, base64.urlsafe_b64encode('/go/here')]),
+                [self.fxa_state,
+                 force_text(base64.urlsafe_b64encode(b'/go/here'))]),
             'config': 'skip',
         })
         self.fxa_identify.assert_called_with(
@@ -874,7 +882,8 @@ class TestAuthenticateView(BaseAuthenticationView):
             'code': 'code',
             'state': ':'.join([
                 self.fxa_state,
-                base64.urlsafe_b64encode('/en-US/firefox/a/path')]),
+                force_text(
+                    base64.urlsafe_b64encode(b'/en-US/firefox/a/path'))]),
         })
         self.assertRedirects(
             response, '/en-US/firefox/a/path', target_status_code=404)
@@ -890,7 +899,8 @@ class TestAuthenticateView(BaseAuthenticationView):
             'code': 'code',
             'state': ':'.join([
                 self.fxa_state,
-                base64.urlsafe_b64encode('/en-US/firefox/a/path')]),
+                force_text(
+                    base64.urlsafe_b64encode(b'/en-US/firefox/a/path'))]),
         })
         self.assertRedirects(
             response, '/en-US/firefox/a/path', target_status_code=404)
@@ -1053,7 +1063,7 @@ class TestAccountViewSetUpdate(TestCase):
         response = self.patch()
         assert response.status_code == 200
         assert response.content != original
-        modified_json = json.loads(response.content)
+        modified_json = json.loads(force_text(response.content))
         self.user = self.user.reload()
         for prop, value in six.iteritems(self.update_data):
             assert modified_json[prop] == value
@@ -1078,7 +1088,7 @@ class TestAccountViewSetUpdate(TestCase):
         response = self.patch(url=url)
         assert response.status_code == 200
         assert response.content != original
-        modified_json = json.loads(response.content)
+        modified_json = json.loads(force_text(response.content))
         random_user = random_user.reload()
         for prop, value in six.iteritems(self.update_data):
             assert modified_json[prop] == value
@@ -1095,9 +1105,10 @@ class TestAccountViewSetUpdate(TestCase):
         assert response.content == original
         self.user = self.user.reload()
         # Confirm field hasn't been updated.
-        assert json.loads(response.content)['last_login_ip'] == ''
+        response = json.loads(force_text(response.content))
+        assert response['last_login_ip'] == ''
         assert self.user.last_login_ip == ''
-        assert json.loads(response.content)['username'] == existing_username
+        assert response['username'] == existing_username
         assert self.user.username == existing_username
 
     def test_biography_no_links(self):
@@ -1105,7 +1116,7 @@ class TestAccountViewSetUpdate(TestCase):
         response = self.patch(
             data={'biography': '<a href="https://google.com">google</a>'})
         assert response.status_code == 400
-        assert json.loads(response.content) == {
+        assert json.loads(force_text(response.content)) == {
             'biography': ['No links are allowed.']}
 
     def test_display_name_validation(self):
@@ -1113,20 +1124,20 @@ class TestAccountViewSetUpdate(TestCase):
         response = self.patch(
             data={'display_name': 'a'})
         assert response.status_code == 400
-        assert json.loads(response.content) == {
+        assert json.loads(force_text(response.content)) == {
             'display_name': ['Ensure this field has at least 2 characters.']}
 
         response = self.patch(
             data={'display_name': 'a' * 51})
         assert response.status_code == 400
-        assert json.loads(response.content) == {
+        assert json.loads(force_text(response.content)) == {
             'display_name': [
                 'Ensure this field has no more than 50 characters.']}
 
         response = self.patch(
             data={'display_name': u'\x7F\u20DF'})
         assert response.status_code == 400
-        assert json.loads(response.content) == {
+        assert json.loads(force_text(response.content)) == {
             'display_name': [
                 'Must contain at least one printable character.']}
 
@@ -1148,7 +1159,7 @@ class TestAccountViewSetUpdate(TestCase):
         response = self.client.patch(
             self.url, data, format='multipart')
         assert response.status_code == 200
-        json_content = json.loads(response.content)
+        json_content = json.loads(force_text(response.content))
         self.user = self.user.reload()
         assert 'anon_user.png' not in json_content['picture_url']
         assert '%s.png' % self.user.id in json_content['picture_url']
@@ -1167,7 +1178,7 @@ class TestAccountViewSetUpdate(TestCase):
         assert response.status_code == 200
         # Should delete the photo
         assert not path.exists(self.user.picture_path)
-        json_content = json.loads(response.content)
+        json_content = json.loads(force_text(response.content))
         assert json_content['picture_url'] is None
 
     def test_account_picture_disallowed_verbs(self):
@@ -1190,7 +1201,7 @@ class TestAccountViewSetUpdate(TestCase):
         response = self.client.patch(
             self.url, data, format='multipart')
         assert response.status_code == 400
-        assert json.loads(response.content) == {
+        assert json.loads(force_text(response.content)) == {
             'picture_upload': [u'Images must be either PNG or JPG.']}
 
     def test_picture_upload_animated(self):
@@ -1200,7 +1211,7 @@ class TestAccountViewSetUpdate(TestCase):
         response = self.client.patch(
             self.url, data, format='multipart')
         assert response.status_code == 400
-        assert json.loads(response.content) == {
+        assert json.loads(force_text(response.content)) == {
             'picture_upload': [u'Images cannot be animated.']}
 
     def test_picture_upload_not_image(self):
@@ -1210,7 +1221,7 @@ class TestAccountViewSetUpdate(TestCase):
         response = self.client.patch(
             self.url, data, format='multipart')
         assert response.status_code == 400
-        assert json.loads(response.content) == {
+        assert json.loads(force_text(response.content)) == {
             'picture_upload': [
                 u'Upload a valid image. The file you uploaded was either not '
                 u'an image or a corrupted image.'
@@ -1279,7 +1290,7 @@ class TestAccountViewSetDelete(TestCase):
 
         response = self.client.delete(self.url)
         assert response.status_code == 400
-        assert 'You must delete all add-ons and themes' in response.content
+        assert b'You must delete all add-ons and themes' in response.content
         assert not self.user.reload().deleted
         assert views.API_TOKEN_COOKIE not in response.cookies
         assert self.client.cookies[views.API_TOKEN_COOKIE].value == 'something'
@@ -1299,7 +1310,7 @@ class TestAccountViewSetDelete(TestCase):
 
         response = self.client.delete(self.url)
         assert response.status_code == 400
-        assert 'You must delete all add-ons and themes' in response.content
+        assert b'You must delete all add-ons and themes' in response.content
         assert not self.user.reload().deleted
 
         addon.delete()
@@ -1710,7 +1721,7 @@ class TestAccountNotificationViewSetUpdate(TestCase):
                                     data={'individual_contact': False})
         assert response.status_code == 400
         # Attempt fails.
-        assert 'Attempting to set [individual_contact] to False.' in (
+        assert b'Attempting to set [individual_contact] to False.' in (
             response.content)
         # And the notification hasn't been saved.
         assert not UserNotification.objects.filter(

src/olympia/accounts/views.py

@@ -1,4 +1,5 @@
 import base64
+import binascii
 import functools
 import os
 
@@ -9,7 +10,7 @@ from django.core import signing
 from django.db.models import Q
 from django.http import Http404, HttpResponseRedirect
 from django.urls import reverse
-from django.utils.encoding import force_bytes
+from django.utils.encoding import force_bytes, force_text
 from django.utils.html import format_html
 from django.utils.http import is_safe_url
 from django.utils.translation import ugettext, ugettext_lazy as _
@@ -517,7 +518,7 @@ class AccountSuperCreate(APIView):
         data = serializer.data
 
         group = serializer.validated_data.get('group', None)
-        user_token = os.urandom(4).encode('hex')
+        user_token = force_text(binascii.b2a_hex(os.urandom(4)))
         username = data.get('username', 'super-created-{}'.format(user_token))
         fxa_id = data.get('fxa_id', None)
         email = data.get('email', '{}@addons.mozilla.org'.format(username))

src/olympia/api/models.py

@@ -39,7 +39,7 @@ class APIKey(ModelBase):
     key = models.CharField(max_length=255, db_index=True, unique=True)
     # TODO: use RSA public keys instead? If we were to use JWT RSA keys
     # then we'd only need to store the public key.
-    secret = AESField(aes_key='api_key:secret', aes_prefix=b'aes:')
+    secret = AESField(aes_key='api_key:secret')
 
     class Meta:
         db_table = 'api_key'

src/olympia/api/tests/utils.py

@@ -1,6 +1,7 @@
 from datetime import datetime
 
 from rest_framework.test import APIClient
+from six import text_type
 
 from olympia.amo.tests import TestCase
 from olympia.api.authentication import JWTKeyAuthentication
@@ -17,7 +18,8 @@ class APIKeyAuthTestMixin(TestCase, JWTAuthKeyTester):
             email='a@m.o',
             read_dev_agreement=datetime.today(),
         )
-        self.api_key = self.create_api_key(self.user, str(self.user.pk) + ':f')
+        self.api_key = self.create_api_key(
+            self.user, text_type(self.user.pk) + ':f')
 
     def authorization(self):
         """

tox.ini

@@ -1,5 +1,5 @@
 [tox]
-envlist = es, addons, amo-locales-and-signin, devhub, reviewers-and-zadmin, users-and-ratings, accounts, main, ui-tests, flake8, docs, assets
+envlist = es, addons, amo-locales-and-signin, devhub, reviewers-and-zadmin, accounts-users-and-ratings, main, ui-tests, flake8, docs, assets
 
 [travis]
 unignore_outcomes = True
@@ -49,15 +49,10 @@ commands =
     bash {toxinidir}/locale/compile-mo.sh {toxinidir}/locale/
     pytest -n 2 -m 'needs_locales_compilation' -v src/olympia/ {posargs}
 
-[testenv:users-and-ratings]
+[testenv:accounts-users-and-ratings]
 commands =
     make -f Makefile-docker install_python_test_dependencies
-    pytest -n 2 -m 'not es_tests and not needs_locales_compilation and not static_assets' -v src/olympia/users/ src/olympia/ratings/ {posargs}
-
-[testenv:accounts]
-commands =
-    make -f Makefile-docker install_python_test_dependencies
-    pytest -n 2 -m 'not es_tests and not needs_locales_compilation and not static_assets' -v src/olympia/accounts/ {posargs}
+    pytest -n 2 -m 'not es_tests and not needs_locales_compilation and not static_assets' -v src/olympia/accounts/ src/olympia/users/ src/olympia/ratings/ {posargs}
 
 [testenv:main]
 commands =