Merge pull request #3111 from cisagov/za/3103-prevent-errors-member-management

#3103: Prevent errors in django admin Member Management - [MEOWARD]

src/registrar/fixtures/fixtures_user_portfolio_permissions.py

@@ -16,7 +16,7 @@ logger = logging.getLogger(__name__)
 
 class UserPortfolioPermissionFixture:
     """Create user portfolio permissions for each user.
-    Each user will be admin on 2 portfolios.
+    Each user will be admin on only one portfolio.
 
     Depends on fixture_portfolios"""
 

src/registrar/models/__init__.py

@@ -10,18 +10,21 @@ from .host import Host
 from .domain_invitation import DomainInvitation
 from .user_domain_role import UserDomainRole
 from .public_contact import PublicContact
+
+# IMPORTANT: UserPortfolioPermission must be before PortfolioInvitation.
+# PortfolioInvitation imports from UserPortfolioPermission, so you will get a circular import otherwise.
+from .user_portfolio_permission import UserPortfolioPermission
+from .portfolio_invitation import PortfolioInvitation
 from .user import User
 from .user_group import UserGroup
 from .website import Website
 from .transition_domain import TransitionDomain
 from .verified_by_staff import VerifiedByStaff
 from .waffle_flag import WaffleFlag
-from .portfolio_invitation import PortfolioInvitation
 from .portfolio import Portfolio
 from .domain_group import DomainGroup
 from .suborganization import Suborganization
 from .senior_official import SeniorOfficial
-from .user_portfolio_permission import UserPortfolioPermission
 from .allowed_email import AllowedEmail
 
 

src/registrar/models/portfolio_invitation.py

@@ -1,16 +1,18 @@
 """People are invited by email to administer domains."""
 
 import logging
-from django.contrib.auth import get_user_model
 from django.db import models
 from django_fsm import FSMField, transition
-from registrar.models.domain_invitation import DomainInvitation
-from registrar.models.user_portfolio_permission import UserPortfolioPermission
-from .utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices  # type: ignore
+from django.contrib.auth import get_user_model
+from registrar.models import DomainInvitation, UserPortfolioPermission
+from .utility.portfolio_helper import (
+    UserPortfolioPermissionChoices,
+    UserPortfolioRoleChoices,
+    validate_portfolio_invitation,
+)  # type: ignore
 from .utility.time_stamped_model import TimeStampedModel
 from django.contrib.postgres.fields import ArrayField
 
-
 logger = logging.getLogger(__name__)
 
 
@@ -108,3 +110,8 @@ class PortfolioInvitation(TimeStampedModel):
         if self.additional_permissions and len(self.additional_permissions) > 0:
             user_portfolio_permission.additional_permissions = self.additional_permissions
         user_portfolio_permission.save()
+
+    def clean(self):
+        """Extends clean method to perform additional validation, which can raise errors in django admin."""
+        super().clean()
+        validate_portfolio_invitation(self)

src/registrar/models/user.py

@@ -1,15 +1,13 @@
 import logging
 
-from django.apps import apps
 from django.contrib.auth.models import AbstractUser
 from django.db import models
 from django.db.models import Q
 
-from registrar.models import DomainInformation, UserDomainRole
+from registrar.models import DomainInformation, UserDomainRole, PortfolioInvitation, UserPortfolioPermission
 from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices
 
 from .domain_invitation import DomainInvitation
-from .portfolio_invitation import PortfolioInvitation
 from .transition_domain import TransitionDomain
 from .verified_by_staff import VerifiedByStaff
 from .domain import Domain
@@ -501,8 +499,6 @@ class User(AbstractUser):
     def is_only_admin_of_portfolio(self, portfolio):
         """Check if the user is the only admin of the given portfolio."""
 
-        UserPortfolioPermission = apps.get_model("registrar", "UserPortfolioPermission")
-
         admin_permission = UserPortfolioRoleChoices.ORGANIZATION_ADMIN
 
         admins = UserPortfolioPermission.objects.filter(portfolio=portfolio, roles__contains=[admin_permission])

src/registrar/models/user_portfolio_permission.py

@@ -1,12 +1,11 @@
 from django.db import models
-from django.forms import ValidationError
 from registrar.models.user_domain_role import UserDomainRole
-from registrar.utility.waffle import flag_is_active_for_user
 from registrar.models.utility.portfolio_helper import (
     UserPortfolioPermissionChoices,
     UserPortfolioRoleChoices,
     DomainRequestPermissionDisplay,
     MemberPermissionDisplay,
+    validate_user_portfolio_permission,
 )
 from .utility.time_stamped_model import TimeStampedModel
 from django.contrib.postgres.fields import ArrayField
@@ -22,18 +21,29 @@ class UserPortfolioPermission(TimeStampedModel):
         UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [
             UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS,
             UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS,
-            UserPortfolioPermissionChoices.EDIT_REQUESTS,
+            UserPortfolioPermissionChoices.VIEW_MEMBERS,
             UserPortfolioPermissionChoices.VIEW_PORTFOLIO,
             UserPortfolioPermissionChoices.EDIT_PORTFOLIO,
             # Domain: field specific permissions
             UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION,
             UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION,
         ],
+        # 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,
+            UserPortfolioPermissionChoices.EDIT_MEMBERS,
+            UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS,
+        ],
+    }
+
     user = models.ForeignKey(
         "registrar.User",
         null=False,
@@ -142,30 +152,30 @@ class UserPortfolioPermission(TimeStampedModel):
         else:
             return MemberPermissionDisplay.NONE
 
+    @classmethod
+    def get_forbidden_permissions(cls, roles, additional_permissions):
+        """Some permissions are forbidden for certain roles, like member.
+        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
+        # so called "forbidden" ones. But just member on their own cannot.
+        # The solution to this is to only grab what is only COMMONLY "forbidden".
+        # This will scale if we add more roles in the future.
+        # This is thes same as applying the `&` operator across all sets for each role.
+        common_forbidden_perms = set.intersection(
+            *[set(cls.FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS.get(role, [])) for role in roles]
+        )
+
+        # 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.
+        return portfolio_permissions.intersection(common_forbidden_perms)
+
     def clean(self):
         """Extends clean method to perform additional validation, which can raise errors in django admin."""
         super().clean()
-
-        # Check if portfolio is set without accessing the related object.
-        has_portfolio = bool(self.portfolio_id)
-        if not has_portfolio and self._get_portfolio_permissions():
-            raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.")
-
-        if has_portfolio and not self._get_portfolio_permissions():
-            raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.")
-
-        # Check if a user is set without accessing the related object.
-        has_user = bool(self.user_id)
-        if has_user:
-            existing_permission_pks = UserPortfolioPermission.objects.filter(user=self.user).values_list(
-                "pk", flat=True
-            )
-            if (
-                not flag_is_active_for_user(self.user, "multiple_portfolios")
-                and existing_permission_pks.exists()
-                and self.pk not in existing_permission_pks
-            ):
-                raise ValidationError(
-                    "This user is already assigned to a portfolio. "
-                    "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
-                )
+        validate_user_portfolio_permission(self)

src/registrar/models/utility/portfolio_helper.py

@@ -1,5 +1,9 @@
 from registrar.utility import StrEnum
 from django.db import models
+from django.apps import apps
+from django.forms import ValidationError
+from registrar.utility.waffle import flag_is_active_for_user
+from django.contrib.auth import get_user_model
 
 
 class UserPortfolioRoleChoices(models.TextChoices):
@@ -69,3 +73,131 @@ class MemberPermissionDisplay(StrEnum):
     MANAGER = "Manager"
     VIEWER = "Viewer"
     NONE = "None"
+
+
+def validate_user_portfolio_permission(user_portfolio_permission):
+    """
+    Validates a UserPortfolioPermission instance. Located in portfolio_helper to avoid circular imports
+    between PortfolioInvitation and UserPortfolioPermission models.
+
+    Used in UserPortfolioPermission.clean() for model validation.
+
+    Validates:
+    1. A portfolio must be assigned if roles or additional permissions are specified, and vice versa.
+    2. Assigned roles do not include any forbidden permissions.
+    3. If the 'multiple_portfolios' flag is inactive for the user,
+    they must not have existing portfolio permissions or invitations.
+
+    Raises:
+        ValidationError: If any of the validation rules are violated.
+    """
+    PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation")
+    UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
+
+    has_portfolio = bool(user_portfolio_permission.portfolio_id)
+    portfolio_permissions = set(user_portfolio_permission._get_portfolio_permissions())
+
+    # == Validate required fields == #
+    if not has_portfolio and portfolio_permissions:
+        raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.")
+
+    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 = user_portfolio_permission.roles if user_portfolio_permission.roles is not None else []
+    bad_perms = user_portfolio_permission.get_forbidden_permissions(
+        roles, user_portfolio_permission.additional_permissions
+    )
+    if bad_perms:
+        readable_perms = [
+            UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) for perm in bad_perms
+        ]
+        readable_roles = [UserPortfolioRoleChoices.get_user_portfolio_role_label(role) for role in roles]
+        raise ValidationError(
+            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
+        )
+        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. "
+                "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
+            )
+
+
+def validate_portfolio_invitation(portfolio_invitation):
+    """
+    Validates a PortfolioInvitation instance. Located in portfolio_helper to avoid circular imports
+    between PortfolioInvitation and UserPortfolioPermission models.
+
+    Used in PortfolioInvitation.clean() for model validation.
+
+    Validates:
+    1. A portfolio must be assigned if roles or additional permissions are specified, and vice versa.
+    2. Assigned roles do not include any forbidden permissions.
+    3. If the 'multiple_portfolios' flag is inactive for the user,
+    they must not have existing portfolio permissions or invitations.
+
+    Raises:
+        ValidationError: If any of the validation rules are violated.
+    """
+    PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation")
+    UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
+    User = get_user_model()
+
+    has_portfolio = bool(portfolio_invitation.portfolio_id)
+    portfolio_permissions = set(portfolio_invitation.get_portfolio_permissions())
+
+    # == Validate required fields == #
+    if not has_portfolio and portfolio_permissions:
+        raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.")
+
+    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(roles, portfolio_invitation.additional_permissions)
+    if bad_perms:
+        readable_perms = [
+            UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) for perm in bad_perms
+        ]
+        readable_roles = [UserPortfolioRoleChoices.get_user_portfolio_role_label(role) for role in roles]
+        raise ValidationError(
+            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 user returns None, then we check for global assignment of multiple_portfolios.
+    # Otherwise we just check on the user.
+    if not flag_is_active_for_user(user, "multiple_portfolios"):
+        existing_permissions = UserPortfolioPermission.objects.filter(user=user)
+
+        existing_invitations = PortfolioInvitation.objects.exclude(id=portfolio_invitation.id).filter(
+            email=portfolio_invitation.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."
+            )
+
+        if existing_invitations.exists():
+            raise ValidationError(
+                "This user is already assigned to a portfolio invitation. "
+                "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
+            )

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
@@ -25,6 +26,7 @@ from registrar.admin import (
     TransitionDomainAdmin,
     UserGroupAdmin,
     PortfolioAdmin,
+    UserPortfolioPermissionAdmin,
 )
 from registrar.models import (
     Domain,
@@ -65,6 +67,7 @@ from django.contrib.sessions.backends.db import SessionStore
 from django.contrib.auth import get_user_model
 
 from unittest.mock import ANY, patch, Mock
+from django.forms import ValidationError
 
 
 import logging
@@ -187,6 +190,93 @@ class TestDomainInvitationAdmin(TestCase):
             self.assertContains(response, retrieved_html, count=1)
 
 
+class TestUserPortfolioPermissionAdmin(TestCase):
+    """Tests for the PortfolioInivtationAdmin class"""
+
+    def setUp(self):
+        """Create a client object"""
+        self.factory = RequestFactory()
+        self.admin = ListHeaderAdmin(model=UserPortfolioPermissionAdmin, admin_site=AdminSite())
+        self.client = Client(HTTP_HOST="localhost:8080")
+        self.superuser = create_superuser()
+        self.portfolio = Portfolio.objects.create(organization_name="Test Portfolio", creator=self.superuser)
+
+    def tearDown(self):
+        """Delete all DomainInvitation objects"""
+        Portfolio.objects.all().delete()
+        PortfolioInvitation.objects.all().delete()
+        Contact.objects.all().delete()
+        User.objects.all().delete()
+
+    @less_console_noise_decorator
+    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:
+            permission.clean()
+            self.assertEqual(
+                str(err.exception),
+                "When portfolio roles or additional permissions are assigned, portfolio is required.",
+            )
+
+        # 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:
+            permission.clean()
+            self.assertEqual(
+                str(err.exception),
+                "When portfolio is assigned, portfolio roles or additional permissions are required.",
+            )
+
+        # Test validation fails with forbidden permissions for single role
+        forbidden_member_roles = UserPortfolioPermission.FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS.get(
+            UserPortfolioRoleChoices.ORGANIZATION_MEMBER
+        )
+        permission = UserPortfolioPermission(
+            user=self.superuser,
+            roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER],
+            additional_permissions=forbidden_member_roles,
+            portfolio=self.portfolio,
+        )
+        with self.assertRaises(ValidationError) as err:
+            permission.clean()
+            self.assertEqual(
+                str(err.exception),
+                "These permissions cannot be assigned to Member: "
+                "<Create and edit members, View all domains and domain reports, View members>",
+            )
+
+    @less_console_noise_decorator
+    def test_get_forbidden_permissions_with_multiple_roles(self):
+        """Tests that forbidden permissions are properly handled when a user has multiple roles"""
+        # Get forbidden permissions for member role
+        member_forbidden = UserPortfolioPermission.FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS.get(
+            UserPortfolioRoleChoices.ORGANIZATION_MEMBER
+        )
+
+        # Test with both admin and member roles
+        roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN, UserPortfolioRoleChoices.ORGANIZATION_MEMBER]
+
+        # These permissions would be forbidden for member alone, but should be allowed
+        # when combined with admin role
+        permissions = UserPortfolioPermission.get_forbidden_permissions(
+            roles=roles, additional_permissions=member_forbidden
+        )
+
+        # Should return empty set since no permissions are commonly forbidden between admin and member
+        self.assertEqual(permissions, set())
+
+        # Verify the same permissions are forbidden when only member role is present
+        member_only_permissions = UserPortfolioPermission.get_forbidden_permissions(
+            roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], additional_permissions=member_forbidden
+        )
+
+        # Should return the forbidden permissions for member role
+        self.assertEqual(member_only_permissions, set(member_forbidden))
+
+
 class TestPortfolioInvitationAdmin(TestCase):
     """Tests for the PortfolioInvitationAdmin class as super user
 
@@ -204,9 +294,11 @@ class TestPortfolioInvitationAdmin(TestCase):
     def setUp(self):
         """Create a client object"""
         self.client = Client(HTTP_HOST="localhost:8080")
+        self.portfolio = Portfolio.objects.create(organization_name="Test Portfolio", creator=self.superuser)
 
     def tearDown(self):
         """Delete all DomainInvitation objects"""
+        Portfolio.objects.all().delete()
         PortfolioInvitation.objects.all().delete()
         Contact.objects.all().delete()
 
@@ -214,6 +306,112 @@ class TestPortfolioInvitationAdmin(TestCase):
     def tearDownClass(self):
         User.objects.all().delete()
 
+    @less_console_noise_decorator
+    @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
+        invitation = PortfolioInvitation(email="test@example.com", roles=["organization_admin"], portfolio=None)
+        with self.assertRaises(ValidationError) as err:
+            invitation.clean()
+            self.assertEqual(
+                str(err.exception),
+                "When portfolio roles or additional permissions are assigned, portfolio is required.",
+            )
+
+        # Test validation fails when portfolio present but no permissions
+        invitation = PortfolioInvitation(email="test@example.com", roles=None, portfolio=self.portfolio)
+        with self.assertRaises(ValidationError) as err:
+            invitation.clean()
+            self.assertEqual(
+                str(err.exception),
+                "When portfolio is assigned, portfolio roles or additional permissions are required.",
+            )
+
+        # Test validation fails with forbidden permissions
+        forbidden_member_roles = UserPortfolioPermission.FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS.get(
+            UserPortfolioRoleChoices.ORGANIZATION_MEMBER
+        )
+        invitation = PortfolioInvitation(
+            email="test@example.com",
+            roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER],
+            additional_permissions=forbidden_member_roles,
+            portfolio=self.portfolio,
+        )
+        with self.assertRaises(ValidationError) as err:
+            invitation.clean()
+            self.assertEqual(
+                str(err.exception),
+                "These permissions cannot be assigned to Member: "
+                "<View all domains and domain reports, Create and edit members, View members>",
+            )
+
     @less_console_noise_decorator
     def test_has_model_description(self):
         """Tests if this model has a model description on the table view"""

src/registrar/tests/test_reports.py

@@ -885,13 +885,13 @@ class MemberExportTest(MockDbForIndividualTests, MockEppLib):
             "big_lebowski@dude.co,False,help@get.gov,2022-04-01,Invalid date,None,Viewer,True,1,cdomain1.gov\n"
             "tired_sleepy@igorville.gov,False,System,2022-04-01,Invalid date,Viewer,None,False,0,\n"
             "icy_superuser@igorville.gov,True,help@get.gov,2022-04-01,2024-02-01,Viewer Requester,Manager,False,0,\n"
-            "cozy_staffuser@igorville.gov,True,help@get.gov,2022-04-01,2024-02-01,Viewer Requester,None,False,0,\n"
+            "cozy_staffuser@igorville.gov,True,help@get.gov,2022-04-01,2024-02-01,Viewer,Viewer,False,0,\n"
             "nonexistentmember_1@igorville.gov,False,help@get.gov,Unretrieved,Invited,None,Manager,False,0,\n"
             "nonexistentmember_2@igorville.gov,False,help@get.gov,Unretrieved,Invited,None,Viewer,False,0,\n"
             "nonexistentmember_3@igorville.gov,False,help@get.gov,Unretrieved,Invited,Viewer,None,False,0,\n"
             "nonexistentmember_4@igorville.gov,True,help@get.gov,Unretrieved,"
             "Invited,Viewer Requester,Manager,False,0,\n"
-            "nonexistentmember_5@igorville.gov,True,help@get.gov,Unretrieved,Invited,Viewer Requester,None,False,0,\n"
+            "nonexistentmember_5@igorville.gov,True,help@get.gov,Unretrieved,Invited,Viewer,Viewer,False,0,\n"
         )
         # Normalize line endings and remove commas,
         # spaces and leading/trailing whitespace

src/registrar/tests/test_views_portfolio.py

@@ -677,18 +677,15 @@ class TestPortfolio(WebTest):
     @override_flag("organization_feature", active=True)
     @override_flag("organization_members", active=True)
     def test_cannot_view_members_table(self):
-        """Test that user without proper permission is denied access to members view"""
+        """Test that user without proper permission is denied access to members view."""
 
         # Users can only view the members table if they have
         # Portfolio Permission "view_members" selected.
-        # NOTE: Admins, by default, do NOT have permission
-        # to view/edit members.  This must be enabled explicitly
-        # in the "additional permissions" section for a portfolio
-        # permission.
-        #
+        # NOTE: Admins, by default, DO have permission
+        # to view/edit members.
         # Scenarios to test include;
         # (1) - User is not admin and can view portfolio, but not the members table
-        # (1) - User is admin and can view portfolio, but not the members table
+        # (1) - User is admin and can view portfolio, as well as the members table
 
         # --- non-admin
         self.app.set_user(self.user.username)
@@ -713,11 +710,9 @@ class TestPortfolio(WebTest):
             roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
         )
 
-        # Verify that the user cannot access the members page
-        # This will redirect the user to the members page.
+        # Admins should have access to this page by default
         response = self.client.get(reverse("members"), follow=True)
-        # Assert the response is a 403 Forbidden
-        self.assertEqual(response.status_code, 403)
+        self.assertEqual(response.status_code, 200)
 
     @less_console_noise_decorator
     @override_flag("organization_feature", active=True)
@@ -940,6 +935,7 @@ class TestPortfolio(WebTest):
             portfolio=self.portfolio,
             roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
             additional_permissions=[
+                UserPortfolioPermissionChoices.EDIT_REQUESTS,
                 UserPortfolioPermissionChoices.EDIT_MEMBERS,
             ],
         )
@@ -1052,6 +1048,7 @@ class TestPortfolio(WebTest):
             portfolio=self.portfolio,
             roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
             additional_permissions=[
+                UserPortfolioPermissionChoices.EDIT_REQUESTS,
                 UserPortfolioPermissionChoices.EDIT_MEMBERS,
             ],
         )
@@ -1060,6 +1057,7 @@ class TestPortfolio(WebTest):
             portfolio=self.portfolio,
             roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
             additional_permissions=[
+                UserPortfolioPermissionChoices.EDIT_REQUESTS,
                 UserPortfolioPermissionChoices.EDIT_MEMBERS,
             ],
         )
@@ -1137,7 +1135,10 @@ class TestPortfolio(WebTest):
         """Test the nav contains a dropdown with a link to create and another link to view requests
         Also test for the existence of the Create a new request btn on the requests page"""
         UserPortfolioPermission.objects.get_or_create(
-            user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+            user=self.user,
+            portfolio=self.portfolio,
+            roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
+            additional_permissions=[UserPortfolioPermissionChoices.EDIT_REQUESTS],
         )
         self.client.force_login(self.user)
         # create and submit a domain request
@@ -2124,7 +2125,10 @@ class TestRequestingEntity(WebTest):
             portfolio=self.portfolio_2,
         )
         self.portfolio_role = UserPortfolioPermission.objects.create(
-            portfolio=self.portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+            portfolio=self.portfolio,
+            user=self.user,
+            roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
+            additional_permissions=[UserPortfolioPermissionChoices.EDIT_REQUESTS],
         )
         # Login the current user
         self.app.set_user(self.user.username)

src/registrar/tests/test_views_request.py

@@ -26,7 +26,7 @@ from registrar.views.domain_request import DomainRequestWizard, Step
 
 from .common import less_console_noise
 from .test_views import TestWithUser
-from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices
+from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices, UserPortfolioPermissionChoices
 import logging
 
 logger = logging.getLogger(__name__)
@@ -47,10 +47,12 @@ class DomainRequestTests(TestWithUser, WebTest):
 
     def tearDown(self):
         super().tearDown()
-        DomainRequest.objects.all().delete()
+        Domain.objects.all().delete()
         DomainInformation.objects.all().delete()
+        DomainRequest.objects.all().delete()
+        UserPortfolioPermission.objects.all().delete()
+        Portfolio.objects.all().delete()
         User.objects.all().delete()
-        self.federal_agency.delete()
 
     @less_console_noise_decorator
     def test_domain_request_form_intro_acknowledgement(self):
@@ -2753,7 +2755,10 @@ class DomainRequestTests(TestWithUser, WebTest):
         """Tests that a portfolio user with edit request permissions can edit and add new requests"""
         portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Test Portfolio")
         portfolio_perm, _ = UserPortfolioPermission.objects.get_or_create(
-            user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+            user=self.user,
+            portfolio=portfolio,
+            roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
+            additional_permissions=[UserPortfolioPermissionChoices.EDIT_REQUESTS],
         )
 
         # This user should be allowed to create new domain requests
@@ -2765,11 +2770,6 @@ class DomainRequestTests(TestWithUser, WebTest):
         edit_page = self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})).follow()
         self.assertEqual(edit_page.status_code, 200)
 
-        # Cleanup
-        DomainRequest.objects.all().delete()
-        portfolio_perm.delete()
-        portfolio.delete()
-
     def test_non_creator_access(self):
         """Tests that a user cannot edit a domain request they didn't create"""
         p = "password"
@@ -2863,7 +2863,10 @@ class DomainRequestTestDifferentStatuses(TestWithUser, WebTest):
         """Tests that the withdraw button on portfolio redirects to the portfolio domain requests page"""
         portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Test Portfolio")
         UserPortfolioPermission.objects.get_or_create(
-            user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+            user=self.user,
+            portfolio=portfolio,
+            roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
+            additional_permissions=[UserPortfolioPermissionChoices.EDIT_REQUESTS],
         )
         domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.SUBMITTED, user=self.user)
         domain_request.save()
@@ -3007,6 +3010,7 @@ class TestDomainRequestWizard(TestWithUser, WebTest):
                 user=self.user,
                 portfolio=portfolio,
                 roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
+                additional_permissions=[UserPortfolioPermissionChoices.EDIT_REQUESTS],
             )
 
             # Check portfolio-specific breadcrumb
@@ -3165,6 +3169,9 @@ class TestDomainRequestWizard(TestWithUser, WebTest):
             user=self.user,
             portfolio=portfolio,
             roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
+            additional_permissions=[
+                UserPortfolioPermissionChoices.EDIT_REQUESTS,
+            ],
         )
 
         response = self.app.get(f"/domain-request/{domain_request.id}/edit/")