Closed jakehova closed 1 year ago
Hey team! Please add your planning poker estimate with Zenhub @cris-oddball @EvanParish @jakehova @k-macmillan @kalbfled @trevor2718
meeting schedule with Dave for Friday, 12/2, to understand how he tested this ticket.
@kalbfled I have a few questions about this after all. We can schedule a time to discuss if they are not easily answered here.
inbound_number
and set self-managed
to true, is it required to then add a url_endpoint
(will the system reject the POST if I do not set a url)?provider
be nulled? Is provider a list of enums or any string?active
indicate?/inbound-number/available
indicate? how is that different from just GET /inbound-number/
- when I make the call in DEV, I just get back an empty array {'data': []}
- if this could be populated with data, would this route also be expected to return the two new fields?inbound_number
with a service_id
?* If I create a new `inbound_number` and set `self-managed` to true, is it required to then add a `url_endpoint `(will the system reject the POST if I do not set a url)?
As implemented, negative. These values are completely independent of each other.
@jakehova Is that the intended logic? If an inbound number is self-managed, should we require the user to provide a url?
* Can `provider` be nulled? Is provider a list of enums or any string?
provider
is a non-nullible string according to models.py. It is not restricted to a list of acceptable values.
* what does `active` indicate?
Active numbers can be assigned to a service.
* what does GET `/inbound-number/available` indicate? how is that different from just GET `/inbound-number/` - when I make the call in DEV, I just get back an empty array `{'data': []}` - if this could be populated with data, would this route also be expected to return the two new fields?
GET /inbound-number
returns a list of all inbound numbers.
GET /inbound-number/available
returns a list of active inbound numbers not associated with any service (service_id is NULL).
* How do I associate an `inbound_number` with a `service_id`?
Looking at what the code implements, you could use POST /inbound-number/<inbound_number_id>
to update an instance's service_id
attribute.
@jakehova says that a URL endpoint should be required whenever the inbound number is self-managed. I will create a new PR for that change.
Useful reference: https://json-schema.org/understanding-json-schema/reference/conditionals.html
PASSES QA
Given that the ticket was to create two new fields in the db, have the existing API endpoints accept them in POST routes and return them in GET routes, and that if a number is set to "self_managed": true
then the client must provide a "url_endpoint", then this ticket passes.
Tested in Perf with all fake numbers assigned to the "11/16 Test" service_id 4e1b252d-1006-414f-a119-049ba9182365 as follows:
Create - Happy Path Test Cases
Create - Negative Test Cases
Update - Happy Path test Cases
Update - Negative Test Cases
Turn Number Off - Happy Path
Read - Happy Path
Notes for possible future work ( @mjones-oddball @kalbfled @k-macmillan - do we want tickets for any of the following?), none of which are blocker or fail this ticket.
Final Note: this could be added to a regression suite except that there is no way via the API to delete numbers (only turn them off, or to "active": false
) then the database could rapidly become polluted.
@k-macmillan the debug log for staging shows the following message, do we need to address this by adding certificate verification? Or will this not be an issue when we use the vetext url_endpoint?
[DEBUG] 2022-12-14T22:18:09.808Z 73954377-cb41-4162-becb-bd9c2bf38cd5 Connecting to https://eou9ebpdvxw3lva.m.pipedream.net/, sending: {'originationNumber': '+18506283352', 'destinationNumber': '+15738792361', 'messageKeyword': 'KEYWORD_171875617347', 'messageBody': 'Bob', 'previousPublishedMessageId': '3p2beq4rrvnll0ul88nu3498ghr34mj8kjp6h9o0', 'inboundMessageId': '9578a03c-3e21-4e9f-8c47-ce34bfaa567d'}
/var/task/urllib3/connectionpool.py:1045: InsecureRequestWarning: Unverified HTTPS request is being made to host 'eou9ebpdvxw3lva.m.pipedream.net'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/1.26.x/advanced-usage.html#ssl-warnings
warnings.warn(
Good catch, this is likely a problem.
verify=False
was set previously for intranet communication within the VA's network and this was probably a copy/paste transfer from elsewhere. Ideally, it is always set to True
(the default), otherwise it is subject to MiTM attacks. We should disable it and see if we're still able to POST
without issue in and out of the VA's network.
It won't work with verify=true because the lambda doesn't have access to a CA.
Get Outlook for Androidhttps://aka.ms/AAb9ysg
From: Kyle MacMillan @.> Sent: Wednesday, December 14, 2022 7:09:23 PM To: department-of-veterans-affairs/notification-api @.> Cc: Jacob @.>; Mention @.> Subject: Re: [department-of-veterans-affairs/notification-api] Two-Way SMS Infrastructure - Update Inbound_Numbers Table (Issue #932)
Good catch, this is likely a problem. verify=False was set previously for intranet communication within the VA's network and this was probably a copy/paste transfer from elsewhere. Ideally, it is always set to True (the default), otherwise it is subject to MiTMhttps://en.wikipedia.org/wiki/Man-in-the-middle_attack attacks. We should disable ithttps://github.com/department-of-veterans-affairs/notification-api/blob/master/lambda_functions/two_way_sms/two_way_sms_v2.py#L274 and see if we're still able to POST without issue in and out of the VA's network.
— Reply to this email directly, view it on GitHubhttps://github.com/department-of-veterans-affairs/notification-api/issues/932#issuecomment-1352386503, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABFARIVY7GTW2F6ZSYRGVITWNJOTHANCNFSM6AAAAAARVIOSOY. You are receiving this because you were mentioned.Message ID: @.***>
Yeah, IIRC the CA is why we failed with intranet. There are other options but sending Veteran texts with verify=False
cannot remain if we have any external links.
Note that when verify is set to False, requests will accept any TLS certificate presented by the server, and will ignore hostname mismatches and/or expired certificates, which will make your application vulnerable to man-in-the-middle (MitM) attacks. Setting verify to False may be useful during local development or testing.
There are other options, we need another solution.
Another possible ticket: Given that the service is set when the sms_sender is configured with the inbound_number_id. If the service is set on the inbound_number prior to configuring the sms_sender then the inbound_number is not available and the service must be removed from the inbound_number, then should we remove the ability to post (create/update) the inbond_number with the service?
All questions above are moved to this Google doc and QA label removed.
Please verify before altering the dataset that inbound_numbers is the applicable table that handles inbound sms for our clients.
POST
for inbound_numbers