diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 000000000..cb6b2c84b --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,79 @@ + + +*A note to PR reviewers: it may be helpful to review our +[code review documentation](https://github.com/GSA/notifications-api/blob/main/docs/all.md#code-reviews) +to know what to keep in mind while reviewing pull requests.* + +## Description + +Please enter a clear description about your proposed changes and what the +expected outcome(s) is/are from there. If there are complex implementation +details within the changes, this is a great place to explain those details using +plain language. + +This should include: + +- Links to issues that this PR addresses +- Screenshots or screen captures of any visible changes, especially for UI work +- Dependency changes + +If there are any caveats, known issues, follow-up items, etc., make a quick note +of them here as well, though more details are probably warranted in the issue +itself in this case. + +## TODO (optional) + +If you're opening a draft PR, it might be helpful to list any outstanding work, +especially if you're asking folks to take a look before it's ready for full +review. In this case, create a small checklist with the outstanding items: + +- [ ] TODO item 1 +- [ ] TODO item 2 +- [ ] TODO item ... + +## Security Considerations + +Please think about the security compliance aspect of your changes and what the +potential impacts might be. + +**NOTE: Please be mindful of sharing sensitive information here! If you're not +sure of what to write, please ask the team first before writing anything here.** + +Relevant details could include (and are not limited to) the following: + +- Handling secrets/credential management (or specifically calling out that there + is nothing to handle) +- Any adjustments to the flow of data in and out the system, or even within it +- Connecting or disconnecting any external services to the application +- Handling of any sensitive information, such as PII +- Handling of information within log statements or other application monitoring + services/hooks +- The inclusion of a new external dependency or the removal of an existing one +- ... (anything else relevant from a security compliance perspective) + +There are some cases where there are no security considerations to be had, e.g., +updating our documentation with publicly available information. In those cases +it is fine to simply put something like this: + +- None; this is a documentation update with publicly available information. diff --git a/app/clients/cloudwatch/aws_cloudwatch.py b/app/clients/cloudwatch/aws_cloudwatch.py index 4fac575a3..0deac089e 100644 --- a/app/clients/cloudwatch/aws_cloudwatch.py +++ b/app/clients/cloudwatch/aws_cloudwatch.py @@ -90,23 +90,39 @@ class AwsCloudwatchClient(Client): account_number = ses_domain_arn.split(":") return account_number + def warn_if_dev_is_opted_out(self, provider_response, notification_id): + if ( + "is opted out" in provider_response.lower() + or "has blocked sms" in provider_response.lower() + ): + if os.getenv("NOTIFY_ENVIRONMENT") in ["development", "test"]: + ansi_red = "\033[31m" + ansi_reset = "\033[0m" + logline = ( + ansi_red + + f"The phone number for notification_id {notification_id} is OPTED OUT. You need to opt back in" + + ansi_reset + ) + current_app.logger.warning(logline) + return logline + return None + def check_sms(self, message_id, notification_id, created_at): - current_app.logger.info(f"CREATED AT = {created_at}") region = cloud_config.sns_region # TODO this clumsy approach to getting the account number will be fixed as part of notify-api #258 account_number = self._extract_account_number(cloud_config.ses_domain_arn) time_now = datetime.utcnow() log_group_name = f"sns/{region}/{account_number[4]}/DirectPublishToPhoneNumber" - current_app.logger.info( - f"Log group name: {log_group_name} message id: {message_id}" - ) filter_pattern = '{$.notification.messageId="XXXXX"}' filter_pattern = filter_pattern.replace("XXXXX", message_id) all_log_events = self._get_log(filter_pattern, log_group_name, created_at) if all_log_events and len(all_log_events) > 0: event = all_log_events[0] message = json.loads(event["message"]) + self.warn_if_dev_is_opted_out( + message["delivery"]["providerResponse"], notification_id + ) return ( "success", message["delivery"]["providerResponse"], @@ -116,13 +132,13 @@ class AwsCloudwatchClient(Client): log_group_name = ( f"sns/{region}/{account_number[4]}/DirectPublishToPhoneNumber/Failure" ) - current_app.logger.info(f"Failure log group name: {log_group_name}") all_failed_events = self._get_log(filter_pattern, log_group_name, created_at) if all_failed_events and len(all_failed_events) > 0: - current_app.logger.info("SHOULD RETURN FAILED BECAUSE WE FOUND A FAILURE") event = all_failed_events[0] message = json.loads(event["message"]) - current_app.logger.info(f"MESSAGE {message}") + self.warn_if_dev_is_opted_out( + message["delivery"]["providerResponse"], notification_id + ) return ( "failure", message["delivery"]["providerResponse"], diff --git a/docs/all.md b/docs/all.md index c74b11af3..4803102dd 100644 --- a/docs/all.md +++ b/docs/all.md @@ -36,17 +36,20 @@ - [Celery scheduled tasks](#celery-scheduled-tasks) - [Notify.gov](#notifygov) - [System Description](#system-description) +- [Code Reviews](#code-reviews) + - [For the reviewer](#for-the-reviewer) + - [For the author](#for-the-author) - [Run Book](#run-book) - - [ Alerts, Notifications, Monitoring](#-alerts-notifications-monitoring) - - [ Restaging Apps](#-restaging-apps) - - [ Smoke-testing the App](#-smoke-testing-the-app) - - [ Simulated bulk send testing](#-simulated-bulk-send-testing) - - [ Configuration Management](#-configuration-management) - - [ DNS Changes](#-dns-changes) + - [Alerts, Notifications, Monitoring](#-alerts-notifications-monitoring) + - [Restaging Apps](#-restaging-apps) + - [Smoke-testing the App](#-smoke-testing-the-app) + - [Simulated bulk send testing](#-simulated-bulk-send-testing) + - [Configuration Management](#-configuration-management) + - [DNS Changes](#-dns-changes) - [Exporting test results for compliance monitoring](#exporting-test-results-for-compliance-monitoring) - - [ Known Gotchas](#-known-gotchas) - - [ User Account Management](#-user-account-management) - - [ SMS Phone Number Management](#-sms-phone-number-management) + - [Known Gotchas](#-known-gotchas) + - [User Account Management](#-user-account-management) + - [SMS Phone Number Management](#-sms-phone-number-management) - [Data Storage Policies \& Procedures](#data-storage-policies--procedures) - [Potential PII Locations](#potential-pii-locations) - [Data Retention Policy](#data-retention-policy) @@ -711,10 +714,6 @@ make run-celery-beat ``` - - - - Notify.gov ========= @@ -755,6 +754,102 @@ Notify.gov also provisions and uses two AWS services via a [supplemental service For further details of the system and how it connects to supporting services, see the [application boundary diagram](https://github.com/GSA/us-notify-compliance/blob/main/diagrams/rendered/apps/application.boundary.png) +Code Reviews +============ + +When conducting a code review there are several things to keep in mind to ensure +a quality and valuable review. Remember, we're trying to improve Notify.gov as +best we can; it does us no good if we do not double check that our work meets +our standards, especially before going out the door! + +It also does us no good if we do not treat each other without mutual respect or +consideration either; if there are mistakes or oversights found in a pull +request, or even just suggestions for alternative ways of approaching something, +these become learning opportunities for all parties involved in addition to +modeling positive behavior and practices for the public and broader open source +community. + +Given this basis of approaching code reviews, here are some general guidelines +and suggestions for how to approach a code review from the perspectives of both +the reviewer and the author. + +### For the reviewer + +When performing a code review, please be curious and critical while also being +respectful and appreciative of the work submitted. Code reviews are a chance +to check that things meet our standards and provide learning opportunities. +They are not places for belittling or disparaging someone's work or approach to +a task, and absolutely not the person(s) themselves. + +That said, any responses to the code review should also be respectful and +considerate. Remember, this is a chance to not only improve our work and the +state of Notify.gov, it's also a chance to learn something new! + +**Note: If a response is condescending, derogatory, disrespectful, etc., please +do not hesitate to either speak with the author(s) directly about this or reach +out to a team lead/supervisor for additional help to rectify the issue. Such +behavior and lack of professionalism is not acceptable or tolerated.** + +When performing a code review, it is helpful to keep the following guidelines in +mind: + +- Be on the lookout for any sensitive information and/or leaked credentials, + secrets, PII, etc. +- Ask and call out things that aren't clear to you; it never hurts to double + check your understanding of something! +- Check that things are named descriptively and appropriately and call out + anything that is not. +- Check that comments are present for complex areas when needed. +- Make sure the pull request itself is properly prepared - it has a clear + description, calls out security concerns, and has the necessary labels, flags, + issue link, etc., set on it. +- Do not be shy about using the suggested changes feature in GitHub pull request + comments; this can help save a lot of time! +- Do not be shy about marking a review with the `Request Changes` status - yes, + it looks big and red when it shows up, but this is completely fine and not to + be taken as a personal mark against the author(s) of the pull request! + +Additionally, if you find yourself making a lot of comments and/or end up having +several concerns about the overall approach, it will likely be helpful to +schedule time to speak with the author(s) directly and talk through everything. +This can save folks a lot of misunderstanding and back-and-forth! + +### For the author + +When receiving a code review, please remember that someone took the time to look +over all of your work with a critical eye to make sure our standards are being +met and that we're producing the best quality work possible. It's completely +fine if there are specific changes requested and/or other parts are sent back +for additional work! + +That said, the review should also be respectful, helpful, and a learning +opportunity where possible. Remember, this is a chance to not only improve your +work and the state of Notify.gov, it's also a chance to learn something new! + +**Note: If a review is condescending, derogatory, disrespectful, etc., please do +not hesitate to either speak with the reviewer(s) directly about this or reach +out to a team lead/supervisor for additional help to rectify the issue. Such +behavior and lack of professionalism is not acceptable or tolerated.** + +When going over a review, it may be helpful to keep these perspectives in mind: + +- Approach the review with an open mind, curiosity, and appreciation. +- If anything the reviewer(s) mentions is unclear to you, please ask for + clarification and engage them in further dialogue! +- If you disagree with a suggestion or request, please say so and engage in an + open and respecful dialogue to come to a mutual understanding of what the + appropriate next step(S) should be - accept the change, reject the change, + take a different path entirely, etc. +- If there are no issues with any suggested edits or requested changes, make + the necessary adjustments and let the reviewer(s) know when the work is ready + for review again. + +Additionally, if you find yourself responding to a lot of things and questioning +the feedback received throughout much of the code review, it will likely be +helpful to schedule time to speak with the reviewer(s) directly and talk through +everything. This can save folks a lot of misunderstanding and back-and-forth! + + Run Book ======== diff --git a/tests/app/clients/test_aws_cloudwatch.py b/tests/app/clients/test_aws_cloudwatch.py index 5eeffb979..2eb70c94b 100644 --- a/tests/app/clients/test_aws_cloudwatch.py +++ b/tests/app/clients/test_aws_cloudwatch.py @@ -1,6 +1,7 @@ # import pytest from datetime import datetime +import pytest from flask import current_app from app import aws_cloudwatch_client @@ -54,6 +55,27 @@ def side_effect(filterPattern, logGroupName, startTime, endTime): return {"events": []} +@pytest.mark.parametrize( + "response, notify_id, expected_message", + [ + ( + "Phone has blocked SMS", + "abc", + "\x1b[31mThe phone number for notification_id abc is OPTED OUT. You need to opt back in\x1b[0m", + ), + ( + "Some phone is opted out", + "xyz", + "\x1b[31mThe phone number for notification_id xyz is OPTED OUT. You need to opt back in\x1b[0m", + ), + ("Phone is A-OK", "123", None), + ], +) +def test_warn_if_dev_is_opted_out(response, notify_id, expected_message): + result = aws_cloudwatch_client.warn_if_dev_is_opted_out(response, notify_id) + assert result == expected_message + + def test_check_sms_success(notify_api, mocker): aws_cloudwatch_client.init_app(current_app) boto_mock = mocker.patch.object(aws_cloudwatch_client, "_client", create=True)