Merge pull request #5441 from hypothesis/user-update-service

Add user_update service; tighten UserUpdateAPISchema

h/schemas/api/user.py

@@ -96,3 +96,19 @@ class UpdateUserAPISchema(JSONSchema):
             },
         },
     }
+
+    def validate(self, data):
+        appstruct = super(UpdateUserAPISchema, self).validate(data)
+        appstruct = self._whitelisted_properties_only(appstruct)
+        return appstruct
+
+    def _whitelisted_properties_only(self, appstruct):
+        """Return a new appstruct containing only schema-defined fields"""
+
+        new_appstruct = {}
+
+        for allowed_field in UpdateUserAPISchema.schema['properties'].keys():
+            if allowed_field in appstruct:
+                new_appstruct[allowed_field] = appstruct[allowed_field]
+
+        return new_appstruct

h/services/__init__.py

@@ -38,6 +38,7 @@ def includeme(config):
     config.register_service_factory('.user_unique.user_unique_factory', name='user_unique')
     config.register_service_factory('.user_password.user_password_service_factory', name='user_password')
     config.register_service_factory('.user_signup.user_signup_service_factory', name='user_signup')
+    config.register_service_factory('.user_update.user_update_factory', name='user_update')
 
     config.add_directive('add_annotation_link_generator',
                          '.links.add_annotation_link_generator')

h/services/user_update.py

@@ -0,0 +1,70 @@
+# -*- coding: utf-8 -*-
+
+from __future__ import unicode_literals
+
+from sqlalchemy.exc import SQLAlchemyError
+
+from h.services.exceptions import ConflictError, ValidationError
+
+
+class UserUpdateService(object):
+    def __init__(self, session):
+        """
+        Create a new UserUpdateService
+
+        :param session: the SQLAlchemy session object
+        """
+        self.session = session
+
+    def update(self, user, **kwargs):
+        """
+        Update a user model with the args provided.
+
+        :arg user: the group to update
+        :type user: ~h.models.User
+
+        :raise ValidationError: if setting an attribute on the model raises :exc:`ValueError`
+                                or if ``authority`` is present in ``kwargs``
+
+        :rtype: ~h.models.User
+        """
+
+        # Much repurcussion if a user's authority is changed at this point.
+        # May wish to re-evaluate later if users need to be moved between
+        # authorities.
+        if "authority" in kwargs:
+            raise ValidationError("A user's authority may not be changed")
+
+        for key, value in kwargs.items():
+            try:
+                setattr(user, key, value)
+            except ValueError as err:
+                raise ValidationError(err)
+
+        try:
+            self.session.flush()
+
+        except SQLAlchemyError as err:
+            # Handle DB integrity issues with duplicate ``authority_provided_id``
+            if (
+                'duplicate key value violates unique constraint "ix__user__userid"'
+                in repr(err)
+            ):
+                # This conflict can arise from changes to either username or authority.
+                # We know this isn't authority, because the presence of authority
+                # would have already raised.
+                raise ConflictError(
+                    "username '{username}' is already in use".format(
+                        username=kwargs["username"]
+                    )
+                )
+            else:
+                # Re-raise as this is an unexpected problem
+                raise
+
+        return user
+
+
+def user_update_factory(context, request):
+    """Return a UserUpdateService instance for the passed context and request."""
+    return UserUpdateService(session=request.db)

tests/h/schemas/api/user_test.py

@@ -213,6 +213,17 @@ def test_it_raises_when_display_name_too_long(self, schema, payload):
         with pytest.raises(ValidationError):
             schema.validate(payload)
 
+    def test_it_ignores_non_whitelisted_properties(self, schema):
+        appstruct = schema.validate({
+            'display_name': 'Full Name',
+            'authority': 'dangerous.biz',
+            'orcid': '3094839jkfj',
+        })
+
+        assert 'display_name' in appstruct
+        assert 'authority' not in appstruct
+        assert 'orcid' not in appstruct
+
     @pytest.fixture
     def payload(self):
         return {

tests/h/services/user_update_test.py

@@ -0,0 +1,100 @@
+# -*- coding: utf-8 -*-
+
+from __future__ import unicode_literals
+
+import pytest
+import mock
+
+from sqlalchemy.exc import SQLAlchemyError
+
+from h.services.exceptions import ConflictError, ValidationError
+from h.services.user_update import UserUpdateService
+from h.services.user_update import user_update_factory
+
+
+class TestUserUpdate(object):
+
+    def test_it_updates_valid_user_attrs(self, factories, svc):
+        user = factories.User()
+        data = {
+            'display_name': 'foobar',
+            'email': 'foobar@example.com',
+        }
+
+        svc.update(user, **data)
+
+        assert user.display_name == 'foobar'
+        assert user.email == 'foobar@example.com'
+
+    def test_it_returns_updated_user_model(self, factories, svc):
+        user = factories.User()
+        data = {'display_name': 'whatnot'}
+
+        updated_user = svc.update(user, **data)
+
+        assert updated_user == user
+
+    def test_it_does_not_protect_against_undefined_properties(self, factories, svc):
+        user = factories.User()
+        data = {'some_random_field': 'whatever'}
+
+        updated_user = svc.update(user, **data)
+
+        # This won't be persisted in the DB, of course, but the model instance
+        # doesn't have a problem with it
+        assert updated_user.some_random_field == 'whatever'
+
+    def test_it_raises_ValidationError_if_authority_present_in_kwargs(self, factories, svc, db_session):
+        user = factories.User()
+
+        with pytest.raises(ValidationError, match="A user's authority may not be changed"):
+            svc.update(user, authority='something.com')
+
+    def test_it_raises_ValidationError_if_email_fails_model_validation(self, factories, svc, db_session):
+        user = factories.User()
+
+        with pytest.raises(ValidationError, match='email must be less than.*characters long'):
+            svc.update(user, email='o' * 150)
+
+    def test_it_raises_ValidationError_if_username_fails_model_validation(self, factories, svc, db_session):
+        user = factories.User()
+
+        with pytest.raises(ValidationError, match='username must be between.*characters long'):
+            svc.update(user, username='lo')
+
+    def test_it_will_not_raise_on_malformed_email(self, factories, svc, db_session):
+        user = factories.User()
+
+        # It's up to callers to validate email at this point
+        updated_user = svc.update(user, email='fingers')
+
+        assert updated_user.email == 'fingers'
+
+    def test_it_raises_ConflictError_on_username_authority_uniqueness_violation(self, factories, svc, db_session):
+        factories.User(username='user1', authority='baz.com')
+        user2 = factories.User(username='user2', authority='baz.com')
+
+        with pytest.raises(ConflictError, match='username'):
+            svc.update(user2, username='user1')
+
+    def test_it_raises_on_any_other_SQLAlchemy_exception(self, factories):
+        fake_session = mock.Mock()
+        fake_session.flush.side_effect = SQLAlchemyError('foo')
+
+        update_svc = UserUpdateService(session=fake_session)
+        user = factories.User()
+
+        with pytest.raises(SQLAlchemyError):
+            update_svc.update(user, username='fingers')
+
+
+class TestFactory(object):
+    def test_returns_user_update_service(self, pyramid_request):
+        user_update_service = user_update_factory(None, pyramid_request)
+
+        assert isinstance(user_update_service, UserUpdateService)
+
+
+@pytest.fixture
+def svc(db_session):
+    return UserUpdateService(session=db_session)