forbidden perm check + cleanup circular imports

src/registrar/migrations/0139_alter_portfolioinvitation_portfolio_and_more.py

@@ -1,27 +0,0 @@
-# Generated by Django 4.2.10 on 2024-11-21 20:18
-
-from django.db import migrations, models
-import django.db.models.deletion
-
-
-class Migration(migrations.Migration):
-
-    dependencies = [
-        ("registrar", "0138_alter_domaininvitation_status"),
-    ]
-
-    operations = [
-        migrations.AlterField(
-            model_name="portfolioinvitation",
-            name="portfolio",
-            field=models.ForeignKey(
-                on_delete=django.db.models.deletion.CASCADE,
-                related_name="portfolio_invitations",
-                to="registrar.portfolio",
-            ),
-        ),
-        migrations.AlterUniqueTogether(
-            name="portfolioinvitation",
-            unique_together={("email", "portfolio")},
-        ),
-    ]

src/registrar/models/__init__.py

@@ -16,12 +16,14 @@ from .website import Website
 from .transition_domain import TransitionDomain
 from .verified_by_staff import VerifiedByStaff
 from .waffle_flag import WaffleFlag
+# 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 .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,11 +1,9 @@
 """People are invited by email to administer domains."""
 
 import logging
-from django.apps import apps
-from django.contrib.auth import get_user_model
 from django.db import models
 from django_fsm import FSMField, transition
-from registrar.models import DomainInvitation
+from registrar.models import DomainInvitation, UserPortfolioPermission, User
 from registrar.utility.waffle import flag_is_active_for_user
 from .utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices  # type: ignore
 from .utility.time_stamped_model import TimeStampedModel
@@ -24,9 +22,6 @@ class PortfolioInvitation(TimeStampedModel):
             models.Index(fields=["status"]),
         ]
 
-        # Determine if we need this
-        unique_together = [("email", "portfolio")]
-
     # Constants for status field
     class PortfolioInvitationStatus(models.TextChoices):
         INVITED = "invited", "Invited"
@@ -41,7 +36,7 @@ class PortfolioInvitation(TimeStampedModel):
         "registrar.Portfolio",
         on_delete=models.CASCADE,  # delete portfolio, then get rid of invitations
         null=False,
-        related_name="portfolio_invitations",
+        related_name="portfolios",
     )
 
     roles = ArrayField(
@@ -85,8 +80,8 @@ class PortfolioInvitation(TimeStampedModel):
         """
         Retrieve the permissions for the user's portfolio roles from the invite.
         """
-        UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
+        # UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
-        return self.roles, self.additional_permissions)
+        return UserPortfolioPermission.get_portfolio_permissions(self.roles, self.additional_permissions)
 
     @transition(field="status", source=PortfolioInvitationStatus.INVITED, target=PortfolioInvitationStatus.RETRIEVED)
     def retrieve(self):
@@ -97,8 +92,7 @@ class PortfolioInvitation(TimeStampedModel):
         """
 
         # get a user with this email address
-        UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
+        # UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
-        User = get_user_model()
         try:
             user = User.objects.get(email=self.email)
         except User.DoesNotExist:
@@ -131,14 +125,25 @@ class PortfolioInvitation(TimeStampedModel):
             if UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS:
                 raise ValidationError("View all domains cannot be assigned to non-admin roles.")
 
-        # Check if a user is set without accessing the related object.
+        # UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
-        # TODO - revise this user check
+        roles = self.roles if self.roles is not None else []
-        # get a user with this email address
+        bad_perms = UserPortfolioPermission.get_forbidden_permissions(self.roles, self.additional_permissions)
-        User = get_user_model()
+        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)}>"
+            )
+
         user = User.objects.filter(email=self.email).first()
         if not flag_is_active_for_user(user, "multiple_portfolios"):
             # TODO - need to display multiple validation errors
-            UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
             existing_permissions = UserPortfolioPermission.objects.filter(user=user)
             existing_invitations = PortfolioInvitation.objects.exclude(id=self.id).filter(email=user.email)
             if existing_permissions.exists():

src/registrar/models/user.py

@@ -9,7 +9,6 @@ from registrar.models import DomainInformation, UserDomainRole
 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
@@ -420,6 +419,7 @@ class User(AbstractUser):
     def check_portfolio_invitations_on_login(self):
         """When a user first arrives on the site, we need to retrieve any portfolio
         invitations that match their email address."""
+        PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation")
         for invitation in PortfolioInvitation.objects.filter(
             email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED
         ):

src/registrar/models/user_portfolio_permission.py

@@ -32,6 +32,15 @@ class UserPortfolioPermission(TimeStampedModel):
         ],
     }
 
+    # Determines which roles are forbidden for certain role types to possess.
+    FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS = {
+        UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [
+            UserPortfolioPermissionChoices.VIEW_MEMBERS,
+            UserPortfolioPermissionChoices.EDIT_MEMBERS,
+            UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS,
+        ],
+    }
+
     user = models.ForeignKey(
         "registrar.User",
         null=False,
@@ -109,34 +118,72 @@ class UserPortfolioPermission(TimeStampedModel):
             portfolio_permissions.update(additional_permissions)
         return list(portfolio_permissions)
 
+    @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."""
+        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.
+        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
+        bad_perms = portfolio_permissions & common_forbidden_perms
+        return bad_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)
-        portfolio_permissions = self._get_portfolio_permissions()
+        portfolio_permissions = set(self._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.")
 
-        is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in self.roles
+        # == Validate role permissions. Compares existing permissions to forbidden ones. == #
-        all_permissions = portfolio_permissions
+        roles = self.roles if self.roles is not None else []
-        if not is_admin:
+        bad_perms = self.get_forbidden_permissions(self.roles, self.additional_permissions)
-            # Question: should we also check the edit perms?
+        if bad_perms:
-            # TODO - need to display multiple validation errors at once
+            readable_perms = [
-            if UserPortfolioPermissionChoices.VIEW_MEMBERS in all_permissions:
+                UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) 
-                raise ValidationError("View members can only be assigned to admin roles.")
+                for perm in bad_perms
-            
+            ]
-            if UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS:
+            readable_roles = [
-                raise ValidationError("View all domains can only be assigned to admin 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)}>"
+            )
+        # NOTE: if the user has two conflicting roles, like ["admin", "member"],
+        # then this validation will still fail if a role is found that conflicts with member.
+        # Question for reviewers: thoughts? 
+        # for role in roles:
+        #     bad_perms = portfolio_permissions & set(self.FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS.get(role, []))
+        #     if bad_perms:
+        #         readable_role = UserPortfolioRoleChoices.get_user_portfolio_role_label(role)
+        #         invalid = [UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) for perm in bad_perms]
+        #         raise ValidationError(
+        #             f"These permissions cannot be assigned to {readable_role}: {', '.join(invalid)}"
+        #         )
 
+        # == Validate that only one permission exists when multiple_portfolios is disabled == #
         # Check if a user is set without accessing the related object.
         PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation")
         existing_permissions = UserPortfolioPermission.objects.exclude(id=self.id).filter(user=self.user)
-        # Should check on isnull portfolio
         existing_invitations = PortfolioInvitation.objects.filter(email=self.user.email)
         if not flag_is_active_for_user(self.user, "multiple_portfolios"):
             # TODO - need to display multiple validation errors