diff --git a/src/registrar/migrations/0139_alter_portfolioinvitation_portfolio_and_more.py b/src/registrar/migrations/0139_alter_portfolioinvitation_portfolio_and_more.py deleted file mode 100644 index dd2d5f24b..000000000 --- a/src/registrar/migrations/0139_alter_portfolioinvitation_portfolio_and_more.py +++ /dev/null @@ -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")}, - ), - ] diff --git a/src/registrar/models/__init__.py b/src/registrar/models/__init__.py index a1738cc76..d57c0f0d5 100644 --- a/src/registrar/models/__init__.py +++ b/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 diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index e28fb5b5e..6b46eebb3 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/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") - return self.roles, self.additional_permissions) + # UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission") + 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") - User = get_user_model() + # UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission") 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. - # TODO - revise this user check - # get a user with this email address - User = get_user_model() + # UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission") + roles = self.roles if self.roles is not None else [] + bad_perms = UserPortfolioPermission.get_forbidden_permissions(self.roles, self.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)}>" + ) + 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(): diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 2d65aa02e..9b5c1d50f 100644 --- a/src/registrar/models/user.py +++ b/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 ): diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index f8f7a72c3..d78302aba 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/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 - all_permissions = portfolio_permissions - if not is_admin: - # Question: should we also check the edit perms? - # TODO - need to display multiple validation errors at once - if UserPortfolioPermissionChoices.VIEW_MEMBERS in all_permissions: - raise ValidationError("View members can only be assigned to admin roles.") - - if UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS: - raise ValidationError("View all domains can only be assigned to admin roles.") + # == Validate role permissions. Compares existing permissions to forbidden ones. == # + roles = self.roles if self.roles is not None else [] + bad_perms = self.get_forbidden_permissions(self.roles, self.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)}>" + ) + # 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