From f14c3dbfa5014197ad9255dd3824f93f6bcdb8f4 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 3 Nov 2016 11:20:24 +0000 Subject: [PATCH 1/6] Stop passing the 'whole' user object when making changes to profile --- app/main/views/user_profile.py | 19 +++++++++---------- app/notify_client/user_api_client.py | 6 +++--- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index cc71fc758..7b6fbe24d 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(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(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(current_user.id, + mobile_number=mobile_number) return redirect(url_for('.user_profile')) return render_template( @@ -202,8 +201,8 @@ def user_profile_password(): form = ChangePasswordForm(_check_password) if form.validate_on_submit(): - current_user.set_password(form.new_password.data) - user_api_client.update_user(current_user) + user_api_client.update_user(current_user.id, + password=form.new_password.data) 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..1e5468505 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -47,9 +47,9 @@ class UserApiClient(BaseAPIClient): users.append(User(user, max_failed_login_count=self.max_failed_login_count)) return users - def update_user(self, user): - data = user.serialize() - url = "/user/{}".format(user.id) + def update_user(self, user_id, **kwargs): + data = dict(**kwargs) + url = "/user/{}".format(user_id) user_data = self.put(url, data=data) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) From 1c42640137a8d1666349a2aabb2a758a19055cb4 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 3 Nov 2016 11:20:43 +0000 Subject: [PATCH 2/6] Update update user mock to conform to new format --- tests/conftest.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 7dd5c9359..d96f2c6e0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -753,9 +753,9 @@ 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) From c28aea2de172fa92c34442a4870e72ef7c5e04e1 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 9 Nov 2016 15:05:06 +0000 Subject: [PATCH 3/6] Update user profile attributes with new method --- app/main/views/user_profile.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 7b6fbe24d..041b0a817 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -47,7 +47,7 @@ def user_profile_name(): form = ChangeNameForm(new_name=current_user.name) if form.validate_on_submit(): - user_api_client.update_user(current_user.id, + user_api_client.update_user_attribute(current_user.id, name=form.new_name.data) return redirect(url_for('.user_profile')) @@ -114,7 +114,7 @@ def user_profile_email_confirm(token): token_data = json.loads(token_data) user_id = token_data['user_id'] new_email = token_data['email'] - user_api_client.update_user(user_id, + user_api_client.update_user_attribute(user_id, email_address=new_email) session.pop(NEW_EMAIL, None) @@ -180,7 +180,7 @@ def user_profile_mobile_number_confirm(): mobile_number = session[NEW_MOBILE] del session[NEW_MOBILE] del session[NEW_MOBILE_PASSWORD_CONFIRMED] - user_api_client.update_user(current_user.id, + user_api_client.update_user_attribute(current_user.id, mobile_number=mobile_number) return redirect(url_for('.user_profile')) @@ -201,8 +201,8 @@ def user_profile_password(): form = ChangePasswordForm(_check_password) if form.validate_on_submit(): - user_api_client.update_user(current_user.id, - password=form.new_password.data) + current_user.set_password(form.new_password.data) + user_api_client.update_user(current_user) return redirect(url_for('.user_profile')) return render_template( From f3ca33dad3a1ca66e172b31ccfca5f60ac310fa6 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 9 Nov 2016 15:06:02 +0000 Subject: [PATCH 4/6] Revert original update user method and add new attribute update (with strict checking) --- app/notify_client/user_api_client.py | 23 +++++++++++++++++++-- tests/app/notify_client/test_user_client.py | 9 ++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 1e5468505..3457274ae 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): @@ -47,9 +53,22 @@ class UserApiClient(BaseAPIClient): users.append(User(user, max_failed_login_count=self.max_failed_login_count)) return users - def update_user(self, user_id, **kwargs): + def update_user(self, user): + data = user.serialize() + url = "/user/{}".format(user.id) + 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) + url = "/user/{}/update-attribute".format(user_id) user_data = self.put(url, data=data) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) 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' From e5ea81b1844d811b6a3ba9891dcb077c2cdc4759 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 10 Nov 2016 10:45:09 +0000 Subject: [PATCH 5/6] Fix pep issues and refactor tests --- app/main/views/user_profile.py | 6 +++--- tests/app/main/views/test_user_profile.py | 10 ++++++---- tests/conftest.py | 8 ++++++++ 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 041b0a817..4107e53cd 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -48,7 +48,7 @@ def user_profile_name(): if form.validate_on_submit(): user_api_client.update_user_attribute(current_user.id, - name=form.new_name.data) + name=form.new_name.data) return redirect(url_for('.user_profile')) return render_template( @@ -115,7 +115,7 @@ def user_profile_email_confirm(token): user_id = token_data['user_id'] new_email = token_data['email'] user_api_client.update_user_attribute(user_id, - email_address=new_email) + email_address=new_email) session.pop(NEW_EMAIL, None) return redirect(url_for('.user_profile')) @@ -181,7 +181,7 @@ def user_profile_mobile_number_confirm(): del session[NEW_MOBILE] del session[NEW_MOBILE_PASSWORD_CONFIRMED] user_api_client.update_user_attribute(current_user.id, - mobile_number=mobile_number) + mobile_number=mobile_number) return redirect(url_for('.user_profile')) return render_template( 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/conftest.py b/tests/conftest.py index d96f2c6e0..e7eea554d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -760,6 +760,14 @@ def mock_update_user(mocker, 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) From 1da498164fa30620f1274e72add565f9874d27dd Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 10 Nov 2016 12:10:01 +0000 Subject: [PATCH 6/6] Use POST req to update a user attr --- app/notify_client/user_api_client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 3457274ae..c68a2e1e7 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -68,8 +68,8 @@ class UserApiClient(BaseAPIClient): )) data = dict(**kwargs) - url = "/user/{}/update-attribute".format(user_id) - user_data = self.put(url, data=data) + 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):