From e65dcbe199efb4bf73528eef9e72512900a37585 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 13 Jun 2017 11:05:57 +0100 Subject: [PATCH 1/3] Make search by recipient form POST not GET MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phone numbers and email addresses are showing up in URLs where we let users search for sent notifications by phone number or email address. `GET` requests put the form data as a query string in the URL. This is problematic when people are searching by a recipient’s phone number or email address, because the URL may show up: - in our server logs - in our analytics - in the user’s browser history This is bad because these are all places where we don’t want people’s personal information. It’s not too bad when this is happening a handful of times. But it would be bad if we kept aggregating this information because it would allow us to track users across services. So, while it’s not especially RESTful, it’s better for the search form to submit as a `POST` request. This way the phone number or email address goes in the body of the request and does not show up in the URL. --- app/main/views/jobs.py | 8 ++-- app/templates/views/notifications.html | 3 +- tests/app/main/views/test_activity.py | 56 +++++++++++++++++++------- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 1e85faf0b..db7d9f7ea 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -190,7 +190,7 @@ def view_job_updates(service_id, job_id): )) -@main.route('/services//notifications/') +@main.route('/services//notifications/', methods=['GET', 'POST']) @login_required @user_has_permissions('view_activity', admin_override=True) def view_notifications(service_id, message_type): @@ -200,8 +200,8 @@ def view_notifications(service_id, message_type): message_type=message_type, status=request.args.get('status') or 'sending,delivered,failed', page=request.args.get('page', 1), - to=request.args.get('to'), - search_form=SearchNotificationsForm(to=request.args.get('to')), + to=request.form.get('to', ''), + search_form=SearchNotificationsForm(to=request.form.get('to', '')), ) @@ -245,7 +245,7 @@ def get_notifications(service_id, message_type, status_override=None): template_type=[message_type], status=filter_args.get('status'), limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'], - to=request.args.get('to'), + to=request.form.get('to', ''), ) url_args = { diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 3190b4fa0..e3a55cca0 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -21,7 +21,7 @@ ) }}
@@ -34,6 +34,7 @@ ) }}
+
diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index c5e8a753b..c625165aa 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -70,14 +70,27 @@ def test_can_show_notifications( to_argument, expected_to_argument, ): - response = logged_in_client.get(url_for( - 'main.view_notifications', - service_id=service_one['id'], - message_type=message_type, - status=status_argument, - page=page_argument, - to=to_argument, - )) + if expected_to_argument: + response = logged_in_client.post( + url_for( + 'main.view_notifications', + service_id=service_one['id'], + message_type=message_type, + status=status_argument, + page=page_argument, + ), + data={ + 'to': to_argument + } + ) + else: + response = logged_in_client.get(url_for( + 'main.view_notifications', + service_id=service_one['id'], + message_type=message_type, + status=status_argument, + page=page_argument, + )) assert response.status_code == 200 content = response.get_data(as_text=True) notifications = notification_json(service_one['id']) @@ -119,17 +132,25 @@ def test_can_show_notifications( assert json_content.keys() == {'counts', 'notifications'} -@pytest.mark.parametrize("initial_query_arguments, expected_status_field_value, expected_search_box_contents", [ +@pytest.mark.parametrize(( + 'initial_query_arguments,' + 'form_post_data,' + 'expected_status_field_value,' + 'expected_search_box_contents' +), [ ( { 'message_type': 'sms', }, + {}, 'sending,delivered,failed', '', ), ( { 'message_type': 'sms', + }, + { 'to': '+33(0)5-12-34-56-78', }, 'sending,delivered,failed', @@ -140,6 +161,8 @@ def test_can_show_notifications( 'status': 'failed', 'message_type': 'email', 'page': '99', + }, + { 'to': 'test@example.com', }, 'failed', @@ -151,17 +174,22 @@ def test_search_recipient_form( mock_get_notifications, mock_get_detailed_service, initial_query_arguments, + form_post_data, expected_status_field_value, expected_search_box_contents, ): - response = logged_in_client.get(url_for( - 'main.view_notifications', - service_id=SERVICE_ONE_ID, - **initial_query_arguments - )) + response = logged_in_client.post( + url_for( + 'main.view_notifications', + service_id=SERVICE_ONE_ID, + **initial_query_arguments + ), + data=form_post_data + ) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.find("form")['method'] == 'post' action_url = page.find("form")['action'] url = urlparse(action_url) assert url.path == '/services/{}/notifications/{}'.format( From 681cea1d34eb86b43d6304bb4b3673de2d8e5fe8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 13 Jun 2017 11:20:41 +0100 Subject: [PATCH 2/3] Make AJAX requests on activity page POST not GET MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See parent commit for the reason we’re doing this. Currently our AJAX requests only work as `GET` requests. So this commit does a bit of work to make them work as `POST` requests. This is optional behaviour, and will only happen when the element in the page that should be updated with AJAX has the `data-form` attribute set. It will take the form that has the corresponding `id`, serialise it, and use that data as the body of the post request. If not form is specified it will not do the serialisation, and submit as a `GET` request as before. --- app/assets/javascripts/updateContent.js | 11 ++++++++--- app/main/views/jobs.py | 2 +- app/templates/components/ajax-block.html | 3 ++- app/templates/views/notifications.html | 7 ++++--- tests/app/main/views/test_activity.py | 9 +-------- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index 89c87c550..a8a6146d6 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -19,10 +19,14 @@ var clearQueue = queue => (queue.length = 0); - var poll = function(renderer, resource, queue, interval) { + var poll = function(renderer, resource, queue, interval, form) { if (queue.push(renderer) === 1) $.ajax( - resource + resource, + { + 'method': form ? 'post' : 'get', + 'data': form ? $('#' + form).serialize() : {} + } ).done( response => flushQueue(queue, response) ).fail( @@ -41,7 +45,8 @@ getRenderer($(component)), $(component).data('resource'), getQueue($(component).data('resource')), - ($(component).data('interval-seconds') || 1.5) * 1000 + ($(component).data('interval-seconds') || 1.5) * 1000, + $(component).data('form') ); }; diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index db7d9f7ea..d10028b35 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -205,7 +205,7 @@ def view_notifications(service_id, message_type): ) -@main.route('/services//notifications/.json') +@main.route('/services//notifications/.json', methods=['GET', 'POST']) @user_has_permissions('view_activity', admin_override=True) def get_notifications_as_json(service_id, message_type): return jsonify(get_notifications( diff --git a/app/templates/components/ajax-block.html b/app/templates/components/ajax-block.html index 4382bf700..4d40a25b3 100644 --- a/app/templates/components/ajax-block.html +++ b/app/templates/components/ajax-block.html @@ -1,10 +1,11 @@ -{% macro ajax_block(partials, url, key, interval=2, finished=False) %} +{% macro ajax_block(partials, url, key, interval=2, finished=False, form='') %} {% if not finished %}
{% endif %} diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index e3a55cca0..a323c9ae8 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -24,9 +24,9 @@ method="post" action="{{ url_for('.view_notifications', service_id=current_service.id, message_type=message_type) }}" class="grid-row" + id="search-form" >
- {{ textbox( search_form.to, width='1-1', @@ -41,8 +41,9 @@ {{ ajax_block( partials, - url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page, to=to), - 'notifications' + url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page), + 'notifications', + form='search-form' ) }} {% endblock %} diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index c625165aa..61c5cc873 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -110,8 +110,7 @@ def test_can_show_notifications( assert query_dict['status'] == [status_argument] if expected_page_argument: assert query_dict['page'] == [str(expected_page_argument)] - if to_argument: - assert query_dict['to'] == [to_argument] + assert 'to' not in query_dict mock_get_notifications.assert_called_with( limit_days=7, @@ -135,7 +134,6 @@ def test_can_show_notifications( @pytest.mark.parametrize(( 'initial_query_arguments,' 'form_post_data,' - 'expected_status_field_value,' 'expected_search_box_contents' ), [ ( @@ -143,7 +141,6 @@ def test_can_show_notifications( 'message_type': 'sms', }, {}, - 'sending,delivered,failed', '', ), ( @@ -153,7 +150,6 @@ def test_can_show_notifications( { 'to': '+33(0)5-12-34-56-78', }, - 'sending,delivered,failed', '+33(0)5-12-34-56-78', ), ( @@ -165,7 +161,6 @@ def test_can_show_notifications( { 'to': 'test@example.com', }, - 'failed', 'test@example.com', ), ]) @@ -175,7 +170,6 @@ def test_search_recipient_form( mock_get_detailed_service, initial_query_arguments, form_post_data, - expected_status_field_value, expected_search_box_contents, ): response = logged_in_client.post( @@ -199,7 +193,6 @@ def test_search_recipient_form( query_dict = parse_qs(url.query) assert query_dict == {} - assert page.find("input", {'name': 'status'})['value'] == expected_status_field_value assert page.find("input", {'name': 'to'})['value'] == expected_search_box_contents From 7411256fc2bd76d716f839ebc9a1cba9a154ce45 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 14 Jun 2017 16:25:20 +0100 Subject: [PATCH 3/3] Add hidden form to re-`post` AJAX requests Serializing the search box form is bad, because the AJAX thing submit any changes that the user makes to the contents of the box. This results in unexpected behaviour. --- app/templates/views/notifications.html | 8 ++++++-- tests/app/main/views/test_activity.py | 6 +++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index a323c9ae8..7540720b2 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -24,7 +24,6 @@ method="post" action="{{ url_for('.view_notifications', service_id=current_service.id, message_type=message_type) }}" class="grid-row" - id="search-form" >
{{ textbox( @@ -34,11 +33,16 @@ ) }}
- +
+
+ + +
+ {{ ajax_block( partials, url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page), diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index 61c5cc873..b0f230ced 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -193,7 +193,11 @@ def test_search_recipient_form( query_dict = parse_qs(url.query) assert query_dict == {} - assert page.find("input", {'name': 'to'})['value'] == expected_search_box_contents + recipient_inputs = page.select("input[name=to]") + assert(len(recipient_inputs) == 2) + + for field in recipient_inputs: + assert field['value'] == expected_search_box_contents def test_should_show_notifications_for_a_service_with_next_previous(