Capture Payment Phone numbers #50

sravfeyn · 2024-11-07T15:47:58Z

This implements connect-id side endpoints for capturing users' mobile payment phone numbers. I have tested these locally using curl

Spec

sravfeyn · 2024-11-10T12:04:48Z

Corresponding connect PR dimagi/commcare-connect#428

sravfeyn · 2024-11-12T15:51:33Z

@calellowitz Please have a review of this and the corresponding connect PR that's linked. It will make it easy for mobile to test these.

calellowitz

Left a few questions and a few suggestions

calellowitz · 2024-11-13T22:17:34Z

payments/models.py

+    telecom_provider = models.CharField(max_length=50, blank=True, null=True)
+    # whether the number is verified using OTP
+    is_verified = models.BooleanField(default=False)
+    status = models.CharField(


what does this column do?

This tracks whether this is pending for review or approved or rejected by connect user.

calellowitz · 2024-11-13T22:21:11Z

utils/twilio.py

+from django.conf import settings
+
+# Create the client instance only once
+twilio_client = Client(settings.TWILIO_ACCOUNT_SID, settings.TWILIO_AUTH_TOKEN)


What is the reason to have this be a singleton?

When you instantiate the client, there's bunch of auth overhead API calls. Singleton would avoid that overhead each time we need it

there's bunch of auth overhead API calls

Where do you see this? I looked at the code and don't see any calls on instantiation, and that would be odd behavior, since auth need to be per request anyway.

Thanks for looking that up, it looks like there is no overhead, I thought I read this somewhere but I was wrong. I have removed singleton

calellowitz · 2024-11-13T22:21:33Z

utils/twilio.py

+    try:
+        phone_info = client.lookups.v1.phone_numbers(phone_number).fetch(type="carrier")
+        return phone_info.carrier.get("name")
+    except Exception as e:


This shouldn't catch bare exceptions. Is there a specific error you are looking for?

That's true, I will adjust it such it logs the exception if any. For now, I will catch all exceptions.

Why catch the exceptions and return instead of just letting the exceptions fail loudly? Especially since that will allow them to be picked up by sentry or similar error logging (once that is fully set up). Letting the exception happen also results in it being logged, but would also let the mobile know that it failed.

My reasoning is that hard failing would block user from giving us the payment number at all. If we capture and log the exception and return empty telecom-provider, user's number would still be captured. We can look at the logs and retroactively update the telecom provider after dealing with the exception as we roll out this feature.

If we hardfail, we would have missed user's input and even if we deal with the exception latter, there is no way to go ask user again for payment number. They will have to know that the issue is fixed and then retry.

Though, I agree that this means the partial loss of data integrity. I am hoping to keep an eye on the logs initially, deal with them as they come up and then implement hardfailure latter.

calellowitz · 2024-11-13T22:24:43Z

payments/views.py

+            'status': PaymentProfile.PENDING
+        }
+    )
+    return PhoneDevice.send_otp_httpresponse(phone_number=payment_profile.phone_number, user=payment_profile.user)


We want to send this every time they hit here or only on initial creation (and maybe if the phone number changes)?

We have this single view to manage resend func, so we want to send it each time this view is called.

calellowitz · 2024-11-13T22:27:11Z

payments/models.py

+from users.models import ConnectUser
+
+
+class PaymentProfile(models.Model):


If there can only be one of these per user, what is the reason to have a separate model rather than adding these columns to the existing model (and then avoiding the extra lookups and possible exceptions).

There are too many fields to add on user directly (telecom_provider, otp-verification status, payment validation status).

calellowitz · 2024-11-13T22:32:07Z

payments/views.py

+    return JsonResponse({"success": True})
+
+
+class FetchPhoneNumbers(APIView):


nit: these views should include the "impossible" scopes to prevent access tokens from working. I think it isn't necessary but am not sure enough to avoid adding it for safety.

What do you mean by "impossible" scopes?

calellowitz · 2024-11-13T22:35:52Z

payments/views.py

+            phone_number = data["phone_number"]
+            status = data["status"]
+
+            filter_conditions |= Q(user__username=username, phone_number=phone_number)


since payment profiles are one to one with username why does the phone number need to be in the query? Can't it just be user__usernames__in=<list from request>, and similarly the dict can key just off the username?

Yeah I guess, we could simplify it. Though, one (debatable) advantage of this is that it would guard against the hypothetical scenario of two users using same payment phone number.

How does it guard against that? If we fetch by username, we are guaranteed to get the right number regardless of if it is shared.

Hmm, I have updated it and corresponding api call on connect side to remove phone-numbers

calellowitz · 2024-11-13T22:36:35Z

payments/views.py

+
+        profiles = PaymentProfile.objects.filter(filter_conditions).select_related("user")
+        if len(profiles) != len(users_data):
+            return Response(status=drf_status.HTTP_404_NOT_FOUND)


This throws a 404 if any aren't found? It might be nicer to just return the success list at the end so one bad entry doesn't ruin the whole request.

Ideally, this shouldn't happen since the request is coming from the view which returns valid profiles in the first place. So, it's not worth partially succeeding and then managing partial failure that calls this view.

Ideally, this shouldn't happen

I agree it shouldn't happen, but if it does, as this code implies might happen, is there a reason to fail the whole request, rather than succeed partially? Especially since the users don't choose which numbers are sent (connect does it automatically).

Let me clarify the flow here:

The profiles in this request come from a UI where users select from valid phone numbers that we've already verified exist. The only way this 404 could trigger is if someone manually manipulates the POST parameters.

Given this is an authenticated view and parameters are user-selected from validated options, failing fast on invalid input serves as a security boundary - it indicates either request tampering or data integrity issues that we'd want to catch and log rather than partially process. This is different from cases where users don't control the input parameters.

Does this help explain the reasoning behind?

calellowitz · 2024-11-13T22:37:01Z

payments/views.py

+        }
+        with transaction.atomic():
+            for profile in profiles:
+                key = (profile.user.username, profile.phone_number)


same question about just using username

calellowitz · 2024-11-13T22:40:13Z

payments/views.py

+            "approved": [],
+            "rejected": [],
+        }
+        with transaction.atomic():


what is this guarding against? It only wraps the update at the end, so I am not sure how it could end up rolled back in an unfinished state.

I think I had it when I did it sequentially instead of bulk update, I will remove it.

sravfeyn · 2024-11-20T09:47:28Z

@calellowitz please have a look again.

calellowitz

The changes look good, but I am still curious about some of my previous questions

calellowitz · 2024-11-22T01:49:46Z

payments/views.py

+            'phone_number': phone_number,
+            'telecom_provider': telecom_provider,
+            'is_verified': False,
+            'status': PaymentProfile.PENDING


If the number changes, do we not want to always reset that status value?

Yes, we are re-setting to PENDING whenever the payment data is updated.

Ah sorry, I was confused about the behavior of the defaults dict.

calellowitz · 2024-11-22T01:51:25Z

utils/twilio.py

+    try:
+        phone_info = client.lookups.v1.phone_numbers(phone_number).fetch(type="carrier")
+        return phone_info.carrier.get("name")
+    except Exception as e:


Why catch the exceptions and return instead of just letting the exceptions fail loudly? Especially since that will allow them to be picked up by sentry or similar error logging (once that is fully set up). Letting the exception happen also results in it being logged, but would also let the mobile know that it failed.

sravfeyn · 2024-12-12T11:14:15Z

utils/twilio.py

+        phone_info = client.lookups.v1.phone_numbers(phone_number).fetch(type="carrier")
+        return phone_info.carrier.get("name")
+    except Exception as e:
+        logger.exception("Error occurred during Twilio call for phone number %s: %s", phone_number, str(e))


To fix the problem, we should avoid logging the sensitive phone_number directly. Instead, we can log a masked version of the phone number or omit it entirely from the log message. This way, we still retain useful information for debugging purposes without exposing sensitive data.

The best way to fix this issue is to mask the phone number before logging it. We can replace the middle digits of the phone number with asterisks or another placeholder character. This approach ensures that the phone number is not exposed in clear text while still providing enough information to identify the general format of the number.

We need to modify the logging statement on line 16 to mask the phone number before logging it.

The number is getting intentionally logged to see which numbers are failing

calellowitz · 2024-12-11T03:16:56Z

Still not thrilled with the swallowed errors, but this looks fine once the conflicts are fixed

sravfeyn added 2 commits November 5, 2024 20:36

Prefactor DRY

1abdbca

Capture payment phone numbers

5923600

sravfeyn requested a review from calellowitz November 7, 2024 15:47

send payment valid status message

bb30069

sravfeyn force-pushed the sr/payments branch from f0ac6d2 to bb30069 Compare November 10, 2024 11:16

calellowitz reviewed Nov 14, 2024

View reviewed changes

PR feedback

d9b1316

sravfeyn force-pushed the sr/payments branch from c303395 to f8037f0 Compare November 20, 2024 09:21

Track payment number owner name

6b3377c

sravfeyn force-pushed the sr/payments branch from f8037f0 to 6b3377c Compare November 20, 2024 09:21

calellowitz reviewed Nov 22, 2024

View reviewed changes

no need for singleton

cf6d2f6

github-advanced-security bot found potential problems Nov 22, 2024

View reviewed changes

sravfeyn added 2 commits November 22, 2024 15:28

Remove phone number param from post

97546fd

Use e164 format for phone

efb609e

sravfeyn added 2 commits December 12, 2024 16:38

Merge branch 'main' into sr/payments

107337e

Merge branch 'main' into sr/payments

9dcd237

sravfeyn merged commit 161f4c7 into main Dec 12, 2024
1 of 2 checks passed

@@ -15,3 +15,4 @@
                 except Exception as e:
-                    logger.exception("Error occurred during Twilio call for phone number %s: %s", phone_number, str(e))
+                    masked_phone_number = phone_number[:2] + "****" + phone_number[-2:]
+                    logger.exception("Error occurred during Twilio call for phone number %s: %s", masked_phone_number, str(e))
                     return None

		from users.models import ConnectUser


		class PaymentProfile(models.Model):

		return JsonResponse({"success": True})


		class FetchPhoneNumbers(APIView):

Capture Payment Phone numbers #50

Capture Payment Phone numbers #50

Conversation

sravfeyn commented Nov 7, 2024 • edited Loading

sravfeyn commented Nov 10, 2024

sravfeyn commented Nov 12, 2024

calellowitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calellowitz Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

sravfeyn Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sravfeyn Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sravfeyn commented Nov 20, 2024

calellowitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calellowitz commented Dec 11, 2024

sravfeyn commented Nov 7, 2024 •

edited

Loading

calellowitz Nov 22, 2024 •

edited

Loading

sravfeyn Nov 22, 2024 •

edited

Loading

sravfeyn Nov 14, 2024 •

edited

Loading