phonenumbers.PhoneNumberMatcher does not correctly parse a phone number

[kalbfled]

The phone number +17553927664 is not correctly being parsed, which is causing errors downstream in notification-api. The problem seems to be that phonenumbers.PhoneNumberMatcher, which is called here, does not work as expected.

I opened a ticket in the python-phonenumbers repo, and the maintainer closed it with a useful comment. Please read the details there.

Possible mitigations:

1. Address this by using phonenumbers.parse instead. This parses the 755 phone number as expected, but making the change causes a cascade of unit test failures. I'm not sure if the logic needs to change further or if the unit tests need to change.
2. Proceed using the "leniency" argument to PhoneNumberMatcher as provided by the maintainer of python-phonenumbers. This causes 8 unit test failures.
3. Directly validate the phone number in notification-api, and do not rely on this repo.

---

• update utils version
• update error message/logging to ensure we accurately define the issue that is being reported
• summarize findings in a comment (this was found to not be a bug)
• QA validation

[kalbfled]

@k-macmillan The python-phonenumbers maintainer gave me helpful information. Now I'm thinking we should go with options 2 or 3 (as enumerated above). The downside to 2 is that I'm not entirely certain what the behavior should be, so I'm unsure if I should address failing unit tests by changing the functionality or changing the tests.

[kalbfled]

Concerning Option 3, here are the affected files in notifications-api:

.../notification-api$ grep -r notifications_utils\.recipients .
./tests/app/celery/test_provider_tasks.py:from notifications_utils.recipients import InvalidEmailError
./tests/app/delivery/test_send_to_providers.py:from notifications_utils.recipients import validate_and_format_phone_number
./tests/app/notifications/test_process_notifications.py:from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address
./tests/app/service/test_send_one_off_notification.py:from notifications_utils.recipients import InvalidPhoneError
./tests/app/clients/test_aws_ses.py:from notifications_utils.recipients import InvalidEmailError
./tests/app/clients/test_govdelivery_client.py:from notifications_utils.recipients import InvalidEmailError
./app/v2/errors.py:from notifications_utils.recipients import InvalidEmailError
./app/v2/notifications/post_notifications.py:from notifications_utils.recipients import try_validate_and_format_phone_number
./app/celery/provider_tasks.py:from notifications_utils.recipients import InvalidEmailError
./app/celery/tasks.py:from notifications_utils.recipients import (
./app/dao/notifications_dao.py:from notifications_utils.recipients import (
./app/schemas.py:from notifications_utils.recipients import (
./app/inbound_sms/rest.py:from notifications_utils.recipients import try_validate_and_format_phone_number
./app/delivery/send_to_providers.py:from notifications_utils.recipients import (
./app/errors.py:from notifications_utils.recipients import InvalidEmailError
./app/notifications/validators.py:from notifications_utils.recipients import (
./app/notifications/rest.py:from notifications_utils.recipients import get_international_phone_info
./app/notifications/receive_notifications.py:from notifications_utils.recipients import try_validate_and_format_phone_number
./app/notifications/process_notifications.py:from notifications_utils.recipients import (
./app/service/utils.py:from notifications_utils.recipients import allowed_to_send_to
./app/clients/email/aws_ses.py:from notifications_utils.recipients import InvalidEmailError
./app/clients/email/govdelivery_client.py:from notifications_utils.recipients import InvalidEmailError
./app/models.py:from notifications_utils.recipients import (
./app/schema_validation/__init__.py:from notifications_utils.recipients import (validate_phone_number, validate_email_address, InvalidPhoneError,

I think my first choice is to write a short phone number validation function in notification-api/app/utils.py and remove the dependency on notifications_utils.recipients from notifications-api.

[k-macmillan]

I think we do need to roll our own. In this case we have several options. We can specify a list of characters to allow, using list comprehension so it is formatted. We could use re.match() and match.group(0). I'm sure there are others.

We need to be as forgiving as possible. So if they give us all the numbers we rip out everything else and ensure there's a + at the front.

[cris-oddball]

The number in question appears to not be a valid number, however (there is no 392 prefix in that area code). The benefit of using a library like this one is that it actually checks if the number is valid (assigned to an exchange) and not just if the number is possible (formatted as expected with the correct number of digits). I'd go with option 2 here, and fix the 8 unit tests.

[mjones-oddball]

1. Since this isn't a bug and is working as intended (invalid number returns an error that it is invalid), there is no additional "Fix" needed to validate appropriately. We need a summary on this ticket explaining the investigation and findings please @kalbfled
2. @k-macmillan please contact VEText to let them know the findings
3. Remaining work on this ticket is just to update the error message and logging to be more clear/appropriate. @kalbfled please consult with @k-macmillan to determine what that is.
4. Once this is done, we should validate with QA - @cris-oddball

Removing high priority as it is no longer a functional bug

[kalbfled]

phonenumbers.PhoneNumberMatcher calls is_possible_number and is_valid_number. The latter is what is raising errors for numbers like +17553927664. This is the correct functionality, but the error messages, including the one about international numbers, are misleading. The remaining work for this ticket should be to:

1. Update the error messages in recipients.py (linked in the issue description) from the master branch. Do not create a new release from the master branch.
2. Create a new branch from the 1.0.67 release of notification-utils, and cherry-pick the squashed commit for (1) into this new branch to create release 1.0.68.
3. Update requirements-app.txt for notification-api to depend on notification-utils v1.0.68.

[k-macmillan]

VeText was notified yesterday regarding the findings.

Additional Info for those working this:
You can see the validation error by going to the correct log group and searching for
ValidationError -"email" -"raise"

[ianperera]

#109
department-of-veterans-affairs/notification-api#1045

[k-macmillan]

An example of what the new return message could look like:

The phone_number field contains an invalid number due to either formatting or an impossible combination of area code and/or telephone prefix.

That way we don't have to really change any code or add logic.

[ianperera]

@k-macmillan, can you review, as I've made the changes you requested.

[k-macmillan]

Reviewed your draft @ianperera . Please remember to ping in slack.

[ianperera]

Deploy to dev and tested

[ianperera]

{ "errors": [ { "error": "ValidationError", "message": "phone_number Field contains an invalid number due to either formatting or an impossible combination of area code and/or telephone prefix." } ], "status_code": 400 }

[cris-oddball]

QA PASSES in PERF

• Happy Path send to myself with valid phone, expect 201 and response from POST, also expect delivery status from /notifications

Payload - for all future tests, only the phone_number changes

{
    "template_id": "{{sms-template-id}}",
    "phone_number": "+17192445254",
    "sms_sender_id": "40c9e594-6f5d-461f-bc4a-382be1837b1c"
}

201 response from POST route

{
    "billing_code": null,
    "content": {
        "body": "test",
        "from_number": "+16075362872"
    },
    "id": "5264fbd4-d6c0-43a3-a543-5cc23a839f45",
    "reference": null,
    "scheduled_for": null,
    "template": {
        "id": "5ded3992-e30d-4360-9f77-8a1a644dd4a4",
        "uri": "https://sandbox-api.va.gov/services/d6aa2c68-a2d9-4437-ab19-3ae8eb202553/templates/5ded3992-e30d-4360-9f77-8a1a644dd4a4",
        "version": 1
    },
    "uri": "https://sandbox-api.va.gov/v2/notifications/5264fbd4-d6c0-43a3-a543-5cc23a839f45"
}

received text and in GET Notifications, the status updated to delivered.

• Negative test, incorrect prefix "08515111111", 400
  Field contains an invalid number due to either formatting or an impossible combination of area code and/or telephone prefix.
  400 response

{
    "errors": [
        {
            "error": "ValidationError",
            "message": "phone_number Field contains an invalid number due to either formatting or an impossible combination of area code and/or telephone prefix."
        }
    ],
    "status_code": 400
}

• Negative test, formatting "07515111*11", 400
  Field contains an invalid number due to either formatting or an impossible combination of area code and/or telephone prefix.
  (see above response)

• Negative test, formatting "+171924F5254", 400
  Field contains an invalid number due to either formatting or an impossible combination of area code and/or telephone prefix.
  (see above response)

• Negative test, invalid ac and prefix "17553927664", 400
  Field contains an invalid number due to either formatting or an impossible combination of area code and/or telephone prefix.
  (see above response)

• Negative test, not enough digits "12345", 400
  Field contains an invalid number due to either formatting or an impossible combination of area code and/or telephone prefix.
  (see above response)

• Negative test, empty string "", 400
  Invalid phone number: Not a valid number

{
    "errors": [
        {
            "error": "ValidationError",
            "message": "phone_number Not a valid number"
        }
    ],
    "status_code": 400
}

• Negative test, all text "invalidNumber", 400
  Invalid phone number: Not a valid number
  (see above response)

• Negative test, sent as number not string 12345, 400
  is not of type string

{
    "errors": [
        {
            "error": "ValidationError",
            "message": "phone_number 17192445254 is not of type string"
        }
    ],
    "status_code": 400
}

Logs showing the WARNING status