From dd1b92340401a462d22dbd9df2cdf383e3bd3029 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 4 Sep 2024 13:41:26 -0400 Subject: [PATCH 01/10] wip --- src/registrar/context_processors.py | 6 ++ src/registrar/models/user.py | 56 ++++++++++--------- .../models/user_portfolio_permission.py | 6 +- .../models/utility/portfolio_helper.py | 4 +- .../templates/includes/header_extended.html | 6 +- 5 files changed, 45 insertions(+), 33 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index ea04dca80..beae164c3 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -69,6 +69,8 @@ def portfolio_permissions(request): "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission( portfolio ), + "has_view_members_portfolio_permission": request.user.has_view_members_portfolio_permission(portfolio), + "has_edit_members_portfolio_permission": request.user.has_edit_members_portfolio_permission(portfolio), "has_view_suborganization": request.user.has_view_suborganization(portfolio), "has_edit_suborganization": request.user.has_edit_suborganization(portfolio), "portfolio": portfolio, @@ -78,6 +80,8 @@ def portfolio_permissions(request): "has_base_portfolio_permission": False, "has_domains_portfolio_permission": False, "has_domain_requests_portfolio_permission": False, + "has_view_members_portfolio_permission": False, + "has_edit_members_portfolio_permission": False, "has_view_suborganization": False, "has_edit_suborganization": False, "portfolio": None, @@ -90,6 +94,8 @@ def portfolio_permissions(request): "has_base_portfolio_permission": False, "has_domains_portfolio_permission": False, "has_domain_requests_portfolio_permission": False, + "has_view_members_portfolio_permission": False, + "has_edit_members_portfolio_permission": False, "has_view_suborganization": False, "has_edit_suborganization": False, "portfolio": None, diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index a7ea1e14a..d68104566 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -64,32 +64,6 @@ class User(AbstractUser): # after they login. FIXTURE_USER = "fixture_user", "Created by fixtures" - PORTFOLIO_ROLE_PERMISSIONS = { - UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, - UserPortfolioPermissionChoices.VIEW_MEMBER, - UserPortfolioPermissionChoices.EDIT_MEMBER, - UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, - UserPortfolioPermissionChoices.EDIT_REQUESTS, - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - UserPortfolioPermissionChoices.EDIT_PORTFOLIO, - # Domain: field specific permissions - UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, - UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION, - ], - UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY: [ - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, - UserPortfolioPermissionChoices.VIEW_MEMBER, - UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - # Domain: field specific permissions - UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, - ], - UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - ], - } - # #### Constants for choice fields #### RESTRICTED = "restricted" STATUS_CHOICES = ((RESTRICTED, RESTRICTED),) @@ -224,10 +198,40 @@ class User(AbstractUser): ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) def has_domain_requests_portfolio_permission(self, portfolio): + ## BEGIN + ## Note code below is to add organization_request feature + request = HttpRequest() + request.user = self + has_organization_requests_flag = flag_is_active(request, "organization_requests") + if not has_organization_requests_flag: + return False + ## END return self._has_portfolio_permission( portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) + def has_view_members_portfolio_permission(self, portfolio): + ## BEGIN + ## Note code below is to add organization_request feature + request = HttpRequest() + request.user = self + has_organization_members_flag = flag_is_active(request, "organization_members") + if not has_organization_members_flag: + return False + ## END + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MEMBERS) + + def has_edit_members_portfolio_permission(self, portfolio): + ## BEGIN + ## Note code below is to add organization_request feature + request = HttpRequest() + request.user = self + has_organization_members_flag = flag_is_active(request, "organization_members") + if not has_organization_members_flag: + return False + ## END + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_MEMBERS) + def has_view_all_domains_permission(self, portfolio): """Determines if the current user can view all available domains in a given portfolio""" return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index bf1c3e566..0c2487df3 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -16,8 +16,8 @@ class UserPortfolioPermission(TimeStampedModel): PORTFOLIO_ROLE_PERMISSIONS = { UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, - UserPortfolioPermissionChoices.VIEW_MEMBER, - UserPortfolioPermissionChoices.EDIT_MEMBER, + UserPortfolioPermissionChoices.VIEW_MEMBERS, + UserPortfolioPermissionChoices.EDIT_MEMBERS, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, UserPortfolioPermissionChoices.EDIT_REQUESTS, UserPortfolioPermissionChoices.VIEW_PORTFOLIO, @@ -28,7 +28,7 @@ class UserPortfolioPermission(TimeStampedModel): ], UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY: [ UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, - UserPortfolioPermissionChoices.VIEW_MEMBER, + UserPortfolioPermissionChoices.VIEW_MEMBERS, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, UserPortfolioPermissionChoices.VIEW_PORTFOLIO, # Domain: field specific permissions diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 86aaa5e16..7afd32603 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -17,8 +17,8 @@ class UserPortfolioPermissionChoices(models.TextChoices): VIEW_ALL_DOMAINS = "view_all_domains", "View all domains and domain reports" VIEW_MANAGED_DOMAINS = "view_managed_domains", "View managed domains" - VIEW_MEMBER = "view_member", "View members" - EDIT_MEMBER = "edit_member", "Create and edit members" + VIEW_MEMBERS = "view_members", "View members" + EDIT_MEMBERS = "edit_members", "Create and edit members" VIEW_ALL_REQUESTS = "view_all_requests", "View all requests" VIEW_CREATED_REQUESTS = "view_created_requests", "View created requests" diff --git a/src/registrar/templates/includes/header_extended.html b/src/registrar/templates/includes/header_extended.html index 54ceb3a67..3bda2c326 100644 --- a/src/registrar/templates/includes/header_extended.html +++ b/src/registrar/templates/includes/header_extended.html @@ -23,11 +23,11 @@ Domains -
  • + {% if has_domain_requests_portfolio_permission %}
  • @@ -37,11 +37,13 @@
    • {% endif %} + {% if has_view_members_portfolio_permission %}
  • Members
    • + {% endif %}
  • {% url 'organization' as url %} From 39e6cb89330ed2a3e43c7c4ed9cdf797ef9179bd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 4 Sep 2024 11:50:44 -0600 Subject: [PATCH 02/10] Add members --- src/registrar/views/utility/__init__.py | 1 + src/registrar/views/utility/mixins.py | 17 +++++++++++++++++ src/registrar/views/utility/permission_views.py | 9 +++++++++ 3 files changed, 27 insertions(+) diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index 7219f4358..7e4e19085 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -7,5 +7,6 @@ from .permission_views import ( DomainRequestPermissionWithdrawView, DomainInvitationPermissionDeleteView, DomainRequestWizardPermissionView, + 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 6f0745f41..190d80981 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -454,3 +454,20 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): return False return super().has_permission() + + +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): + 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 0ff7d1676..734a6dbfe 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -18,6 +18,7 @@ from .mixins import ( UserDeleteDomainRolePermission, UserProfilePermission, PortfolioBasePermission, + PortfolioMembersPermission, ) import logging @@ -229,3 +230,11 @@ class PortfolioDomainRequestsPermissionView(PortfolioDomainRequestsPermission, P This abstract view cannot be instantiated. Actual views must specify `template_name`. """ + + +class PortfolioMembersPermissionView(PortfolioMembersPermission, 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`. + """ \ No newline at end of file From dbe438b6dcf4c0208a71da57e2060e85b6c2072e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 4 Sep 2024 11:52:02 -0600 Subject: [PATCH 03/10] Update context_processors.py --- src/registrar/context_processors.py | 35 +++++++++++------------------ 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index beae164c3..2ac22b2e0 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -60,6 +60,17 @@ def add_has_profile_feature_flag_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" + context = { + "has_base_portfolio_permission": False, + "has_domains_portfolio_permission": False, + "has_domain_requests_portfolio_permission": False, + "has_view_members_portfolio_permission": False, + "has_edit_members_portfolio_permission": False, + "has_view_suborganization": False, + "has_edit_suborganization": False, + "portfolio": None, + "has_organization_feature_flag": False, + } try: portfolio = request.session.get("portfolio") if portfolio: @@ -76,28 +87,8 @@ def portfolio_permissions(request): "portfolio": portfolio, "has_organization_feature_flag": True, } - return { - "has_base_portfolio_permission": False, - "has_domains_portfolio_permission": False, - "has_domain_requests_portfolio_permission": False, - "has_view_members_portfolio_permission": False, - "has_edit_members_portfolio_permission": False, - "has_view_suborganization": False, - "has_edit_suborganization": False, - "portfolio": None, - "has_organization_feature_flag": False, - } + return context except AttributeError: # Handles cases where request.user might not exist - return { - "has_base_portfolio_permission": False, - "has_domains_portfolio_permission": False, - "has_domain_requests_portfolio_permission": False, - "has_view_members_portfolio_permission": False, - "has_edit_members_portfolio_permission": False, - "has_view_suborganization": False, - "has_edit_suborganization": False, - "portfolio": None, - "has_organization_feature_flag": False, - } + return context From d802b51dc47d5abeb854dd111bb70af3e28798b2 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 4 Sep 2024 14:38:58 -0400 Subject: [PATCH 04/10] unit tests and lint --- src/registrar/models/user.py | 20 ++-- src/registrar/tests/test_views_portfolio.py | 98 +++++++++++++++++++ .../views/utility/permission_views.py | 2 +- 3 files changed, 109 insertions(+), 11 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index d68104566..4b4659094 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -6,7 +6,7 @@ from django.db.models import Q from django.http import HttpRequest from registrar.models import DomainInformation, UserDomainRole -from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices from .domain_invitation import DomainInvitation from .portfolio_invitation import PortfolioInvitation @@ -198,38 +198,38 @@ class User(AbstractUser): ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) def has_domain_requests_portfolio_permission(self, portfolio): - ## BEGIN - ## Note code below is to add organization_request feature + # BEGIN + # Note code below is to add organization_request feature request = HttpRequest() request.user = self has_organization_requests_flag = flag_is_active(request, "organization_requests") if not has_organization_requests_flag: return False - ## END + # END return self._has_portfolio_permission( portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) def has_view_members_portfolio_permission(self, portfolio): - ## BEGIN - ## Note code below is to add organization_request feature + # BEGIN + # Note code below is to add organization_request feature request = HttpRequest() request.user = self has_organization_members_flag = flag_is_active(request, "organization_members") if not has_organization_members_flag: return False - ## END + # END return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MEMBERS) def has_edit_members_portfolio_permission(self, portfolio): - ## BEGIN - ## Note code below is to add organization_request feature + # BEGIN + # Note code below is to add organization_request feature request = HttpRequest() request.user = self has_organization_members_flag = flag_is_active(request, "organization_members") if not has_organization_members_flag: return False - ## END + # END return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_MEMBERS) def has_view_all_domains_permission(self, portfolio): diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c5d1a9830..807c66cf7 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -230,6 +230,7 @@ class TestPortfolio(WebTest): self.assertContains(response, 'for="id_city"') @less_console_noise_decorator + @override_flag("organization_requests", active=True) def test_accessible_pages_when_user_does_not_have_permission(self): """Tests which pages are accessible when user does not have portfolio permissions""" self.app.set_user(self.user.username) @@ -280,6 +281,7 @@ class TestPortfolio(WebTest): self.assertEquals(domain_request_page.status_code, 403) @less_console_noise_decorator + @override_flag("organization_requests", active=True) def test_accessible_pages_when_user_does_not_have_role(self): """Test that admin / memmber roles are associated with the right access""" self.app.set_user(self.user.username) @@ -532,3 +534,99 @@ class TestPortfolio(WebTest): self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") permission.delete() + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_requests", active=False) + def test_organization_requests_waffle_flag_off_hides_nav_link_and_restricts_permission(self): + """Setting the organization_requests waffle off hides the nav link and restricts access to the requests page""" + self.app.set_user(self.user.username) + + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS, + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + ], + ) + + home = self.app.get(reverse("home")).follow() + + self.assertContains(home, "Hotel California") + self.assertNotContains(home, "Domain requests") + + domain_requests = self.app.get(reverse("domain-requests"), expect_errors=True) + self.assertEqual(domain_requests.status_code, 403) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_requests", active=True) + def test_organization_requests_waffle_flag_on_shows_nav_link_and_allows_permission(self): + """Setting the organization_requests waffle on shows the nav link and allows access to the requests page""" + self.app.set_user(self.user.username) + + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS, + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + ], + ) + + home = self.app.get(reverse("home")).follow() + + self.assertContains(home, "Hotel California") + self.assertContains(home, "Domain requests") + + domain_requests = self.app.get(reverse("domain-requests")) + self.assertEqual(domain_requests.status_code, 200) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=False) + def test_organization_members_waffle_flag_off_hides_nav_link(self): + """Setting the organization_members waffle off hides the nav link""" + self.app.set_user(self.user.username) + + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS, + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + ], + ) + + home = self.app.get(reverse("home")).follow() + + self.assertContains(home, "Hotel California") + self.assertNotContains(home, "Members") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_organization_members_waffle_flag_on_shows_nav_link(self): + """Setting the organization_members waffle on shows the nav link""" + self.app.set_user(self.user.username) + + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_MEMBERS, + ], + ) + + home = self.app.get(reverse("home")).follow() + + self.assertContains(home, "Hotel California") + self.assertContains(home, "Members") diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 734a6dbfe..e7031cf0d 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -237,4 +237,4 @@ class PortfolioMembersPermissionView(PortfolioMembersPermission, PortfolioBasePe This abstract view cannot be instantiated. Actual views must specify `template_name`. - """ \ No newline at end of file + """ From e5f2b83f6a34b200dcb9c6f6545bf620be31f6e9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 4 Sep 2024 15:34:54 -0600 Subject: [PATCH 05/10] Fix failing tests --- ...rtfolio_additional_permissions_and_more.py | 66 +++++++++++++++++++ src/registrar/tests/test_models.py | 6 +- 2 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 src/registrar/migrations/0123_alter_portfolioinvitation_portfolio_additional_permissions_and_more.py diff --git a/src/registrar/migrations/0123_alter_portfolioinvitation_portfolio_additional_permissions_and_more.py b/src/registrar/migrations/0123_alter_portfolioinvitation_portfolio_additional_permissions_and_more.py new file mode 100644 index 000000000..c14a70ab0 --- /dev/null +++ b/src/registrar/migrations/0123_alter_portfolioinvitation_portfolio_additional_permissions_and_more.py @@ -0,0 +1,66 @@ +# Generated by Django 4.2.10 on 2024-09-04 21:29 + +import django.contrib.postgres.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0122_create_groups_v16"), + ] + + operations = [ + migrations.AlterField( + model_name="portfolioinvitation", + name="portfolio_additional_permissions", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[ + ("view_all_domains", "View all domains and domain reports"), + ("view_managed_domains", "View managed domains"), + ("view_members", "View members"), + ("edit_members", "Create and edit members"), + ("view_all_requests", "View all requests"), + ("view_created_requests", "View created requests"), + ("edit_requests", "Create and edit requests"), + ("view_portfolio", "View organization"), + ("edit_portfolio", "Edit organization"), + ("view_suborganization", "View suborganization"), + ("edit_suborganization", "Edit suborganization"), + ], + max_length=50, + ), + blank=True, + help_text="Select one or more additional permissions.", + null=True, + size=None, + ), + ), + migrations.AlterField( + model_name="userportfoliopermission", + name="additional_permissions", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[ + ("view_all_domains", "View all domains and domain reports"), + ("view_managed_domains", "View managed domains"), + ("view_members", "View members"), + ("edit_members", "Create and edit members"), + ("view_all_requests", "View all requests"), + ("view_created_requests", "View created requests"), + ("edit_requests", "Create and edit requests"), + ("view_portfolio", "View organization"), + ("edit_portfolio", "Edit organization"), + ("view_suborganization", "View suborganization"), + ("edit_suborganization", "Edit suborganization"), + ], + max_length=50, + ), + blank=True, + help_text="Select one or more additional permissions.", + null=True, + size=None, + ), + ), + ] diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a1b21ee2..d0a3e0462 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1474,6 +1474,7 @@ class TestUser(TestCase): self.assertFalse(self.user.has_contact_info()) @less_console_noise_decorator + @override_flag("organization_requests", active=True) def test_has_portfolio_permission(self): """ 0. Returns False when user does not have a permission @@ -1495,7 +1496,10 @@ class TestUser(TestCase): portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( portfolio=portfolio, user=self.user, - additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS + ], ) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) From 49110461ef542f3a230f28cb3e11f32597f73cdc Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 5 Sep 2024 12:10:49 -0400 Subject: [PATCH 06/10] lint --- src/registrar/tests/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index d0a3e0462..827f349b9 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1498,7 +1498,7 @@ class TestUser(TestCase): user=self.user, additional_permissions=[ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, ], ) From 467536600efe075c20d7c16288b74eb15b5ec970 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 6 Sep 2024 08:56:27 -0400 Subject: [PATCH 07/10] limit domains_json view to domains based on user's portfolio permissions --- .../tests/test_views_domains_json.py | 106 +++++++++++++++++- src/registrar/views/domains_json.py | 14 ++- 2 files changed, 113 insertions(+), 7 deletions(-) diff --git a/src/registrar/tests/test_views_domains_json.py b/src/registrar/tests/test_views_domains_json.py index 70ae23b43..bf8fe4c0a 100644 --- a/src/registrar/tests/test_views_domains_json.py +++ b/src/registrar/tests/test_views_domains_json.py @@ -1,9 +1,13 @@ from registrar.models import UserDomainRole, Domain, DomainInformation, Portfolio from django.urls import reverse + +from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .test_views import TestWithUser from django_webtest import WebTest # type: ignore from django.utils.dateparse import parse_date from api.tests.common import less_console_noise_decorator +from waffle.testutils import override_flag class GetDomainsJsonTest(TestWithUser, WebTest): @@ -31,6 +35,7 @@ class GetDomainsJsonTest(TestWithUser, WebTest): def tearDown(self): UserDomainRole.objects.all().delete() + UserPortfolioPermission.objects.all().delete() DomainInformation.objects.all().delete() Portfolio.objects.all().delete() super().tearDown() @@ -115,8 +120,105 @@ class GetDomainsJsonTest(TestWithUser, WebTest): self.assertEqual(svg_icon_expected, svg_icons[i]) @less_console_noise_decorator - def test_get_domains_json_with_portfolio(self): - """Test that an authenticated user gets the list of 2 domains for portfolio.""" + @override_flag("organization_feature", active=True) + def test_get_domains_json_with_portfolio_view_managed_domains(self): + """Test that an authenticated user gets the list of 1 domain for portfolio. The 1 domain + is the domain that they manage within the portfolio.""" + + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS], + ) + + response = self.app.get(reverse("get_domains_json"), {"portfolio": self.portfolio.id}) + self.assertEqual(response.status_code, 200) + data = response.json + + # Check pagination info + self.assertEqual(data["page"], 1) + self.assertFalse(data["has_next"]) + self.assertFalse(data["has_previous"]) + self.assertEqual(data["num_pages"], 1) + + # Check the number of domains + self.assertEqual(len(data["domains"]), 1) + + # Expected domains + expected_domains = [self.domain3] + + # Extract fields from response + domain_ids = [domain["id"] for domain in data["domains"]] + names = [domain["name"] for domain in data["domains"]] + expiration_dates = [domain["expiration_date"] for domain in data["domains"]] + states = [domain["state"] for domain in data["domains"]] + state_displays = [domain["state_display"] for domain in data["domains"]] + get_state_help_texts = [domain["get_state_help_text"] for domain in data["domains"]] + action_urls = [domain["action_url"] for domain in data["domains"]] + action_labels = [domain["action_label"] for domain in data["domains"]] + svg_icons = [domain["svg_icon"] for domain in data["domains"]] + + # Check fields for each domain + for i, expected_domain in enumerate(expected_domains): + self.assertEqual(expected_domain.id, domain_ids[i]) + self.assertEqual(expected_domain.name, names[i]) + self.assertEqual(expected_domain.expiration_date, expiration_dates[i]) + self.assertEqual(expected_domain.state, states[i]) + + # Parsing the expiration date from string to date + parsed_expiration_date = parse_date(expiration_dates[i]) + expected_domain.expiration_date = parsed_expiration_date + + # Check state_display and get_state_help_text + self.assertEqual(expected_domain.state_display(), state_displays[i]) + self.assertEqual(expected_domain.get_state_help_text(), get_state_help_texts[i]) + + self.assertEqual(reverse("domain", kwargs={"pk": expected_domain.id}), action_urls[i]) + + # Check action_label + user_domain_role_exists = UserDomainRole.objects.filter( + domain_id=expected_domains[i].id, user=self.user + ).exists() + action_label_expected = ( + "View" + if not user_domain_role_exists + or expected_domains[i].state + in [ + Domain.State.DELETED, + Domain.State.ON_HOLD, + ] + else "Manage" + ) + self.assertEqual(action_label_expected, action_labels[i]) + + # Check svg_icon + svg_icon_expected = ( + "visibility" + if not user_domain_role_exists + or expected_domains[i].state + in [ + Domain.State.DELETED, + Domain.State.ON_HOLD, + ] + else "settings" + ) + self.assertEqual(svg_icon_expected, svg_icons[i]) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_get_domains_json_with_portfolio_view_all_domains(self): + """Test that an authenticated user gets the list of 2 domains for portfolio. One is a domain which + they manage within the portfolio. The other is a domain which they don't manage within the + portfolio.""" + + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], + ) + # self.app.set_user(self.user.username) response = self.app.get(reverse("get_domains_json"), {"portfolio": self.portfolio.id}) self.assertEqual(response.status_code, 200) diff --git a/src/registrar/views/domains_json.py b/src/registrar/views/domains_json.py index 06c211227..aaecd33b7 100644 --- a/src/registrar/views/domains_json.py +++ b/src/registrar/views/domains_json.py @@ -50,11 +50,15 @@ def get_domain_ids_from_request(request): """ portfolio = request.GET.get("portfolio") if portfolio: - domain_infos = DomainInformation.objects.filter(portfolio=portfolio) - return domain_infos.values_list("domain_id", flat=True) - else: - user_domain_roles = UserDomainRole.objects.filter(user=request.user) - return user_domain_roles.values_list("domain_id", flat=True) + if request.user.is_org_user(request) and request.user.has_view_all_domains_permission(portfolio): + domain_infos = DomainInformation.objects.filter(portfolio=portfolio) + return domain_infos.values_list("domain_id", flat=True) + else: + domain_info_ids = DomainInformation.objects.filter(portfolio=portfolio).values_list("domain_id", flat=True) + user_domain_roles = UserDomainRole.objects.filter(user=request.user).values_list("domain_id", flat=True) + return domain_info_ids.intersection(user_domain_roles) + user_domain_roles = UserDomainRole.objects.filter(user=request.user) + return user_domain_roles.values_list("domain_id", flat=True) def apply_search(queryset, request): From 008b9d57a596f2f5e74dba743b69f6abb90fbac7 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 6 Sep 2024 08:58:28 -0400 Subject: [PATCH 08/10] cleanup --- src/registrar/tests/test_views_domains_json.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/tests/test_views_domains_json.py b/src/registrar/tests/test_views_domains_json.py index bf8fe4c0a..07799104b 100644 --- a/src/registrar/tests/test_views_domains_json.py +++ b/src/registrar/tests/test_views_domains_json.py @@ -218,7 +218,6 @@ class GetDomainsJsonTest(TestWithUser, WebTest): roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], ) - # self.app.set_user(self.user.username) response = self.app.get(reverse("get_domains_json"), {"portfolio": self.portfolio.id}) self.assertEqual(response.status_code, 200) From ffaf3b5a375364fc6e78977bc8600d5b00747ede Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 9 Sep 2024 10:00:34 -0500 Subject: [PATCH 09/10] fix linter errors --- src/registrar/management/commands/update_first_ready.py | 2 +- src/registrar/management/commands/utility/terminal_helper.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index f1ebdd555..22320a3d3 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -27,7 +27,7 @@ class Command(BaseCommand, PopulateScriptTemplate): # check if a transition domain object for this domain name exists, # or if so whether its first_ready value matches its created_at date - def custom_filter(self, records: BaseManager[Domain]) -> BaseManager[Domain]: + def custom_filter(self, records): to_include_pks = [] for record in records: if ( diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index e69d54c07..fa7cde683 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -78,7 +78,7 @@ class PopulateScriptTemplate(ABC): run_summary_header = None @abstractmethod - def update_record(self, record: Model): + def update_record(self, record): """Defines how we update each field. raises: @@ -86,7 +86,7 @@ class PopulateScriptTemplate(ABC): """ raise NotImplementedError - def mass_update_records(self, object_class: Model, filter_conditions, fields_to_update, debug=True, verbose=False): + def mass_update_records(self, object_class, filter_conditions, fields_to_update, debug=True, verbose=False): """Loops through each valid "object_class" object - specified by filter_conditions - and updates fields defined by fields_to_update using update_record. From 872f943220e43274a02cd202801de5b0bb5c825e Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 9 Sep 2024 10:10:04 -0500 Subject: [PATCH 10/10] unused import --- src/registrar/management/commands/update_first_ready.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index 22320a3d3..0a4ea10a7 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -1,6 +1,5 @@ import logging from django.core.management import BaseCommand -from django.db.models.manager import BaseManager from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import Domain, TransitionDomain