From 906d54aa3d374359110fbcec5300c1e08aa6d880 Mon Sep 17 00:00:00 2001 From: Mack Halliday Date: Fri, 13 Dec 2024 12:19:41 -0500 Subject: [PATCH 1/3] TEAM-1420 - Updating API Key generation Co-authored-by: David Kalbfleisch <1.21e9W@protonmail.com> --- .talismanrc | 4 ++-- app/dao/api_key_dao.py | 10 +++++----- tests/app/dao/test_api_key_dao.py | 11 +++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/.talismanrc b/.talismanrc index 025f84a201..565dfae0c0 100644 --- a/.talismanrc +++ b/.talismanrc @@ -14,7 +14,7 @@ fileignoreconfig: - filename: app/constants.py checksum: 44390d0a1258b184cf84dc9b6e97bd0768af84a9aa346ba963aa7735fc8bcb36 - filename: app/dao/api_key_dao.py - checksum: ab93313f306c8a3f6576141e8f32d9fc99b0de7da8d44a1ddbe6ea55d167dcdb + checksum: c44cbd8ae02fb1d551a1f0941365c11977564a6444950ee2b0282ee4b5fd1314 - filename: app/letters/utils.py checksum: 5e6071b9cab380f9f3ee172f8c731061241200f53453a9863f22bb5eaa05e6af - filename: app/notifications/process_notifications.py @@ -72,7 +72,7 @@ fileignoreconfig: - filename: tests/app/conftest.py checksum: a80aa727586db82ed1b50bdb81ddfe1379e649a9dfc1ece2c36047486b41b83d - filename: tests/app/dao/test_api_key_dao.py - checksum: ef306fcc1dc512b74abeb5dde5f20977cf95e67a2fa049df6289a7b5500339a9 + checksum: 40e551ca6677aab7657bbb43efdac56aa3c51065ed99052faff9bc1519e5b0df - filename: tests/app/notifications/test_process_notifications_for_profile_v3.py checksum: 4e15e63d349635131173ffdd7aebcd547621db08de877ef926d3a41fde72d065 - filename: tests/app/notifications/test_send_notifications.py diff --git a/app/dao/api_key_dao.py b/app/dao/api_key_dao.py index 87b0aaba7b..1e1e8b8066 100644 --- a/app/dao/api_key_dao.py +++ b/app/dao/api_key_dao.py @@ -1,12 +1,12 @@ +import secrets import uuid from datetime import datetime, timedelta -from app import db -from app.models import ApiKey +from sqlalchemy import func, or_, select +from app import db from app.dao.dao_utils import transactional, version_class - -from sqlalchemy import or_, func, select +from app.models import ApiKey @transactional @@ -15,7 +15,7 @@ def save_model_api_key(api_key): if not api_key.id: api_key.id = uuid.uuid4() # must be set now so version history model can use same id if not api_key.secret: - api_key.secret = uuid.uuid4() + api_key.secret = secrets.token_urlsafe(64) db.session.add(api_key) diff --git a/tests/app/dao/test_api_key_dao.py b/tests/app/dao/test_api_key_dao.py index 5167866e34..360ab9d309 100644 --- a/tests/app/dao/test_api_key_dao.py +++ b/tests/app/dao/test_api_key_dao.py @@ -187,3 +187,14 @@ def test_should_not_return_revoked_api_keys_older_than_7_days( all_api_keys = get_model_api_keys(service_id=service.id) assert len(all_api_keys) == expected_length + + +def test_save_api_key_should_generate_secret_with_expected_format(sample_service): + service = sample_service() + api_key = ApiKey( + **{'service': service, 'name': service.name, 'created_by': service.created_by, 'key_type': KEY_TYPE_NORMAL} + ) + save_model_api_key(api_key) + + assert api_key.secret is not None + assert len(api_key.secret) >= 86 From 5c5d5286dec87967247157b3e64866cb50d48797 Mon Sep 17 00:00:00 2001 From: Michael Wellman Date: Fri, 13 Dec 2024 16:11:32 -0500 Subject: [PATCH 2/3] #1357 Redact Personalization in Serialized Notifications (#2180) --- .talismanrc | 6 +- app/models.py | 9 ++- tests/app/test_model.py | 2 +- .../notifications/test_get_notifications.py | 55 ++++++++++++++++++- 4 files changed, 65 insertions(+), 7 deletions(-) diff --git a/.talismanrc b/.talismanrc index 565dfae0c0..0832a6f9ad 100644 --- a/.talismanrc +++ b/.talismanrc @@ -69,6 +69,8 @@ fileignoreconfig: checksum: 6ffb8742a19c5b834c608826fd459cc1b6ea35ebfffd2d929a3a0f269c74183d - filename: tests/app/celery/test_service_callback_tasks.py checksum: 70575434f7a4fedd43d4c9164bc899a606768526d432c364db372524eec26542 +- filename: tests/app/clients/test_twilio.py + checksum: cad49e634cc5ba56157358aa3dfba2dafe7b9dbd3a0c580ec5cda3072f6a76e5 - filename: tests/app/conftest.py checksum: a80aa727586db82ed1b50bdb81ddfe1379e649a9dfc1ece2c36047486b41b83d - filename: tests/app/dao/test_api_key_dao.py @@ -77,6 +79,8 @@ fileignoreconfig: checksum: 4e15e63d349635131173ffdd7aebcd547621db08de877ef926d3a41fde72d065 - filename: tests/app/notifications/test_send_notifications.py checksum: 69304e1edd6993acd9a842a87dba8ee16854befc643863e1589b15c303a71464 +- filename: tests/app/v2/notifications/test_get_notifications.py + checksum: f2460d8b22ff7ff2e89778dbbf3e0740cbb41dead60c49c32648d367aa05fe0e - filename: tests/app/v2/notifications/test_post_notifications.py checksum: 3181930a13e3679bb2f17eaa3f383512eb9caf4ed5d5e14496ca4193c6083965 - filename: tests/app/v3/notifications/test_rest.py @@ -85,6 +89,4 @@ fileignoreconfig: checksum: 4f0f4d7a4113762219e45a51f7b26a7c0cb83f1d8f10c5598533f6cdcf0e0ada - filename: tests/lambda_functions/vetext_incoming_forwarder_lambda/test_vetext_incoming_forwarder_lambda.py checksum: 7494eb4321fd2fbc3ff3915d8753d8fec7a936a69dc6ab78f0b532a701f032eb -- filename: tests/app/clients/test_twilio.py - checksum: cad49e634cc5ba56157358aa3dfba2dafe7b9dbd3a0c580ec5cda3072f6a76e5 version: "1.0" diff --git a/app/models.py b/app/models.py index 0ee53de0ec..658845759f 100644 --- a/app/models.py +++ b/app/models.py @@ -2,6 +2,7 @@ from typing import Any, Dict, Optional import datetime +import html import itertools import uuid @@ -1331,7 +1332,7 @@ def _substitute_status_seq(_statuses): def content(self): from app.utils import get_template_instance - template_object = get_template_instance(self.template.__dict__, self.personalisation) + template_object = get_template_instance(self.template.__dict__, {k: '' for k in self.personalisation}) return str(template_object) @property @@ -1339,8 +1340,10 @@ def subject(self): from app.utils import get_template_instance if self.notification_type != SMS_TYPE: - template_object = get_template_instance(self.template.__dict__, self.personalisation) - return template_object.subject + template_object = get_template_instance( + self.template.__dict__, {k: '' for k in self.personalisation} + ) + return html.unescape(str(template_object.subject)) @property def formatted_status(self): diff --git a/tests/app/test_model.py b/tests/app/test_model.py index d259268c52..6bc05b3611 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -133,7 +133,7 @@ def test_notification_subject_fills_in_placeholders( ): template = sample_template(template_type=EMAIL_TYPE, subject='((name))') notification = sample_notification(template=template, personalisation={'name': 'hello'}) - assert notification.subject == 'hello' + assert notification.subject == '' def test_notification_serializes_created_by_name_with_no_created_by_id(client, sample_notification): diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 64ff0dd892..3a6d43839f 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -155,7 +155,7 @@ def test_get_notification_by_id_with_placeholders_and_recipient_identifiers_retu 'template': expected_template_response, 'created_at': notification.created_at.strftime(DATETIME_FORMAT), 'created_by_name': None, - 'body': 'Hello Bob\nThis is an email from va.gov', + 'body': 'Hello \nThis is an email from va.gov', 'subject': 'Subject', 'sent_at': notification.sent_at, 'sent_by': None, @@ -881,3 +881,56 @@ def test_get_notifications_renames_letter_statuses(client, sample_letter_templat json_response = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 assert json_response['status'] == expected_status + + +@pytest.mark.parametrize('template_type', [SMS_TYPE, EMAIL_TYPE]) +def test_get_notifications_removes_personalisation_from_content( + client, + sample_api_key, + sample_notification, + sample_template, + template_type, +): + template = sample_template( + content='Hello ((name))\nThis is an email from VA Notify about some ((thing))', + template_type=template_type, + ) + notification = sample_notification( + template=template, + personalisation={'name': 'Bob', 'thing': 'important stuff'}, + ) + auth_header = create_authorization_header(sample_api_key(service=template.service)) + response = client.get( + path=url_for('v2_notifications.get_notification_by_id', notification_id=notification.id), + headers=[('Content-Type', 'application/json'), auth_header], + ) + + json_response = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_response['body'] == 'Hello \nThis is an email from VA Notify about some ' + + +def test_get_notifications_removes_personalisation_from_subject( + client, + sample_api_key, + sample_notification, + sample_template, +): + template = sample_template( + subject='Hello ((name))', + content='This is an email', + template_type=EMAIL_TYPE, + ) + notification = sample_notification( + template=template, + personalisation={'name': 'Bob'}, + ) + auth_header = create_authorization_header(sample_api_key(service=template.service)) + response = client.get( + path=url_for('v2_notifications.get_notification_by_id', notification_id=notification.id), + headers=[('Content-Type', 'application/json'), auth_header], + ) + + json_response = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_response['subject'] == 'Hello ' From 3ea622784c6773d3b7834e730e393124f160fb6a Mon Sep 17 00:00:00 2001 From: Michael Wellman Date: Mon, 16 Dec 2024 13:56:49 -0500 Subject: [PATCH 3/3] #1357 Redact personalization in POST responses. (#2185) --- app/v2/notifications/post_notifications.py | 5 +++-- .../v2/notifications/test_post_notifications.py | 14 +++++++------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index ffed2020b4..524fb5c980 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,4 +1,5 @@ import functools +import html import werkzeug from flask import request, jsonify, current_app, abort @@ -128,13 +129,13 @@ def post_notification(notification_type): # noqa: C901 current_app.logger.debug('Sending a notification without contact information is not implemented.') return jsonify(result='error', message='Not Implemented'), 501 - template_with_content.values = notification.personalisation + template_with_content.values = {k: '' for k in notification.personalisation} if notification_type == SMS_TYPE: create_resp_partial = functools.partial(create_post_sms_response_from_notification, from_number=reply_to) elif notification_type == EMAIL_TYPE: create_resp_partial = functools.partial( - create_post_email_response_from_notification, subject=template_with_content.subject + create_post_email_response_from_notification, subject=html.unescape(template_with_content.subject) ) elif notification_type == LETTER_TYPE: create_resp_partial = functools.partial( diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 09df6e14b1..614ad4bf19 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -98,7 +98,7 @@ def test_post_sms_notification_returns_201( # endpoint checks assert resp_json['id'] == str(notifications[0].id) assert resp_json['reference'] == reference - assert resp_json['content']['body'] == template.content.replace('(( Name))', 'Jo') + assert resp_json['content']['body'] == template.content.replace('(( Name))', '') assert resp_json['content']['from_number'] == current_app.config['FROM_NUMBER'] assert f'v2/notifications/{notifications[0].id}' in resp_json['uri'] assert resp_json['template']['id'] == str(template.id) @@ -423,7 +423,7 @@ def test_post_sms_notification_without_callback_url( assert notification.callback_url is None assert resp_json['id'] == str(notification.id) - assert resp_json['content']['body'] == template.content.replace('(( Name))', 'Jo') + assert resp_json['content']['body'] == template.content.replace('(( Name))', '') assert resp_json['callback_url'] is None @@ -504,8 +504,8 @@ def test_post_email_notification_returns_201( assert resp_json['reference'] == reference assert notification.reference is None assert notification.reply_to_text is None - assert resp_json['content']['body'] == template.content.replace('((name))', 'Bob') - assert resp_json['content']['subject'] == template.subject.replace('((name))', 'Bob') + assert resp_json['content']['body'] == template.content.replace('((name))', '') + assert resp_json['content']['subject'] == template.subject.replace('((name))', '') assert 'v2/notifications/{}'.format(notification.id) in resp_json['uri'] assert resp_json['template']['id'] == str(template.id) assert resp_json['template']['version'] == template.version @@ -560,8 +560,8 @@ def test_post_email_notification_with_reply_to_returns_201( assert resp_json['id'] == str(notification.id) assert resp_json['billing_code'] == 'TESTCODE' assert resp_json['reference'] == reference - assert resp_json['content']['body'] == template.content.replace('((name))', 'Bob') - assert resp_json['content']['subject'] == template.subject.replace('((name))', 'Bob') + assert resp_json['content']['body'] == template.content.replace('((name))', '') + assert resp_json['content']['subject'] == template.subject.replace('((name))', '') assert 'v2/notifications/{}'.format(notification.id) in resp_json['uri'] assert resp_json['template']['id'] == str(template.id) assert resp_json['template']['version'] == template.version @@ -1337,7 +1337,7 @@ def test_simulated( resp_json = response.get_json() assert validate(resp_json, post_email_response) == resp_json - assert resp_json['content']['body'] == 'Document: simulated-attachment-url' + assert resp_json['content']['body'] == 'Document: ' def test_without_document_upload_permission( self,