diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 4e9375d29..940183646 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -519,6 +519,7 @@ def multiple_unalphabetical_domain_objects( applications.append(application) return applications + def generic_domain_object(domain_type, object_name): """Returns a generic domain object of domain_type 'application', 'information', or 'invitation'""" diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 84b4535b6..f4e7dae3a 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -14,7 +14,7 @@ from registrar.models import ( DomainInformation, User, DomainInvitation, - Domain + Domain, ) from .common import ( completed_application, @@ -643,12 +643,13 @@ class AuditedAdminTest(TestCase): class DomainSessionVariableTest(TestCase): """Test cases for session variables in Django Admin""" + def setUp(self): self.factory = RequestFactory() self.admin = DomainAdmin(Domain, None) self.client = Client(HTTP_HOST="localhost:8080") - def test_session_variables_set_correctly(self): + def test_session_vars_set_correctly(self): """Checks if session variables are being set correctly""" p = "adminpass" @@ -657,26 +658,85 @@ class DomainSessionVariableTest(TestCase): dummy_domain_information = generic_domain_object("information", "session") request = self.get_factory_post_edit_domain(dummy_domain_information.domain.pk) self.populate_session_values(request, dummy_domain_information.domain) + self.assertEqual(request.session["analyst_action"], "edit") + self.assertEqual( + request.session["analyst_action_location"], + dummy_domain_information.domain.pk, + ) - self.assertEqual(request.session['analyst_action'], 'edit') - self.assertEqual(request.session['analyst_action_location'], dummy_domain_information.domain.pk) - - def test_session_variables_retain_information(self): - """ Checks to see if session variables retain old information """ + def test_session_vars_set_correctly_hardcoded_domain(self): + """Checks if session variables are being set correctly""" p = "adminpass" self.client.login(username="superuser", password=p) - dummy_domain_information_list = multiple_unalphabetical_domain_objects("information") + dummy_domain_information: Domain = generic_domain_object( + "information", "session" + ) + dummy_domain_information.domain.pk = 1 + request = self.get_factory_post_edit_domain(dummy_domain_information.domain.pk) + self.populate_session_values(request, dummy_domain_information.domain) + self.assertEqual(request.session["analyst_action"], "edit") + self.assertEqual(request.session["analyst_action_location"], 1) + + def test_session_variables_reset_correctly(self): + """Checks if incorrect session variables get overridden""" + + p = "adminpass" + self.client.login(username="superuser", password=p) + + dummy_domain_information = generic_domain_object("information", "session") + request = self.get_factory_post_edit_domain(dummy_domain_information.domain.pk) + + self.populate_session_values( + request, dummy_domain_information.domain, preloadBadData=True + ) + + self.assertEqual(request.session["analyst_action"], "edit") + self.assertEqual( + request.session["analyst_action_location"], + dummy_domain_information.domain.pk, + ) + + def test_unauthorized_user(self): + """Checks for when a user has invalid perms""" + + p = "userpass" + self.client.login(username="staffuser", password=p) + + dummy_domain_information = generic_domain_object("information", "session") + request = self.get_factory_post_edit_domain(dummy_domain_information.domain.pk) + self.populate_session_values(request, dummy_domain_information.domain) + + self.assertEqual(request.session["analyst_action"], "edit") + self.assertEqual( + request.session["analyst_action_location"], + dummy_domain_information.domain.pk, + ) + + def test_session_variables_retain_information(self): + """Checks to see if session variables retain old information""" + + p = "adminpass" + self.client.login(username="superuser", password=p) + + dummy_domain_information_list = multiple_unalphabetical_domain_objects( + "information" + ) for item in dummy_domain_information_list: request = self.get_factory_post_edit_domain(item.domain.pk) + logger.debug( + f"Before populate - Domain Pk: {item.domain.pk} obj pk: {item.pk}" + ) self.populate_session_values(request, item) - - self.assertEqual(request.session['analyst_action'], 'edit') - self.assertEqual(request.session['analyst_action_location'], item.domain.pk) + logger.debug( + f"After populate - Domain Pk: {item.domain.pk} obj pk: {item.pk}" + ) + self.assertEqual(request.session["analyst_action"], "edit") + self.assertEqual(request.session["analyst_action_location"], item.domain.pk) def test_session_variables_concurrent_requests(self): - """ Simulates two requests at once """ + """Simulates two requests at once""" p = "adminpass" self.client.login(username="superuser", password=p) @@ -691,23 +751,29 @@ class DomainSessionVariableTest(TestCase): self.populate_session_values(request_second, info_second.domain) # Check if anything got nulled out - self.assertNotEqual(request_first.session['analyst_action'], None) - self.assertNotEqual(request_second.session['analyst_action'], None) - self.assertNotEqual(request_first.session['analyst_action_location'], None) - self.assertNotEqual(request_second.session['analyst_action_location'], None) + self.assertNotEqual(request_first.session["analyst_action"], None) + self.assertNotEqual(request_second.session["analyst_action"], None) + self.assertNotEqual(request_first.session["analyst_action_location"], None) + self.assertNotEqual(request_second.session["analyst_action_location"], None) # Check if they are both the same action 'type' - self.assertEqual(request_first.session['analyst_action'], 'edit') - self.assertEqual(request_second.session['analyst_action'], 'edit') + self.assertEqual(request_first.session["analyst_action"], "edit") + self.assertEqual(request_second.session["analyst_action"], "edit") # Check their locations, and ensure they aren't the same across both - self.assertNotEqual(request_first.session['analyst_action_location'], request_second.session['analyst_action_location']) + self.assertNotEqual( + request_first.session["analyst_action_location"], + request_second.session["analyst_action_location"], + ) - def populate_session_values(self, request, domain_object): + def populate_session_values(self, request, domain_object, preloadBadData=False): """Boilerplate for creating mock sessions""" request.user = self.client request.session = SessionStore() request.session.create() + if preloadBadData: + request.session["analyst_action"] = "invalid" + request.session["analyst_action_location"] = "bad location" self.admin.response_change(request, domain_object) def get_factory_post_edit_domain(self, primary_key): @@ -715,7 +781,7 @@ class DomainSessionVariableTest(TestCase): with the edit domain button 'clicked', then returns the factory object""" return self.factory.post( - reverse('admin:registrar_domain_change', args=(primary_key,)), - {'_edit_domain': 'true'}, - follow=True + reverse("admin:registrar_domain_change", args=(primary_key,)), + {"_edit_domain": "true"}, + follow=True, ) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 804948ae9..559ea4517 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -83,9 +83,10 @@ class DomainOrgNameAddressView(DomainPermissionView, FormMixin): # Q: Is there a more efficent way to do this? # It would be ideal if we didn't have to repeat this. if self.request.user.is_staff or self.request.user.is_superuser: + changes = {field: form.cleaned_data[field] for field in form.changed_data} # if they are editing from an '/admin' redirect, log their actions self.log_analyst_form_actions( - self.form_class.__name__, self.get_object().domain_info + self.form_class.__name__, self.get_object().domain_info, changes ) # superclass has the redirect diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 86d6c681e..38d4233f9 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -49,7 +49,7 @@ class DomainPermission(PermissionsLoginMixin): # ticket 806 requested_domain = None - if(DomainInformation.objects.filter(id=pk).exists()): + if DomainInformation.objects.filter(id=pk).exists(): requested_domain = DomainInformation.objects.get(id=pk) # Analysts may manage domains, when they are in these statuses: @@ -77,17 +77,14 @@ class DomainPermission(PermissionsLoginMixin): # a status or DomainInformation... aka a status of 'None' # This checks that it has a status, before checking if it does # Otherwise, analysts can edit these domains - if (requested_domain is not None): + if requested_domain is not None: can_do_action = ( - can_do_action + can_do_action and requested_domain.domain_application.status in valid_domain_statuses ) # If the valid session keys exist, if the user is permissioned, # and if its in a valid status - if ( - can_do_action - and user_is_analyst_or_superuser - ): + if can_do_action and user_is_analyst_or_superuser: return True # ticket 796 diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index f5a8365d4..0d88d8e06 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -46,10 +46,17 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): return context - def log_analyst_form_actions(self, form_class_name, printable_object_info): + def log_analyst_form_actions( + self, form_class_name, printable_object_info, changes=None + ): """Generates a log for when key 'analyst_action' exists on the session. - Follows this format: f"{user_type} {self.request.user} - edited {form_class_name} in {printable_object_info}" + Follows this format: + + for field, new_value in changes.items(): + "{user_type} '{self.request.user}' + set field '{field}': '{new_value}' + under class '{form_class_name}' + in domain '{printable_object_info}'" """ if "analyst_action" in self.request.session: action = self.request.session["analyst_action"] @@ -67,11 +74,24 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): # Q: do we want to be logging on every changed field? # I could see that becoming spammy log-wise, # but it may also be important. - - # noqa here as breaking this up further leaves it hard to read - logger.info( - f"{user_type} {self.request.user} edited {form_class_name} in {printable_object_info}" # noqa - ) + if changes is not None: + # Logs every change made to the domain field + # noqa for readability/format + for field, new_value in changes.items(): + logger.info( + f""" + An analyst or superuser made alterations to a domain: + {user_type} '{self.request.user}' + set field '{field}': '{new_value}' + under class '{form_class_name}' + in domain '{printable_object_info}' + """ # noqa + ) + else: + # noqa here as breaking this up further leaves it hard to read + logger.info( + f"{user_type} {self.request.user} edited {form_class_name} in {printable_object_info}" # noqa + ) # Abstract property enforces NotImplementedError on an attribute. @property