Add ConnectID Invite/De-Link/Re-Link features #34967

sravfeyn · 2024-08-12T11:34:35Z

Product Description

Technical Summary

This provides UI actions to provision users using ConnectID auth backend implemented previously.

Feature Flag

COMMCARE_CONNECT

Safety Assurance

Safety story

I have tested this locally and it's within a feature flag

Automated test coverage

QA Plan

Todo

Migrations

The migrations in this code can be safely applied first independently of the code

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

sravfeyn · 2024-08-12T11:40:39Z

corehq/apps/users/views/mobile/users.py

+        messages.error(request, "The user is already confirmed or is not a mobile user")
+        return HttpResponse(status=400)
+
+    invite_code = encrypt_account_confirmation_info(user)


Todo; I will need to coordinate with mobile to get a link that opens ConnectID app with the invite intent.

One other alternative is to send this as push notification (probably via the connectid server). That way the app is already handling it, and we know they are logged in. It will also allow us to hide all the info that doesn't matter to the user (like the code or username, which they won't be familiar with), since we can pass that in the data params.

Yeah, definitely push notification would be better.

I remember Dave mentioning this specifically needed an SMS deeplink (I don't recall why). I will check with him and see if we can do push notification (on top of SMS).

calellowitz

general idea looks good. added a few questions and suggestions

calellowitz · 2024-08-30T21:57:07Z

corehq/apps/users/forms.py

@@ -832,10 +840,15 @@ def __init__(self, project, request_user, *args, **kwargs):
                data_bind='value: send_account_confirmation_email',
            )

-        if TWO_STAGE_USER_PROVISIONING_BY_SMS.enabled(self.domain):
+        # cid => connect-id


nit: might be better to just have connect_id in the variable than use cid and add a comment to explain it

calellowitz · 2024-08-30T22:03:25Z

corehq/apps/users/views/mobile/users.py

-        ) or toggles.TWO_STAGE_USER_PROVISIONING_BY_SMS.enabled(self.domain)
+        return (toggles.TWO_STAGE_USER_PROVISIONING.enabled(self.domain)
+            or toggles.TWO_STAGE_USER_PROVISIONING_BY_SMS.enabled(self.domain)
+            or toggles.COMMCARE_CONNECT.enabled(self.domain))


Does this require stage confirmation for connect domains or just allow it?

This is a context variable that's used in the template for things that are common for all two-stage provision workflows.

calellowitz · 2024-08-30T22:04:49Z

corehq/apps/users/views/mobile/users.py

+@require_can_edit_commcare_users
+@require_POST
+@location_safe
+def activate_connectid_link(request, domain, user_id):


nit: if it is a POST anyway these views can be combined and just pass the is_active value in the payload, but definitely not blocking and fine to keep it this way

True. But there is a tradeoff on JS side. The corresponding JS here which is also used for regular (de/)activate needs to handle the POST param for connectid separately. So it's not much worth.

calellowitz · 2024-08-30T22:05:36Z

corehq/apps/users/views/mobile/users.py

+
+def _toggle_connectid_link(request, domain, user_id, is_active):
+    if not toggles.COMMCARE_CONNECT.enabled(domain):
+        return HttpResponseBadRequest()


nit: i think this should be a 403 and can be added as a permission check to the views (the request isn't bad, they just aren't allowed to do it)

calellowitz · 2024-08-30T22:07:15Z

corehq/apps/users/views/mobile/users.py

+def _toggle_connectid_link(request, domain, user_id, is_active):
+    if not toggles.COMMCARE_CONNECT.enabled(domain):
+        return HttpResponseBadRequest()
+    user = CommCareUser.get_by_user_id(user_id, domain)


is it possible to pass the username in the view so that we can fetch the django user directly rather than the couch user and then the django user?

The tradeoff is that we lookup and send django user IDs for all users in the paginate_mobile_workers so that the caller knows the django userid to pass it to this view.

calellowitz · 2024-09-01T17:51:08Z

corehq/apps/users/views/mobile/users.py

+            commcare_user__username__in=usernames
+        ).annotate(commcare_username=F("commcare_user__username")).all()
+        return {
+            link.commcare_username.split("@")[0]: link


do we have a util for this somewhere? I realize its a small thing, but we do it so much in HQ that I am surprised we dont have a function for it.

Yeah, I thought the same, I couldn't find either. Most places do this same thing, and perhaps it wasn't worth a util that has just this one line in it.

corehq.apps.users.util.raw_username

calellowitz · 2024-09-01T18:16:24Z

corehq/apps/users/views/mobile/users.py

+    domain: {domain}
+    invite_code: {invite_code}
+    url: {url}
+    Make a POST call to above url with invite_code and your ConnectID token


I don't think we want to send the user a message telling them to make a POST, because they won't know what to do with that info. I think we want to pass the right info to the phone so it can do that automatically (or have a clickable link that will confirm it, but a link that is clickable in the sms but doesn't do the right thing is going to be confusing)

Yeah, definitely, this is a ToDo; mobile team has given me the correct format, so I will update this.

calellowitz · 2024-09-01T18:17:04Z

corehq/apps/users/views/mobile/users.py

+        messages.error(request, "The user is already confirmed or is not a mobile user")
+        return HttpResponse(status=400)
+
+    invite_code = encrypt_account_confirmation_info(user)


One other alternative is to send this as push notification (probably via the connectid server). That way the app is already handling it, and we know they are logged in. It will also allow us to hide all the info that doesn't matter to the user (like the code or username, which they won't be familiar with), since we can pass that in the data params.

calellowitz · 2024-09-01T18:18:46Z

corehq/apps/users/views/mobile/users.py

+        messages.error(request, "The user is already confirmed or is not a mobile user")
+        return HttpResponse(status=400)
+
+    invite_code = encrypt_account_confirmation_info(user)


what does encrypting the invite code do for us here?

I think the code 'encodes' the invitation sent time, so that before confirming it can check the invite didn't expire. This is really a soft enforcement used in the other two multi-stage user provision workflows, so I took the pattern as it is.

calellowitz · 2024-09-01T18:19:48Z

corehq/apps/users/views/mobile/users.py

+    if user.domain != domain:
+        return HttpResponseBadRequest("Invalid Request")
+
+    connectid_username = get_connectid_userinfo(token)


Does this confirm that the token was valid at all? What response do we send if it wasn't?

If the token is not valid this would not return a username. So right now, this would 500 if the token is not valid. Which is same behaviour as that of link_connectid_user.

sravfeyn · 2024-11-28T15:14:44Z

@calellowitz Bumping for review please

sravfeyn · 2024-12-11T09:37:55Z

@calellowitz Bumping for review. This, as well as the cchq one dimagi/commcare-cloud#6449

calellowitz

Are you planning to address the lint errors. Code generally looks fine, but that is generally worth doing. Otherwise one question (I see that one of them was corrected in a later commit). Otherwise, I would feel a bit more comfortable if someone outside connect reviewed this, since I am a bit rusty on hq code, but looks good to me.

calellowitz · 2024-12-13T00:29:13Z

corehq/apps/users/views/mobile/users.py

+
+def send_connectid_invite_sms(user):
+    if user.is_account_confirmed or not user.is_commcare_user():
+        messages.error(request, "The user is already confirmed or is not a mobile user")


This lint error looks real

calellowitz · 2024-12-13T00:31:28Z

corehq/apps/users/views/mobile/users.py

    # ConnectID server delivers the invite to user
    if user.is_account_confirmed or not user.is_commcare_user():
-        messages.error(request, "The user is already confirmed or is not a mobile user")
+        if request:
+            messages.error(request, "The user is already confirmed or is not a mobile user")


Do we not want these messages for the import results?

sravfeyn added 2 commits August 12, 2024 08:01

Invite mobile workers using ConnectID phone number

55c8233

Add Delink/relink

6c180df

sravfeyn requested a review from esoergel as a code owner August 12, 2024 11:34

dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Aug 12, 2024

sravfeyn requested a review from calellowitz August 12, 2024 11:34

sravfeyn commented Aug 12, 2024

View reviewed changes

calellowitz reviewed Sep 1, 2024

View reviewed changes

Merge master; add migrations file

80a6157

sravfeyn force-pushed the sr/connectid branch from 4e2231e to 80a6157 Compare October 25, 2024 05:47

sravfeyn added 3 commits October 25, 2024 14:20

format the deeplink

1f3e480

Fix connectid sms

cf7b51f

Use ConnectID to process user invites

407303f

sravfeyn force-pushed the sr/connectid branch from 431d542 to 407303f Compare November 20, 2024 05:31

sravfeyn added 3 commits November 27, 2024 18:16

remove urlparam decoding

c7bd88e

use raw_username util

def8ce9

Bulk connectid invite flag

f0b60a1

sravfeyn requested a review from stephherbers as a code owner November 28, 2024 15:10

dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Nov 28, 2024

Merge master

c0a2f49

sravfeyn mentioned this pull request Dec 11, 2024

Add connectid user invite setting dimagi/commcare-cloud#6449

Open

1 task

Merge master

5b496a5

sravfeyn force-pushed the sr/connectid branch from c899c82 to 5b496a5 Compare December 11, 2024 09:37

sravfeyn added 3 commits December 12, 2024 10:45

Merge branch 'master' into sr/connectid

b7d6dd8

remove unintended check

7150aaf

format phone number

3f285b1

calellowitz approved these changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ConnectID Invite/De-Link/Re-Link features #34967

Add ConnectID Invite/De-Link/Re-Link features #34967

sravfeyn commented Aug 12, 2024 •

edited

Loading

sravfeyn Aug 12, 2024

calellowitz Sep 1, 2024

sravfeyn Oct 24, 2024

calellowitz left a comment

calellowitz Aug 30, 2024

calellowitz Aug 30, 2024

sravfeyn Oct 24, 2024

calellowitz Aug 30, 2024

sravfeyn Oct 24, 2024

calellowitz Aug 30, 2024

calellowitz Aug 30, 2024

sravfeyn Oct 24, 2024

calellowitz Sep 1, 2024

sravfeyn Oct 24, 2024

snopoke Nov 28, 2024

calellowitz Sep 1, 2024

sravfeyn Oct 24, 2024

calellowitz Sep 1, 2024

calellowitz Sep 1, 2024

sravfeyn Oct 24, 2024

calellowitz Sep 1, 2024

sravfeyn Oct 24, 2024

sravfeyn commented Nov 28, 2024

sravfeyn commented Dec 11, 2024

calellowitz left a comment

calellowitz Dec 13, 2024

calellowitz Dec 13, 2024

Add ConnectID Invite/De-Link/Re-Link features #34967

Are you sure you want to change the base?

Add ConnectID Invite/De-Link/Re-Link features #34967

Conversation

sravfeyn commented Aug 12, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

Rollback instructions

Labels & Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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 28, 2024

sravfeyn commented Dec 11, 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

sravfeyn commented Aug 12, 2024 •

edited

Loading