Task to auto deactivate removed sso users #34151

jingcheng16 · 2024-02-20T21:08:05Z

Product Description

Technical Summary

Ticket: https://dimagi.atlassian.net/browse/SAAS-15233
Tech spec: https://docs.google.com/document/d/1LOF1lCl3wTsEQkEEsHtZi3u4N6kvjXaLv-Lu6e9aYjo/edit#heading=h.ll7lzqwsaw9w

This task will iterate through all IdentityProvider objects where enable_user_deactivation is True. For now we will also filter by idp_type equal to IdentityProviderType.AZURE_AD.

This task will query the Microsoft Graph Users API to fetch a list of all users that are members of the Azure AD Tenant that uses SSO to grant access to CommCare HQ. This task will fetch a list of usernames (email addresses) based on the userPrincipalName field returned by the Microsoft Graph Users API. It will then fetch a list of active WebUser usernames that are members of project spaces associated with the BillingAccount owner of the IdentityProvider. This task will then compare the two lists.

If a user is present in the list of active Web Users on HQ but is not present in the list returned by the Microsoft API query, we will deactivate that user by setting the is_active field on the WebUser to False. Any users that are in the “SSO Exempt” users list should not be automatically deactivated.

It is essential to handle errors in this task in a way that communicates issues swiftly both to the support team and the enterprise admins. For instance, if we are unable to access the Microsoft Graph API, we should raise the error to Sentry and send an email to the enterprise admins that we are having difficulty connecting with the Microsoft Graph API.

Additionally, if the Graph Users API returns an empty list of users, we should consider that an errored state and hold off from automatically deactivating Web Users if the number of active Web Users (that are not SSO Exempt) is greater than 3. This error should also be raised in an email to the enterprise admins alerting that we have temporarily skipped automatic deactivation of Web Users because we are receiving an empty list of Users from Azure AD.

Feature Flag

Safety Assurance

Safety story

I realized this task can be dangerous, we want to make sure we won't accidentally deactivate users that should not be deactivated. I wrote tests for the task, but did I catch all the edge cases?

Automated test coverage

corehq.apps.sso.tests.test_tasks:TestAutoDeactivationTask

QA Plan

No QA at this stage

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

corehq/apps/sso/models.py

biyeun · 2024-02-26T16:56:30Z

corehq/apps/sso/tasks.py

+                body,
+            )
+        else:
+            users_to_deactivate = [user for user in web_user_in_account if user not in idp_users


what about users that are members of the domains but do not have an email domain controlled by the Identity Provider?

for instance, what if your dimagi user was a member of the domain?

I would like to see tests written for this scenario too :)

Hi Biyeun, thank you for raising this important point! Fixed here: fc918be

biyeun · 2024-02-26T16:56:53Z

requirements/sso-requirements.in

@@ -1,3 +1,4 @@
 pyOpenSSL>=23.2.0  # for cryptography>=41
 cryptography>=41   # for security vulnerability
 python3-saml
+msal


Suggested change

msal

msal

new line at end of file nit

Thank you Biyeun! Fixed here: a86ca6c (I should just commit suggestion)

millerdev · 2024-02-26T19:50:56Z

requirements/docs-requirements.in

@@ -7,5 +7,6 @@ sphinxcontrib-django
 sphinx-rtd-theme

 pyOpenSSL # CI builds fail without OpenSSL
+msal # CI builds fail without OpenSSL


I think this should be added in base-requirements.in and not here. This is not a docs requirement. Also, the comment seems incorrect.

Alternately move the import in models.py into get_all_members_of_the_idp so it does not need to be added here.

Thank you Daniel! I go with your second option : 11b1160

Co-authored-by: Biyeun <bbuczyk@dimagi.com>

biyeun · 2024-03-01T16:01:37Z

corehq/apps/sso/tasks.py

+                # If the user is not returned by IdP and is not exempted from SSO
+                if user not in idp_users and user not in exempt_usernames:
+                    email_domain = get_email_domain_from_username(user)
+                    authenticated_email_domains = authenticated_domains.values_list('email_domain', flat=True)


does this need to happen within the loop? this only needs to be set once, yes?

Yes. I moved it: da9df8e

corehq/apps/sso/models.py

Co-authored-by: Biyeun <bbuczyk@dimagi.com>

biyeun · 2024-03-04T20:36:03Z

corehq/apps/sso/models.py

+
+            # Send email
+            subject = _("Issue Connecting to Microsoft Graph API")
+            body = f'There was an issue connecting to the Microsoft Graph API: {str(e)}'


we should have a similar template like the email sent here

It should read something like...

Dear enterprise administrator, There was an issue connecting to the Microsoft Graph API when running a task to deactivate SSO Web Users on CommCare HQ that have been removed from your Microsoft Entra ID Enterprise Application. This was the error message: {{ error }} Please check the configuration of the Remote User Management settings in your Identity Provider. If you have any questions, please don’t hesitate to contact {{ contact_email }}. Thank you for your use and support of CommCare. Best regards, The CommCare HQ Team

Additionally, the send email portion of this task should probably be its own utility. likewise with the similar suggestion below

Thank you Biyeun. I improved it here: f2ed995... I made three mistakes and fixed in 9d29c36 , 9fdd9f1 and bbf62ee

corehq/apps/sso/tasks.py

biyeun · 2024-03-04T20:40:03Z

corehq/apps/sso/tasks.py

+            # Send email
+            subject = _("Temporarily skipped automatic deactivation of Web Users")
+            recipient = idp.owner.enterprise_admin_emails
+            body = _("we have temporarily skipped automatic deactivation of Web Users because we are receiving "


Similar to the previous suggestion for updating the body text in get_all_members_of_the_idp

Dear enterprise administrator, We have temporarily skipped automatic deactivate of SSO Web Users because we are receiving an empty list of users from your Microsoft Entra ID instance. Please check the configuration of the Remote User Management settings in your Identity Provider. If you have any questions, or if this issue persists, please don’t hesitate to contact {{ contact_email }}. Thank you for your use and support of CommCare. Best regards, The CommCare HQ Team

this email bit can also be its own utility abstracted away

Thank you Biyeun. Improved in 9128522

corehq/apps/sso/models.py

corehq/apps/sso/utils/context_helpers.py

corehq/apps/sso/utils/entra.py

Co-authored-by: Graham Herceg <gherceg@dimagi.com>

corehq/apps/sso/tests/test_tasks.py

biyeun

great! thank you! 🎉

gherceg

Thanks for your patience on this one Jing. Great job!

Co-authored-by: Biyeun <bbuczyk@dimagi.com>

The result returned looks like this: {'error': 'invalid_client', 'error_description': "AADSTS7000215: Invalid client secret provided. Ensure the secret being sent in the request is the client secret value, not the client secret ID, for a secret added to app 'f09df325-2ca9-4756-93d4-14934194ae35'. Trace ID: fa552a4f-bb3b-45e0-8432-792489cf1e00 Correlation ID: 379dcf42-fc82-4a36-bce0-d8823be8046b Timestamp: 2024-04-17 14:26:07Z", 'error_codes': [7000215], 'timestamp': '2024-04-17 14:26:07Z', 'trace_id': 'fa552a4f-bb3b-45e0-8432-792489cf1e00', 'correlation_id': '379dcf42-fc82-4a36-bce0-d8823be8046b', 'error_uri': 'https://login.microsoftonline.com/error?code=7000215'}

The batch_result will look like: {'responses': [{'id': '3', 'status': 403, 'headers': {'Cache-Control': 'no-cache', 'x-ms-resource-unit': '1', 'Content-Type': 'application/json'}, 'body': {'error': {'code': 'Authorization_RequestDenied', 'message': 'Insufficient privileges to complete the operation.', 'innerError': {'date': '2024-04-24T15:36:38', 'request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd', 'client-request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd'}}}}, {'id': '1', 'status': 403, 'headers': {'Cache-Control': 'no-cache', 'x-ms-resource-unit': '1', 'Content-Type': 'application/json'}, 'body': {'error': {'code': 'Authorization_RequestDenied', 'message': 'Insufficient privileges to complete the operation.', 'innerError': {'date': '2024-04-24T15:36:38', 'request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd', 'client-request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd'}}}}, {'id': '2', 'status': 403, 'headers': {'Cache-Control': 'no-cache', 'x-ms-resource-unit': '1', 'Content-Type': 'application/json'}, 'body': {'error': {'code': 'Authorization_RequestDenied', 'message': 'Insufficient privileges to complete the operation.', 'innerError': {'date': '2024-04-24T15:36:38', 'request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd', 'client-request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd'}}}}, {'id': '4', 'status': 403, 'headers': {'Cache-Control': 'no-cache', 'x-ms-resource-unit': '1', 'Content-Type': 'application/json'}, 'body': {'error': {'code': 'Authorization_RequestDenied', 'message': 'Insufficient privileges to complete the operation.', 'innerError': {'date': '2024-04-24T15:36:38', 'request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd', 'client-request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd'}}}}, {'id': '0', 'status': 403, 'headers': {'Cache-Control': 'no-cache', 'x-ms-resource-unit': '1', 'Content-Type': 'application/json'}, 'body': {'error': {'code': 'Authorization_RequestDenied', 'message': 'Insufficient privileges to complete the operation.', 'innerError': {'date': '2024-04-24T15:36:38', 'request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd', 'client-request-id': '30dbadf0-2abc-42aa-b838-280a99a477dd'}}}}]}

biyeun

retracting approval.
The general error escape shouldn't be sent as an email. those errors we want to use notify_exception and not send the error message raw to the user

biyeun · 2024-04-24T16:49:03Z

corehq/apps/sso/tasks.py

+            notify_exception(None, f"Failed to get members of the IdP. {str(e)}")
+            send_deactivation_skipped_email(idp=idp, failure_reason=MSGraphIssue.VERIFICATION_ERROR,
+                                            error="verification error",
+                                            error_description={str(e)})


We should avoid sending the raw error to the user, in the chance it contains sensitive information we didn't expect. notify_exception is fine and necessary, but for this we should say "unknown issue: please contact support"

Ah, I see! I replied without seeing this comment. I'll work on an improvement now!

Hi Biyeun, changed in 8871bad

jingcheng16 · 2024-04-24T16:52:06Z

@biyeun
The general exception might catch wrong configuration.

    app = msal.ConfidentialClientApplication(
        config["client_id"], authority=config["authority"],
        client_credential=config["secret"],
    )

This initialization can raise ValueError or general exception.

biyeun

thank you!

jingcheng16 added 5 commits February 20, 2024 10:19

New dependency msal required for sso

98a30e1

Task to autodeactivate web user

e80fd62

lint

74b9c3e

Add test for auto deactivate web user

19b4cbe

Call send_html_email_async with delay

bf80903

jingcheng16 requested review from biyeun and mjriley February 20, 2024 21:08

dimagimon added the dependencies Pull requests that update a dependency file label Feb 20, 2024

jingcheng16 marked this pull request as ready for review February 20, 2024 21:08

jingcheng16 requested a review from a team as a code owner February 20, 2024 21:08

jingcheng16 added the product/all-users-all-environments Change impacts all users on all environments label Feb 20, 2024

jingcheng16 added 2 commits February 20, 2024 17:51

Try to fix CI builds fail

93cc7af

the mock send should be called with delay

255574b

biyeun reviewed Feb 26, 2024

View reviewed changes

corehq/apps/sso/models.py Outdated Show resolved Hide resolved

biyeun reviewed Feb 26, 2024

View reviewed changes

millerdev reviewed Feb 26, 2024

View reviewed changes

jingcheng16 and others added 4 commits February 27, 2024 15:42

IdP not IDP

412f3a0

Co-authored-by: Biyeun <bbuczyk@dimagi.com>

only import msal in a function and remove it from docs-requirements

11b1160

New line at the end of the file

a86ca6c

Exclude user whose email domain is not controlled by the IdP

fc918be

biyeun reviewed Mar 1, 2024

View reviewed changes

Take authenticated_email_domains out of the loop

da9df8e

jingcheng16 requested a review from biyeun March 1, 2024 16:53

biyeun reviewed Mar 4, 2024

View reviewed changes

corehq/apps/sso/models.py Outdated Show resolved Hide resolved

Update text

a6a0e3b

Co-authored-by: Biyeun <bbuczyk@dimagi.com>

biyeun reviewed Mar 4, 2024

View reviewed changes

corehq/apps/sso/tasks.py Outdated Show resolved Hide resolved

biyeun reviewed Mar 4, 2024

View reviewed changes

corehq/apps/sso/models.py Outdated Show resolved Hide resolved

Add a new line at the end of the file

3c8a976

jingcheng16 requested review from gherceg and biyeun April 3, 2024 01:35

gherceg reviewed Apr 3, 2024

View reviewed changes

corehq/apps/sso/utils/context_helpers.py Outdated Show resolved Hide resolved

corehq/apps/sso/utils/context_helpers.py Outdated Show resolved Hide resolved

corehq/apps/sso/utils/entra.py Outdated Show resolved Hide resolved

jingcheng16 and others added 2 commits April 6, 2024 20:02

email_context's "to" only accept iterable

72bd477

Co-authored-by: Graham Herceg <gherceg@dimagi.com>

Remove wrong comment

674be9f

jingcheng16 requested a review from gherceg April 7, 2024 02:23

biyeun reviewed Apr 16, 2024

View reviewed changes

corehq/apps/sso/tests/test_tasks.py Outdated Show resolved Hide resolved

biyeun approved these changes Apr 16, 2024

View reviewed changes

gherceg approved these changes Apr 16, 2024

View reviewed changes

jingcheng16 and others added 2 commits April 16, 2024 09:54

rename IdP to idp

f14e689

Co-authored-by: Biyeun <bbuczyk@dimagi.com>

gherceg approved these changes Apr 17, 2024

View reviewed changes

jingcheng16 requested a review from biyeun April 17, 2024 21:43

biyeun approved these changes Apr 22, 2024

View reviewed changes

jingcheng16 added 3 commits April 23, 2024 09:56

Merge branch 'master' into jc/auto-deactivate-removed-sso-users

6188332

Catch general exception so we still process other idps

c4fdf0a

Fix keyword argument

b6ea6f3

jingcheng16 requested review from biyeun and gherceg April 24, 2024 00:00

biyeun approved these changes Apr 24, 2024

View reviewed changes

biyeun requested changes Apr 24, 2024

View reviewed changes

biyeun reviewed Apr 24, 2024

View reviewed changes

Avoid sending raw error to user

8871bad

jingcheng16 requested a review from biyeun April 24, 2024 17:20

biyeun approved these changes Apr 24, 2024

View reviewed changes

jingcheng16 merged commit b88782e into master Apr 24, 2024
13 checks passed

jingcheng16 deleted the jc/auto-deactivate-removed-sso-users branch April 24, 2024 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task to auto deactivate removed sso users #34151

Task to auto deactivate removed sso users #34151

jingcheng16 commented Feb 20, 2024

biyeun Feb 26, 2024

jingcheng16 Mar 1, 2024

biyeun Feb 26, 2024

biyeun Feb 26, 2024

jingcheng16 Feb 28, 2024

millerdev Feb 26, 2024 •

edited

Loading

jingcheng16 Feb 28, 2024

biyeun Mar 1, 2024

jingcheng16 Mar 1, 2024

biyeun Mar 4, 2024

biyeun Mar 4, 2024

jingcheng16 Mar 7, 2024 •

edited

Loading

biyeun Mar 4, 2024

biyeun Mar 4, 2024

jingcheng16 Mar 7, 2024

biyeun left a comment

gherceg left a comment

biyeun left a comment

biyeun Apr 24, 2024

jingcheng16 Apr 24, 2024

biyeun Apr 24, 2024

jingcheng16 Apr 24, 2024

jingcheng16 commented Apr 24, 2024

biyeun left a comment

Task to auto deactivate removed sso users #34151

Task to auto deactivate removed sso users #34151

Conversation

jingcheng16 commented Feb 20, 2024

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millerdev Feb 26, 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

jingcheng16 Mar 7, 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

biyeun left a comment

Choose a reason for hiding this comment

gherceg left a comment

Choose a reason for hiding this comment

biyeun 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

jingcheng16 commented Apr 24, 2024

biyeun left a comment

Choose a reason for hiding this comment

millerdev Feb 26, 2024 •

edited

Loading

jingcheng16 Mar 7, 2024 •

edited

Loading