endpoints to receive/forward hq invites #51

sravfeyn · 2024-11-11T12:02:33Z

calellowitz · 2024-11-22T01:36:03Z

users/views.py

+        user_token = request.data["user_token"]
+        invite_code = unquote_plus(callback_url)
+        try:
+            response = requests.post(callback_url, data={"invite_code": invite_code, "token": user_token})


Are you planning to address this? Given your answer below about where this points, it should be doable to check it against an allowed list (only hq instances?)

Yes, I have addressed this

calellowitz

Couple small questions

calellowitz · 2024-11-14T01:04:22Z

users/views.py

+    authentication_classes = [ClientProtectedResourceAuth]
+
+    def post(self, request, *args, **kwargs):
+        callback_url = quote_plus(request.data["callback_url"])


where does callback_url come from and point to?

This is all according to the modified spec.

This comes from HQ and it points to HQ's invite confirmation view. It's for actual invite confirmation, when mobile confirms it to ConnectID, ConnectID calls back HQ on callback_url with the user's token.

This is the HQ change dimagi/commcare-hq@407303f

calellowitz · 2024-11-14T01:05:25Z

users/views.py

+
+
+class ConfirmHQInviteCallback(APIView):
+    authentication_classes = [ClientProtectedResourceAuth]


Who makes this call? Is this not from mobile?

Yes, this is made by mobile.

From spec

To accept invitation (when user confirms), call below endpoint

‘connectid.dimagi.com/users/confirm_hq_invite’

POST params
callback_url: callback_url (forwarded in deep link)
invite_code: invite_code (forwarded in deep link)
user_token: user’s active auth token

In that case, this auth is incorrect. It only works with oauth client credentials grants (what we use for connect or other servers). I think the default auth classes should be fine.

You are right! I will remove those classes.

sravfeyn · 2024-11-20T09:55:29Z

@calellowitz Please let me know if this is good to go.

calellowitz

Left a few questions on the new code in addition to the previous comments

calellowitz · 2024-11-22T01:38:01Z

connectid/views.py

@@ -28,3 +28,10 @@ def assetlinks_json(request):
        },
    ]
    return JsonResponse(assetfile, safe=False)
+
+
+def deeplink(request, subpath):


Are we able to direct them directly to the play store? That is what we discussed on the call and seems to make the most sense if it is possible. Mobile seemed to believe that it would be.

Right, I have updated it to redirect to the app.

calellowitz · 2024-11-22T01:40:16Z

users/views.py

@@ -434,7 +434,8 @@ def post(self, request, *args, **kwargs):
            # We don't want to make this a user lookup service
            # So fake a success message
            return JsonResponse({"success": True})
-        deeplink = f"https://connectid.dimagi.com/hq_invite/{callback_url}/{username}/{invite_code}/{user_domain}"
+        details = f"{callback_url}/{username}/{invite_code}/{user_domain}/{user.username}"


I think we spoke about this a while ago, but why is this encoded as a path rather than using query params (which libraries on web and mobile could parse much more easily)?

Yeah, I think there was some issue regarding formatting this using query params on mobile, this format was specified by mobile.

I spoke to @OrangeAndGreen and he seemed to believe query params would be better for them as well. Let's offline on slack about this with a mobile dev.

Okay, I have updated this to use queryparams.

sravfeyn · 2024-11-22T07:43:23Z

@calellowitz Thanks for the review, I have followed up.

calellowitz · 2024-11-24T22:27:14Z

connectid/views.py

-    #   and the URL is opened in the browser, ask the
-    #   user to install the app and try again
-    return HttpResponse("Please install CommCare and re-open this link to accept invite")
+    html_content = """


Why inline HTML instead of using a separate file

I have moved it to its own dir.

calellowitz · 2024-11-24T22:28:27Z

connectid/settings.py

@@ -80,7 +80,10 @@
 WSGI_APPLICATION = 'connectid.wsgi.application'


-
+TRUSTED_COMMCAREHQ_HOSTS = [
+    "commcarehq.org",


how does this work? Specifically, if commcarehq.org is enough to catch www.commcarehq.org I would expect it to also catch staging, and if we need to be more specific, I would expect www to be required.

Good point, I will add www to the list to be specific.

calellowitz · 2024-11-24T22:32:35Z

users/views.py

@@ -434,7 +434,8 @@ def post(self, request, *args, **kwargs):
            # We don't want to make this a user lookup service
            # So fake a success message
            return JsonResponse({"success": True})
-        deeplink = f"https://connectid.dimagi.com/hq_invite/{callback_url}/{username}/{invite_code}/{user_domain}"
+        details = f"{callback_url}/{username}/{invite_code}/{user_domain}/{user.username}"


I spoke to @OrangeAndGreen and he seemed to believe query params would be better for them as well. Let's offline on slack about this with a mobile dev.

calellowitz

Couple small questions

sravfeyn · 2024-11-27T12:53:02Z

@calellowitz I have addressed your feedback, please have a look. I think the CodeQL failure is incorrect as it is not able to detect the trusted URL check that is added.

sravfeyn · 2024-11-28T15:15:17Z

@calellowitz Bumping for review please.

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

Thanks @calellowitz please also review the corresponding HQ PR https://github.com/dimagi/commcare-hq/pull/34967/files#diff-dc2c79b556e950acc7dea90175fd5febba22bf9fa4faaba1b61c3b3d5acdc9e9

sravfeyn requested a review from calellowitz November 11, 2024 12:02

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

View reviewed changes

endpoints to receive/forward hq invites

8464e3c

sravfeyn force-pushed the sr/hqusers branch from 88e406b to 8464e3c Compare November 12, 2024 14:36

calellowitz reviewed Nov 14, 2024

View reviewed changes

Merge branch 'main' into sr/hqusers

f3b298b

handle deeplink fallback; pass username in URL

02f2e20

calellowitz requested changes Nov 22, 2024

View reviewed changes

Check trusted hosts

19d464c

calellowitz reviewed Nov 24, 2024

View reviewed changes

Use urlencode for deeplink

0af8cf1

calellowitz approved these changes Dec 11, 2024

View reviewed changes

sravfeyn merged commit ef65275 into main Dec 11, 2024
1 of 2 checks passed



		class ConfirmHQInviteCallback(APIView):
		authentication_classes = [ClientProtectedResourceAuth]

endpoints to receive/forward hq invites #51

endpoints to receive/forward hq invites #51

Uh oh!

Conversation

sravfeyn commented Nov 11, 2024

Uh oh!

Check failure

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calellowitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

From spec

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sravfeyn commented Nov 20, 2024

Uh oh!

calellowitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sravfeyn commented Nov 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calellowitz left a comment

Choose a reason for hiding this comment

Uh oh!

sravfeyn commented Nov 27, 2024

Uh oh!

sravfeyn commented Nov 28, 2024

Uh oh!

Uh oh!

sravfeyn commented Dec 11, 2024

Uh oh!

Uh oh!