Remove ProfileController dependency on horus

Integration with horus is causing us endless pain, both in testing and
in extending the behaviour of account endpoints. This commit removes the
ProfileController's dependency on horus.

h/accounts/test/views_test.py

@@ -56,6 +56,7 @@ def _get_fake_request(username, password):
     def get_fake_token():
         return 'fake_token'
 
+    fake_request.method = 'POST'
     fake_request.params['csrf_token'] = 'fake_token'
     fake_request.session.get_csrf_token = get_fake_token
     fake_request.POST['username'] = username
@@ -120,114 +121,73 @@ class TestEditProfile(object):
 
     """Unit tests for ProfileController's edit_profile() method."""
 
-    @pytest.mark.usefixtures('activation_model', 'dummy_db_session')
-    def test_edit_profile_invalid_password(self, config, form_validator, user_model):
+    def test_edit_profile_invalid_password(self, authn_policy, form_validator, user_model):
         """Make sure our edit_profile call validates the user password."""
-        configure(config)
+        authn_policy.authenticated_userid.return_value = "johndoe"
         form_validator.return_value = (None, {
             "username": "john",
             "pwd": "blah",
             "subscriptions": "",
         })
 
-        # With an invalid password, get_user returns None
-        user_model.get_user.return_value = None
+        # Mock an invalid password
+        user_model.validate_user.return_value = False
 
-        request = _get_fake_request('john', 'doe')
+        request = DummyRequest(method='POST')
         profile = ProfileController(request)
         result = profile.edit_profile()
 
         assert result['code'] == 401
         assert any('pwd' in err for err in result['errors'])
 
-    @pytest.mark.usefixtures('activation_model', 'dummy_db_session')
-    def test_edit_profile_with_validation_failure(self, config, form_validator, user_model):
+    def test_edit_profile_with_validation_failure(self, authn_policy, form_validator):
         """If form validation fails, return the error object."""
-        configure(config)
+        authn_policy.authenticated_userid.return_value = "johndoe"
         form_validator.return_value = ({"errors": "BOOM!"}, None)
 
-        profile = ProfileController(DummyRequest())
+        request = DummyRequest(method='POST')
+        profile = ProfileController(request)
         result = profile.edit_profile()
 
         assert result == {"errors": "BOOM!"}
 
-    @pytest.mark.usefixtures('activation_model', 'dummy_db_session')
-    def test_edit_profile_successfully(self, config, form_validator, user_model):
+    def test_edit_profile_successfully(self, authn_policy, form_validator, user_model):
         """edit_profile() returns a dict with key "form" when successful."""
-        configure(config)
+        authn_policy.authenticated_userid.return_value = "johndoe"
         form_validator.return_value = (None, {
             "username": "johndoe",
             "pwd": "password",
             "subscriptions": "",
         })
+        user_model.validate_user.return_value = True
+        user_model.get_by_id.return_value = FakeUser(email="john@doe.com")
 
-        profile = ProfileController(DummyRequest())
-
+        request = DummyRequest(method='POST')
+        profile = ProfileController(request)
         result = profile.edit_profile()
 
-        assert "form" in result
-        assert "errors" not in result
+        assert result == {"model": {"email": "john@doe.com"}}
 
-    @pytest.mark.usefixtures('activation_model', 'dummy_db_session')
-    def test_edit_profile_returns_email(self, config, form_validator, user_model,
-                                        authn_policy):
-        """edit_profile()'s response should contain the user's current email.
-
-        For a valid edit_profile() request
-        horus.views.ProfileController.edit_profile() returns an HTTPRedirection
-        object. h.accounts.views.ProfileController.edit_profile() should
-        add a JSON body to this response containing a "model" dict with the
-        user's current email address.
-
-        AsyncFormViewMapper will pick up this JSON body and preserve it in the
-        body of the 200 OK response that is finally sent back to the browser.
-
-        The frontend uses this email field to show the user's current email
-        address in the form.
-
-        """
-        configure(config)
-        edit_profile_patcher = patch(
-            "horus.views.ProfileController.edit_profile")
-
-        form_validator.return_value = (None, {
-            "username": "fake user name",
-            "pwd": "fake password",
-            "subscriptions": "",
-        })
-
-        result = None
-        try:
-            edit_profile = edit_profile_patcher.start()
-            edit_profile.return_value = httpexceptions.HTTPFound("fake url")
-
-            user_model.get_by_id.return_value = FakeUser(email="fake email")
-
-            result = ProfileController(DummyRequest()).edit_profile()
-
-            assert result.json["model"]["email"] == "fake email"
-
-        finally:
-            edit_profile = edit_profile_patcher.stop()
-
-    @pytest.mark.usefixtures('activation_model', 'user_model')
-    def test_subscription_update(self, config, dummy_db_session, form_validator):
+    def test_subscription_update(self, authn_policy, form_validator,
+                                 subscriptions_model, user_model):
         """Make sure that the new status is written into the DB."""
-        configure(config)
-        request = _get_fake_request('acct:john@doe', 'smith')
-
+        authn_policy.authenticated_userid.return_value = "acct:john@doe"
         form_validator.return_value = (None, {
             "username": "acct:john@doe",
             "pwd": "smith",
-            "subscriptions": '{"active":true,"uri":"acct:john@doe","id": 1}',
+            "subscriptions": '{"active":true,"uri":"acct:john@doe","id":1}',
         })
+        mock_sub = Mock(active=False, uri="acct:john@doe")
+        subscriptions_model.get_by_id.return_value = mock_sub
+        user_model.get_by_id.return_value = FakeUser(email="john@doe")
+
+        request = DummyRequest(method='POST')
+        profile = ProfileController(request)
+        result = profile.edit_profile()
+
+        assert mock_sub.active == True
+        assert result == {"model": {"email": "john@doe"}}
 
-        with patch('h.accounts.views.Subscriptions') as mock_subs:
-            mock_subs.get_by_id = MagicMock()
-            mock_subs.get_by_id.return_value = Mock(active=True)
-            profile = ProfileController(request)
-            profile.edit_profile()
-            assert dummy_db_session.added
 
 
 class TestAsyncFormViewMapper(object):
@@ -320,6 +280,13 @@ def test_registration_does_not_autologin(config, authn_policy):
     assert not authn_policy.remember.called
 
 
+@pytest.fixture
+def subscriptions_model(request):
+    patcher = patch('h.accounts.views.Subscriptions', autospec=True)
+    request.addfinalizer(patcher.stop)
+    return patcher.start()
+
+
 @pytest.fixture
 def user_model(config, request):
     patcher = patch('h.accounts.views.User', autospec=True)

h/accounts/views.py

@@ -245,54 +245,81 @@ class AsyncRegisterController(RegisterController):
 @view_config(attr='edit_profile', route_name='edit_profile')
 @view_config(attr='disable_user', route_name='disable_user')
 @view_config(attr='profile', route_name='profile')
-class ProfileController(horus.views.ProfileController):
+class ProfileController(object):
+    def __init__(self, request):
+        self.request = request
+        self.schema = schemas.ProfileSchema().bind(request=self.request)
+        self.form = deform.Form(self.schema)
+
     def edit_profile(self):
+        if self.request.method != 'POST':
+            return httpexceptions.HTTPMethodNotAllowed()
+
+        # Nothing to do here for non logged-in users
+        if self.request.authenticated_userid is None:
+            return httpexceptions.HTTPUnauthorized()
+
         err, appstruct = validate_form(self.form, self.request.POST.items())
         if err is not None:
             return err
 
-        username = appstruct['username']
-        pwd = appstruct['pwd']
-        subscriptions = appstruct['subscriptions']
+        user = User.get_by_id(self.request, self.request.authenticated_userid)
+        response = {'model': {'email': user.email}}
 
+        # We allow updating subscriptions without validating a password
+        subscriptions = appstruct.get('subscriptions')
         if subscriptions:
-            # Update the subscriptions table
-            subs = json.loads(subscriptions)
-            if username == subs['uri']:
-                s = Subscriptions.get_by_id(self.request, subs['id'])
-                if s:
-                    s.active = subs['active']
-                    self.db.add(s)
-                    return {}
-                else:
-                    return dict(
-                        errors=[
-                            {'subscriptions': _('Non existing subscription')}
-                        ],
-                        code=404
-                    )
-            else:
-                return dict(
-                    errors=[{'username': _('Invalid username')}], code=400
-                )
+            data = json.loads(subscriptions)
+            s = Subscriptions.get_by_id(self.request, data['id'])
+            if s is None:
+                return {
+                    'errors': [{'subscriptions': _('Subscription not found')}],
+                    'code': 400
+                }
+
+            # If we're trying to update a subscription for anyone other than
+            # the currently logged-in user, bail fast.
+            #
+            # The error message is deliberately identical to the one above, so
+            # as not to leak any information about who which subscription ids
+            # belong to.
+            if s.uri != self.request.authenticated_userid:
+                return {
+                    'errors': [{'subscriptions': _('Subscription not found')}],
+                    'code': 400
+                }
+
+            s.active = data.get('active', True)
+
+            FlashMessage(self.request, _('Changes saved!'), kind='success')
+            return response
 
-        # Password check
-        user = User.get_user(self.request, username, pwd)
-        if user:
-            self.request.context = user
-            response = super(ProfileController, self).edit_profile()
+        # Any updates to fields below this point require password validation.
+        #
+        #   `pwd` is the current password
+        #   `password` (used below) is optional, and is the new password
+        #
+        if not User.validate_user(user, appstruct.get('pwd')):
+            return {'errors': [{'pwd': _('Invalid password')}], 'code': 401}
 
-            # Add the user's email into the model dict that eventually gets
-            # returned to the browser. This is needed so that the edit profile
-            # forms can show the value of the user's current email.
-            if self.request.authenticated_userid:
-                user = User.get_by_id(
-                    self.request, self.request.authenticated_userid)
-                response.json = {"model": {"email": user.email}}
+        email = appstruct.get('email')
+        if email:
+            email_user = User.get_by_email(self.request, email)
 
-            return response
-        else:
-            return dict(errors=[{'pwd': _('Invalid password')}], code=401)
+            if email_user:
+                if email_user.id != user.id:
+                    return {
+                        'errors': [{'pwd': _('That email is already used')}],
+                    }
+
+            response['model']['email'] = user.email = email
+
+        password = appstruct.get('password')
+        if password:
+            user.password = password
+
+        FlashMessage(self.request, _('Changes saved!'), kind='success')
+        return response
 
     def disable_user(self):
         err, appstruct = validate_form(self.form, self.request.POST.items())
@@ -307,7 +334,6 @@ def disable_user(self):
         if user:
             # TODO: maybe have an explicit disabled flag in the status
             user.password = User.generate_random_password()
-            self.db.add(user)
             FlashMessage(self.request, _('Account disabled.'), kind='success')
             return {}
         else:
@@ -332,7 +358,6 @@ def unsubscribe(self):
         subscription = Subscriptions.get_by_id(request, subscription_id)
         if subscription:
             subscription.active = False
-            self.db.add(subscription)
             return {}
         return {}