From 98a30e11190788d2834fc178dbd70a9465467573 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 20 Feb 2024 10:19:46 -0500 Subject: [PATCH 01/57] New dependency msal required for sso --- requirements/dev-requirements.txt | 5 +++++ requirements/prod-requirements.txt | 5 +++++ requirements/requirements.txt | 5 +++++ requirements/sso-requirements.in | 1 + requirements/test-requirements.txt | 5 +++++ 5 files changed, 21 insertions(+) diff --git a/requirements/dev-requirements.txt b/requirements/dev-requirements.txt index 7b31bf0227fa..e158149e31ac 100644 --- a/requirements/dev-requirements.txt +++ b/requirements/dev-requirements.txt @@ -101,6 +101,7 @@ cryptography==41.0.6 # via # -r sso-requirements.in # jwcrypto + # msal # oic # pyjwt # pyopenssl @@ -424,6 +425,8 @@ mdit-py-plugins==0.4.0 # via myst-parser mdurl==0.1.2 # via markdown-it-py +msal==1.26.0 + # via -r sso-requirements.in msgpack==1.0.5 # via cachecontrol myst-parser==2.0.0 @@ -544,6 +547,7 @@ pyjwt[crypto]==2.7.0 # via # -r base-requirements.in # firebase-admin + # msal # pygithub # twilio pynacl==1.5.0 @@ -621,6 +625,7 @@ requests==2.28.2 # dropbox # google-api-core # google-cloud-storage + # msal # oic # pygithub # pyjwkest diff --git a/requirements/prod-requirements.txt b/requirements/prod-requirements.txt index 286785a746ea..517a47120612 100644 --- a/requirements/prod-requirements.txt +++ b/requirements/prod-requirements.txt @@ -88,6 +88,7 @@ cryptography==41.0.6 # -r prod-requirements.in # -r sso-requirements.in # jwcrypto + # msal # oic # pyjwt # pyopenssl @@ -364,6 +365,8 @@ markupsafe==2.1.2 # werkzeug matplotlib-inline==0.1.3 # via ipython +msal==1.26.0 + # via -r sso-requirements.in msgpack==1.0.5 # via cachecontrol ndg-httpsclient==0.5.1 @@ -462,6 +465,7 @@ pyjwt[crypto]==2.7.0 # via # -r base-requirements.in # firebase-admin + # msal # pygithub # twilio pynacl==1.5.0 @@ -530,6 +534,7 @@ requests==2.28.2 # dropbox # google-api-core # google-cloud-storage + # msal # oic # pygithub # pyjwkest diff --git a/requirements/requirements.txt b/requirements/requirements.txt index 69c5016d8699..bb076eb59cad 100644 --- a/requirements/requirements.txt +++ b/requirements/requirements.txt @@ -81,6 +81,7 @@ cryptography==41.0.6 # via # -r sso-requirements.in # jwcrypto + # msal # oic # pyjwt # pyopenssl @@ -342,6 +343,8 @@ markupsafe==2.1.2 # jinja2 # mako # werkzeug +msal==1.26.0 + # via -r sso-requirements.in msgpack==1.0.5 # via cachecontrol oauthlib==3.1.0 @@ -420,6 +423,7 @@ pyjwt[crypto]==2.7.0 # via # -r base-requirements.in # firebase-admin + # msal # pygithub # twilio pynacl==1.5.0 @@ -484,6 +488,7 @@ requests==2.28.2 # dropbox # google-api-core # google-cloud-storage + # msal # oic # pygithub # pyjwkest diff --git a/requirements/sso-requirements.in b/requirements/sso-requirements.in index 638f658f06ae..8144cfdba4cd 100644 --- a/requirements/sso-requirements.in +++ b/requirements/sso-requirements.in @@ -1,3 +1,4 @@ pyOpenSSL>=23.2.0 # for cryptography>=41 cryptography>=41 # for security vulnerability python3-saml +msal \ No newline at end of file diff --git a/requirements/test-requirements.txt b/requirements/test-requirements.txt index 9483281be4e1..6fa7e6846a43 100644 --- a/requirements/test-requirements.txt +++ b/requirements/test-requirements.txt @@ -88,6 +88,7 @@ cryptography==41.0.6 # via # -r sso-requirements.in # jwcrypto + # msal # oic # pyjwt # pyopenssl @@ -361,6 +362,8 @@ markupsafe==2.1.2 # jinja2 # mako # werkzeug +msal==1.26.0 + # via -r sso-requirements.in msgpack==1.0.5 # via cachecontrol nose==1.3.7 @@ -453,6 +456,7 @@ pyjwt[crypto]==2.7.0 # via # -r base-requirements.in # firebase-admin + # msal # pygithub # twilio pynacl==1.5.0 @@ -521,6 +525,7 @@ requests==2.28.2 # dropbox # google-api-core # google-cloud-storage + # msal # oic # pygithub # pyjwkest From e80fd621488ec19f7e3bea1d7258f885a3a070c2 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 20 Feb 2024 13:22:47 -0500 Subject: [PATCH 02/57] Task to autodeactivate web user --- corehq/apps/sso/models.py | 56 +++++++++++++++++++++++++++++++++++++++ corehq/apps/sso/tasks.py | 53 +++++++++++++++++++++++++++++++++++- 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index dd4dd98ce63b..c27edcc9a57e 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -1,13 +1,19 @@ +import msal +import requests + from django.db import models from django.db.models.signals import post_save, post_delete from django.dispatch import receiver from django.urls import reverse +from django.utils.translation import gettext as _ from corehq.apps.accounting.models import BillingAccount, Subscription +from corehq.apps.hqwebapp.tasks import send_html_email_async from corehq.apps.sso import certificates from corehq.apps.sso.exceptions import ServiceProviderCertificateError from corehq.apps.sso.utils.user_helpers import get_email_domain_from_username from corehq.util.quickcache import quickcache +from dimagi.utils.logging import notify_exception class IdentityProviderType: @@ -404,6 +410,56 @@ def get_required_identity_provider(cls, username): return idp return None + def get_all_members_of_the_idp(self): + authority_base_url = "https://login.microsoftonline.com/" + authority = f"{authority_base_url}{self.api_host}" + + config = { + "authority": authority, + "client_id": self.api_id, + "scope": ["https://graph.microsoft.com/.default"], + "secret": self.api_secret, + "endpoint": "https://graph.microsoft.com/v1.0/users" + } + + try: + # Create a preferably long-lived app instance which maintains a token cache. + app = msal.ConfidentialClientApplication( + config["client_id"], authority=config["authority"], + client_credential=config["secret"], + ) + # looks up a token from cache + result = app.acquire_token_silent(config["scope"], account=None) + if not result: + result = app.acquire_token_for_client(scopes=config["scope"]) + if "access_token" in result: + # Calling graph using the access token + response = requests.get( + config["endpoint"], + headers={'Authorization': 'Bearer ' + result['access_token']}, + ) + response.raise_for_status() # Raises an error for bad status + graph_data = response.json() + + user_principal_names = [user["userPrincipalName"] for user in graph_data["value"]] + return user_principal_names + else: + error_message = f"Error acquiring token: {result.get('error')}, Description: " + f"{result.get('error_description')}, Correlation ID: {result.get('correlation_id')}" + raise Exception(error_message) + except Exception as e: + notify_exception(None, f"Failed to get members of the IDP. {str(e)}") + + # Send email + subject = _("Issue Connecting to Microsoft Graph API") + body = f'There was an issue connecting to the Microsoft Graph API: {str(e)}' + recipient = self.owner.enterprise_admin_emails + send_html_email_async( + subject, + recipient, + body, + ) + @receiver(post_save, sender=Subscription) @receiver(post_delete, sender=Subscription) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 9261b6ef0967..c6844d0e17ab 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -3,12 +3,21 @@ from celery.schedules import crontab +from django.utils.translation import gettext as _ + from corehq.apps.celery import periodic_task from corehq.apps.hqwebapp.tasks import send_html_email_async -from corehq.apps.sso.models import IdentityProvider, IdentityProviderProtocol +from corehq.apps.sso.models import ( + AuthenticatedEmailDomain, + IdentityProvider, + IdentityProviderProtocol, + IdentityProviderType, + UserExemptFromSingleSignOn +) from corehq.apps.sso.utils.context_helpers import ( get_idp_cert_expiration_email_context, ) +from corehq.apps.users.models import WebUser log = logging.getLogger(__name__) @@ -99,3 +108,45 @@ def send_idp_cert_expires_reminder_emails(num_days): f"Failed to send cert reminder email for IdP {idp}: {exc!s}", exc_info=True, ) + + +@periodic_task(run_every=crontab(minute=0, hour=2), acks_late=True) +def auto_deactivate_removed_sso_users(): + for idp in IdentityProvider.objects.filter( + enable_user_deactivation=True, + idp_type=IdentityProviderType.AZURE_AD + ).all(): + # Fetch a list of users usernames that are members of the idp + idp_users = idp.get_all_members_of_the_idp() + # Fetch a list of active WebUser usernames that are members of project spaces associated with the idp + domains = idp.owner.get_domains() + web_user_in_account = set() + for domain in domains: + [web_user_in_account.add(user.username) for user in WebUser.by_domain(domain)] + + # Get a list of SSO Exempt users + authenticated_domains = AuthenticatedEmailDomain.objects.filter(identity_provider=idp) + sso_exempt_users = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains) + exempt_usernames = sso_exempt_users.values_list('username', flat=True) + + if len(idp_users) == 0 and len(web_user_in_account) - len(exempt_usernames) > 3: + # 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 " + "an empty list of Users from Azure AD") + send_html_email_async( + subject, + recipient, + body, + ) + else: + users_to_deactivate = [user for user in web_user_in_account if user not in idp_users + and user not in exempt_usernames] + + # Deactivate user that is not returned by Graph Users API + for username in users_to_deactivate: + user = WebUser.get_by_username(username) + if user and user.is_active: + user.is_active = False + user.save() From 74b9c3e5d087b90dcf606bacaafd1e2b1837418a Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 20 Feb 2024 13:24:30 -0500 Subject: [PATCH 03/57] lint --- corehq/apps/sso/tests/test_tasks.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index 510ad66e3689..122d52b678e6 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -32,8 +32,8 @@ def setUp(self): def test_create_rollover_service_provider_x509_certificates(self): self.idp.create_service_provider_certificate() self.idp.date_sp_cert_expiration = ( - datetime.datetime.utcnow() - - datetime.timedelta(days=_get_days_before_expiration(13)) + datetime.datetime.utcnow() + - datetime.timedelta(days=_get_days_before_expiration(13)) ) self.idp.save() @@ -64,8 +64,8 @@ def test_renew_service_provider_x509_certificates(self): self.idp.create_service_provider_certificate() self.idp.create_rollover_service_provider_certificate() self.idp.date_sp_cert_expiration = ( - datetime.datetime.utcnow() - - datetime.timedelta(seconds=_get_days_before_expiration(7)) + datetime.datetime.utcnow() + - datetime.timedelta(seconds=_get_days_before_expiration(7)) ) self.idp.save() From 19b4cbec61a139ef9a0a046d2dd93e56414e8f35 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 20 Feb 2024 13:24:35 -0500 Subject: [PATCH 04/57] Add test for auto deactivate web user --- corehq/apps/sso/tests/test_tasks.py | 92 +++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index 122d52b678e6..aaa74401617d 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -4,14 +4,20 @@ from django.test import TestCase from freezegun import freeze_time +from corehq.apps.accounting.models import SoftwarePlanEdition +from corehq.apps.accounting.tests import generator as accounting_generator +from corehq.apps.domain.models import Domain from corehq.apps.sso.certificates import DEFAULT_EXPIRATION +from corehq.apps.sso.models import AuthenticatedEmailDomain, IdentityProviderType, UserExemptFromSingleSignOn from corehq.apps.sso.tasks import ( IDP_CERT_EXPIRES_REMINDER_DAYS, + auto_deactivate_removed_sso_users, idp_cert_expires_reminder, renew_service_provider_x509_certificates, create_rollover_service_provider_x509_certificates, ) from corehq.apps.sso.tests import generator +from corehq.apps.users.models import WebUser def _get_days_before_expiration(days_before): @@ -133,3 +139,89 @@ def tearDown(self): def tearDownClass(cls): cls.account.delete() super().tearDownClass() + + +class TestAutoDeactivationTask(TestCase): + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.account = generator.get_billing_account_for_idp() + cls.account.enterprise_admin_emails = ['test@vaultwax.com'] + cls.account.save() + cls.domain = Domain.get_or_create_with_name("vaultwax-001", is_active=True) + + enterprise_plan = accounting_generator.subscribable_plan_version(edition=SoftwarePlanEdition.ENTERPRISE) + accounting_generator.generate_domain_subscription( + cls.account, + cls.domain, + date_start=datetime.date.today(), + date_end=None, + plan_version=enterprise_plan, + is_active=True, + ) + + cls.idp = generator.create_idp('vaultwax', cls.account) + cls.idp.enable_user_deactivation = True + cls.idp.idp_type = IdentityProviderType.AZURE_AD + cls.idp.save() + cls.email_domain = AuthenticatedEmailDomain.objects.create( + email_domain='vaultwax.com', + identity_provider=cls.idp, + ) + + def setUp(self): + super().setUp() + self.web_user_a = self._create_web_user('a@vaultwax.com') + self.web_user_b = self._create_web_user('b@vaultwax.com') + self.web_user_c = self._create_web_user('c@vaultwax.com') + self.web_user_d = self._create_web_user('d@vaultwax.com') + + @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') + def test_user_deactivation_logic(self, mock_get_all_members_of_the_idp): + self.assertTrue(self.web_user_c.is_active) + mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] + + auto_deactivate_removed_sso_users() + + # Refetch Web User + web_user = WebUser.get_by_username(self.web_user_c.username) + self.assertFalse(web_user.is_active) + + @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') + def test_sso_exempt_users_are_not_deactivated(self, mock_get_all_members_of_the_idp): + sso_exempt = self._create_web_user('exempt@vaultwax.com') + UserExemptFromSingleSignOn.objects.create( + username=sso_exempt.username, + email_domain=self.email_domain, + ) + mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] + + auto_deactivate_removed_sso_users() + + # Refetch Web User + web_user = WebUser.get_by_username(sso_exempt.username) + self.assertTrue(web_user.is_active) + + @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') + @patch('corehq.apps.sso.tasks.send_html_email_async') + def test_deactivation_skipped_if_azure_return_empty_sso_user(self, mock_send, mock_get_all_members_of_the_idp): + mock_get_all_members_of_the_idp.return_value = [] + + auto_deactivate_removed_sso_users() + + # Refetch Web User + web_user_a = WebUser.get_by_username(self.web_user_a.username) + self.assertTrue(web_user_a.is_active) + web_user_b = WebUser.get_by_username(self.web_user_b.username) + self.assertTrue(web_user_b.is_active) + web_user_c = WebUser.get_by_username(self.web_user_c.username) + self.assertTrue(web_user_c.is_active) + mock_send.assert_called_once() + + def _create_web_user(self, username): + user = WebUser.create( + self.domain.name, username, 'testpwd', None, None + ) + self.addCleanup(user.delete, self.domain.name, deleted_by=None) + return user From bf809032fef14b42fbb7ef695d641017eb15f6f1 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 20 Feb 2024 16:07:41 -0500 Subject: [PATCH 05/57] Call send_html_email_async with delay --- corehq/apps/sso/models.py | 2 +- corehq/apps/sso/tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index c27edcc9a57e..f2c041f9b117 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -454,7 +454,7 @@ def get_all_members_of_the_idp(self): subject = _("Issue Connecting to Microsoft Graph API") body = f'There was an issue connecting to the Microsoft Graph API: {str(e)}' recipient = self.owner.enterprise_admin_emails - send_html_email_async( + send_html_email_async.delay( subject, recipient, body, diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index c6844d0e17ab..dad6dca50280 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -135,7 +135,7 @@ def auto_deactivate_removed_sso_users(): recipient = idp.owner.enterprise_admin_emails body = _("we have temporarily skipped automatic deactivation of Web Users because we are receiving " "an empty list of Users from Azure AD") - send_html_email_async( + send_html_email_async.delay( subject, recipient, body, From 93cc7af895eb9c0bb6f9abf00d73222edbad455f Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 20 Feb 2024 17:51:19 -0500 Subject: [PATCH 06/57] Try to fix CI builds fail --- requirements/dev-requirements.txt | 4 +++- requirements/docs-requirements.in | 1 + requirements/docs-requirements.txt | 5 +++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/requirements/dev-requirements.txt b/requirements/dev-requirements.txt index e158149e31ac..b8f2f3756953 100644 --- a/requirements/dev-requirements.txt +++ b/requirements/dev-requirements.txt @@ -426,7 +426,9 @@ mdit-py-plugins==0.4.0 mdurl==0.1.2 # via markdown-it-py msal==1.26.0 - # via -r sso-requirements.in + # via + # -r docs-requirements.in + # -r sso-requirements.in msgpack==1.0.5 # via cachecontrol myst-parser==2.0.0 diff --git a/requirements/docs-requirements.in b/requirements/docs-requirements.in index d1d269b03f67..3a9fe87db621 100644 --- a/requirements/docs-requirements.in +++ b/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 urllib3<=1.26.18 # fix doc tests AssertionError: Current app already created diff --git a/requirements/docs-requirements.txt b/requirements/docs-requirements.txt index 327839c68093..654cd29501d6 100644 --- a/requirements/docs-requirements.txt +++ b/requirements/docs-requirements.txt @@ -84,6 +84,7 @@ crispy-bootstrap3to5 @ git+https://github.com/dimagi/crispy-bootstrap3to5.git@77 cryptography==41.0.6 # via # jwcrypto + # msal # oic # pyjwt # pyopenssl @@ -364,6 +365,8 @@ mdit-py-plugins==0.4.0 # via myst-parser mdurl==0.1.2 # via markdown-it-py +msal==1.26.0 + # via -r docs-requirements.in msgpack==1.0.5 # via cachecontrol myst-parser==2.0.0 @@ -449,6 +452,7 @@ pyjwt[crypto]==2.7.0 # via # -r base-requirements.in # firebase-admin + # msal # pygithub # twilio pynacl==1.5.0 @@ -514,6 +518,7 @@ requests==2.28.2 # dropbox # google-api-core # google-cloud-storage + # msal # oic # pygithub # pyjwkest From 255574b34017b880e7a07ba23fc5d760a38f65a3 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 21 Feb 2024 07:22:02 -0500 Subject: [PATCH 07/57] the mock send should be called with delay --- corehq/apps/sso/tests/test_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index aaa74401617d..610b33b675c5 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -204,7 +204,7 @@ def test_sso_exempt_users_are_not_deactivated(self, mock_get_all_members_of_the_ self.assertTrue(web_user.is_active) @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') - @patch('corehq.apps.sso.tasks.send_html_email_async') + @patch('corehq.apps.sso.tasks.send_html_email_async.delay') def test_deactivation_skipped_if_azure_return_empty_sso_user(self, mock_send, mock_get_all_members_of_the_idp): mock_get_all_members_of_the_idp.return_value = [] From 412f3a05de38e01874ba2c05bd3c293c07da8890 Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Tue, 27 Feb 2024 15:42:02 -0500 Subject: [PATCH 08/57] IdP not IDP Co-authored-by: Biyeun --- corehq/apps/sso/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index f2c041f9b117..03f47efed651 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -448,7 +448,7 @@ def get_all_members_of_the_idp(self): f"{result.get('error_description')}, Correlation ID: {result.get('correlation_id')}" raise Exception(error_message) except Exception as e: - notify_exception(None, f"Failed to get members of the IDP. {str(e)}") + notify_exception(None, f"Failed to get members of the IdP. {str(e)}") # Send email subject = _("Issue Connecting to Microsoft Graph API") From 11b11607f135468f8e9c84e184969e073dcd9e09 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 28 Feb 2024 09:03:01 -0500 Subject: [PATCH 09/57] only import msal in a function and remove it from docs-requirements --- corehq/apps/sso/models.py | 2 +- requirements/dev-requirements.txt | 4 +--- requirements/docs-requirements.in | 1 - requirements/docs-requirements.txt | 5 ----- 4 files changed, 2 insertions(+), 10 deletions(-) diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index 03f47efed651..419e3a761218 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -1,4 +1,3 @@ -import msal import requests from django.db import models @@ -411,6 +410,7 @@ def get_required_identity_provider(cls, username): return None def get_all_members_of_the_idp(self): + import msal authority_base_url = "https://login.microsoftonline.com/" authority = f"{authority_base_url}{self.api_host}" diff --git a/requirements/dev-requirements.txt b/requirements/dev-requirements.txt index b8f2f3756953..e158149e31ac 100644 --- a/requirements/dev-requirements.txt +++ b/requirements/dev-requirements.txt @@ -426,9 +426,7 @@ mdit-py-plugins==0.4.0 mdurl==0.1.2 # via markdown-it-py msal==1.26.0 - # via - # -r docs-requirements.in - # -r sso-requirements.in + # via -r sso-requirements.in msgpack==1.0.5 # via cachecontrol myst-parser==2.0.0 diff --git a/requirements/docs-requirements.in b/requirements/docs-requirements.in index 3a9fe87db621..d1d269b03f67 100644 --- a/requirements/docs-requirements.in +++ b/requirements/docs-requirements.in @@ -7,6 +7,5 @@ sphinxcontrib-django sphinx-rtd-theme pyOpenSSL # CI builds fail without OpenSSL -msal # CI builds fail without OpenSSL urllib3<=1.26.18 # fix doc tests AssertionError: Current app already created diff --git a/requirements/docs-requirements.txt b/requirements/docs-requirements.txt index 654cd29501d6..327839c68093 100644 --- a/requirements/docs-requirements.txt +++ b/requirements/docs-requirements.txt @@ -84,7 +84,6 @@ crispy-bootstrap3to5 @ git+https://github.com/dimagi/crispy-bootstrap3to5.git@77 cryptography==41.0.6 # via # jwcrypto - # msal # oic # pyjwt # pyopenssl @@ -365,8 +364,6 @@ mdit-py-plugins==0.4.0 # via myst-parser mdurl==0.1.2 # via markdown-it-py -msal==1.26.0 - # via -r docs-requirements.in msgpack==1.0.5 # via cachecontrol myst-parser==2.0.0 @@ -452,7 +449,6 @@ pyjwt[crypto]==2.7.0 # via # -r base-requirements.in # firebase-admin - # msal # pygithub # twilio pynacl==1.5.0 @@ -518,7 +514,6 @@ requests==2.28.2 # dropbox # google-api-core # google-cloud-storage - # msal # oic # pygithub # pyjwkest From a86ca6cb8243f2bf98b40ab643db861df72e7099 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 28 Feb 2024 09:03:33 -0500 Subject: [PATCH 10/57] New line at the end of the file --- requirements/sso-requirements.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/sso-requirements.in b/requirements/sso-requirements.in index 8144cfdba4cd..b73db9e3a720 100644 --- a/requirements/sso-requirements.in +++ b/requirements/sso-requirements.in @@ -1,4 +1,4 @@ pyOpenSSL>=23.2.0 # for cryptography>=41 cryptography>=41 # for security vulnerability python3-saml -msal \ No newline at end of file +msal From fc918be0f096ef40ba4d021f5be75647d3df3a85 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 29 Feb 2024 21:55:03 -0500 Subject: [PATCH 11/57] Exclude user whose email domain is not controlled by the IdP --- corehq/apps/sso/tasks.py | 13 +++++++++++-- corehq/apps/sso/tests/test_tasks.py | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index dad6dca50280..284267fb2687 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -17,6 +17,7 @@ from corehq.apps.sso.utils.context_helpers import ( get_idp_cert_expiration_email_context, ) +from corehq.apps.sso.utils.user_helpers import get_email_domain_from_username from corehq.apps.users.models import WebUser log = logging.getLogger(__name__) @@ -141,8 +142,16 @@ def auto_deactivate_removed_sso_users(): body, ) else: - users_to_deactivate = [user for user in web_user_in_account if user not in idp_users - and user not in exempt_usernames] + users_to_deactivate = [] + + for user in web_user_in_account: + # 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) + # If the user's email domain is in the list of email domains controlled by IdP + if email_domain in authenticated_email_domains: + users_to_deactivate.append(user) # Deactivate user that is not returned by Graph Users API for username in users_to_deactivate: diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index 610b33b675c5..e4b709eff54b 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -219,6 +219,20 @@ def test_deactivation_skipped_if_azure_return_empty_sso_user(self, mock_send, mo self.assertTrue(web_user_c.is_active) mock_send.assert_called_once() + @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') + def test_deactivation_skip_members_of_the_domains_but_not_have_an_email_domain_controlled_by_the_IdP(self, + mock_get_all_members_of_the_idp): + dimagi_user = self._create_web_user('superuser@dimagi.com') + mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] + + auto_deactivate_removed_sso_users() + + # Refetch Web User + dimagi_user = WebUser.get_by_username(dimagi_user.username) + self.assertTrue(dimagi_user.is_active) + web_user_c = WebUser.get_by_username(self.web_user_c.username) + self.assertFalse(web_user_c.is_active) + def _create_web_user(self, username): user = WebUser.create( self.domain.name, username, 'testpwd', None, None From da9df8e615a01c4e0ae3126370f417e9490e9e65 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Fri, 1 Mar 2024 11:53:00 -0500 Subject: [PATCH 12/57] Take authenticated_email_domains out of the loop --- corehq/apps/sso/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 284267fb2687..5f45bfdd7efe 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -143,12 +143,12 @@ def auto_deactivate_removed_sso_users(): ) else: users_to_deactivate = [] + authenticated_email_domains = authenticated_domains.values_list('email_domain', flat=True) for user in web_user_in_account: # 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) # If the user's email domain is in the list of email domains controlled by IdP if email_domain in authenticated_email_domains: users_to_deactivate.append(user) From a6a0e3b8ce973a89ba5598a4689c8a5eac1c328f Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Mon, 4 Mar 2024 13:58:43 -0500 Subject: [PATCH 13/57] Update text Co-authored-by: Biyeun --- corehq/apps/sso/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index 419e3a761218..70f42e172ecf 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -451,7 +451,7 @@ def get_all_members_of_the_idp(self): notify_exception(None, f"Failed to get members of the IdP. {str(e)}") # Send email - subject = _("Issue Connecting to Microsoft Graph API") + subject = _("CommCare HQ Alert: SSO Remote User Management, Issue Connecting to Microsoft Graph API") body = f'There was an issue connecting to the Microsoft Graph API: {str(e)}' recipient = self.owner.enterprise_admin_emails send_html_email_async.delay( From af4d832d73c8c1477d6908b4a9da93d2b47462b1 Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Mon, 4 Mar 2024 20:02:32 -0500 Subject: [PATCH 14/57] Update text Co-authored-by: Biyeun --- corehq/apps/sso/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 5f45bfdd7efe..86d43fba814e 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -132,7 +132,7 @@ def auto_deactivate_removed_sso_users(): if len(idp_users) == 0 and len(web_user_in_account) - len(exempt_usernames) > 3: # Send email - subject = _("Temporarily skipped automatic deactivation of Web Users") + subject = _("CommCare HQ Alert: Temporarily skipped automatic deactivation of SSO Web Users (Remote User Management)") recipient = idp.owner.enterprise_admin_emails body = _("we have temporarily skipped automatic deactivation of Web Users because we are receiving " "an empty list of Users from Azure AD") From 2b2d8850c3df875583ca4207b2ae6752d74c03e4 Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Mon, 4 Mar 2024 20:03:01 -0500 Subject: [PATCH 15/57] Better error message Co-authored-by: Biyeun --- corehq/apps/sso/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index 70f42e172ecf..f477ace5fcbc 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -444,7 +444,7 @@ def get_all_members_of_the_idp(self): user_principal_names = [user["userPrincipalName"] for user in graph_data["value"]] return user_principal_names else: - error_message = f"Error acquiring token: {result.get('error')}, Description: " + error_message = f"Error acquiring Microsoft Graph API token: {result.get('error')}, Description: " f"{result.get('error_description')}, Correlation ID: {result.get('correlation_id')}" raise Exception(error_message) except Exception as e: From 1cb05a41e76857d22be91e9ce3bf212ebdf3f53d Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Mon, 4 Mar 2024 20:48:04 -0500 Subject: [PATCH 16/57] Update comment Co-authored-by: Biyeun --- corehq/apps/sso/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 86d43fba814e..4e17c6040e06 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -125,7 +125,7 @@ def auto_deactivate_removed_sso_users(): for domain in domains: [web_user_in_account.add(user.username) for user in WebUser.by_domain(domain)] - # Get a list of SSO Exempt users + # Get criteria for exempting usernames and email domains from the deactivation list authenticated_domains = AuthenticatedEmailDomain.objects.filter(identity_provider=idp) sso_exempt_users = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains) exempt_usernames = sso_exempt_users.values_list('username', flat=True) From b49c369325bd7ae58a04967d281837519b245aad Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Mon, 4 Mar 2024 21:11:38 -0500 Subject: [PATCH 17/57] lint --- corehq/apps/sso/tasks.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 4e17c6040e06..79cb1c1751db 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -132,7 +132,8 @@ def auto_deactivate_removed_sso_users(): if len(idp_users) == 0 and len(web_user_in_account) - len(exempt_usernames) > 3: # Send email - subject = _("CommCare HQ Alert: Temporarily skipped automatic deactivation of SSO Web Users (Remote User Management)") + subject = _("CommCare HQ Alert: Temporarily skipped automatic deactivation of SSO Web Users" + " (Remote User Management)") recipient = idp.owner.enterprise_admin_emails body = _("we have temporarily skipped automatic deactivation of Web Users because we are receiving " "an empty list of Users from Azure AD") From c2ec7bac32f420db122d6730ab5f2fa5678eb44c Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Mon, 4 Mar 2024 21:11:53 -0500 Subject: [PATCH 18/57] Better readability --- corehq/apps/sso/tasks.py | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 79cb1c1751db..25f53a44e214 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -142,21 +142,22 @@ def auto_deactivate_removed_sso_users(): recipient, body, ) - else: - users_to_deactivate = [] - authenticated_email_domains = authenticated_domains.values_list('email_domain', flat=True) - - for user in web_user_in_account: - # 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) - # If the user's email domain is in the list of email domains controlled by IdP - if email_domain in authenticated_email_domains: - users_to_deactivate.append(user) - - # Deactivate user that is not returned by Graph Users API - for username in users_to_deactivate: - user = WebUser.get_by_username(username) - if user and user.is_active: - user.is_active = False - user.save() + return + + users_to_deactivate = [] + authenticated_email_domains = authenticated_domains.values_list('email_domain', flat=True) + + for user in web_user_in_account: + # 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) + # If the user's email domain is in the list of email domains controlled by IdP + if email_domain in authenticated_email_domains: + users_to_deactivate.append(user) + + # Deactivate user that is not returned by Graph Users API + for username in users_to_deactivate: + user = WebUser.get_by_username(username) + if user and user.is_active: + user.is_active = False + user.save() From 9d7d3d0255a69b48797b6ae2ab803009c625b0e9 Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Tue, 5 Mar 2024 11:34:02 -0500 Subject: [PATCH 19/57] Simplify exmept_usernames query logic Co-authored-by: Graham Herceg --- corehq/apps/sso/tasks.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 25f53a44e214..4921599cb2a1 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -127,8 +127,7 @@ def auto_deactivate_removed_sso_users(): # Get criteria for exempting usernames and email domains from the deactivation list authenticated_domains = AuthenticatedEmailDomain.objects.filter(identity_provider=idp) - sso_exempt_users = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains) - exempt_usernames = sso_exempt_users.values_list('username', flat=True) + exempt_usernames = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains).values_list('username', flat=True) if len(idp_users) == 0 and len(web_user_in_account) - len(exempt_usernames) > 3: # Send email From 043c4ea1953cdcb247de9d5c646e03d4c12d74ce Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 5 Mar 2024 11:37:34 -0500 Subject: [PATCH 20/57] lint --- corehq/apps/sso/tasks.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 4921599cb2a1..73ea3dc7486f 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -127,7 +127,8 @@ def auto_deactivate_removed_sso_users(): # Get criteria for exempting usernames and email domains from the deactivation list authenticated_domains = AuthenticatedEmailDomain.objects.filter(identity_provider=idp) - exempt_usernames = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains).values_list('username', flat=True) + exempt_usernames = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains + ).values_list('username', flat=True) if len(idp_users) == 0 and len(web_user_in_account) - len(exempt_usernames) > 3: # Send email From 28aaa49c3bf080454d2f36941b64d7a2f7d129e9 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 5 Mar 2024 11:42:30 -0500 Subject: [PATCH 21/57] Add comment to explain why web_user_d is required --- corehq/apps/sso/tests/test_tasks.py | 1 + 1 file changed, 1 insertion(+) diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index e4b709eff54b..923914608ed3 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -175,6 +175,7 @@ def setUp(self): self.web_user_a = self._create_web_user('a@vaultwax.com') self.web_user_b = self._create_web_user('b@vaultwax.com') self.web_user_c = self._create_web_user('c@vaultwax.com') + # web_user_d is required so the total number of IdP user meet the threshold for auto-deactivation self.web_user_d = self._create_web_user('d@vaultwax.com') @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') From ee625d0e1d570114f2e753ad02010a6c12d52596 Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Tue, 5 Mar 2024 20:38:01 -0500 Subject: [PATCH 22/57] Refactored the process of gathering usernames from WebUsers across all domains within an account Co-authored-by: Graham Herceg --- corehq/apps/sso/tasks.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 73ea3dc7486f..04ae3ae7f891 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -120,10 +120,7 @@ def auto_deactivate_removed_sso_users(): # Fetch a list of users usernames that are members of the idp idp_users = idp.get_all_members_of_the_idp() # Fetch a list of active WebUser usernames that are members of project spaces associated with the idp - domains = idp.owner.get_domains() - web_user_in_account = set() - for domain in domains: - [web_user_in_account.add(user.username) for user in WebUser.by_domain(domain)] + web_users_in_account = { user.username for domain in idp.owner.get_domains() for user in WebUser.by_domain(domain) } # Get criteria for exempting usernames and email domains from the deactivation list authenticated_domains = AuthenticatedEmailDomain.objects.filter(identity_provider=idp) From e2e815edcccb178b5a0c6f1594db66337412a722 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 5 Mar 2024 21:21:47 -0500 Subject: [PATCH 23/57] lint --- corehq/apps/sso/tasks.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 04ae3ae7f891..d4596b354120 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -120,14 +120,15 @@ def auto_deactivate_removed_sso_users(): # Fetch a list of users usernames that are members of the idp idp_users = idp.get_all_members_of_the_idp() # Fetch a list of active WebUser usernames that are members of project spaces associated with the idp - web_users_in_account = { user.username for domain in idp.owner.get_domains() for user in WebUser.by_domain(domain) } + web_users_in_account = {user.username for domain in idp.owner.get_domains() + for user in WebUser.by_domain(domain)} # Get criteria for exempting usernames and email domains from the deactivation list authenticated_domains = AuthenticatedEmailDomain.objects.filter(identity_provider=idp) exempt_usernames = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains ).values_list('username', flat=True) - if len(idp_users) == 0 and len(web_user_in_account) - len(exempt_usernames) > 3: + if len(idp_users) == 0 and len(web_users_in_account) - len(exempt_usernames) > 3: # Send email subject = _("CommCare HQ Alert: Temporarily skipped automatic deactivation of SSO Web Users" " (Remote User Management)") @@ -144,8 +145,8 @@ def auto_deactivate_removed_sso_users(): users_to_deactivate = [] authenticated_email_domains = authenticated_domains.values_list('email_domain', flat=True) - for user in web_user_in_account: # If the user is not returned by IdP and is not exempted from SSO + for user in web_users_in_account: if user not in idp_users and user not in exempt_usernames: email_domain = get_email_domain_from_username(user) # If the user's email domain is in the list of email domains controlled by IdP From 2ea9229df77a001740a709ab3712175631ab586c Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 5 Mar 2024 21:21:55 -0500 Subject: [PATCH 24/57] Code is self-explanatory so remove comment --- corehq/apps/sso/tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index d4596b354120..751a2a56ce5d 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -145,7 +145,6 @@ def auto_deactivate_removed_sso_users(): users_to_deactivate = [] authenticated_email_domains = authenticated_domains.values_list('email_domain', flat=True) - # If the user is not returned by IdP and is not exempted from SSO for user in web_users_in_account: if user not in idp_users and user not in exempt_usernames: email_domain = get_email_domain_from_username(user) From 2ffda903f204a989fd72babfa21ae34cd9d419d8 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 5 Mar 2024 21:28:46 -0500 Subject: [PATCH 25/57] Remove unnecessary comment --- corehq/apps/sso/tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 751a2a56ce5d..5e0c98741840 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -148,7 +148,6 @@ def auto_deactivate_removed_sso_users(): for user in web_users_in_account: if user not in idp_users and user not in exempt_usernames: email_domain = get_email_domain_from_username(user) - # If the user's email domain is in the list of email domains controlled by IdP if email_domain in authenticated_email_domains: users_to_deactivate.append(user) From 9b31683ecfdbc7e470c55ec212bd38f7b64a1244 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 6 Mar 2024 14:44:24 -0500 Subject: [PATCH 26/57] Refactor: "user" to refer to WebUser objects, "username" to refer to the actual string property --- corehq/apps/sso/tasks.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 5e0c98741840..97a060f5d976 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -120,7 +120,7 @@ def auto_deactivate_removed_sso_users(): # Fetch a list of users usernames that are members of the idp idp_users = idp.get_all_members_of_the_idp() # Fetch a list of active WebUser usernames that are members of project spaces associated with the idp - web_users_in_account = {user.username for domain in idp.owner.get_domains() + web_user_usernames_in_account = {user.username for domain in idp.owner.get_domains() for user in WebUser.by_domain(domain)} # Get criteria for exempting usernames and email domains from the deactivation list @@ -128,7 +128,7 @@ def auto_deactivate_removed_sso_users(): exempt_usernames = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains ).values_list('username', flat=True) - if len(idp_users) == 0 and len(web_users_in_account) - len(exempt_usernames) > 3: + if len(idp_users) == 0 and len(web_user_usernames_in_account) - len(exempt_usernames) > 3: # Send email subject = _("CommCare HQ Alert: Temporarily skipped automatic deactivation of SSO Web Users" " (Remote User Management)") @@ -142,17 +142,17 @@ def auto_deactivate_removed_sso_users(): ) return - users_to_deactivate = [] + usernames_to_deactivate = [] authenticated_email_domains = authenticated_domains.values_list('email_domain', flat=True) - for user in web_users_in_account: - if user not in idp_users and user not in exempt_usernames: - email_domain = get_email_domain_from_username(user) + for username in web_user_usernames_in_account: + if username not in idp_users and username not in exempt_usernames: + email_domain = get_email_domain_from_username(username) if email_domain in authenticated_email_domains: - users_to_deactivate.append(user) + usernames_to_deactivate.append(username) # Deactivate user that is not returned by Graph Users API - for username in users_to_deactivate: + for username in usernames_to_deactivate: user = WebUser.get_by_username(username) if user and user.is_active: user.is_active = False From a1940ef363250d2c4d7b5ca6b1ee708fb4081e81 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 6 Mar 2024 14:56:52 -0500 Subject: [PATCH 27/57] Refactor: a more specific test name --- corehq/apps/sso/tests/test_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index 923914608ed3..181f364a8d47 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -179,7 +179,7 @@ def setUp(self): self.web_user_d = self._create_web_user('d@vaultwax.com') @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') - def test_user_deactivation_logic(self, mock_get_all_members_of_the_idp): + def test_user_is_deactivated_if_not_member_of_idp(self, mock_get_all_members_of_the_idp): self.assertTrue(self.web_user_c.is_active) mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] From 080b22a5c40adab3718728ed3261e3ad3afccfdb Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 6 Mar 2024 17:14:02 -0500 Subject: [PATCH 28/57] Refactor: set the patcher up at the class level --- corehq/apps/sso/tests/test_tasks.py | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index 181f364a8d47..a68325fffbdb 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -169,6 +169,9 @@ def setUpClass(cls): email_domain='vaultwax.com', identity_provider=cls.idp, ) + idp_patcher = patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') + cls.mock_get_all_members_of_the_idp = idp_patcher.start() + cls.addClassCleanup(idp_patcher.stop) def setUp(self): super().setUp() @@ -178,10 +181,9 @@ def setUp(self): # web_user_d is required so the total number of IdP user meet the threshold for auto-deactivation self.web_user_d = self._create_web_user('d@vaultwax.com') - @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') - def test_user_is_deactivated_if_not_member_of_idp(self, mock_get_all_members_of_the_idp): + def test_user_is_deactivated_if_not_member_of_idp(self): self.assertTrue(self.web_user_c.is_active) - mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] + self.mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] auto_deactivate_removed_sso_users() @@ -189,14 +191,13 @@ def test_user_is_deactivated_if_not_member_of_idp(self, mock_get_all_members_of_ web_user = WebUser.get_by_username(self.web_user_c.username) self.assertFalse(web_user.is_active) - @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') - def test_sso_exempt_users_are_not_deactivated(self, mock_get_all_members_of_the_idp): + def test_sso_exempt_users_are_not_deactivated(self): sso_exempt = self._create_web_user('exempt@vaultwax.com') UserExemptFromSingleSignOn.objects.create( username=sso_exempt.username, email_domain=self.email_domain, ) - mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] + self.mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] auto_deactivate_removed_sso_users() @@ -204,10 +205,9 @@ def test_sso_exempt_users_are_not_deactivated(self, mock_get_all_members_of_the_ web_user = WebUser.get_by_username(sso_exempt.username) self.assertTrue(web_user.is_active) - @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') @patch('corehq.apps.sso.tasks.send_html_email_async.delay') - def test_deactivation_skipped_if_azure_return_empty_sso_user(self, mock_send, mock_get_all_members_of_the_idp): - mock_get_all_members_of_the_idp.return_value = [] + def test_deactivation_skipped_if_azure_return_empty_sso_user(self, mock_send): + self.mock_get_all_members_of_the_idp.return_value = [] auto_deactivate_removed_sso_users() @@ -220,11 +220,9 @@ def test_deactivation_skipped_if_azure_return_empty_sso_user(self, mock_send, mo self.assertTrue(web_user_c.is_active) mock_send.assert_called_once() - @patch('corehq.apps.sso.models.IdentityProvider.get_all_members_of_the_idp') - def test_deactivation_skip_members_of_the_domains_but_not_have_an_email_domain_controlled_by_the_IdP(self, - mock_get_all_members_of_the_idp): + def test_deactivation_skip_members_of_the_domains_but_not_have_an_email_domain_controlled_by_the_IdP(self): dimagi_user = self._create_web_user('superuser@dimagi.com') - mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] + self.mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] auto_deactivate_removed_sso_users() From 602208719e9ee591431876a37c68bc37f357aee6 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 6 Mar 2024 17:15:48 -0500 Subject: [PATCH 29/57] Add class cleanup --- corehq/apps/sso/tests/test_tasks.py | 1 + 1 file changed, 1 insertion(+) diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index a68325fffbdb..64807126067c 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -150,6 +150,7 @@ def setUpClass(cls): cls.account.enterprise_admin_emails = ['test@vaultwax.com'] cls.account.save() cls.domain = Domain.get_or_create_with_name("vaultwax-001", is_active=True) + cls.addClassCleanup(cls.domain.delete) enterprise_plan = accounting_generator.subscribable_plan_version(edition=SoftwarePlanEdition.ENTERPRISE) accounting_generator.generate_domain_subscription( From 9027492e09b46026c8723d965ce0b4e473f84208 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 6 Mar 2024 18:05:41 -0500 Subject: [PATCH 30/57] lint --- corehq/apps/accounting/tasks.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/corehq/apps/accounting/tasks.py b/corehq/apps/accounting/tasks.py index 760ec194564a..83382a844e5e 100644 --- a/corehq/apps/accounting/tasks.py +++ b/corehq/apps/accounting/tasks.py @@ -554,13 +554,13 @@ def weekly_digest(): in_forty_days = today + datetime.timedelta(days=40) ending_in_forty_days = [sub for sub in Subscription.visible_objects.filter( - date_end__lte=in_forty_days, - date_end__gte=today, - is_active=True, - is_trial=False, - ).exclude( - account__dimagi_contact='', - ) if not sub.is_renewed] + date_end__lte=in_forty_days, + date_end__gte=today, + is_active=True, + is_trial=False, + ).exclude( + account__dimagi_contact='', + ) if not sub.is_renewed] if not ending_in_forty_days: log_accounting_info( From 9b27bcefe42dc5fd0fd9e46cca1920cb2a06751b Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 6 Mar 2024 18:07:16 -0500 Subject: [PATCH 31/57] Refactor: create method on BillingAccount and call it in tasks --- corehq/apps/accounting/models.py | 19 +++++++++++++++++++ corehq/apps/accounting/tasks.py | 6 +----- corehq/apps/sso/tasks.py | 7 +++---- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/corehq/apps/accounting/models.py b/corehq/apps/accounting/models.py index 77b46366fd49..3a777901ef49 100644 --- a/corehq/apps/accounting/models.py +++ b/corehq/apps/accounting/models.py @@ -600,6 +600,25 @@ def _send_autopay_card_added_email(self, domain): use_domain_gateway=True, ) + def get_web_users(self, info_type='id'): + """ + Get a set of web user information in the billing account. + + :param info_type: Specify 'id' to get user IDs, or 'username' to get user usernames. + :return: A set of user IDs or usernames, based on the info_type parameter. + """ + domains = self.get_domains() + web_users = set() + + if info_type == 'id': + for domain in domains: + web_users.update(WebUser.ids_by_domain(domain)) + elif info_type == 'username': + for domain in domains: + web_users.update(user.username for user in WebUser.by_domain(domain)) + + return web_users + @staticmethod def should_show_sms_billable_report(domain): account = BillingAccount.get_account_by_domain(domain) diff --git a/corehq/apps/accounting/tasks.py b/corehq/apps/accounting/tasks.py index 83382a844e5e..557e4e9f36cd 100644 --- a/corehq/apps/accounting/tasks.py +++ b/corehq/apps/accounting/tasks.py @@ -69,7 +69,6 @@ from corehq.apps.accounting.utils.subscription import ( assign_explicit_unpaid_subscription, ) -from corehq.apps.users.models import WebUser from corehq.apps.app_manager.dbaccessors import get_all_apps from corehq.apps.celery import periodic_task, task from corehq.apps.domain.models import Domain @@ -833,10 +832,7 @@ def calculate_web_users_in_all_billing_accounts(today=None): today = today or datetime.date.today() for account in BillingAccount.objects.all(): record_date = today - relativedelta(days=1) - domains = account.get_domains() - web_user_in_account = set() - for domain in domains: - [web_user_in_account.add(id) for id in WebUser.ids_by_domain(domain)] + web_user_in_account = account.get_web_users() num_users = len(web_user_in_account) try: BillingAccountWebUserHistory.objects.create( diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 97a060f5d976..5fe8584dd253 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -120,15 +120,14 @@ def auto_deactivate_removed_sso_users(): # Fetch a list of users usernames that are members of the idp idp_users = idp.get_all_members_of_the_idp() # Fetch a list of active WebUser usernames that are members of project spaces associated with the idp - web_user_usernames_in_account = {user.username for domain in idp.owner.get_domains() - for user in WebUser.by_domain(domain)} + usernames_in_account = idp.owner.get_web_users(info_type='username') # Get criteria for exempting usernames and email domains from the deactivation list authenticated_domains = AuthenticatedEmailDomain.objects.filter(identity_provider=idp) exempt_usernames = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains ).values_list('username', flat=True) - if len(idp_users) == 0 and len(web_user_usernames_in_account) - len(exempt_usernames) > 3: + if len(idp_users) == 0 and len(usernames_in_account) - len(exempt_usernames) > 3: # Send email subject = _("CommCare HQ Alert: Temporarily skipped automatic deactivation of SSO Web Users" " (Remote User Management)") @@ -145,7 +144,7 @@ def auto_deactivate_removed_sso_users(): usernames_to_deactivate = [] authenticated_email_domains = authenticated_domains.values_list('email_domain', flat=True) - for username in web_user_usernames_in_account: + for username in usernames_in_account: if username not in idp_users and username not in exempt_usernames: email_domain = get_email_domain_from_username(username) if email_domain in authenticated_email_domains: From f2ed9954fff9c42ebc6453bf679be75c8b10dc7b Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 7 Mar 2024 17:29:55 -0500 Subject: [PATCH 32/57] Utility function and new template for api connection issue email --- corehq/apps/sso/models.py | 35 +++++++++++----- ...aph_api_connection_issue_notification.html | 40 +++++++++++++++++++ ...raph_api_connection_issue_notification.txt | 20 ++++++++++ corehq/apps/sso/utils/context_helpers.py | 24 +++++++++++ 4 files changed, 109 insertions(+), 10 deletions(-) create mode 100644 corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html create mode 100644 corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index f477ace5fcbc..e5a93e025f63 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -1,19 +1,22 @@ +import logging import requests from django.db import models from django.db.models.signals import post_save, post_delete from django.dispatch import receiver from django.urls import reverse -from django.utils.translation import gettext as _ from corehq.apps.accounting.models import BillingAccount, Subscription from corehq.apps.hqwebapp.tasks import send_html_email_async from corehq.apps.sso import certificates from corehq.apps.sso.exceptions import ServiceProviderCertificateError +from corehq.apps.sso.utils.context_helpers import get_graph_api_connection_issue_email_context from corehq.apps.sso.utils.user_helpers import get_email_domain_from_username from corehq.util.quickcache import quickcache from dimagi.utils.logging import notify_exception +log = logging.getLogger(__name__) + class IdentityProviderType: AZURE_AD = 'azure_ad' @@ -450,15 +453,27 @@ def get_all_members_of_the_idp(self): except Exception as e: notify_exception(None, f"Failed to get members of the IdP. {str(e)}") - # Send email - subject = _("CommCare HQ Alert: SSO Remote User Management, Issue Connecting to Microsoft Graph API") - body = f'There was an issue connecting to the Microsoft Graph API: {str(e)}' - recipient = self.owner.enterprise_admin_emails - send_html_email_async.delay( - subject, - recipient, - body, - ) + context = get_graph_api_connection_issue_email_context(self, str(e)) + try: + for send_to in context["to"]: + send_html_email_async.delay( + context["subject"], + send_to, + context["html"], + text_content=context["plaintext"], + email_from=context["from"], + bcc=context["bcc"], + ) + log.info( + "Sent Microsoft Graph API connection issue email " + "for %(idp_name)s to %(send_to)s." % { + "idp_name": self.name, + "send_to": send_to, + } + ) + except Exception as exc: + notify_exception(None, f"Failed to send Microsoft Graph API connection issue" + f" for IdP {self.name}: {exc!s}", exc_info=True) @receiver(post_save, sender=Subscription) diff --git a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html new file mode 100644 index 000000000000..950d7b3f9958 --- /dev/null +++ b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html @@ -0,0 +1,40 @@ +{% load i18n %} +

{% blocktrans %}Dear enterprise administrator,{% endblocktrans %}

+ +

+ {% blocktrans %} + 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. + {% endblocktrans %} +

+ +

+ {% blocktrans %} + This was the error message: {{ error }} + {% endblocktrans %} +

+ +

+ {% blocktrans %} + Please check the configuration of the Remote User Management settings in your Identity Provider. + {% endblocktrans %} +

+ +

+ {% blocktrans %} + If you have any questions, please don’t hesitate to contact + {{ contact_email }}. Thank you for your use and support of CommCare. + {% endblocktrans %} +

+ + +

+ {% blocktrans %} + Best regards, + {% endblocktrans %} +

+ +

+ {% blocktrans %}The CommCare HQ Team{% endblocktrans %}
+ {{ base_url }} +

diff --git a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt new file mode 100644 index 000000000000..0607f6c28455 --- /dev/null +++ b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt @@ -0,0 +1,20 @@ +{% load i18n %} +{% blocktrans %} +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 +{{ base_url }} +{% endblocktrans %} \ No newline at end of file diff --git a/corehq/apps/sso/utils/context_helpers.py b/corehq/apps/sso/utils/context_helpers.py index 72bd2aa2c385..4a1e41ccb8cb 100644 --- a/corehq/apps/sso/utils/context_helpers.py +++ b/corehq/apps/sso/utils/context_helpers.py @@ -64,3 +64,27 @@ def get_idp_cert_expiration_email_context(idp): if idp.owner.dimagi_contact: email_context["bcc"].append(idp.owner.dimagi_contact) return email_context + + +def get_graph_api_connection_issue_email_context(idp, error): + subject = _("CommCare HQ Alert: SSO Remote User Management, Issue Connecting to Microsoft Graph API") + template_context = { + "error": error, + "contact_email": settings.ACCOUNTS_EMAIL, + } + body_html, body_txt = render_multiple_to_strings( + template_context, + "sso/email/microsoft_graph_api_connection_issue_email_context.html", + "sso/email/microsoft_graph_api_connection_issue_email_context.txt", + ) + email_context = { + "subject": subject, + "from": _(f"Dimagi CommCare Accounts <{settings.ACCOUNTS_EMAIL}>"), + "to": idp.owner.enterprise_admin_emails, + "bcc": [settings.ACCOUNTS_EMAIL], + "html": body_html, + "plaintext": body_txt, + } + if idp.owner.dimagi_contact: + email_context["bcc"].append(idp.owner.dimagi_contact) + return email_context From 9d29c36af755aabddcfcc2e3ab9423bf35ee1c41 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 7 Mar 2024 17:40:55 -0500 Subject: [PATCH 33/57] wrong file name... --- corehq/apps/sso/utils/context_helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/corehq/apps/sso/utils/context_helpers.py b/corehq/apps/sso/utils/context_helpers.py index 4a1e41ccb8cb..eb3099f3147c 100644 --- a/corehq/apps/sso/utils/context_helpers.py +++ b/corehq/apps/sso/utils/context_helpers.py @@ -74,8 +74,8 @@ def get_graph_api_connection_issue_email_context(idp, error): } body_html, body_txt = render_multiple_to_strings( template_context, - "sso/email/microsoft_graph_api_connection_issue_email_context.html", - "sso/email/microsoft_graph_api_connection_issue_email_context.txt", + "sso/email/microsoft_graph_api_connection_issue_notification.html", + "sso/email/microsoft_graph_api_connection_issue_notification.txt", ) email_context = { "subject": subject, From 9fdd9f1ed5e75c001f45468ea850204e46d22f0a Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 7 Mar 2024 17:52:55 -0500 Subject: [PATCH 34/57] Add baseurl as it is required by the template --- corehq/apps/sso/utils/context_helpers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/corehq/apps/sso/utils/context_helpers.py b/corehq/apps/sso/utils/context_helpers.py index eb3099f3147c..e435f27fe116 100644 --- a/corehq/apps/sso/utils/context_helpers.py +++ b/corehq/apps/sso/utils/context_helpers.py @@ -71,6 +71,7 @@ def get_graph_api_connection_issue_email_context(idp, error): template_context = { "error": error, "contact_email": settings.ACCOUNTS_EMAIL, + "base_url": get_site_domain(), } body_html, body_txt = render_multiple_to_strings( template_context, From 9128522fe0edb09107879367efc6cfb52478079b Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 7 Mar 2024 18:10:10 -0500 Subject: [PATCH 35/57] Add email context utility for deactivation skip notifaction --- corehq/apps/sso/tasks.py | 38 ++++++++++++------- .../sso_deactivation_skip_notification.html | 34 +++++++++++++++++ .../sso_deactivation_skip_notification.txt | 17 +++++++++ corehq/apps/sso/utils/context_helpers.py | 25 ++++++++++++ 4 files changed, 101 insertions(+), 13 deletions(-) create mode 100644 corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html create mode 100644 corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 5fe8584dd253..9b9947ace5b4 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -3,8 +3,6 @@ from celery.schedules import crontab -from django.utils.translation import gettext as _ - from corehq.apps.celery import periodic_task from corehq.apps.hqwebapp.tasks import send_html_email_async from corehq.apps.sso.models import ( @@ -16,9 +14,11 @@ ) from corehq.apps.sso.utils.context_helpers import ( get_idp_cert_expiration_email_context, + get_sso_deactivation_skip_email_context, ) from corehq.apps.sso.utils.user_helpers import get_email_domain_from_username from corehq.apps.users.models import WebUser +from dimagi.utils.logging import notify_exception log = logging.getLogger(__name__) @@ -128,17 +128,29 @@ def auto_deactivate_removed_sso_users(): ).values_list('username', flat=True) if len(idp_users) == 0 and len(usernames_in_account) - len(exempt_usernames) > 3: - # Send email - subject = _("CommCare HQ Alert: Temporarily skipped automatic deactivation of SSO Web Users" - " (Remote User Management)") - recipient = idp.owner.enterprise_admin_emails - body = _("we have temporarily skipped automatic deactivation of Web Users because we are receiving " - "an empty list of Users from Azure AD") - send_html_email_async.delay( - subject, - recipient, - body, - ) + context = get_sso_deactivation_skip_email_context(idp) + if not context["to"]: + notify_exception(None, f"no admin email addresses for IdP: {idp}") + try: + for send_to in context["to"]: + send_html_email_async.delay( + context["subject"], + send_to, + context["html"], + text_content=context["plaintext"], + email_from=context["from"], + bcc=context["bcc"], + ) + log.info( + "Sent sso user deactivation skipped notification" + "email for %(idp_name)s to %(send_to)s." % { + "idp_name": idp.name, + "send_to": send_to, + } + ) + except Exception as exc: + notify_exception(None, f"Failed to send sso user deactivation skipped notification email for" + f" IdP {idp}: {exc!s}", exc_info=True) return usernames_to_deactivate = [] diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html new file mode 100644 index 000000000000..d7f2431693e0 --- /dev/null +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html @@ -0,0 +1,34 @@ +{% load i18n %} +

{% blocktrans %}Dear enterprise administrator,{% endblocktrans %}

+ +

+ {% blocktrans %} + 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. + {% endblocktrans %} +

+ +

+ {% blocktrans %} + Please check the configuration of the Remote User Management settings in your Identity Provider. + {% endblocktrans %} +

+ +

+ {% blocktrans %} + 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. + {% endblocktrans %} +

+ + +

+ {% blocktrans %} + Best regards, + {% endblocktrans %} +

+ +

+ {% blocktrans %}The CommCare HQ Team{% endblocktrans %}
+ {{ base_url }} +

\ No newline at end of file diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt new file mode 100644 index 000000000000..749c2400aa7e --- /dev/null +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt @@ -0,0 +1,17 @@ +{% load i18n %} +{% blocktrans %} +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 +{{ base_url }} +{% endblocktrans %} \ No newline at end of file diff --git a/corehq/apps/sso/utils/context_helpers.py b/corehq/apps/sso/utils/context_helpers.py index e435f27fe116..68a2d3b6c2c1 100644 --- a/corehq/apps/sso/utils/context_helpers.py +++ b/corehq/apps/sso/utils/context_helpers.py @@ -89,3 +89,28 @@ def get_graph_api_connection_issue_email_context(idp, error): if idp.owner.dimagi_contact: email_context["bcc"].append(idp.owner.dimagi_contact) return email_context + + +def get_sso_deactivation_skip_email_context(idp): + subject = _("CommCare HQ Alert: Temporarily skipped automatic deactivation of SSO Web Users" + " (Remote User Management)") + template_context = { + "contact_email": settings.ACCOUNTS_EMAIL, + "base_url": get_site_domain(), + } + body_html, body_txt = render_multiple_to_strings( + template_context, + "sso/email/sso_deactivation_skip_notification.html", + "sso/email/sso_deactivation_skip_notification.txt", + ) + email_context = { + "subject": subject, + "from": _(f"Dimagi CommCare Accounts <{settings.ACCOUNTS_EMAIL}>"), + "to": idp.owner.enterprise_admin_emails, + "bcc": [settings.ACCOUNTS_EMAIL], + "html": body_html, + "plaintext": body_txt, + } + if idp.owner.dimagi_contact: + email_context["bcc"].append(idp.owner.dimagi_contact) + return email_context From bbf62ee332b5ed43cac022020d22c7d2627ef42f Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 7 Mar 2024 18:12:12 -0500 Subject: [PATCH 36/57] handle error if no admin email --- corehq/apps/sso/models.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index e5a93e025f63..11d220e6a680 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -454,6 +454,8 @@ def get_all_members_of_the_idp(self): notify_exception(None, f"Failed to get members of the IdP. {str(e)}") context = get_graph_api_connection_issue_email_context(self, str(e)) + if not context["to"]: + notify_exception(None, f"no admin email addresses for IdP: {self}") try: for send_to in context["to"]: send_html_email_async.delay( From 9e871028b9474127c75e96287df074446cfd57ce Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 7 Mar 2024 18:17:37 -0500 Subject: [PATCH 37/57] Remove redundant comments for self-explanatory code --- corehq/apps/sso/tasks.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 9b9947ace5b4..01937d59821b 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -117,9 +117,7 @@ def auto_deactivate_removed_sso_users(): enable_user_deactivation=True, idp_type=IdentityProviderType.AZURE_AD ).all(): - # Fetch a list of users usernames that are members of the idp idp_users = idp.get_all_members_of_the_idp() - # Fetch a list of active WebUser usernames that are members of project spaces associated with the idp usernames_in_account = idp.owner.get_web_users(info_type='username') # Get criteria for exempting usernames and email domains from the deactivation list From 030eb789d288dd76863fe48a9ad0ac3c75329d38 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 7 Mar 2024 18:31:50 -0500 Subject: [PATCH 38/57] Wrong intendation --- corehq/apps/sso/tasks.py | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 01937d59821b..a3ebd3a28126 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -129,26 +129,26 @@ def auto_deactivate_removed_sso_users(): context = get_sso_deactivation_skip_email_context(idp) if not context["to"]: notify_exception(None, f"no admin email addresses for IdP: {idp}") - try: - for send_to in context["to"]: - send_html_email_async.delay( - context["subject"], - send_to, - context["html"], - text_content=context["plaintext"], - email_from=context["from"], - bcc=context["bcc"], - ) - log.info( - "Sent sso user deactivation skipped notification" - "email for %(idp_name)s to %(send_to)s." % { - "idp_name": idp.name, - "send_to": send_to, - } - ) - except Exception as exc: - notify_exception(None, f"Failed to send sso user deactivation skipped notification email for" - f" IdP {idp}: {exc!s}", exc_info=True) + try: + for send_to in context["to"]: + send_html_email_async.delay( + context["subject"], + send_to, + context["html"], + text_content=context["plaintext"], + email_from=context["from"], + bcc=context["bcc"], + ) + log.info( + "Sent sso user deactivation skipped notification" + "email for %(idp_name)s to %(send_to)s." % { + "idp_name": idp.name, + "send_to": send_to, + } + ) + except Exception as exc: + notify_exception(None, f"Failed to send sso user deactivation skipped notification email for" + f" IdP {idp}: {exc!s}", exc_info=True) return usernames_to_deactivate = [] From e515ddada9fd16c2012217992ecb67383199a095 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 7 Mar 2024 18:54:57 -0500 Subject: [PATCH 39/57] Have a constant for the minimum active user threshold --- corehq/apps/sso/tasks.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index a3ebd3a28126..558332efed08 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -125,7 +125,9 @@ def auto_deactivate_removed_sso_users(): exempt_usernames = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains ).values_list('username', flat=True) - if len(idp_users) == 0 and len(usernames_in_account) - len(exempt_usernames) > 3: + # if the Graph Users API returns an empty list of users and the account still have active web users + MIN_ACTIVE_USERS_THRESHOLD = 3 + if len(idp_users) == 0 and len(usernames_in_account) - len(exempt_usernames) > MIN_ACTIVE_USERS_THRESHOLD: context = get_sso_deactivation_skip_email_context(idp) if not context["to"]: notify_exception(None, f"no admin email addresses for IdP: {idp}") From e52b6bcd202b5658dcbf15c01e8b169d8795e43e Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 21 Mar 2024 14:41:04 -0400 Subject: [PATCH 40/57] Rename Azure to Entra --- corehq/apps/sso/tasks.py | 2 +- corehq/apps/sso/tests/test_tasks.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index b98ddc1d80db..1ed8005ca1d2 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -117,7 +117,7 @@ def send_idp_cert_expires_reminder_emails(num_days): def auto_deactivate_removed_sso_users(): for idp in IdentityProvider.objects.filter( enable_user_deactivation=True, - idp_type=IdentityProviderType.AZURE_AD + idp_type=IdentityProviderType.ENTRA_ID ).all(): idp_users = idp.get_all_members_of_the_idp() usernames_in_account = idp.owner.get_web_users(info_type='username') diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index d134b0cde00e..337cb0d4025f 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -199,7 +199,7 @@ def setUpClass(cls): cls.idp = generator.create_idp('vaultwax', cls.account) cls.idp.enable_user_deactivation = True - cls.idp.idp_type = IdentityProviderType.AZURE_AD + cls.idp.idp_type = IdentityProviderType.ENTRA_ID cls.idp.save() cls.email_domain = AuthenticatedEmailDomain.objects.create( email_domain='vaultwax.com', @@ -242,7 +242,7 @@ def test_sso_exempt_users_are_not_deactivated(self): self.assertTrue(web_user.is_active) @patch('corehq.apps.sso.tasks.send_html_email_async.delay') - def test_deactivation_skipped_if_azure_return_empty_sso_user(self, mock_send): + def test_deactivation_skipped_if_entra_return_empty_sso_user(self, mock_send): self.mock_get_all_members_of_the_idp.return_value = [] auto_deactivate_removed_sso_users() From 66b26a40d36868585cf193b5630dc43efa7fcf22 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 26 Mar 2024 17:36:06 -0400 Subject: [PATCH 41/57] Remove the threshold for skip auto deactivation --- corehq/apps/sso/tasks.py | 5 ++--- corehq/apps/sso/tests/test_tasks.py | 2 -- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 35fbc9c1f7fb..42573d0acc5a 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -133,9 +133,8 @@ def auto_deactivate_removed_sso_users(): exempt_usernames = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains ).values_list('username', flat=True) - # if the Graph Users API returns an empty list of users and the account still have active web users - MIN_ACTIVE_USERS_THRESHOLD = 3 - if len(idp_users) == 0 and len(usernames_in_account) - len(exempt_usernames) > MIN_ACTIVE_USERS_THRESHOLD: + # if the Graph Users API returns an empty list of users we will skip auto deactivation + if len(idp_users) == 0: context = get_sso_deactivation_skip_email_context(idp) if not context["to"]: notify_exception(None, f"no admin email addresses for IdP: {idp}") diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index aa548271b5c2..263c6d50517c 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -342,8 +342,6 @@ def setUp(self): self.web_user_a = self._create_web_user('a@vaultwax.com') self.web_user_b = self._create_web_user('b@vaultwax.com') self.web_user_c = self._create_web_user('c@vaultwax.com') - # web_user_d is required so the total number of IdP user meet the threshold for auto-deactivation - self.web_user_d = self._create_web_user('d@vaultwax.com') def test_user_is_deactivated_if_not_member_of_idp(self): self.assertTrue(self.web_user_c.is_active) From 316ef728595b4d25e33290876589c670f31cd1d4 Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Wed, 27 Mar 2024 13:48:45 -0400 Subject: [PATCH 42/57] Text update Co-authored-by: Biyeun Co-authored-by: Graham Herceg --- .../microsoft_graph_api_connection_issue_notification.html | 2 +- .../microsoft_graph_api_connection_issue_notification.txt | 2 +- .../sso/email/sso_deactivation_skip_notification.html | 4 ++-- .../sso/email/sso_deactivation_skip_notification.txt | 4 ++-- corehq/apps/sso/utils/context_helpers.py | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html index 950d7b3f9958..ddfae9c8be5c 100644 --- a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html +++ b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html @@ -16,7 +16,7 @@

{% blocktrans %} - Please check the configuration of the Remote User Management settings in your Identity Provider. + Please check the configuration for Remote User Management in your Identity Provider settings. {% endblocktrans %}

diff --git a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt index 0607f6c28455..8512ce742976 100644 --- a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt +++ b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt @@ -8,7 +8,7 @@ deactivate SSO Web Users on CommCare HQ that have been removed from your Microso This was the error message: {{ error }} -Please check the configuration of the Remote User Management settings in your Identity Provider. +Please check the configuration for Remote User Management in your Identity Provider settings. If you have any questions, please don’t hesitate to contact {{ contact_email }}. Thank you for your use and support of CommCare. diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html index d7f2431693e0..715240593397 100644 --- a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html @@ -3,14 +3,14 @@

{% blocktrans %} - We have temporarily skipped automatic deactivate of SSO Web Users + We have temporarily skipped automatic deactivation of SSO-managed Web Users because we are receiving an empty list of users from your Microsoft Entra ID instance. {% endblocktrans %}

{% blocktrans %} - Please check the configuration of the Remote User Management settings in your Identity Provider. + Please check the configuration for Remote User Management in your Identity Provider settings. {% endblocktrans %}

diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt index 749c2400aa7e..a0cb4642fc7f 100644 --- a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt @@ -2,10 +2,10 @@ {% blocktrans %} Dear enterprise administrator, -We have temporarily skipped automatic deactivate of SSO Web Users +We have temporarily skipped automatic deactivation of SSO-managed 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. +Please check the configuration for Remote User Management in your Identity Provider settings. 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. diff --git a/corehq/apps/sso/utils/context_helpers.py b/corehq/apps/sso/utils/context_helpers.py index d98e7ac0ec9a..f90527788fbe 100644 --- a/corehq/apps/sso/utils/context_helpers.py +++ b/corehq/apps/sso/utils/context_helpers.py @@ -67,7 +67,7 @@ def get_idp_cert_expiration_email_context(idp): def get_graph_api_connection_issue_email_context(idp, error): - subject = _("CommCare HQ Alert: SSO Remote User Management, Issue Connecting to Microsoft Graph API") + subject = _("CommCare HQ Alert: SSO Remote User Management - Issue Connecting to Microsoft Graph API") template_context = { "error": error, "contact_email": settings.ACCOUNTS_EMAIL, From d1fbad1ae3f7805b848d7c22457a5e0b95e9b929 Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Wed, 27 Mar 2024 16:35:04 -0400 Subject: [PATCH 43/57] Update text Co-authored-by: Biyeun --- .../templates/sso/email/sso_deactivation_skip_notification.html | 2 +- .../templates/sso/email/sso_deactivation_skip_notification.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html index 715240593397..ac6b1c30d679 100644 --- a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html @@ -4,7 +4,7 @@

{% blocktrans %} We have temporarily skipped automatic deactivation of SSO-managed Web Users - because we are receiving an empty list of users from your Microsoft Entra ID instance. + because we received an empty list of users from your Microsoft Entra ID instance. {% endblocktrans %}

diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt index a0cb4642fc7f..db580cd62eb6 100644 --- a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt @@ -3,7 +3,7 @@ Dear enterprise administrator, We have temporarily skipped automatic deactivation of SSO-managed Web Users -because we are receiving an empty list of users from your Microsoft Entra ID instance. +because we received an empty list of users from your Microsoft Entra ID instance. Please check the configuration for Remote User Management in your Identity Provider settings. From 0473337f1290ffc09fc1435f61a66e90556ab8a1 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 27 Mar 2024 17:01:02 -0400 Subject: [PATCH 44/57] Refactor: better query web users by domain --- corehq/apps/accounting/models.py | 24 +++++++++++------------- corehq/apps/accounting/tasks.py | 3 +-- corehq/apps/sso/tasks.py | 2 +- corehq/apps/users/dbaccessors.py | 4 ++++ 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/corehq/apps/accounting/models.py b/corehq/apps/accounting/models.py index 2247d5ec3e29..f68812c9086b 100644 --- a/corehq/apps/accounting/models.py +++ b/corehq/apps/accounting/models.py @@ -22,6 +22,7 @@ from corehq.apps.accounting.utils.stripe import charge_through_stripe from corehq.apps.domain.shortcuts import publish_domain_saved +from corehq.apps.users.dbaccessors import get_active_web_usernames_by_domain, get_web_user_count from dimagi.ext.couchdbkit import ( BooleanProperty, DateTimeProperty, @@ -603,25 +604,22 @@ def _send_autopay_card_added_email(self, domain): use_domain_gateway=True, ) - def get_web_users(self, info_type='id'): - """ - Get a set of web user information in the billing account. - - :param info_type: Specify 'id' to get user IDs, or 'username' to get user usernames. - :return: A set of user IDs or usernames, based on the info_type parameter. - """ + def get_web_user_usernames(self): domains = self.get_domains() web_users = set() - if info_type == 'id': - for domain in domains: - web_users.update(WebUser.ids_by_domain(domain)) - elif info_type == 'username': - for domain in domains: - web_users.update(user.username for user in WebUser.by_domain(domain)) + for domain in domains: + web_users.update(get_active_web_usernames_by_domain(domain)) return web_users + def get_web_user_count(self): + domains = self.get_domains() + count = 0 + for domain in domains: + count += get_web_user_count(domain, include_inactive=False) + return count + @staticmethod def should_show_sms_billable_report(domain): account = BillingAccount.get_account_by_domain(domain) diff --git a/corehq/apps/accounting/tasks.py b/corehq/apps/accounting/tasks.py index 557e4e9f36cd..64377ef974ff 100644 --- a/corehq/apps/accounting/tasks.py +++ b/corehq/apps/accounting/tasks.py @@ -832,8 +832,7 @@ def calculate_web_users_in_all_billing_accounts(today=None): today = today or datetime.date.today() for account in BillingAccount.objects.all(): record_date = today - relativedelta(days=1) - web_user_in_account = account.get_web_users() - num_users = len(web_user_in_account) + num_users = account.get_web_user_count() try: BillingAccountWebUserHistory.objects.create( billing_account=account, diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 42573d0acc5a..27df6f18322b 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -126,7 +126,7 @@ def auto_deactivate_removed_sso_users(): idp_type=IdentityProviderType.ENTRA_ID ).all(): idp_users = idp.get_all_members_of_the_idp() - usernames_in_account = idp.owner.get_web_users(info_type='username') + usernames_in_account = idp.owner.get_web_user_usernames() # Get criteria for exempting usernames and email domains from the deactivation list authenticated_domains = AuthenticatedEmailDomain.objects.filter(identity_provider=idp) diff --git a/corehq/apps/users/dbaccessors.py b/corehq/apps/users/dbaccessors.py index 8bf641462606..8d5d43077976 100644 --- a/corehq/apps/users/dbaccessors.py +++ b/corehq/apps/users/dbaccessors.py @@ -221,6 +221,10 @@ def get_all_user_id_username_pairs_by_domain(domain, include_web_users=True, inc )) +def get_active_web_usernames_by_domain(domain): + return (row['key'][3] for row in get_all_user_rows(domain, include_mobile_users=False, include_inactive=False)) + + def get_web_user_count(domain, include_inactive=True): return sum([ row['value'] From f938d82981d5acf472185436ea0685b7cd414df8 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Thu, 28 Mar 2024 09:21:40 -0400 Subject: [PATCH 45/57] Remove unnecessary try catch --- corehq/apps/sso/models.py | 38 ++++++++++-------------- corehq/apps/sso/tasks.py | 38 ++++++++++-------------- corehq/apps/sso/utils/context_helpers.py | 4 +-- 3 files changed, 34 insertions(+), 46 deletions(-) diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index 5781e80f105e..70f2eebf55fb 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -468,28 +468,22 @@ def get_all_members_of_the_idp(self): notify_exception(None, f"Failed to get members of the IdP. {str(e)}") context = get_graph_api_connection_issue_email_context(self, str(e)) - if not context["to"]: - notify_exception(None, f"no admin email addresses for IdP: {self}") - try: - for send_to in context["to"]: - send_html_email_async.delay( - context["subject"], - send_to, - context["html"], - text_content=context["plaintext"], - email_from=context["from"], - bcc=context["bcc"], - ) - log.info( - "Sent Microsoft Graph API connection issue email " - "for %(idp_name)s to %(send_to)s." % { - "idp_name": self.name, - "send_to": send_to, - } - ) - except Exception as exc: - notify_exception(None, f"Failed to send Microsoft Graph API connection issue" - f" for IdP {self.name}: {exc!s}", exc_info=True) + for send_to in context["to"]: + send_html_email_async.delay( + context["subject"], + send_to, + context["html"], + text_content=context["plaintext"], + email_from=context["from"], + bcc=context["bcc"], + ) + log.info( + "Sent Microsoft Graph API connection issue email " + "for %(idp_name)s to %(send_to)s." % { + "idp_name": self.name, + "send_to": send_to, + } + ) @receiver(post_save, sender=Subscription) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 27df6f18322b..68efd9c72322 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -136,28 +136,22 @@ def auto_deactivate_removed_sso_users(): # if the Graph Users API returns an empty list of users we will skip auto deactivation if len(idp_users) == 0: context = get_sso_deactivation_skip_email_context(idp) - if not context["to"]: - notify_exception(None, f"no admin email addresses for IdP: {idp}") - try: - for send_to in context["to"]: - send_html_email_async.delay( - context["subject"], - send_to, - context["html"], - text_content=context["plaintext"], - email_from=context["from"], - bcc=context["bcc"], - ) - log.info( - "Sent sso user deactivation skipped notification" - "email for %(idp_name)s to %(send_to)s." % { - "idp_name": idp.name, - "send_to": send_to, - } - ) - except Exception as exc: - notify_exception(None, f"Failed to send sso user deactivation skipped notification email for" - f" IdP {idp}: {exc!s}", exc_info=True) + for send_to in context["to"]: + send_html_email_async.delay( + context["subject"], + send_to, + context["html"], + text_content=context["plaintext"], + email_from=context["from"], + bcc=context["bcc"], + ) + log.info( + "Sent sso user deactivation skipped notification" + "email for %(idp_name)s to %(send_to)s." % { + "idp_name": idp.name, + "send_to": send_to, + } + ) return usernames_to_deactivate = [] diff --git a/corehq/apps/sso/utils/context_helpers.py b/corehq/apps/sso/utils/context_helpers.py index f90527788fbe..0c8248dedcc3 100644 --- a/corehq/apps/sso/utils/context_helpers.py +++ b/corehq/apps/sso/utils/context_helpers.py @@ -81,7 +81,7 @@ def get_graph_api_connection_issue_email_context(idp, error): email_context = { "subject": subject, "from": _(f"Dimagi CommCare Accounts <{settings.ACCOUNTS_EMAIL}>"), - "to": idp.owner.enterprise_admin_emails, + "to": idp.owner.enterprise_admin_emails or idp.owner.dimagi_contact or settings.ACCOUNT_EMAIL, "bcc": [settings.ACCOUNTS_EMAIL], "html": body_html, "plaintext": body_txt, @@ -106,7 +106,7 @@ def get_sso_deactivation_skip_email_context(idp): email_context = { "subject": subject, "from": _(f"Dimagi CommCare Accounts <{settings.ACCOUNTS_EMAIL}>"), - "to": idp.owner.enterprise_admin_emails, + "to": idp.owner.enterprise_admin_emails or idp.owner.dimagi_contact or settings.ACCOUNT_EMAIL, "bcc": [settings.ACCOUNTS_EMAIL], "html": body_html, "plaintext": body_txt, From 0c36b5e711029bcf3b4994ef6495fc369b6d7035 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Mon, 1 Apr 2024 23:22:58 -0400 Subject: [PATCH 46/57] One auto deactivation skipped email for different failure reason --- corehq/apps/sso/exceptions.py | 10 +++ corehq/apps/sso/models.py | 66 ++--------------- corehq/apps/sso/tasks.py | 73 +++++++++++++------ ...aph_api_connection_issue_notification.html | 40 ---------- ...raph_api_connection_issue_notification.txt | 20 ----- .../sso_deactivation_skip_notification.html | 4 +- .../sso_deactivation_skip_notification.txt | 4 +- corehq/apps/sso/utils/context_helpers.py | 3 +- corehq/apps/sso/utils/entra.py | 48 ++++++++++++ 9 files changed, 120 insertions(+), 148 deletions(-) delete mode 100644 corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html delete mode 100644 corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt create mode 100644 corehq/apps/sso/utils/entra.py diff --git a/corehq/apps/sso/exceptions.py b/corehq/apps/sso/exceptions.py index 2708ec7b1274..247fe0d09177 100644 --- a/corehq/apps/sso/exceptions.py +++ b/corehq/apps/sso/exceptions.py @@ -20,3 +20,13 @@ def __init__(self, error_code, message=None): super().__init__() self.code = error_code self.message = message + + +class EntraVerificationFailed(Exception): + def __init__(self, error_code, message): + super().__init__(f"{error_code}: {message}") + self.code = error_code + self.message = message + + def __str__(self): + return f"EntraVerificationFailed({self.error_code}, {self.message})" diff --git a/corehq/apps/sso/models.py b/corehq/apps/sso/models.py index 70f2eebf55fb..31b44bdd4a90 100644 --- a/corehq/apps/sso/models.py +++ b/corehq/apps/sso/models.py @@ -1,5 +1,4 @@ import logging -import requests from django.db import models from django.db.models.signals import post_save, post_delete @@ -8,13 +7,11 @@ from django.utils.translation import gettext_lazy from corehq.apps.accounting.models import BillingAccount, Subscription -from corehq.apps.hqwebapp.tasks import send_html_email_async from corehq.apps.sso import certificates from corehq.apps.sso.exceptions import ServiceProviderCertificateError -from corehq.apps.sso.utils.context_helpers import get_graph_api_connection_issue_email_context +from corehq.apps.sso.utils.entra import get_all_members_of_the_idp_from_entra from corehq.apps.sso.utils.user_helpers import get_email_domain_from_username from corehq.util.quickcache import quickcache -from dimagi.utils.logging import notify_exception log = logging.getLogger(__name__) @@ -427,63 +424,10 @@ def get_required_identity_provider(cls, username): return None def get_all_members_of_the_idp(self): - import msal - authority_base_url = "https://login.microsoftonline.com/" - authority = f"{authority_base_url}{self.api_host}" - - config = { - "authority": authority, - "client_id": self.api_id, - "scope": ["https://graph.microsoft.com/.default"], - "secret": self.api_secret, - "endpoint": "https://graph.microsoft.com/v1.0/users" - } - - try: - # Create a preferably long-lived app instance which maintains a token cache. - app = msal.ConfidentialClientApplication( - config["client_id"], authority=config["authority"], - client_credential=config["secret"], - ) - # looks up a token from cache - result = app.acquire_token_silent(config["scope"], account=None) - if not result: - result = app.acquire_token_for_client(scopes=config["scope"]) - if "access_token" in result: - # Calling graph using the access token - response = requests.get( - config["endpoint"], - headers={'Authorization': 'Bearer ' + result['access_token']}, - ) - response.raise_for_status() # Raises an error for bad status - graph_data = response.json() - - user_principal_names = [user["userPrincipalName"] for user in graph_data["value"]] - return user_principal_names - else: - error_message = f"Error acquiring Microsoft Graph API token: {result.get('error')}, Description: " - f"{result.get('error_description')}, Correlation ID: {result.get('correlation_id')}" - raise Exception(error_message) - except Exception as e: - notify_exception(None, f"Failed to get members of the IdP. {str(e)}") - - context = get_graph_api_connection_issue_email_context(self, str(e)) - for send_to in context["to"]: - send_html_email_async.delay( - context["subject"], - send_to, - context["html"], - text_content=context["plaintext"], - email_from=context["from"], - bcc=context["bcc"], - ) - log.info( - "Sent Microsoft Graph API connection issue email " - "for %(idp_name)s to %(send_to)s." % { - "idp_name": self.name, - "send_to": send_to, - } - ) + if self.idp_type == IdentityProviderType.ENTRA_ID: + return get_all_members_of_the_idp_from_entra(self) + else: + raise NotImplementedError("Not implemented") @receiver(post_save, sender=Subscription) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 68efd9c72322..0ee14c0d2991 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -1,10 +1,12 @@ import datetime import logging +import requests from celery.schedules import crontab from corehq.apps.celery import periodic_task, task from corehq.apps.hqwebapp.tasks import send_html_email_async +from corehq.apps.sso.exceptions import EntraVerificationFailed from corehq.apps.sso.models import ( AuthenticatedEmailDomain, IdentityProvider, @@ -17,6 +19,7 @@ get_idp_cert_expiration_email_context, get_sso_deactivation_skip_email_context, ) +from corehq.apps.sso.utils.entra import MSGraphIssue from corehq.apps.sso.utils.user_helpers import get_email_domain_from_username from corehq.apps.users.models import WebUser from corehq.apps.users.models import HQApiKey @@ -24,6 +27,7 @@ from corehq.sql_db.util import paginate_query from django.db import router from django.db.models import Q +from django.utils.translation import gettext as _ from dimagi.utils.chunked import chunked from dimagi.utils.logging import notify_exception @@ -125,7 +129,24 @@ def auto_deactivate_removed_sso_users(): enable_user_deactivation=True, idp_type=IdentityProviderType.ENTRA_ID ).all(): - idp_users = idp.get_all_members_of_the_idp() + try: + idp_users = idp.get_all_members_of_the_idp() + except EntraVerificationFailed as e: + notify_exception(None, f"Failed to get members of the IdP. {str(e)}") + send_deactivation_skipped_email(failure_reason=MSGraphIssue.VERIFICATION_ERROR, + error_code=EntraVerificationFailed.code, + error_description=EntraVerificationFailed.message) + continue + except requests.exceptions.HTTPError as e: + notify_exception(None, f"Failed to get members of the IdP. {str(e)}") + send_deactivation_skipped_email(failure_reason=MSGraphIssue.HTTP_ERROR) + continue + + # if the Graph Users API returns an empty list of users we will skip auto deactivation + if len(idp_users) == 0: + send_deactivation_skipped_email(failure_code=MSGraphIssue.EMPTY_ERROR) + continue + usernames_in_account = idp.owner.get_web_user_usernames() # Get criteria for exempting usernames and email domains from the deactivation list @@ -133,27 +154,6 @@ def auto_deactivate_removed_sso_users(): exempt_usernames = UserExemptFromSingleSignOn.objects.filter(email_domain__in=authenticated_domains ).values_list('username', flat=True) - # if the Graph Users API returns an empty list of users we will skip auto deactivation - if len(idp_users) == 0: - context = get_sso_deactivation_skip_email_context(idp) - for send_to in context["to"]: - send_html_email_async.delay( - context["subject"], - send_to, - context["html"], - text_content=context["plaintext"], - email_from=context["from"], - bcc=context["bcc"], - ) - log.info( - "Sent sso user deactivation skipped notification" - "email for %(idp_name)s to %(send_to)s." % { - "idp_name": idp.name, - "send_to": send_to, - } - ) - return - usernames_to_deactivate = [] authenticated_email_domains = authenticated_domains.values_list('email_domain', flat=True) @@ -171,6 +171,35 @@ def auto_deactivate_removed_sso_users(): user.save() +def send_deactivation_skipped_email(idp, failure_code, error_code=None, error_description=None): + if failure_code == MSGraphIssue.VERIFICATION_ERROR: + failure_reason = _("There was an issue connecting to the Microsoft Graph API. " + f"Error code: {error_code}. Error description: {error_description}") + elif failure_code == MSGraphIssue.HTTP_ERROR: + failure_reason = _("An HTTP error occured when connecting to the Microsoft Graph API, which usually" + "indicates an issue with Microsoft's servers.") + elif failure_code == MSGraphIssue.EMPTY_ERROR: + failure_reason = _("We received an empty list of users from your Microsoft Entra ID instance.") + + context = get_sso_deactivation_skip_email_context(idp, failure_reason) + for send_to in context["to"]: + send_html_email_async.delay( + context["subject"], + send_to, + context["html"], + text_content=context["plaintext"], + email_from=context["from"], + bcc=context["bcc"], + ) + log.info( + "Sent sso user deactivation skipped notification" + "email for %(idp_name)s to %(send_to)s." % { + "idp_name": idp.name, + "send_to": send_to, + } + ) + + @task(bind=True, default_retry_delay=15 * 60, max_retries=10, acks_late=True) def update_sso_user_api_key_expiration_dates(self, identity_provider_id): idp = IdentityProvider.objects.get(id=identity_provider_id) diff --git a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html deleted file mode 100644 index ddfae9c8be5c..000000000000 --- a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.html +++ /dev/null @@ -1,40 +0,0 @@ -{% load i18n %} -

{% blocktrans %}Dear enterprise administrator,{% endblocktrans %}

- -

- {% blocktrans %} - 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. - {% endblocktrans %} -

- -

- {% blocktrans %} - This was the error message: {{ error }} - {% endblocktrans %} -

- -

- {% blocktrans %} - Please check the configuration for Remote User Management in your Identity Provider settings. - {% endblocktrans %} -

- -

- {% blocktrans %} - If you have any questions, please don’t hesitate to contact - {{ contact_email }}. Thank you for your use and support of CommCare. - {% endblocktrans %} -

- - -

- {% blocktrans %} - Best regards, - {% endblocktrans %} -

- -

- {% blocktrans %}The CommCare HQ Team{% endblocktrans %}
- {{ base_url }} -

diff --git a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt b/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt deleted file mode 100644 index 8512ce742976..000000000000 --- a/corehq/apps/sso/templates/sso/email/microsoft_graph_api_connection_issue_notification.txt +++ /dev/null @@ -1,20 +0,0 @@ -{% load i18n %} -{% blocktrans %} -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 for Remote User Management in your Identity Provider settings. - -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 -{{ base_url }} -{% endblocktrans %} \ No newline at end of file diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html index ac6b1c30d679..d5fd3747ea60 100644 --- a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html @@ -3,8 +3,8 @@

{% blocktrans %} - We have temporarily skipped automatic deactivation of SSO-managed Web Users - because we received an empty list of users from your Microsoft Entra ID instance. + We have temporarily skipped automatic deactivation of SSO-managed Web Users for the following reason: + {{ failure_reason }} {% endblocktrans %}

diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt index db580cd62eb6..ac678033a560 100644 --- a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt @@ -2,8 +2,8 @@ {% blocktrans %} Dear enterprise administrator, -We have temporarily skipped automatic deactivation of SSO-managed Web Users -because we received an empty list of users from your Microsoft Entra ID instance. +We have temporarily skipped automatic deactivation of SSO-managed Web Users for the following reason: +{{ failure_reason }} Please check the configuration for Remote User Management in your Identity Provider settings. diff --git a/corehq/apps/sso/utils/context_helpers.py b/corehq/apps/sso/utils/context_helpers.py index 0c8248dedcc3..1dcb60e97ea1 100644 --- a/corehq/apps/sso/utils/context_helpers.py +++ b/corehq/apps/sso/utils/context_helpers.py @@ -91,12 +91,13 @@ def get_graph_api_connection_issue_email_context(idp, error): return email_context -def get_sso_deactivation_skip_email_context(idp): +def get_sso_deactivation_skip_email_context(idp, failure_reason): subject = _("CommCare HQ Alert: Temporarily skipped automatic deactivation of SSO Web Users" " (Remote User Management)") template_context = { "contact_email": settings.ACCOUNTS_EMAIL, "base_url": get_site_domain(), + "failure_reason": failure_reason, } body_html, body_txt = render_multiple_to_strings( template_context, diff --git a/corehq/apps/sso/utils/entra.py b/corehq/apps/sso/utils/entra.py new file mode 100644 index 000000000000..3001955ba0cb --- /dev/null +++ b/corehq/apps/sso/utils/entra.py @@ -0,0 +1,48 @@ +import requests + +from corehq.apps.sso.exceptions import EntraVerificationFailed + + +class MSGraphIssue: + HTTP_ERROR = "http_error" + VERIFICATION_ERROR = "verification_error" + EMPTY_ERROR = "empty_error" + + +def get_all_members_of_the_idp_from_entra(idp): + import msal + authority_base_url = "https://login.microsoftonline.com/" + authority = f"{authority_base_url}{idp.api_host}" + + config = { + "authority": authority, + "client_id": idp.api_id, + "scope": ["https://graph.microsoft.com/.default"], + "secret": idp.api_secret, + "endpoint": "https://graph.microsoft.com/v1.0/users" + } + + # Create a preferably long-lived app instance which maintains a token cache. + app = msal.ConfidentialClientApplication( + config["client_id"], authority=config["authority"], + client_credential=config["secret"], + ) + # looks up a token from cache + result = app.acquire_token_silent(config["scope"], account=None) + # try: + if not result: + result = app.acquire_token_for_client(scopes=config["scope"]) + if "access_token" in result: + # Calling graph using the access token + response = requests.get( + config["endpoint"], + headers={'Authorization': 'Bearer ' + result['access_token']}, + ) + response.raise_for_status() # Raises an error for bad status + graph_data = response.json() + + user_principal_names = [user["userPrincipalName"] for user in graph_data["value"]] + return user_principal_names + else: + raise EntraVerificationFailed(result.get('error', {}).get('code', 'Unknown'), + result.get('error', {}).get('message', 'No error message provided.')) From 5cb837bffeb88d73cd3efd45d99d5af9f6edc2bd Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Mon, 1 Apr 2024 23:52:39 -0400 Subject: [PATCH 47/57] Miss one parameter --- corehq/apps/sso/tasks.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 0ee14c0d2991..b98afcbbd18e 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -133,18 +133,18 @@ def auto_deactivate_removed_sso_users(): idp_users = idp.get_all_members_of_the_idp() except EntraVerificationFailed as e: notify_exception(None, f"Failed to get members of the IdP. {str(e)}") - send_deactivation_skipped_email(failure_reason=MSGraphIssue.VERIFICATION_ERROR, + send_deactivation_skipped_email(idp=idp, failure_reason=MSGraphIssue.VERIFICATION_ERROR, error_code=EntraVerificationFailed.code, error_description=EntraVerificationFailed.message) continue except requests.exceptions.HTTPError as e: notify_exception(None, f"Failed to get members of the IdP. {str(e)}") - send_deactivation_skipped_email(failure_reason=MSGraphIssue.HTTP_ERROR) + send_deactivation_skipped_email(idp=idp, failure_reason=MSGraphIssue.HTTP_ERROR) continue # if the Graph Users API returns an empty list of users we will skip auto deactivation if len(idp_users) == 0: - send_deactivation_skipped_email(failure_code=MSGraphIssue.EMPTY_ERROR) + send_deactivation_skipped_email(idp=idp, failure_code=MSGraphIssue.EMPTY_ERROR) continue usernames_in_account = idp.owner.get_web_user_usernames() From 7ae2fbbc2e54f8af2a2c2478e84b58b3292be96d Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 2 Apr 2024 17:16:58 -0400 Subject: [PATCH 48/57] Return user in an application --- corehq/apps/sso/utils/entra.py | 37 +++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/corehq/apps/sso/utils/entra.py b/corehq/apps/sso/utils/entra.py index 3001955ba0cb..f86b3f6c2104 100644 --- a/corehq/apps/sso/utils/entra.py +++ b/corehq/apps/sso/utils/entra.py @@ -1,3 +1,4 @@ +import json import requests from corehq.apps.sso.exceptions import EntraVerificationFailed @@ -19,7 +20,8 @@ def get_all_members_of_the_idp_from_entra(idp): "client_id": idp.api_id, "scope": ["https://graph.microsoft.com/.default"], "secret": idp.api_secret, - "endpoint": "https://graph.microsoft.com/v1.0/users" + "endpoint": f"https://graph.microsoft.com/v1.0/servicePrincipals(appId='{idp.api_id}')/" + "appRoleAssignedTo?$select=principalId, principalType" } # Create a preferably long-lived app instance which maintains a token cache. @@ -39,9 +41,38 @@ def get_all_members_of_the_idp_from_entra(idp): headers={'Authorization': 'Bearer ' + result['access_token']}, ) response.raise_for_status() # Raises an error for bad status - graph_data = response.json() + assignments = response.json() - user_principal_names = [user["userPrincipalName"] for user in graph_data["value"]] + # microsoft.graph.appRoleAssignment's property doesn't userPrincipalName + # Property principalType can either be User, Group or ServicePrincipal + principal_ids = {assignment["principalId"] for assignment in assignments["value"] + if assignment["principalType"] == "User"} + + # Prepare batch request + batch_payload = { + "requests": [ + { + "id": str(i), + "method": "GET", + "url": f"/users/{principal_id}?$select=userPrincipalName" + } for i, principal_id in enumerate(principal_ids) + ] + } + + # Send batch request + batch_response = requests.post( + 'https://graph.microsoft.com/v1.0/$batch', + headers={'Authorization': 'Bearer ' + result['access_token'], 'Content-Type': 'application/json'}, + data=json.dumps(batch_payload) + ) + batch_response.raise_for_status() + batch_result = batch_response.json() + + # Extract userPrincipalName from batch response + user_principal_names = [ + resp['body']['userPrincipalName'] for resp in batch_result['responses'] + if 'body' in resp and 'userPrincipalName' in resp['body'] + ] return user_principal_names else: raise EntraVerificationFailed(result.get('error', {}).get('code', 'Unknown'), From 3c8a976f888436363f98090be59966202a9050f6 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 2 Apr 2024 17:25:13 -0400 Subject: [PATCH 49/57] Add a new line at the end of the file --- .../templates/sso/email/sso_deactivation_skip_notification.html | 2 +- .../templates/sso/email/sso_deactivation_skip_notification.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html index d5fd3747ea60..d51578bad96d 100644 --- a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.html @@ -31,4 +31,4 @@

{% blocktrans %}The CommCare HQ Team{% endblocktrans %}
{{ base_url }} -

\ No newline at end of file +

diff --git a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt index ac678033a560..2b9c36ca835c 100644 --- a/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt +++ b/corehq/apps/sso/templates/sso/email/sso_deactivation_skip_notification.txt @@ -14,4 +14,4 @@ Best regards, The CommCare HQ Team {{ base_url }} -{% endblocktrans %} \ No newline at end of file +{% endblocktrans %} From 72bd477d4d4167634d9ae0bae8d82d2eabed05b0 Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Sat, 6 Apr 2024 20:02:31 -0400 Subject: [PATCH 50/57] email_context's "to" only accept iterable Co-authored-by: Graham Herceg --- corehq/apps/sso/utils/context_helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/corehq/apps/sso/utils/context_helpers.py b/corehq/apps/sso/utils/context_helpers.py index 1dcb60e97ea1..a417cbed1f39 100644 --- a/corehq/apps/sso/utils/context_helpers.py +++ b/corehq/apps/sso/utils/context_helpers.py @@ -81,7 +81,7 @@ def get_graph_api_connection_issue_email_context(idp, error): email_context = { "subject": subject, "from": _(f"Dimagi CommCare Accounts <{settings.ACCOUNTS_EMAIL}>"), - "to": idp.owner.enterprise_admin_emails or idp.owner.dimagi_contact or settings.ACCOUNT_EMAIL, + "to": idp.owner.enterprise_admin_emails or [idp.owner.dimagi_contact] or [settings.ACCOUNT_EMAIL], "bcc": [settings.ACCOUNTS_EMAIL], "html": body_html, "plaintext": body_txt, @@ -107,7 +107,7 @@ def get_sso_deactivation_skip_email_context(idp, failure_reason): email_context = { "subject": subject, "from": _(f"Dimagi CommCare Accounts <{settings.ACCOUNTS_EMAIL}>"), - "to": idp.owner.enterprise_admin_emails or idp.owner.dimagi_contact or settings.ACCOUNT_EMAIL, + "to": idp.owner.enterprise_admin_emails or [idp.owner.dimagi_contact] or [settings.ACCOUNT_EMAIL], "bcc": [settings.ACCOUNTS_EMAIL], "html": body_html, "plaintext": body_txt, From 674be9f4176fe1f39c8b04348be97cd4974203f4 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Sat, 6 Apr 2024 21:21:13 -0400 Subject: [PATCH 51/57] Remove wrong comment --- corehq/apps/sso/utils/entra.py | 1 - 1 file changed, 1 deletion(-) diff --git a/corehq/apps/sso/utils/entra.py b/corehq/apps/sso/utils/entra.py index f86b3f6c2104..d5c51805943a 100644 --- a/corehq/apps/sso/utils/entra.py +++ b/corehq/apps/sso/utils/entra.py @@ -31,7 +31,6 @@ def get_all_members_of_the_idp_from_entra(idp): ) # looks up a token from cache result = app.acquire_token_silent(config["scope"], account=None) - # try: if not result: result = app.acquire_token_for_client(scopes=config["scope"]) if "access_token" in result: From f14e6895d42030cfa8108f9c2123f579ed93c257 Mon Sep 17 00:00:00 2001 From: Jing Cheng <39149002+jingcheng16@users.noreply.github.com> Date: Tue, 16 Apr 2024 09:54:51 -0400 Subject: [PATCH 52/57] rename IdP to idp Co-authored-by: Biyeun --- corehq/apps/sso/tests/test_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/sso/tests/test_tasks.py b/corehq/apps/sso/tests/test_tasks.py index 263c6d50517c..cb267bd62068 100644 --- a/corehq/apps/sso/tests/test_tasks.py +++ b/corehq/apps/sso/tests/test_tasks.py @@ -382,7 +382,7 @@ def test_deactivation_skipped_if_entra_return_empty_sso_user(self, mock_send): self.assertTrue(web_user_c.is_active) mock_send.assert_called_once() - def test_deactivation_skip_members_of_the_domains_but_not_have_an_email_domain_controlled_by_the_IdP(self): + def test_deactivation_skip_members_of_the_domains_but_not_have_an_email_domain_controlled_by_the_idp(self): dimagi_user = self._create_web_user('superuser@dimagi.com') self.mock_get_all_members_of_the_idp.return_value = [self.web_user_a.username, self.web_user_b.username] From 586797be2b96d42897d4b2903facdbd327f0acb9 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 17 Apr 2024 16:53:21 -0400 Subject: [PATCH 53/57] Restructure EntraVerificationFailed 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'} --- corehq/apps/sso/exceptions.py | 8 ++++---- corehq/apps/sso/tasks.py | 6 +++--- corehq/apps/sso/utils/entra.py | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/corehq/apps/sso/exceptions.py b/corehq/apps/sso/exceptions.py index 247fe0d09177..77a347e55c91 100644 --- a/corehq/apps/sso/exceptions.py +++ b/corehq/apps/sso/exceptions.py @@ -23,10 +23,10 @@ def __init__(self, error_code, message=None): class EntraVerificationFailed(Exception): - def __init__(self, error_code, message): - super().__init__(f"{error_code}: {message}") - self.code = error_code + def __init__(self, error, message): + super().__init__(f"{error}: {message}") + self.error = error self.message = message def __str__(self): - return f"EntraVerificationFailed({self.error_code}, {self.message})" + return f"EntraVerificationFailed({self.error}, {self.message})" diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index b98afcbbd18e..adf2c30f74ff 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -134,7 +134,7 @@ def auto_deactivate_removed_sso_users(): except EntraVerificationFailed as e: 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_code=EntraVerificationFailed.code, + error=EntraVerificationFailed.error, error_description=EntraVerificationFailed.message) continue except requests.exceptions.HTTPError as e: @@ -171,10 +171,10 @@ def auto_deactivate_removed_sso_users(): user.save() -def send_deactivation_skipped_email(idp, failure_code, error_code=None, error_description=None): +def send_deactivation_skipped_email(idp, failure_code, error=None, error_description=None): if failure_code == MSGraphIssue.VERIFICATION_ERROR: failure_reason = _("There was an issue connecting to the Microsoft Graph API. " - f"Error code: {error_code}. Error description: {error_description}") + f"Error: {error}. Error description: {error_description}") elif failure_code == MSGraphIssue.HTTP_ERROR: failure_reason = _("An HTTP error occured when connecting to the Microsoft Graph API, which usually" "indicates an issue with Microsoft's servers.") diff --git a/corehq/apps/sso/utils/entra.py b/corehq/apps/sso/utils/entra.py index d5c51805943a..581486225d90 100644 --- a/corehq/apps/sso/utils/entra.py +++ b/corehq/apps/sso/utils/entra.py @@ -74,5 +74,5 @@ def get_all_members_of_the_idp_from_entra(idp): ] return user_principal_names else: - raise EntraVerificationFailed(result.get('error', {}).get('code', 'Unknown'), - result.get('error', {}).get('message', 'No error message provided.')) + raise EntraVerificationFailed(result.get('error', {}), + result.get('error_description', 'No error description provided')) From c4fdf0a1eee76a88b6ef2034238195ac26c10eae Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 23 Apr 2024 16:32:46 -0400 Subject: [PATCH 54/57] Catch general exception so we still process other idps --- corehq/apps/sso/tasks.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index adf2c30f74ff..906a1c118dd2 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -141,6 +141,12 @@ def auto_deactivate_removed_sso_users(): notify_exception(None, f"Failed to get members of the IdP. {str(e)}") send_deactivation_skipped_email(idp=idp, failure_reason=MSGraphIssue.HTTP_ERROR) continue + except Exception as e: + 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)}) + continue # if the Graph Users API returns an empty list of users we will skip auto deactivation if len(idp_users) == 0: From b6ea6f3afd211499049c057e4c4055032f928b29 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Tue, 23 Apr 2024 18:08:00 -0400 Subject: [PATCH 55/57] Fix keyword argument --- corehq/apps/sso/tasks.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 906a1c118dd2..40f8ed95fea4 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -133,17 +133,17 @@ def auto_deactivate_removed_sso_users(): idp_users = idp.get_all_members_of_the_idp() except EntraVerificationFailed as e: notify_exception(None, f"Failed to get members of the IdP. {str(e)}") - send_deactivation_skipped_email(idp=idp, failure_reason=MSGraphIssue.VERIFICATION_ERROR, + send_deactivation_skipped_email(idp=idp, failure_code=MSGraphIssue.VERIFICATION_ERROR, error=EntraVerificationFailed.error, error_description=EntraVerificationFailed.message) continue except requests.exceptions.HTTPError as e: notify_exception(None, f"Failed to get members of the IdP. {str(e)}") - send_deactivation_skipped_email(idp=idp, failure_reason=MSGraphIssue.HTTP_ERROR) + send_deactivation_skipped_email(idp=idp, failure_code=MSGraphIssue.HTTP_ERROR) continue except Exception as e: notify_exception(None, f"Failed to get members of the IdP. {str(e)}") - send_deactivation_skipped_email(idp=idp, failure_reason=MSGraphIssue.VERIFICATION_ERROR, + send_deactivation_skipped_email(idp=idp, failure_code=MSGraphIssue.VERIFICATION_ERROR, error="verification error", error_description={str(e)}) continue From 4069f8802d3364f3241e272871726fb818a8c4ff Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 24 Apr 2024 11:53:21 -0400 Subject: [PATCH 56/57] Raise error for permission denied 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'}}}}]} --- corehq/apps/sso/utils/entra.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/corehq/apps/sso/utils/entra.py b/corehq/apps/sso/utils/entra.py index 581486225d90..c6156229eb43 100644 --- a/corehq/apps/sso/utils/entra.py +++ b/corehq/apps/sso/utils/entra.py @@ -67,6 +67,10 @@ def get_all_members_of_the_idp_from_entra(idp): batch_response.raise_for_status() batch_result = batch_response.json() + for resp in batch_result['responses']: + if 'body' in resp and 'error' in resp['body']: + raise EntraVerificationFailed(resp['body']['error']['code'], resp['body']['message']) + # Extract userPrincipalName from batch response user_principal_names = [ resp['body']['userPrincipalName'] for resp in batch_result['responses'] From 8871badc4af1022a6b2cfa5dccbc78483274bec9 Mon Sep 17 00:00:00 2001 From: Jing Cheng Date: Wed, 24 Apr 2024 13:19:50 -0400 Subject: [PATCH 57/57] Avoid sending raw error to user --- corehq/apps/sso/tasks.py | 6 +++--- corehq/apps/sso/utils/entra.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/corehq/apps/sso/tasks.py b/corehq/apps/sso/tasks.py index 40f8ed95fea4..4ca1b0f50305 100644 --- a/corehq/apps/sso/tasks.py +++ b/corehq/apps/sso/tasks.py @@ -143,9 +143,7 @@ def auto_deactivate_removed_sso_users(): continue except Exception as e: notify_exception(None, f"Failed to get members of the IdP. {str(e)}") - send_deactivation_skipped_email(idp=idp, failure_code=MSGraphIssue.VERIFICATION_ERROR, - error="verification error", - error_description={str(e)}) + send_deactivation_skipped_email(idp=idp, failure_code=MSGraphIssue.OTHER_ERROR) continue # if the Graph Users API returns an empty list of users we will skip auto deactivation @@ -186,6 +184,8 @@ def send_deactivation_skipped_email(idp, failure_code, error=None, error_descrip "indicates an issue with Microsoft's servers.") elif failure_code == MSGraphIssue.EMPTY_ERROR: failure_reason = _("We received an empty list of users from your Microsoft Entra ID instance.") + elif failure_code == MSGraphIssue.OTHER_ERROR: + failure_reason = _("We encountered an unknown issue, please contact Commcare HQ Support.") context = get_sso_deactivation_skip_email_context(idp, failure_reason) for send_to in context["to"]: diff --git a/corehq/apps/sso/utils/entra.py b/corehq/apps/sso/utils/entra.py index c6156229eb43..847fd60eac3a 100644 --- a/corehq/apps/sso/utils/entra.py +++ b/corehq/apps/sso/utils/entra.py @@ -8,6 +8,7 @@ class MSGraphIssue: HTTP_ERROR = "http_error" VERIFICATION_ERROR = "verification_error" EMPTY_ERROR = "empty_error" + OTHER_ERROR = "other_error" def get_all_members_of_the_idp_from_entra(idp):