diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index cc71fc758..4107e53cd 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -47,8 +47,8 @@ def user_profile_name(): form = ChangeNameForm(new_name=current_user.name) if form.validate_on_submit(): - current_user.name = form.new_name.data - user_api_client.update_user(current_user) + user_api_client.update_user_attribute(current_user.id, + name=form.new_name.data) return redirect(url_for('.user_profile')) return render_template( @@ -107,7 +107,6 @@ def user_profile_email_authenticate(): @main.route("/user-profile/email/confirm/", methods=['GET']) @login_required def user_profile_email_confirm(token): - token_data = check_token(token, current_app.config['SECRET_KEY'], current_app.config['DANGEROUS_SALT'], @@ -115,9 +114,8 @@ def user_profile_email_confirm(token): token_data = json.loads(token_data) user_id = token_data['user_id'] new_email = token_data['email'] - user = user_api_client.get_user(user_id) - user.email_address = new_email - user_api_client.update_user(user) + user_api_client.update_user_attribute(user_id, + email_address=new_email) session.pop(NEW_EMAIL, None) return redirect(url_for('.user_profile')) @@ -179,10 +177,11 @@ def user_profile_mobile_number_confirm(): form = ConfirmMobileNumberForm(_check_code) if form.validate_on_submit(): - current_user.mobile_number = session[NEW_MOBILE] + mobile_number = session[NEW_MOBILE] del session[NEW_MOBILE] del session[NEW_MOBILE_PASSWORD_CONFIRMED] - user_api_client.update_user(current_user) + user_api_client.update_user_attribute(current_user.id, + mobile_number=mobile_number) return redirect(url_for('.user_profile')) return render_template( diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index d3c077333..c68a2e1e7 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -3,6 +3,12 @@ from notifications_python_client.errors import HTTPError from app.notify_client.models import User +ALLOWED_ATTRIBUTES = { + 'name', + 'email_address', + 'mobile_number' +} + class UserApiClient(BaseAPIClient): def __init__(self): @@ -53,6 +59,19 @@ class UserApiClient(BaseAPIClient): user_data = self.put(url, data=data) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) + def update_user_attribute(self, user_id, **kwargs): + data = dict(kwargs) + disallowed_attributes = set(data.keys()) - ALLOWED_ATTRIBUTES + if disallowed_attributes: + raise TypeError('Not allowed to update user attributes: {}'.format( + ", ".join(disallowed_attributes) + )) + + data = dict(**kwargs) + url = "/user/{}".format(user_id) + user_data = self.post(url, data=data) + return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) + def verify_password(self, user_id, password): try: url = "/user/{}/verify/password".format(user_id) diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index c2711ae05..09bd83f95 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -32,8 +32,8 @@ def test_should_show_name_page(app_, def test_should_redirect_after_name_change(app_, api_user_active, mock_login, - mock_update_user, - mock_get_user): + mock_get_user, + mock_update_user_attribute): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -46,7 +46,7 @@ def test_should_redirect_after_name_change(app_, assert response.location == url_for( 'main.user_profile', _external=True) api_user_active.name = new_name - assert mock_update_user.called + assert mock_update_user_attribute.called def test_should_show_email_page(app_, @@ -116,7 +116,8 @@ def test_should_render_change_email_continue_after_authenticate_email(app_, def test_should_redirect_to_user_profile_when_user_confirms_email_link(app_, api_user_active, - mock_login + mock_login, + mock_update_user_attribute ): with app_.test_request_context(): with app_.test_client() as client: @@ -218,6 +219,7 @@ def test_should_redirect_after_mobile_number_confirm(app_, api_user_active, mock_login, mock_get_user, + mock_update_user_attribute, mock_check_verify_code): with app_.test_request_context(): with app_.test_client() as client: diff --git a/tests/app/notify_client/test_user_client.py b/tests/app/notify_client/test_user_client.py index 433a36952..9062ba8bd 100644 --- a/tests/app/notify_client/test_user_client.py +++ b/tests/app/notify_client/test_user_client.py @@ -1,3 +1,5 @@ +import pytest + from app.notify_client.user_api_client import UserApiClient @@ -13,3 +15,10 @@ def test_client_uses_correct_find_by_email(mocker, api_user_active): client.get_user_by_email(api_user_active.email_address) mock_get.assert_called_once_with(expected_url, params=expected_params) + + +def test_client_only_updates_allowed_attributes(mocker): + mocker.patch('app.notify_client.current_user', id='1') + with pytest.raises(TypeError) as error: + UserApiClient().update_user_attribute('user_id', id='1') + assert str(error.value) == 'Not allowed to update user attributes: id' diff --git a/tests/conftest.py b/tests/conftest.py index 6771b0546..0b115a1b3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -753,13 +753,21 @@ def mock_verify_password(mocker): @pytest.fixture(scope='function') -def mock_update_user(mocker): - def _update(user): - return user +def mock_update_user(mocker, api_user_active): + def _update(user_id, **kwargs): + return api_user_active return mocker.patch('app.user_api_client.update_user', side_effect=_update) +@pytest.fixture(scope='function') +def mock_update_user_attribute(mocker, api_user_active): + def _update(user_id, **kwargs): + return api_user_active + + return mocker.patch('app.user_api_client.update_user_attribute', side_effect=_update) + + @pytest.fixture(scope='function') def mock_is_email_unique(mocker): return mocker.patch('app.user_api_client.is_email_unique', return_value=True)