From 907c0b00f29ead36d50deeed94a3da866d59aeaa Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 2 Dec 2024 09:23:17 -0700 Subject: [PATCH] Add unit test on multiple portfolio flag --- .../models/user_portfolio_permission.py | 12 +-- .../models/utility/portfolio_helper.py | 13 ++-- src/registrar/tests/test_admin.py | 75 ++++++++++++++++++- 3 files changed, 84 insertions(+), 16 deletions(-) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index a5fb0a31b..a149a9476 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -28,14 +28,14 @@ class UserPortfolioPermission(TimeStampedModel): UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION, ], - # NOTE: We currently forbid members from posessing view_members or view_all_domains. - # If those are added here, clean() will throw errors. + # NOTE: Check FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS before adding roles here. UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, ], } # Determines which roles are forbidden for certain role types to possess. + # Used to throw a ValidationError on clean() for UserPortfolioPermission and PortfolioInvitation. FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS = { UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ UserPortfolioPermissionChoices.VIEW_MEMBERS, @@ -155,7 +155,10 @@ class UserPortfolioPermission(TimeStampedModel): @classmethod def get_forbidden_permissions(cls, roles, additional_permissions): """Some permissions are forbidden for certain roles, like member. - This checks for conflicts between the role and additional_permissions.""" + This checks for conflicts between the current permission list and forbidden perms.""" + + # Get the portfolio permissions that the user currently possesses + portfolio_permissions = set(cls.get_portfolio_permissions(roles, additional_permissions)) # Get intersection of forbidden permissions across all roles. # This is because if you have roles ["admin", "member"], then they can have the @@ -169,8 +172,7 @@ class UserPortfolioPermission(TimeStampedModel): # Check if the users current permissions overlap with any forbidden permissions # by getting the intersection between current user permissions, and forbidden ones. - # This is the same as portfolio_permissions & common_forbidden_perms. - portfolio_permissions = set(cls.get_portfolio_permissions(roles, additional_permissions)) + # This is the same as portfolio_permissions & common_forbidden_perms. return portfolio_permissions.intersection(common_forbidden_perms) def clean(self): diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 9877ff0a2..e95b3097e 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -107,7 +107,7 @@ def validate_user_portfolio_permission(user_portfolio_permission): # == Validate role permissions. Compares existing permissions to forbidden ones. == # roles = user_portfolio_permission.roles if user_portfolio_permission.roles is not None else [] bad_perms = user_portfolio_permission.get_forbidden_permissions( - user_portfolio_permission.roles, user_portfolio_permission.additional_permissions + roles, user_portfolio_permission.additional_permissions ) if bad_perms: readable_perms = [ @@ -118,19 +118,18 @@ def validate_user_portfolio_permission(user_portfolio_permission): f"These permissions cannot be assigned to {', '.join(readable_roles)}: <{', '.join(readable_perms)}>" ) + # == Validate the multiple_porfolios flag. == # if not flag_is_active_for_user(user_portfolio_permission.user, "multiple_portfolios"): existing_permissions = UserPortfolioPermission.objects.exclude(id=user_portfolio_permission.id).filter( user=user_portfolio_permission.user ) - - existing_invitations = PortfolioInvitation.objects.filter(email=user_portfolio_permission.user.email) - if existing_permissions.exists(): raise ValidationError( "This user is already assigned to a portfolio. " "Based on current waffle flag settings, users cannot be assigned to multiple portfolios." ) + existing_invitations = PortfolioInvitation.objects.filter(email=user_portfolio_permission.user.email) if existing_invitations.exists(): raise ValidationError( "This user is already assigned to a portfolio invitation. " @@ -168,10 +167,9 @@ def validate_portfolio_invitation(portfolio_invitation): if has_portfolio and not portfolio_permissions: raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") + # == Validate role permissions. Compares existing permissions to forbidden ones. == # roles = portfolio_invitation.roles if portfolio_invitation.roles is not None else [] - bad_perms = UserPortfolioPermission.get_forbidden_permissions( - portfolio_invitation.roles, portfolio_invitation.additional_permissions - ) + bad_perms = UserPortfolioPermission.get_forbidden_permissions(roles, portfolio_invitation.additional_permissions) if bad_perms: readable_perms = [ UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) for perm in bad_perms @@ -181,6 +179,7 @@ def validate_portfolio_invitation(portfolio_invitation): f"These permissions cannot be assigned to {', '.join(readable_roles)}: <{', '.join(readable_perms)}>" ) + # == Validate the multiple_porfolios flag. == # user = User.objects.filter(email=portfolio_invitation.email).first() if not flag_is_active_for_user(user, "multiple_portfolios"): existing_permissions = UserPortfolioPermission.objects.filter(user=user) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index eef59e871..46fe0692e 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2,6 +2,7 @@ from datetime import datetime from django.utils import timezone from django.test import TestCase, RequestFactory, Client from django.contrib.admin.sites import AdminSite +from waffle.testutils import override_flag from django_webtest import WebTest # type: ignore from api.tests.common import less_console_noise_decorator from django.urls import reverse @@ -209,13 +210,13 @@ class TestUserPortfolioPermissionAdmin(TestCase): User.objects.all().delete() @less_console_noise_decorator - def test_validate_user_portfolio_permission(self): + def test_clean_user_portfolio_permission(self): """Tests validation of user portfolio permission""" # Test validation fails when portfolio missing but permissions are present permission = UserPortfolioPermission(user=self.superuser, roles=["organization_admin"], portfolio=None) with self.assertRaises(ValidationError) as err: - validate_user_portfolio_permission(permission) + permission.clean() self.assertEqual( str(err.exception), "When portfolio roles or additional permissions are assigned, portfolio is required.", @@ -224,7 +225,7 @@ class TestUserPortfolioPermissionAdmin(TestCase): # Test validation fails when portfolio present but no permissions are present permission = UserPortfolioPermission(user=self.superuser, roles=None, portfolio=self.portfolio) with self.assertRaises(ValidationError) as err: - validate_user_portfolio_permission(permission) + permission.clean() self.assertEqual( str(err.exception), "When portfolio is assigned, portfolio roles or additional permissions are required.", @@ -307,7 +308,73 @@ class TestPortfolioInvitationAdmin(TestCase): User.objects.all().delete() @less_console_noise_decorator - def test_portfolio_invitation_clean(self): + @override_flag("multiple_portfolios", active=False) + def test_clean_multiple_portfolios_inactive(self): + """Tests that users cannot have multiple portfolios or invitations when flag is inactive""" + # Create the first portfolio permission + UserPortfolioPermission.objects.create( + user=self.superuser, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # Test a second portfolio permission object (should fail) + second_portfolio = Portfolio.objects.create(organization_name="Second Portfolio", creator=self.superuser) + second_permission = UserPortfolioPermission( + user=self.superuser, portfolio=second_portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + with self.assertRaises(ValidationError) as err: + second_permission.clean() + self.assertIn("users cannot be assigned to multiple portfolios", str(err.exception)) + + # Test that adding a new portfolio invitation also fails + third_portfolio = Portfolio.objects.create(organization_name="Third Portfolio", creator=self.superuser) + invitation = PortfolioInvitation( + email=self.superuser.email, portfolio=third_portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + with self.assertRaises(ValidationError) as err: + invitation.clean() + self.assertIn("users cannot be assigned to multiple portfolios", str(err.exception)) + + @less_console_noise_decorator + @override_flag("multiple_portfolios", active=True) + def test_clean_multiple_portfolios_active(self): + """Tests that users can have multiple portfolios and invitations when flag is active""" + # Create first portfolio permission + UserPortfolioPermission.objects.create( + user=self.superuser, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # Second portfolio permission should succeed + second_portfolio = Portfolio.objects.create(organization_name="Second Portfolio", creator=self.superuser) + second_permission = UserPortfolioPermission( + user=self.superuser, portfolio=second_portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + second_permission.clean() + second_permission.save() + + # Verify both permissions exist + user_permissions = UserPortfolioPermission.objects.filter(user=self.superuser) + self.assertEqual(user_permissions.count(), 2) + + # Portfolio invitation should also succeed + third_portfolio = Portfolio.objects.create(organization_name="Third Portfolio", creator=self.superuser) + invitation = PortfolioInvitation( + email=self.superuser.email, portfolio=third_portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + invitation.clean() + invitation.save() + + # Verify invitation exists + self.assertTrue( + PortfolioInvitation.objects.filter( + email=self.superuser.email, + portfolio=third_portfolio, + ).exists() + ) + + @less_console_noise_decorator + def test_clean_portfolio_invitation(self): """Tests validation of portfolio invitation permissions""" # Test validation fails when portfolio missing but permissions present