From f10cfe2c9bd143029af27270807619eab41be630 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 4 Feb 2025 14:45:08 -0600 Subject: [PATCH 001/125] delete ds data --- src/registrar/models/domain.py | 15 +++++++++- src/registrar/tests/test_models_domain.py | 36 +++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 0f0b3f112..134da7ead 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1053,13 +1053,15 @@ class Domain(TimeStampedModel, DomainHelper): note=f"Host {host.name} is in use by {host.domain}", ) + # set hosts to empty list so nameservers are deleted ( deleted_values, updated_values, new_values, oldNameservers, ) = self.getNameserverChanges(hosts=[]) - + + # update the hosts _ = self._update_host_values(updated_values, oldNameservers) # returns nothing, just need to be run and errors addToDomainList, _ = self.createNewHostList(new_values) deleteHostList, _ = self.createDeleteHostList(deleted_values) @@ -1073,6 +1075,7 @@ class Domain(TimeStampedModel, DomainHelper): # but we still need to delete the object themselves self._delete_hosts_if_not_used(hostsToDelete=deleted_values) + # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) contacts = PublicContact.objects.filter(domain=self) for contact in contacts: @@ -1081,6 +1084,16 @@ class Domain(TimeStampedModel, DomainHelper): request = commands.DeleteContact(contact.registry_id) registry.send(request, cleaned=True) + # delete ds data if it exists + if self.dnssecdata: + logger.debug("Deleting ds data for %s", self.name) + try: + self.dnssecdata = None + except RegistryError as e: + logger.error("Error deleting ds data for %s: %s", self.name, e) + e.note = "Error deleting ds data for %s" % self.name + raise e + logger.info("Deleting domain %s", self.name) request = commands.DeleteDomain(name=self.name) registry.send(request, cleaned=True) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 083725a55..5bb783fd0 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -2863,6 +2863,42 @@ class TestAnalystDelete(MockEppLib): # State should have changed self.assertEqual(self.domain_with_contacts.state, Domain.State.DELETED) + def test_analyst_deletes_domain_with_ds_data(self): + """ + Scenario: Domain with DS data is deleted + When `domain.deletedInEpp()` is called + Then `commands.DeleteDomain` is sent to the registry + And `state` is set to `DELETED` + """ + # Create a domain with DS data + domain, _ = Domain.objects.get_or_create(name="dsdomain.gov", state=Domain.State.READY) + domain.dnssecdata = extensions.DNSSECExtension( + dsdata=[extensions.DSData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], + keydata=[extensions.DNSSECKeyData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], + ) + domain.save() + + # Delete the domain + domain.deletedInEpp() + domain.save() + + # Check that dsdata is None + self.assertEqual(domain.dnssecdata, None) + + # Check that the UpdateDomain command was sent to the registry with the correct extension + self.mockedSendFunction.assert_has_calls( + [ + call( + commands.UpdateDomain(name="dsdomain.gov", add=[], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None), + cleaned=True, + ), + ] + ) + + # Check that the domain was deleted + self.assertEqual(domain.state, Domain.State.DELETED) + + @less_console_noise_decorator def test_deletion_ready_fsm_failure(self): """ From 59c22643316dd87b7ed7910eb62de07962f8e6fe Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 4 Feb 2025 14:55:19 -0600 Subject: [PATCH 002/125] remove raise error on delete_hosts_if_not_used --- src/registrar/models/domain.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 134da7ead..44eb1cde0 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -750,11 +750,7 @@ class Domain(TimeStampedModel, DomainHelper): successTotalNameservers = len(oldNameservers) - deleteCount + addToDomainCount - try: - self._delete_hosts_if_not_used(hostsToDelete=deleted_values) - except Exception as e: - # we don't need this part to succeed in order to continue. - logger.error("Failed to delete nameserver hosts: %s", e) + self._delete_hosts_if_not_used(hostsToDelete=deleted_values) if successTotalNameservers < 2: try: @@ -1849,8 +1845,6 @@ class Domain(TimeStampedModel, DomainHelper): else: logger.error("Error _delete_hosts_if_not_used, code was %s error was %s" % (e.code, e)) - raise e - def _fix_unknown_state(self, cleaned): """ _fix_unknown_state: Calls _add_missing_contacts_if_unknown From 041ca70a8b9fa0dc8c1d924a1940278c39f6d005 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 08:04:32 -0500 Subject: [PATCH 003/125] portfolio removed email and associated tests --- .../templates/emails/portfolio_removal.txt | 24 ++++++ .../emails/portfolio_removal_subject.txt | 1 + src/registrar/tests/test_email_invitations.py | 74 +++++++++++++++++++ src/registrar/tests/test_views_portfolio.py | 58 +++++++++++++-- src/registrar/utility/email_invitations.py | 41 ++++++++++ src/registrar/views/portfolios.py | 18 +++-- 6 files changed, 204 insertions(+), 12 deletions(-) create mode 100644 src/registrar/templates/emails/portfolio_removal.txt create mode 100644 src/registrar/templates/emails/portfolio_removal_subject.txt diff --git a/src/registrar/templates/emails/portfolio_removal.txt b/src/registrar/templates/emails/portfolio_removal.txt new file mode 100644 index 000000000..6de2190ae --- /dev/null +++ b/src/registrar/templates/emails/portfolio_removal.txt @@ -0,0 +1,24 @@ +{% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} +Hi,{% if requested_user and requested_user.first_name %} {{ requested_user.first_name }}.{% endif %} + +{{ requestor_email }} has removed you from {{ portfolio.organization_name }}. + +You can no longer view this organization or its related domains within the .gov registrar. + + +SOMETHING WRONG? +If you have questions or concerns, reach out to the person who removed you from the +organization, or reply to this email. + + +THANK YOU +.Gov helps the public identify official, trusted information. Thank you for using a .gov domain. + +---------------------------------------------------------------- + +The .gov team +Contact us: +Learn about .gov + +The .gov registry is a part of the Cybersecurity and Infrastructure Security Agency (CISA) +{% endautoescape %} diff --git a/src/registrar/templates/emails/portfolio_removal_subject.txt b/src/registrar/templates/emails/portfolio_removal_subject.txt new file mode 100644 index 000000000..d60ef9859 --- /dev/null +++ b/src/registrar/templates/emails/portfolio_removal_subject.txt @@ -0,0 +1 @@ +You've been removed from a .gov organization \ No newline at end of file diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index 20ac4a565..f07e2f2a7 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -16,6 +16,7 @@ from registrar.utility.email_invitations import ( send_portfolio_admin_addition_emails, send_portfolio_admin_removal_emails, send_portfolio_invitation_email, + send_portfolio_member_permission_remove_email, send_portfolio_member_permission_update_email, ) @@ -962,3 +963,76 @@ class TestSendPortfolioMemberPermissionUpdateEmail(unittest.TestCase): # Assertions mock_logger.warning.assert_not_called() # Function should fail before logging email failure + + +class TestSendPortfolioMemberPermissionRemoveEmail(unittest.TestCase): + """Unit tests for send_portfolio_member_permission_remove_email function.""" + + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.utility.email_invitations._get_requestor_email") + def test_send_email_success(self, mock_get_requestor_email, mock_send_email): + """Test that the email is sent successfully when there are no errors.""" + # Mock data + requestor = MagicMock() + permissions = MagicMock(spec=UserPortfolioPermission) + permissions.user.email = "user@example.com" + permissions.portfolio.organization_name = "Test Portfolio" + + mock_get_requestor_email.return_value = "requestor@example.com" + + # Call function + result = send_portfolio_member_permission_remove_email(requestor, permissions) + + # Assertions + mock_get_requestor_email.assert_called_once_with(requestor, portfolio=permissions.portfolio) + mock_send_email.assert_called_once_with( + "emails/portfolio_removal.txt", + "emails/portfolio_removal_subject.txt", + to_address="user@example.com", + context={ + "requested_user": permissions.user, + "portfolio": permissions.portfolio, + "requestor_email": "requestor@example.com", + }, + ) + self.assertTrue(result) + + @patch("registrar.utility.email_invitations.send_templated_email", side_effect=EmailSendingError("Email failed")) + @patch("registrar.utility.email_invitations._get_requestor_email") + @patch("registrar.utility.email_invitations.logger") + def test_send_email_failure(self, mock_logger, mock_get_requestor_email, mock_send_email): + """Test that the function returns False and logs an error when email sending fails.""" + # Mock data + requestor = MagicMock() + permissions = MagicMock(spec=UserPortfolioPermission) + permissions.user.email = "user@example.com" + permissions.portfolio.organization_name = "Test Portfolio" + + mock_get_requestor_email.return_value = "requestor@example.com" + + # Call function + result = send_portfolio_member_permission_remove_email(requestor, permissions) + + # Assertions + mock_logger.warning.assert_called_once_with( + "Could not send email organization member removal notification to %s for portfolio: %s", + permissions.user.email, + permissions.portfolio.organization_name, + exc_info=True, + ) + self.assertFalse(result) + + @patch("registrar.utility.email_invitations._get_requestor_email", side_effect=Exception("Unexpected error")) + @patch("registrar.utility.email_invitations.logger") + def test_requestor_email_retrieval_failure(self, mock_logger, mock_get_requestor_email): + """Test that an exception in retrieving requestor email is logged.""" + # Mock data + requestor = MagicMock() + permissions = MagicMock(spec=UserPortfolioPermission) + + # Call function + with self.assertRaises(Exception): + send_portfolio_member_permission_remove_email(requestor, permissions) + + # Assertions + mock_logger.warning.assert_not_called() # Function should fail before logging email failure diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 65e0350ee..329e8e9f1 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1669,7 +1669,8 @@ class TestPortfolioMemberDeleteView(WebTest): @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_delete_view_members_table_active_requests(self, send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_delete_view_members_table_active_requests(self, send_member_removal, send_removal_emails): """Error state w/ deleting a member with active request on Members Table""" # I'm a user UserPortfolioPermission.objects.get_or_create( @@ -1709,12 +1710,15 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is not called send_removal_emails.assert_not_called() + # assert that send_portfolio_member_permission_remove_email is not called + send_member_removal.assert_not_called() @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_delete_view_members_table_only_admin(self, send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_delete_view_members_table_only_admin(self, send_member_removal, send_removal_emails): """Error state w/ deleting a member that's the only admin on Members Table""" # I'm a user with admin permission @@ -1744,12 +1748,15 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is not called send_removal_emails.assert_not_called() + # assert that send_portfolio_member_permission_remove_email is not called + send_member_removal.assert_not_called() @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_table_delete_member_success(self, mock_send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_table_delete_member_success(self, send_member_removal, mock_send_removal_emails): """Success state with deleting on Members Table page bc no active request AND not only admin""" # I'm a user @@ -1774,6 +1781,9 @@ class TestPortfolioMemberDeleteView(WebTest): roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], ) + # Member removal email sent successfully + send_member_removal.return_value = True + # And set that the member has no active requests AND it's not the only admin with patch.object(User, "get_active_requests_count_in_portfolio", return_value=0), patch.object( User, "is_only_admin_of_portfolio", return_value=False @@ -1796,12 +1806,23 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is not called # because member being removed is not an admin mock_send_removal_emails.assert_not_called() + # assert that send_portfolio_member_permission_remove_email is called + send_member_removal.assert_called_once() + + # Get the arguments passed to send_portfolio_member_permission_remove_email + _, called_kwargs = send_member_removal.call_args + + # Assert the email content + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["permissions"].user, upp.user) + self.assertEqual(called_kwargs["permissions"].portfolio, upp.portfolio) @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_table_delete_admin_success(self, mock_send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_table_delete_admin_success(self, send_member_removal, mock_send_removal_emails): """Success state with deleting on Members Table page bc no active request AND not only admin. Because admin, removal emails are sent.""" @@ -1828,6 +1849,7 @@ class TestPortfolioMemberDeleteView(WebTest): ) mock_send_removal_emails.return_value = True + send_member_removal.return_value = True # And set that the member has no active requests AND it's not the only admin with patch.object(User, "get_active_requests_count_in_portfolio", return_value=0), patch.object( @@ -1850,6 +1872,8 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is called mock_send_removal_emails.assert_called_once() + # assert that send_portfolio_member_permission_remove_email is called + send_member_removal.assert_called_once() # Get the arguments passed to send_portfolio_admin_addition_emails _, called_kwargs = mock_send_removal_emails.call_args @@ -1859,13 +1883,23 @@ class TestPortfolioMemberDeleteView(WebTest): self.assertEqual(called_kwargs["requestor"], self.user) self.assertEqual(called_kwargs["portfolio"], self.portfolio) + # Get the arguments passed to send_portfolio_member_permission_remove_email + _, called_kwargs = send_member_removal.call_args + + # Assert the email content + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["permissions"].user, upp.user) + self.assertEqual(called_kwargs["permissions"].portfolio, upp.portfolio) + @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_table_delete_admin_success_removal_email_fail(self, mock_send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_table_delete_admin_success_removal_email_fail(self, send_member_removal, mock_send_removal_emails): """Success state with deleting on Members Table page bc no active request AND - not only admin. Because admin, removal emails are sent, but fail to send.""" + not only admin. Because admin, removal emails are sent, but fail to send. + Email to removed member also fails to send.""" # I'm a user UserPortfolioPermission.objects.get_or_create( @@ -1890,6 +1924,7 @@ class TestPortfolioMemberDeleteView(WebTest): ) mock_send_removal_emails.return_value = False + send_member_removal.return_value = False # And set that the member has no active requests AND it's not the only admin with patch.object(User, "get_active_requests_count_in_portfolio", return_value=0), patch.object( @@ -1912,6 +1947,8 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is called mock_send_removal_emails.assert_called_once() + # assert that send_portfolio_member_permission_remove_email is called + send_member_removal.assert_called_once() # Get the arguments passed to send_portfolio_admin_addition_emails _, called_kwargs = mock_send_removal_emails.call_args @@ -1920,6 +1957,15 @@ class TestPortfolioMemberDeleteView(WebTest): self.assertEqual(called_kwargs["email"], member_email) self.assertEqual(called_kwargs["requestor"], self.user) self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + # Get the arguments passed to send_portfolio_member_permission_remove_email + _, called_kwargs = send_member_removal.call_args + + # Assert the email content + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["permissions"].user, upp.user) + self.assertEqual(called_kwargs["permissions"].portfolio, upp.portfolio) + @less_console_noise_decorator @override_flag("organization_feature", active=True) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index d206bf279..2ddf74cc0 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -268,6 +268,47 @@ def send_portfolio_member_permission_update_email(requestor, permissions: UserPo return True +def send_portfolio_member_permission_remove_email(requestor, permissions: UserPortfolioPermission): + """ + Sends an email notification to a portfolio member when their permissions are deleted. + + This function retrieves the requestor's email and sends a templated email to the affected user, + notifying them of the removal of their portfolio permissions. + + Args: + requestor (User): The user initiating the permission update. + permissions (UserPortfolioPermission): The updated permissions object containing the affected user + and the portfolio details. + + Returns: + bool: True if the email was sent successfully, False if an EmailSendingError occurred. + + Raises: + MissingEmailError: If the requestor has no email associated with their account. + """ + requestor_email = _get_requestor_email(requestor, portfolio=permissions.portfolio) + try: + send_templated_email( + "emails/portfolio_removal.txt", + "emails/portfolio_removal_subject.txt", + to_address=permissions.user.email, + context={ + "requested_user": permissions.user, + "portfolio": permissions.portfolio, + "requestor_email": requestor_email, + }, + ) + except EmailSendingError: + logger.warning( + "Could not send email organization member removal notification to %s " "for portfolio: %s", + permissions.user.email, + permissions.portfolio.organization_name, + exc_info=True, + ) + return False + return True + + def send_portfolio_admin_addition_emails(email: str, requestor, portfolio: Portfolio): """ Notifies all portfolio admins of the provided portfolio of a newly invited portfolio admin diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index d63b5964e..3a1124898 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -20,6 +20,7 @@ from registrar.utility.email_invitations import ( send_portfolio_admin_addition_emails, send_portfolio_admin_removal_emails, send_portfolio_invitation_email, + send_portfolio_member_permission_remove_email, send_portfolio_member_permission_update_email, ) from registrar.utility.errors import MissingEmailError @@ -149,18 +150,23 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): messages.error(request, error_message) return redirect(reverse("member", kwargs={"pk": pk})) - # if member being removed is an admin - if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_member_permission.roles: - try: + try: + # if member being removed is an admin + if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_member_permission.roles: # attempt to send notification emails of the removal to other portfolio admins if not send_portfolio_admin_removal_emails( email=portfolio_member_permission.user.email, requestor=request.user, portfolio=portfolio_member_permission.portfolio, ): - messages.warning(self.request, "Could not send email notification to existing organization admins.") - except Exception as e: - self._handle_exceptions(e) + messages.warning(request, "Could not send email notification to existing organization admins.") + # send notification email to member being removed + if not send_portfolio_member_permission_remove_email( + requestor=request.user, permissions=portfolio_member_permission + ): + messages.warning(request, f"Could not send email notification to {member.email}") + except Exception as e: + self._handle_exceptions(e) # passed all error conditions portfolio_member_permission.delete() From a7f08aa5868005d27bfb688694b3a40d8656a4ba Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 6 Feb 2025 11:09:23 -0600 Subject: [PATCH 004/125] fix test --- src/registrar/tests/test_models_domain.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 5bb783fd0..973a5ad39 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -2863,6 +2863,7 @@ class TestAnalystDelete(MockEppLib): # State should have changed self.assertEqual(self.domain_with_contacts.state, Domain.State.DELETED) + # @less_console_noise def test_analyst_deletes_domain_with_ds_data(self): """ Scenario: Domain with DS data is deleted @@ -2872,9 +2873,10 @@ class TestAnalystDelete(MockEppLib): """ # Create a domain with DS data domain, _ = Domain.objects.get_or_create(name="dsdomain.gov", state=Domain.State.READY) + # set domain to be on hold + domain.place_client_hold() domain.dnssecdata = extensions.DNSSECExtension( - dsdata=[extensions.DSData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], - keydata=[extensions.DNSSECKeyData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], + dsData=[extensions.DSData(keyTag=1, alg=1, digestType=1, digest="1234567890")], ) domain.save() @@ -2885,6 +2887,11 @@ class TestAnalystDelete(MockEppLib): # Check that dsdata is None self.assertEqual(domain.dnssecdata, None) + # Print out all calls from the mockedSendFunction + print("\nAll calls to mockedSendFunction:") + for call in self.mockedSendFunction.call_args_list: + print(f"- {call}") + # Check that the UpdateDomain command was sent to the registry with the correct extension self.mockedSendFunction.assert_has_calls( [ From e3e001fe0d0efd9d09abef7ee20a6492879775e6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 13:25:54 -0500 Subject: [PATCH 005/125] add static messages to admin confirmation messages --- src/registrar/admin.py | 2 ++ ...rtfolio_invitation_delete_confirmation.html | 17 +++++++++++++++++ ...rtfolio_permission_delete_confirmation.html | 12 ++++++++++++ src/registrar/tests/test_admin.py | 18 ++++++++++++++++++ 4 files changed, 49 insertions(+) create mode 100644 src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html create mode 100644 src/registrar/templates/django/admin/user_portfolio_permission_delete_confirmation.html diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 928ead442..d27a4849c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1326,6 +1326,7 @@ class UserPortfolioPermissionAdmin(ListHeaderAdmin): search_help_text = "Search by first name, last name, email, or portfolio." change_form_template = "django/admin/user_portfolio_permission_change_form.html" + delete_confirmation_template = "django/admin/user_portfolio_permission_delete_confirmation.html" def get_roles(self, obj): readable_roles = obj.get_readable_roles() @@ -1631,6 +1632,7 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): autocomplete_fields = ["portfolio"] change_form_template = "django/admin/portfolio_invitation_change_form.html" + delete_confirmation_template = "django/admin/portfolio_invitation_delete_confirmation.html" # Select portfolio invitations to change -> Portfolio invitations def changelist_view(self, request, extra_context=None): diff --git a/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html b/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html new file mode 100644 index 000000000..5aa01c33f --- /dev/null +++ b/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html @@ -0,0 +1,17 @@ +{% extends "admin/delete_confirmation.html" %} + +{% block content_subtitle %} +
+
+

+ If you cancel the portfolio invitation here, it won't trigger any emails. It also won't remove the user's + portfolio access if they already logged in. Go to the + + User Portfolio Permissions + + table if you want to remove the user from a portfolio. +

+
+
+ {{ block.super }} +{% endblock %} \ No newline at end of file diff --git a/src/registrar/templates/django/admin/user_portfolio_permission_delete_confirmation.html b/src/registrar/templates/django/admin/user_portfolio_permission_delete_confirmation.html new file mode 100644 index 000000000..71c789a63 --- /dev/null +++ b/src/registrar/templates/django/admin/user_portfolio_permission_delete_confirmation.html @@ -0,0 +1,12 @@ +{% extends "admin/delete_confirmation.html" %} + +{% block content_subtitle %} +
+
+

+ If you remove someone from a portfolio here, it will not send any emails when you click "Save". +

+
+
+ {{ block.super }} +{% endblock %} diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 28a407036..e237f9509 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1549,6 +1549,24 @@ class TestPortfolioInvitationAdmin(TestCase): request, "Could not send email notification to existing organization admins." ) + @less_console_noise_decorator + def test_delete_confirmation_page_contains_static_message(self): + """Ensure the custom message appears in the delete confirmation page.""" + self.client.force_login(self.superuser) + # Create a test portfolio invitation + self.invitation = PortfolioInvitation.objects.create( + email="testuser@example.com", + portfolio=self.portfolio, + roles=["organization_member"] + ) + delete_url = reverse( + "admin:registrar_portfolioinvitation_delete", args=[self.invitation.pk] + ) + response = self.client.get(delete_url) + + # Check if the response contains the expected static message + expected_message = "If you cancel the portfolio invitation here" + self.assertIn(expected_message, response.content.decode("utf-8")) class TestHostAdmin(TestCase): """Tests for the HostAdmin class as super user From 6d0b9d1a9e2f995c37fce5b0f27245534def08dd Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 13:39:04 -0500 Subject: [PATCH 006/125] last test for messages in dja --- src/registrar/tests/test_admin.py | 26 ++++++++++++++++----- src/registrar/tests/test_views_portfolio.py | 7 +++--- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index e237f9509..8e04899cd 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -55,6 +55,7 @@ from .common import ( MockDbForSharedTests, AuditedAdminMockData, completed_domain_request, + create_test_user, generic_domain_object, less_console_noise, mock_user, @@ -1079,6 +1080,7 @@ class TestUserPortfolioPermissionAdmin(TestCase): """Create a client object""" self.client = Client(HTTP_HOST="localhost:8080") self.superuser = create_superuser() + self.testuser = create_test_user() self.portfolio = Portfolio.objects.create(organization_name="Test Portfolio", creator=self.superuser) def tearDown(self): @@ -1111,6 +1113,21 @@ class TestUserPortfolioPermissionAdmin(TestCase): "If you add someone to a portfolio here, it will not trigger an invitation email.", ) + @less_console_noise_decorator + def test_delete_confirmation_page_contains_static_message(self): + """Ensure the custom message appears in the delete confirmation page.""" + self.client.force_login(self.superuser) + # Create a test portfolio permission + self.permission = UserPortfolioPermission.objects.create( + user=self.testuser, portfolio=self.portfolio, roles=["organization_member"] + ) + delete_url = reverse("admin:registrar_userportfoliopermission_delete", args=[self.permission.pk]) + response = self.client.get(delete_url) + + # Check if the response contains the expected static message + expected_message = "If you remove someone from a portfolio here, it will not send any emails" + self.assertIn(expected_message, response.content.decode("utf-8")) + class TestPortfolioInvitationAdmin(TestCase): """Tests for the PortfolioInvitationAdmin class as super user @@ -1555,19 +1572,16 @@ class TestPortfolioInvitationAdmin(TestCase): self.client.force_login(self.superuser) # Create a test portfolio invitation self.invitation = PortfolioInvitation.objects.create( - email="testuser@example.com", - portfolio=self.portfolio, - roles=["organization_member"] - ) - delete_url = reverse( - "admin:registrar_portfolioinvitation_delete", args=[self.invitation.pk] + email="testuser@example.com", portfolio=self.portfolio, roles=["organization_member"] ) + delete_url = reverse("admin:registrar_portfolioinvitation_delete", args=[self.invitation.pk]) response = self.client.get(delete_url) # Check if the response contains the expected static message expected_message = "If you cancel the portfolio invitation here" self.assertIn(expected_message, response.content.decode("utf-8")) + class TestHostAdmin(TestCase): """Tests for the HostAdmin class as super user diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 329e8e9f1..76290ff3c 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1896,7 +1896,9 @@ class TestPortfolioMemberDeleteView(WebTest): @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") - def test_portfolio_member_table_delete_admin_success_removal_email_fail(self, send_member_removal, mock_send_removal_emails): + def test_portfolio_member_table_delete_admin_success_removal_email_fail( + self, send_member_removal, mock_send_removal_emails + ): """Success state with deleting on Members Table page bc no active request AND not only admin. Because admin, removal emails are sent, but fail to send. Email to removed member also fails to send.""" @@ -1957,7 +1959,7 @@ class TestPortfolioMemberDeleteView(WebTest): self.assertEqual(called_kwargs["email"], member_email) self.assertEqual(called_kwargs["requestor"], self.user) self.assertEqual(called_kwargs["portfolio"], self.portfolio) - + # Get the arguments passed to send_portfolio_member_permission_remove_email _, called_kwargs = send_member_removal.call_args @@ -1966,7 +1968,6 @@ class TestPortfolioMemberDeleteView(WebTest): self.assertEqual(called_kwargs["permissions"].user, upp.user) self.assertEqual(called_kwargs["permissions"].portfolio, upp.portfolio) - @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) From 9dd357c81f90bd7c0d16cd86ac22c87c841a2f2a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 15:02:59 -0500 Subject: [PATCH 007/125] refactored method to make more readable --- src/registrar/views/portfolios.py | 77 +++++++++++++++++++------------ 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 3a1124898..f85e8ebea 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -119,65 +119,82 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): """ portfolio_member_permission = get_object_or_404(UserPortfolioPermission, pk=pk) member = portfolio_member_permission.user + portfolio = portfolio_member_permission.portfolio + # Validate if the member can be removed + error_message = self._validate_member_removal(request, member, portfolio) + if error_message: + return self._handle_error_response(request, error_message, pk) + + # Attempt to send notification emails + self._send_removal_notifications(request, portfolio_member_permission) + + # Passed all error conditions, proceed with deletion + portfolio_member_permission.delete() + + # Return success response + return self._handle_success_response(request, member.email) + + def _validate_member_removal(self, request, member, portfolio): + """ + Check whether the member can be removed from the portfolio. + Returns an error message if removal is not allowed; otherwise, returns None. + """ active_requests_count = member.get_active_requests_count_in_portfolio(request) - support_url = "https://get.gov/contact/" - error_message = "" - if active_requests_count > 0: - # If they have any in progress requests - error_message = mark_safe( # nosec + return mark_safe( # nosec "This member can't be removed from the organization because they have an active domain request. " f"Please contact us to remove this member." ) - elif member.is_only_admin_of_portfolio(portfolio_member_permission.portfolio): - # If they are the last manager of a domain - error_message = ( + if member.is_only_admin_of_portfolio(portfolio): + return ( "There must be at least one admin in your organization. Give another member admin " "permissions, make sure they log into the registrar, and then remove this member." ) + return None - # From the Members Table page Else the Member Page - if error_message: - if request.headers.get("X-Requested-With") == "XMLHttpRequest": - return JsonResponse( - {"error": error_message}, - status=400, - ) - else: - messages.error(request, error_message) - return redirect(reverse("member", kwargs={"pk": pk})) + def _handle_error_response(self, request, error_message, pk): + """ + Return an error response (JSON or redirect with messages). + """ + if request.headers.get("X-Requested-With") == "XMLHttpRequest": + return JsonResponse({"error": error_message}, status=400) + messages.error(request, error_message) + return redirect(reverse("member", kwargs={"pk": pk})) + def _send_removal_notifications(self, request, portfolio_member_permission): + """ + Attempt to send notification emails about the member's removal. + """ try: - # if member being removed is an admin + # Notify other portfolio admins if removing an admin if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_member_permission.roles: - # attempt to send notification emails of the removal to other portfolio admins if not send_portfolio_admin_removal_emails( email=portfolio_member_permission.user.email, requestor=request.user, portfolio=portfolio_member_permission.portfolio, ): messages.warning(request, "Could not send email notification to existing organization admins.") - # send notification email to member being removed + + # Notify the member being removed if not send_portfolio_member_permission_remove_email( requestor=request.user, permissions=portfolio_member_permission ): - messages.warning(request, f"Could not send email notification to {member.email}") + messages.warning(request, f"Could not send email notification to {portfolio_member_permission.user.email}") except Exception as e: self._handle_exceptions(e) - # passed all error conditions - portfolio_member_permission.delete() - - # From the Members Table page Else the Member Page - success_message = f"You've removed {member.email} from the organization." + def _handle_success_response(self, request, member_email): + """ + Return a success response (JSON or redirect with messages). + """ + success_message = f"You've removed {member_email} from the organization." if request.headers.get("X-Requested-With") == "XMLHttpRequest": return JsonResponse({"success": success_message}, status=200) - else: - messages.success(request, success_message) - return redirect(reverse("members")) + messages.success(request, success_message) + return redirect(reverse("members")) def _handle_exceptions(self, exception): """Handle exceptions raised during the process.""" From 5de149394932407d49a0002c0592fed22ac3bfa6 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 6 Feb 2025 17:10:53 -0500 Subject: [PATCH 008/125] Change sort icon and sort header ui --- src/registrar/assets/js/uswds-edited.js | 20 +++++++++- .../assets/src/js/getgov/formset-forms.js | 1 + .../assets/src/sass/_theme/_tables.scss | 39 ++++++++++++++----- 3 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index 9d4dd2e51..ae246b05c 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -5695,19 +5695,35 @@ const createHeaderButton = (header, headerName) => { buttonEl.setAttribute("tabindex", "0"); buttonEl.classList.add(SORT_BUTTON_CLASS); // ICON_SOURCE + // ---- END DOTGOV EDIT + // Change icons on sort, use source from arro_upward and arrow_downward + // buttonEl.innerHTML = Sanitizer.escapeHTML` + // + // + // + // + // + // + // + // + // + // + // + // `; buttonEl.innerHTML = Sanitizer.escapeHTML` - + - + `; + // ---- END DOTGOV EDIT header.appendChild(buttonEl); updateSortLabel(header, headerName); }; diff --git a/src/registrar/assets/src/js/getgov/formset-forms.js b/src/registrar/assets/src/js/getgov/formset-forms.js index 27b85212e..faad54639 100644 --- a/src/registrar/assets/src/js/getgov/formset-forms.js +++ b/src/registrar/assets/src/js/getgov/formset-forms.js @@ -208,6 +208,7 @@ function hideDeletedForms() { * it everywhere. */ export function initFormsetsForms() { + console.log('init formsets'); let formIdentifier = "form" let repeatableForm = document.querySelectorAll(".repeatable-form"); let container = document.querySelector("#form-container"); diff --git a/src/registrar/assets/src/sass/_theme/_tables.scss b/src/registrar/assets/src/sass/_theme/_tables.scss index 37ae22b1b..d13cb8b0a 100644 --- a/src/registrar/assets/src/sass/_theme/_tables.scss +++ b/src/registrar/assets/src/sass/_theme/_tables.scss @@ -1,4 +1,5 @@ @use "uswds-core" as *; +@use "cisa_colors" as *; td, th { @@ -68,7 +69,9 @@ th { border-bottom: 1px solid color('base-lighter'); } - thead th { + thead th, + thead th[aria-sort], + thead th[aria-sort] { color: color('primary-darker'); border-bottom: 2px solid color('base-light'); } @@ -93,17 +96,35 @@ th { } } - @include at-media(tablet-lg) { - th[data-sortable] .usa-table__header__button { - right: auto; - - &[aria-sort=ascending], - &[aria-sort=descending], - &:not([aria-sort]) { - right: auto; + th[aria-sort], + th[data-sortable][aria-sort=ascending], + th[data-sortable][aria-sort=descending] { + background-color: transparent; + .usa-table__header__button { + background-color: rgba(214, 233, 242, 0.6); + background-color: $theme-color-accent-cool-lightest; + border-radius: 4px; + // position: relative; + // left: 4px; + // top: 16px; + color: color('primary-darker'); + + &:hover { + background-color: rgba(214, 233, 242, 0.6); } } } + + @include at-media(tablet-lg) { + th[data-sortable] .usa-table__header__button, + th[data-sortable] .usa-table__header__button:not([aria-sort]), + th[data-sortable][aria-sort=ascending] .usa-table__header__button, + th[data-sortable][aria-sort=descending] .usa-table__header__button { + right: auto; + top: 12px; + transform: translateX(10px); + } + } } .dotgov-table--cell-padding-2 { From 9c1e3bbc9419fe618c0100a03aebdbae49074e7a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 17:36:04 -0500 Subject: [PATCH 009/125] lint --- src/registrar/views/portfolios.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index c0a0a442f..a52a757ec 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -182,7 +182,9 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): if not send_portfolio_member_permission_remove_email( requestor=request.user, permissions=portfolio_member_permission ): - messages.warning(request, f"Could not send email notification to {portfolio_member_permission.user.email}") + messages.warning( + request, f"Could not send email notification to {portfolio_member_permission.user.email}" + ) except Exception as e: self._handle_exceptions(e) From ec9df83154565e8d1ae74e4cbeed4ba6594b0454 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 6 Feb 2025 20:01:15 -0500 Subject: [PATCH 010/125] Finish tweaking the UI --- .../assets/src/sass/_theme/_tables.scss | 49 ++++++++++--------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/src/registrar/assets/src/sass/_theme/_tables.scss b/src/registrar/assets/src/sass/_theme/_tables.scss index d13cb8b0a..4b9ccbb8a 100644 --- a/src/registrar/assets/src/sass/_theme/_tables.scss +++ b/src/registrar/assets/src/sass/_theme/_tables.scss @@ -1,5 +1,4 @@ @use "uswds-core" as *; -@use "cisa_colors" as *; td, th { @@ -42,13 +41,8 @@ th { } } -// The member table has an extra "expand" row, which looks like a single row. -// But the DOM disagrees - so we basically need to hide the border on both rows. -#members__table-wrapper .dotgov-table tr:nth-last-child(2) td, -#members__table-wrapper .dotgov-table tr:nth-last-child(2) th { - border-bottom: none; -} - +// .dotgov-table allows us to customize .usa-table on the user-facing pages, +// while leaving the default styles for use on the admin pages .dotgov-table { width: 100%; @@ -96,35 +90,44 @@ th { } } - th[aria-sort], + // Sortable headers th[data-sortable][aria-sort=ascending], th[data-sortable][aria-sort=descending] { background-color: transparent; .usa-table__header__button { - background-color: rgba(214, 233, 242, 0.6); - background-color: $theme-color-accent-cool-lightest; - border-radius: 4px; - // position: relative; - // left: 4px; - // top: 16px; + background-color: color('accent-cool-lightest'); + border-radius: units(.5); color: color('primary-darker'); - &:hover { - background-color: rgba(214, 233, 242, 0.6); + background-color: color('accent-cool-lightest'); } } } - @include at-media(tablet-lg) { th[data-sortable] .usa-table__header__button, - th[data-sortable] .usa-table__header__button:not([aria-sort]), - th[data-sortable][aria-sort=ascending] .usa-table__header__button, - th[data-sortable][aria-sort=descending] .usa-table__header__button { + th[data-sortable] .usa-table__header__button:not([aria-sort]) { + // position next to the copy right: auto; - top: 12px; - transform: translateX(10px); + // slide left to mock a margin between the copy and the icon + transform: translateX(units(1)); + // fix vertical alignment + top: units(1.5); } } + + // Currently the 'flash' when sort is clicked, + // this will become persistent if the double-sort bug is fixed + td[data-sort-active], + th[data-sort-active] { + background-color: color('primary-lightest'); + } +} + +// The member table has an extra "expand" row, which looks like a single row. +// But the DOM disagrees - so we basically need to hide the border on both rows. +#members__table-wrapper .dotgov-table tr:nth-last-child(2) td, +#members__table-wrapper .dotgov-table tr:nth-last-child(2) th { + border-bottom: none; } .dotgov-table--cell-padding-2 { From 1ca3fba2f74f1065e40c7ad8145978595bdc9763 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 6 Feb 2025 20:04:29 -0500 Subject: [PATCH 011/125] cleanup --- src/registrar/assets/src/js/getgov/formset-forms.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/assets/src/js/getgov/formset-forms.js b/src/registrar/assets/src/js/getgov/formset-forms.js index faad54639..27b85212e 100644 --- a/src/registrar/assets/src/js/getgov/formset-forms.js +++ b/src/registrar/assets/src/js/getgov/formset-forms.js @@ -208,7 +208,6 @@ function hideDeletedForms() { * it everywhere. */ export function initFormsetsForms() { - console.log('init formsets'); let formIdentifier = "form" let repeatableForm = document.querySelectorAll(".repeatable-form"); let container = document.querySelector("#form-container"); From f28ec7cf74d132eab38f328a179f9858b7706091 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 6 Feb 2025 20:06:54 -0500 Subject: [PATCH 012/125] cleanup --- src/registrar/assets/src/sass/_theme/_tables.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/assets/src/sass/_theme/_tables.scss b/src/registrar/assets/src/sass/_theme/_tables.scss index 4b9ccbb8a..d67b08041 100644 --- a/src/registrar/assets/src/sass/_theme/_tables.scss +++ b/src/registrar/assets/src/sass/_theme/_tables.scss @@ -64,7 +64,6 @@ th { } thead th, - thead th[aria-sort], thead th[aria-sort] { color: color('primary-darker'); border-bottom: 2px solid color('base-light'); From dea13ec27b58406a812db3464942a96523e461e9 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 10:37:52 -0600 Subject: [PATCH 013/125] bug fixes for deletion --- src/registrar/models/domain.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 44eb1cde0..032cac8b4 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1074,17 +1074,24 @@ class Domain(TimeStampedModel, DomainHelper): # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) contacts = PublicContact.objects.filter(domain=self) + for contact in contacts: - if contact.contact_type != PublicContact.ContactTypeChoices.REGISTRANT: - self._update_domain_with_contact(contact, rem=True) - request = commands.DeleteContact(contact.registry_id) - registry.send(request, cleaned=True) + try: + if contact.contact_type != PublicContact.ContactTypeChoices.REGISTRANT: + self._update_domain_with_contact(contact, rem=True) + request = commands.DeleteContact(contact.registry_id) + registry.send(request, cleaned=True) + except RegistryError as e: + logger.error(f"Error deleting contact: {contact}, {e}", exec_info=True) # delete ds data if it exists if self.dnssecdata: logger.debug("Deleting ds data for %s", self.name) try: + # set and unset client hold to be able to change ds data + self._remove_client_hold() self.dnssecdata = None + self._place_client_hold() except RegistryError as e: logger.error("Error deleting ds data for %s: %s", self.name, e) e.note = "Error deleting ds data for %s" % self.name From d4a5e3e40081bd12d5b6d93e0d1f1147eb5483a1 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 10:45:21 -0600 Subject: [PATCH 014/125] fix deletion regex --- src/registrar/models/domain.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 032cac8b4..4a3edcd4a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1040,7 +1040,7 @@ class Domain(TimeStampedModel, DomainHelper): logger.info("Deleting subdomains for %s", self.name) # check if any subdomains are in use by another domain - hosts = Host.objects.filter(name__regex=r".+{}".format(self.name)) + hosts = Host.objects.filter(name__regex=r".+\.{}".format(self.name)) for host in hosts: if host.domain != self: logger.error("Unable to delete host: %s is in use by another domain: %s", host.name, host.domain) From 826adc575355f26024a4954316c6e9af01800ef6 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 11:02:35 -0600 Subject: [PATCH 015/125] add more logging --- src/registrar/models/domain.py | 45 ++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 4a3edcd4a..18a4442f4 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1048,21 +1048,23 @@ class Domain(TimeStampedModel, DomainHelper): code=ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION, note=f"Host {host.name} is in use by {host.domain}", ) - - # set hosts to empty list so nameservers are deleted - ( - deleted_values, - updated_values, - new_values, - oldNameservers, - ) = self.getNameserverChanges(hosts=[]) - - # update the hosts - _ = self._update_host_values(updated_values, oldNameservers) # returns nothing, just need to be run and errors - addToDomainList, _ = self.createNewHostList(new_values) - deleteHostList, _ = self.createDeleteHostList(deleted_values) - responseCode = self.addAndRemoveHostsFromDomain(hostsToAdd=addToDomainList, hostsToDelete=deleteHostList) - + try: + # set hosts to empty list so nameservers are deleted + ( + deleted_values, + updated_values, + new_values, + oldNameservers, + ) = self.getNameserverChanges(hosts=[]) + + # update the hosts + _ = self._update_host_values(updated_values, oldNameservers) # returns nothing, just need to be run and errors + addToDomainList, _ = self.createNewHostList(new_values) + deleteHostList, _ = self.createDeleteHostList(deleted_values) + responseCode = self.addAndRemoveHostsFromDomain(hostsToAdd=addToDomainList, hostsToDelete=deleteHostList) + except RegistryError as e: + logger.error(f"Error trying to delete hosts from domain {self}: {e}") + raise e # if unable to update domain raise error and stop if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: raise NameserverError(code=nsErrorCodes.BAD_DATA) @@ -1070,6 +1072,7 @@ class Domain(TimeStampedModel, DomainHelper): # addAndRemoveHostsFromDomain removes the hosts from the domain object, # but we still need to delete the object themselves self._delete_hosts_if_not_used(hostsToDelete=deleted_values) + logger.info("Finished _delete_host_if_not_used inside _delete_domain()") # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) @@ -1083,6 +1086,8 @@ class Domain(TimeStampedModel, DomainHelper): registry.send(request, cleaned=True) except RegistryError as e: logger.error(f"Error deleting contact: {contact}, {e}", exec_info=True) + + logger.info("Finished deleting contacts") # delete ds data if it exists if self.dnssecdata: @@ -1097,9 +1102,13 @@ class Domain(TimeStampedModel, DomainHelper): e.note = "Error deleting ds data for %s" % self.name raise e - logger.info("Deleting domain %s", self.name) - request = commands.DeleteDomain(name=self.name) - registry.send(request, cleaned=True) + try: + logger.info("Deleting domain %s", self.name) + request = commands.DeleteDomain(name=self.name) + registry.send(request, cleaned=True) + except RegistryError as e: + logger.error(f"Error deleting domain {self}: {e}") + raise e def __str__(self) -> str: return self.name From 68732c7895d2ca0217b4bd481bd0cfdf5a2bc4c8 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 11:22:06 -0600 Subject: [PATCH 016/125] add MOAR LOGS --- src/registrar/models/domain.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 18a4442f4..a77641872 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1068,6 +1068,8 @@ class Domain(TimeStampedModel, DomainHelper): # if unable to update domain raise error and stop if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: raise NameserverError(code=nsErrorCodes.BAD_DATA) + + logger.info("Finished removing nameservers from domain") # addAndRemoveHostsFromDomain removes the hosts from the domain object, # but we still need to delete the object themselves @@ -1077,13 +1079,19 @@ class Domain(TimeStampedModel, DomainHelper): # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) contacts = PublicContact.objects.filter(domain=self) + logger.info(f"retrieved contacts for domain: {contacts}") for contact in contacts: try: if contact.contact_type != PublicContact.ContactTypeChoices.REGISTRANT: - self._update_domain_with_contact(contact, rem=True) + logger.info(f"Deleting contact: {contact}") + try: + self._update_domain_with_contact(contact, rem=True) + except Exception as e: + logger.error(f"Error while updateing domain with contact: {contact}, e: {e}", exc_info=True) request = commands.DeleteContact(contact.registry_id) registry.send(request, cleaned=True) + logger.info(f"sent DeleteContact for {contact}") except RegistryError as e: logger.error(f"Error deleting contact: {contact}, {e}", exec_info=True) @@ -1094,8 +1102,10 @@ class Domain(TimeStampedModel, DomainHelper): logger.debug("Deleting ds data for %s", self.name) try: # set and unset client hold to be able to change ds data + logger.info("removing client hold") self._remove_client_hold() self.dnssecdata = None + logger.info("placing client hold") self._place_client_hold() except RegistryError as e: logger.error("Error deleting ds data for %s: %s", self.name, e) From 0f50fd62e9c238f54f2bfeb509813576c921d2f9 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 11:38:32 -0600 Subject: [PATCH 017/125] fix typo --- src/registrar/models/domain.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index a77641872..adaa8703b 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1088,12 +1088,12 @@ class Domain(TimeStampedModel, DomainHelper): try: self._update_domain_with_contact(contact, rem=True) except Exception as e: - logger.error(f"Error while updateing domain with contact: {contact}, e: {e}", exc_info=True) + logger.error(f"Error while updating domain with contact: {contact}, e: {e}", exc_info=True) request = commands.DeleteContact(contact.registry_id) registry.send(request, cleaned=True) logger.info(f"sent DeleteContact for {contact}") except RegistryError as e: - logger.error(f"Error deleting contact: {contact}, {e}", exec_info=True) + logger.error(f"Error deleting contact: {contact}, {e}", exc_info=True) logger.info("Finished deleting contacts") From 6fc5a76e923a98a6180bf94771d22e3d94eca557 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Sun, 9 Feb 2025 18:20:38 -0700 Subject: [PATCH 018/125] Removed header markup for "clear all" filters link --- .../templates/admin/change_list.html | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/registrar/templates/admin/change_list.html b/src/registrar/templates/admin/change_list.html index 43abf0861..43e6fa5b5 100644 --- a/src/registrar/templates/admin/change_list.html +++ b/src/registrar/templates/admin/change_list.html @@ -1,4 +1,5 @@ {% extends "admin/change_list.html" %} +{% load i18n admin_urls static admin_list %} {% block content_title %}

{{ title }}

@@ -46,4 +47,25 @@ {{ block.super }} {% endblock %} -{% endblock %} \ No newline at end of file +{% endblock %} + + +{% comment %} Replace the Django header markup for clearing all filters with a div. {% endcomment %} +{% block filters %} +{% if cl.has_filters %} + +{% endif %} +{% endblock %} + From f90fe6c4629696616d50cee8bcc87064a18a7c39 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Sun, 9 Feb 2025 18:20:51 -0700 Subject: [PATCH 019/125] Removed header markup for "By Status" multiple choice filter --- .../admin/multiple_choice_list_filter.html | 70 ++++++++++--------- 1 file changed, 37 insertions(+), 33 deletions(-) diff --git a/src/registrar/templates/django/admin/multiple_choice_list_filter.html b/src/registrar/templates/django/admin/multiple_choice_list_filter.html index 167059594..2a61fee93 100644 --- a/src/registrar/templates/django/admin/multiple_choice_list_filter.html +++ b/src/registrar/templates/django/admin/multiple_choice_list_filter.html @@ -1,37 +1,41 @@ {% load i18n %} {% load static field_helpers url_helpers %} +
+ + {% blocktrans with filter_title=title %} By {{ filter_title }} {% endblocktrans %} + +
    + {% for choice in choices %} + {% if choice.reset %} + + {{ choice.display }} + + {% endif %} + {% endfor %} -

    {% blocktrans with filter_title=title %} By {{ filter_title }} {% endblocktrans %}

    - + {% for choice in choices %} + {% if not choice.reset %} + + {% if choice.selected and choice.exclude_query_string %} + {{ choice.display }} + + + + {% endif %} + {% if not choice.selected and choice.include_query_string %} + {{ choice.display }} + + + {% endif %} + + {% endif %} + {% endfor %} +
+
From 2c74654544c9ae41223ca527d393a2efc71045d5 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Sun, 9 Feb 2025 19:25:37 -0700 Subject: [PATCH 020/125] add skip-to-filters link for admin tables --- src/registrar/templates/admin/change_list.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/registrar/templates/admin/change_list.html b/src/registrar/templates/admin/change_list.html index 43abf0861..7256f2af2 100644 --- a/src/registrar/templates/admin/change_list.html +++ b/src/registrar/templates/admin/change_list.html @@ -37,6 +37,8 @@ for {{ search_query }} {% endif %} + + Skip to filters {% endblock %} {% comment %} Replace the Django ul markup with a div. We'll replace the li with a p in change_list_object_tools {% endcomment %} From f08c2ff4ad39d9c97477b225490b3e8edf848d63 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 10 Feb 2025 10:30:50 -0600 Subject: [PATCH 021/125] linter fixes --- src/registrar/models/domain.py | 14 ++++++++------ src/registrar/tests/test_models_domain.py | 10 +++------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index adaa8703b..812cc3894 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1034,7 +1034,7 @@ class Domain(TimeStampedModel, DomainHelper): logger.error(f"registry error removing client hold: {err}") raise (err) - def _delete_domain(self): + def _delete_domain(self): # noqa """This domain should be deleted from the registry may raises RegistryError, should be caught or handled correctly by caller""" @@ -1056,9 +1056,11 @@ class Domain(TimeStampedModel, DomainHelper): new_values, oldNameservers, ) = self.getNameserverChanges(hosts=[]) - + # update the hosts - _ = self._update_host_values(updated_values, oldNameservers) # returns nothing, just need to be run and errors + _ = self._update_host_values( + updated_values, oldNameservers + ) # returns nothing, just need to be run and errors addToDomainList, _ = self.createNewHostList(new_values) deleteHostList, _ = self.createDeleteHostList(deleted_values) responseCode = self.addAndRemoveHostsFromDomain(hostsToAdd=addToDomainList, hostsToDelete=deleteHostList) @@ -1068,7 +1070,7 @@ class Domain(TimeStampedModel, DomainHelper): # if unable to update domain raise error and stop if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: raise NameserverError(code=nsErrorCodes.BAD_DATA) - + logger.info("Finished removing nameservers from domain") # addAndRemoveHostsFromDomain removes the hosts from the domain object, @@ -1080,7 +1082,7 @@ class Domain(TimeStampedModel, DomainHelper): logger.debug("Deleting non-registrant contacts for %s", self.name) contacts = PublicContact.objects.filter(domain=self) logger.info(f"retrieved contacts for domain: {contacts}") - + for contact in contacts: try: if contact.contact_type != PublicContact.ContactTypeChoices.REGISTRANT: @@ -1094,7 +1096,7 @@ class Domain(TimeStampedModel, DomainHelper): logger.info(f"sent DeleteContact for {contact}") except RegistryError as e: logger.error(f"Error deleting contact: {contact}, {e}", exc_info=True) - + logger.info("Finished deleting contacts") # delete ds data if it exists diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 973a5ad39..8bbd6d60f 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -2887,16 +2887,13 @@ class TestAnalystDelete(MockEppLib): # Check that dsdata is None self.assertEqual(domain.dnssecdata, None) - # Print out all calls from the mockedSendFunction - print("\nAll calls to mockedSendFunction:") - for call in self.mockedSendFunction.call_args_list: - print(f"- {call}") - # Check that the UpdateDomain command was sent to the registry with the correct extension self.mockedSendFunction.assert_has_calls( [ call( - commands.UpdateDomain(name="dsdomain.gov", add=[], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None), + commands.UpdateDomain( + name="dsdomain.gov", add=[], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None + ), cleaned=True, ), ] @@ -2904,7 +2901,6 @@ class TestAnalystDelete(MockEppLib): # Check that the domain was deleted self.assertEqual(domain.state, Domain.State.DELETED) - @less_console_noise_decorator def test_deletion_ready_fsm_failure(self): From 78be172ee4fd3bf6744cffad46420573047d1dbb Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 10 Feb 2025 13:20:46 -0600 Subject: [PATCH 022/125] add retry mechanism to domain deletion --- src/registrar/models/domain.py | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 812cc3894..8366014a3 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -2,7 +2,7 @@ from itertools import zip_longest import logging import ipaddress import re -from datetime import date, timedelta +from datetime import date, time, timedelta from typing import Optional from django.db import transaction from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore @@ -1114,12 +1114,34 @@ class Domain(TimeStampedModel, DomainHelper): e.note = "Error deleting ds data for %s" % self.name raise e + # Previous deletions have to complete before we can delete the domain + # This is a polling mechanism to ensure that the domain is deleted try: - logger.info("Deleting domain %s", self.name) - request = commands.DeleteDomain(name=self.name) - registry.send(request, cleaned=True) + logger.info("Attempting to delete domain %s", self.name) + delete_request = commands.DeleteDomain(name=self.name) + max_attempts = 5 # maximum number of retries + wait_interval = 1 # seconds to wait between attempts + + for attempt in range(max_attempts): + try: + registry.send(delete_request, cleaned=True) + logger.info("Domain %s deleted successfully.", self.name) + break # exit the loop on success + except RegistryError as e: + error = e + logger.warning( + "Attempt %d of %d: Domain deletion not possible yet: %s. Retrying in %d seconds.", + attempt + 1, + max_attempts, + e, + wait_interval, + ) + time.sleep(wait_interval) + else: + logger.error("Exceeded maximum attempts to delete domain %s", self.name) + raise error except RegistryError as e: - logger.error(f"Error deleting domain {self}: {e}") + logger.error("Error deleting domain %s after polling: %s", self.name, e) raise e def __str__(self) -> str: From 6f3888f57aa97c223bd67957bb55f53bb27439fc Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 15:27:31 -0500 Subject: [PATCH 023/125] expanded row --- .../assets/src/js/getgov/table-members.js | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index 75a7c29ac..5e93ecab8 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -87,7 +87,7 @@ export class MembersTable extends BaseTable { // generate html blocks for domains and permissions for the member let domainsHTML = this.generateDomainsHTML(num_domains, member.domain_names, member.domain_urls, member.action_url); - let permissionsHTML = this.generatePermissionsHTML(member.permissions, customTableOptions.UserPortfolioPermissionChoices); + let permissionsHTML = this.generatePermissionsHTML(member.is_admin, member.permissions, customTableOptions.UserPortfolioPermissionChoices); // domainsHTML block and permissionsHTML block need to be wrapped with hide/show toggle, Expand let showMoreButton = ''; @@ -257,10 +257,11 @@ export class MembersTable extends BaseTable { let domainsHTML = ''; // Only generate HTML if the member has one or more assigned domains + + domainsHTML += "
"; + domainsHTML += "

Domains assigned

"; if (num_domains > 0) { - domainsHTML += "
"; - domainsHTML += "

Domains assigned

"; - domainsHTML += `

This member is assigned to ${num_domains} domain${num_domains > 1 ? 's' : ''}:

`; + domainsHTML += `

This member is assigned to ${num_domains} domain${num_domains > 1 ? 's' : ''}:

`; domainsHTML += "
    "; // Display up to 6 domains with their URLs @@ -269,13 +270,15 @@ export class MembersTable extends BaseTable { } domainsHTML += "
"; - - // If there are more than 6 domains, display a "View assigned domains" link - domainsHTML += `

View assigned domains

`; - - domainsHTML += "
"; + } else { + domainsHTML += `

This member is assigned to 0 domains.

`; } + // If there are more than 6 domains, display a "View assigned domains" link + domainsHTML += `

View domains assigned

`; + + domainsHTML += "
"; + return domainsHTML; } @@ -387,40 +390,51 @@ export class MembersTable extends BaseTable { * - If no relevant permissions are found, the function returns a message stating that the user has no additional permissions. * - The resulting HTML always includes a header "Additional permissions for this member" and appends the relevant permission descriptions. */ - generatePermissionsHTML(member_permissions, UserPortfolioPermissionChoices) { + generatePermissionsHTML(is_admin, member_permissions, UserPortfolioPermissionChoices) { let permissionsHTML = ''; // Define shared classes across elements for easier refactoring - let sharedParagraphClasses = "font-body-xs text-base-dark margin-top-1 p--blockquote"; + let sharedParagraphClasses = "font-body-xs text-base-darker margin-top-1 p--blockquote"; + + // Member access + if (is_admin) { + permissionsHTML += `

Member access: Admin

`; + } else { + permissionsHTML += `

Member access: Basic

`; + } // Check domain-related permissions if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS)) { - permissionsHTML += `

Domains: Can view all organization domains. Can manage domains they are assigned to and edit information about the domain (including DNS settings).

`; + permissionsHTML += `

Domains: Viewer

`; } else if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS)) { - permissionsHTML += `

Domains: Can manage domains they are assigned to and edit information about the domain (including DNS settings).

`; + permissionsHTML += `

Domains: Viewer, limited

`; } // Check request-related permissions if (member_permissions.includes(UserPortfolioPermissionChoices.EDIT_REQUESTS)) { - permissionsHTML += `

Domain requests: Can view all organization domain requests. Can create domain requests and modify their own requests.

`; + permissionsHTML += `

Domain requests: Creator

`; } else if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS)) { - permissionsHTML += `

Domain requests (view-only): Can view all organization domain requests. Can't create or modify any domain requests.

`; + permissionsHTML += `

Domain requests: Viewer

`; + } else { + permissionsHTML += `

Domain requests: No access

`; } // Check member-related permissions if (member_permissions.includes(UserPortfolioPermissionChoices.EDIT_MEMBERS)) { - permissionsHTML += `

Members: Can manage members including inviting new members, removing current members, and assigning domains to members.

`; + permissionsHTML += `

Members: Manager

`; } else if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_MEMBERS)) { - permissionsHTML += `

Members (view-only): Can view all organizational members. Can't manage any members.

`; + permissionsHTML += `

Members: Viewer

`; + } else { + permissionsHTML += `

Members: No access

`; } // If no specific permissions are assigned, display a message indicating no additional permissions if (!permissionsHTML) { - permissionsHTML += `

No additional permissions: There are no additional permissions for this member.

`; + permissionsHTML += `

No additional permissions: There are no additional permissions for this member.

`; } // Add a permissions header and wrap the entire output in a container - permissionsHTML = `

Additional permissions for this member

${permissionsHTML}
`; + permissionsHTML = `

Member access and permissions

${permissionsHTML}
`; return permissionsHTML; } From e751388533a40176dea8285d783b1078f7747989 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Mon, 10 Feb 2025 15:58:14 -0500 Subject: [PATCH 024/125] better error handling in domain deletion --- src/registrar/admin.py | 4 ++-- src/registrar/models/domain.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 7dbe7abb0..d22bff58f 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3679,10 +3679,10 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): ), messages.ERROR, ) - except Exception: + except Exception as e: self.message_user( request, - "Could not delete: An unspecified error occured", + f"Could not delete: An unspecified error occured: {e}", messages.ERROR, ) else: diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 8366014a3..23ffb2ad6 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1097,7 +1097,7 @@ class Domain(TimeStampedModel, DomainHelper): except RegistryError as e: logger.error(f"Error deleting contact: {contact}, {e}", exc_info=True) - logger.info("Finished deleting contacts") + logger.info(f"Finished deleting contacts for {self.name}") # delete ds data if it exists if self.dnssecdata: From 72a2f6305104119e6f9c138118254e92918aa51f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:18:03 -0500 Subject: [PATCH 025/125] Member profile page --- .../src/js/getgov/portfolio-member-page.js | 2 +- src/registrar/forms/portfolio.py | 2 +- .../includes/member_domain_management.html | 4 +-- .../includes/member_permissions_summary.html | 16 ++++++------ .../templates/includes/summary_item.html | 26 ++++++++++++------- src/registrar/templates/portfolio_member.html | 6 ++--- src/registrar/tests/test_views_portfolio.py | 2 +- 7 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index c96677ebc..95723fc7e 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -128,7 +128,7 @@ export function initAddNewMemberPageListeners() { }); } else { // for admin users, the permissions are always the same - appendPermissionInContainer('Domains', 'Viewer, all', permissionDetailsContainer); + appendPermissionInContainer('Domains', 'Viewer', permissionDetailsContainer); appendPermissionInContainer('Domain requests', 'Creator', permissionDetailsContainer); appendPermissionInContainer('Members', 'Manager', permissionDetailsContainer); } diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 2725224f1..76a5032da 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -127,7 +127,7 @@ class BasePortfolioMemberForm(forms.ModelForm): domain_permissions = forms.ChoiceField( choices=[ (UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value, "Viewer, limited"), - (UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS.value, "Viewer, all"), + (UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS.value, "Viewer"), ], widget=forms.RadioSelect, required=False, diff --git a/src/registrar/templates/includes/member_domain_management.html b/src/registrar/templates/includes/member_domain_management.html index 1e5b29994..2adc3f950 100644 --- a/src/registrar/templates/includes/member_domain_management.html +++ b/src/registrar/templates/includes/member_domain_management.html @@ -1,6 +1,6 @@ -

Assigned domains

{% if domain_count > 0 %} +

Domains assigned

{{domain_count}}

{% else %} -

This member does not manage any domains.{% if manage_button %} To assign this member a domain, click "Manage".{% endif %}

+

This member does not manage any domains.{% if manage_button %} To assign this member a domain, click "Edit".{% endif %}

{% endif %} diff --git a/src/registrar/templates/includes/member_permissions_summary.html b/src/registrar/templates/includes/member_permissions_summary.html index 3a91d16f6..8e3dad528 100644 --- a/src/registrar/templates/includes/member_permissions_summary.html +++ b/src/registrar/templates/includes/member_permissions_summary.html @@ -9,25 +9,25 @@

Domains

{% if member_has_view_all_domains_portfolio_permission %} -

Viewer, all

+

Viewer: Can view all domains for the organization

{% else %} -

Viewer, limited

+

Viewer, limited: Can view only the domains they manage

{% endif %}

Domain requests

{% if member_has_edit_request_portfolio_permission %} -

Creator

+

Creator: Can view all domain requests for the organization and create requests

{% elif member_has_view_all_requests_portfolio_permission %} -

Viewer

+

Viewer: Can view all domain requests for the organization

{% else %} -

No access

+

No access: Cannot view or create domain requests

{% endif %}

Members

{% if member_has_edit_members_portfolio_permission %} -

Manager

+

Manager: Can view and manage all member permissions

{% elif member_has_view_members_portfolio_permission %} -

Viewer

+

Viewer: Can view all members permissions

{% else %} -

No access

+

No access: Cannot view member permissions

{% endif %} \ No newline at end of file diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index d062a7b4e..a091e5ab7 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -134,18 +134,26 @@ {% endif %} - {% if editable and edit_link %} + {% if editable and edit_link or view_button %}
- - - {% if manage_button %}Manage{% elif view_button %}View{% else %}Edit{% endif %} {{ title }} - + > + + {% if manage_button %} + Manage + {% elif editable and edit_link %} + Edit + {% else %} + View + {% endif %} + {{ title|default:"Page" }} +
{% endif %} + diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 709582e71..477599d49 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -101,11 +101,11 @@ Organization member {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_invitation edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% endif %} - {% comment %}view_button is passed below as true in all cases. This is because manage_button logic will trump view_button logic; ie. if manage_button is true, view_button will never be looked at{% endcomment %} + {% comment %}view_button is passed below as true in all cases. This is because editable logic will trump view_button logic; ie. if editable is true, view_button will never be looked at{% endcomment %} {% if portfolio_permission %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt=True value=portfolio_permission.get_managed_domains_count edit_link=domains_url editable=True manage_button=has_edit_members_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Domain assignment' domain_mgmt=True value=portfolio_permission.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} {% elif portfolio_invitation %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt=True value=portfolio_invitation.get_managed_domains_count edit_link=domains_url editable=True manage_button=has_edit_members_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Domain assignment' domain_mgmt=True value=portfolio_invitation.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} {% endif %} diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 097aa1879..7a664a8c6 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1063,7 +1063,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Invited") self.assertContains(response, portfolio_invitation.email) self.assertContains(response, "Admin") - self.assertContains(response, "Viewer, all") + self.assertContains(response, "Viewer") self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( From 891d0f3dbb11bb13d434adb03f47feca1003a894 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:21:17 -0500 Subject: [PATCH 026/125] domain assignments page --- src/registrar/templates/includes/member_domains_edit_table.html | 2 +- src/registrar/templates/includes/member_domains_table.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/includes/member_domains_edit_table.html b/src/registrar/templates/includes/member_domains_edit_table.html index 0b8ff005a..ac05dc14f 100644 --- a/src/registrar/templates/includes/member_domains_edit_table.html +++ b/src/registrar/templates/includes/member_domains_edit_table.html @@ -100,7 +100,7 @@ > From 77711894b3061e36ba63867f8c99d150b075304a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:29:40 -0500 Subject: [PATCH 028/125] Add a new member page --- src/registrar/templates/portfolio_members_add_new.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/portfolio_members_add_new.html b/src/registrar/templates/portfolio_members_add_new.html index 464eaefce..c3a648bdc 100644 --- a/src/registrar/templates/portfolio_members_add_new.html +++ b/src/registrar/templates/portfolio_members_add_new.html @@ -55,7 +55,7 @@

What level of access would you like to grant this member?

-

Select one *

+

Select the level of access for this member. *

{% with add_class="usa-radio__input--tile" add_legend_class="usa-sr-only" %} {% input_with_errors form.role %} @@ -88,7 +88,7 @@ aria-controls="invite-member-modal" data-open-modal >Trigger invite member modal - + From 586d83adc172a058e288c1bba7207123ee734cbe Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:44:57 -0500 Subject: [PATCH 029/125] Fix unit tests --- src/registrar/tests/test_views_portfolio.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 7a664a8c6..7d56e1650 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -952,7 +952,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( - response, 'This member does not manage any domains. To assign this member a domain, click "Manage"' + response, 'This member does not manage any domains. To assign this member a domain, click "Edit"' ) # Assert buttons and links within the page are correct @@ -1067,7 +1067,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( - response, 'This member does not manage any domains. To assign this member a domain, click "Manage"' + response, 'This member does not manage any domains. To assign this member a domain, click "Edit"' ) # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present From 1d54fe6b43e1716a20c39ee269408b86c13437a8 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:50:54 -0500 Subject: [PATCH 030/125] fix unit tests --- src/registrar/tests/test_views_portfolio.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 7d56e1650..c00892eca 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -952,7 +952,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( - response, 'This member does not manage any domains. To assign this member a domain, click "Edit"' + response, 'This member does not manage any domains.' ) # Assert buttons and links within the page are correct @@ -1067,7 +1067,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( - response, 'This member does not manage any domains. To assign this member a domain, click "Edit"' + response, 'This member does not manage any domains.' ) # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present From c44c66ae96eebbff82ac1883b7fb79c69f4daf1a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:01:43 -0500 Subject: [PATCH 031/125] wip --- src/registrar/assets/src/js/getgov/table-members.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index 5e93ecab8..e876ce0f9 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -108,7 +108,7 @@ export class MembersTable extends BaseTable { `; - showMoreRow.innerHTML = `
${domainsHTML} ${permissionsHTML}
`; + showMoreRow.innerHTML = `
${domainsHTML} ${permissionsHTML}
`; showMoreRow.classList.add('show-more-content'); showMoreRow.classList.add('display-none'); showMoreRow.id = unique_id; @@ -258,7 +258,7 @@ export class MembersTable extends BaseTable { // Only generate HTML if the member has one or more assigned domains - domainsHTML += "
"; + domainsHTML += "
"; domainsHTML += "

Domains assigned

"; if (num_domains > 0) { domainsHTML += `

This member is assigned to ${num_domains} domain${num_domains > 1 ? 's' : ''}:

`; @@ -275,7 +275,7 @@ export class MembersTable extends BaseTable { } // If there are more than 6 domains, display a "View assigned domains" link - domainsHTML += `

View domains assigned

`; + domainsHTML += `

View domain assignments

`; domainsHTML += "
"; @@ -434,7 +434,7 @@ export class MembersTable extends BaseTable { } // Add a permissions header and wrap the entire output in a container - permissionsHTML = `

Member access and permissions

${permissionsHTML}
`; + permissionsHTML = `

Member access and permissions

${permissionsHTML}
`; return permissionsHTML; } From d7c76b6d20e612c661a413eace0f150ec25550bd Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:03:34 -0500 Subject: [PATCH 032/125] Domain assignments with s --- src/registrar/templates/portfolio_member.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 477599d49..46e0cced6 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -103,9 +103,9 @@ Organization member {% comment %}view_button is passed below as true in all cases. This is because editable logic will trump view_button logic; ie. if editable is true, view_button will never be looked at{% endcomment %} {% if portfolio_permission %} - {% include "includes/summary_item.html" with title='Domain assignment' domain_mgmt=True value=portfolio_permission.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Domain assignments' domain_mgmt=True value=portfolio_permission.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} {% elif portfolio_invitation %} - {% include "includes/summary_item.html" with title='Domain assignment' domain_mgmt=True value=portfolio_invitation.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Domain assignments' domain_mgmt=True value=portfolio_invitation.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} {% endif %}
From fe1780fbb921138e88a2faea6a734b5a2681f30c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:07:56 -0500 Subject: [PATCH 033/125] wip --- src/registrar/templates/portfolio_member.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 46e0cced6..b270e03fb 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -65,7 +65,7 @@ Organization member
{% csrf_token %}
- Last active: + Last active: {% if member and member.last_login %} {{ member.last_login }} {% elif portfolio_invitation %} @@ -75,7 +75,7 @@ Organization member {% endif %}
- Full name: + Full name: {% if member %} {% if member.first_name or member.last_name %} {{ member.get_formatted_name }} @@ -87,7 +87,7 @@ Organization member {% endif %}
- Title or organization role: + Title or organization role: {% if member and member.title %} {{ member.title }} {% else %} From 5216cb4a53ca9e8dc4e4b1aeaed01c60f12ff4ac Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:15:28 -0500 Subject: [PATCH 034/125] wip --- src/registrar/templates/portfolio_members_add_new.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/portfolio_members_add_new.html b/src/registrar/templates/portfolio_members_add_new.html index c3a648bdc..715ad53ab 100644 --- a/src/registrar/templates/portfolio_members_add_new.html +++ b/src/registrar/templates/portfolio_members_add_new.html @@ -67,7 +67,7 @@ {% include "includes/member_basic_permissions.html" %} -

Domain management

+

Domain assignments

After you invite this person to your organization, you can assign domain management permissions on their member profile.

From e031d04fa5cdb9559a318dcce806b412b35ced6a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:23:18 -0500 Subject: [PATCH 035/125] lint --- src/registrar/tests/test_views_portfolio.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c00892eca..e45f65f6f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -951,9 +951,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Admin") self.assertContains(response, "Creator") self.assertContains(response, "Manager") - self.assertContains( - response, 'This member does not manage any domains.' - ) + self.assertContains(response, "This member does not manage any domains.") # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present @@ -1066,9 +1064,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Viewer") self.assertContains(response, "Creator") self.assertContains(response, "Manager") - self.assertContains( - response, 'This member does not manage any domains.' - ) + self.assertContains(response, "This member does not manage any domains.") # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present self.assertContains(response, "sprite.svg#edit") # test that Edit link is present From 819fd35e591f2db7af19191e792d0c73e1a9780e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 18:04:17 -0500 Subject: [PATCH 036/125] remove last active on member page --- src/registrar/templates/portfolio_member.html | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index b270e03fb..b4b54291b 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -65,16 +65,6 @@ Organization member
{% csrf_token %}
- Last active: - {% if member and member.last_login %} - {{ member.last_login }} - {% elif portfolio_invitation %} - Invited - {% else %} - ⎯ - {% endif %} -
- Full name: {% if member %} {% if member.first_name or member.last_name %} From aa253a85dad9602c82e84786a4b3efb2c560c4d1 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 18:09:13 -0500 Subject: [PATCH 037/125] wip --- src/registrar/assets/src/js/getgov/table-members.js | 2 +- src/registrar/tests/test_views_portfolio.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index e876ce0f9..bb2d8c185 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -108,7 +108,7 @@ export class MembersTable extends BaseTable { `; - showMoreRow.innerHTML = `
${domainsHTML} ${permissionsHTML}
`; + showMoreRow.innerHTML = `
${domainsHTML} ${permissionsHTML}
`; showMoreRow.classList.add('show-more-content'); showMoreRow.classList.add('display-none'); showMoreRow.id = unique_id; diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index e45f65f6f..052155bb0 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -956,7 +956,7 @@ class TestPortfolio(WebTest): # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present self.assertContains(response, "sprite.svg#edit") # test that Edit link is present - self.assertContains(response, "sprite.svg#settings") # test that Manage link is present + self.assertContains(response, "sprite.svg#edit") # test that Manage link is present self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present @less_console_noise_decorator @@ -1068,7 +1068,7 @@ class TestPortfolio(WebTest): # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present self.assertContains(response, "sprite.svg#edit") # test that Edit link is present - self.assertContains(response, "sprite.svg#settings") # test that Manage link is present + self.assertContains(response, "sprite.svg#edit") # test that Manage link is present self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present @less_console_noise_decorator From d84aa421d90ba2d3a26bb998f428f3643ecf1841 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 10 Feb 2025 19:23:50 -0500 Subject: [PATCH 038/125] wip --- src/djangooidc/views.py | 2 + src/registrar/config/settings.py | 2 + src/registrar/decorators.py | 72 ++++++++++++++++++++ src/registrar/registrar_middleware.py | 39 +++++++++++ src/registrar/utility/domain_cache_helper.py | 0 src/registrar/views/domain_requests_json.py | 3 +- src/registrar/views/domains_json.py | 3 +- src/registrar/views/index.py | 2 + 8 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 src/registrar/decorators.py create mode 100644 src/registrar/utility/domain_cache_helper.py diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 815df4ecf..984936a4c 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -5,12 +5,14 @@ import logging from django.conf import settings from django.contrib.auth import logout as auth_logout from django.contrib.auth import authenticate, login +from login_required import login_not_required from django.http import HttpResponseRedirect from django.shortcuts import redirect from urllib.parse import parse_qs, urlencode from djangooidc.oidc import Client from djangooidc import exceptions as o_e +from registrar.decorators import grant_access from registrar.models import User from registrar.views.utility.error_views import custom_500_error_view, custom_401_error_view diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 78439188e..9b51383f3 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -200,6 +200,8 @@ MIDDLEWARE = [ "waffle.middleware.WaffleMiddleware", "registrar.registrar_middleware.CheckUserProfileMiddleware", "registrar.registrar_middleware.CheckPortfolioMiddleware", + # Restrict access using Opt-Out approach + "registrar.registrar_middleware.RestrictAccessMiddleware", ] # application object used by Django's built-in servers (e.g. `runserver`) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py new file mode 100644 index 000000000..08214f89c --- /dev/null +++ b/src/registrar/decorators.py @@ -0,0 +1,72 @@ +from functools import wraps +from django.http import JsonResponse +from django.core.exceptions import ObjectDoesNotExist + +from registrar.models.domain import Domain +from registrar.models.user_domain_role import UserDomainRole + +# Constants for clarity +ALL = "all" +IS_SUPERUSER = "is_superuser" +IS_STAFF = "is_staff" +IS_DOMAIN_MANAGER = "is_domain_manager" + +def grant_access(*rules): + """ + Allows multiple rules in a single decorator call: + @grant_access(IS_STAFF, IS_SUPERUSER, IS_DOMAIN_MANAGER) + or multiple stacked decorators: + @grant_access(IS_SUPERUSER) + @grant_access(IS_DOMAIN_MANAGER) + """ + + def decorator(view_func): + view_func.has_explicit_access = True # Mark as explicitly access-controlled + existing_rules = getattr(view_func, "_access_rules", set()) + existing_rules.update(rules) # Support multiple rules in one call + view_func._access_rules = existing_rules # Store rules on the function + + @wraps(view_func) + def wrapper(request, *args, **kwargs): + user = request.user + + # Skip authentication if @login_not_required is applied + if getattr(view_func, "login_not_required", False): + return view_func(request, *args, **kwargs) + + # Allow everyone if `ALL` is in rules + if ALL in view_func._access_rules: + return view_func(request, *args, **kwargs) + + # Ensure user is authenticated + if not user.is_authenticated: + return JsonResponse({"error": "Authentication required"}, status=403) + + conditions_met = [] + + if IS_STAFF in view_func._access_rules: + conditions_met.append(user.is_staff) + + if not any(conditions_met) and IS_SUPERUSER in view_func._access_rules: + conditions_met.append(user.is_superuser) + + if not any(conditions_met) and IS_DOMAIN_MANAGER in view_func._access_rules: + domain_id = kwargs.get('pk') or kwargs.get('domain_id') + if not domain_id: + return JsonResponse({"error": "Domain ID missing"}, status=400) + try: + domain = Domain.objects.get(pk=domain_id) + has_permission = UserDomainRole.objects.filter( + user=user, domain=domain + ).exists() + conditions_met.append(has_permission) + except ObjectDoesNotExist: + return JsonResponse({"error": "Invalid Domain"}, status=404) + + if not any(conditions_met): + return JsonResponse({"error": "Access Denied"}, status=403) + + return view_func(request, *args, **kwargs) + + return wrapper + return decorator diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index ed7a4dffc..c23fb0515 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -3,9 +3,13 @@ Contains middleware used in settings.py """ import logging +import re from urllib.parse import parse_qs +from django.conf import settings from django.urls import reverse from django.http import HttpResponseRedirect +from django.http import JsonResponse +from django.urls import resolve from registrar.models import User from waffle.decorators import flag_is_active @@ -170,3 +174,38 @@ class CheckPortfolioMiddleware: request.session["portfolio"] = request.user.get_first_portfolio() else: request.session["portfolio"] = request.user.get_first_portfolio() + + +class RestrictAccessMiddleware: + """ Middleware that blocks all views unless explicitly permitted """ + + def __init__(self, get_response): + self.get_response = get_response + self.ignored_paths = [re.compile(pattern) for pattern in getattr(settings, "LOGIN_REQUIRED_IGNORE_PATHS", [])] + + def __call__(self, request): + # Allow requests that match LOGIN_REQUIRED_IGNORE_PATHS + if any(pattern.match(request.path) for pattern in self.ignored_paths): + return self.get_response(request) + + # Try to resolve the view function + try: + resolver_match = resolve(request.path_info) + view_func = resolver_match.func + app_name = resolver_match.app_name # Get app name of resolved view + except Exception: + return JsonResponse({"error": "Not Found"}, status=404) + + # Auto-allow Django's built-in admin views (but NOT custom /admin/* views) + if app_name == "admin": + return self.get_response(request) + + # Skip access restriction if the view explicitly allows unauthenticated access + if getattr(view_func, "login_required", True) is False: + return self.get_response(request) + + # Enforce explicit access fules for other views + if not getattr(view_func, "has_explicit_access", False): + return JsonResponse({"error": "Access Denied"}, status=403) + + return self.get_response(request) \ No newline at end of file diff --git a/src/registrar/utility/domain_cache_helper.py b/src/registrar/utility/domain_cache_helper.py new file mode 100644 index 000000000..e69de29bb diff --git a/src/registrar/views/domain_requests_json.py b/src/registrar/views/domain_requests_json.py index 88590010e..b6a9d1072 100644 --- a/src/registrar/views/domain_requests_json.py +++ b/src/registrar/views/domain_requests_json.py @@ -1,5 +1,6 @@ from django.http import JsonResponse from django.core.paginator import Paginator +from registrar.decorators import grant_access, ALL from registrar.models import DomainRequest from django.utils.dateformat import format from django.contrib.auth.decorators import login_required @@ -7,7 +8,7 @@ from django.urls import reverse from django.db.models import Q -@login_required +@grant_access(ALL) def get_domain_requests_json(request): """Given the current request, get all domain requests that are associated with the request user and exclude the APPROVED ones. diff --git a/src/registrar/views/domains_json.py b/src/registrar/views/domains_json.py index 8734ef89c..e0081450d 100644 --- a/src/registrar/views/domains_json.py +++ b/src/registrar/views/domains_json.py @@ -1,6 +1,7 @@ import logging from django.http import JsonResponse from django.core.paginator import Paginator +from registrar.decorators import grant_access, ALL from registrar.models import UserDomainRole, Domain, DomainInformation, User from django.contrib.auth.decorators import login_required from django.urls import reverse @@ -9,7 +10,7 @@ from django.db.models import Q logger = logging.getLogger(__name__) -@login_required +@grant_access(ALL) def get_domains_json(request): """Given the current request, get all domains that are associated with the UserDomainRole object""" diff --git a/src/registrar/views/index.py b/src/registrar/views/index.py index be7149018..d9ff9b209 100644 --- a/src/registrar/views/index.py +++ b/src/registrar/views/index.py @@ -1,6 +1,8 @@ from django.shortcuts import render +from registrar.decorators import grant_access, ALL +@grant_access(ALL) def index(request): """This page is available to anyone without logging in.""" context = {} From a334f7dc3ef02c2aaa2a8bdb7f9fa43d5ac9e60d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 11 Feb 2025 10:06:24 -0500 Subject: [PATCH 039/125] wip --- src/registrar/config/urls.py | 29 +-- src/registrar/decorators.py | 176 +++++++++++++----- src/registrar/registrar_middleware.py | 8 +- src/registrar/templates/domain_add_user.html | 6 +- src/registrar/templates/domain_detail.html | 20 +- src/registrar/templates/domain_dns.html | 8 +- src/registrar/templates/domain_dnssec.html | 6 +- src/registrar/templates/domain_dsdata.html | 6 +- .../templates/domain_nameservers.html | 4 +- src/registrar/templates/domain_renewal.html | 8 +- .../templates/domain_security_email.html | 2 +- src/registrar/templates/domain_sidebar.html | 8 +- .../templates/domain_suborganization.html | 2 +- src/registrar/templates/domain_users.html | 8 +- src/registrar/views/domain.py | 41 ++-- src/registrar/views/domains_json.py | 2 +- src/registrar/views/utility/error_views.py | 8 + src/registrar/views/utility/mixins.py | 4 +- .../views/utility/permission_views.py | 1 + 19 files changed, 216 insertions(+), 131 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index eb095c5ca..27ad01dc8 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -296,56 +296,56 @@ urlpatterns = [ lambda r: always_404(r, "We forgot to include this link, sorry."), name="todo", ), - path("domain/", views.DomainView.as_view(), name="domain"), - path("domain//prototype-dns", views.PrototypeDomainDNSRecordView.as_view(), name="prototype-domain-dns"), - path("domain//users", views.DomainUsersView.as_view(), name="domain-users"), + path("domain/", views.DomainView.as_view(), name="domain"), + path("domain//prototype-dns", views.PrototypeDomainDNSRecordView.as_view(), name="prototype-domain-dns"), + path("domain//users", views.DomainUsersView.as_view(), name="domain-users"), path( - "domain//dns", + "domain//dns", views.DomainDNSView.as_view(), name="domain-dns", ), path( - "domain//dns/nameservers", + "domain//dns/nameservers", views.DomainNameserversView.as_view(), name="domain-dns-nameservers", ), path( - "domain//dns/dnssec", + "domain//dns/dnssec", views.DomainDNSSECView.as_view(), name="domain-dns-dnssec", ), path( - "domain//dns/dnssec/dsdata", + "domain//dns/dnssec/dsdata", views.DomainDsDataView.as_view(), name="domain-dns-dnssec-dsdata", ), path( - "domain//org-name-address", + "domain//org-name-address", views.DomainOrgNameAddressView.as_view(), name="domain-org-name-address", ), path( - "domain//suborganization", + "domain//suborganization", views.DomainSubOrganizationView.as_view(), name="domain-suborganization", ), path( - "domain//senior-official", + "domain//senior-official", views.DomainSeniorOfficialView.as_view(), name="domain-senior-official", ), path( - "domain//security-email", + "domain//security-email", views.DomainSecurityEmailView.as_view(), name="domain-security-email", ), path( - "domain//renewal", + "domain//renewal", views.DomainRenewalView.as_view(), name="domain-renewal", ), path( - "domain//users/add", + "domain//users/add", views.DomainAddUserView.as_view(), name="domain-users-add", ), @@ -370,7 +370,7 @@ urlpatterns = [ name="domain-request-delete", ), path( - "domain//users//delete", + "domain//users//delete", views.DomainDeleteUserView.as_view(http_method_names=["post"]), name="domain-user-delete", ), @@ -392,6 +392,7 @@ urlpatterns = [ # This way, we can share a view for djangooidc, and other pages as we see fit. handler500 = "registrar.views.utility.error_views.custom_500_error_view" handler403 = "registrar.views.utility.error_views.custom_403_error_view" +handler404 = "registrar.views.utility.error_views.custom_404_error_view" # we normally would guard these with `if settings.DEBUG` but tests run with # DEBUG = False even when these apps have been loaded because settings.DEBUG diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 08214f89c..177dfab62 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -1,15 +1,14 @@ -from functools import wraps -from django.http import JsonResponse -from django.core.exceptions import ObjectDoesNotExist - -from registrar.models.domain import Domain -from registrar.models.user_domain_role import UserDomainRole +import functools +from django.core.exceptions import PermissionDenied +from django.utils.decorators import method_decorator +from registrar.models import DomainInformation, DomainRequest, UserDomainRole # Constants for clarity ALL = "all" IS_SUPERUSER = "is_superuser" IS_STAFF = "is_staff" IS_DOMAIN_MANAGER = "is_domain_manager" +IS_STAFF_MANAGING_DOMAIN = "is_staff_managing_domain" def grant_access(*rules): """ @@ -20,53 +19,128 @@ def grant_access(*rules): @grant_access(IS_DOMAIN_MANAGER) """ - def decorator(view_func): - view_func.has_explicit_access = True # Mark as explicitly access-controlled - existing_rules = getattr(view_func, "_access_rules", set()) - existing_rules.update(rules) # Support multiple rules in one call - view_func._access_rules = existing_rules # Store rules on the function + def decorator(view): + if isinstance(view, type): # If decorating a class-based view (CBV) + original_dispatch = view.dispatch # save original dispatch method - @wraps(view_func) - def wrapper(request, *args, **kwargs): - user = request.user - - # Skip authentication if @login_not_required is applied - if getattr(view_func, "login_not_required", False): - return view_func(request, *args, **kwargs) - - # Allow everyone if `ALL` is in rules - if ALL in view_func._access_rules: - return view_func(request, *args, **kwargs) + @method_decorator(grant_access(*rules)) # apply the decorator to dispatch + def wrapped_dispatch(self, request, *args, **kwargs): + if not _user_has_permission(request.user, request, rules, **kwargs): + raise PermissionDenied + return original_dispatch(self, request, *args, **kwargs) - # Ensure user is authenticated - if not user.is_authenticated: - return JsonResponse({"error": "Authentication required"}, status=403) + view.dispatch = wrapped_dispatch # replace dispatch with wrapped version + return view - conditions_met = [] + else: # If decorating a function-based view (FBV) + view.has_explicit_access = True + existing_rules = getattr(view, "_access_rules", set()) + existing_rules.update(rules) + view._access_rules = existing_rules - if IS_STAFF in view_func._access_rules: - conditions_met.append(user.is_staff) - - if not any(conditions_met) and IS_SUPERUSER in view_func._access_rules: - conditions_met.append(user.is_superuser) - - if not any(conditions_met) and IS_DOMAIN_MANAGER in view_func._access_rules: - domain_id = kwargs.get('pk') or kwargs.get('domain_id') - if not domain_id: - return JsonResponse({"error": "Domain ID missing"}, status=400) - try: - domain = Domain.objects.get(pk=domain_id) - has_permission = UserDomainRole.objects.filter( - user=user, domain=domain - ).exists() - conditions_met.append(has_permission) - except ObjectDoesNotExist: - return JsonResponse({"error": "Invalid Domain"}, status=404) - - if not any(conditions_met): - return JsonResponse({"error": "Access Denied"}, status=403) - - return view_func(request, *args, **kwargs) - - return wrapper + @functools.wraps(view) + def wrapper(request, *args, **kwargs): + if not _user_has_permission(request.user, request, rules, **kwargs): + raise PermissionDenied + return view(request, *args, **kwargs) + + return wrapper + return decorator + + +def _user_has_permission(user, request, rules, **kwargs): + """ + Checks if the user meets the permission requirements. + """ + + # Skip authentication if @login_not_required is applied + if getattr(request, "login_not_required", False): + return True + + # Allow everyone if `ALL` is in rules + if ALL in rules: + return True + + # Ensure user is authenticated + if not user.is_authenticated: + return False + + conditions_met = [] + + if IS_STAFF in rules: + conditions_met.append(user.is_staff) + + if not any(conditions_met) and IS_SUPERUSER in rules: + conditions_met.append(user.is_superuser) + + if not any(conditions_met) and IS_DOMAIN_MANAGER in rules: + domain_id = kwargs.get('domain_pk') + # Check UserDomainRole directly instead of fetching Domain + has_permission = UserDomainRole.objects.filter(user=user, domain_id=domain_id).exists() + conditions_met.append(has_permission) + + if not any(conditions_met) and IS_STAFF_MANAGING_DOMAIN in rules: + domain_id = kwargs.get('domain_pk') + has_permission = _can_access_other_user_domains(request, domain_id) + conditions_met.append(has_permission) + + return any(conditions_met) + + +def _can_access_other_user_domains(request, domain_pk): + """Checks to see if an authorized user (staff or superuser) + can access a domain that they did not create or were invited to. + """ + + # Check if the request user is permissioned... + user_is_analyst_or_superuser = request.user.has_perm( + "registrar.analyst_access_permission" + ) or request.user.has_perm("registrar.full_access_permission") + + if not user_is_analyst_or_superuser: + return False + + # Check if the user is attempting a valid edit action. + # In other words, if the analyst/admin did not click + # the 'Manage Domain' button in /admin, + # then they cannot access this page. + session = request.session + can_do_action = ( + "analyst_action" in session + and "analyst_action_location" in session + and session["analyst_action_location"] == domain_pk + ) + + if not can_do_action: + return False + + # Analysts may manage domains, when they are in these statuses: + valid_domain_statuses = [ + DomainRequest.DomainRequestStatus.APPROVED, + DomainRequest.DomainRequestStatus.IN_REVIEW, + DomainRequest.DomainRequestStatus.REJECTED, + DomainRequest.DomainRequestStatus.ACTION_NEEDED, + # Edge case - some domains do not have + # a status or DomainInformation... aka a status of 'None'. + # It is necessary to access those to correct errors. + None, + ] + + requested_domain = DomainInformation.objects.filter(domain_id=domain_pk).first() + + # if no domain information or domain request exist, the user + # should be able to manage the domain; however, if domain information + # and domain request exist, and domain request is not in valid status, + # user should not be able to manage domain + if ( + requested_domain + and requested_domain.domain_request + and requested_domain.domain_request.status not in valid_domain_statuses + ): + return False + + # Valid session keys exist, + # the user is permissioned, + # and it is in a valid status + return True diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index c23fb0515..e3dcbe788 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -6,9 +6,9 @@ import logging import re from urllib.parse import parse_qs from django.conf import settings +from django.core.exceptions import PermissionDenied from django.urls import reverse from django.http import HttpResponseRedirect -from django.http import JsonResponse from django.urls import resolve from registrar.models import User from waffle.decorators import flag_is_active @@ -182,7 +182,7 @@ class RestrictAccessMiddleware: def __init__(self, get_response): self.get_response = get_response self.ignored_paths = [re.compile(pattern) for pattern in getattr(settings, "LOGIN_REQUIRED_IGNORE_PATHS", [])] - + def __call__(self, request): # Allow requests that match LOGIN_REQUIRED_IGNORE_PATHS if any(pattern.match(request.path) for pattern in self.ignored_paths): @@ -194,7 +194,7 @@ class RestrictAccessMiddleware: view_func = resolver_match.func app_name = resolver_match.app_name # Get app name of resolved view except Exception: - return JsonResponse({"error": "Not Found"}, status=404) + return self.get_response(request) # Auto-allow Django's built-in admin views (but NOT custom /admin/* views) if app_name == "admin": @@ -206,6 +206,6 @@ class RestrictAccessMiddleware: # Enforce explicit access fules for other views if not getattr(view_func, "has_explicit_access", False): - return JsonResponse({"error": "Access Denied"}, status=403) + raise PermissionDenied return self.get_response(request) \ No newline at end of file diff --git a/src/registrar/templates/domain_add_user.html b/src/registrar/templates/domain_add_user.html index 04565f61e..abc549a82 100644 --- a/src/registrar/templates/domain_add_user.html +++ b/src/registrar/templates/domain_add_user.html @@ -16,10 +16,10 @@ Domains
  • - {{ domain.name }} + {{ domain.name }}
  • - Domain managers + Domain managers
  • Add a domain manager @@ -27,7 +27,7 @@ {% else %} - {% url 'domain-users' pk=domain.id as url %} + {% url 'domain-users' domain_pk=domain.id as url %} {% elif steps.prev %} - + Previous step diff --git a/src/registrar/templates/domain_request_sidebar.html b/src/registrar/templates/domain_request_sidebar.html index 1af54bb24..e7a0a307c 100644 --- a/src/registrar/templates/domain_request_sidebar.html +++ b/src/registrar/templates/domain_request_sidebar.html @@ -15,7 +15,7 @@ {% endif %} {% endif %} - If you withdraw your request, we won't review it. Once you withdraw your request, you can edit it and submit it again.

    -

    Withdraw request - Cancel

    +

    Withdraw request + Cancel

    diff --git a/src/registrar/templates/includes/portfolio_request_review_steps.html b/src/registrar/templates/includes/portfolio_request_review_steps.html index 89c8e652a..53ad36a3f 100644 --- a/src/registrar/templates/includes/portfolio_request_review_steps.html +++ b/src/registrar/templates/includes/portfolio_request_review_steps.html @@ -4,7 +4,7 @@ {% for step in steps %}
    {% if is_editable %} - {% namespaced_url 'domain-request' step id=domain_request_id as domain_request_url %} + {% namespaced_url 'domain-request' step domain_request_pk=domain_request_id as domain_request_url %} {% endif %} {% if step == Step.REQUESTING_ENTITY %} diff --git a/src/registrar/templates/includes/request_review_steps.html b/src/registrar/templates/includes/request_review_steps.html index f1b13f890..5e00c2160 100644 --- a/src/registrar/templates/includes/request_review_steps.html +++ b/src/registrar/templates/includes/request_review_steps.html @@ -4,7 +4,7 @@ {% for step in steps %}
    {% if is_editable %} - {% namespaced_url 'domain-request' step id=domain_request_id as domain_request_url %} + {% namespaced_url 'domain-request' step domain_request_pk=domain_request_id as domain_request_url %} {% endif %} {% if step == Step.ORGANIZATION_TYPE %} diff --git a/src/registrar/templates/includes/request_status_manage.html b/src/registrar/templates/includes/request_status_manage.html index d96adedf6..f616522a7 100644 --- a/src/registrar/templates/includes/request_status_manage.html +++ b/src/registrar/templates/includes/request_status_manage.html @@ -114,7 +114,7 @@ {% block modify_request %} {% if DomainRequest.is_withdrawable %} -

    +

    Withdraw request

    {% endif %} diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 3248c1368..bcefbbc77 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -5,28 +5,27 @@ from django.shortcuts import redirect, render from django.urls import resolve, reverse from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ -from django.views.generic import TemplateView +from django.views.generic import DeleteView, DetailView, TemplateView from django.contrib import messages +from registrar.decorators import ( + HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT, + HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL, + IS_DOMAIN_REQUEST_CREATOR, + grant_access, +) from registrar.forms import domain_request_wizard as forms from registrar.forms.utility.wizard_form_helper import request_step_list from registrar.models import DomainRequest from registrar.models.contact import Contact from registrar.models.user import User from registrar.views.utility import StepsHelper -from registrar.views.utility.permission_views import DomainRequestPermissionDeleteView from registrar.utility.enums import Step, PortfolioDomainRequestStep -from .utility import ( - DomainRequestPermissionView, - DomainRequestPermissionWithdrawView, - DomainRequestWizardPermissionView, - DomainRequestPortfolioViewonlyView, -) - logger = logging.getLogger(__name__) -class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR, HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT) +class DomainRequestWizard(TemplateView): """ A common set of methods and configuration. @@ -51,7 +50,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): # NB: this is included here for reference. Do not change it without # also changing the many places it is hardcoded in the HTML templates URL_NAMESPACE = "domain-request" - # name for accessing /domain-request//edit + # name for accessing /domain-request//edit EDIT_URL_NAME = "edit-domain-request" NEW_URL_NAME = "start" FINISHED_URL_NAME = "finished" @@ -174,7 +173,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): def has_pk(self): """Does this wizard know about a DomainRequest database record?""" - return bool(self.kwargs.get("id") is not None) + return bool(self.kwargs.get("domain_request_pk") is not None) def get_step_enum(self): """Determines which step enum we should use for the wizard""" @@ -209,11 +208,11 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): try: self._domain_request = DomainRequest.objects.get( creator=creator, - pk=self.kwargs.get("id"), + pk=self.kwargs.get("domain_request_pk"), ) return self._domain_request except DomainRequest.DoesNotExist: - logger.debug("DomainRequest id %s did not have a DomainRequest" % id) + logger.debug("DomainRequest id %s did not have a DomainRequest" % self.kwargs.get("domain_request_pk")) # If a user is creating a request, we assume that perms are handled upstream if self.request.user.is_org_user(self.request): @@ -292,10 +291,10 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): current_url = resolve(request.path_info).url_name - # if user visited via an "edit" url, associate the id of the + # if user visited via an "edit" url, associate the pk of the # domain request they are trying to edit to this wizard instance # and remove any prior wizard data from their session - if current_url == self.EDIT_URL_NAME and "id" in kwargs: + if current_url == self.EDIT_URL_NAME and "domain_request_pk" in kwargs: del self.storage # if accessing this class directly, redirect to either to an acknowledgement @@ -474,7 +473,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): def goto(self, step): self.steps.current = step - return redirect(reverse(f"{self.URL_NAMESPACE}:{step}", kwargs={"id": self.domain_request.id})) + return redirect(reverse(f"{self.URL_NAMESPACE}:{step}", kwargs={"domain_request_pk": self.domain_request.id})) def goto_next_step(self): """Redirects to the next step.""" @@ -823,23 +822,12 @@ class Finished(DomainRequestWizard): return render(self.request, self.template_name, context) -class DomainRequestStatus(DomainRequestPermissionView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR, HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT) +class DomainRequestStatus(DetailView): template_name = "domain_request_status.html" - - def has_permission(self): - """ - Override of the base has_permission class to account for portfolio permissions - """ - has_base_perms = super().has_permission() - if not has_base_perms: - return False - - if self.request.user.is_org_user(self.request): - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_request_portfolio_permission(portfolio): - return False - - return True + model = DomainRequest + pk_url_kwarg = "domain_request_pk" + context_object_name = "DomainRequest" def get_context_data(self, **kwargs): """Context override to add a step list to the context""" @@ -854,19 +842,27 @@ class DomainRequestStatus(DomainRequestPermissionView): return context -class DomainRequestWithdrawConfirmation(DomainRequestPermissionWithdrawView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR) +class DomainRequestWithdrawConfirmation(DetailView): """This page will ask user to confirm if they want to withdraw - The DomainRequestPermissionView restricts access so that only the + Access is restricted so that only the `creator` of the domain request may withdraw it. """ - template_name = "domain_request_withdraw_confirmation.html" + template_name = "domain_request_withdraw_confirmation.html" # DetailView property for what model this is viewing + model = DomainRequest + pk_url_kwarg = "domain_request_pk" + context_object_name = "DomainRequest" -class DomainRequestWithdrawn(DomainRequestPermissionWithdrawView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR) +class DomainRequestWithdrawn(DetailView): # this view renders no template template_name = "" + model = DomainRequest + pk_url_kwarg = "domain_request_pk" + context_object_name = "DomainRequest" def get(self, *args, **kwargs): """View class that does the actual withdrawing. @@ -874,7 +870,7 @@ class DomainRequestWithdrawn(DomainRequestPermissionWithdrawView): If user click on withdraw confirm button, this view updates the status to withdraw and send back to homepage. """ - domain_request = DomainRequest.objects.get(id=self.kwargs["pk"]) + domain_request = DomainRequest.objects.get(id=self.kwargs["domain_request_pk"]) domain_request.withdraw() domain_request.save() if self.request.user.is_org_user(self.request): @@ -883,28 +879,22 @@ class DomainRequestWithdrawn(DomainRequestPermissionWithdrawView): return HttpResponseRedirect(reverse("home")) -class DomainRequestDeleteView(DomainRequestPermissionDeleteView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR, HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT) +class DomainRequestDeleteView(DeleteView): """Delete view for home that allows the end user to delete DomainRequests""" object: DomainRequest # workaround for type mismatch in DeleteView + model = DomainRequest + pk_url_kwarg = "domain_request_pk" def has_permission(self): """Custom override for has_permission to exclude all statuses, except WITHDRAWN and STARTED""" - has_perm = super().has_permission() - if not has_perm: - return False status = self.get_object().status valid_statuses = [DomainRequest.DomainRequestStatus.WITHDRAWN, DomainRequest.DomainRequestStatus.STARTED] if status not in valid_statuses: return False - # Portfolio users cannot delete their requests if they aren't permissioned to do so - if self.request.user.is_org_user(self.request): - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_request_portfolio_permission(portfolio): - return False - return True def get_success_url(self): @@ -989,8 +979,12 @@ class DomainRequestDeleteView(DomainRequestPermissionDeleteView): # region Portfolio views -class PortfolioDomainRequestStatusViewOnly(DomainRequestPortfolioViewonlyView): +@grant_access(HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL) +class PortfolioDomainRequestStatusViewOnly(DetailView): template_name = "portfolio_domain_request_status_viewonly.html" + model = DomainRequest + pk_url_kwarg = "domain_request_pk" + context_object_name = "DomainRequest" def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) diff --git a/src/registrar/views/domain_requests_json.py b/src/registrar/views/domain_requests_json.py index b6a9d1072..4e475321c 100644 --- a/src/registrar/views/domain_requests_json.py +++ b/src/registrar/views/domain_requests_json.py @@ -155,9 +155,9 @@ def serialize_domain_request(request, domain_request, user): # Map the action label to corresponding URLs and icons action_url_map = { - "Edit": reverse("edit-domain-request", kwargs={"id": domain_request.id}), - "Manage": reverse("domain-request-status", kwargs={"pk": domain_request.id}), - "View": reverse("domain-request-status-viewonly", kwargs={"pk": domain_request.id}), + "Edit": reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.id}), + "Manage": reverse("domain-request-status", kwargs={"domain_request_pk": domain_request.id}), + "View": reverse("domain-request-status-viewonly", kwargs={"domain_request_pk": domain_request.id}), } svg_icon_map = {"Edit": "edit", "Manage": "settings", "View": "visibility"} diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 0f93ec8e1..9f864324f 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -6,6 +6,7 @@ from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse from django.utils.safestring import mark_safe from django.contrib import messages +from registrar.decorators import HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM, grant_access from registrar.forms import portfolio as portfolioForms from registrar.models import Portfolio, User from registrar.models.domain import Domain @@ -25,7 +26,6 @@ from registrar.utility.errors import MissingEmailError from registrar.utility.enums import DefaultUserValues from registrar.views.utility.mixins import PortfolioMemberPermission from registrar.views.utility.permission_views import ( - PortfolioDomainRequestsPermissionView, PortfolioDomainsPermissionView, PortfolioBasePermissionView, NoPortfolioDomainsPermissionView, @@ -58,7 +58,8 @@ class PortfolioDomainsView(PortfolioDomainsPermissionView, View): return render(request, "portfolio_domains.html", context) -class PortfolioDomainRequestsView(PortfolioDomainRequestsPermissionView, View): +@grant_access(HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM) +class PortfolioDomainRequestsView(View): template_name = "portfolio_requests.html" diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index 6798eb4ee..4a33c9944 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -3,11 +3,7 @@ from .always_404 import always_404 from .permission_views import ( DomainPermissionView, - DomainRequestPermissionView, - DomainRequestPermissionWithdrawView, - DomainRequestWizardPermissionView, PortfolioMembersPermission, - DomainRequestPortfolioViewonlyView, DomainInvitationPermissionCancelView, ) from .api_views import get_senior_official_from_federal_agency_json diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 23bcff162..682d9ffcb 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -286,51 +286,6 @@ class DomainPermission(PermissionsLoginMixin): return True -class DomainRequestPermission(PermissionsLoginMixin): - """Permission mixin that redirects to domain request if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to this domain request. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - if not self.request.user.is_authenticated: - return False - - # user needs to be the creator of the domain request - # this query is empty if there isn't a domain request with this - # id and this user as creator - if not DomainRequest.objects.filter(creator=self.request.user, id=self.kwargs["pk"]).exists(): - return False - - return True - - -class DomainRequestPortfolioViewonlyPermission(PermissionsLoginMixin): - """Permission mixin that redirects to domain request if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to this domain request. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - if not self.request.user.is_authenticated: - return False - - if not self.request.user.is_org_user(self.request): - return False - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_all_requests_portfolio_permission(portfolio): - return False - - return True - - class UserDeleteDomainRolePermission(PermissionsLoginMixin): """Permission mixin for UserDomainRole if user has access, otherwise 403""" @@ -365,67 +320,6 @@ class UserDeleteDomainRolePermission(PermissionsLoginMixin): return True -class DomainRequestPermissionWithdraw(PermissionsLoginMixin): - """Permission mixin that redirects to withdraw action on domain request - if user has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to withdraw this domain request.""" - if not self.request.user.is_authenticated: - return False - - # user needs to be the creator of the domain request - # this query is empty if there isn't a domain request with this - # id and this user as creator - if not DomainRequest.objects.filter(creator=self.request.user, id=self.kwargs["pk"]).exists(): - return False - - # Restricted users should not be able to withdraw domain requests - if self.request.user.is_restricted(): - return False - - return True - - -class DomainRequestWizardPermission(PermissionsLoginMixin): - """Permission mixin that redirects to start or edit domain request if - user has access, otherwise 403""" - - def has_permission(self): - """Check if this user has permission to start or edit a domain request. - - The user is in self.request.user - """ - - if not self.request.user.is_authenticated: - return False - - # The user has an ineligible flag - if self.request.user.is_restricted(): - return False - - # If the user is an org user and doesn't have add/edit perms, forbid this - if self.request.user.is_org_user(self.request): - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_request_portfolio_permission(portfolio): - return False - - # user needs to be the creator of the domain request to edit it. - id = self.kwargs.get("id") if hasattr(self, "kwargs") else None - if not id: - domain_request_wizard = self.request.session.get("wizard_domain_request") - if domain_request_wizard: - id = domain_request_wizard.get("domain_request_id") - - # If no id is provided, we can assume that the user is starting a new request. - # If one IS provided, check that they are the original creator of it. - if id: - if not DomainRequest.objects.filter(creator=self.request.user, id=id).exists(): - return False - - return True - - class DomainInvitationPermission(PermissionsLoginMixin): """Permission mixin that redirects to domain invitation if user has access, otherwise 403" @@ -496,23 +390,6 @@ class PortfolioDomainsPermission(PortfolioBasePermission): return super().has_permission() -class PortfolioDomainRequestsPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio domain request pages if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to domain requests for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_any_requests_portfolio_permission(portfolio): - return False - - return super().has_permission() - - class PortfolioMembersPermission(PortfolioBasePermission): """Permission mixin that allows access to portfolio members pages if user has access, otherwise 403""" diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index b430845f4..41d620d42 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -2,18 +2,14 @@ import abc # abstract base class -from django.views.generic import DetailView, DeleteView, TemplateView, UpdateView -from registrar.models import Domain, DomainRequest, DomainInvitation, Portfolio +from django.views.generic import DetailView, DeleteView, UpdateView +from registrar.models import Domain, DomainInvitation, Portfolio from registrar.models.user import User from registrar.models.user_domain_role import UserDomainRole from .mixins import ( DomainPermission, - DomainRequestPermission, - DomainRequestPermissionWithdraw, DomainInvitationPermission, - DomainRequestWizardPermission, - PortfolioDomainRequestsPermission, PortfolioDomainsPermission, PortfolioMemberDomainsPermission, PortfolioMemberDomainsEditPermission, @@ -23,7 +19,6 @@ from .mixins import ( PortfolioBasePermission, PortfolioMembersPermission, PortfolioMemberPermission, - DomainRequestPortfolioViewonlyPermission, ) import logging @@ -86,77 +81,6 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): raise NotImplementedError -class DomainRequestPermissionView(DomainRequestPermission, DetailView, abc.ABC): - """Abstract base view for domain requests that enforces permissions - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = DomainRequest - # variable name in template context for the model object - context_object_name = "DomainRequest" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class DomainRequestPortfolioViewonlyView(DomainRequestPortfolioViewonlyPermission, DetailView, abc.ABC): - """Abstract base view for domain requests that enforces permissions - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = DomainRequest - # variable name in template context for the model object - context_object_name = "DomainRequest" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class DomainRequestPermissionWithdrawView(DomainRequestPermissionWithdraw, DetailView, abc.ABC): - """Abstract base view for domain request withdraw function - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = DomainRequest - # variable name in template context for the model object - context_object_name = "DomainRequest" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class DomainRequestWizardPermissionView(DomainRequestWizardPermission, TemplateView, abc.ABC): - """Abstract base view for the domain request form that enforces permissions - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - class DomainInvitationPermissionCancelView(DomainInvitationPermission, UpdateView, abc.ABC): """Abstract view for cancelling a DomainInvitation.""" @@ -164,13 +88,6 @@ class DomainInvitationPermissionCancelView(DomainInvitationPermission, UpdateVie object: DomainInvitation -class DomainRequestPermissionDeleteView(DomainRequestPermission, DeleteView, abc.ABC): - """Abstract view for deleting a DomainRequest.""" - - model = DomainRequest - object: DomainRequest - - class UserDomainRolePermissionDeleteView(UserDeleteDomainRolePermission, DeleteView, abc.ABC): """Abstract base view for deleting a UserDomainRole. @@ -242,14 +159,6 @@ class NoPortfolioDomainsPermissionView(PortfolioBasePermissionView, abc.ABC): """ -class PortfolioDomainRequestsPermissionView(PortfolioDomainRequestsPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio domain request views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - class PortfolioMembersPermissionView(PortfolioMembersPermission, PortfolioBasePermissionView, abc.ABC): """Abstract base view for portfolio members views that enforces permissions. From 40737cbcf7beb4e0d1af167e0e2898c0410eb7cb Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 11 Feb 2025 23:47:55 -0500 Subject: [PATCH 048/125] wip --- src/registrar/decorators.py | 14 ++++++++++- .../includes/domain_sidenav_item.html | 2 +- src/registrar/views/domain.py | 7 ++++-- src/registrar/views/domain_request.py | 2 +- src/registrar/views/domain_requests_json.py | 20 +++++++-------- src/registrar/views/portfolios.py | 18 ++++++++----- src/registrar/views/utility/mixins.py | 17 ------------- .../views/utility/permission_views.py | 25 ++++++------------- 8 files changed, 49 insertions(+), 56 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index cd19bcc95..fe71342cc 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -10,8 +10,10 @@ IS_STAFF = "is_staff" IS_DOMAIN_MANAGER = "is_domain_manager" IS_DOMAIN_REQUEST_CREATOR = "is_domain_request_creator" IS_STAFF_MANAGING_DOMAIN = "is_staff_managing_domain" +IS_PORTFOLIO_MEMBER = "is_portfolio_member" IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER = "is_portfolio_member_and_domain_manager" IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER = "is_domain_manager_and_not_portfolio_member" +HAS_PORTFOLIO_DOMAINS_ANY_PERM = "has_portfolio_domains_any_perm" HAS_PORTFOLIO_DOMAINS_VIEW_ALL = "has_portfolio_domains_view_all" HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM = "has_portfolio_domain_requests_any_perm" HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL = "has_portfolio_domain_requests_view_all" @@ -97,11 +99,21 @@ def _user_has_permission(user, request, rules, **kwargs): has_permission = _can_access_other_user_domains(request, domain_id) conditions_met.append(has_permission) + if not any(conditions_met) and IS_PORTFOLIO_MEMBER in rules: + has_permission = user.is_org_user(request) + conditions_met.append(has_permission) + if not any(conditions_met) and HAS_PORTFOLIO_DOMAINS_VIEW_ALL in rules: domain_id = kwargs.get("domain_pk") has_permission = _can_access_domain_via_portfolio_view_all_domains(request, domain_id) conditions_met.append(has_permission) + if not any(conditions_met) and HAS_PORTFOLIO_DOMAINS_ANY_PERM in rules: + has_permission = user.is_org_user(request) and user.has_any_domains_portfolio_permission( + request.session.get("portfolio") + ) + conditions_met.append(has_permission) + if not any(conditions_met) and IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER in rules: domain_id = kwargs.get("domain_pk") has_permission = _is_domain_manager(user, domain_id) and _is_portfolio_member(request) @@ -142,7 +154,7 @@ def _has_portfolio_domain_requests_edit(user, request, domain_request_id): if domain_request_id and not _is_domain_request_creator(user, domain_request_id): return False return user.is_org_user(request) and user.has_edit_request_portfolio_permission(request.session.get("portfolio")) - + def _is_domain_manager(user, domain_pk): """Checks to see if the user is a domain manager of the diff --git a/src/registrar/templates/includes/domain_sidenav_item.html b/src/registrar/templates/includes/domain_sidenav_item.html index 206e49c05..715f76865 100644 --- a/src/registrar/templates/includes/domain_sidenav_item.html +++ b/src/registrar/templates/includes/domain_sidenav_item.html @@ -1,6 +1,6 @@
  • {% if url_name %} - {% url url_name pk=domain.id as url %} + {% url url_name domain_pk=domain.id as url %} {% endif %} Date: Wed, 12 Feb 2025 08:16:16 -0500 Subject: [PATCH 049/125] additional cleanup of permission views and mixins, handling of domain invitations and user domain roles in decorators --- src/registrar/config/urls.py | 2 +- src/registrar/decorators.py | 72 +++++--- src/registrar/templates/domain_users.html | 2 +- src/registrar/views/domain.py | 154 ++++++++++++++++-- src/registrar/views/utility/__init__.py | 2 - src/registrar/views/utility/mixins.py | 153 ----------------- .../views/utility/permission_views.py | 94 +---------- 7 files changed, 196 insertions(+), 283 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 665f2cd82..12efe5a9f 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -364,7 +364,7 @@ urlpatterns = [ name="user-profile", ), path( - "invitation//cancel", + "invitation//cancel", views.DomainInvitationCancelView.as_view(http_method_names=["post"]), name="invitation-cancel", ), diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index fe71342cc..237985f02 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -1,7 +1,7 @@ import functools from django.core.exceptions import PermissionDenied from django.utils.decorators import method_decorator -from registrar.models import Domain, DomainInformation, DomainRequest, UserDomainRole +from registrar.models import Domain, DomainInformation, DomainInvitation, DomainRequest, UserDomainRole # Constants for clarity ALL = "all" @@ -18,7 +18,6 @@ HAS_PORTFOLIO_DOMAINS_VIEW_ALL = "has_portfolio_domains_view_all" HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM = "has_portfolio_domain_requests_any_perm" HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL = "has_portfolio_domain_requests_view_all" HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT = "has_portfolio_domain_requests_edit" -# HAS_PORTFOLIO_DOMAINS_VIEW_MANAGED = "has_portfolio_domains_view_managed" def grant_access(*rules): @@ -90,13 +89,11 @@ def _user_has_permission(user, request, rules, **kwargs): conditions_met.append(user.is_superuser) if not any(conditions_met) and IS_DOMAIN_MANAGER in rules: - domain_id = kwargs.get("domain_pk") - has_permission = _is_domain_manager(user, domain_id) + has_permission = _is_domain_manager(user, **kwargs) conditions_met.append(has_permission) if not any(conditions_met) and IS_STAFF_MANAGING_DOMAIN in rules: - domain_id = kwargs.get("domain_pk") - has_permission = _can_access_other_user_domains(request, domain_id) + has_permission = _is_staff_managing_domain(request, **kwargs) conditions_met.append(has_permission) if not any(conditions_met) and IS_PORTFOLIO_MEMBER in rules: @@ -115,13 +112,11 @@ def _user_has_permission(user, request, rules, **kwargs): conditions_met.append(has_permission) if not any(conditions_met) and IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER in rules: - domain_id = kwargs.get("domain_pk") - has_permission = _is_domain_manager(user, domain_id) and _is_portfolio_member(request) + has_permission = _is_domain_manager(user, **kwargs) and _is_portfolio_member(request) conditions_met.append(has_permission) if not any(conditions_met) and IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER in rules: - domain_id = kwargs.get("domain_pk") - has_permission = _is_domain_manager(user, domain_id) and not _is_portfolio_member(request) + has_permission = _is_domain_manager(user, **kwargs) and not _is_portfolio_member(request) conditions_met.append(has_permission) if not any(conditions_met) and IS_DOMAIN_REQUEST_CREATOR in rules: @@ -156,10 +151,24 @@ def _has_portfolio_domain_requests_edit(user, request, domain_request_id): return user.is_org_user(request) and user.has_edit_request_portfolio_permission(request.session.get("portfolio")) -def _is_domain_manager(user, domain_pk): - """Checks to see if the user is a domain manager of the - domain with domain_pk.""" - return UserDomainRole.objects.filter(user=user, domain_id=domain_pk).exists() +def _is_domain_manager(user, **kwargs): + """ + Determines if the given user is a domain manager for a specified domain. + + - First, it checks if 'domain_pk' is present in the URL parameters. + - If 'domain_pk' exists, it verifies if the user has a domain role for that domain. + - If 'domain_pk' is absent, it checks for 'domain_invitation_pk' to determine if the user has domain permissions through an invitation. + + Returns: + bool: True if the user is a domain manager, False otherwise. + """ + domain_id = kwargs.get("domain_pk") + if domain_id: + return UserDomainRole.objects.filter(user=user, domain_id=domain_id).exists() + domain_invitation_id = kwargs.get("domain_invitation_pk") + if domain_invitation_id: + return DomainInvitation.objects.filter(id=domain_invitation_id, domain__permissions__user=user).exists() + return False def _is_domain_request_creator(user, domain_request_pk): @@ -176,10 +185,35 @@ def _is_portfolio_member(request): return request.user.is_org_user(request) -def _can_access_other_user_domains(request, domain_pk): - """Checks to see if an authorized user (staff or superuser) - can access a domain that they did not create or were invited to. +def _is_staff_managing_domain(request, **kwargs): """ + Determines whether a staff user (analyst or superuser) has permission to manage a domain + that they did not create or were not invited to. + + The function enforces: + 1. **User Authorization** - The user must have `analyst_access_permission` or `full_access_permission`. + 2. **Valid Session Context** - The user must have explicitly selected the domain for management + via an 'analyst action' (e.g., by clicking 'Manage Domain' in the admin interface). + 3. **Domain Status Check** - Only domains in specific statuses (e.g., APPROVED, IN_REVIEW, etc.) + can be managed, except in cases where the domain lacks a status due to errors. + + Process: + - First, the function retrieves the `domain_pk` from the URL parameters. + - If `domain_pk` is not provided, it attempts to resolve the domain via `domain_invitation_pk`. + - It checks if the user has the required permissions. + - It verifies that the user has an active 'analyst action' session for the domain. + - Finally, it ensures that the domain is in a status that allows management. + + Returns: + bool: True if the user is allowed to manage the domain, False otherwise. + """ + + domain_id = kwargs.get("domain_pk") + if not domain_id: + domain_invitation_id = kwargs.get("domain_invitation_pk") + domain_invitation = DomainInvitation.objects.filter(id=domain_invitation_id).first() + if domain_invitation: + domain_id = domain_invitation.domain_id # Check if the request user is permissioned... user_is_analyst_or_superuser = request.user.has_perm( @@ -197,7 +231,7 @@ def _can_access_other_user_domains(request, domain_pk): can_do_action = ( "analyst_action" in session and "analyst_action_location" in session - and session["analyst_action_location"] == domain_pk + and session["analyst_action_location"] == domain_id ) if not can_do_action: @@ -215,7 +249,7 @@ def _can_access_other_user_domains(request, domain_pk): None, ] - requested_domain = DomainInformation.objects.filter(domain_id=domain_pk).first() + requested_domain = DomainInformation.objects.filter(domain_id=domain_id).first() # if no domain information or domain request exist, the user # should be able to manage the domain; however, if domain information diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index 984b4160b..4c5dbc728 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -154,7 +154,7 @@ {% if not portfolio %}{{ invitation.domain_invitation.status|title }}{% endif %} {% if invitation.domain_invitation.status == invitation.domain_invitation.DomainInvitationStatus.INVITED %} -
    + {% csrf_token %}
    {% endif %} diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 31f715d18..7a76284a4 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1,10 +1,3 @@ -"""Views for a single Domain. - -Authorization is handled by the `DomainPermissionView`. To ensure that only -authorized users can see information on a domain, every view here should -inherit from `DomainPermissionView` (or DomainInvitationPermissionCancelView). -""" - from datetime import date import logging import requests @@ -13,7 +6,7 @@ from django.contrib.messages.views import SuccessMessageMixin from django.http import HttpResponseRedirect from django.shortcuts import redirect, render, get_object_or_404 from django.urls import reverse -from django.views.generic import DeleteView +from django.views.generic import DeleteView, DetailView, UpdateView from django.views.generic.edit import FormMixin from django.conf import settings from registrar.decorators import ( @@ -49,7 +42,6 @@ from registrar.utility.errors import ( SecurityEmailErrorCodes, ) from registrar.models.utility.contact_error import ContactError -from registrar.views.utility.permission_views import UserDomainRolePermissionDeleteView from registrar.utility.waffle import flag_is_active_for_user from registrar.views.utility.invitation_helper import ( get_org_membership, @@ -76,19 +68,22 @@ from epplibwrapper import ( from ..utility.email import send_templated_email, EmailSendingError from ..utility.email_invitations import send_domain_invitation_email, send_portfolio_invitation_email -from .utility import DomainPermissionView, DomainInvitationPermissionCancelView from django import forms logger = logging.getLogger(__name__) -class DomainBaseView(DomainPermissionView): +class DomainBaseView(DetailView): """ Base View for the Domain. Handles getting and setting the domain in session cache on GETs. Also provides methods for getting and setting the domain in cache """ + model = Domain + pk_url_kwarg = "domain_pk" + context_object_name = "domain" + def get(self, request, *args, **kwargs): self._get_domain(request) context = self.get_context_data(object=self.object) @@ -120,6 +115,134 @@ class DomainBaseView(DomainPermissionView): domain_pk = "domain:" + str(self.kwargs.get("domain_pk")) self.session[domain_pk] = self.object + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + user = self.request.user + context["is_analyst_or_superuser"] = user.has_perm("registrar.analyst_access_permission") or user.has_perm( + "registrar.full_access_permission" + ) + context["is_domain_manager"] = UserDomainRole.objects.filter(user=user, domain=self.object).exists() + context["is_portfolio_user"] = self.can_access_domain_via_portfolio(self.object.pk) + context["is_editable"] = self.is_editable() + # Stored in a variable for the linter + action = "analyst_action" + action_location = "analyst_action_location" + # Flag to see if an analyst is attempting to make edits + if action in self.request.session: + context[action] = self.request.session[action] + if action_location in self.request.session: + context[action_location] = self.request.session[action_location] + + return context + + def is_editable(self): + """Returns whether domain is editable in the context of the view""" + domain_editable = self.object.is_editable() + if not domain_editable: + return False + + # if user is domain manager or analyst or admin, return True + if ( + self.can_access_other_user_domains(self.object.id) + or UserDomainRole.objects.filter(user=self.request.user, domain=self.object).exists() + ): + return True + + return False + + def can_access_domain_via_portfolio(self, pk): + """Most views should not allow permission to portfolio users. + If particular views allow access to the domain pages, they will need to override + this function. + """ + return False + + def has_permission(self): + """Check if this user has access to this domain. + + The user is in self.request.user and the domain needs to be looked + up from the domain's primary key in self.kwargs["domain_pk"] + """ + pk = self.kwargs["domain_pk"] + + # test if domain in editable state + if not self.in_editable_state(pk): + return False + + # if we need to check more about the nature of role, do it here. + return True + + def in_editable_state(self, pk): + """Is the domain in an editable state""" + + requested_domain = None + if Domain.objects.filter(id=pk).exists(): + requested_domain = Domain.objects.get(id=pk) + + # if domain is editable return true + if requested_domain and requested_domain.is_editable(): + return True + return False + + def can_access_other_user_domains(self, pk): + """Checks to see if an authorized user (staff or superuser) + can access a domain that they did not create or was invited to. + """ + + # Check if the user is permissioned... + user_is_analyst_or_superuser = self.request.user.has_perm( + "registrar.analyst_access_permission" + ) or self.request.user.has_perm("registrar.full_access_permission") + + if not user_is_analyst_or_superuser: + return False + + # Check if the user is attempting a valid edit action. + # In other words, if the analyst/admin did not click + # the 'Manage Domain' button in /admin, + # then they cannot access this page. + session = self.request.session + can_do_action = ( + "analyst_action" in session + and "analyst_action_location" in session + and session["analyst_action_location"] == pk + ) + + if not can_do_action: + return False + + # Analysts may manage domains, when they are in these statuses: + valid_domain_statuses = [ + DomainRequest.DomainRequestStatus.APPROVED, + DomainRequest.DomainRequestStatus.IN_REVIEW, + DomainRequest.DomainRequestStatus.REJECTED, + DomainRequest.DomainRequestStatus.ACTION_NEEDED, + # Edge case - some domains do not have + # a status or DomainInformation... aka a status of 'None'. + # It is necessary to access those to correct errors. + None, + ] + + requested_domain = None + if DomainInformation.objects.filter(id=pk).exists(): + requested_domain = DomainInformation.objects.get(id=pk) + + # if no domain information or domain request exist, the user + # should be able to manage the domain; however, if domain information + # and domain request exist, and domain request is not in valid status, + # user should not be able to manage domain + if ( + requested_domain + and requested_domain.domain_request + and requested_domain.domain_request.status not in valid_domain_statuses + ): + return False + + # Valid session keys exist, + # the user is permissioned, + # and it is in a valid status + return True + class DomainFormBaseView(DomainBaseView, FormMixin): """ @@ -433,7 +556,7 @@ class DomainOrgNameAddressView(DomainFormBaseView): return super().has_permission() -@grant_access(IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER) +@grant_access(IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER, IS_STAFF_MANAGING_DOMAIN) class DomainSubOrganizationView(DomainFormBaseView): """Suborganization view""" @@ -480,7 +603,7 @@ class DomainSubOrganizationView(DomainFormBaseView): return super().form_valid(form) -@grant_access(IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER) +@grant_access(IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER, IS_STAFF_MANAGING_DOMAIN) class DomainSeniorOfficialView(DomainFormBaseView): """Domain senior official editing view.""" @@ -1307,8 +1430,9 @@ class DomainAddUserView(DomainFormBaseView): @grant_access(IS_DOMAIN_MANAGER, IS_STAFF_MANAGING_DOMAIN) -class DomainInvitationCancelView(SuccessMessageMixin, DomainInvitationPermissionCancelView): - object: DomainInvitation +class DomainInvitationCancelView(SuccessMessageMixin, UpdateView): + model = DomainInvitation + pk_url_kwarg = "domain_invitation_pk" fields = [] def post(self, request, *args, **kwargs): diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index 4a33c9944..f80774ef3 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -2,8 +2,6 @@ from .steps_helper import StepsHelper from .always_404 import always_404 from .permission_views import ( - DomainPermissionView, PortfolioMembersPermission, - DomainInvitationPermissionCancelView, ) from .api_views import get_senior_official_from_federal_agency_json diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index ad0794edd..23895cb5c 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -1,14 +1,6 @@ """Permissions-related mixin classes.""" from django.contrib.auth.mixins import PermissionRequiredMixin - -from registrar.models import ( - Domain, - DomainRequest, - DomainInvitation, - DomainInformation, - UserDomainRole, -) import logging @@ -195,151 +187,6 @@ class PortfolioReportsPermission(PermissionsLoginMixin): return self.request.user.is_org_user(self.request) -class DomainPermission(PermissionsLoginMixin): - """Permission mixin that redirects to domain if user has access, - otherwise 403""" - - def has_permission(self): - """Check if this user has access to this domain. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["domain_pk"] - """ - pk = self.kwargs["domain_pk"] - - # test if domain in editable state - if not self.in_editable_state(pk): - return False - - # if we need to check more about the nature of role, do it here. - return True - - def in_editable_state(self, pk): - """Is the domain in an editable state""" - - requested_domain = None - if Domain.objects.filter(id=pk).exists(): - requested_domain = Domain.objects.get(id=pk) - - # if domain is editable return true - if requested_domain and requested_domain.is_editable(): - return True - return False - - def can_access_other_user_domains(self, pk): - """Checks to see if an authorized user (staff or superuser) - can access a domain that they did not create or was invited to. - """ - - # Check if the user is permissioned... - user_is_analyst_or_superuser = self.request.user.has_perm( - "registrar.analyst_access_permission" - ) or self.request.user.has_perm("registrar.full_access_permission") - - if not user_is_analyst_or_superuser: - return False - - # Check if the user is attempting a valid edit action. - # In other words, if the analyst/admin did not click - # the 'Manage Domain' button in /admin, - # then they cannot access this page. - session = self.request.session - can_do_action = ( - "analyst_action" in session - and "analyst_action_location" in session - and session["analyst_action_location"] == pk - ) - - if not can_do_action: - return False - - # Analysts may manage domains, when they are in these statuses: - valid_domain_statuses = [ - DomainRequest.DomainRequestStatus.APPROVED, - DomainRequest.DomainRequestStatus.IN_REVIEW, - DomainRequest.DomainRequestStatus.REJECTED, - DomainRequest.DomainRequestStatus.ACTION_NEEDED, - # Edge case - some domains do not have - # a status or DomainInformation... aka a status of 'None'. - # It is necessary to access those to correct errors. - None, - ] - - requested_domain = None - if DomainInformation.objects.filter(id=pk).exists(): - requested_domain = DomainInformation.objects.get(id=pk) - - # if no domain information or domain request exist, the user - # should be able to manage the domain; however, if domain information - # and domain request exist, and domain request is not in valid status, - # user should not be able to manage domain - if ( - requested_domain - and requested_domain.domain_request - and requested_domain.domain_request.status not in valid_domain_statuses - ): - return False - - # Valid session keys exist, - # the user is permissioned, - # and it is in a valid status - return True - - -class UserDeleteDomainRolePermission(PermissionsLoginMixin): - """Permission mixin for UserDomainRole if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to this domain request. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - domain_pk = self.kwargs["pk"] - user_pk = self.kwargs["user_pk"] - - if not self.request.user.is_authenticated: - return False - - # Check if the UserDomainRole object exists, then check - # if the user requesting the delete has permissions to do so - has_delete_permission = UserDomainRole.objects.filter( - user=user_pk, - domain=domain_pk, - domain__permissions__user=self.request.user, - ).exists() - - user_is_analyst_or_superuser = self.request.user.has_perm( - "registrar.analyst_access_permission" - ) or self.request.user.has_perm("registrar.full_access_permission") - - if not (has_delete_permission or user_is_analyst_or_superuser): - return False - - return True - - -class DomainInvitationPermission(PermissionsLoginMixin): - """Permission mixin that redirects to domain invitation if user has - access, otherwise 403" - - A user has access to a domain invitation if they have a role on the - associated domain. - """ - - def has_permission(self): - """Check if this user has a role on the domain of this invitation.""" - if not self.request.user.is_authenticated: - return False - - if not DomainInvitation.objects.filter( - id=self.kwargs["pk"], domain__permissions__user=self.request.user - ).exists(): - return False - return True - - class UserProfilePermission(PermissionsLoginMixin): """Permission mixin that redirects to user profile if user has access, otherwise 403""" diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index b2b56666f..02a2b029b 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -2,18 +2,14 @@ import abc # abstract base class -from django.views.generic import DetailView, DeleteView, UpdateView -from registrar.models import Domain, DomainInvitation, Portfolio +from django.views.generic import DetailView +from registrar.models import Portfolio from registrar.models.user import User -from registrar.models.user_domain_role import UserDomainRole from .mixins import ( - DomainPermission, - DomainInvitationPermission, PortfolioMemberDomainsPermission, PortfolioMemberDomainsEditPermission, PortfolioMemberEditPermission, - UserDeleteDomainRolePermission, UserProfilePermission, PortfolioBasePermission, PortfolioMembersPermission, @@ -24,92 +20,6 @@ import logging logger = logging.getLogger(__name__) -class DomainPermissionView(DomainPermission, DetailView, abc.ABC): - """Abstract base view for domains that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = Domain - pk_url_kwarg = "domain_pk" - # variable name in template context for the model object - context_object_name = "domain" - - # Adds context information for user permissions - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - user = self.request.user - context["is_analyst_or_superuser"] = user.has_perm("registrar.analyst_access_permission") or user.has_perm( - "registrar.full_access_permission" - ) - context["is_domain_manager"] = UserDomainRole.objects.filter(user=user, domain=self.object).exists() - context["is_portfolio_user"] = self.can_access_domain_via_portfolio(self.object.pk) - context["is_editable"] = self.is_editable() - # Stored in a variable for the linter - action = "analyst_action" - action_location = "analyst_action_location" - # Flag to see if an analyst is attempting to make edits - if action in self.request.session: - context[action] = self.request.session[action] - if action_location in self.request.session: - context[action_location] = self.request.session[action_location] - - return context - - def is_editable(self): - """Returns whether domain is editable in the context of the view""" - domain_editable = self.object.is_editable() - if not domain_editable: - return False - - # if user is domain manager or analyst or admin, return True - if ( - self.can_access_other_user_domains(self.object.id) - or UserDomainRole.objects.filter(user=self.request.user, domain=self.object).exists() - ): - return True - - return False - - def can_access_domain_via_portfolio(self, pk): - """Most views should not allow permission to portfolio users. - If particular views allow access to the domain pages, they will need to override - this function. - """ - return False - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class DomainInvitationPermissionCancelView(DomainInvitationPermission, UpdateView, abc.ABC): - """Abstract view for cancelling a DomainInvitation.""" - - model = DomainInvitation - object: DomainInvitation - - -class UserDomainRolePermissionDeleteView(UserDeleteDomainRolePermission, DeleteView, abc.ABC): - """Abstract base view for deleting a UserDomainRole. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = UserDomainRole - # workaround for type mismatch in DeleteView - object: UserDomainRole - - # variable name in template context for the model object - context_object_name = "userdomainrole" - - class UserProfilePermissionView(UserProfilePermission, DetailView, abc.ABC): """Abstract base view for user profile view that enforces permissions. From 908e71e3ae1d6de82cb42ce6e2282c9d44937178 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 09:53:55 -0500 Subject: [PATCH 050/125] portfolio member permissions --- src/registrar/decorators.py | 18 +++ src/registrar/views/member_domains_json.py | 27 ++--- src/registrar/views/portfolio_members_json.py | 7 +- src/registrar/views/portfolios.py | 89 +++++++++------ src/registrar/views/utility/__init__.py | 3 - src/registrar/views/utility/mixins.py | 107 ------------------ .../views/utility/permission_views.py | 67 ----------- 7 files changed, 90 insertions(+), 228 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 237985f02..98a37c62c 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -18,6 +18,8 @@ HAS_PORTFOLIO_DOMAINS_VIEW_ALL = "has_portfolio_domains_view_all" HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM = "has_portfolio_domain_requests_any_perm" HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL = "has_portfolio_domain_requests_view_all" HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT = "has_portfolio_domain_requests_edit" +HAS_PORTFOLIO_MEMBERS_ANY_PERM = "has_portfolio_members_any_perm" +HAS_PORTFOLIO_MEMBERS_EDIT = "has_portfolio_members_edit" def grant_access(*rules): @@ -142,6 +144,22 @@ def _user_has_permission(user, request, rules, **kwargs): print(has_permission) conditions_met.append(has_permission) + if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_ANY_PERM in rules: + portfolio = request.session.get("portfolio") + has_permission = user.is_org_user(request) and ( + user.has_view_members_portfolio_permission(portfolio) or + user.has_edit_members_portfolio_permission(portfolio) + ) + conditions_met.append(has_permission) + + if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_EDIT in rules: + portfolio = request.session.get("portfolio") + has_permission = ( + user.is_org_user(request) and + user.has_edit_members_portfolio_permission(portfolio) + ) + conditions_met.append(has_permission) + return any(conditions_met) diff --git a/src/registrar/views/member_domains_json.py b/src/registrar/views/member_domains_json.py index 3d24336bb..40b7bea74 100644 --- a/src/registrar/views/member_domains_json.py +++ b/src/registrar/views/member_domains_json.py @@ -4,37 +4,38 @@ from django.http import JsonResponse from django.core.paginator import Paginator from django.shortcuts import get_object_or_404 from django.views import View +from registrar.decorators import HAS_PORTFOLIO_MEMBERS_ANY_PERM, grant_access from registrar.models import UserDomainRole, Domain, DomainInformation, User from django.urls import reverse from django.db.models import Q from registrar.models.domain_invitation import DomainInvitation -from registrar.views.utility.mixins import PortfolioMemberDomainsPermission logger = logging.getLogger(__name__) -class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMemberDomainsJson(View): def get(self, request): """Given the current request, get all domains that are associated with the portfolio, or associated with the member/invited member""" - domain_ids = self.get_domain_ids_from_request(request) + domain_ids = self._get_domain_ids_from_request(request) objects = Domain.objects.filter(id__in=domain_ids).select_related("domain_info__sub_organization") unfiltered_total = objects.count() - objects = self.apply_search(objects, request) - objects = self.apply_sorting(objects, request) + objects = self._apply_search(objects, request) + objects = self._apply_sorting(objects, request) - paginator = Paginator(objects, self.get_page_size(request)) + paginator = Paginator(objects, self._get_page_size(request)) page_number = request.GET.get("page") page_obj = paginator.get_page(page_number) member_id = request.GET.get("member_id") - domains = [self.serialize_domain(domain, member_id, request.user) for domain in page_obj.object_list] + domains = [self._serialize_domain(domain, member_id, request.user) for domain in page_obj.object_list] return JsonResponse( { @@ -48,7 +49,7 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): } ) - def get_page_size(self, request): + def _get_page_size(self, request): """Gets the page size. If member_only, need to return the entire result set every time, so need @@ -65,7 +66,7 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): # later return 1000 - def get_domain_ids_from_request(self, request): + def _get_domain_ids_from_request(self, request): """Get domain ids from request. request.get.email - email address of invited member @@ -100,13 +101,13 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): logger.warning("Invalid search criteria, returning empty results list") return [] - def apply_search(self, queryset, request): + def _apply_search(self, queryset, request): search_term = request.GET.get("search_term") if search_term: queryset = queryset.filter(Q(name__icontains=search_term)) return queryset - def apply_sorting(self, queryset, request): + def _apply_sorting(self, queryset, request): # Get the sorting parameters from the request sort_by = request.GET.get("sort_by", "name") order = request.GET.get("order", "asc") @@ -141,7 +142,7 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): return queryset - def serialize_domain(self, domain, member_id, user): + def _serialize_domain(self, domain, member_id, user): suborganization_name = None try: domain_info = domain.domain_info @@ -176,7 +177,7 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): "state": domain.state, "state_display": domain.state_display(), "get_state_help_text": domain.get_state_help_text(), - "action_url": reverse("domain", kwargs={"pk": domain.id}), + "action_url": reverse("domain", kwargs={"domain_pk": domain.id}), "action_label": ("View" if view_only else "Manage"), "svg_icon": ("visibility" if view_only else "settings"), "domain_info__sub_organization": suborganization_name, diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 29dc6a71c..ecdd24441 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -6,16 +6,17 @@ from django.contrib.postgres.aggregates import ArrayAgg from django.urls import reverse from django.views import View +from registrar.decorators import HAS_PORTFOLIO_MEMBERS_ANY_PERM, grant_access from registrar.models.domain_invitation import DomainInvitation from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from registrar.views.utility.mixins import PortfolioMembersPermission from registrar.models.utility.orm_helper import ArrayRemoveNull from django.contrib.postgres.aggregates import StringAgg -class PortfolioMembersJson(PortfolioMembersPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMembersJson(View): def get(self, request): """Fetch members (permissions and invitations) for the given portfolio.""" @@ -236,7 +237,7 @@ class PortfolioMembersJson(PortfolioMembersPermission, View): ), # split domain_info array values into ids to form urls, and names "domain_urls": [ - reverse("domain", kwargs={"pk": domain_info.split(":")[0]}) for domain_info in domain_info_list + reverse("domain", kwargs={"domain_pk": domain_info.split(":")[0]}) for domain_info in domain_info_list ], "domain_names": [domain_info.split(":")[1] for domain_info in domain_info_list], "is_admin": is_admin, diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index dc1357585..f0c4841f1 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -5,20 +5,26 @@ from django.http import Http404, JsonResponse from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse from django.utils.safestring import mark_safe +from django.views.generic import DetailView from django.contrib import messages from registrar.decorators import ( HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM, HAS_PORTFOLIO_DOMAINS_ANY_PERM, + HAS_PORTFOLIO_MEMBERS_ANY_PERM, + HAS_PORTFOLIO_MEMBERS_EDIT, IS_PORTFOLIO_MEMBER, grant_access, ) from registrar.forms import portfolio as portfolioForms -from registrar.models import Portfolio, User -from registrar.models.domain import Domain -from registrar.models.domain_invitation import DomainInvitation -from registrar.models.portfolio_invitation import PortfolioInvitation -from registrar.models.user_domain_role import UserDomainRole -from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models import ( + Domain, + DomainInvitation, + Portfolio, + PortfolioInvitation, + User, + UserDomainRole, + UserPortfolioPermission +) from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError from registrar.utility.email_invitations import ( @@ -29,15 +35,6 @@ from registrar.utility.email_invitations import ( ) from registrar.utility.errors import MissingEmailError from registrar.utility.enums import DefaultUserValues -from registrar.views.utility.mixins import PortfolioMemberPermission -from registrar.views.utility.permission_views import ( - PortfolioBasePermissionView, - PortfolioMemberDomainsPermissionView, - PortfolioMemberDomainsEditPermissionView, - PortfolioMemberEditPermissionView, - PortfolioMemberPermissionView, - PortfolioMembersPermissionView, -) from django.views.generic import View from django.views.generic.edit import FormMixin from django.db import IntegrityError @@ -71,8 +68,10 @@ class PortfolioDomainRequestsView(View): return render(request, "portfolio_requests.html") -class PortfolioMemberView(PortfolioMemberPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMemberView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member.html" def get(self, request, pk): @@ -113,7 +112,8 @@ class PortfolioMemberView(PortfolioMemberPermissionView, View): ) -class PortfolioMemberDeleteView(PortfolioMemberPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMemberDeleteView(View): def post(self, request, pk): """ @@ -190,8 +190,10 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) +class PortfolioMemberEditView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member_permissions.html" form_class = portfolioForms.PortfolioMemberForm @@ -265,7 +267,8 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -class PortfolioMemberDomainsView(PortfolioMemberDomainsPermissionView, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMemberDomainsView(View): template_name = "portfolio_member_domains.html" @@ -283,8 +286,10 @@ class PortfolioMemberDomainsView(PortfolioMemberDomainsPermissionView, View): ) -class PortfolioMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) +class PortfolioMemberDomainsEditView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member_domains_edit.html" def get(self, request, pk): @@ -393,8 +398,10 @@ class PortfolioMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, V UserDomainRole.objects.filter(domain_id__in=removed_domain_ids, user=member).delete() -class PortfolioInvitedMemberView(PortfolioMemberPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioInvitedMemberView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member.html" # form_class = PortfolioInvitedMemberForm @@ -435,7 +442,8 @@ class PortfolioInvitedMemberView(PortfolioMemberPermissionView, View): ) -class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioInvitedMemberDeleteView(View): def post(self, request, pk): """ @@ -478,8 +486,10 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) +class PortfolioInvitedMemberEditView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member_permissions.html" form_class = portfolioForms.PortfolioInvitedMemberForm @@ -547,7 +557,8 @@ class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -class PortfolioInvitedMemberDomainsView(PortfolioMemberDomainsPermissionView, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioInvitedMemberDomainsView(View): template_name = "portfolio_member_domains.html" @@ -562,9 +573,11 @@ class PortfolioInvitedMemberDomainsView(PortfolioMemberDomainsPermissionView, Vi }, ) +@grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) +class PortfolioInvitedMemberDomainsEditView(DetailView, View): -class PortfolioInvitedMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, View): - + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member_domains_edit.html" def get(self, request, pk): @@ -749,7 +762,8 @@ class PortfolioNoDomainRequestsView(View): return context -class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): +@grant_access(IS_PORTFOLIO_MEMBER) +class PortfolioOrganizationView(DetailView, FormMixin): """ View to handle displaying and updating the portfolio's organization details. """ @@ -811,7 +825,8 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): return reverse("organization") -class PortfolioSeniorOfficialView(PortfolioBasePermissionView, FormMixin): +@grant_access(IS_PORTFOLIO_MEMBER) +class PortfolioSeniorOfficialView(DetailView, FormMixin): """ View to handle displaying and updating the portfolio's senior official details. For now, this view is readonly. @@ -842,7 +857,8 @@ class PortfolioSeniorOfficialView(PortfolioBasePermissionView, FormMixin): return self.render_to_response(self.get_context_data(form=form)) -class PortfolioMembersView(PortfolioMembersPermissionView, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMembersView(View): template_name = "portfolio_members.html" @@ -851,10 +867,13 @@ class PortfolioMembersView(PortfolioMembersPermissionView, View): return render(request, "portfolio_members.html") -class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioAddMemberView(DetailView, FormMixin): template_name = "portfolio_members_add_new.html" form_class = portfolioForms.PortfolioNewMemberForm + model = Portfolio + context_object_name = "portfolio" def get(self, request, *args, **kwargs): """Handle GET requests to display the form.""" diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index f80774ef3..12fcc325d 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -1,7 +1,4 @@ from .steps_helper import StepsHelper from .always_404 import always_404 -from .permission_views import ( - PortfolioMembersPermission, -) from .api_views import get_senior_official_from_federal_agency_json diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 23895cb5c..1762d4900 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -202,110 +202,3 @@ class UserProfilePermission(PermissionsLoginMixin): return False return True - - -class PortfolioBasePermission(PermissionsLoginMixin): - """Permission mixin that redirects to portfolio pages if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"] - """ - if not self.request.user.is_authenticated: - return False - - return self.request.user.is_org_user(self.request) - - -class PortfolioMembersPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio members pages if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to members for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ) and not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() - - -class PortfolioMemberPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio member or invited member pages if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to members or invited members for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ) and not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() - - -class PortfolioMemberEditPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio member or invited member pages if user - has access to edit, otherwise 403""" - - def has_permission(self): - """Check if this user has access to members or invited members for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() - - -class PortfolioMemberDomainsPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio member or invited member domains pages if user - has access to edit, otherwise 403""" - - def has_permission(self): - """Check if this user has access to member or invited member domains for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ) and not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() - - -class PortfolioMemberDomainsEditPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio member or invited member domains edit pages if user - has access to edit, otherwise 403""" - - def has_permission(self): - """Check if this user has access to member or invited member domains for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 02a2b029b..1961e1cdd 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -7,13 +7,7 @@ from registrar.models import Portfolio from registrar.models.user import User from .mixins import ( - PortfolioMemberDomainsPermission, - PortfolioMemberDomainsEditPermission, - PortfolioMemberEditPermission, UserProfilePermission, - PortfolioBasePermission, - PortfolioMembersPermission, - PortfolioMemberPermission, ) import logging @@ -37,64 +31,3 @@ class UserProfilePermissionView(UserProfilePermission, DetailView, abc.ABC): @abc.abstractmethod def template_name(self): raise NotImplementedError - - -class PortfolioBasePermissionView(PortfolioBasePermission, DetailView, abc.ABC): - """Abstract base view for portfolio views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = Portfolio - # variable name in template context for the model object - context_object_name = "portfolio" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class PortfolioMembersPermissionView(PortfolioMembersPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio members views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - -class PortfolioMemberPermissionView(PortfolioMemberPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio member views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - -class PortfolioMemberEditPermissionView(PortfolioMemberEditPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio member edit views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - -class PortfolioMemberDomainsPermissionView(PortfolioMemberDomainsPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio member domains views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - -class PortfolioMemberDomainsEditPermissionView( - PortfolioMemberDomainsEditPermission, PortfolioBasePermissionView, abc.ABC -): - """Abstract base view for portfolio member domains edit views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ From 4daac2bec03e452176c984f7ef4015609faf0a37 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 10:07:51 -0500 Subject: [PATCH 051/125] user profile permissions --- src/registrar/views/user_profile.py | 7 ++-- src/registrar/views/utility/mixins.py | 17 ---------- .../views/utility/permission_views.py | 33 ------------------- 3 files changed, 5 insertions(+), 52 deletions(-) delete mode 100644 src/registrar/views/utility/permission_views.py diff --git a/src/registrar/views/user_profile.py b/src/registrar/views/user_profile.py index 2012d12ab..3434eedb3 100644 --- a/src/registrar/views/user_profile.py +++ b/src/registrar/views/user_profile.py @@ -5,22 +5,25 @@ import logging from django.contrib import messages from django.http import QueryDict +from django.views.generic import DetailView from django.views.generic.edit import FormMixin +from registrar.decorators import ALL, grant_access from registrar.forms.user_profile import UserProfileForm, FinishSetupProfileForm from django.urls import NoReverseMatch, reverse from registrar.models.user import User from registrar.models.utility.generic_helper import replace_url_queryparams -from registrar.views.utility.permission_views import UserProfilePermissionView logger = logging.getLogger(__name__) -class UserProfileView(UserProfilePermissionView, FormMixin): +@grant_access(ALL) +class UserProfileView(DetailView, FormMixin): """ Base View for the User Profile. Handles getting and setting the User Profile """ model = User + context_object_name = "user" template_name = "profile.html" form_class = UserProfileForm base_view_name = "user-profile" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 1762d4900..d3ec95af5 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -185,20 +185,3 @@ class PortfolioReportsPermission(PermissionsLoginMixin): return False return self.request.user.is_org_user(self.request) - - -class UserProfilePermission(PermissionsLoginMixin): - """Permission mixin that redirects to user profile if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access. - - If the user is authenticated, they have access - """ - - # Check if the user is authenticated - if not self.request.user.is_authenticated: - return False - - return True diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py deleted file mode 100644 index 1961e1cdd..000000000 --- a/src/registrar/views/utility/permission_views.py +++ /dev/null @@ -1,33 +0,0 @@ -"""View classes that enforce authorization.""" - -import abc # abstract base class - -from django.views.generic import DetailView -from registrar.models import Portfolio -from registrar.models.user import User - -from .mixins import ( - UserProfilePermission, -) -import logging - -logger = logging.getLogger(__name__) - - -class UserProfilePermissionView(UserProfilePermission, DetailView, abc.ABC): - """Abstract base view for user profile view that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = User - # variable name in template context for the model object - context_object_name = "user" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError From 9ad7b7523b242cd982a973da5567807aa02e88c1 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 10:46:06 -0500 Subject: [PATCH 052/125] analytics, reports and transfer user --- src/registrar/decorators.py | 9 +++++ src/registrar/views/report_views.py | 28 +++++++------- src/registrar/views/transfer_user.py | 2 + src/registrar/views/utility/mixins.py | 54 +-------------------------- 4 files changed, 26 insertions(+), 67 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 98a37c62c..0f2c948b3 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -20,6 +20,7 @@ HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL = "has_portfolio_domain_requests_view_all HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT = "has_portfolio_domain_requests_edit" HAS_PORTFOLIO_MEMBERS_ANY_PERM = "has_portfolio_members_any_perm" HAS_PORTFOLIO_MEMBERS_EDIT = "has_portfolio_members_edit" +HAS_PORTFOLIO_MEMBERS_VIEW = "has_portfolio_members_view" def grant_access(*rules): @@ -160,6 +161,14 @@ def _user_has_permission(user, request, rules, **kwargs): ) conditions_met.append(has_permission) + if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_VIEW in rules: + portfolio = request.session.get("portfolio") + has_permission = ( + user.is_org_user(request) and + user.has_view_members_portfolio_permission(portfolio) + ) + conditions_met.append(has_permission) + return any(conditions_met) diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index 1b9198c79..d64586a2b 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -6,19 +6,17 @@ from django.shortcuts import render from django.contrib import admin from django.db.models import Avg, F -from registrar.views.utility.mixins import DomainAndRequestsReportsPermission, PortfolioReportsPermission +from registrar.decorators import ALL, HAS_PORTFOLIO_MEMBERS_VIEW, IS_STAFF, grant_access from .. import models import datetime from django.utils import timezone -from django.contrib.admin.views.decorators import staff_member_required -from django.utils.decorators import method_decorator from registrar.utility import csv_export import logging logger = logging.getLogger(__name__) -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class AnalyticsView(View): def get(self, request): thirty_days_ago = datetime.datetime.today() - datetime.timedelta(days=30) @@ -152,7 +150,7 @@ class AnalyticsView(View): return render(request, "admin/analytics.html", context) -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataType(View): def get(self, request, *args, **kwargs): # match the CSV example with all the fields @@ -162,7 +160,8 @@ class ExportDataType(View): return response -class ExportDataTypeUser(DomainAndRequestsReportsPermission, View): +@grant_access(ALL) +class ExportDataTypeUser(View): """Returns a domain report for a given user on the request""" def get(self, request, *args, **kwargs): @@ -173,7 +172,8 @@ class ExportDataTypeUser(DomainAndRequestsReportsPermission, View): return response -class ExportMembersPortfolio(PortfolioReportsPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW) +class ExportMembersPortfolio(View): """Returns a members report for a given portfolio""" def get(self, request, *args, **kwargs): @@ -201,7 +201,7 @@ class ExportMembersPortfolio(PortfolioReportsPermission, View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataFull(View): def get(self, request, *args, **kwargs): # Smaller export based on 1 @@ -211,7 +211,7 @@ class ExportDataFull(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataFederal(View): def get(self, request, *args, **kwargs): # Federal only @@ -221,7 +221,7 @@ class ExportDataFederal(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDomainRequestDataFull(View): """Generates a downloaded report containing all Domain Requests (except started)""" @@ -233,7 +233,7 @@ class ExportDomainRequestDataFull(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataDomainsGrowth(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") @@ -246,7 +246,7 @@ class ExportDataDomainsGrowth(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataRequestsGrowth(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") @@ -259,7 +259,7 @@ class ExportDataRequestsGrowth(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataManagedDomains(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") @@ -271,7 +271,7 @@ class ExportDataManagedDomains(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataUnmanagedDomains(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py index f574b76d9..62cd0a9d2 100644 --- a/src/registrar/views/transfer_user.py +++ b/src/registrar/views/transfer_user.py @@ -4,6 +4,7 @@ from django.db.models import ForeignKey, OneToOneField, ManyToManyField, ManyToO from django.shortcuts import render, get_object_or_404, redirect from django.views import View +from registrar.decorators import IS_STAFF, grant_access from registrar.models.domain import Domain from registrar.models.domain_request import DomainRequest from registrar.models.user import User @@ -18,6 +19,7 @@ from registrar.utility.db_helpers import ignore_unique_violation logger = logging.getLogger(__name__) +@grant_access(IS_STAFF) class TransferUserView(View): """Transfer user methods that set up the transfer_user template and handle the forms on it.""" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index d3ec95af5..eb58a5125 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -1,6 +1,4 @@ -"""Permissions-related mixin classes.""" - -from django.contrib.auth.mixins import PermissionRequiredMixin +"""Mixin classes.""" import logging @@ -135,53 +133,3 @@ class OrderableFieldsMixin: # Infer the column name in a similar manner to how Django does method.short_description = field.replace("_", " ") return method - - -class PermissionsLoginMixin(PermissionRequiredMixin): - """Mixin that redirects to login page if not logged in, otherwise 403.""" - - def handle_no_permission(self): - self.raise_exception = self.request.user.is_authenticated - return super().handle_no_permission() - - -class DomainAndRequestsReportsPermission(PermissionsLoginMixin): - """Permission mixin for domain and requests csv downloads""" - - def has_permission(self): - """Check if this user has access to this domain. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - - if not self.request.user.is_authenticated: - return False - - if self.request.user.is_restricted(): - return False - - return True - - -class PortfolioReportsPermission(PermissionsLoginMixin): - """Permission mixin for portfolio csv downloads""" - - def has_permission(self): - """Check if this user has access to this domain. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - - if not self.request.user.is_authenticated: - return False - - if self.request.user.is_restricted(): - return False - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission(portfolio): - return False - - return self.request.user.is_org_user(self.request) From 7a1348258d14d443b8f44392652ece8ea95e6169 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 10:50:42 -0500 Subject: [PATCH 053/125] remove superuser, and assign perm to transfer user --- src/registrar/decorators.py | 18 ++++-------------- src/registrar/views/portfolios.py | 3 ++- src/registrar/views/utility/mixins.py | 1 + 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 0f2c948b3..517985b6d 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -5,7 +5,6 @@ from registrar.models import Domain, DomainInformation, DomainInvitation, Domain # Constants for clarity ALL = "all" -IS_SUPERUSER = "is_superuser" IS_STAFF = "is_staff" IS_DOMAIN_MANAGER = "is_domain_manager" IS_DOMAIN_REQUEST_CREATOR = "is_domain_request_creator" @@ -88,9 +87,6 @@ def _user_has_permission(user, request, rules, **kwargs): if IS_STAFF in rules: conditions_met.append(user.is_staff) - if not any(conditions_met) and IS_SUPERUSER in rules: - conditions_met.append(user.is_superuser) - if not any(conditions_met) and IS_DOMAIN_MANAGER in rules: has_permission = _is_domain_manager(user, **kwargs) conditions_met.append(has_permission) @@ -148,25 +144,19 @@ def _user_has_permission(user, request, rules, **kwargs): if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_ANY_PERM in rules: portfolio = request.session.get("portfolio") has_permission = user.is_org_user(request) and ( - user.has_view_members_portfolio_permission(portfolio) or - user.has_edit_members_portfolio_permission(portfolio) + user.has_view_members_portfolio_permission(portfolio) + or user.has_edit_members_portfolio_permission(portfolio) ) conditions_met.append(has_permission) if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_EDIT in rules: portfolio = request.session.get("portfolio") - has_permission = ( - user.is_org_user(request) and - user.has_edit_members_portfolio_permission(portfolio) - ) + has_permission = user.is_org_user(request) and user.has_edit_members_portfolio_permission(portfolio) conditions_met.append(has_permission) if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_VIEW in rules: portfolio = request.session.get("portfolio") - has_permission = ( - user.is_org_user(request) and - user.has_view_members_portfolio_permission(portfolio) - ) + has_permission = user.is_org_user(request) and user.has_view_members_portfolio_permission(portfolio) conditions_met.append(has_permission) return any(conditions_met) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index f0c4841f1..41038443b 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -23,7 +23,7 @@ from registrar.models import ( PortfolioInvitation, User, UserDomainRole, - UserPortfolioPermission + UserPortfolioPermission, ) from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError @@ -573,6 +573,7 @@ class PortfolioInvitedMemberDomainsView(View): }, ) + @grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) class PortfolioInvitedMemberDomainsEditView(DetailView, View): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index eb58a5125..e228fbcd8 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -1,4 +1,5 @@ """Mixin classes.""" + import logging From 76b72d19627af77fe21e27c7c34597d82aaf0250 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 12 Feb 2025 15:49:12 -0500 Subject: [PATCH 054/125] kebob --- .../assets/src/js/getgov/portfolio-member-page.js | 2 +- src/registrar/assets/src/js/getgov/table-base.js | 6 +++--- .../assets/src/sass/_theme/_accordions.scss | 12 +++++++++++- src/registrar/templates/portfolio_member.html | 10 ++++++++++ 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index 95723fc7e..602fbc65e 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -18,7 +18,7 @@ export function initPortfolioNewMemberPageToggle() { const unique_id = `${member_type}-${member_id}`; let cancelInvitationButton = member_type === "invitedmember" ? "Cancel invitation" : "Remove member"; - wrapperDeleteAction.innerHTML = generateKebabHTML('remove-member', unique_id, cancelInvitationButton, `More Options for ${member_name}`); + wrapperDeleteAction.innerHTML = generateKebabHTML('remove-member', unique_id, cancelInvitationButton, `More Options for ${member_name}`, "usa-icon--large"); // This easter egg is only for fixtures that dont have names as we are displaying their emails // All prod users will have emails linked to their account diff --git a/src/registrar/assets/src/js/getgov/table-base.js b/src/registrar/assets/src/js/getgov/table-base.js index ce4397887..f36fbc7b1 100644 --- a/src/registrar/assets/src/js/getgov/table-base.js +++ b/src/registrar/assets/src/js/getgov/table-base.js @@ -79,7 +79,7 @@ export function addModal(id, ariaLabelledby, ariaDescribedby, modalHeading, moda * @param {string} modal_button_text - The action button's text * @param {string} screen_reader_text - A screen reader helper */ -export function generateKebabHTML(action, unique_id, modal_button_text, screen_reader_text) { +export function generateKebabHTML(action, unique_id, modal_button_text, screen_reader_text, icon_class) { const generateModalButton = (mobileOnly = false) => `
    - {{ block.super }} -{% endblock %} \ No newline at end of file +{% endblock %} From 3795e1405885a56823bb775463b3c860516bd71c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 20 Feb 2025 22:08:11 -0500 Subject: [PATCH 125/125] refactor summary_item --- .../src/js/getgov/portfolio-member-page.js | 2 +- .../assets/src/sass/_theme/_accordions.scss | 2 + .../templates/includes/summary_item.html | 41 ++++++++++--------- 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index 0510c875d..96961e5dc 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -100,7 +100,7 @@ export function initAddNewMemberPageListeners() { const permissionSections = document.querySelectorAll(`#${permission_details_div_id} > h3`); permissionSections.forEach(section => { - // Find the

    element text + // Find the

    element text, strip out the '*' const sectionTitle = section.textContent.trim().replace(/\*$/, "") + ": "; // Find the associated radio buttons container (next fieldset) diff --git a/src/registrar/assets/src/sass/_theme/_accordions.scss b/src/registrar/assets/src/sass/_theme/_accordions.scss index 803a4510a..86b8779ab 100644 --- a/src/registrar/assets/src/sass/_theme/_accordions.scss +++ b/src/registrar/assets/src/sass/_theme/_accordions.scss @@ -47,6 +47,8 @@ } .usa-accordion--more-actions .usa-accordion__content { + // We need to match the height of the trigger button + // to align the 'popup' underneath top: 20px; &.top-28px { top: 28px; diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index a091e5ab7..0ce7910bb 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -134,25 +134,28 @@ {% endif %} - {% if editable and edit_link or view_button %} - + {% comment %}We have conditions where an edit_link is set but editable can be true or false{% endcomment %} + {% if edit_link %} + {% if manage_button or editable or view_button %} + + {% endif %} {% endif %}