diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index dfe58021c..1a449bd0e 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -1,3 +1,4 @@ +import logging import functools from django.core.exceptions import PermissionDenied from django.utils.decorators import method_decorator @@ -5,6 +6,9 @@ from registrar.models import Domain, DomainInformation, DomainInvitation, Domain from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission + +logger = logging.getLogger(__name__) + # Constants for clarity ALL = "all" IS_STAFF = "is_staff" @@ -20,6 +24,7 @@ HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM = "has_portfolio_domain_requests_any_perm HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL = "has_portfolio_domain_requests_view_all" HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT = "has_portfolio_domain_requests_edit" HAS_PORTFOLIO_MEMBERS_ANY_PERM = "has_portfolio_members_any_perm" +HAS_PORTFOLIO_MEMBERS_VIEW_AND_EDIT = "has_portfolio_members_view_and_edit" HAS_PORTFOLIO_MEMBERS_EDIT = "has_portfolio_members_edit" HAS_PORTFOLIO_MEMBERS_VIEW = "has_portfolio_members_view" @@ -109,7 +114,9 @@ def _user_has_permission(user, request, rules, **kwargs): (IS_PORTFOLIO_MEMBER, lambda: user.is_org_user(request)), ( HAS_PORTFOLIO_DOMAINS_VIEW_ALL, - lambda: _has_portfolio_view_all_domains(request, kwargs.get("domain_pk")), + lambda: user.is_org_user(request) + and user.has_view_all_domains_portfolio_permission(portfolio) + and _domain_exists_under_portfolio(portfolio, kwargs.get("domain_pk")), ), ( HAS_PORTFOLIO_DOMAINS_ANY_PERM, @@ -155,10 +162,29 @@ def _user_has_permission(user, request, rules, **kwargs): or user.has_edit_members_portfolio_permission(portfolio) ), ), + ( + # More restrictive check as compared to HAS_PORTFOLIO_MEMBERS_ANY_PERM. + # This is needed because grant_access does not apply perms in a layered fashion + # and grants access when any valid perm is found - so chaining view and edit works differently. + HAS_PORTFOLIO_MEMBERS_VIEW_AND_EDIT, + lambda: user.is_org_user(request) + and ( + user.has_view_members_portfolio_permission(portfolio) + and user.has_edit_members_portfolio_permission(portfolio) + ) + and ( + _member_exists_under_portfolio(portfolio, kwargs.get("member_pk")) + or _member_invitation_exists_under_portfolio(portfolio, kwargs.get("invitedmember_pk")) + ), + ), ( HAS_PORTFOLIO_MEMBERS_EDIT, lambda: user.is_org_user(request) and user.has_edit_members_portfolio_permission(portfolio) + and ( + _member_exists_under_portfolio(portfolio, kwargs.get("member_pk")) + or _member_invitation_exists_under_portfolio(portfolio, kwargs.get("invitedmember_pk")) + ), ), ( HAS_PORTFOLIO_MEMBERS_VIEW, @@ -202,17 +228,47 @@ def _is_domain_manager(user, **kwargs): return False def _domain_exists_under_portfolio(portfolio, domain_pk): - """Checks to see if the given domain exists under the provided portfolio""" + """Checks to see if the given domain exists under the provided portfolio. Returns True if the pk is None. + HELPFUL REMINDER: Watch for typos! Verify that the kwarg key exists before using this function. + """ + # The view expects this, and the page will throw an error without this if it needs it. + # Thus, if it is none, we are not checking on a specific record and therefore there is nothing to check. + if not domain_pk: + logger.info("_domain_exists_under_portfolio => Could not find domain_pk. This is a non-issue if called from the right context.") + return True return Domain.objects.filter(domain_info__portfolio=portfolio, id=domain_pk).exists() def _domain_request_exists_under_portfolio(portfolio, domain_request_pk): - """Checks to see if the given domain request exists under the provided portfolio""" + """Checks to see if the given domain request exists under the provided portfolio. Returns True if the pk is None. + HELPFUL REMINDER: Watch for typos! Verify that the kwarg key exists before using this function. + """ + # The view expects this, and the page will throw an error without this if it needs it. + # Thus, if it is none, we are not checking on a specific record and therefore there is nothing to check. + if not domain_request_pk: + logger.info("_domain_request_exists_under_portfolio => Could not find domain_request_pk. This is a non-issue if called from the right context.") + return True return DomainRequest.objects.filter(portfolio=portfolio, id=domain_request_pk).exists() def _member_exists_under_portfolio(portfolio, member_pk): + """Checks to see if the given UserPortfolioPermission exists under the provided portfolio. Returns True if the pk is None. + HELPFUL REMINDER: Watch for typos! Verify that the kwarg key exists before using this function. + """ + # The view expects this, and the page will throw an error without this if it needs it. + # Thus, if it is none, we are not checking on a specific record and therefore there is nothing to check. + if not member_pk: + logger.info("_member_exists_under_portfolio => Could not find member_pk. This is a non-issue if called from the right context.") + return True return UserPortfolioPermission.objects.filter(portfolio=portfolio, id=member_pk).exists() def _member_invitation_exists_under_portfolio(portfolio, invitedmember_pk): + """Checks to see if the given PortfolioInvitation exists under the provided portfolio. Returns True if the pk is None. + HELPFUL REMINDER: Watch for typos! Verify that the kwarg key exists before using this function. + """ + # The view expects this, and the page will throw an error without this if it needs it. + # Thus, if it is none, we are not checking on a specific record and therefore there is nothing to check. + if not invitedmember_pk: + logger.info("_member_invitation_exists_under_portfolio => Could not find invitedmember_pk. This is a non-issue if called from the right context.") + return True return PortfolioInvitation.objects.filter(portfolio=portfolio, id=invitedmember_pk).exists() def _is_domain_request_creator(user, domain_request_pk): @@ -310,15 +366,3 @@ def _is_staff_managing_domain(request, **kwargs): # the user is permissioned, # and it is in a valid status return True - - -def _has_portfolio_view_all_domains(request, domain_pk): - """Returns whether the user in the request can access the domain - via portfolio view all domains permission.""" - portfolio = request.session.get("portfolio") - if request.user.has_view_all_domains_portfolio_permission(portfolio): - if Domain.objects.filter(id=domain_pk).exists(): - domain = Domain.objects.get(id=domain_pk) - if domain.domain_info.portfolio == portfolio: - return True - return False diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 3a04d4c86..1a31f9a7f 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -13,6 +13,7 @@ from registrar.decorators import ( HAS_PORTFOLIO_MEMBERS_ANY_PERM, HAS_PORTFOLIO_MEMBERS_EDIT, HAS_PORTFOLIO_MEMBERS_VIEW, + HAS_PORTFOLIO_MEMBERS_VIEW_AND_EDIT, IS_PORTFOLIO_MEMBER, grant_access, ) @@ -117,7 +118,9 @@ class PortfolioMemberView(DetailView, View): ) -@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW, HAS_PORTFOLIO_MEMBERS_EDIT) +# The parent page requires VIEW access, the child requires EDIT. +# This prevents url skimming without having to inherit the base class. +@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW_AND_EDIT) class PortfolioMemberDeleteView(View): pk_url_kwarg = "member_pk" @@ -220,7 +223,9 @@ class PortfolioMemberDeleteView(View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW, HAS_PORTFOLIO_MEMBERS_EDIT) +# The parent page requires VIEW access, the child requires EDIT. +# This prevents url skimming without having to inherit the base class. +@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW_AND_EDIT) class PortfolioMemberEditView(DetailView, View): model = Portfolio context_object_name = "portfolio" @@ -324,7 +329,9 @@ class PortfolioMemberDomainsView(View): ) -@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW, HAS_PORTFOLIO_MEMBERS_EDIT) +# The parent page requires VIEW access, the child requires EDIT. +# This prevents url skimming without having to inherit the base class. +@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW_AND_EDIT) class PortfolioMemberDomainsEditView(DetailView, View): model = Portfolio context_object_name = "portfolio" @@ -482,7 +489,9 @@ class PortfolioInvitedMemberView(DetailView, View): ) -@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW, HAS_PORTFOLIO_MEMBERS_EDIT) +# The parent page requires VIEW access, the child requires EDIT. +# This prevents url skimming without having to inherit the base class. +@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW_AND_EDIT) class PortfolioInvitedMemberDeleteView(View): pk_url_kwarg = "invitedmember_pk" def post(self, request, invitedmember_pk): @@ -528,7 +537,9 @@ class PortfolioInvitedMemberDeleteView(View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW, HAS_PORTFOLIO_MEMBERS_EDIT) +# The parent page requires VIEW access, the child requires EDIT. +# This prevents url skimming without having to inherit the base class. +@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW_AND_EDIT) class PortfolioInvitedMemberEditView(DetailView, View): model = Portfolio context_object_name = "portfolio" @@ -618,7 +629,9 @@ class PortfolioInvitedMemberDomainsView(View): ) -@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW, HAS_PORTFOLIO_MEMBERS_EDIT) +# The parent page requires VIEW access, the child requires EDIT. +# This prevents url skimming without having to inherit the base class. +@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW_AND_EDIT) class PortfolioInvitedMemberDomainsEditView(DetailView, View): model = Portfolio