-
-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ConnectID: Messaging Consent and Key #35212
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few changes, but I think this is close
|
||
|
||
class ConnectIDMessagingKey(models.Model): | ||
domain = models.TextField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary since the user_link
has a domain field already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have an active
boolean as well, so we rotate out keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the domain here to support the domain deletion test. It was complaining before. I will add the active
boolean here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. 7d02e73
def connectid_messaging_key(request, domain): | ||
link = get_object_or_404(ConnectIDUserLink, commcare_user=request.user, domain=request.domain) | ||
key = generate_aes_key().decode("utf-8") | ||
messaging_key = ConnectIDMessagingKey.objects.create(connectid_user_link=link, domain=request.domain, key=key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be get_or_create
so we don't generate a new one each time.
@@ -1685,6 +1687,25 @@ def link_connectid_user(request, domain): | |||
return HttpResponse() | |||
|
|||
|
|||
@csrf_exempt | |||
@login_or_basic_ex(allow_cc_users=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to allow auth via connectid token, which this decorator can't do. We might need a new decorator that looks up the user via the connectid token (and validates it with connectid), or to just do it in the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. 9c3e5f6
@csrf_exempt | ||
@require_POST | ||
@login_or_basic_ex(allow_cc_users=True) | ||
def update_connectid_messaging_consent(request, domain): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will come from the connectid server, not a specific user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. b9bc775
@@ -3282,6 +3282,14 @@ class ConnectIDUserLink(models.Model): | |||
connectid_username = models.TextField() | |||
commcare_user = models.ForeignKey(User, related_name='connectid_user', on_delete=models.CASCADE) | |||
domain = models.TextField() | |||
messaging_consent = models.BooleanField(default=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a channel_id
as well, since that is how incoming requests will be able to show which channel they go to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. 7d02e73
a7f19b9
to
b9bc775
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies this review is so late, but I think we want these URLs to be global for HQ, rather than domain specific. I think they can go in the smsbackend app I will add, so no need to fix them up here. When I push my code, we can move them. Otherwise looks good
_, token = auth_header.split(" ") | ||
if not token: | ||
return HttpResponseBadRequest("ConnectID Token Required") | ||
username = get_connectid_userinfo(token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to DRY this with ConnectIDAuthBackend
Product Description
No user-facing changes.
Technical Summary
This PR adds functionality to manage consent and keys required for messaging functionality provided by ConnectID.
Ticket
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Migrations
Rollback instructions
Labels & Review