From d84aa421d90ba2d3a26bb998f428f3643ecf1841 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 10 Feb 2025 19:23:50 -0500 Subject: [PATCH 01/17] wip --- src/djangooidc/views.py | 2 + src/registrar/config/settings.py | 2 + src/registrar/decorators.py | 72 ++++++++++++++++++++ src/registrar/registrar_middleware.py | 39 +++++++++++ src/registrar/utility/domain_cache_helper.py | 0 src/registrar/views/domain_requests_json.py | 3 +- src/registrar/views/domains_json.py | 3 +- src/registrar/views/index.py | 2 + 8 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 src/registrar/decorators.py create mode 100644 src/registrar/utility/domain_cache_helper.py diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 815df4ecf..984936a4c 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -5,12 +5,14 @@ import logging from django.conf import settings from django.contrib.auth import logout as auth_logout from django.contrib.auth import authenticate, login +from login_required import login_not_required from django.http import HttpResponseRedirect from django.shortcuts import redirect from urllib.parse import parse_qs, urlencode from djangooidc.oidc import Client from djangooidc import exceptions as o_e +from registrar.decorators import grant_access from registrar.models import User from registrar.views.utility.error_views import custom_500_error_view, custom_401_error_view diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 78439188e..9b51383f3 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -200,6 +200,8 @@ MIDDLEWARE = [ "waffle.middleware.WaffleMiddleware", "registrar.registrar_middleware.CheckUserProfileMiddleware", "registrar.registrar_middleware.CheckPortfolioMiddleware", + # Restrict access using Opt-Out approach + "registrar.registrar_middleware.RestrictAccessMiddleware", ] # application object used by Django's built-in servers (e.g. `runserver`) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py new file mode 100644 index 000000000..08214f89c --- /dev/null +++ b/src/registrar/decorators.py @@ -0,0 +1,72 @@ +from functools import wraps +from django.http import JsonResponse +from django.core.exceptions import ObjectDoesNotExist + +from registrar.models.domain import Domain +from registrar.models.user_domain_role import UserDomainRole + +# Constants for clarity +ALL = "all" +IS_SUPERUSER = "is_superuser" +IS_STAFF = "is_staff" +IS_DOMAIN_MANAGER = "is_domain_manager" + +def grant_access(*rules): + """ + Allows multiple rules in a single decorator call: + @grant_access(IS_STAFF, IS_SUPERUSER, IS_DOMAIN_MANAGER) + or multiple stacked decorators: + @grant_access(IS_SUPERUSER) + @grant_access(IS_DOMAIN_MANAGER) + """ + + def decorator(view_func): + view_func.has_explicit_access = True # Mark as explicitly access-controlled + existing_rules = getattr(view_func, "_access_rules", set()) + existing_rules.update(rules) # Support multiple rules in one call + view_func._access_rules = existing_rules # Store rules on the function + + @wraps(view_func) + def wrapper(request, *args, **kwargs): + user = request.user + + # Skip authentication if @login_not_required is applied + if getattr(view_func, "login_not_required", False): + return view_func(request, *args, **kwargs) + + # Allow everyone if `ALL` is in rules + if ALL in view_func._access_rules: + return view_func(request, *args, **kwargs) + + # Ensure user is authenticated + if not user.is_authenticated: + return JsonResponse({"error": "Authentication required"}, status=403) + + conditions_met = [] + + if IS_STAFF in view_func._access_rules: + conditions_met.append(user.is_staff) + + if not any(conditions_met) and IS_SUPERUSER in view_func._access_rules: + conditions_met.append(user.is_superuser) + + if not any(conditions_met) and IS_DOMAIN_MANAGER in view_func._access_rules: + domain_id = kwargs.get('pk') or kwargs.get('domain_id') + if not domain_id: + return JsonResponse({"error": "Domain ID missing"}, status=400) + try: + domain = Domain.objects.get(pk=domain_id) + has_permission = UserDomainRole.objects.filter( + user=user, domain=domain + ).exists() + conditions_met.append(has_permission) + except ObjectDoesNotExist: + return JsonResponse({"error": "Invalid Domain"}, status=404) + + if not any(conditions_met): + return JsonResponse({"error": "Access Denied"}, status=403) + + return view_func(request, *args, **kwargs) + + return wrapper + return decorator diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index ed7a4dffc..c23fb0515 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -3,9 +3,13 @@ Contains middleware used in settings.py """ import logging +import re from urllib.parse import parse_qs +from django.conf import settings from django.urls import reverse from django.http import HttpResponseRedirect +from django.http import JsonResponse +from django.urls import resolve from registrar.models import User from waffle.decorators import flag_is_active @@ -170,3 +174,38 @@ class CheckPortfolioMiddleware: request.session["portfolio"] = request.user.get_first_portfolio() else: request.session["portfolio"] = request.user.get_first_portfolio() + + +class RestrictAccessMiddleware: + """ Middleware that blocks all views unless explicitly permitted """ + + def __init__(self, get_response): + self.get_response = get_response + self.ignored_paths = [re.compile(pattern) for pattern in getattr(settings, "LOGIN_REQUIRED_IGNORE_PATHS", [])] + + def __call__(self, request): + # Allow requests that match LOGIN_REQUIRED_IGNORE_PATHS + if any(pattern.match(request.path) for pattern in self.ignored_paths): + return self.get_response(request) + + # Try to resolve the view function + try: + resolver_match = resolve(request.path_info) + view_func = resolver_match.func + app_name = resolver_match.app_name # Get app name of resolved view + except Exception: + return JsonResponse({"error": "Not Found"}, status=404) + + # Auto-allow Django's built-in admin views (but NOT custom /admin/* views) + if app_name == "admin": + return self.get_response(request) + + # Skip access restriction if the view explicitly allows unauthenticated access + if getattr(view_func, "login_required", True) is False: + return self.get_response(request) + + # Enforce explicit access fules for other views + if not getattr(view_func, "has_explicit_access", False): + return JsonResponse({"error": "Access Denied"}, status=403) + + return self.get_response(request) \ No newline at end of file diff --git a/src/registrar/utility/domain_cache_helper.py b/src/registrar/utility/domain_cache_helper.py new file mode 100644 index 000000000..e69de29bb diff --git a/src/registrar/views/domain_requests_json.py b/src/registrar/views/domain_requests_json.py index 88590010e..b6a9d1072 100644 --- a/src/registrar/views/domain_requests_json.py +++ b/src/registrar/views/domain_requests_json.py @@ -1,5 +1,6 @@ from django.http import JsonResponse from django.core.paginator import Paginator +from registrar.decorators import grant_access, ALL from registrar.models import DomainRequest from django.utils.dateformat import format from django.contrib.auth.decorators import login_required @@ -7,7 +8,7 @@ from django.urls import reverse from django.db.models import Q -@login_required +@grant_access(ALL) def get_domain_requests_json(request): """Given the current request, get all domain requests that are associated with the request user and exclude the APPROVED ones. diff --git a/src/registrar/views/domains_json.py b/src/registrar/views/domains_json.py index 8734ef89c..e0081450d 100644 --- a/src/registrar/views/domains_json.py +++ b/src/registrar/views/domains_json.py @@ -1,6 +1,7 @@ import logging from django.http import JsonResponse from django.core.paginator import Paginator +from registrar.decorators import grant_access, ALL from registrar.models import UserDomainRole, Domain, DomainInformation, User from django.contrib.auth.decorators import login_required from django.urls import reverse @@ -9,7 +10,7 @@ from django.db.models import Q logger = logging.getLogger(__name__) -@login_required +@grant_access(ALL) def get_domains_json(request): """Given the current request, get all domains that are associated with the UserDomainRole object""" diff --git a/src/registrar/views/index.py b/src/registrar/views/index.py index be7149018..d9ff9b209 100644 --- a/src/registrar/views/index.py +++ b/src/registrar/views/index.py @@ -1,6 +1,8 @@ from django.shortcuts import render +from registrar.decorators import grant_access, ALL +@grant_access(ALL) def index(request): """This page is available to anyone without logging in.""" context = {} From a334f7dc3ef02c2aaa2a8bdb7f9fa43d5ac9e60d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 11 Feb 2025 10:06:24 -0500 Subject: [PATCH 02/17] wip --- src/registrar/config/urls.py | 29 +-- src/registrar/decorators.py | 176 +++++++++++++----- src/registrar/registrar_middleware.py | 8 +- src/registrar/templates/domain_add_user.html | 6 +- src/registrar/templates/domain_detail.html | 20 +- src/registrar/templates/domain_dns.html | 8 +- src/registrar/templates/domain_dnssec.html | 6 +- src/registrar/templates/domain_dsdata.html | 6 +- .../templates/domain_nameservers.html | 4 +- src/registrar/templates/domain_renewal.html | 8 +- .../templates/domain_security_email.html | 2 +- src/registrar/templates/domain_sidebar.html | 8 +- .../templates/domain_suborganization.html | 2 +- src/registrar/templates/domain_users.html | 8 +- src/registrar/views/domain.py | 41 ++-- src/registrar/views/domains_json.py | 2 +- src/registrar/views/utility/error_views.py | 8 + src/registrar/views/utility/mixins.py | 4 +- .../views/utility/permission_views.py | 1 + 19 files changed, 216 insertions(+), 131 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index eb095c5ca..27ad01dc8 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -296,56 +296,56 @@ urlpatterns = [ lambda r: always_404(r, "We forgot to include this link, sorry."), name="todo", ), - path("domain/", views.DomainView.as_view(), name="domain"), - path("domain//prototype-dns", views.PrototypeDomainDNSRecordView.as_view(), name="prototype-domain-dns"), - path("domain//users", views.DomainUsersView.as_view(), name="domain-users"), + path("domain/", views.DomainView.as_view(), name="domain"), + path("domain//prototype-dns", views.PrototypeDomainDNSRecordView.as_view(), name="prototype-domain-dns"), + path("domain//users", views.DomainUsersView.as_view(), name="domain-users"), path( - "domain//dns", + "domain//dns", views.DomainDNSView.as_view(), name="domain-dns", ), path( - "domain//dns/nameservers", + "domain//dns/nameservers", views.DomainNameserversView.as_view(), name="domain-dns-nameservers", ), path( - "domain//dns/dnssec", + "domain//dns/dnssec", views.DomainDNSSECView.as_view(), name="domain-dns-dnssec", ), path( - "domain//dns/dnssec/dsdata", + "domain//dns/dnssec/dsdata", views.DomainDsDataView.as_view(), name="domain-dns-dnssec-dsdata", ), path( - "domain//org-name-address", + "domain//org-name-address", views.DomainOrgNameAddressView.as_view(), name="domain-org-name-address", ), path( - "domain//suborganization", + "domain//suborganization", views.DomainSubOrganizationView.as_view(), name="domain-suborganization", ), path( - "domain//senior-official", + "domain//senior-official", views.DomainSeniorOfficialView.as_view(), name="domain-senior-official", ), path( - "domain//security-email", + "domain//security-email", views.DomainSecurityEmailView.as_view(), name="domain-security-email", ), path( - "domain//renewal", + "domain//renewal", views.DomainRenewalView.as_view(), name="domain-renewal", ), path( - "domain//users/add", + "domain//users/add", views.DomainAddUserView.as_view(), name="domain-users-add", ), @@ -370,7 +370,7 @@ urlpatterns = [ name="domain-request-delete", ), path( - "domain//users//delete", + "domain//users//delete", views.DomainDeleteUserView.as_view(http_method_names=["post"]), name="domain-user-delete", ), @@ -392,6 +392,7 @@ urlpatterns = [ # This way, we can share a view for djangooidc, and other pages as we see fit. handler500 = "registrar.views.utility.error_views.custom_500_error_view" handler403 = "registrar.views.utility.error_views.custom_403_error_view" +handler404 = "registrar.views.utility.error_views.custom_404_error_view" # we normally would guard these with `if settings.DEBUG` but tests run with # DEBUG = False even when these apps have been loaded because settings.DEBUG diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 08214f89c..177dfab62 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -1,15 +1,14 @@ -from functools import wraps -from django.http import JsonResponse -from django.core.exceptions import ObjectDoesNotExist - -from registrar.models.domain import Domain -from registrar.models.user_domain_role import UserDomainRole +import functools +from django.core.exceptions import PermissionDenied +from django.utils.decorators import method_decorator +from registrar.models import DomainInformation, DomainRequest, UserDomainRole # Constants for clarity ALL = "all" IS_SUPERUSER = "is_superuser" IS_STAFF = "is_staff" IS_DOMAIN_MANAGER = "is_domain_manager" +IS_STAFF_MANAGING_DOMAIN = "is_staff_managing_domain" def grant_access(*rules): """ @@ -20,53 +19,128 @@ def grant_access(*rules): @grant_access(IS_DOMAIN_MANAGER) """ - def decorator(view_func): - view_func.has_explicit_access = True # Mark as explicitly access-controlled - existing_rules = getattr(view_func, "_access_rules", set()) - existing_rules.update(rules) # Support multiple rules in one call - view_func._access_rules = existing_rules # Store rules on the function + def decorator(view): + if isinstance(view, type): # If decorating a class-based view (CBV) + original_dispatch = view.dispatch # save original dispatch method - @wraps(view_func) - def wrapper(request, *args, **kwargs): - user = request.user - - # Skip authentication if @login_not_required is applied - if getattr(view_func, "login_not_required", False): - return view_func(request, *args, **kwargs) - - # Allow everyone if `ALL` is in rules - if ALL in view_func._access_rules: - return view_func(request, *args, **kwargs) + @method_decorator(grant_access(*rules)) # apply the decorator to dispatch + def wrapped_dispatch(self, request, *args, **kwargs): + if not _user_has_permission(request.user, request, rules, **kwargs): + raise PermissionDenied + return original_dispatch(self, request, *args, **kwargs) - # Ensure user is authenticated - if not user.is_authenticated: - return JsonResponse({"error": "Authentication required"}, status=403) + view.dispatch = wrapped_dispatch # replace dispatch with wrapped version + return view - conditions_met = [] + else: # If decorating a function-based view (FBV) + view.has_explicit_access = True + existing_rules = getattr(view, "_access_rules", set()) + existing_rules.update(rules) + view._access_rules = existing_rules - if IS_STAFF in view_func._access_rules: - conditions_met.append(user.is_staff) - - if not any(conditions_met) and IS_SUPERUSER in view_func._access_rules: - conditions_met.append(user.is_superuser) - - if not any(conditions_met) and IS_DOMAIN_MANAGER in view_func._access_rules: - domain_id = kwargs.get('pk') or kwargs.get('domain_id') - if not domain_id: - return JsonResponse({"error": "Domain ID missing"}, status=400) - try: - domain = Domain.objects.get(pk=domain_id) - has_permission = UserDomainRole.objects.filter( - user=user, domain=domain - ).exists() - conditions_met.append(has_permission) - except ObjectDoesNotExist: - return JsonResponse({"error": "Invalid Domain"}, status=404) - - if not any(conditions_met): - return JsonResponse({"error": "Access Denied"}, status=403) - - return view_func(request, *args, **kwargs) - - return wrapper + @functools.wraps(view) + def wrapper(request, *args, **kwargs): + if not _user_has_permission(request.user, request, rules, **kwargs): + raise PermissionDenied + return view(request, *args, **kwargs) + + return wrapper + return decorator + + +def _user_has_permission(user, request, rules, **kwargs): + """ + Checks if the user meets the permission requirements. + """ + + # Skip authentication if @login_not_required is applied + if getattr(request, "login_not_required", False): + return True + + # Allow everyone if `ALL` is in rules + if ALL in rules: + return True + + # Ensure user is authenticated + if not user.is_authenticated: + return False + + conditions_met = [] + + if IS_STAFF in rules: + conditions_met.append(user.is_staff) + + if not any(conditions_met) and IS_SUPERUSER in rules: + conditions_met.append(user.is_superuser) + + if not any(conditions_met) and IS_DOMAIN_MANAGER in rules: + domain_id = kwargs.get('domain_pk') + # Check UserDomainRole directly instead of fetching Domain + has_permission = UserDomainRole.objects.filter(user=user, domain_id=domain_id).exists() + conditions_met.append(has_permission) + + if not any(conditions_met) and IS_STAFF_MANAGING_DOMAIN in rules: + domain_id = kwargs.get('domain_pk') + has_permission = _can_access_other_user_domains(request, domain_id) + conditions_met.append(has_permission) + + return any(conditions_met) + + +def _can_access_other_user_domains(request, domain_pk): + """Checks to see if an authorized user (staff or superuser) + can access a domain that they did not create or were invited to. + """ + + # Check if the request user is permissioned... + user_is_analyst_or_superuser = request.user.has_perm( + "registrar.analyst_access_permission" + ) or request.user.has_perm("registrar.full_access_permission") + + if not user_is_analyst_or_superuser: + return False + + # Check if the user is attempting a valid edit action. + # In other words, if the analyst/admin did not click + # the 'Manage Domain' button in /admin, + # then they cannot access this page. + session = request.session + can_do_action = ( + "analyst_action" in session + and "analyst_action_location" in session + and session["analyst_action_location"] == domain_pk + ) + + if not can_do_action: + return False + + # Analysts may manage domains, when they are in these statuses: + valid_domain_statuses = [ + DomainRequest.DomainRequestStatus.APPROVED, + DomainRequest.DomainRequestStatus.IN_REVIEW, + DomainRequest.DomainRequestStatus.REJECTED, + DomainRequest.DomainRequestStatus.ACTION_NEEDED, + # Edge case - some domains do not have + # a status or DomainInformation... aka a status of 'None'. + # It is necessary to access those to correct errors. + None, + ] + + requested_domain = DomainInformation.objects.filter(domain_id=domain_pk).first() + + # if no domain information or domain request exist, the user + # should be able to manage the domain; however, if domain information + # and domain request exist, and domain request is not in valid status, + # user should not be able to manage domain + if ( + requested_domain + and requested_domain.domain_request + and requested_domain.domain_request.status not in valid_domain_statuses + ): + return False + + # Valid session keys exist, + # the user is permissioned, + # and it is in a valid status + return True diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index c23fb0515..e3dcbe788 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -6,9 +6,9 @@ import logging import re from urllib.parse import parse_qs from django.conf import settings +from django.core.exceptions import PermissionDenied from django.urls import reverse from django.http import HttpResponseRedirect -from django.http import JsonResponse from django.urls import resolve from registrar.models import User from waffle.decorators import flag_is_active @@ -182,7 +182,7 @@ class RestrictAccessMiddleware: def __init__(self, get_response): self.get_response = get_response self.ignored_paths = [re.compile(pattern) for pattern in getattr(settings, "LOGIN_REQUIRED_IGNORE_PATHS", [])] - + def __call__(self, request): # Allow requests that match LOGIN_REQUIRED_IGNORE_PATHS if any(pattern.match(request.path) for pattern in self.ignored_paths): @@ -194,7 +194,7 @@ class RestrictAccessMiddleware: view_func = resolver_match.func app_name = resolver_match.app_name # Get app name of resolved view except Exception: - return JsonResponse({"error": "Not Found"}, status=404) + return self.get_response(request) # Auto-allow Django's built-in admin views (but NOT custom /admin/* views) if app_name == "admin": @@ -206,6 +206,6 @@ class RestrictAccessMiddleware: # Enforce explicit access fules for other views if not getattr(view_func, "has_explicit_access", False): - return JsonResponse({"error": "Access Denied"}, status=403) + raise PermissionDenied return self.get_response(request) \ No newline at end of file diff --git a/src/registrar/templates/domain_add_user.html b/src/registrar/templates/domain_add_user.html index 04565f61e..abc549a82 100644 --- a/src/registrar/templates/domain_add_user.html +++ b/src/registrar/templates/domain_add_user.html @@ -16,10 +16,10 @@ Domains
  • - {{ domain.name }} + {{ domain.name }}
  • - Domain managers + Domain managers
  • Add a domain manager @@ -27,7 +27,7 @@ {% else %} - {% url 'domain-users' pk=domain.id as url %} + {% url 'domain-users' domain_pk=domain.id as url %} {% elif steps.prev %} - + Previous step diff --git a/src/registrar/templates/domain_request_sidebar.html b/src/registrar/templates/domain_request_sidebar.html index 1af54bb24..e7a0a307c 100644 --- a/src/registrar/templates/domain_request_sidebar.html +++ b/src/registrar/templates/domain_request_sidebar.html @@ -15,7 +15,7 @@ {% endif %} {% endif %} - If you withdraw your request, we won't review it. Once you withdraw your request, you can edit it and submit it again.

    -

    Withdraw request - Cancel

    +

    Withdraw request + Cancel

    diff --git a/src/registrar/templates/includes/portfolio_request_review_steps.html b/src/registrar/templates/includes/portfolio_request_review_steps.html index 89c8e652a..53ad36a3f 100644 --- a/src/registrar/templates/includes/portfolio_request_review_steps.html +++ b/src/registrar/templates/includes/portfolio_request_review_steps.html @@ -4,7 +4,7 @@ {% for step in steps %}
    {% if is_editable %} - {% namespaced_url 'domain-request' step id=domain_request_id as domain_request_url %} + {% namespaced_url 'domain-request' step domain_request_pk=domain_request_id as domain_request_url %} {% endif %} {% if step == Step.REQUESTING_ENTITY %} diff --git a/src/registrar/templates/includes/request_review_steps.html b/src/registrar/templates/includes/request_review_steps.html index f1b13f890..5e00c2160 100644 --- a/src/registrar/templates/includes/request_review_steps.html +++ b/src/registrar/templates/includes/request_review_steps.html @@ -4,7 +4,7 @@ {% for step in steps %}
    {% if is_editable %} - {% namespaced_url 'domain-request' step id=domain_request_id as domain_request_url %} + {% namespaced_url 'domain-request' step domain_request_pk=domain_request_id as domain_request_url %} {% endif %} {% if step == Step.ORGANIZATION_TYPE %} diff --git a/src/registrar/templates/includes/request_status_manage.html b/src/registrar/templates/includes/request_status_manage.html index d96adedf6..f616522a7 100644 --- a/src/registrar/templates/includes/request_status_manage.html +++ b/src/registrar/templates/includes/request_status_manage.html @@ -114,7 +114,7 @@ {% block modify_request %} {% if DomainRequest.is_withdrawable %} -

    +

    Withdraw request

    {% endif %} diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 3248c1368..bcefbbc77 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -5,28 +5,27 @@ from django.shortcuts import redirect, render from django.urls import resolve, reverse from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ -from django.views.generic import TemplateView +from django.views.generic import DeleteView, DetailView, TemplateView from django.contrib import messages +from registrar.decorators import ( + HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT, + HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL, + IS_DOMAIN_REQUEST_CREATOR, + grant_access, +) from registrar.forms import domain_request_wizard as forms from registrar.forms.utility.wizard_form_helper import request_step_list from registrar.models import DomainRequest from registrar.models.contact import Contact from registrar.models.user import User from registrar.views.utility import StepsHelper -from registrar.views.utility.permission_views import DomainRequestPermissionDeleteView from registrar.utility.enums import Step, PortfolioDomainRequestStep -from .utility import ( - DomainRequestPermissionView, - DomainRequestPermissionWithdrawView, - DomainRequestWizardPermissionView, - DomainRequestPortfolioViewonlyView, -) - logger = logging.getLogger(__name__) -class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR, HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT) +class DomainRequestWizard(TemplateView): """ A common set of methods and configuration. @@ -51,7 +50,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): # NB: this is included here for reference. Do not change it without # also changing the many places it is hardcoded in the HTML templates URL_NAMESPACE = "domain-request" - # name for accessing /domain-request//edit + # name for accessing /domain-request//edit EDIT_URL_NAME = "edit-domain-request" NEW_URL_NAME = "start" FINISHED_URL_NAME = "finished" @@ -174,7 +173,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): def has_pk(self): """Does this wizard know about a DomainRequest database record?""" - return bool(self.kwargs.get("id") is not None) + return bool(self.kwargs.get("domain_request_pk") is not None) def get_step_enum(self): """Determines which step enum we should use for the wizard""" @@ -209,11 +208,11 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): try: self._domain_request = DomainRequest.objects.get( creator=creator, - pk=self.kwargs.get("id"), + pk=self.kwargs.get("domain_request_pk"), ) return self._domain_request except DomainRequest.DoesNotExist: - logger.debug("DomainRequest id %s did not have a DomainRequest" % id) + logger.debug("DomainRequest id %s did not have a DomainRequest" % self.kwargs.get("domain_request_pk")) # If a user is creating a request, we assume that perms are handled upstream if self.request.user.is_org_user(self.request): @@ -292,10 +291,10 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): current_url = resolve(request.path_info).url_name - # if user visited via an "edit" url, associate the id of the + # if user visited via an "edit" url, associate the pk of the # domain request they are trying to edit to this wizard instance # and remove any prior wizard data from their session - if current_url == self.EDIT_URL_NAME and "id" in kwargs: + if current_url == self.EDIT_URL_NAME and "domain_request_pk" in kwargs: del self.storage # if accessing this class directly, redirect to either to an acknowledgement @@ -474,7 +473,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): def goto(self, step): self.steps.current = step - return redirect(reverse(f"{self.URL_NAMESPACE}:{step}", kwargs={"id": self.domain_request.id})) + return redirect(reverse(f"{self.URL_NAMESPACE}:{step}", kwargs={"domain_request_pk": self.domain_request.id})) def goto_next_step(self): """Redirects to the next step.""" @@ -823,23 +822,12 @@ class Finished(DomainRequestWizard): return render(self.request, self.template_name, context) -class DomainRequestStatus(DomainRequestPermissionView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR, HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT) +class DomainRequestStatus(DetailView): template_name = "domain_request_status.html" - - def has_permission(self): - """ - Override of the base has_permission class to account for portfolio permissions - """ - has_base_perms = super().has_permission() - if not has_base_perms: - return False - - if self.request.user.is_org_user(self.request): - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_request_portfolio_permission(portfolio): - return False - - return True + model = DomainRequest + pk_url_kwarg = "domain_request_pk" + context_object_name = "DomainRequest" def get_context_data(self, **kwargs): """Context override to add a step list to the context""" @@ -854,19 +842,27 @@ class DomainRequestStatus(DomainRequestPermissionView): return context -class DomainRequestWithdrawConfirmation(DomainRequestPermissionWithdrawView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR) +class DomainRequestWithdrawConfirmation(DetailView): """This page will ask user to confirm if they want to withdraw - The DomainRequestPermissionView restricts access so that only the + Access is restricted so that only the `creator` of the domain request may withdraw it. """ - template_name = "domain_request_withdraw_confirmation.html" + template_name = "domain_request_withdraw_confirmation.html" # DetailView property for what model this is viewing + model = DomainRequest + pk_url_kwarg = "domain_request_pk" + context_object_name = "DomainRequest" -class DomainRequestWithdrawn(DomainRequestPermissionWithdrawView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR) +class DomainRequestWithdrawn(DetailView): # this view renders no template template_name = "" + model = DomainRequest + pk_url_kwarg = "domain_request_pk" + context_object_name = "DomainRequest" def get(self, *args, **kwargs): """View class that does the actual withdrawing. @@ -874,7 +870,7 @@ class DomainRequestWithdrawn(DomainRequestPermissionWithdrawView): If user click on withdraw confirm button, this view updates the status to withdraw and send back to homepage. """ - domain_request = DomainRequest.objects.get(id=self.kwargs["pk"]) + domain_request = DomainRequest.objects.get(id=self.kwargs["domain_request_pk"]) domain_request.withdraw() domain_request.save() if self.request.user.is_org_user(self.request): @@ -883,28 +879,22 @@ class DomainRequestWithdrawn(DomainRequestPermissionWithdrawView): return HttpResponseRedirect(reverse("home")) -class DomainRequestDeleteView(DomainRequestPermissionDeleteView): +@grant_access(IS_DOMAIN_REQUEST_CREATOR, HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT) +class DomainRequestDeleteView(DeleteView): """Delete view for home that allows the end user to delete DomainRequests""" object: DomainRequest # workaround for type mismatch in DeleteView + model = DomainRequest + pk_url_kwarg = "domain_request_pk" def has_permission(self): """Custom override for has_permission to exclude all statuses, except WITHDRAWN and STARTED""" - has_perm = super().has_permission() - if not has_perm: - return False status = self.get_object().status valid_statuses = [DomainRequest.DomainRequestStatus.WITHDRAWN, DomainRequest.DomainRequestStatus.STARTED] if status not in valid_statuses: return False - # Portfolio users cannot delete their requests if they aren't permissioned to do so - if self.request.user.is_org_user(self.request): - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_request_portfolio_permission(portfolio): - return False - return True def get_success_url(self): @@ -989,8 +979,12 @@ class DomainRequestDeleteView(DomainRequestPermissionDeleteView): # region Portfolio views -class PortfolioDomainRequestStatusViewOnly(DomainRequestPortfolioViewonlyView): +@grant_access(HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL) +class PortfolioDomainRequestStatusViewOnly(DetailView): template_name = "portfolio_domain_request_status_viewonly.html" + model = DomainRequest + pk_url_kwarg = "domain_request_pk" + context_object_name = "DomainRequest" def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) diff --git a/src/registrar/views/domain_requests_json.py b/src/registrar/views/domain_requests_json.py index b6a9d1072..4e475321c 100644 --- a/src/registrar/views/domain_requests_json.py +++ b/src/registrar/views/domain_requests_json.py @@ -155,9 +155,9 @@ def serialize_domain_request(request, domain_request, user): # Map the action label to corresponding URLs and icons action_url_map = { - "Edit": reverse("edit-domain-request", kwargs={"id": domain_request.id}), - "Manage": reverse("domain-request-status", kwargs={"pk": domain_request.id}), - "View": reverse("domain-request-status-viewonly", kwargs={"pk": domain_request.id}), + "Edit": reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.id}), + "Manage": reverse("domain-request-status", kwargs={"domain_request_pk": domain_request.id}), + "View": reverse("domain-request-status-viewonly", kwargs={"domain_request_pk": domain_request.id}), } svg_icon_map = {"Edit": "edit", "Manage": "settings", "View": "visibility"} diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 0f93ec8e1..9f864324f 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -6,6 +6,7 @@ from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse from django.utils.safestring import mark_safe from django.contrib import messages +from registrar.decorators import HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM, grant_access from registrar.forms import portfolio as portfolioForms from registrar.models import Portfolio, User from registrar.models.domain import Domain @@ -25,7 +26,6 @@ from registrar.utility.errors import MissingEmailError from registrar.utility.enums import DefaultUserValues from registrar.views.utility.mixins import PortfolioMemberPermission from registrar.views.utility.permission_views import ( - PortfolioDomainRequestsPermissionView, PortfolioDomainsPermissionView, PortfolioBasePermissionView, NoPortfolioDomainsPermissionView, @@ -58,7 +58,8 @@ class PortfolioDomainsView(PortfolioDomainsPermissionView, View): return render(request, "portfolio_domains.html", context) -class PortfolioDomainRequestsView(PortfolioDomainRequestsPermissionView, View): +@grant_access(HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM) +class PortfolioDomainRequestsView(View): template_name = "portfolio_requests.html" diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index 6798eb4ee..4a33c9944 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -3,11 +3,7 @@ from .always_404 import always_404 from .permission_views import ( DomainPermissionView, - DomainRequestPermissionView, - DomainRequestPermissionWithdrawView, - DomainRequestWizardPermissionView, PortfolioMembersPermission, - DomainRequestPortfolioViewonlyView, DomainInvitationPermissionCancelView, ) from .api_views import get_senior_official_from_federal_agency_json diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 23bcff162..682d9ffcb 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -286,51 +286,6 @@ class DomainPermission(PermissionsLoginMixin): return True -class DomainRequestPermission(PermissionsLoginMixin): - """Permission mixin that redirects to domain request if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to this domain request. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - if not self.request.user.is_authenticated: - return False - - # user needs to be the creator of the domain request - # this query is empty if there isn't a domain request with this - # id and this user as creator - if not DomainRequest.objects.filter(creator=self.request.user, id=self.kwargs["pk"]).exists(): - return False - - return True - - -class DomainRequestPortfolioViewonlyPermission(PermissionsLoginMixin): - """Permission mixin that redirects to domain request if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to this domain request. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - if not self.request.user.is_authenticated: - return False - - if not self.request.user.is_org_user(self.request): - return False - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_all_requests_portfolio_permission(portfolio): - return False - - return True - - class UserDeleteDomainRolePermission(PermissionsLoginMixin): """Permission mixin for UserDomainRole if user has access, otherwise 403""" @@ -365,67 +320,6 @@ class UserDeleteDomainRolePermission(PermissionsLoginMixin): return True -class DomainRequestPermissionWithdraw(PermissionsLoginMixin): - """Permission mixin that redirects to withdraw action on domain request - if user has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to withdraw this domain request.""" - if not self.request.user.is_authenticated: - return False - - # user needs to be the creator of the domain request - # this query is empty if there isn't a domain request with this - # id and this user as creator - if not DomainRequest.objects.filter(creator=self.request.user, id=self.kwargs["pk"]).exists(): - return False - - # Restricted users should not be able to withdraw domain requests - if self.request.user.is_restricted(): - return False - - return True - - -class DomainRequestWizardPermission(PermissionsLoginMixin): - """Permission mixin that redirects to start or edit domain request if - user has access, otherwise 403""" - - def has_permission(self): - """Check if this user has permission to start or edit a domain request. - - The user is in self.request.user - """ - - if not self.request.user.is_authenticated: - return False - - # The user has an ineligible flag - if self.request.user.is_restricted(): - return False - - # If the user is an org user and doesn't have add/edit perms, forbid this - if self.request.user.is_org_user(self.request): - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_request_portfolio_permission(portfolio): - return False - - # user needs to be the creator of the domain request to edit it. - id = self.kwargs.get("id") if hasattr(self, "kwargs") else None - if not id: - domain_request_wizard = self.request.session.get("wizard_domain_request") - if domain_request_wizard: - id = domain_request_wizard.get("domain_request_id") - - # If no id is provided, we can assume that the user is starting a new request. - # If one IS provided, check that they are the original creator of it. - if id: - if not DomainRequest.objects.filter(creator=self.request.user, id=id).exists(): - return False - - return True - - class DomainInvitationPermission(PermissionsLoginMixin): """Permission mixin that redirects to domain invitation if user has access, otherwise 403" @@ -496,23 +390,6 @@ class PortfolioDomainsPermission(PortfolioBasePermission): return super().has_permission() -class PortfolioDomainRequestsPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio domain request pages if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to domain requests for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_any_requests_portfolio_permission(portfolio): - return False - - return super().has_permission() - - class PortfolioMembersPermission(PortfolioBasePermission): """Permission mixin that allows access to portfolio members pages if user has access, otherwise 403""" diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index b430845f4..41d620d42 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -2,18 +2,14 @@ import abc # abstract base class -from django.views.generic import DetailView, DeleteView, TemplateView, UpdateView -from registrar.models import Domain, DomainRequest, DomainInvitation, Portfolio +from django.views.generic import DetailView, DeleteView, UpdateView +from registrar.models import Domain, DomainInvitation, Portfolio from registrar.models.user import User from registrar.models.user_domain_role import UserDomainRole from .mixins import ( DomainPermission, - DomainRequestPermission, - DomainRequestPermissionWithdraw, DomainInvitationPermission, - DomainRequestWizardPermission, - PortfolioDomainRequestsPermission, PortfolioDomainsPermission, PortfolioMemberDomainsPermission, PortfolioMemberDomainsEditPermission, @@ -23,7 +19,6 @@ from .mixins import ( PortfolioBasePermission, PortfolioMembersPermission, PortfolioMemberPermission, - DomainRequestPortfolioViewonlyPermission, ) import logging @@ -86,77 +81,6 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): raise NotImplementedError -class DomainRequestPermissionView(DomainRequestPermission, DetailView, abc.ABC): - """Abstract base view for domain requests that enforces permissions - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = DomainRequest - # variable name in template context for the model object - context_object_name = "DomainRequest" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class DomainRequestPortfolioViewonlyView(DomainRequestPortfolioViewonlyPermission, DetailView, abc.ABC): - """Abstract base view for domain requests that enforces permissions - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = DomainRequest - # variable name in template context for the model object - context_object_name = "DomainRequest" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class DomainRequestPermissionWithdrawView(DomainRequestPermissionWithdraw, DetailView, abc.ABC): - """Abstract base view for domain request withdraw function - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = DomainRequest - # variable name in template context for the model object - context_object_name = "DomainRequest" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class DomainRequestWizardPermissionView(DomainRequestWizardPermission, TemplateView, abc.ABC): - """Abstract base view for the domain request form that enforces permissions - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - class DomainInvitationPermissionCancelView(DomainInvitationPermission, UpdateView, abc.ABC): """Abstract view for cancelling a DomainInvitation.""" @@ -164,13 +88,6 @@ class DomainInvitationPermissionCancelView(DomainInvitationPermission, UpdateVie object: DomainInvitation -class DomainRequestPermissionDeleteView(DomainRequestPermission, DeleteView, abc.ABC): - """Abstract view for deleting a DomainRequest.""" - - model = DomainRequest - object: DomainRequest - - class UserDomainRolePermissionDeleteView(UserDeleteDomainRolePermission, DeleteView, abc.ABC): """Abstract base view for deleting a UserDomainRole. @@ -242,14 +159,6 @@ class NoPortfolioDomainsPermissionView(PortfolioBasePermissionView, abc.ABC): """ -class PortfolioDomainRequestsPermissionView(PortfolioDomainRequestsPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio domain request views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - class PortfolioMembersPermissionView(PortfolioMembersPermission, PortfolioBasePermissionView, abc.ABC): """Abstract base view for portfolio members views that enforces permissions. From 40737cbcf7beb4e0d1af167e0e2898c0410eb7cb Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 11 Feb 2025 23:47:55 -0500 Subject: [PATCH 06/17] wip --- src/registrar/decorators.py | 14 ++++++++++- .../includes/domain_sidenav_item.html | 2 +- src/registrar/views/domain.py | 7 ++++-- src/registrar/views/domain_request.py | 2 +- src/registrar/views/domain_requests_json.py | 20 +++++++-------- src/registrar/views/portfolios.py | 18 ++++++++----- src/registrar/views/utility/mixins.py | 17 ------------- .../views/utility/permission_views.py | 25 ++++++------------- 8 files changed, 49 insertions(+), 56 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index cd19bcc95..fe71342cc 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -10,8 +10,10 @@ IS_STAFF = "is_staff" IS_DOMAIN_MANAGER = "is_domain_manager" IS_DOMAIN_REQUEST_CREATOR = "is_domain_request_creator" IS_STAFF_MANAGING_DOMAIN = "is_staff_managing_domain" +IS_PORTFOLIO_MEMBER = "is_portfolio_member" IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER = "is_portfolio_member_and_domain_manager" IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER = "is_domain_manager_and_not_portfolio_member" +HAS_PORTFOLIO_DOMAINS_ANY_PERM = "has_portfolio_domains_any_perm" HAS_PORTFOLIO_DOMAINS_VIEW_ALL = "has_portfolio_domains_view_all" HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM = "has_portfolio_domain_requests_any_perm" HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL = "has_portfolio_domain_requests_view_all" @@ -97,11 +99,21 @@ def _user_has_permission(user, request, rules, **kwargs): has_permission = _can_access_other_user_domains(request, domain_id) conditions_met.append(has_permission) + if not any(conditions_met) and IS_PORTFOLIO_MEMBER in rules: + has_permission = user.is_org_user(request) + conditions_met.append(has_permission) + if not any(conditions_met) and HAS_PORTFOLIO_DOMAINS_VIEW_ALL in rules: domain_id = kwargs.get("domain_pk") has_permission = _can_access_domain_via_portfolio_view_all_domains(request, domain_id) conditions_met.append(has_permission) + if not any(conditions_met) and HAS_PORTFOLIO_DOMAINS_ANY_PERM in rules: + has_permission = user.is_org_user(request) and user.has_any_domains_portfolio_permission( + request.session.get("portfolio") + ) + conditions_met.append(has_permission) + if not any(conditions_met) and IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER in rules: domain_id = kwargs.get("domain_pk") has_permission = _is_domain_manager(user, domain_id) and _is_portfolio_member(request) @@ -142,7 +154,7 @@ def _has_portfolio_domain_requests_edit(user, request, domain_request_id): if domain_request_id and not _is_domain_request_creator(user, domain_request_id): return False return user.is_org_user(request) and user.has_edit_request_portfolio_permission(request.session.get("portfolio")) - + def _is_domain_manager(user, domain_pk): """Checks to see if the user is a domain manager of the diff --git a/src/registrar/templates/includes/domain_sidenav_item.html b/src/registrar/templates/includes/domain_sidenav_item.html index 206e49c05..715f76865 100644 --- a/src/registrar/templates/includes/domain_sidenav_item.html +++ b/src/registrar/templates/includes/domain_sidenav_item.html @@ -1,6 +1,6 @@
  • {% if url_name %} - {% url url_name pk=domain.id as url %} + {% url url_name domain_pk=domain.id as url %} {% endif %} Date: Wed, 12 Feb 2025 08:16:16 -0500 Subject: [PATCH 07/17] additional cleanup of permission views and mixins, handling of domain invitations and user domain roles in decorators --- src/registrar/config/urls.py | 2 +- src/registrar/decorators.py | 72 +++++--- src/registrar/templates/domain_users.html | 2 +- src/registrar/views/domain.py | 154 ++++++++++++++++-- src/registrar/views/utility/__init__.py | 2 - src/registrar/views/utility/mixins.py | 153 ----------------- .../views/utility/permission_views.py | 94 +---------- 7 files changed, 196 insertions(+), 283 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 665f2cd82..12efe5a9f 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -364,7 +364,7 @@ urlpatterns = [ name="user-profile", ), path( - "invitation//cancel", + "invitation//cancel", views.DomainInvitationCancelView.as_view(http_method_names=["post"]), name="invitation-cancel", ), diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index fe71342cc..237985f02 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -1,7 +1,7 @@ import functools from django.core.exceptions import PermissionDenied from django.utils.decorators import method_decorator -from registrar.models import Domain, DomainInformation, DomainRequest, UserDomainRole +from registrar.models import Domain, DomainInformation, DomainInvitation, DomainRequest, UserDomainRole # Constants for clarity ALL = "all" @@ -18,7 +18,6 @@ HAS_PORTFOLIO_DOMAINS_VIEW_ALL = "has_portfolio_domains_view_all" 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_DOMAINS_VIEW_MANAGED = "has_portfolio_domains_view_managed" def grant_access(*rules): @@ -90,13 +89,11 @@ def _user_has_permission(user, request, rules, **kwargs): conditions_met.append(user.is_superuser) if not any(conditions_met) and IS_DOMAIN_MANAGER in rules: - domain_id = kwargs.get("domain_pk") - has_permission = _is_domain_manager(user, domain_id) + has_permission = _is_domain_manager(user, **kwargs) conditions_met.append(has_permission) if not any(conditions_met) and IS_STAFF_MANAGING_DOMAIN in rules: - domain_id = kwargs.get("domain_pk") - has_permission = _can_access_other_user_domains(request, domain_id) + has_permission = _is_staff_managing_domain(request, **kwargs) conditions_met.append(has_permission) if not any(conditions_met) and IS_PORTFOLIO_MEMBER in rules: @@ -115,13 +112,11 @@ def _user_has_permission(user, request, rules, **kwargs): conditions_met.append(has_permission) if not any(conditions_met) and IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER in rules: - domain_id = kwargs.get("domain_pk") - has_permission = _is_domain_manager(user, domain_id) and _is_portfolio_member(request) + has_permission = _is_domain_manager(user, **kwargs) and _is_portfolio_member(request) conditions_met.append(has_permission) if not any(conditions_met) and IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER in rules: - domain_id = kwargs.get("domain_pk") - has_permission = _is_domain_manager(user, domain_id) and not _is_portfolio_member(request) + has_permission = _is_domain_manager(user, **kwargs) and not _is_portfolio_member(request) conditions_met.append(has_permission) if not any(conditions_met) and IS_DOMAIN_REQUEST_CREATOR in rules: @@ -156,10 +151,24 @@ def _has_portfolio_domain_requests_edit(user, request, domain_request_id): return user.is_org_user(request) and user.has_edit_request_portfolio_permission(request.session.get("portfolio")) -def _is_domain_manager(user, domain_pk): - """Checks to see if the user is a domain manager of the - domain with domain_pk.""" - return UserDomainRole.objects.filter(user=user, domain_id=domain_pk).exists() +def _is_domain_manager(user, **kwargs): + """ + Determines if the given user is a domain manager for a specified domain. + + - First, it checks if 'domain_pk' is present in the URL parameters. + - If 'domain_pk' exists, it verifies if the user has a domain role for that domain. + - If 'domain_pk' is absent, it checks for 'domain_invitation_pk' to determine if the user has domain permissions through an invitation. + + Returns: + bool: True if the user is a domain manager, False otherwise. + """ + domain_id = kwargs.get("domain_pk") + if domain_id: + return UserDomainRole.objects.filter(user=user, domain_id=domain_id).exists() + domain_invitation_id = kwargs.get("domain_invitation_pk") + if domain_invitation_id: + return DomainInvitation.objects.filter(id=domain_invitation_id, domain__permissions__user=user).exists() + return False def _is_domain_request_creator(user, domain_request_pk): @@ -176,10 +185,35 @@ def _is_portfolio_member(request): return request.user.is_org_user(request) -def _can_access_other_user_domains(request, domain_pk): - """Checks to see if an authorized user (staff or superuser) - can access a domain that they did not create or were invited to. +def _is_staff_managing_domain(request, **kwargs): """ + Determines whether a staff user (analyst or superuser) has permission to manage a domain + that they did not create or were not invited to. + + The function enforces: + 1. **User Authorization** - The user must have `analyst_access_permission` or `full_access_permission`. + 2. **Valid Session Context** - The user must have explicitly selected the domain for management + via an 'analyst action' (e.g., by clicking 'Manage Domain' in the admin interface). + 3. **Domain Status Check** - Only domains in specific statuses (e.g., APPROVED, IN_REVIEW, etc.) + can be managed, except in cases where the domain lacks a status due to errors. + + Process: + - First, the function retrieves the `domain_pk` from the URL parameters. + - If `domain_pk` is not provided, it attempts to resolve the domain via `domain_invitation_pk`. + - It checks if the user has the required permissions. + - It verifies that the user has an active 'analyst action' session for the domain. + - Finally, it ensures that the domain is in a status that allows management. + + Returns: + bool: True if the user is allowed to manage the domain, False otherwise. + """ + + domain_id = kwargs.get("domain_pk") + if not domain_id: + domain_invitation_id = kwargs.get("domain_invitation_pk") + domain_invitation = DomainInvitation.objects.filter(id=domain_invitation_id).first() + if domain_invitation: + domain_id = domain_invitation.domain_id # Check if the request user is permissioned... user_is_analyst_or_superuser = request.user.has_perm( @@ -197,7 +231,7 @@ def _can_access_other_user_domains(request, domain_pk): can_do_action = ( "analyst_action" in session and "analyst_action_location" in session - and session["analyst_action_location"] == domain_pk + and session["analyst_action_location"] == domain_id ) if not can_do_action: @@ -215,7 +249,7 @@ def _can_access_other_user_domains(request, domain_pk): None, ] - requested_domain = DomainInformation.objects.filter(domain_id=domain_pk).first() + requested_domain = DomainInformation.objects.filter(domain_id=domain_id).first() # if no domain information or domain request exist, the user # should be able to manage the domain; however, if domain information diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index 984b4160b..4c5dbc728 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -154,7 +154,7 @@ {% if not portfolio %}{{ invitation.domain_invitation.status|title }}{% endif %} {% if invitation.domain_invitation.status == invitation.domain_invitation.DomainInvitationStatus.INVITED %} -
    + {% csrf_token %}
    {% endif %} diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 31f715d18..7a76284a4 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1,10 +1,3 @@ -"""Views for a single Domain. - -Authorization is handled by the `DomainPermissionView`. To ensure that only -authorized users can see information on a domain, every view here should -inherit from `DomainPermissionView` (or DomainInvitationPermissionCancelView). -""" - from datetime import date import logging import requests @@ -13,7 +6,7 @@ from django.contrib.messages.views import SuccessMessageMixin from django.http import HttpResponseRedirect from django.shortcuts import redirect, render, get_object_or_404 from django.urls import reverse -from django.views.generic import DeleteView +from django.views.generic import DeleteView, DetailView, UpdateView from django.views.generic.edit import FormMixin from django.conf import settings from registrar.decorators import ( @@ -49,7 +42,6 @@ from registrar.utility.errors import ( SecurityEmailErrorCodes, ) from registrar.models.utility.contact_error import ContactError -from registrar.views.utility.permission_views import UserDomainRolePermissionDeleteView from registrar.utility.waffle import flag_is_active_for_user from registrar.views.utility.invitation_helper import ( get_org_membership, @@ -76,19 +68,22 @@ from epplibwrapper import ( from ..utility.email import send_templated_email, EmailSendingError from ..utility.email_invitations import send_domain_invitation_email, send_portfolio_invitation_email -from .utility import DomainPermissionView, DomainInvitationPermissionCancelView from django import forms logger = logging.getLogger(__name__) -class DomainBaseView(DomainPermissionView): +class DomainBaseView(DetailView): """ Base View for the Domain. Handles getting and setting the domain in session cache on GETs. Also provides methods for getting and setting the domain in cache """ + model = Domain + pk_url_kwarg = "domain_pk" + context_object_name = "domain" + def get(self, request, *args, **kwargs): self._get_domain(request) context = self.get_context_data(object=self.object) @@ -120,6 +115,134 @@ class DomainBaseView(DomainPermissionView): domain_pk = "domain:" + str(self.kwargs.get("domain_pk")) self.session[domain_pk] = self.object + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + user = self.request.user + context["is_analyst_or_superuser"] = user.has_perm("registrar.analyst_access_permission") or user.has_perm( + "registrar.full_access_permission" + ) + context["is_domain_manager"] = UserDomainRole.objects.filter(user=user, domain=self.object).exists() + context["is_portfolio_user"] = self.can_access_domain_via_portfolio(self.object.pk) + context["is_editable"] = self.is_editable() + # Stored in a variable for the linter + action = "analyst_action" + action_location = "analyst_action_location" + # Flag to see if an analyst is attempting to make edits + if action in self.request.session: + context[action] = self.request.session[action] + if action_location in self.request.session: + context[action_location] = self.request.session[action_location] + + return context + + def is_editable(self): + """Returns whether domain is editable in the context of the view""" + domain_editable = self.object.is_editable() + if not domain_editable: + return False + + # if user is domain manager or analyst or admin, return True + if ( + self.can_access_other_user_domains(self.object.id) + or UserDomainRole.objects.filter(user=self.request.user, domain=self.object).exists() + ): + return True + + return False + + def can_access_domain_via_portfolio(self, pk): + """Most views should not allow permission to portfolio users. + If particular views allow access to the domain pages, they will need to override + this function. + """ + return False + + def has_permission(self): + """Check if this user has access to this domain. + + The user is in self.request.user and the domain needs to be looked + up from the domain's primary key in self.kwargs["domain_pk"] + """ + pk = self.kwargs["domain_pk"] + + # test if domain in editable state + if not self.in_editable_state(pk): + return False + + # if we need to check more about the nature of role, do it here. + return True + + def in_editable_state(self, pk): + """Is the domain in an editable state""" + + requested_domain = None + if Domain.objects.filter(id=pk).exists(): + requested_domain = Domain.objects.get(id=pk) + + # if domain is editable return true + if requested_domain and requested_domain.is_editable(): + return True + return False + + def can_access_other_user_domains(self, pk): + """Checks to see if an authorized user (staff or superuser) + can access a domain that they did not create or was invited to. + """ + + # Check if the user is permissioned... + user_is_analyst_or_superuser = self.request.user.has_perm( + "registrar.analyst_access_permission" + ) or self.request.user.has_perm("registrar.full_access_permission") + + if not user_is_analyst_or_superuser: + return False + + # Check if the user is attempting a valid edit action. + # In other words, if the analyst/admin did not click + # the 'Manage Domain' button in /admin, + # then they cannot access this page. + session = self.request.session + can_do_action = ( + "analyst_action" in session + and "analyst_action_location" in session + and session["analyst_action_location"] == pk + ) + + if not can_do_action: + return False + + # Analysts may manage domains, when they are in these statuses: + valid_domain_statuses = [ + DomainRequest.DomainRequestStatus.APPROVED, + DomainRequest.DomainRequestStatus.IN_REVIEW, + DomainRequest.DomainRequestStatus.REJECTED, + DomainRequest.DomainRequestStatus.ACTION_NEEDED, + # Edge case - some domains do not have + # a status or DomainInformation... aka a status of 'None'. + # It is necessary to access those to correct errors. + None, + ] + + requested_domain = None + if DomainInformation.objects.filter(id=pk).exists(): + requested_domain = DomainInformation.objects.get(id=pk) + + # if no domain information or domain request exist, the user + # should be able to manage the domain; however, if domain information + # and domain request exist, and domain request is not in valid status, + # user should not be able to manage domain + if ( + requested_domain + and requested_domain.domain_request + and requested_domain.domain_request.status not in valid_domain_statuses + ): + return False + + # Valid session keys exist, + # the user is permissioned, + # and it is in a valid status + return True + class DomainFormBaseView(DomainBaseView, FormMixin): """ @@ -433,7 +556,7 @@ class DomainOrgNameAddressView(DomainFormBaseView): return super().has_permission() -@grant_access(IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER) +@grant_access(IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER, IS_STAFF_MANAGING_DOMAIN) class DomainSubOrganizationView(DomainFormBaseView): """Suborganization view""" @@ -480,7 +603,7 @@ class DomainSubOrganizationView(DomainFormBaseView): return super().form_valid(form) -@grant_access(IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER) +@grant_access(IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER, IS_STAFF_MANAGING_DOMAIN) class DomainSeniorOfficialView(DomainFormBaseView): """Domain senior official editing view.""" @@ -1307,8 +1430,9 @@ class DomainAddUserView(DomainFormBaseView): @grant_access(IS_DOMAIN_MANAGER, IS_STAFF_MANAGING_DOMAIN) -class DomainInvitationCancelView(SuccessMessageMixin, DomainInvitationPermissionCancelView): - object: DomainInvitation +class DomainInvitationCancelView(SuccessMessageMixin, UpdateView): + model = DomainInvitation + pk_url_kwarg = "domain_invitation_pk" fields = [] def post(self, request, *args, **kwargs): diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index 4a33c9944..f80774ef3 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -2,8 +2,6 @@ from .steps_helper import StepsHelper from .always_404 import always_404 from .permission_views import ( - DomainPermissionView, PortfolioMembersPermission, - DomainInvitationPermissionCancelView, ) from .api_views import get_senior_official_from_federal_agency_json diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index ad0794edd..23895cb5c 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -1,14 +1,6 @@ """Permissions-related mixin classes.""" from django.contrib.auth.mixins import PermissionRequiredMixin - -from registrar.models import ( - Domain, - DomainRequest, - DomainInvitation, - DomainInformation, - UserDomainRole, -) import logging @@ -195,151 +187,6 @@ class PortfolioReportsPermission(PermissionsLoginMixin): return self.request.user.is_org_user(self.request) -class DomainPermission(PermissionsLoginMixin): - """Permission mixin that redirects to domain if user has access, - otherwise 403""" - - def has_permission(self): - """Check if this user has access to this domain. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["domain_pk"] - """ - pk = self.kwargs["domain_pk"] - - # test if domain in editable state - if not self.in_editable_state(pk): - return False - - # if we need to check more about the nature of role, do it here. - return True - - def in_editable_state(self, pk): - """Is the domain in an editable state""" - - requested_domain = None - if Domain.objects.filter(id=pk).exists(): - requested_domain = Domain.objects.get(id=pk) - - # if domain is editable return true - if requested_domain and requested_domain.is_editable(): - return True - return False - - def can_access_other_user_domains(self, pk): - """Checks to see if an authorized user (staff or superuser) - can access a domain that they did not create or was invited to. - """ - - # Check if the user is permissioned... - user_is_analyst_or_superuser = self.request.user.has_perm( - "registrar.analyst_access_permission" - ) or self.request.user.has_perm("registrar.full_access_permission") - - if not user_is_analyst_or_superuser: - return False - - # Check if the user is attempting a valid edit action. - # In other words, if the analyst/admin did not click - # the 'Manage Domain' button in /admin, - # then they cannot access this page. - session = self.request.session - can_do_action = ( - "analyst_action" in session - and "analyst_action_location" in session - and session["analyst_action_location"] == pk - ) - - if not can_do_action: - return False - - # Analysts may manage domains, when they are in these statuses: - valid_domain_statuses = [ - DomainRequest.DomainRequestStatus.APPROVED, - DomainRequest.DomainRequestStatus.IN_REVIEW, - DomainRequest.DomainRequestStatus.REJECTED, - DomainRequest.DomainRequestStatus.ACTION_NEEDED, - # Edge case - some domains do not have - # a status or DomainInformation... aka a status of 'None'. - # It is necessary to access those to correct errors. - None, - ] - - requested_domain = None - if DomainInformation.objects.filter(id=pk).exists(): - requested_domain = DomainInformation.objects.get(id=pk) - - # if no domain information or domain request exist, the user - # should be able to manage the domain; however, if domain information - # and domain request exist, and domain request is not in valid status, - # user should not be able to manage domain - if ( - requested_domain - and requested_domain.domain_request - and requested_domain.domain_request.status not in valid_domain_statuses - ): - return False - - # Valid session keys exist, - # the user is permissioned, - # and it is in a valid status - return True - - -class UserDeleteDomainRolePermission(PermissionsLoginMixin): - """Permission mixin for UserDomainRole if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to this domain request. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - domain_pk = self.kwargs["pk"] - user_pk = self.kwargs["user_pk"] - - if not self.request.user.is_authenticated: - return False - - # Check if the UserDomainRole object exists, then check - # if the user requesting the delete has permissions to do so - has_delete_permission = UserDomainRole.objects.filter( - user=user_pk, - domain=domain_pk, - domain__permissions__user=self.request.user, - ).exists() - - user_is_analyst_or_superuser = self.request.user.has_perm( - "registrar.analyst_access_permission" - ) or self.request.user.has_perm("registrar.full_access_permission") - - if not (has_delete_permission or user_is_analyst_or_superuser): - return False - - return True - - -class DomainInvitationPermission(PermissionsLoginMixin): - """Permission mixin that redirects to domain invitation if user has - access, otherwise 403" - - A user has access to a domain invitation if they have a role on the - associated domain. - """ - - def has_permission(self): - """Check if this user has a role on the domain of this invitation.""" - if not self.request.user.is_authenticated: - return False - - if not DomainInvitation.objects.filter( - id=self.kwargs["pk"], domain__permissions__user=self.request.user - ).exists(): - return False - return True - - class UserProfilePermission(PermissionsLoginMixin): """Permission mixin that redirects to user profile if user has access, otherwise 403""" diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index b2b56666f..02a2b029b 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -2,18 +2,14 @@ import abc # abstract base class -from django.views.generic import DetailView, DeleteView, UpdateView -from registrar.models import Domain, DomainInvitation, Portfolio +from django.views.generic import DetailView +from registrar.models import Portfolio from registrar.models.user import User -from registrar.models.user_domain_role import UserDomainRole from .mixins import ( - DomainPermission, - DomainInvitationPermission, PortfolioMemberDomainsPermission, PortfolioMemberDomainsEditPermission, PortfolioMemberEditPermission, - UserDeleteDomainRolePermission, UserProfilePermission, PortfolioBasePermission, PortfolioMembersPermission, @@ -24,92 +20,6 @@ import logging logger = logging.getLogger(__name__) -class DomainPermissionView(DomainPermission, DetailView, abc.ABC): - """Abstract base view for domains that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = Domain - pk_url_kwarg = "domain_pk" - # variable name in template context for the model object - context_object_name = "domain" - - # Adds context information for user permissions - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - user = self.request.user - context["is_analyst_or_superuser"] = user.has_perm("registrar.analyst_access_permission") or user.has_perm( - "registrar.full_access_permission" - ) - context["is_domain_manager"] = UserDomainRole.objects.filter(user=user, domain=self.object).exists() - context["is_portfolio_user"] = self.can_access_domain_via_portfolio(self.object.pk) - context["is_editable"] = self.is_editable() - # Stored in a variable for the linter - action = "analyst_action" - action_location = "analyst_action_location" - # Flag to see if an analyst is attempting to make edits - if action in self.request.session: - context[action] = self.request.session[action] - if action_location in self.request.session: - context[action_location] = self.request.session[action_location] - - return context - - def is_editable(self): - """Returns whether domain is editable in the context of the view""" - domain_editable = self.object.is_editable() - if not domain_editable: - return False - - # if user is domain manager or analyst or admin, return True - if ( - self.can_access_other_user_domains(self.object.id) - or UserDomainRole.objects.filter(user=self.request.user, domain=self.object).exists() - ): - return True - - return False - - def can_access_domain_via_portfolio(self, pk): - """Most views should not allow permission to portfolio users. - If particular views allow access to the domain pages, they will need to override - this function. - """ - return False - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class DomainInvitationPermissionCancelView(DomainInvitationPermission, UpdateView, abc.ABC): - """Abstract view for cancelling a DomainInvitation.""" - - model = DomainInvitation - object: DomainInvitation - - -class UserDomainRolePermissionDeleteView(UserDeleteDomainRolePermission, DeleteView, abc.ABC): - """Abstract base view for deleting a UserDomainRole. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = UserDomainRole - # workaround for type mismatch in DeleteView - object: UserDomainRole - - # variable name in template context for the model object - context_object_name = "userdomainrole" - - class UserProfilePermissionView(UserProfilePermission, DetailView, abc.ABC): """Abstract base view for user profile view that enforces permissions. From 908e71e3ae1d6de82cb42ce6e2282c9d44937178 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 09:53:55 -0500 Subject: [PATCH 08/17] portfolio member permissions --- src/registrar/decorators.py | 18 +++ src/registrar/views/member_domains_json.py | 27 ++--- src/registrar/views/portfolio_members_json.py | 7 +- src/registrar/views/portfolios.py | 89 +++++++++------ src/registrar/views/utility/__init__.py | 3 - src/registrar/views/utility/mixins.py | 107 ------------------ .../views/utility/permission_views.py | 67 ----------- 7 files changed, 90 insertions(+), 228 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 237985f02..98a37c62c 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -18,6 +18,8 @@ HAS_PORTFOLIO_DOMAINS_VIEW_ALL = "has_portfolio_domains_view_all" 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_EDIT = "has_portfolio_members_edit" def grant_access(*rules): @@ -142,6 +144,22 @@ def _user_has_permission(user, request, rules, **kwargs): print(has_permission) conditions_met.append(has_permission) + if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_ANY_PERM in rules: + portfolio = request.session.get("portfolio") + has_permission = user.is_org_user(request) and ( + user.has_view_members_portfolio_permission(portfolio) or + user.has_edit_members_portfolio_permission(portfolio) + ) + conditions_met.append(has_permission) + + if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_EDIT in rules: + portfolio = request.session.get("portfolio") + has_permission = ( + user.is_org_user(request) and + user.has_edit_members_portfolio_permission(portfolio) + ) + conditions_met.append(has_permission) + return any(conditions_met) diff --git a/src/registrar/views/member_domains_json.py b/src/registrar/views/member_domains_json.py index 3d24336bb..40b7bea74 100644 --- a/src/registrar/views/member_domains_json.py +++ b/src/registrar/views/member_domains_json.py @@ -4,37 +4,38 @@ from django.http import JsonResponse from django.core.paginator import Paginator from django.shortcuts import get_object_or_404 from django.views import View +from registrar.decorators import HAS_PORTFOLIO_MEMBERS_ANY_PERM, grant_access from registrar.models import UserDomainRole, Domain, DomainInformation, User from django.urls import reverse from django.db.models import Q from registrar.models.domain_invitation import DomainInvitation -from registrar.views.utility.mixins import PortfolioMemberDomainsPermission logger = logging.getLogger(__name__) -class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMemberDomainsJson(View): def get(self, request): """Given the current request, get all domains that are associated with the portfolio, or associated with the member/invited member""" - domain_ids = self.get_domain_ids_from_request(request) + domain_ids = self._get_domain_ids_from_request(request) objects = Domain.objects.filter(id__in=domain_ids).select_related("domain_info__sub_organization") unfiltered_total = objects.count() - objects = self.apply_search(objects, request) - objects = self.apply_sorting(objects, request) + objects = self._apply_search(objects, request) + objects = self._apply_sorting(objects, request) - paginator = Paginator(objects, self.get_page_size(request)) + paginator = Paginator(objects, self._get_page_size(request)) page_number = request.GET.get("page") page_obj = paginator.get_page(page_number) member_id = request.GET.get("member_id") - domains = [self.serialize_domain(domain, member_id, request.user) for domain in page_obj.object_list] + domains = [self._serialize_domain(domain, member_id, request.user) for domain in page_obj.object_list] return JsonResponse( { @@ -48,7 +49,7 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): } ) - def get_page_size(self, request): + def _get_page_size(self, request): """Gets the page size. If member_only, need to return the entire result set every time, so need @@ -65,7 +66,7 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): # later return 1000 - def get_domain_ids_from_request(self, request): + def _get_domain_ids_from_request(self, request): """Get domain ids from request. request.get.email - email address of invited member @@ -100,13 +101,13 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): logger.warning("Invalid search criteria, returning empty results list") return [] - def apply_search(self, queryset, request): + def _apply_search(self, queryset, request): search_term = request.GET.get("search_term") if search_term: queryset = queryset.filter(Q(name__icontains=search_term)) return queryset - def apply_sorting(self, queryset, request): + def _apply_sorting(self, queryset, request): # Get the sorting parameters from the request sort_by = request.GET.get("sort_by", "name") order = request.GET.get("order", "asc") @@ -141,7 +142,7 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): return queryset - def serialize_domain(self, domain, member_id, user): + def _serialize_domain(self, domain, member_id, user): suborganization_name = None try: domain_info = domain.domain_info @@ -176,7 +177,7 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): "state": domain.state, "state_display": domain.state_display(), "get_state_help_text": domain.get_state_help_text(), - "action_url": reverse("domain", kwargs={"pk": domain.id}), + "action_url": reverse("domain", kwargs={"domain_pk": domain.id}), "action_label": ("View" if view_only else "Manage"), "svg_icon": ("visibility" if view_only else "settings"), "domain_info__sub_organization": suborganization_name, diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 29dc6a71c..ecdd24441 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -6,16 +6,17 @@ from django.contrib.postgres.aggregates import ArrayAgg from django.urls import reverse from django.views import View +from registrar.decorators import HAS_PORTFOLIO_MEMBERS_ANY_PERM, grant_access from registrar.models.domain_invitation import DomainInvitation from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from registrar.views.utility.mixins import PortfolioMembersPermission from registrar.models.utility.orm_helper import ArrayRemoveNull from django.contrib.postgres.aggregates import StringAgg -class PortfolioMembersJson(PortfolioMembersPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMembersJson(View): def get(self, request): """Fetch members (permissions and invitations) for the given portfolio.""" @@ -236,7 +237,7 @@ class PortfolioMembersJson(PortfolioMembersPermission, View): ), # split domain_info array values into ids to form urls, and names "domain_urls": [ - reverse("domain", kwargs={"pk": domain_info.split(":")[0]}) for domain_info in domain_info_list + reverse("domain", kwargs={"domain_pk": domain_info.split(":")[0]}) for domain_info in domain_info_list ], "domain_names": [domain_info.split(":")[1] for domain_info in domain_info_list], "is_admin": is_admin, diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index dc1357585..f0c4841f1 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -5,20 +5,26 @@ from django.http import Http404, JsonResponse from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse from django.utils.safestring import mark_safe +from django.views.generic import DetailView from django.contrib import messages from registrar.decorators import ( HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM, HAS_PORTFOLIO_DOMAINS_ANY_PERM, + HAS_PORTFOLIO_MEMBERS_ANY_PERM, + HAS_PORTFOLIO_MEMBERS_EDIT, IS_PORTFOLIO_MEMBER, grant_access, ) from registrar.forms import portfolio as portfolioForms -from registrar.models import Portfolio, User -from registrar.models.domain import Domain -from registrar.models.domain_invitation import DomainInvitation -from registrar.models.portfolio_invitation import PortfolioInvitation -from registrar.models.user_domain_role import UserDomainRole -from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models import ( + Domain, + DomainInvitation, + Portfolio, + PortfolioInvitation, + User, + UserDomainRole, + UserPortfolioPermission +) from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError from registrar.utility.email_invitations import ( @@ -29,15 +35,6 @@ from registrar.utility.email_invitations import ( ) from registrar.utility.errors import MissingEmailError from registrar.utility.enums import DefaultUserValues -from registrar.views.utility.mixins import PortfolioMemberPermission -from registrar.views.utility.permission_views import ( - PortfolioBasePermissionView, - PortfolioMemberDomainsPermissionView, - PortfolioMemberDomainsEditPermissionView, - PortfolioMemberEditPermissionView, - PortfolioMemberPermissionView, - PortfolioMembersPermissionView, -) from django.views.generic import View from django.views.generic.edit import FormMixin from django.db import IntegrityError @@ -71,8 +68,10 @@ class PortfolioDomainRequestsView(View): return render(request, "portfolio_requests.html") -class PortfolioMemberView(PortfolioMemberPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMemberView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member.html" def get(self, request, pk): @@ -113,7 +112,8 @@ class PortfolioMemberView(PortfolioMemberPermissionView, View): ) -class PortfolioMemberDeleteView(PortfolioMemberPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMemberDeleteView(View): def post(self, request, pk): """ @@ -190,8 +190,10 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) +class PortfolioMemberEditView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member_permissions.html" form_class = portfolioForms.PortfolioMemberForm @@ -265,7 +267,8 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -class PortfolioMemberDomainsView(PortfolioMemberDomainsPermissionView, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMemberDomainsView(View): template_name = "portfolio_member_domains.html" @@ -283,8 +286,10 @@ class PortfolioMemberDomainsView(PortfolioMemberDomainsPermissionView, View): ) -class PortfolioMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) +class PortfolioMemberDomainsEditView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member_domains_edit.html" def get(self, request, pk): @@ -393,8 +398,10 @@ class PortfolioMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, V UserDomainRole.objects.filter(domain_id__in=removed_domain_ids, user=member).delete() -class PortfolioInvitedMemberView(PortfolioMemberPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioInvitedMemberView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member.html" # form_class = PortfolioInvitedMemberForm @@ -435,7 +442,8 @@ class PortfolioInvitedMemberView(PortfolioMemberPermissionView, View): ) -class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioInvitedMemberDeleteView(View): def post(self, request, pk): """ @@ -478,8 +486,10 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): - +@grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) +class PortfolioInvitedMemberEditView(DetailView, View): + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member_permissions.html" form_class = portfolioForms.PortfolioInvitedMemberForm @@ -547,7 +557,8 @@ class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): messages.warning(self.request, "Could not send email notification to existing organization admins.") -class PortfolioInvitedMemberDomainsView(PortfolioMemberDomainsPermissionView, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioInvitedMemberDomainsView(View): template_name = "portfolio_member_domains.html" @@ -562,9 +573,11 @@ class PortfolioInvitedMemberDomainsView(PortfolioMemberDomainsPermissionView, Vi }, ) +@grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) +class PortfolioInvitedMemberDomainsEditView(DetailView, View): -class PortfolioInvitedMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, View): - + model = Portfolio + context_object_name = "portfolio" template_name = "portfolio_member_domains_edit.html" def get(self, request, pk): @@ -749,7 +762,8 @@ class PortfolioNoDomainRequestsView(View): return context -class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): +@grant_access(IS_PORTFOLIO_MEMBER) +class PortfolioOrganizationView(DetailView, FormMixin): """ View to handle displaying and updating the portfolio's organization details. """ @@ -811,7 +825,8 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): return reverse("organization") -class PortfolioSeniorOfficialView(PortfolioBasePermissionView, FormMixin): +@grant_access(IS_PORTFOLIO_MEMBER) +class PortfolioSeniorOfficialView(DetailView, FormMixin): """ View to handle displaying and updating the portfolio's senior official details. For now, this view is readonly. @@ -842,7 +857,8 @@ class PortfolioSeniorOfficialView(PortfolioBasePermissionView, FormMixin): return self.render_to_response(self.get_context_data(form=form)) -class PortfolioMembersView(PortfolioMembersPermissionView, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioMembersView(View): template_name = "portfolio_members.html" @@ -851,10 +867,13 @@ class PortfolioMembersView(PortfolioMembersPermissionView, View): return render(request, "portfolio_members.html") -class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): +@grant_access(HAS_PORTFOLIO_MEMBERS_ANY_PERM) +class PortfolioAddMemberView(DetailView, FormMixin): template_name = "portfolio_members_add_new.html" form_class = portfolioForms.PortfolioNewMemberForm + model = Portfolio + context_object_name = "portfolio" def get(self, request, *args, **kwargs): """Handle GET requests to display the form.""" diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index f80774ef3..12fcc325d 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -1,7 +1,4 @@ from .steps_helper import StepsHelper from .always_404 import always_404 -from .permission_views import ( - PortfolioMembersPermission, -) from .api_views import get_senior_official_from_federal_agency_json diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 23895cb5c..1762d4900 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -202,110 +202,3 @@ class UserProfilePermission(PermissionsLoginMixin): return False return True - - -class PortfolioBasePermission(PermissionsLoginMixin): - """Permission mixin that redirects to portfolio pages if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"] - """ - if not self.request.user.is_authenticated: - return False - - return self.request.user.is_org_user(self.request) - - -class PortfolioMembersPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio members pages if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to members for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ) and not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() - - -class PortfolioMemberPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio member or invited member pages if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access to members or invited members for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ) and not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() - - -class PortfolioMemberEditPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio member or invited member pages if user - has access to edit, otherwise 403""" - - def has_permission(self): - """Check if this user has access to members or invited members for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() - - -class PortfolioMemberDomainsPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio member or invited member domains pages if user - has access to edit, otherwise 403""" - - def has_permission(self): - """Check if this user has access to member or invited member domains for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ) and not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() - - -class PortfolioMemberDomainsEditPermission(PortfolioBasePermission): - """Permission mixin that allows access to portfolio member or invited member domains edit pages if user - has access to edit, otherwise 403""" - - def has_permission(self): - """Check if this user has access to member or invited member domains for this portfolio. - - The user is in self.request.user and the portfolio can be looked - up from the portfolio's primary key in self.kwargs["pk"]""" - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_edit_members_portfolio_permission(portfolio): - return False - - return super().has_permission() diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 02a2b029b..1961e1cdd 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -7,13 +7,7 @@ from registrar.models import Portfolio from registrar.models.user import User from .mixins import ( - PortfolioMemberDomainsPermission, - PortfolioMemberDomainsEditPermission, - PortfolioMemberEditPermission, UserProfilePermission, - PortfolioBasePermission, - PortfolioMembersPermission, - PortfolioMemberPermission, ) import logging @@ -37,64 +31,3 @@ class UserProfilePermissionView(UserProfilePermission, DetailView, abc.ABC): @abc.abstractmethod def template_name(self): raise NotImplementedError - - -class PortfolioBasePermissionView(PortfolioBasePermission, DetailView, abc.ABC): - """Abstract base view for portfolio views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = Portfolio - # variable name in template context for the model object - context_object_name = "portfolio" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError - - -class PortfolioMembersPermissionView(PortfolioMembersPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio members views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - -class PortfolioMemberPermissionView(PortfolioMemberPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio member views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - -class PortfolioMemberEditPermissionView(PortfolioMemberEditPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio member edit views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - -class PortfolioMemberDomainsPermissionView(PortfolioMemberDomainsPermission, PortfolioBasePermissionView, abc.ABC): - """Abstract base view for portfolio member domains views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - -class PortfolioMemberDomainsEditPermissionView( - PortfolioMemberDomainsEditPermission, PortfolioBasePermissionView, abc.ABC -): - """Abstract base view for portfolio member domains edit views that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ From 4daac2bec03e452176c984f7ef4015609faf0a37 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 10:07:51 -0500 Subject: [PATCH 09/17] user profile permissions --- src/registrar/views/user_profile.py | 7 ++-- src/registrar/views/utility/mixins.py | 17 ---------- .../views/utility/permission_views.py | 33 ------------------- 3 files changed, 5 insertions(+), 52 deletions(-) delete mode 100644 src/registrar/views/utility/permission_views.py diff --git a/src/registrar/views/user_profile.py b/src/registrar/views/user_profile.py index 2012d12ab..3434eedb3 100644 --- a/src/registrar/views/user_profile.py +++ b/src/registrar/views/user_profile.py @@ -5,22 +5,25 @@ import logging from django.contrib import messages from django.http import QueryDict +from django.views.generic import DetailView from django.views.generic.edit import FormMixin +from registrar.decorators import ALL, grant_access from registrar.forms.user_profile import UserProfileForm, FinishSetupProfileForm from django.urls import NoReverseMatch, reverse from registrar.models.user import User from registrar.models.utility.generic_helper import replace_url_queryparams -from registrar.views.utility.permission_views import UserProfilePermissionView logger = logging.getLogger(__name__) -class UserProfileView(UserProfilePermissionView, FormMixin): +@grant_access(ALL) +class UserProfileView(DetailView, FormMixin): """ Base View for the User Profile. Handles getting and setting the User Profile """ model = User + context_object_name = "user" template_name = "profile.html" form_class = UserProfileForm base_view_name = "user-profile" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 1762d4900..d3ec95af5 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -185,20 +185,3 @@ class PortfolioReportsPermission(PermissionsLoginMixin): return False return self.request.user.is_org_user(self.request) - - -class UserProfilePermission(PermissionsLoginMixin): - """Permission mixin that redirects to user profile if user - has access, otherwise 403""" - - def has_permission(self): - """Check if this user has access. - - If the user is authenticated, they have access - """ - - # Check if the user is authenticated - if not self.request.user.is_authenticated: - return False - - return True diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py deleted file mode 100644 index 1961e1cdd..000000000 --- a/src/registrar/views/utility/permission_views.py +++ /dev/null @@ -1,33 +0,0 @@ -"""View classes that enforce authorization.""" - -import abc # abstract base class - -from django.views.generic import DetailView -from registrar.models import Portfolio -from registrar.models.user import User - -from .mixins import ( - UserProfilePermission, -) -import logging - -logger = logging.getLogger(__name__) - - -class UserProfilePermissionView(UserProfilePermission, DetailView, abc.ABC): - """Abstract base view for user profile view that enforces permissions. - - This abstract view cannot be instantiated. Actual views must specify - `template_name`. - """ - - # DetailView property for what model this is viewing - model = User - # variable name in template context for the model object - context_object_name = "user" - - # Abstract property enforces NotImplementedError on an attribute. - @property - @abc.abstractmethod - def template_name(self): - raise NotImplementedError From 9ad7b7523b242cd982a973da5567807aa02e88c1 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 10:46:06 -0500 Subject: [PATCH 10/17] analytics, reports and transfer user --- src/registrar/decorators.py | 9 +++++ src/registrar/views/report_views.py | 28 +++++++------- src/registrar/views/transfer_user.py | 2 + src/registrar/views/utility/mixins.py | 54 +-------------------------- 4 files changed, 26 insertions(+), 67 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 98a37c62c..0f2c948b3 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -20,6 +20,7 @@ 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_EDIT = "has_portfolio_members_edit" +HAS_PORTFOLIO_MEMBERS_VIEW = "has_portfolio_members_view" def grant_access(*rules): @@ -160,6 +161,14 @@ def _user_has_permission(user, request, rules, **kwargs): ) conditions_met.append(has_permission) + if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_VIEW in rules: + portfolio = request.session.get("portfolio") + has_permission = ( + user.is_org_user(request) and + user.has_view_members_portfolio_permission(portfolio) + ) + conditions_met.append(has_permission) + return any(conditions_met) diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index 1b9198c79..d64586a2b 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -6,19 +6,17 @@ from django.shortcuts import render from django.contrib import admin from django.db.models import Avg, F -from registrar.views.utility.mixins import DomainAndRequestsReportsPermission, PortfolioReportsPermission +from registrar.decorators import ALL, HAS_PORTFOLIO_MEMBERS_VIEW, IS_STAFF, grant_access from .. import models import datetime from django.utils import timezone -from django.contrib.admin.views.decorators import staff_member_required -from django.utils.decorators import method_decorator from registrar.utility import csv_export import logging logger = logging.getLogger(__name__) -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class AnalyticsView(View): def get(self, request): thirty_days_ago = datetime.datetime.today() - datetime.timedelta(days=30) @@ -152,7 +150,7 @@ class AnalyticsView(View): return render(request, "admin/analytics.html", context) -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataType(View): def get(self, request, *args, **kwargs): # match the CSV example with all the fields @@ -162,7 +160,8 @@ class ExportDataType(View): return response -class ExportDataTypeUser(DomainAndRequestsReportsPermission, View): +@grant_access(ALL) +class ExportDataTypeUser(View): """Returns a domain report for a given user on the request""" def get(self, request, *args, **kwargs): @@ -173,7 +172,8 @@ class ExportDataTypeUser(DomainAndRequestsReportsPermission, View): return response -class ExportMembersPortfolio(PortfolioReportsPermission, View): +@grant_access(HAS_PORTFOLIO_MEMBERS_VIEW) +class ExportMembersPortfolio(View): """Returns a members report for a given portfolio""" def get(self, request, *args, **kwargs): @@ -201,7 +201,7 @@ class ExportMembersPortfolio(PortfolioReportsPermission, View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataFull(View): def get(self, request, *args, **kwargs): # Smaller export based on 1 @@ -211,7 +211,7 @@ class ExportDataFull(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataFederal(View): def get(self, request, *args, **kwargs): # Federal only @@ -221,7 +221,7 @@ class ExportDataFederal(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDomainRequestDataFull(View): """Generates a downloaded report containing all Domain Requests (except started)""" @@ -233,7 +233,7 @@ class ExportDomainRequestDataFull(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataDomainsGrowth(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") @@ -246,7 +246,7 @@ class ExportDataDomainsGrowth(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataRequestsGrowth(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") @@ -259,7 +259,7 @@ class ExportDataRequestsGrowth(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataManagedDomains(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") @@ -271,7 +271,7 @@ class ExportDataManagedDomains(View): return response -@method_decorator(staff_member_required, name="dispatch") +@grant_access(IS_STAFF) class ExportDataUnmanagedDomains(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py index f574b76d9..62cd0a9d2 100644 --- a/src/registrar/views/transfer_user.py +++ b/src/registrar/views/transfer_user.py @@ -4,6 +4,7 @@ from django.db.models import ForeignKey, OneToOneField, ManyToManyField, ManyToO from django.shortcuts import render, get_object_or_404, redirect from django.views import View +from registrar.decorators import IS_STAFF, grant_access from registrar.models.domain import Domain from registrar.models.domain_request import DomainRequest from registrar.models.user import User @@ -18,6 +19,7 @@ from registrar.utility.db_helpers import ignore_unique_violation logger = logging.getLogger(__name__) +@grant_access(IS_STAFF) class TransferUserView(View): """Transfer user methods that set up the transfer_user template and handle the forms on it.""" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index d3ec95af5..eb58a5125 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -1,6 +1,4 @@ -"""Permissions-related mixin classes.""" - -from django.contrib.auth.mixins import PermissionRequiredMixin +"""Mixin classes.""" import logging @@ -135,53 +133,3 @@ class OrderableFieldsMixin: # Infer the column name in a similar manner to how Django does method.short_description = field.replace("_", " ") return method - - -class PermissionsLoginMixin(PermissionRequiredMixin): - """Mixin that redirects to login page if not logged in, otherwise 403.""" - - def handle_no_permission(self): - self.raise_exception = self.request.user.is_authenticated - return super().handle_no_permission() - - -class DomainAndRequestsReportsPermission(PermissionsLoginMixin): - """Permission mixin for domain and requests csv downloads""" - - def has_permission(self): - """Check if this user has access to this domain. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - - if not self.request.user.is_authenticated: - return False - - if self.request.user.is_restricted(): - return False - - return True - - -class PortfolioReportsPermission(PermissionsLoginMixin): - """Permission mixin for portfolio csv downloads""" - - def has_permission(self): - """Check if this user has access to this domain. - - The user is in self.request.user and the domain needs to be looked - up from the domain's primary key in self.kwargs["pk"] - """ - - if not self.request.user.is_authenticated: - return False - - if self.request.user.is_restricted(): - return False - - portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission(portfolio): - return False - - return self.request.user.is_org_user(self.request) From 7a1348258d14d443b8f44392652ece8ea95e6169 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 10:50:42 -0500 Subject: [PATCH 11/17] remove superuser, and assign perm to transfer user --- src/registrar/decorators.py | 18 ++++-------------- src/registrar/views/portfolios.py | 3 ++- src/registrar/views/utility/mixins.py | 1 + 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 0f2c948b3..517985b6d 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -5,7 +5,6 @@ from registrar.models import Domain, DomainInformation, DomainInvitation, Domain # Constants for clarity ALL = "all" -IS_SUPERUSER = "is_superuser" IS_STAFF = "is_staff" IS_DOMAIN_MANAGER = "is_domain_manager" IS_DOMAIN_REQUEST_CREATOR = "is_domain_request_creator" @@ -88,9 +87,6 @@ def _user_has_permission(user, request, rules, **kwargs): if IS_STAFF in rules: conditions_met.append(user.is_staff) - if not any(conditions_met) and IS_SUPERUSER in rules: - conditions_met.append(user.is_superuser) - if not any(conditions_met) and IS_DOMAIN_MANAGER in rules: has_permission = _is_domain_manager(user, **kwargs) conditions_met.append(has_permission) @@ -148,25 +144,19 @@ def _user_has_permission(user, request, rules, **kwargs): if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_ANY_PERM in rules: portfolio = request.session.get("portfolio") has_permission = user.is_org_user(request) and ( - user.has_view_members_portfolio_permission(portfolio) or - user.has_edit_members_portfolio_permission(portfolio) + user.has_view_members_portfolio_permission(portfolio) + or user.has_edit_members_portfolio_permission(portfolio) ) conditions_met.append(has_permission) if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_EDIT in rules: portfolio = request.session.get("portfolio") - has_permission = ( - user.is_org_user(request) and - user.has_edit_members_portfolio_permission(portfolio) - ) + has_permission = user.is_org_user(request) and user.has_edit_members_portfolio_permission(portfolio) conditions_met.append(has_permission) if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_VIEW in rules: portfolio = request.session.get("portfolio") - has_permission = ( - user.is_org_user(request) and - user.has_view_members_portfolio_permission(portfolio) - ) + has_permission = user.is_org_user(request) and user.has_view_members_portfolio_permission(portfolio) conditions_met.append(has_permission) return any(conditions_met) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index f0c4841f1..41038443b 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -23,7 +23,7 @@ from registrar.models import ( PortfolioInvitation, User, UserDomainRole, - UserPortfolioPermission + UserPortfolioPermission, ) from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError @@ -573,6 +573,7 @@ class PortfolioInvitedMemberDomainsView(View): }, ) + @grant_access(HAS_PORTFOLIO_MEMBERS_EDIT) class PortfolioInvitedMemberDomainsEditView(DetailView, View): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index eb58a5125..e228fbcd8 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -1,4 +1,5 @@ """Mixin classes.""" + import logging From b170a47f465ae5b1d17b8ee8ab3f079c2ef7ad17 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 17:15:59 -0500 Subject: [PATCH 12/17] updating tests --- src/registrar/tests/test_admin_domain.py | 2 +- src/registrar/tests/test_api.py | 6 +- .../tests/test_management_scripts.py | 643 +++++++++--------- src/registrar/tests/test_models.py | 2 +- src/registrar/tests/test_url_auth.py | 3 + src/registrar/tests/test_views_domain.py | 244 +++---- src/registrar/views/domain.py | 6 +- src/registrar/views/utility/api_views.py | 19 +- 8 files changed, 469 insertions(+), 456 deletions(-) diff --git a/src/registrar/tests/test_admin_domain.py b/src/registrar/tests/test_admin_domain.py index 867bf1b82..f6807294c 100644 --- a/src/registrar/tests/test_admin_domain.py +++ b/src/registrar/tests/test_admin_domain.py @@ -223,7 +223,7 @@ class TestDomainAdminAsStaff(MockEppLib): self.assertEqual(domain.state, Domain.State.DELETED) - # @less_console_noise_decorator + @less_console_noise_decorator def test_deletion_is_unsuccessful(self): """ Scenario: Domain deletion is unsuccessful diff --git a/src/registrar/tests/test_api.py b/src/registrar/tests/test_api.py index f0d5dbcec..ea4ee69cc 100644 --- a/src/registrar/tests/test_api.py +++ b/src/registrar/tests/test_api.py @@ -62,7 +62,7 @@ class GetSeniorOfficialJsonTest(TestCase): p = "password" self.client.login(username="testuser", password=p) response = self.client.get(self.api_url, {"agency_name": "Test Agency"}) - self.assertEqual(response.status_code, 302) + self.assertEqual(response.status_code, 403) @less_console_noise_decorator def test_get_senior_official_json_not_found(self): @@ -138,7 +138,7 @@ class GetPortfolioJsonTest(TestCase): """Test that an unauthenticated user receives a 403 with an error message.""" self.client.force_login(self.user) response = self.client.get(self.api_url, {"id": self.portfolio.id}) - self.assertEqual(response.status_code, 302) + self.assertEqual(response.status_code, 403) @less_console_noise_decorator def test_get_portfolio_json_not_found(self): @@ -181,7 +181,7 @@ class GetFederalPortfolioTypeJsonTest(TestCase): p = "password" self.client.login(username="testuser", password=p) response = self.client.get(self.api_url, {"agency_name": "Test Agency", "organization_type": "federal"}) - self.assertEqual(response.status_code, 302) + self.assertEqual(response.status_code, 403) class GetActionNeededEmailForUserJsonTest(TestCase): diff --git a/src/registrar/tests/test_management_scripts.py b/src/registrar/tests/test_management_scripts.py index fd53c21f8..668eeff0e 100644 --- a/src/registrar/tests/test_management_scripts.py +++ b/src/registrar/tests/test_management_scripts.py @@ -453,6 +453,7 @@ class TestPopulateFirstReady(TestCase): # Delete domains Domain.objects.all().delete() + @less_console_noise_decorator def run_populate_first_ready(self): """ This method executes the populate_first_ready command. @@ -460,103 +461,102 @@ class TestPopulateFirstReady(TestCase): The 'call_command' function from Django's management framework is then used to execute the populate_first_ready command with the specified arguments. """ - with less_console_noise(): - with patch( - "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa - return_value=True, - ): - call_command("populate_first_ready") + with patch( + "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa + return_value=True, + ): + call_command("populate_first_ready") + @less_console_noise_decorator def test_populate_first_ready_state_ready(self): """ Tests that the populate_first_ready works as expected for the state 'ready' """ - with less_console_noise(): - # Set the created at date - self.ready_domain.created_at = self.ready_at_date_tz_aware - self.ready_domain.save() - desired_domain = copy.deepcopy(self.ready_domain) - desired_domain.first_ready = self.ready_at_date - # Run the expiration date script - self.run_populate_first_ready() - self.assertEqual(desired_domain, self.ready_domain) - # Explicitly test the first_ready date - first_ready = Domain.objects.filter(name="fakeready.gov").get().first_ready - self.assertEqual(first_ready, self.ready_at_date) + # Set the created at date + self.ready_domain.created_at = self.ready_at_date_tz_aware + self.ready_domain.save() + desired_domain = copy.deepcopy(self.ready_domain) + desired_domain.first_ready = self.ready_at_date + # Run the expiration date script + self.run_populate_first_ready() + self.assertEqual(desired_domain, self.ready_domain) + # Explicitly test the first_ready date + first_ready = Domain.objects.filter(name="fakeready.gov").get().first_ready + self.assertEqual(first_ready, self.ready_at_date) + @less_console_noise_decorator def test_populate_first_ready_state_deleted(self): """ Tests that the populate_first_ready works as expected for the state 'deleted' """ - with less_console_noise(): - # Set the created at date - self.deleted_domain.created_at = self.ready_at_date_tz_aware - self.deleted_domain.save() - desired_domain = copy.deepcopy(self.deleted_domain) - desired_domain.first_ready = self.ready_at_date - # Run the expiration date script - self.run_populate_first_ready() - self.assertEqual(desired_domain, self.deleted_domain) - # Explicitly test the first_ready date - first_ready = Domain.objects.filter(name="fakedeleted.gov").get().first_ready - self.assertEqual(first_ready, self.ready_at_date) + # Set the created at date + self.deleted_domain.created_at = self.ready_at_date_tz_aware + self.deleted_domain.save() + desired_domain = copy.deepcopy(self.deleted_domain) + desired_domain.first_ready = self.ready_at_date + # Run the expiration date script + self.run_populate_first_ready() + self.assertEqual(desired_domain, self.deleted_domain) + # Explicitly test the first_ready date + first_ready = Domain.objects.filter(name="fakedeleted.gov").get().first_ready + self.assertEqual(first_ready, self.ready_at_date) + @less_console_noise_decorator def test_populate_first_ready_state_dns_needed(self): """ Tests that the populate_first_ready doesn't make changes when a domain's state is 'dns_needed' """ - with less_console_noise(): - # Set the created at date - self.dns_needed_domain.created_at = self.ready_at_date_tz_aware - self.dns_needed_domain.save() - desired_domain = copy.deepcopy(self.dns_needed_domain) - desired_domain.first_ready = None - # Run the expiration date script - self.run_populate_first_ready() - current_domain = self.dns_needed_domain - # The object should largely be unaltered (does not test first_ready) - self.assertEqual(desired_domain, current_domain) - first_ready = Domain.objects.filter(name="fakedns.gov").get().first_ready - # Explicitly test the first_ready date - self.assertNotEqual(first_ready, self.ready_at_date) - self.assertEqual(first_ready, None) + # Set the created at date + self.dns_needed_domain.created_at = self.ready_at_date_tz_aware + self.dns_needed_domain.save() + desired_domain = copy.deepcopy(self.dns_needed_domain) + desired_domain.first_ready = None + # Run the expiration date script + self.run_populate_first_ready() + current_domain = self.dns_needed_domain + # The object should largely be unaltered (does not test first_ready) + self.assertEqual(desired_domain, current_domain) + first_ready = Domain.objects.filter(name="fakedns.gov").get().first_ready + # Explicitly test the first_ready date + self.assertNotEqual(first_ready, self.ready_at_date) + self.assertEqual(first_ready, None) + @less_console_noise_decorator def test_populate_first_ready_state_on_hold(self): """ Tests that the populate_first_ready works as expected for the state 'on_hold' """ - with less_console_noise(): - self.hold_domain.created_at = self.ready_at_date_tz_aware - self.hold_domain.save() - desired_domain = copy.deepcopy(self.hold_domain) - desired_domain.first_ready = self.ready_at_date - # Run the update first ready_at script - self.run_populate_first_ready() - current_domain = self.hold_domain - self.assertEqual(desired_domain, current_domain) - # Explicitly test the first_ready date - first_ready = Domain.objects.filter(name="fakehold.gov").get().first_ready - self.assertEqual(first_ready, self.ready_at_date) + self.hold_domain.created_at = self.ready_at_date_tz_aware + self.hold_domain.save() + desired_domain = copy.deepcopy(self.hold_domain) + desired_domain.first_ready = self.ready_at_date + # Run the update first ready_at script + self.run_populate_first_ready() + current_domain = self.hold_domain + self.assertEqual(desired_domain, current_domain) + # Explicitly test the first_ready date + first_ready = Domain.objects.filter(name="fakehold.gov").get().first_ready + self.assertEqual(first_ready, self.ready_at_date) + @less_console_noise_decorator def test_populate_first_ready_state_unknown(self): """ Tests that the populate_first_ready works as expected for the state 'unknown' """ - with less_console_noise(): - # Set the created at date - self.unknown_domain.created_at = self.ready_at_date_tz_aware - self.unknown_domain.save() - desired_domain = copy.deepcopy(self.unknown_domain) - desired_domain.first_ready = None - # Run the expiration date script - self.run_populate_first_ready() - current_domain = self.unknown_domain - # The object should largely be unaltered (does not test first_ready) - self.assertEqual(desired_domain, current_domain) - # Explicitly test the first_ready date - first_ready = Domain.objects.filter(name="fakeunknown.gov").get().first_ready - self.assertNotEqual(first_ready, self.ready_at_date) - self.assertEqual(first_ready, None) + # Set the created at date + self.unknown_domain.created_at = self.ready_at_date_tz_aware + self.unknown_domain.save() + desired_domain = copy.deepcopy(self.unknown_domain) + desired_domain.first_ready = None + # Run the expiration date script + self.run_populate_first_ready() + current_domain = self.unknown_domain + # The object should largely be unaltered (does not test first_ready) + self.assertEqual(desired_domain, current_domain) + # Explicitly test the first_ready date + first_ready = Domain.objects.filter(name="fakeunknown.gov").get().first_ready + self.assertNotEqual(first_ready, self.ready_at_date) + self.assertEqual(first_ready, None) class TestPatchAgencyInfo(TestCase): @@ -577,10 +577,10 @@ class TestPatchAgencyInfo(TestCase): TransitionDomain.objects.all().delete() @patch("registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", return_value=True) + @less_console_noise_decorator def call_patch_federal_agency_info(self, mock_prompt): """Calls the patch_federal_agency_info command and mimics a keypress""" - with less_console_noise(): - call_command("patch_federal_agency_info", "registrar/tests/data/fake_current_full.csv", debug=True) + call_command("patch_federal_agency_info", "registrar/tests/data/fake_current_full.csv", debug=True) class TestExtendExpirationDates(MockEppLib): @@ -636,6 +636,7 @@ class TestExtendExpirationDates(MockEppLib): User.objects.all().delete() UserDomainRole.objects.all().delete() + @less_console_noise_decorator def run_extend_expiration_dates(self): """ This method executes the extend_expiration_dates command. @@ -643,83 +644,83 @@ class TestExtendExpirationDates(MockEppLib): The 'call_command' function from Django's management framework is then used to execute the extend_expiration_dates command with the specified arguments. """ - with less_console_noise(): - with patch( - "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa - return_value=True, - ): - call_command("extend_expiration_dates") + with patch( + "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa + return_value=True, + ): + call_command("extend_expiration_dates") + @less_console_noise_decorator def test_extends_expiration_date_correctly(self): """ Tests that the extend_expiration_dates method extends dates as expected """ - with less_console_noise(): - desired_domain = Domain.objects.filter(name="waterbutpurple.gov").get() - desired_domain.expiration_date = date(2024, 11, 15) - # Run the expiration date script - self.run_extend_expiration_dates() - current_domain = Domain.objects.filter(name="waterbutpurple.gov").get() - self.assertEqual(desired_domain, current_domain) - # Explicitly test the expiration date - self.assertEqual(current_domain.expiration_date, date(2024, 11, 15)) + desired_domain = Domain.objects.filter(name="waterbutpurple.gov").get() + desired_domain.expiration_date = date(2024, 11, 15) + # Run the expiration date script + self.run_extend_expiration_dates() + current_domain = Domain.objects.filter(name="waterbutpurple.gov").get() + self.assertEqual(desired_domain, current_domain) + # Explicitly test the expiration date + self.assertEqual(current_domain.expiration_date, date(2024, 11, 15)) + @less_console_noise_decorator def test_extends_expiration_date_skips_non_current(self): """ Tests that the extend_expiration_dates method correctly skips domains with an expiration date less than a certain threshold. """ - with less_console_noise(): - desired_domain = Domain.objects.filter(name="fake.gov").get() - desired_domain.expiration_date = date(2022, 5, 25) - # Run the expiration date script - self.run_extend_expiration_dates() - current_domain = Domain.objects.filter(name="fake.gov").get() - self.assertEqual(desired_domain, current_domain) - # Explicitly test the expiration date. The extend_expiration_dates script - # will skip all dates less than date(2023, 11, 15), meaning that this domain - # should not be affected by the change. - self.assertEqual(current_domain.expiration_date, date(2022, 5, 25)) + desired_domain = Domain.objects.filter(name="fake.gov").get() + desired_domain.expiration_date = date(2022, 5, 25) + # Run the expiration date script + self.run_extend_expiration_dates() + current_domain = Domain.objects.filter(name="fake.gov").get() + self.assertEqual(desired_domain, current_domain) + # Explicitly test the expiration date. The extend_expiration_dates script + # will skip all dates less than date(2023, 11, 15), meaning that this domain + # should not be affected by the change. + self.assertEqual(current_domain.expiration_date, date(2022, 5, 25)) + @less_console_noise_decorator def test_extends_expiration_date_skips_maximum_date(self): """ Tests that the extend_expiration_dates method correctly skips domains with an expiration date more than a certain threshold. """ - with less_console_noise(): - desired_domain = Domain.objects.filter(name="fakemaximum.gov").get() - desired_domain.expiration_date = date(2024, 12, 31) + desired_domain = Domain.objects.filter(name="fakemaximum.gov").get() + desired_domain.expiration_date = date(2024, 12, 31) - # Run the expiration date script - self.run_extend_expiration_dates() + # Run the expiration date script + self.run_extend_expiration_dates() - current_domain = Domain.objects.filter(name="fakemaximum.gov").get() - self.assertEqual(desired_domain, current_domain) + current_domain = Domain.objects.filter(name="fakemaximum.gov").get() + self.assertEqual(desired_domain, current_domain) - # Explicitly test the expiration date. The extend_expiration_dates script - # will skip all dates less than date(2023, 11, 15), meaning that this domain - # should not be affected by the change. - self.assertEqual(current_domain.expiration_date, date(2024, 12, 31)) + # Explicitly test the expiration date. The extend_expiration_dates script + # will skip all dates less than date(2023, 11, 15), meaning that this domain + # should not be affected by the change. + self.assertEqual(current_domain.expiration_date, date(2024, 12, 31)) + @less_console_noise_decorator def test_extends_expiration_date_skips_non_ready(self): """ Tests that the extend_expiration_dates method correctly skips domains not in the state "ready" """ - with less_console_noise(): - desired_domain = Domain.objects.filter(name="fakeneeded.gov").get() - desired_domain.expiration_date = date(2023, 11, 15) + desired_domain = Domain.objects.filter(name="fakeneeded.gov").get() + desired_domain.expiration_date = date(2023, 11, 15) - # Run the expiration date script - self.run_extend_expiration_dates() + # Run the expiration date script + self.run_extend_expiration_dates() - current_domain = Domain.objects.filter(name="fakeneeded.gov").get() - self.assertEqual(desired_domain, current_domain) + current_domain = Domain.objects.filter(name="fakeneeded.gov").get() + self.assertEqual(desired_domain, current_domain) - # Explicitly test the expiration date. The extend_expiration_dates script - # will skip all dates less than date(2023, 11, 15), meaning that this domain - # should not be affected by the change. - self.assertEqual(current_domain.expiration_date, date(2023, 11, 15)) + # Explicitly test the expiration date. The extend_expiration_dates script + # will skip all dates less than date(2023, 11, 15), meaning that this domain + # should not be affected by the change. + self.assertEqual(current_domain.expiration_date, date(2023, 11, 15)) + @less_console_noise_decorator def test_extends_expiration_date_idempotent(self): """ Tests the idempotency of the extend_expiration_dates command. @@ -727,21 +728,20 @@ class TestExtendExpirationDates(MockEppLib): Verifies that running the method multiple times does not change the expiration date of a domain beyond the initial extension. """ - with less_console_noise(): - desired_domain = Domain.objects.filter(name="waterbutpurple.gov").get() - desired_domain.expiration_date = date(2024, 11, 15) - # Run the expiration date script - self.run_extend_expiration_dates() - current_domain = Domain.objects.filter(name="waterbutpurple.gov").get() - self.assertEqual(desired_domain, current_domain) - # Explicitly test the expiration date - self.assertEqual(desired_domain.expiration_date, date(2024, 11, 15)) - # Run the expiration date script again - self.run_extend_expiration_dates() - # The old domain shouldn't have changed - self.assertEqual(desired_domain, current_domain) - # Explicitly test the expiration date - should be the same - self.assertEqual(desired_domain.expiration_date, date(2024, 11, 15)) + desired_domain = Domain.objects.filter(name="waterbutpurple.gov").get() + desired_domain.expiration_date = date(2024, 11, 15) + # Run the expiration date script + self.run_extend_expiration_dates() + current_domain = Domain.objects.filter(name="waterbutpurple.gov").get() + self.assertEqual(desired_domain, current_domain) + # Explicitly test the expiration date + self.assertEqual(desired_domain.expiration_date, date(2024, 11, 15)) + # Run the expiration date script again + self.run_extend_expiration_dates() + # The old domain shouldn't have changed + self.assertEqual(desired_domain, current_domain) + # Explicitly test the expiration date - should be the same + self.assertEqual(desired_domain.expiration_date, date(2024, 11, 15)) class TestDiscloseEmails(MockEppLib): @@ -753,6 +753,7 @@ class TestDiscloseEmails(MockEppLib): PublicContact.objects.all().delete() Domain.objects.all().delete() + @less_console_noise_decorator def run_disclose_security_emails(self): """ This method executes the disclose_security_emails command. @@ -760,44 +761,43 @@ class TestDiscloseEmails(MockEppLib): The 'call_command' function from Django's management framework is then used to execute the disclose_security_emails command. """ - with less_console_noise(): - with patch( - "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa - return_value=True, - ): - call_command("disclose_security_emails") + with patch( + "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa + return_value=True, + ): + call_command("disclose_security_emails") + @less_console_noise_decorator def test_disclose_security_emails(self): """ Tests that command disclose_security_emails runs successfully with appropriate EPP calll to UpdateContact. """ - with less_console_noise(): - domain, _ = Domain.objects.get_or_create(name="testdisclose.gov", state=Domain.State.READY) - expectedSecContact = PublicContact.get_default_security() - expectedSecContact.domain = domain - expectedSecContact.email = "123@mail.gov" - # set domain security email to 123@mail.gov instead of default email - domain.security_contact = expectedSecContact - self.run_disclose_security_emails() + domain, _ = Domain.objects.get_or_create(name="testdisclose.gov", state=Domain.State.READY) + expectedSecContact = PublicContact.get_default_security() + expectedSecContact.domain = domain + expectedSecContact.email = "123@mail.gov" + # set domain security email to 123@mail.gov instead of default email + domain.security_contact = expectedSecContact + self.run_disclose_security_emails() - # running disclose_security_emails sends EPP call UpdateContact with disclose - self.mockedSendFunction.assert_has_calls( - [ - call( - commands.UpdateContact( - id=domain.security_contact.registry_id, - postal_info=domain._make_epp_contact_postal_info(contact=domain.security_contact), - email=domain.security_contact.email, - voice=domain.security_contact.voice, - fax=domain.security_contact.fax, - auth_info=common.ContactAuthInfo(pw="2fooBAR123fooBaz"), - disclose=domain._disclose_fields(contact=domain.security_contact), - ), - cleaned=True, - ) - ] - ) + # running disclose_security_emails sends EPP call UpdateContact with disclose + self.mockedSendFunction.assert_has_calls( + [ + call( + commands.UpdateContact( + id=domain.security_contact.registry_id, + postal_info=domain._make_epp_contact_postal_info(contact=domain.security_contact), + email=domain.security_contact.email, + voice=domain.security_contact.voice, + fax=domain.security_contact.fax, + auth_info=common.ContactAuthInfo(pw="2fooBAR123fooBaz"), + disclose=domain._disclose_fields(contact=domain.security_contact), + ), + cleaned=True, + ) + ] + ) class TestCleanTables(TestCase): @@ -812,17 +812,18 @@ class TestCleanTables(TestCase): self.logger_patcher.stop() @override_settings(IS_PRODUCTION=True) + @less_console_noise_decorator def test_command_logs_error_in_production(self): """Test that the handle method does not process in production""" - with less_console_noise(): - with patch( - "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa - return_value=True, - ): - call_command("clean_tables") - self.logger_mock.error.assert_called_with("clean_tables cannot be run in production") + with patch( + "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa + return_value=True, + ): + call_command("clean_tables") + self.logger_mock.error.assert_called_with("clean_tables cannot be run in production") @override_settings(IS_PRODUCTION=False) + @less_console_noise_decorator def test_command_cleans_tables(self): """test that the handle method functions properly to clean tables""" @@ -890,61 +891,61 @@ class TestCleanTables(TestCase): raise @override_settings(IS_PRODUCTION=False) + @less_console_noise_decorator def test_command_handles_nonexistent_model(self): """Test that exceptions for non existent models are handled properly within the handle method""" - with less_console_noise(): - with patch("django.apps.apps.get_model", side_effect=LookupError): - with patch( - "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa - return_value=True, - ): - call_command("clean_tables") - # Assert that the error message was logged for any of the table names - self.logger_mock.error.assert_any_call("Model for table DomainInformation not found.") - self.logger_mock.error.assert_any_call("Model for table DomainRequest not found.") - self.logger_mock.error.assert_any_call("Model for table PublicContact not found.") - self.logger_mock.error.assert_any_call("Model for table Domain not found.") - self.logger_mock.error.assert_any_call("Model for table User not found.") - self.logger_mock.error.assert_any_call("Model for table Contact not found.") - self.logger_mock.error.assert_any_call("Model for table Website not found.") - self.logger_mock.error.assert_any_call("Model for table DraftDomain not found.") - self.logger_mock.error.assert_any_call("Model for table HostIp not found.") - self.logger_mock.error.assert_any_call("Model for table Host not found.") + with patch("django.apps.apps.get_model", side_effect=LookupError): + with patch( + "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa + return_value=True, + ): + call_command("clean_tables") + # Assert that the error message was logged for any of the table names + self.logger_mock.error.assert_any_call("Model for table DomainInformation not found.") + self.logger_mock.error.assert_any_call("Model for table DomainRequest not found.") + self.logger_mock.error.assert_any_call("Model for table PublicContact not found.") + self.logger_mock.error.assert_any_call("Model for table Domain not found.") + self.logger_mock.error.assert_any_call("Model for table User not found.") + self.logger_mock.error.assert_any_call("Model for table Contact not found.") + self.logger_mock.error.assert_any_call("Model for table Website not found.") + self.logger_mock.error.assert_any_call("Model for table DraftDomain not found.") + self.logger_mock.error.assert_any_call("Model for table HostIp not found.") + self.logger_mock.error.assert_any_call("Model for table Host not found.") @override_settings(IS_PRODUCTION=False) + @less_console_noise_decorator def test_command_logs_other_exceptions(self): """Test that generic exceptions are handled properly in the handle method""" - with less_console_noise(): - with patch("django.apps.apps.get_model") as get_model_mock: - model_mock = MagicMock() - get_model_mock.return_value = model_mock + with patch("django.apps.apps.get_model") as get_model_mock: + model_mock = MagicMock() + get_model_mock.return_value = model_mock - # Mock the values_list so that DomainInformation attempts a delete - pk_batches = [[1, 2, 3, 4, 5, 6], []] + # Mock the values_list so that DomainInformation attempts a delete + pk_batches = [[1, 2, 3, 4, 5, 6], []] - def values_list_side_effect(*args, **kwargs): - if args == ("pk",) and kwargs.get("flat", False): - return pk_batches.pop(0) - return [] + def values_list_side_effect(*args, **kwargs): + if args == ("pk",) and kwargs.get("flat", False): + return pk_batches.pop(0) + return [] - model_mock.objects.values_list.side_effect = values_list_side_effect + model_mock.objects.values_list.side_effect = values_list_side_effect - # Mock delete to raise a generic exception - model_mock.objects.filter.return_value.delete.side_effect = Exception("Mocked delete exception") + # Mock delete to raise a generic exception + model_mock.objects.filter.return_value.delete.side_effect = Exception("Mocked delete exception") - with patch( - "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", - return_value=True, - ): - with self.assertRaises(Exception) as context: - # Execute the command - call_command("clean_tables") + with patch( + "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", + return_value=True, + ): + with self.assertRaises(Exception) as context: + # Execute the command + call_command("clean_tables") - # Check the exception message - self.assertEqual(str(context.exception), "Custom delete error") + # Check the exception message + self.assertEqual(str(context.exception), "Custom delete error") - # Assert that delete was called - model_mock.objects.filter.return_value.delete.assert_called() + # Assert that delete was called + model_mock.objects.filter.return_value.delete.assert_called() class TestExportTables(MockEppLib): @@ -1029,34 +1030,34 @@ class TestExportTables(MockEppLib): self.logger_mock.info.assert_any_call(f"Removed {table_name}_1.csv") @patch("registrar.management.commands.export_tables.getattr") + @less_console_noise_decorator def test_export_table_handles_missing_resource_class(self, mock_getattr): """Test that missing resource classes are handled properly in the handle method""" - with less_console_noise(): - mock_getattr.side_effect = AttributeError + mock_getattr.side_effect = AttributeError - # Import the command to avoid any locale or gettext issues - command_class = import_string("registrar.management.commands.export_tables.Command") - command_instance = command_class() - command_instance.export_table("NonExistentTable") + # Import the command to avoid any locale or gettext issues + command_class = import_string("registrar.management.commands.export_tables.Command") + command_instance = command_class() + command_instance.export_table("NonExistentTable") - self.logger_mock.error.assert_called_with( - "Resource class NonExistentTableResource not found in registrar.admin" - ) + self.logger_mock.error.assert_called_with( + "Resource class NonExistentTableResource not found in registrar.admin" + ) @patch("registrar.management.commands.export_tables.getattr") + @less_console_noise_decorator def test_export_table_handles_generic_exception(self, mock_getattr): """Test that general exceptions in the handle method are handled correctly""" - with less_console_noise(): - mock_resource_class = MagicMock() - mock_resource_class().export.side_effect = Exception("Test Exception") - mock_getattr.return_value = mock_resource_class + mock_resource_class = MagicMock() + mock_resource_class().export.side_effect = Exception("Test Exception") + mock_getattr.return_value = mock_resource_class - # Import the command to avoid any locale or gettext issues - command_class = import_string("registrar.management.commands.export_tables.Command") - command_instance = command_class() - command_instance.export_table("TestTable") + # Import the command to avoid any locale or gettext issues + command_class = import_string("registrar.management.commands.export_tables.Command") + command_instance = command_class() + command_instance.export_table("TestTable") - self.logger_mock.error.assert_called_with("Failed to export TestTable: Test Exception") + self.logger_mock.error.assert_called_with("Failed to export TestTable: Test Exception") class TestImportTables(TestCase): @@ -1072,6 +1073,7 @@ class TestImportTables(TestCase): @patch("registrar.management.commands.import_tables.getattr") @patch("django.apps.apps.get_model") @patch("os.listdir") + @less_console_noise_decorator def test_handle( self, mock_listdir, @@ -1086,105 +1088,104 @@ class TestImportTables(TestCase): mock_makedirs, ): """Test that the handle method properly imports tables""" - with less_console_noise(): - # Mock os.makedirs to do nothing - mock_makedirs.return_value = None + # Mock os.makedirs to do nothing + mock_makedirs.return_value = None - # Mock os.path.exists to always return True - mock_path_exists.return_value = True + # Mock os.path.exists to always return True + mock_path_exists.return_value = True - # Mock the zipfile to have extractall return None - mock_zipfile_instance = mock_zipfile.return_value.__enter__.return_value - mock_zipfile_instance.extractall.return_value = None + # Mock the zipfile to have extractall return None + mock_zipfile_instance = mock_zipfile.return_value.__enter__.return_value + mock_zipfile_instance.extractall.return_value = None - # Check that the import_table function was called for each table - table_names = [ - "User", - "Contact", - "Domain", - "DomainRequest", - "DomainInformation", - "UserDomainRole", - "DraftDomain", - "Website", - "HostIp", - "Host", - "PublicContact", - ] + # Check that the import_table function was called for each table + table_names = [ + "User", + "Contact", + "Domain", + "DomainRequest", + "DomainInformation", + "UserDomainRole", + "DraftDomain", + "Website", + "HostIp", + "Host", + "PublicContact", + ] - # Mock directory listing - mock_listdir.side_effect = lambda path: [f"{table}_1.csv" for table in table_names] + # Mock directory listing + mock_listdir.side_effect = lambda path: [f"{table}_1.csv" for table in table_names] - # Mock the CSV file content - csv_content = b"mock_csv_data" + # Mock the CSV file content + csv_content = b"mock_csv_data" - # Mock the open function to return a mock file - mock_open.return_value.__enter__.return_value.read.return_value = csv_content + # Mock the open function to return a mock file + mock_open.return_value.__enter__.return_value.read.return_value = csv_content - # Mock the Dataset class and its load method to return a dataset - mock_dataset_instance = MagicMock(spec=tablib.Dataset) - with patch( - "registrar.management.commands.import_tables.tablib.Dataset.load", return_value=mock_dataset_instance - ): - # Mock the resource class and its import method - mock_resource_class = MagicMock() - mock_resource_instance = MagicMock() - mock_result = MagicMock() - mock_result.has_errors.return_value = False - mock_resource_instance.import_data.return_value = mock_result - mock_resource_class.return_value = mock_resource_instance - mock_getattr.return_value = mock_resource_class + # Mock the Dataset class and its load method to return a dataset + mock_dataset_instance = MagicMock(spec=tablib.Dataset) + with patch( + "registrar.management.commands.import_tables.tablib.Dataset.load", return_value=mock_dataset_instance + ): + # Mock the resource class and its import method + mock_resource_class = MagicMock() + mock_resource_instance = MagicMock() + mock_result = MagicMock() + mock_result.has_errors.return_value = False + mock_resource_instance.import_data.return_value = mock_result + mock_resource_class.return_value = mock_resource_instance + mock_getattr.return_value = mock_resource_class - # Call the command - call_command("import_tables") + # Call the command + call_command("import_tables") - # Check that os.makedirs was called once to create the tmp directory - mock_makedirs.assert_called_once_with("tmp", exist_ok=True) + # Check that os.makedirs was called once to create the tmp directory + mock_makedirs.assert_called_once_with("tmp", exist_ok=True) - # Check that os.path.exists was called once for the zip file - mock_path_exists.assert_any_call("tmp/exported_tables.zip") + # Check that os.path.exists was called once for the zip file + mock_path_exists.assert_any_call("tmp/exported_tables.zip") - # Check that pyzipper.AESZipFile was called once to open the zip file - mock_zipfile.assert_called_once_with("tmp/exported_tables.zip", "r") + # Check that pyzipper.AESZipFile was called once to open the zip file + mock_zipfile.assert_called_once_with("tmp/exported_tables.zip", "r") - # Check that extractall was called once to extract the zip file contents - mock_zipfile_instance.extractall.assert_called_once_with("tmp") + # Check that extractall was called once to extract the zip file contents + mock_zipfile_instance.extractall.assert_called_once_with("tmp") - # Check that os.path.exists was called for each table - for table_name in table_names: - mock_path_exists.assert_any_call(f"{table_name}_1.csv") + # Check that os.path.exists was called for each table + for table_name in table_names: + mock_path_exists.assert_any_call(f"{table_name}_1.csv") - # Check that logger.info was called for each successful import - for table_name in table_names: - mock_logger.info.assert_any_call(f"Successfully imported {table_name}_1.csv into {table_name}") + # Check that logger.info was called for each successful import + for table_name in table_names: + mock_logger.info.assert_any_call(f"Successfully imported {table_name}_1.csv into {table_name}") - # Check that logger.error was not called for resource class not found - mock_logger.error.assert_not_called() + # Check that logger.error was not called for resource class not found + mock_logger.error.assert_not_called() - # Check that os.remove was called for each CSV file - for table_name in table_names: - mock_remove.assert_any_call(f"{table_name}_1.csv") + # Check that os.remove was called for each CSV file + for table_name in table_names: + mock_remove.assert_any_call(f"{table_name}_1.csv") - # Check that logger.info was called for each CSV file removal - for table_name in table_names: - mock_logger.info.assert_any_call(f"Removed temporary file {table_name}_1.csv") + # Check that logger.info was called for each CSV file removal + for table_name in table_names: + mock_logger.info.assert_any_call(f"Removed temporary file {table_name}_1.csv") @patch("registrar.management.commands.import_tables.logger") @patch("registrar.management.commands.import_tables.os.makedirs") @patch("registrar.management.commands.import_tables.os.path.exists") + @less_console_noise_decorator def test_handle_zip_file_not_found(self, mock_path_exists, mock_makedirs, mock_logger): """Test the handle method when the zip file doesn't exist""" - with less_console_noise(): - # Mock os.makedirs to do nothing - mock_makedirs.return_value = None + # Mock os.makedirs to do nothing + mock_makedirs.return_value = None - # Mock os.path.exists to return False - mock_path_exists.return_value = False + # Mock os.path.exists to return False + mock_path_exists.return_value = False - call_command("import_tables") + call_command("import_tables") - # Check that logger.error was called with the correct message - mock_logger.error.assert_called_once_with("Zip file tmp/exported_tables.zip does not exist.") + # Check that logger.error was called with the correct message + mock_logger.error.assert_called_once_with("Zip file tmp/exported_tables.zip does not exist.") class TestTransferFederalAgencyType(TestCase): @@ -1254,6 +1255,7 @@ class TestTransferFederalAgencyType(TestCase): id__in=[self.amtrak.id, self.legislative_branch.id, self.library_of_congress.id, self.gov_admin.id] ).delete() + @less_console_noise_decorator def run_transfer_federal_agency_type(self): """ This method executes the transfer_federal_agency_type command. @@ -1261,12 +1263,11 @@ class TestTransferFederalAgencyType(TestCase): The 'call_command' function from Django's management framework is then used to execute the populate_first_ready command with the specified arguments. """ - with less_console_noise(): - with patch( - "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa - return_value=True, - ): - call_command("transfer_federal_agency_type") + with patch( + "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa + return_value=True, + ): + call_command("transfer_federal_agency_type") @less_console_noise_decorator def test_transfer_federal_agency_type_script(self): @@ -1628,6 +1629,7 @@ class TestCreateFederalPortfolio(TestCase): # Test the senior official self.assertEqual(portfolio.senior_official, self.senior_official) + @less_console_noise_decorator def test_create_multiple_portfolios_for_branch_judicial(self): """Tests creating all portfolios under a given branch""" federal_choice = DomainRequest.OrganizationChoices.FEDERAL @@ -1655,6 +1657,7 @@ class TestCreateFederalPortfolio(TestCase): self.assertTrue(all([creator == User.get_default_user() for creator in creators])) self.assertTrue(all([note == "Auto-generated record" for note in notes])) + @less_console_noise_decorator def test_create_multiple_portfolios_for_branch_legislative(self): """Tests creating all portfolios under a given branch""" federal_choice = DomainRequest.OrganizationChoices.FEDERAL @@ -1682,6 +1685,7 @@ class TestCreateFederalPortfolio(TestCase): self.assertTrue(all([creator == User.get_default_user() for creator in creators])) self.assertTrue(all([note == "Auto-generated record" for note in notes])) + @less_console_noise_decorator def test_script_adds_requested_suborganization_information(self): """Tests that the script adds the requested suborg fields for domain requests""" # Create a new domain request with some errant spacing @@ -1710,6 +1714,7 @@ class TestCreateFederalPortfolio(TestCase): custom_suborg_request.suborganization_state_territory, DomainRequest.StateTerritoryChoices.TEXAS ) + @less_console_noise_decorator def test_create_multiple_portfolios_for_branch_executive(self): """Tests creating all portfolios under a given branch""" federal_choice = DomainRequest.OrganizationChoices.FEDERAL @@ -1772,6 +1777,7 @@ class TestCreateFederalPortfolio(TestCase): self.assertEqual(expected_requests.count(), 2) self.assertEqual(expected_domain_infos.count(), 2) + @less_console_noise_decorator def test_handle_portfolio_requests(self): """Verify portfolio association with domain requests.""" self.run_create_federal_portfolio(agency_name="Test Federal Agency", parse_requests=True) @@ -1781,6 +1787,7 @@ class TestCreateFederalPortfolio(TestCase): self.assertEqual(self.domain_request.portfolio.federal_agency, self.federal_agency) self.assertEqual(self.domain_request.sub_organization.name, "Testorg") + @less_console_noise_decorator def test_handle_portfolio_domains(self): """Check portfolio association with domain information.""" self.run_create_federal_portfolio(agency_name="Test Federal Agency", parse_domains=True) @@ -1790,6 +1797,7 @@ class TestCreateFederalPortfolio(TestCase): self.assertEqual(self.domain_info.portfolio.federal_agency, self.federal_agency) self.assertEqual(self.domain_info.sub_organization.name, "Testorg") + @less_console_noise_decorator def test_handle_parse_both(self): """Ensure correct parsing of both requests and domains.""" self.run_create_federal_portfolio(agency_name="Test Federal Agency", parse_requests=True, parse_domains=True) @@ -1800,6 +1808,7 @@ class TestCreateFederalPortfolio(TestCase): self.assertIsNotNone(self.domain_info.portfolio) self.assertEqual(self.domain_request.portfolio, self.domain_info.portfolio) + @less_console_noise_decorator def test_command_error_parse_options(self): """Verify error when bad parse options are provided.""" # The command should enforce either --branch or --agency_name @@ -1821,6 +1830,7 @@ class TestCreateFederalPortfolio(TestCase): ): self.run_create_federal_portfolio(agency_name="test") + @less_console_noise_decorator def test_command_error_agency_not_found(self): """Check error handling for non-existent agency.""" expected_message = ( @@ -1830,6 +1840,7 @@ class TestCreateFederalPortfolio(TestCase): with self.assertRaisesRegex(CommandError, expected_message): self.run_create_federal_portfolio(agency_name="Non-existent Agency", parse_requests=True) + @less_console_noise_decorator def test_does_not_update_existing_portfolio(self): """Tests that an existing portfolio is not updated when""" # Create an existing portfolio @@ -1854,6 +1865,7 @@ class TestCreateFederalPortfolio(TestCase): self.assertEqual(existing_portfolio.notes, "Old notes") self.assertEqual(existing_portfolio.creator, self.user) + @less_console_noise_decorator def test_skip_existing_portfolios(self): """Tests the skip_existing_portfolios to ensure that it doesn't add suborgs, domain requests, and domain info.""" @@ -2294,6 +2306,7 @@ class TestRemovePortfolios(TestCase): Portfolio.objects.all().delete() User.objects.all().delete() + @less_console_noise_decorator @patch("registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no") def test_delete_unlisted_portfolios(self, mock_query_yes_no): """Test that portfolios not on the allowed list are deleted.""" @@ -2311,6 +2324,7 @@ class TestRemovePortfolios(TestCase): self.assertFalse(Portfolio.objects.filter(organization_name="Test with suborg").exists()) self.assertTrue(Portfolio.objects.filter(organization_name="Department of Veterans Affairs").exists()) + @less_console_noise_decorator @patch("registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no") def test_delete_entries_with_related_objects(self, mock_query_yes_no): """Test deletion with related objects being handled properly.""" @@ -2334,6 +2348,7 @@ class TestRemovePortfolios(TestCase): # Check that the portfolio was deleted self.assertFalse(Portfolio.objects.filter(organization_name="Test with orphaned objects").exists()) + @less_console_noise_decorator @patch("registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no") def test_delete_entries_with_suborganizations(self, mock_query_yes_no): """Test that suborganizations and their related objects are deleted along with the portfolio.""" diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 4401b73e8..355f2e276 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -2100,7 +2100,7 @@ class TestDomainRequestIncomplete(TestCase): self.wizard = DomainRequestWizard() self.wizard._domain_request = self.domain_request self.wizard.request = Mock(user=self.user, session={}) - self.wizard.kwargs = {"id": self.domain_request.id} + self.wizard.kwargs = {"domain_request_pk": self.domain_request.id} # We use both of these flags in the test. In the normal app these are generated normally. # The alternative syntax is adding the decorator to each test. diff --git a/src/registrar/tests/test_url_auth.py b/src/registrar/tests/test_url_auth.py index 1cd2d1384..17b13c233 100644 --- a/src/registrar/tests/test_url_auth.py +++ b/src/registrar/tests/test_url_auth.py @@ -18,6 +18,9 @@ from .common import less_console_noise # request on the view. SAMPLE_KWARGS = { "app_label": "registrar", + "domain_pk": "1", + "domain_request_pk": "1", + "domain_invitation_pk": "1", "pk": "1", "id": "1", "content_type_id": "2", diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index b84d284d8..d9aac8178 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -175,7 +175,7 @@ class TestDomainPermissions(TestWithDomainPermissions): "domain-security-email", ]: with self.subTest(view_name=view_name): - response = self.client.get(reverse(view_name, kwargs={"pk": self.domain.id})) + response = self.client.get(reverse(view_name, kwargs={"domain_pk": self.domain.id})) self.assertEqual(response.status_code, 302) @less_console_noise_decorator @@ -194,7 +194,7 @@ class TestDomainPermissions(TestWithDomainPermissions): "domain-security-email", ]: with self.subTest(view_name=view_name): - response = self.client.get(reverse(view_name, kwargs={"pk": self.domain.id})) + response = self.client.get(reverse(view_name, kwargs={"domain_pk": self.domain.id})) self.assertEqual(response.status_code, 403) @less_console_noise_decorator @@ -218,7 +218,7 @@ class TestDomainPermissions(TestWithDomainPermissions): self.domain_deleted, ]: with self.subTest(view_name=view_name, domain=domain): - response = self.client.get(reverse(view_name, kwargs={"pk": domain.id})) + response = self.client.get(reverse(view_name, kwargs={"domain_pk": domain.id})) self.assertEqual(response.status_code, 403) @@ -271,20 +271,20 @@ class TestDomainDetail(TestDomainOverview): with less_console_noise(): self.user.status = User.RESTRICTED self.user.save() - response = self.client.get(reverse("domain", kwargs={"pk": self.domain.id})) + response = self.client.get(reverse("domain", kwargs={"domain_pk": self.domain.id})) self.assertEqual(response.status_code, 403) def test_domain_detail_allowed_for_on_hold(self): """Test that the domain overview page displays for on hold domain""" with less_console_noise(): # View domain overview page - detail_page = self.client.get(reverse("domain", kwargs={"pk": self.domain_on_hold.id})) + detail_page = self.client.get(reverse("domain", kwargs={"domain_pk": self.domain_on_hold.id})) self.assertNotContains(detail_page, "Edit") def test_domain_detail_see_just_nameserver(self): with less_console_noise(): # View nameserver on Domain Overview page - detail_page = self.app.get(reverse("domain", kwargs={"pk": self.domain_just_nameserver.id})) + detail_page = self.app.get(reverse("domain", kwargs={"domain_pk": self.domain_just_nameserver.id})) self.assertContains(detail_page, "justnameserver.com") self.assertContains(detail_page, "ns1.justnameserver.com") @@ -293,7 +293,7 @@ class TestDomainDetail(TestDomainOverview): def test_domain_detail_see_nameserver_and_ip(self): with less_console_noise(): # View nameserver on Domain Overview page - detail_page = self.app.get(reverse("domain", kwargs={"pk": self.domain_with_ip.id})) + detail_page = self.app.get(reverse("domain", kwargs={"domain_pk": self.domain_with_ip.id})) self.assertContains(detail_page, "nameserverwithip.gov") @@ -321,7 +321,7 @@ class TestDomainDetail(TestDomainOverview): session["analyst_action_location"] = self.domain_no_information.id session.save() - detail_page = self.client.get(reverse("domain", kwargs={"pk": self.domain_no_information.id})) + detail_page = self.client.get(reverse("domain", kwargs={"domain_pk": self.domain_no_information.id})) self.assertContains(detail_page, "noinformation.gov") self.assertContains(detail_page, "Domain missing domain information") @@ -341,7 +341,7 @@ class TestDomainDetail(TestDomainOverview): session["analyst_action_location"] = self.domain.id session.save() - detail_page = self.client.get(reverse("domain", kwargs={"pk": self.domain.id})) + detail_page = self.client.get(reverse("domain", kwargs={"domain_pk": self.domain.id})) self.assertNotContains( detail_page, "If you need to make updates, contact one of the listed domain managers." @@ -487,7 +487,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): ): self.assertEquals(self.domain_to_renew.state, Domain.State.UNKNOWN) detail_page = self.client.get( - reverse("domain", kwargs={"pk": self.domain_to_renew.id}), + reverse("domain", kwargs={"domain_pk": self.domain_to_renew.id}), ) self.assertContains(detail_page, "Expiring soon") @@ -530,7 +530,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): Domain, "is_expired", self.custom_is_expired_false ): detail_page = self.client.get( - reverse("domain", kwargs={"pk": domain_to_renew2.id}), + reverse("domain", kwargs={"domain_pk": domain_to_renew2.id}), ) self.assertContains(detail_page, "Contact one of the listed domain managers to renew the domain.") @@ -551,7 +551,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): Domain, "is_expired", self.custom_is_expired_false ): detail_page = self.client.get( - reverse("domain", kwargs={"pk": domain_to_renew3.id}), + reverse("domain", kwargs={"domain_pk": domain_to_renew3.id}), ) self.assertContains(detail_page, "Renew to maintain access") @@ -565,7 +565,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): ): # Grab the detail page detail_page = self.client.get( - reverse("domain", kwargs={"pk": self.domain_to_renew.id}), + reverse("domain", kwargs={"domain_pk": self.domain_to_renew.id}), ) # Make sure we see the link as a domain manager @@ -575,7 +575,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): self.assertContains(detail_page, "Renewal form") # Grab link to the renewal page - renewal_form_url = reverse("domain-renewal", kwargs={"pk": self.domain_to_renew.id}) + renewal_form_url = reverse("domain-renewal", kwargs={"domain_pk": self.domain_to_renew.id}) self.assertContains(detail_page, f'href="{renewal_form_url}"') # Simulate clicking the link @@ -595,7 +595,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): ): # Grab the detail page detail_page = self.client.get( - reverse("domain", kwargs={"pk": self.domain_to_renew.id}), + reverse("domain", kwargs={"domain_pk": self.domain_to_renew.id}), ) # Make sure we see the link as a domain manager @@ -605,7 +605,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): self.assertContains(detail_page, "Renewal form") # Grab link to the renewal page - renewal_form_url = reverse("domain-renewal", kwargs={"pk": self.domain_to_renew.id}) + renewal_form_url = reverse("domain-renewal", kwargs={"domain_pk": self.domain_to_renew.id}) self.assertContains(detail_page, f'href="{renewal_form_url}"') # Simulate clicking the link @@ -620,7 +620,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): Your Profile portion of the Renewal Form.""" with less_console_noise(): # Start on the Renewal page for the domain - renewal_page = self.app.get(reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id})) + renewal_page = self.app.get(reverse("domain-renewal", kwargs={"domain_pk": self.domain_with_ip.id})) # Verify we see "Your contact information" on the renewal form self.assertContains(renewal_page, "Your contact information") @@ -640,7 +640,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): Security Email portion of the Renewal Form.""" with less_console_noise(): # Start on the Renewal page for the domain - renewal_page = self.app.get(reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id})) + renewal_page = self.app.get(reverse("domain-renewal", kwargs={"domain_pk": self.domain_with_ip.id})) # Verify we see "Security email" on the renewal form self.assertContains(renewal_page, "Security email") @@ -649,7 +649,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): self.assertContains(renewal_page, "We strongly recommend that you provide a security email.") # Verify that the "Edit" button for Security email is there and links to correct URL - edit_button_url = reverse("domain-security-email", kwargs={"pk": self.domain_with_ip.id}) + edit_button_url = reverse("domain-security-email", kwargs={"domain_pk": self.domain_with_ip.id}) self.assertContains(renewal_page, f'href="{edit_button_url}"') # Simulate clicking on edit button @@ -663,13 +663,13 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): Domain Manager portion of the Renewal Form.""" with less_console_noise(): # Start on the Renewal page for the domain - renewal_page = self.app.get(reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id})) + renewal_page = self.app.get(reverse("domain-renewal", kwargs={"domain_pk": self.domain_with_ip.id})) # Verify we see "Domain managers" on the renewal form self.assertContains(renewal_page, "Domain managers") # Verify that the "Edit" button for Domain managers is there and links to correct URL - edit_button_url = reverse("domain-users", kwargs={"pk": self.domain_with_ip.id}) + edit_button_url = reverse("domain-users", kwargs={"domain_pk": self.domain_with_ip.id}) self.assertContains(renewal_page, f'href="{edit_button_url}"') # Simulate clicking on edit button @@ -683,7 +683,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): to access /renewal and that it should receive a 403.""" with less_console_noise(): # Start on the Renewal page for the domain - renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domain_not_expiring.id})) + renewal_page = self.client.get(reverse("domain-renewal", kwargs={"domain_pk": self.domain_not_expiring.id})) self.assertEqual(renewal_page.status_code, 403) @override_flag("domain_renewal", active=True) @@ -692,14 +692,14 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): with patch.object(Domain, "is_expired", self.custom_is_expired_true), patch.object( Domain, "is_expired", self.custom_is_expired_true ): - renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domain_no_domain_manager.id})) + renewal_page = self.client.get(reverse("domain-renewal", kwargs={"domain_pk": self.domain_no_domain_manager.id})) self.assertEqual(renewal_page.status_code, 403) @override_flag("domain_renewal", active=True) def test_ack_checkbox_not_checked(self): """If user don't check the checkbox, user should receive an error message.""" # Grab the renewal URL - renewal_url = reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id}) + renewal_url = reverse("domain-renewal", kwargs={"domain_pk": self.domain_with_ip.id}) # Test that the checkbox is not checked response = self.client.post(renewal_url, data={"submit_button": "next"}) @@ -713,17 +713,17 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): user should be redirected Domain Over page with an updated by 1 year expiration date""" # Grab the renewal URL with patch.object(Domain, "renew_domain", self.custom_renew_domain): - renewal_url = reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id}) + renewal_url = reverse("domain-renewal", kwargs={"domain_pk": self.domain_with_ip.id}) # Click the check, and submit response = self.client.post(renewal_url, data={"is_policy_acknowledged": "on", "submit_button": "next"}) # Check that it redirects after a successfully submits - self.assertRedirects(response, reverse("domain", kwargs={"pk": self.domain_with_ip.id})) + self.assertRedirects(response, reverse("domain", kwargs={"domain_pk": self.domain_with_ip.id})) # Check for the updated expiration formatted_new_expiration_date = self.expiration_date_one_year_out().strftime("%b. %-d, %Y") - redirect_response = self.client.get(reverse("domain", kwargs={"pk": self.domain_with_ip.id}), follow=True) + redirect_response = self.client.get(reverse("domain", kwargs={"domain_pk": self.domain_with_ip.id}), follow=True) self.assertContains(redirect_response, formatted_new_expiration_date) @@ -766,7 +766,7 @@ class TestDomainManagers(TestDomainOverview): @less_console_noise_decorator def test_domain_managers(self): - response = self.client.get(reverse("domain-users", kwargs={"pk": self.domain.id})) + response = self.client.get(reverse("domain-users", kwargs={"domain_pk": self.domain.id})) self.assertContains(response, "Domain managers") self.assertContains(response, "Add a domain manager") # assert that the non-portfolio view contains Role column and doesn't contain Admin @@ -777,7 +777,7 @@ class TestDomainManagers(TestDomainOverview): @less_console_noise_decorator @override_flag("organization_feature", active=True) def test_domain_managers_portfolio_view(self): - response = self.client.get(reverse("domain-users", kwargs={"pk": self.domain.id})) + response = self.client.get(reverse("domain-users", kwargs={"domain_pk": self.domain.id})) self.assertContains(response, "Domain managers") self.assertContains(response, "Add a domain manager") # assert that the portfolio view doesn't contain Role column and does contain Admin @@ -787,7 +787,7 @@ class TestDomainManagers(TestDomainOverview): @less_console_noise_decorator def test_domain_user_add(self): - response = self.client.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + response = self.client.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) self.assertContains(response, "Add a domain manager") @less_console_noise_decorator @@ -796,7 +796,7 @@ class TestDomainManagers(TestDomainOverview): """Adding an existing user works.""" get_user_model().objects.get_or_create(email="mayor@igorville.gov") user = User.objects.filter(email="mayor@igorville.gov").first() - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = "mayor@igorville.gov" @@ -816,7 +816,7 @@ class TestDomainManagers(TestDomainOverview): self.assertEqual(success_result.status_code, 302) self.assertEqual( success_result["Location"], - reverse("domain-users", kwargs={"pk": self.domain.id}), + reverse("domain-users", kwargs={"domain_pk": self.domain.id}), ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -832,7 +832,7 @@ class TestDomainManagers(TestDomainOverview): """Adding an existing user works and sends portfolio invitation when user is not member of portfolio.""" get_user_model().objects.get_or_create(email="mayor@igorville.gov") - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = "mayor@igorville.gov" @@ -844,7 +844,7 @@ class TestDomainManagers(TestDomainOverview): self.assertEqual(success_result.status_code, 302) self.assertEqual( success_result["Location"], - reverse("domain-users", kwargs={"pk": self.domain.id}), + reverse("domain-users", kwargs={"domain_pk": self.domain.id}), ) # Verify that the invitation emails were sent @@ -889,7 +889,7 @@ class TestDomainManagers(TestDomainOverview): self, mock_send_domain_email, mock_send_portfolio_email ): """Adding an email not associated with a user works and sends portfolio invitation.""" - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = "notauser@igorville.gov" @@ -901,7 +901,7 @@ class TestDomainManagers(TestDomainOverview): self.assertEqual(success_result.status_code, 302) self.assertEqual( success_result["Location"], - reverse("domain-users", kwargs={"pk": self.domain.id}), + reverse("domain-users", kwargs={"domain_pk": self.domain.id}), ) # Verify that the invitation emails were sent @@ -940,7 +940,7 @@ class TestDomainManagers(TestDomainOverview): ): """Adding an email not associated with a user works and sends portfolio invitation, and when domain managers email(s) fail to send, assert proper warning displayed.""" - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = "notauser@igorville.gov" @@ -954,7 +954,7 @@ class TestDomainManagers(TestDomainOverview): self.assertEqual(success_result.status_code, 302) self.assertEqual( success_result["Location"], - reverse("domain-users", kwargs={"pk": self.domain.id}), + reverse("domain-users", kwargs={"domain_pk": self.domain.id}), ) # Verify that the invitation emails were sent @@ -979,7 +979,7 @@ class TestDomainManagers(TestDomainOverview): UserPortfolioPermission.objects.get_or_create( user=other_user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = "mayor@igorville.gov" @@ -991,7 +991,7 @@ class TestDomainManagers(TestDomainOverview): self.assertEqual(success_result.status_code, 302) self.assertEqual( success_result["Location"], - reverse("domain-users", kwargs={"pk": self.domain.id}), + reverse("domain-users", kwargs={"domain_pk": self.domain.id}), ) # Verify that the invitation emails were sent @@ -1027,7 +1027,7 @@ class TestDomainManagers(TestDomainOverview): user is not member of portfolio and send raises an error.""" mock_send_portfolio_email.side_effect = EmailSendingError("Failed to send email.") get_user_model().objects.get_or_create(email="mayor@igorville.gov") - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = "mayor@igorville.gov" @@ -1039,7 +1039,7 @@ class TestDomainManagers(TestDomainOverview): self.assertEqual(success_result.status_code, 302) self.assertEqual( success_result["Location"], - reverse("domain-users", kwargs={"pk": self.domain.id}), + reverse("domain-users", kwargs={"domain_pk": self.domain.id}), ) # Verify that the invitation emails were sent @@ -1070,7 +1070,7 @@ class TestDomainManagers(TestDomainOverview): """Removing a domain manager sends notification email to other domain managers.""" self.manager, _ = User.objects.get_or_create(email="mayor@igorville.com", first_name="Hello", last_name="World") self.manager_domain_permission, _ = UserDomainRole.objects.get_or_create(user=self.manager, domain=self.domain) - self.client.post(reverse("domain-user-delete", kwargs={"pk": self.domain.id, "user_pk": self.manager.id})) + self.client.post(reverse("domain-user-delete", kwargs={"domain_pk": self.domain.id, "user_pk": self.manager.id})) # Verify that the notification emails were sent to domain manager mock_send_templated_email.assert_called_once_with( @@ -1094,7 +1094,7 @@ class TestDomainManagers(TestDomainOverview): self.domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain) - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = email_address self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1127,7 +1127,7 @@ class TestDomainManagers(TestDomainOverview): self.domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain) - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = caps_email_address self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1159,7 +1159,7 @@ class TestDomainManagers(TestDomainOverview): mock_client = MagicMock() mock_client_instance = mock_client.return_value with boto3_mocking.clients.handler_for("sesv2", mock_client): - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = email_address self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1187,7 +1187,7 @@ class TestDomainManagers(TestDomainOverview): mock_client_instance = mock_client.return_value with boto3_mocking.clients.handler_for("sesv2", mock_client): - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = email_address self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1226,7 +1226,7 @@ class TestDomainManagers(TestDomainOverview): mock_client_instance = mock_client.return_value with boto3_mocking.clients.handler_for("sesv2", mock_client): - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = email_address self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1270,7 +1270,7 @@ class TestDomainManagers(TestDomainOverview): mock_client_instance = mock_client.return_value with boto3_mocking.clients.handler_for("sesv2", mock_client): - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = email_address self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1301,7 +1301,7 @@ class TestDomainManagers(TestDomainOverview): email_address = "mayor" self.domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain) - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = email_address self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1325,7 +1325,7 @@ class TestDomainManagers(TestDomainOverview): self.domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain) with patch("django.contrib.messages.error") as mock_error: - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] add_page.form["email"] = email_address self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1344,7 +1344,7 @@ class TestDomainManagers(TestDomainOverview): """Posting to the delete view deletes an invitation.""" email_address = "mayor@igorville.gov" invitation, _ = DomainInvitation.objects.get_or_create(domain=self.domain, email=email_address) - self.client.post(reverse("invitation-cancel", kwargs={"pk": invitation.id})) + self.client.post(reverse("invitation-cancel", kwargs={"domain_invitation_pk": invitation.id})) invitation = DomainInvitation.objects.get(id=invitation.id) self.assertEqual(invitation.status, DomainInvitation.DomainInvitationStatus.CANCELED) @@ -1355,7 +1355,7 @@ class TestDomainManagers(TestDomainOverview): invitation, _ = DomainInvitation.objects.get_or_create( domain=self.domain, email=email_address, status=DomainInvitation.DomainInvitationStatus.RETRIEVED ) - response = self.client.post(reverse("invitation-cancel", kwargs={"pk": invitation.id}), follow=True) + response = self.client.post(reverse("invitation-cancel", kwargs={"domain_invitation_pk": invitation.id}), follow=True) # Assert that an error message is displayed to the user self.assertContains(response, f"Invitation to {email_address} has already been retrieved.") # Assert that the Cancel link (form) is not displayed @@ -1375,7 +1375,7 @@ class TestDomainManagers(TestDomainOverview): self.client.force_login(other_user) mock_client = MagicMock() with boto3_mocking.clients.handler_for("sesv2", mock_client): - result = self.client.post(reverse("invitation-cancel", kwargs={"pk": invitation.id})) + result = self.client.post(reverse("invitation-cancel", kwargs={"domain_invitation_pk": invitation.id})) self.assertEqual(result.status_code, 403) @@ -1392,7 +1392,7 @@ class TestDomainManagers(TestDomainOverview): title = "title" User.objects.filter(email=email_address).delete() - add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + add_page = self.app.get(reverse("domain-users-add", kwargs={"domain_pk": self.domain.id})) self.domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain) @@ -1430,7 +1430,7 @@ class TestDomainManagers(TestDomainOverview): ) UserDomainRole.objects.create(user=new_user_2, domain=self.domain, role=UserDomainRole.Roles.MANAGER) response = self.client.post( - reverse("domain-user-delete", kwargs={"pk": self.domain.id, "user_pk": new_user.id}), follow=True + reverse("domain-user-delete", kwargs={"domain_pk": self.domain.id, "user_pk": new_user.id}), follow=True ) # Assert that a success message is displayed to the user self.assertContains(response, f"Removed {email_address} as a manager for this domain.") @@ -1444,7 +1444,7 @@ class TestDomainManagers(TestDomainOverview): """Posting to the delete view attempts to delete a user domain role when there is only one manager.""" # self.user is the only domain manager, so attempt to delete it response = self.client.post( - reverse("domain-user-delete", kwargs={"pk": self.domain.id, "user_pk": self.user.id}), follow=True + reverse("domain-user-delete", kwargs={"domain_pk": self.domain.id, "user_pk": self.user.id}), follow=True ) # Assert that an error message is displayed to the user self.assertContains(response, "Domains must have at least one domain manager.") @@ -1461,7 +1461,7 @@ class TestDomainManagers(TestDomainOverview): new_user = User.objects.create(email=email_address, username="mayor") UserDomainRole.objects.create(user=new_user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) response = self.client.post( - reverse("domain-user-delete", kwargs={"pk": self.domain.id, "user_pk": self.user.id}), follow=True + reverse("domain-user-delete", kwargs={"domain_pk": self.domain.id, "user_pk": self.user.id}), follow=True ) # Assert that a success message is displayed to the user self.assertContains(response, f"You are no longer managing the domain {self.domain}.") @@ -1473,7 +1473,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): @less_console_noise_decorator def test_domain_nameservers(self): """Can load domain's nameservers page.""" - page = self.client.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + page = self.client.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) self.assertContains(page, "DNS name servers") @less_console_noise_decorator @@ -1483,7 +1483,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): Uses self.app WebTest because we need to interact with forms. """ # initial nameservers page has one server with two ips - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # attempt to submit the form with only one nameserver, should error @@ -1506,7 +1506,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): Uses self.app WebTest because we need to interact with forms. """ # initial nameservers page has one server with two ips - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # attempt to submit the form without two hosts, both subdomains, @@ -1531,7 +1531,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): Uses self.app WebTest because we need to interact with forms. """ # initial nameservers page has one server with two ips - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # attempt to submit the form without two hosts, both subdomains, @@ -1555,7 +1555,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): Uses self.app WebTest because we need to interact with forms. """ # initial nameservers page has one server with two ips - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # attempt to submit the form with duplicate host names of fake.host.com @@ -1583,7 +1583,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): valid_ip = "1.1. 1.1" valid_ip_2 = "2.2. 2.2" # have to throw an error in order to test that the whitespace has been stripped from ip - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # attempt to submit the form without one host and an ip with whitespace @@ -1597,7 +1597,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): self.assertEqual(result.status_code, 302) self.assertEqual( result["Location"], - reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id}), + reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id}), ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) page = result.follow() @@ -1616,7 +1616,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): nameserver2 = "ns2.igorville.com" valid_ip = "127.0.0.1" # initial nameservers page has one server with two ips - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # attempt to submit the form without two hosts, both subdomains, @@ -1644,7 +1644,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): nameserver = "ns2.igorville.gov" invalid_ip = "123" # initial nameservers page has one server with two ips - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # attempt to submit the form without two hosts, both subdomains, @@ -1671,7 +1671,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): nameserver = "invalid-nameserver.gov" valid_ip = "123.2.45.111" # initial nameservers page has one server with two ips - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # attempt to submit the form without two hosts, both subdomains, @@ -1699,7 +1699,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): nameserver2 = "ns2.igorville.gov" valid_ip = "127.0.0.1" valid_ip_2 = "128.0.0.2" - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) nameservers_page.form["form-0-server"] = nameserver1 @@ -1711,7 +1711,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): self.assertEqual(result.status_code, 302) self.assertEqual( result["Location"], - reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id}), + reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id}), ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) page = result.follow() @@ -1731,7 +1731,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): valid_ip = "" valid_ip_2 = "128.0.0.2" valid_ip_3 = "128.0.0.3" - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) nameservers_page.form["form-0-server"] = nameserver1 @@ -1746,7 +1746,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): self.assertEqual(result.status_code, 302) self.assertEqual( result["Location"], - reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id}), + reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id}), ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) nameservers_page = result.follow() @@ -1772,7 +1772,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): self.assertEqual(result.status_code, 302) self.assertEqual( result["Location"], - reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id}), + reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id}), ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) nameservers_page = result.follow() @@ -1797,7 +1797,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): valid_ip_3 = "" valid_ip_4 = "" nameservers_page = self.app.get( - reverse("domain-dns-nameservers", kwargs={"pk": self.domain_with_four_nameservers.id}) + reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain_with_four_nameservers.id}) ) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] @@ -1821,7 +1821,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): self.assertEqual(result.status_code, 302) self.assertEqual( result["Location"], - reverse("domain-dns-nameservers", kwargs={"pk": self.domain_with_four_nameservers.id}), + reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain_with_four_nameservers.id}), ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) nameservers_page = result.follow() @@ -1833,7 +1833,7 @@ class TestDomainNameservers(TestDomainOverview, MockEppLib): Uses self.app WebTest because we need to interact with forms. """ - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # first two nameservers are required, so if we empty one out we should @@ -1855,7 +1855,7 @@ class TestDomainSeniorOfficial(TestDomainOverview): @less_console_noise_decorator def test_domain_senior_official(self): """Can load domain's senior official page.""" - page = self.client.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + page = self.client.get(reverse("domain-senior-official", kwargs={"domain_pk": self.domain.id})) self.assertContains(page, "Senior official", count=4) @less_console_noise_decorator @@ -1864,7 +1864,7 @@ class TestDomainSeniorOfficial(TestDomainOverview): self.domain_information.senior_official = Contact(first_name="Testy") self.domain_information.senior_official.save() self.domain_information.save() - page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + page = self.app.get(reverse("domain-senior-official", kwargs={"domain_pk": self.domain.id})) self.assertContains(page, "Testy") @less_console_noise_decorator @@ -1876,7 +1876,7 @@ class TestDomainSeniorOfficial(TestDomainOverview): ) self.domain_information.senior_official.save() self.domain_information.save() - so_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + so_page = self.app.get(reverse("domain-senior-official", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) so_form = so_page.forms[0] @@ -1934,7 +1934,7 @@ class TestDomainSeniorOfficial(TestDomainOverview): self.domain_information.senior_official.save() self.domain_information.save() - so_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + so_page = self.app.get(reverse("domain-senior-official", kwargs={"domain_pk": self.domain.id})) self.assertContains(so_page, "Apple Tester") self.assertContains(so_page, "CIO") self.assertContains(so_page, "nobody@igorville.gov") @@ -1955,7 +1955,7 @@ class TestDomainSeniorOfficial(TestDomainOverview): self.domain_information.senior_official.save() self.domain_information.save() - so_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + so_page = self.app.get(reverse("domain-senior-official", kwargs={"domain_pk": self.domain.id})) self.assertContains(so_page, "Apple Tester") self.assertContains(so_page, "CIO") self.assertContains(so_page, "nobody@igorville.gov") @@ -1974,7 +1974,7 @@ class TestDomainSeniorOfficial(TestDomainOverview): self.domain_information.other_contacts.add(self.domain_information.senior_official) self.domain_information.save() # load the Senior Official in the web form - so_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + so_page = self.app.get(reverse("domain-senior-official", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) so_form = so_page.forms[0] @@ -2002,7 +2002,7 @@ class TestDomainOrganization(TestDomainOverview): @less_console_noise_decorator def test_domain_org_name_address(self): """Can load domain's org name and mailing address page.""" - page = self.client.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + page = self.client.get(reverse("domain-org-name-address", kwargs={"domain_pk": self.domain.id})) # once on the sidebar, once in the page title, once as H1 self.assertContains(page, "/org-name-address") self.assertContains(page, "Organization name and mailing address") @@ -2013,7 +2013,7 @@ class TestDomainOrganization(TestDomainOverview): """Org name and address information appears on the page.""" self.domain_information.organization_name = "Town of Igorville" self.domain_information.save() - page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + page = self.app.get(reverse("domain-org-name-address", kwargs={"domain_pk": self.domain.id})) self.assertContains(page, "Town of Igorville") @less_console_noise_decorator @@ -2021,7 +2021,7 @@ class TestDomainOrganization(TestDomainOverview): """Submitting changes works on the org name address page.""" self.domain_information.organization_name = "Town of Igorville" self.domain_information.save() - org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] org_name_page.form["organization_name"] = "Not igorville" @@ -2053,7 +2053,7 @@ class TestDomainOrganization(TestDomainOverview): self.assertEqual(self.domain_information.generic_org_type, tribal_org_type) - org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"domain_pk": self.domain.id})) form = org_name_page.forms[0] # Check the value of the input field @@ -2110,7 +2110,7 @@ class TestDomainOrganization(TestDomainOverview): self.assertEqual(self.domain_information.generic_org_type, fed_org_type) - org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"domain_pk": self.domain.id})) form = org_name_page.forms[0] # Check the value of the input field @@ -2172,7 +2172,7 @@ class TestDomainOrganization(TestDomainOverview): new_value = ("Department of State", "Department of State") self.client.post( - reverse("domain-org-name-address", kwargs={"pk": self.domain.id}), + reverse("domain-org-name-address", kwargs={"domain_pk": self.domain.id}), { "federal_agency": new_value, }, @@ -2214,7 +2214,7 @@ class TestDomainSuborganization(TestDomainOverview): self.assertEqual(self.domain_information.sub_organization, suborg) # Navigate to the suborganization page - page = self.app.get(reverse("domain-suborganization", kwargs={"pk": self.domain.id})) + page = self.app.get(reverse("domain-suborganization", kwargs={"domain_pk": self.domain.id})) # The page should contain the choices Vanilla and Chocolate self.assertContains(page, "Vanilla") @@ -2272,7 +2272,7 @@ class TestDomainSuborganization(TestDomainOverview): self.assertEqual(self.domain_information.sub_organization, suborg) # Navigate to the suborganization page - page = self.app.get(reverse("domain-suborganization", kwargs={"pk": self.domain.id})) + page = self.app.get(reverse("domain-suborganization", kwargs={"domain_pk": self.domain.id})) # The page should display the readonly option self.assertContains(page, "Vanilla") @@ -2311,7 +2311,7 @@ class TestDomainSuborganization(TestDomainOverview): self.user.refresh_from_db() # Navigate to the domain overview page - page = self.app.get(reverse("domain", kwargs={"pk": self.domain.id})) + page = self.app.get(reverse("domain", kwargs={"domain_pk": self.domain.id})) # Test for the title change self.assertContains(page, "Suborganization") @@ -2340,7 +2340,7 @@ class TestDomainSecurityEmail(TestDomainOverview): domain_contact, _ = Domain.objects.get_or_create(name="freeman.gov") # Add current user to this domain _ = UserDomainRole(user=self.user, domain=domain_contact, role="admin").save() - page = self.client.get(reverse("domain-security-email", kwargs={"pk": domain_contact.id})) + page = self.client.get(reverse("domain-security-email", kwargs={"domain_pk": domain_contact.id})) # Loads correctly self.assertContains(page, "Security email") @@ -2355,7 +2355,7 @@ class TestDomainSecurityEmail(TestDomainOverview): self.mockedSendFunction = self.mockSendPatch.start() self.mockedSendFunction.side_effect = self.mockSend - page = self.client.get(reverse("domain-security-email", kwargs={"pk": self.domain.id})) + page = self.client.get(reverse("domain-security-email", kwargs={"domain_pk": self.domain.id})) # Loads correctly self.assertContains(page, "Security email") @@ -2365,7 +2365,7 @@ class TestDomainSecurityEmail(TestDomainOverview): def test_domain_security_email(self): """Can load domain's security email page.""" with less_console_noise(): - page = self.client.get(reverse("domain-security-email", kwargs={"pk": self.domain.id})) + page = self.client.get(reverse("domain-security-email", kwargs={"domain_pk": self.domain.id})) self.assertContains(page, "Security email") def test_domain_security_email_form(self): @@ -2373,7 +2373,7 @@ class TestDomainSecurityEmail(TestDomainOverview): Uses self.app WebTest because we need to interact with forms. """ with less_console_noise(): - security_email_page = self.app.get(reverse("domain-security-email", kwargs={"pk": self.domain.id})) + security_email_page = self.app.get(reverse("domain-security-email", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] security_email_page.form["security_email"] = "mayor@igorville.gov" self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -2384,7 +2384,7 @@ class TestDomainSecurityEmail(TestDomainOverview): self.assertEqual(result.status_code, 302) self.assertEqual( result["Location"], - reverse("domain-security-email", kwargs={"pk": self.domain.id}), + reverse("domain-security-email", kwargs={"domain_pk": self.domain.id}), ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -2427,7 +2427,7 @@ class TestDomainSecurityEmail(TestDomainOverview): ] for test_name, data, expected_message in test_cases: response = self.client.post( - reverse("domain-security-email", kwargs={"pk": self.domain.id}), + reverse("domain-security-email", kwargs={"domain_pk": self.domain.id}), data=data, follow=True, ) @@ -2455,7 +2455,7 @@ class TestDomainSecurityEmail(TestDomainOverview): management pages share the same permissions class""" self.user.status = User.RESTRICTED self.user.save() - response = self.client.get(reverse("domain", kwargs={"pk": self.domain.id})) + response = self.client.get(reverse("domain", kwargs={"domain_pk": self.domain.id})) self.assertEqual(response.status_code, 403) @@ -2467,7 +2467,7 @@ class TestDomainDNSSEC(TestDomainOverview): """DNSSEC overview page loads when domain has no DNSSEC data and shows a 'Enable DNSSEC' button.""" - page = self.client.get(reverse("domain-dns-dnssec", kwargs={"pk": self.domain.id})) + page = self.client.get(reverse("domain-dns-dnssec", kwargs={"domain_pk": self.domain.id})) self.assertContains(page, "Enable DNSSEC") @less_console_noise_decorator @@ -2475,7 +2475,7 @@ class TestDomainDNSSEC(TestDomainOverview): """DNSSEC overview page loads when domain has DNSSEC data and the template contains a button to disable DNSSEC.""" - page = self.client.get(reverse("domain-dns-dnssec", kwargs={"pk": self.domain_multdsdata.id})) + page = self.client.get(reverse("domain-dns-dnssec", kwargs={"domain_pk": self.domain_multdsdata.id})) self.assertContains(page, "Disable DNSSEC") # Prepare the data for the POST request @@ -2483,7 +2483,7 @@ class TestDomainDNSSEC(TestDomainOverview): "disable_dnssec": "Disable DNSSEC", } updated_page = self.client.post( - reverse("domain-dns-dnssec", kwargs={"pk": self.domain.id}), + reverse("domain-dns-dnssec", kwargs={"domain_pk": self.domain.id}), post_data, follow=True, ) @@ -2497,7 +2497,7 @@ class TestDomainDNSSEC(TestDomainOverview): """DNSSEC Add DS data page loads when there is no domain DNSSEC data and shows a button to Add new record""" - page = self.client.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dnssec_none.id})) + page = self.client.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dnssec_none.id})) self.assertContains(page, "You have no DS data added") self.assertContains(page, "Add new record") @@ -2506,13 +2506,13 @@ class TestDomainDNSSEC(TestDomainOverview): """DNSSEC Add DS data page loads when there is domain DNSSEC DS data and shows the data""" - page = self.client.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id})) + page = self.client.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id})) self.assertContains(page, "DS data record 1") @less_console_noise_decorator def test_ds_data_form_modal(self): """When user clicks on save, a modal pops up.""" - add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id})) + add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id})) # Assert that a hidden trigger for the modal does not exist. # This hidden trigger will pop on the page when certain condition are met: # 1) Initial form contained DS data, 2) All data is deleted and form is @@ -2521,7 +2521,7 @@ class TestDomainDNSSEC(TestDomainOverview): # Simulate a delete all data form_data = {} response = self.client.post( - reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id}), + reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id}), data=form_data, ) self.assertEqual(response.status_code, 200) # Adjust status code as needed @@ -2534,7 +2534,7 @@ class TestDomainDNSSEC(TestDomainOverview): Uses self.app WebTest because we need to interact with forms. """ - add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id})) + add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) result = add_data_page.forms[0].submit() @@ -2542,7 +2542,7 @@ class TestDomainDNSSEC(TestDomainOverview): self.assertEqual(result.status_code, 302) self.assertEqual( result["Location"], - reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id}), + reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id}), ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) page = result.follow() @@ -2554,7 +2554,7 @@ class TestDomainDNSSEC(TestDomainOverview): Uses self.app WebTest because we need to interact with forms. """ - add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id})) + add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # all four form fields are required, so will test with each blank @@ -2577,7 +2577,7 @@ class TestDomainDNSSEC(TestDomainOverview): Uses self.app WebTest because we need to interact with forms. """ - add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id})) + add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # first two nameservers are required, so if we empty one out we should @@ -2600,7 +2600,7 @@ class TestDomainDNSSEC(TestDomainOverview): Uses self.app WebTest because we need to interact with forms. """ - add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id})) + add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # first two nameservers are required, so if we empty one out we should @@ -2623,7 +2623,7 @@ class TestDomainDNSSEC(TestDomainOverview): Uses self.app WebTest because we need to interact with forms. """ - add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id})) + add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # first two nameservers are required, so if we empty one out we should @@ -2646,7 +2646,7 @@ class TestDomainDNSSEC(TestDomainOverview): Uses self.app WebTest because we need to interact with forms. """ - add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain_dsdata.id})) + add_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain_dsdata.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # first two nameservers are required, so if we empty one out we should @@ -2700,7 +2700,7 @@ class TestDomainChangeNotifications(TestDomainOverview): self.domain_information.zipcode = "62052" self.domain_information.save() - org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] org_name_page.form["organization_name"] = "Not igorville" @@ -2741,7 +2741,7 @@ class TestDomainChangeNotifications(TestDomainOverview): self.domain_information.portfolio = portfolio self.domain_information.save() - org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] org_name_page.form["organization_name"] = "Not igorville" @@ -2768,7 +2768,7 @@ class TestDomainChangeNotifications(TestDomainOverview): self.domain_information.portfolio = portfolio self.domain_information.save() - org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] session = self.app.session @@ -2790,7 +2790,7 @@ class TestDomainChangeNotifications(TestDomainOverview): def test_notification_on_security_email_change(self): """Test that an email is sent when the security email is changed.""" - security_email_page = self.app.get(reverse("domain-security-email", kwargs={"pk": self.domain.id})) + security_email_page = self.app.get(reverse("domain-security-email", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] security_email_page.form["security_email"] = "new_security@example.com" @@ -2813,7 +2813,7 @@ class TestDomainChangeNotifications(TestDomainOverview): def test_notification_on_dnssec_enable(self): """Test that an email is sent when DNSSEC is enabled.""" - page = self.client.get(reverse("domain-dns-dnssec", kwargs={"pk": self.domain_multdsdata.id})) + page = self.client.get(reverse("domain-dns-dnssec", kwargs={"domain_pk": self.domain_multdsdata.id})) self.assertContains(page, "Disable DNSSEC") # Prepare the data for the POST request @@ -2823,7 +2823,7 @@ class TestDomainChangeNotifications(TestDomainOverview): with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): updated_page = self.client.post( - reverse("domain-dns-dnssec", kwargs={"pk": self.domain.id}), + reverse("domain-dns-dnssec", kwargs={"domain_pk": self.domain.id}), post_data, follow=True, ) @@ -2846,7 +2846,7 @@ class TestDomainChangeNotifications(TestDomainOverview): def test_notification_on_ds_data_change(self): """Test that an email is sent when DS data is changed.""" - ds_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain.id})) + ds_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] # Add DS data @@ -2880,7 +2880,7 @@ class TestDomainChangeNotifications(TestDomainOverview): ) self.domain_information.save() - senior_official_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + senior_official_page = self.app.get(reverse("domain-senior-official", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] senior_official_page.form["first_name"] = "New" @@ -2917,7 +2917,7 @@ class TestDomainChangeNotifications(TestDomainOverview): self.domain_information.portfolio = portfolio self.domain_information.save() - senior_official_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + senior_official_page = self.app.get(reverse("domain-senior-official", kwargs={"domain_pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] senior_official_page.form["first_name"] = "New" @@ -2936,7 +2936,7 @@ class TestDomainChangeNotifications(TestDomainOverview): def test_no_notification_when_dns_needed(self): """Test that an email is not sent when nameservers are changed while the state is DNS_NEEDED.""" - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain_dns_needed.id})) + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain_dns_needed.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] # add nameservers diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 7a76284a4..1f0285314 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -2,6 +2,7 @@ from datetime import date import logging import requests from django.contrib import messages +from django.contrib.auth.mixins import PermissionRequiredMixin from django.contrib.messages.views import SuccessMessageMixin from django.http import HttpResponseRedirect from django.shortcuts import redirect, render, get_object_or_404 @@ -73,7 +74,7 @@ from django import forms logger = logging.getLogger(__name__) -class DomainBaseView(DetailView): +class DomainBaseView(PermissionRequiredMixin, DetailView): """ Base View for the Domain. Handles getting and setting the domain in session cache on GETs. Also provides methods for getting @@ -174,7 +175,6 @@ class DomainBaseView(DetailView): def in_editable_state(self, pk): """Is the domain in an editable state""" - requested_domain = None if Domain.objects.filter(id=pk).exists(): requested_domain = Domain.objects.get(id=pk) @@ -1240,7 +1240,7 @@ class DomainUsersView(DomainBaseView): """Domain managers page in the domain details.""" template_name = "domain_users.html" - + def get_context_data(self, **kwargs): """The initial value for the form (which is a formset here).""" context = super().get_context_data(**kwargs) diff --git a/src/registrar/views/utility/api_views.py b/src/registrar/views/utility/api_views.py index aedfa757a..fbbe72f01 100644 --- a/src/registrar/views/utility/api_views.py +++ b/src/registrar/views/utility/api_views.py @@ -1,6 +1,7 @@ import logging from django.http import JsonResponse from django.forms.models import model_to_dict +from registrar.decorators import IS_STAFF, grant_access from registrar.models import FederalAgency, SeniorOfficial, DomainRequest from django.contrib.admin.views.decorators import staff_member_required from django.contrib.auth.decorators import login_required @@ -11,8 +12,7 @@ from registrar.utility.constants import BranchChoices logger = logging.getLogger(__name__) -@login_required -@staff_member_required +@grant_access(IS_STAFF) def get_senior_official_from_federal_agency_json(request): """Returns federal_agency information as a JSON""" @@ -39,8 +39,7 @@ def get_senior_official_from_federal_agency_json(request): return JsonResponse({"error": "Senior Official not found"}, status=404) -@login_required -@staff_member_required +@grant_access(IS_STAFF) def get_portfolio_json(request): """Returns portfolio information as a JSON""" @@ -96,8 +95,7 @@ def get_portfolio_json(request): return JsonResponse(portfolio_dict) -@login_required -@staff_member_required +@grant_access(IS_STAFF) def get_suborganization_list_json(request): """Returns suborganization list information for a portfolio as a JSON""" @@ -119,8 +117,7 @@ def get_suborganization_list_json(request): return JsonResponse({"results": results, "pagination": {"more": False}}) -@login_required -@staff_member_required +@grant_access(IS_STAFF) def get_federal_and_portfolio_types_from_federal_agency_json(request): """Returns specific portfolio information as a JSON. Request must have both agency_name and organization_type.""" @@ -148,8 +145,7 @@ def get_federal_and_portfolio_types_from_federal_agency_json(request): return JsonResponse(response_data) -@login_required -@staff_member_required +@grant_access(IS_STAFF) def get_action_needed_email_for_user_json(request): """Returns a default action needed email for a given user""" @@ -173,8 +169,7 @@ def get_action_needed_email_for_user_json(request): return JsonResponse({"email": email}, status=200) -@login_required -@staff_member_required +@grant_access(IS_STAFF) def get_rejection_email_for_user_json(request): """Returns a default rejection email for a given user""" From 6537c93e63e71153451e8d9d5d9c4bf0cedfab8f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 19:59:01 -0500 Subject: [PATCH 13/17] updated more tests --- src/registrar/decorators.py | 1 - src/registrar/tests/test_admin.py | 2 -- src/registrar/tests/test_url_auth.py | 1 - src/registrar/tests/test_views_domains_json.py | 6 +++--- src/registrar/tests/test_views_portfolio.py | 12 ++++++------ src/registrar/views/domain_request.py | 11 +++++++++-- src/registrar/views/utility/error_views.py | 1 - 7 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 517985b6d..37bdabccd 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -138,7 +138,6 @@ def _user_has_permission(user, request, rules, **kwargs): if not any(conditions_met) and HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT in rules: domain_request_id = kwargs.get("domain_request_pk") has_permission = _has_portfolio_domain_requests_edit(user, request, domain_request_id) - print(has_permission) conditions_met.append(has_permission) if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_ANY_PERM in rules: diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 9447d211f..aadb85c66 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -3834,8 +3834,6 @@ class TestTransferUser(WebTest): self.assertContains(after_submit, "

    Change user

    ") - print(mock_success_message.call_args_list) - mock_success_message.assert_any_call( ANY, ( diff --git a/src/registrar/tests/test_url_auth.py b/src/registrar/tests/test_url_auth.py index 17b13c233..7e0193e1d 100644 --- a/src/registrar/tests/test_url_auth.py +++ b/src/registrar/tests/test_url_auth.py @@ -85,7 +85,6 @@ def iter_sample_urls(urlconf): if not viewname: continue if viewname == "auth_user_password_change": - print(route) break named_groups = route.regex.groupindex.keys() kwargs = {} diff --git a/src/registrar/tests/test_views_domains_json.py b/src/registrar/tests/test_views_domains_json.py index fe63f27de..5ce308e74 100644 --- a/src/registrar/tests/test_views_domains_json.py +++ b/src/registrar/tests/test_views_domains_json.py @@ -104,7 +104,7 @@ class GetDomainsJsonTest(TestWithUser, WebTest): self.assertEqual(expected_domain.state_display(), state_displays[i]) self.assertEqual(expected_domain.get_state_help_text(), get_state_help_texts[i]) - self.assertEqual(reverse("domain", kwargs={"pk": expected_domain.id}), action_urls[i]) + self.assertEqual(reverse("domain", kwargs={"domain_pk": expected_domain.id}), action_urls[i]) # Check action_label action_label_expected = ( @@ -185,7 +185,7 @@ class GetDomainsJsonTest(TestWithUser, WebTest): self.assertEqual(expected_domain.state_display(), state_displays[i]) self.assertEqual(expected_domain.get_state_help_text(), get_state_help_texts[i]) - self.assertEqual(reverse("domain", kwargs={"pk": expected_domain.id}), action_urls[i]) + self.assertEqual(reverse("domain", kwargs={"domain_pk": expected_domain.id}), action_urls[i]) # Check action_label user_domain_role_exists = UserDomainRole.objects.filter( @@ -272,7 +272,7 @@ class GetDomainsJsonTest(TestWithUser, WebTest): self.assertEqual(expected_domain.state_display(), state_displays[i]) self.assertEqual(expected_domain.get_state_help_text(), get_state_help_texts[i]) - self.assertEqual(reverse("domain", kwargs={"pk": expected_domain.id}), action_urls[i]) + self.assertEqual(reverse("domain", kwargs={"domain_pk": expected_domain.id}), action_urls[i]) # Check action_label user_domain_role_exists = UserDomainRole.objects.filter( diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 097aa1879..9de6fbbf2 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1320,7 +1320,7 @@ class TestPortfolio(WebTest): self.client.force_login(self.user) # Perform delete - response = self.client.post(reverse("domain-request-delete", kwargs={"pk": domain_request.pk}), follow=True) + response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) # Check that the response is 200 self.assertEqual(response.status_code, 200) @@ -1354,7 +1354,7 @@ class TestPortfolio(WebTest): self.client.force_login(self.user) # Attempt to delete - response = self.client.post(reverse("domain-request-delete", kwargs={"pk": domain_request.pk}), follow=True) + response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) # Check response is 403 Forbidden self.assertEqual(response.status_code, 403) @@ -1389,7 +1389,7 @@ class TestPortfolio(WebTest): self.client.force_login(self.user) # Perform delete as self.user - response = self.client.post(reverse("domain-request-delete", kwargs={"pk": domain_request.pk}), follow=True) + response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) # Check response is 403 Forbidden self.assertEqual(response.status_code, 403) @@ -3244,7 +3244,7 @@ class TestRequestingEntity(WebTest): def test_requesting_entity_page_errors(self): """Tests that we get the expected form errors on requesting entity""" domain_request = completed_domain_request(user=self.user, portfolio=self.portfolio) - response = self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})).follow() + response = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})).follow() form = response.forms[0] session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -3334,7 +3334,7 @@ class TestRequestingEntity(WebTest): domain_request.submit() - response = self.app.get(reverse("domain-request-status-viewonly", kwargs={"pk": domain_request.pk})) + response = self.app.get(reverse("domain-request-status-viewonly", kwargs={"domain_request_pk": domain_request.pk})) self.assertContains(response, "Requesting entity") self.assertContains(response, "moon") self.assertContains(response, "kepler, AL") @@ -3359,7 +3359,7 @@ class TestRequestingEntity(WebTest): domain_request.submit() - response = self.app.get(reverse("domain-request-status", kwargs={"pk": domain_request.pk})) + response = self.app.get(reverse("domain-request-status", kwargs={"domain_request_pk": domain_request.pk})) self.assertContains(response, "Requesting entity") self.assertContains(response, "moon") self.assertContains(response, "kepler, AL") diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 23596f2ad..7a0ce6d3c 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -1,12 +1,13 @@ import logging from collections import defaultdict +from django.contrib import messages +from django.contrib.auth.mixins import PermissionRequiredMixin from django.http import Http404, HttpResponse, HttpResponseRedirect from django.shortcuts import redirect, render from django.urls import resolve, reverse from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ from django.views.generic import DeleteView, DetailView, TemplateView -from django.contrib import messages from registrar.decorators import ( HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT, HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL, @@ -880,7 +881,7 @@ class DomainRequestWithdrawn(DetailView): @grant_access(IS_DOMAIN_REQUEST_CREATOR, HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT) -class DomainRequestDeleteView(DeleteView): +class DomainRequestDeleteView(PermissionRequiredMixin, DeleteView): """Delete view for home that allows the end user to delete DomainRequests""" object: DomainRequest # workaround for type mismatch in DeleteView @@ -895,6 +896,12 @@ class DomainRequestDeleteView(DeleteView): if status not in valid_statuses: return False + # Portfolio users cannot delete their requests if they aren't permissioned to do so + if self.request.user.is_org_user(self.request): + portfolio = self.request.session.get("portfolio") + if not self.request.user.has_edit_request_portfolio_permission(portfolio): + return False + return True def get_success_url(self): diff --git a/src/registrar/views/utility/error_views.py b/src/registrar/views/utility/error_views.py index 4f9635d09..8e54170c9 100644 --- a/src/registrar/views/utility/error_views.py +++ b/src/registrar/views/utility/error_views.py @@ -39,7 +39,6 @@ def custom_403_error_view(request, exception=None, context=None): def custom_404_error_view(request, exception=None, context=None): """Used to redirect 404 errors to a custom view""" - print("this is called") if context is None: context = {} return render(request, "404.html", context=context, status=404) From 86fbac82496800d47896b8c34efdc4fd394e6ec2 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 21:10:22 -0500 Subject: [PATCH 14/17] updated tests --- src/registrar/decorators.py | 2 +- src/registrar/tests/test_views.py | 16 +-- src/registrar/tests/test_views_request.py | 116 +++++++++--------- .../tests/test_views_requests_json.py | 8 +- src/registrar/views/domain_request.py | 4 +- 5 files changed, 71 insertions(+), 75 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 37bdabccd..158139218 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -120,7 +120,7 @@ def _user_has_permission(user, request, rules, **kwargs): if not any(conditions_met) and IS_DOMAIN_REQUEST_CREATOR in rules: domain_request_id = kwargs.get("domain_request_pk") - has_permission = _is_domain_request_creator(user, domain_request_id) + has_permission = _is_domain_request_creator(user, domain_request_id) and not _is_portfolio_member(request) conditions_met.append(has_permission) if not any(conditions_met) and HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM in rules: diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 2dfead13f..7521719f8 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -126,7 +126,7 @@ class TestEnvironmentVariablesEffects(TestCase): with patch.object(DomainNameserversView, "get_initial", side_effect=self.side_effect_raise_value_error): with self.assertRaises(ValueError): contact_page_500 = self.client.get( - reverse("domain-dns-nameservers", kwargs={"pk": fake_domain.id}), + reverse("domain-dns-nameservers", kwargs={"domain_pk": fake_domain.id}), ) # Check that a 500 response is returned @@ -147,7 +147,7 @@ class TestEnvironmentVariablesEffects(TestCase): with patch.object(DomainNameserversView, "get_initial", side_effect=self.side_effect_raise_value_error): with self.assertRaises(ValueError): contact_page_500 = self.client.get( - reverse("domain-dns-nameservers", kwargs={"pk": fake_domain.id}), + reverse("domain-dns-nameservers", kwargs={"domain_pk": fake_domain.id}), ) # Check that a 500 response is returned @@ -292,7 +292,7 @@ class HomeTests(TestWithUser): ) # Trigger the delete logic - response = self.client.post(reverse("domain-request-delete", kwargs={"pk": domain_request.pk}), follow=True) + response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) self.assertNotContains(response, "igorville.gov") @@ -309,7 +309,7 @@ class HomeTests(TestWithUser): ) # Trigger the delete logic - response = self.client.post(reverse("domain-request-delete", kwargs={"pk": domain_request.pk}), follow=True) + response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) self.assertNotContains(response, "igorville.gov") @@ -335,7 +335,7 @@ class HomeTests(TestWithUser): # Trigger the delete logic response = self.client.post( - reverse("domain-request-delete", kwargs={"pk": domain_request.pk}), follow=True + reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True ) # Check for a 403 error - the end user should not be allowed to do this @@ -392,7 +392,7 @@ class HomeTests(TestWithUser): self.assertTrue(igorville.exists()) # Trigger the delete logic - self.client.post(reverse("domain-request-delete", kwargs={"pk": domain_request.pk})) + self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk})) # igorville is now deleted igorville = DomainRequest.objects.filter(requested_domain__name="igorville.gov") @@ -462,7 +462,7 @@ class HomeTests(TestWithUser): self.assertTrue(teaville.exists()) # Trigger the delete logic - self.client.post(reverse("domain-request-delete", kwargs={"pk": domain_request_2.pk})) + self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request_2.pk})) teaville = DomainRequest.objects.filter(requested_domain__name="teaville.gov") self.assertFalse(teaville.exists()) @@ -935,7 +935,7 @@ class UserProfileTests(TestWithUser, WebTest): @less_console_noise_decorator def test_domain_detail_contains_your_profile(self): """Tests that the domain detail view contains 'your profile' rather than 'your contact information'""" - response = self.client.get(reverse("domain", args=[self.domain.pk])) + response = self.client.get(reverse("domain", kwargs={"domain_pk": self.domain.pk})) self.assertContains(response, "Your profile") self.assertNotContains(response, "Your contact information") diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index f6eba2a56..d70300f9c 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -809,7 +809,7 @@ class DomainRequestTests(TestWithUser, WebTest): # type_page = home_page.click("Edit") session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] - url = reverse("edit-domain-request", kwargs={"id": domain_request.pk}) + url = reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # TODO: The following line results in a django error on middleware @@ -1106,7 +1106,7 @@ class DomainRequestTests(TestWithUser, WebTest): def test_yes_no_contact_form_inits_blank_for_new_domain_request(self): """On the Other Contacts page, the yes/no form gets initialized with nothing selected for new domain requests""" - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": 0})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": 0})) other_contacts_form = other_contacts_page.forms[0] self.assertEquals(other_contacts_form["other_contacts-has_other_contacts"].value, None) @@ -1114,7 +1114,7 @@ class DomainRequestTests(TestWithUser, WebTest): def test_yes_no_additional_form_inits_blank_for_new_domain_request(self): """On the Additional Details page, the yes/no form gets initialized with nothing selected for new domain requests""" - additional_details_page = self.app.get(reverse("domain-request:additional_details", kwargs={"id": 0})) + additional_details_page = self.app.get(reverse("domain-request:additional_details", kwargs={"domain_request_pk": 0})) additional_form = additional_details_page.forms[0] # Check the cisa representative yes/no field @@ -1130,7 +1130,7 @@ class DomainRequestTests(TestWithUser, WebTest): # Domain Request has other contacts by default domain_request = completed_domain_request(user=self.user) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1138,7 +1138,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.pk})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1155,7 +1155,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_request.save() # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1164,7 +1164,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) additional_details_page = self.app.get( - reverse("domain-request:additional_details", kwargs={"id": domain_request.pk}) + reverse("domain-request:additional_details", kwargs={"domain_request_pk": domain_request.pk}) ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1187,7 +1187,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_request.no_other_contacts_rationale = "Hello!" domain_request.save() # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1195,7 +1195,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.pk})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1216,7 +1216,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_request.save() # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1225,7 +1225,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) additional_details_page = self.app.get( - reverse("domain-request:additional_details", kwargs={"id": domain_request.pk}) + reverse("domain-request:additional_details", kwargs={"domain_request_pk": domain_request.pk}) ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1256,7 +1256,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.assertEqual(domain_request.cisa_representative_email, "fake@faketown.gov") # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1265,7 +1265,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) additional_details_page = self.app.get( - reverse("domain-request:additional_details", kwargs={"id": domain_request.pk}) + reverse("domain-request:additional_details", kwargs={"domain_request_pk": domain_request.pk}) ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1320,7 +1320,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.assertEqual(domain_request.has_cisa_representative, None) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1329,7 +1329,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) additional_details_page = self.app.get( - reverse("domain-request:additional_details", kwargs={"id": domain_request.pk}) + reverse("domain-request:additional_details", kwargs={"domain_request_pk": domain_request.pk}) ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1367,7 +1367,7 @@ class DomainRequestTests(TestWithUser, WebTest): ) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1376,7 +1376,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) additional_details_page = self.app.get( - reverse("domain-request:additional_details", kwargs={"id": domain_request.pk}) + reverse("domain-request:additional_details", kwargs={"domain_request_pk": domain_request.pk}) ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1400,7 +1400,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_request = completed_domain_request(name="cisareps.gov", user=self.user, has_anything_else=False) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1409,7 +1409,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) additional_details_page = self.app.get( - reverse("domain-request:additional_details", kwargs={"id": domain_request.pk}) + reverse("domain-request:additional_details", kwargs={"domain_request_pk": domain_request.pk}) ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1439,7 +1439,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.assertEqual(domain_request.has_cisa_representative, None) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1448,7 +1448,7 @@ class DomainRequestTests(TestWithUser, WebTest): self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) additional_details_page = self.app.get( - reverse("domain-request:additional_details", kwargs={"id": domain_request.id}) + reverse("domain-request:additional_details", kwargs={"domain_request_pk": domain_request.id}) ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -1472,7 +1472,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_request.no_other_contacts_rationale = "Hello!" domain_request.save() # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1480,7 +1480,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.pk})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1520,7 +1520,7 @@ class DomainRequestTests(TestWithUser, WebTest): # Domain request has other contacts by default domain_request = completed_domain_request(user=self.user) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1528,7 +1528,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.pk})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1604,7 +1604,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_info.other_contacts.set([other]) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1612,7 +1612,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.pk})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1653,7 +1653,7 @@ class DomainRequestTests(TestWithUser, WebTest): @less_console_noise_decorator def test_if_yes_no_form_is_no_then_no_other_contacts_required(self): """Applicants with no other contacts have to give a reason.""" - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": 0})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": 0})) other_contacts_form = other_contacts_page.forms[0] other_contacts_form["other_contacts-has_other_contacts"] = "False" response = other_contacts_page.forms[0].submit() @@ -1669,7 +1669,7 @@ class DomainRequestTests(TestWithUser, WebTest): @less_console_noise_decorator def test_if_yes_no_form_is_yes_then_other_contacts_required(self): """Applicants with other contacts do not have to give a reason.""" - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": 0})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": 0})) other_contacts_form = other_contacts_page.forms[0] other_contacts_form["other_contacts-has_other_contacts"] = "True" response = other_contacts_page.forms[0].submit() @@ -1737,7 +1737,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_request.other_contacts.add(other2) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1745,7 +1745,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.id})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.id})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1810,7 +1810,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_request.other_contacts.add(other) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1818,7 +1818,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.id})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.id})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1887,7 +1887,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_request.other_contacts.add(other) # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1895,7 +1895,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.id})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.id})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1967,7 +1967,7 @@ class DomainRequestTests(TestWithUser, WebTest): other_contact_pk = other.id # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -1975,7 +1975,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.pk})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -2043,7 +2043,7 @@ class DomainRequestTests(TestWithUser, WebTest): other_contact_pk = so.id # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -2051,7 +2051,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"id": domain_request.pk})) + other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -2113,7 +2113,7 @@ class DomainRequestTests(TestWithUser, WebTest): so_pk = so.id # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -2121,7 +2121,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - so_page = self.app.get(reverse("domain-request:senior_official", kwargs={"id": domain_request.pk})) + so_page = self.app.get(reverse("domain-request:senior_official", kwargs={"domain_request_pk": domain_request.pk})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) so_form = so_page.forms[0] @@ -2182,7 +2182,7 @@ class DomainRequestTests(TestWithUser, WebTest): so_pk = so.id # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -2190,7 +2190,7 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - so_page = self.app.get(reverse("domain-request:senior_official", kwargs={"id": domain_request.pk})) + so_page = self.app.get(reverse("domain-request:senior_official", kwargs={"domain_request_pk": domain_request.pk})) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) so_form = so_page.forms[0] @@ -2240,7 +2240,7 @@ class DomainRequestTests(TestWithUser, WebTest): creator_pk = self.user.id # prime the form by visiting /edit - self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})) + self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})) # django-webtest does not handle cookie-based sessions well because it keeps # resetting the session key on each new request, thus destroying the concept # of a "session". We are going to do it manually, saving the session ID here @@ -2539,7 +2539,7 @@ class DomainRequestTests(TestWithUser, WebTest): new_domain_request_id = all_domain_requests.first().id # Skip to the current sites page - current_sites_page = self.app.get(reverse("domain-request:current_sites", kwargs={"id": new_domain_request_id})) + current_sites_page = self.app.get(reverse("domain-request:current_sites", kwargs={"domain_request_pk": new_domain_request_id})) # fill in the form field current_sites_form = current_sites_page.forms[0] self.assertIn("current_sites-0-website", current_sites_form.fields) @@ -2614,7 +2614,7 @@ class DomainRequestTests(TestWithUser, WebTest): domain_request.alternative_domains.add(alt) # prime the form by visiting /edit - url = reverse("edit-domain-request", kwargs={"id": domain_request.pk}) + url = reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}) response = self.client.get(url) # TODO: this is a sketch of each page in the wizard which needs to be tested @@ -2724,7 +2724,7 @@ class DomainRequestTests(TestWithUser, WebTest): NOTE: This may be a moot point if we implement a more solid pattern in the future, like not a submit action at all on the review page.""" - review_page = self.app.get(reverse("domain-request:review", kwargs={"id": 0})) + review_page = self.app.get(reverse("domain-request:review", kwargs={"domain_request_pk": 0})) self.assertContains(review_page, "toggle-submit-domain-request") self.assertContains(review_page, "Your request form is incomplete") @@ -2742,7 +2742,7 @@ class DomainRequestTests(TestWithUser, WebTest): # This user should also be forbidden from editing existing ones domain_request = completed_domain_request(user=self.user) - edit_page = self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk}), expect_errors=True) + edit_page = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}), expect_errors=True) self.assertEqual(edit_page.status_code, 403) # Cleanup @@ -2767,7 +2767,7 @@ class DomainRequestTests(TestWithUser, WebTest): # This user should also be allowed to edit existing ones domain_request = completed_domain_request(user=self.user) - edit_page = self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})).follow() + edit_page = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})).follow() self.assertEqual(edit_page.status_code, 200) def test_non_creator_access(self): @@ -2776,14 +2776,14 @@ class DomainRequestTests(TestWithUser, WebTest): other_user = User.objects.create_user(username="other_user", password=p) domain_request = completed_domain_request(user=other_user) - edit_page = self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk}), expect_errors=True) + edit_page = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}), expect_errors=True) self.assertEqual(edit_page.status_code, 403) def test_creator_access(self): """Tests that a user can edit a domain request they created""" domain_request = completed_domain_request(user=self.user) - edit_page = self.app.get(reverse("edit-domain-request", kwargs={"id": domain_request.pk})).follow() + edit_page = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})).follow() self.assertEqual(edit_page.status_code, 200) @@ -2898,12 +2898,8 @@ class DomainRequestTestDifferentStatuses(TestWithUser, WebTest): domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.SUBMITTED, user=self.user) domain_request.save() - detail_page = self.app.get(f"/domain-request/{domain_request.id}") - self.assertContains(detail_page, "city.gov") - self.assertContains(detail_page, "city1.gov") - self.assertContains(detail_page, "Chief Tester") - self.assertContains(detail_page, "testy@town.com") - self.assertContains(detail_page, "Status:") + detail_page = self.client.get(f"/domain-request/{domain_request.id}") + self.assertEqual(detail_page.status_code, 403) # Restricted user trying to withdraw results in 403 error with less_console_noise(): for url_name in [ @@ -2911,7 +2907,7 @@ class DomainRequestTestDifferentStatuses(TestWithUser, WebTest): "domain-request-withdrawn", ]: with self.subTest(url_name=url_name): - page = self.client.get(reverse(url_name, kwargs={"pk": domain_request.pk})) + page = self.client.get(reverse(url_name, kwargs={"domain_request_pk": domain_request.pk})) self.assertEqual(page.status_code, 403) @less_console_noise_decorator @@ -2931,7 +2927,7 @@ class DomainRequestTestDifferentStatuses(TestWithUser, WebTest): "domain-request-withdrawn", ]: with self.subTest(url_name=url_name): - page = self.client.get(reverse(url_name, kwargs={"pk": domain_request.pk})) + page = self.client.get(reverse(url_name, kwargs={"domain_request_pk": domain_request.pk})) self.assertEqual(page.status_code, 403) @less_console_noise_decorator @@ -3206,7 +3202,7 @@ class TestDomainRequestWizard(TestWithUser, WebTest): self.assertContains(detail_page, "usa-current", count=2) # We default to the requesting entity page - expected_url = reverse("domain-request:portfolio_requesting_entity", kwargs={"id": domain_request.id}) + expected_url = reverse("domain-request:portfolio_requesting_entity", kwargs={"domain_request_pk": domain_request.id}) # This returns the entire url, thus "in" self.assertIn(expected_url, detail_page.request.url) else: diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index e25d76bf7..55d9141ab 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -199,14 +199,14 @@ class GetRequestsJsonTest(TestWithUser, WebTest): # Check action_url action_url_expected = ( - reverse("edit-domain-request", kwargs={"id": self.domain_requests[i].id}) + reverse("edit-domain-request", kwargs={"domain_request_pk": self.domain_requests[i].id}) if self.domain_requests[i].status in [ DomainRequest.DomainRequestStatus.STARTED, DomainRequest.DomainRequestStatus.ACTION_NEEDED, DomainRequest.DomainRequestStatus.WITHDRAWN, ] - else reverse("domain-request-status", kwargs={"pk": self.domain_requests[i].id}) + else reverse("domain-request-status", kwargs={"domain_request_pk": self.domain_requests[i].id}) ) self.assertEqual(action_url_expected, action_urls[i]) @@ -342,7 +342,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): # Test case where action is View self.assertEqual("View", action_labels[i]) self.assertEqual( - reverse("domain-request-status-viewonly", kwargs={"pk": expected_domain_request.id}), action_urls[i] + reverse("domain-request-status-viewonly", kwargs={"domain_request_pk": expected_domain_request.id}), action_urls[i] ) self.assertEqual("visibility", svg_icons[i]) elif status[i] in [ @@ -360,7 +360,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): # Test case where action is Manage self.assertEqual("Manage", action_labels[i]) self.assertEqual( - reverse("domain-request-status", kwargs={"pk": expected_domain_request.id}), action_urls[i] + reverse("domain-request-status", kwargs={"domain_request_pk": expected_domain_request.id}), action_urls[i] ) self.assertEqual("settings", svg_icons[i]) diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 7a0ce6d3c..1d17e3047 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -843,7 +843,7 @@ class DomainRequestStatus(DetailView): return context -@grant_access(IS_DOMAIN_REQUEST_CREATOR) +@grant_access(IS_DOMAIN_REQUEST_CREATOR, HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT) class DomainRequestWithdrawConfirmation(DetailView): """This page will ask user to confirm if they want to withdraw @@ -857,7 +857,7 @@ class DomainRequestWithdrawConfirmation(DetailView): context_object_name = "DomainRequest" -@grant_access(IS_DOMAIN_REQUEST_CREATOR) +@grant_access(IS_DOMAIN_REQUEST_CREATOR, HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT) class DomainRequestWithdrawn(DetailView): # this view renders no template template_name = "" From 51dc7797b0aa813161a8be4a6b5bea5725ab1146 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 12 Feb 2025 21:30:21 -0500 Subject: [PATCH 15/17] updated code for readability --- src/djangooidc/views.py | 2 - src/registrar/decorators.py | 150 ++++++++---------- .../tests/test_management_scripts.py | 1 - src/registrar/tests/test_views.py | 11 +- src/registrar/tests/test_views_domain.py | 20 ++- src/registrar/tests/test_views_portfolio.py | 20 ++- src/registrar/tests/test_views_request.py | 76 ++++++--- .../tests/test_views_requests_json.py | 6 +- src/registrar/views/domain.py | 2 +- src/registrar/views/domain_requests_json.py | 1 - src/registrar/views/domains_json.py | 1 - src/registrar/views/utility/api_views.py | 2 - 12 files changed, 167 insertions(+), 125 deletions(-) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 984936a4c..815df4ecf 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -5,14 +5,12 @@ import logging from django.conf import settings from django.contrib.auth import logout as auth_logout from django.contrib.auth import authenticate, login -from login_required import login_not_required from django.http import HttpResponseRedirect from django.shortcuts import redirect from urllib.parse import parse_qs, urlencode from djangooidc.oidc import Client from djangooidc import exceptions as o_e -from registrar.decorators import grant_access from registrar.models import User from registrar.views.utility.error_views import custom_500_error_view, custom_401_error_view diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 158139218..1147fe943 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -74,91 +74,74 @@ def _user_has_permission(user, request, rules, **kwargs): if ALL in rules: return True - # Ensure user is authenticated - if not user.is_authenticated: + # Ensure user is authenticated and not restricted + if not user.is_authenticated or user.is_restricted(): return False - # Ensure user is not restricted - if user.is_restricted(): - return False + # Define permission checks + permission_checks = [ + (IS_STAFF, lambda: user.is_staff), + (IS_DOMAIN_MANAGER, lambda: _is_domain_manager(user, **kwargs)), + (IS_STAFF_MANAGING_DOMAIN, lambda: _is_staff_managing_domain(request, **kwargs)), + (IS_PORTFOLIO_MEMBER, lambda: user.is_org_user(request)), + ( + HAS_PORTFOLIO_DOMAINS_VIEW_ALL, + lambda: _can_access_domain_via_portfolio_view_all_domains(request, kwargs.get("domain_pk")), + ), + ( + HAS_PORTFOLIO_DOMAINS_ANY_PERM, + lambda: user.is_org_user(request) + and user.has_any_domains_portfolio_permission(request.session.get("portfolio")), + ), + ( + IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER, + lambda: _is_domain_manager(user, **kwargs) and _is_portfolio_member(request), + ), + ( + IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER, + lambda: _is_domain_manager(user, **kwargs) and not _is_portfolio_member(request), + ), + ( + IS_DOMAIN_REQUEST_CREATOR, + lambda: _is_domain_request_creator(user, kwargs.get("domain_request_pk")) + and not _is_portfolio_member(request), + ), + ( + HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM, + lambda: user.is_org_user(request) + and user.has_any_requests_portfolio_permission(request.session.get("portfolio")), + ), + ( + HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL, + lambda: user.is_org_user(request) + and user.has_view_all_domain_requests_portfolio_permission(request.session.get("portfolio")), + ), + ( + HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT, + lambda: _has_portfolio_domain_requests_edit(user, request, kwargs.get("domain_request_pk")), + ), + ( + HAS_PORTFOLIO_MEMBERS_ANY_PERM, + lambda: user.is_org_user(request) + and ( + user.has_view_members_portfolio_permission(request.session.get("portfolio")) + or user.has_edit_members_portfolio_permission(request.session.get("portfolio")) + ), + ), + ( + HAS_PORTFOLIO_MEMBERS_EDIT, + lambda: user.is_org_user(request) + and user.has_edit_members_portfolio_permission(request.session.get("portfolio")), + ), + ( + HAS_PORTFOLIO_MEMBERS_VIEW, + lambda: user.is_org_user(request) + and user.has_view_members_portfolio_permission(request.session.get("portfolio")), + ), + ] - conditions_met = [] - - if IS_STAFF in rules: - conditions_met.append(user.is_staff) - - if not any(conditions_met) and IS_DOMAIN_MANAGER in rules: - has_permission = _is_domain_manager(user, **kwargs) - conditions_met.append(has_permission) - - if not any(conditions_met) and IS_STAFF_MANAGING_DOMAIN in rules: - has_permission = _is_staff_managing_domain(request, **kwargs) - conditions_met.append(has_permission) - - if not any(conditions_met) and IS_PORTFOLIO_MEMBER in rules: - has_permission = user.is_org_user(request) - conditions_met.append(has_permission) - - if not any(conditions_met) and HAS_PORTFOLIO_DOMAINS_VIEW_ALL in rules: - domain_id = kwargs.get("domain_pk") - has_permission = _can_access_domain_via_portfolio_view_all_domains(request, domain_id) - conditions_met.append(has_permission) - - if not any(conditions_met) and HAS_PORTFOLIO_DOMAINS_ANY_PERM in rules: - has_permission = user.is_org_user(request) and user.has_any_domains_portfolio_permission( - request.session.get("portfolio") - ) - conditions_met.append(has_permission) - - if not any(conditions_met) and IS_PORTFOLIO_MEMBER_AND_DOMAIN_MANAGER in rules: - has_permission = _is_domain_manager(user, **kwargs) and _is_portfolio_member(request) - conditions_met.append(has_permission) - - if not any(conditions_met) and IS_DOMAIN_MANAGER_AND_NOT_PORTFOLIO_MEMBER in rules: - has_permission = _is_domain_manager(user, **kwargs) and not _is_portfolio_member(request) - conditions_met.append(has_permission) - - if not any(conditions_met) and IS_DOMAIN_REQUEST_CREATOR in rules: - domain_request_id = kwargs.get("domain_request_pk") - has_permission = _is_domain_request_creator(user, domain_request_id) and not _is_portfolio_member(request) - conditions_met.append(has_permission) - - if not any(conditions_met) and HAS_PORTFOLIO_DOMAIN_REQUESTS_ANY_PERM in rules: - has_permission = user.is_org_user(request) and user.has_any_requests_portfolio_permission( - request.session.get("portfolio") - ) - conditions_met.append(has_permission) - - if not any(conditions_met) and HAS_PORTFOLIO_DOMAIN_REQUESTS_VIEW_ALL in rules: - has_permission = user.is_org_user(request) and user.has_view_all_domain_requests_portfolio_permission( - request.session.get("portfolio") - ) - conditions_met.append(has_permission) - - if not any(conditions_met) and HAS_PORTFOLIO_DOMAIN_REQUESTS_EDIT in rules: - domain_request_id = kwargs.get("domain_request_pk") - has_permission = _has_portfolio_domain_requests_edit(user, request, domain_request_id) - conditions_met.append(has_permission) - - if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_ANY_PERM in rules: - portfolio = request.session.get("portfolio") - has_permission = user.is_org_user(request) and ( - user.has_view_members_portfolio_permission(portfolio) - or user.has_edit_members_portfolio_permission(portfolio) - ) - conditions_met.append(has_permission) - - if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_EDIT in rules: - portfolio = request.session.get("portfolio") - has_permission = user.is_org_user(request) and user.has_edit_members_portfolio_permission(portfolio) - conditions_met.append(has_permission) - - if not any(conditions_met) and HAS_PORTFOLIO_MEMBERS_VIEW in rules: - portfolio = request.session.get("portfolio") - has_permission = user.is_org_user(request) and user.has_view_members_portfolio_permission(portfolio) - conditions_met.append(has_permission) - - return any(conditions_met) + # Check conditions iteratively + return any(check() for rule, check in permission_checks if rule in rules) def _has_portfolio_domain_requests_edit(user, request, domain_request_id): @@ -173,7 +156,8 @@ def _is_domain_manager(user, **kwargs): - First, it checks if 'domain_pk' is present in the URL parameters. - If 'domain_pk' exists, it verifies if the user has a domain role for that domain. - - If 'domain_pk' is absent, it checks for 'domain_invitation_pk' to determine if the user has domain permissions through an invitation. + - If 'domain_pk' is absent, it checks for 'domain_invitation_pk' to determine if the user + has domain permissions through an invitation. Returns: bool: True if the user is a domain manager, False otherwise. diff --git a/src/registrar/tests/test_management_scripts.py b/src/registrar/tests/test_management_scripts.py index 668eeff0e..17e4736c4 100644 --- a/src/registrar/tests/test_management_scripts.py +++ b/src/registrar/tests/test_management_scripts.py @@ -37,7 +37,6 @@ from epplibwrapper import commands, common from .common import ( MockEppLib, - less_console_noise, completed_domain_request, MockSESClient, MockDbForIndividualTests, diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 7521719f8..3f5413e5a 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -292,7 +292,9 @@ class HomeTests(TestWithUser): ) # Trigger the delete logic - response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) + response = self.client.post( + reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True + ) self.assertNotContains(response, "igorville.gov") @@ -309,7 +311,9 @@ class HomeTests(TestWithUser): ) # Trigger the delete logic - response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) + response = self.client.post( + reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True + ) self.assertNotContains(response, "igorville.gov") @@ -335,7 +339,8 @@ class HomeTests(TestWithUser): # Trigger the delete logic response = self.client.post( - reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True + reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), + follow=True, ) # Check for a 403 error - the end user should not be allowed to do this diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index d9aac8178..676c4ef5f 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -692,7 +692,9 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): with patch.object(Domain, "is_expired", self.custom_is_expired_true), patch.object( Domain, "is_expired", self.custom_is_expired_true ): - renewal_page = self.client.get(reverse("domain-renewal", kwargs={"domain_pk": self.domain_no_domain_manager.id})) + renewal_page = self.client.get( + reverse("domain-renewal", kwargs={"domain_pk": self.domain_no_domain_manager.id}) + ) self.assertEqual(renewal_page.status_code, 403) @override_flag("domain_renewal", active=True) @@ -723,7 +725,9 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): # Check for the updated expiration formatted_new_expiration_date = self.expiration_date_one_year_out().strftime("%b. %-d, %Y") - redirect_response = self.client.get(reverse("domain", kwargs={"domain_pk": self.domain_with_ip.id}), follow=True) + redirect_response = self.client.get( + reverse("domain", kwargs={"domain_pk": self.domain_with_ip.id}), follow=True + ) self.assertContains(redirect_response, formatted_new_expiration_date) @@ -1070,7 +1074,9 @@ class TestDomainManagers(TestDomainOverview): """Removing a domain manager sends notification email to other domain managers.""" self.manager, _ = User.objects.get_or_create(email="mayor@igorville.com", first_name="Hello", last_name="World") self.manager_domain_permission, _ = UserDomainRole.objects.get_or_create(user=self.manager, domain=self.domain) - self.client.post(reverse("domain-user-delete", kwargs={"domain_pk": self.domain.id, "user_pk": self.manager.id})) + self.client.post( + reverse("domain-user-delete", kwargs={"domain_pk": self.domain.id, "user_pk": self.manager.id}) + ) # Verify that the notification emails were sent to domain manager mock_send_templated_email.assert_called_once_with( @@ -1355,7 +1361,9 @@ class TestDomainManagers(TestDomainOverview): invitation, _ = DomainInvitation.objects.get_or_create( domain=self.domain, email=email_address, status=DomainInvitation.DomainInvitationStatus.RETRIEVED ) - response = self.client.post(reverse("invitation-cancel", kwargs={"domain_invitation_pk": invitation.id}), follow=True) + response = self.client.post( + reverse("invitation-cancel", kwargs={"domain_invitation_pk": invitation.id}), follow=True + ) # Assert that an error message is displayed to the user self.assertContains(response, f"Invitation to {email_address} has already been retrieved.") # Assert that the Cancel link (form) is not displayed @@ -2936,7 +2944,9 @@ class TestDomainChangeNotifications(TestDomainOverview): def test_no_notification_when_dns_needed(self): """Test that an email is not sent when nameservers are changed while the state is DNS_NEEDED.""" - nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain_dns_needed.id})) + nameservers_page = self.app.get( + reverse("domain-dns-nameservers", kwargs={"domain_pk": self.domain_dns_needed.id}) + ) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] # add nameservers diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 9de6fbbf2..530908f1f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1320,7 +1320,9 @@ class TestPortfolio(WebTest): self.client.force_login(self.user) # Perform delete - response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) + response = self.client.post( + reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True + ) # Check that the response is 200 self.assertEqual(response.status_code, 200) @@ -1354,7 +1356,9 @@ class TestPortfolio(WebTest): self.client.force_login(self.user) # Attempt to delete - response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) + response = self.client.post( + reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True + ) # Check response is 403 Forbidden self.assertEqual(response.status_code, 403) @@ -1389,7 +1393,9 @@ class TestPortfolio(WebTest): self.client.force_login(self.user) # Perform delete as self.user - response = self.client.post(reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True) + response = self.client.post( + reverse("domain-request-delete", kwargs={"domain_request_pk": domain_request.pk}), follow=True + ) # Check response is 403 Forbidden self.assertEqual(response.status_code, 403) @@ -3244,7 +3250,9 @@ class TestRequestingEntity(WebTest): def test_requesting_entity_page_errors(self): """Tests that we get the expected form errors on requesting entity""" domain_request = completed_domain_request(user=self.user, portfolio=self.portfolio) - response = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})).follow() + response = self.app.get( + reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}) + ).follow() form = response.forms[0] session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -3334,7 +3342,9 @@ class TestRequestingEntity(WebTest): domain_request.submit() - response = self.app.get(reverse("domain-request-status-viewonly", kwargs={"domain_request_pk": domain_request.pk})) + response = self.app.get( + reverse("domain-request-status-viewonly", kwargs={"domain_request_pk": domain_request.pk}) + ) self.assertContains(response, "Requesting entity") self.assertContains(response, "moon") self.assertContains(response, "kepler, AL") diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index d70300f9c..6818759c5 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -1114,7 +1114,9 @@ class DomainRequestTests(TestWithUser, WebTest): def test_yes_no_additional_form_inits_blank_for_new_domain_request(self): """On the Additional Details page, the yes/no form gets initialized with nothing selected for new domain requests""" - additional_details_page = self.app.get(reverse("domain-request:additional_details", kwargs={"domain_request_pk": 0})) + additional_details_page = self.app.get( + reverse("domain-request:additional_details", kwargs={"domain_request_pk": 0}) + ) additional_form = additional_details_page.forms[0] # Check the cisa representative yes/no field @@ -1138,7 +1140,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1195,7 +1199,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1480,7 +1486,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1528,7 +1536,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1612,7 +1622,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1745,7 +1757,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.id})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.id}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1818,7 +1832,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.id})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.id}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1895,7 +1911,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.id})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.id}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -1975,7 +1993,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -2051,7 +2071,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - other_contacts_page = self.app.get(reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk})) + other_contacts_page = self.app.get( + reverse("domain-request:other_contacts", kwargs={"domain_request_pk": domain_request.pk}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) other_contacts_form = other_contacts_page.forms[0] @@ -2121,7 +2143,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - so_page = self.app.get(reverse("domain-request:senior_official", kwargs={"domain_request_pk": domain_request.pk})) + so_page = self.app.get( + reverse("domain-request:senior_official", kwargs={"domain_request_pk": domain_request.pk}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) so_form = so_page.forms[0] @@ -2190,7 +2214,9 @@ class DomainRequestTests(TestWithUser, WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - so_page = self.app.get(reverse("domain-request:senior_official", kwargs={"domain_request_pk": domain_request.pk})) + so_page = self.app.get( + reverse("domain-request:senior_official", kwargs={"domain_request_pk": domain_request.pk}) + ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) so_form = so_page.forms[0] @@ -2539,7 +2565,9 @@ class DomainRequestTests(TestWithUser, WebTest): new_domain_request_id = all_domain_requests.first().id # Skip to the current sites page - current_sites_page = self.app.get(reverse("domain-request:current_sites", kwargs={"domain_request_pk": new_domain_request_id})) + current_sites_page = self.app.get( + reverse("domain-request:current_sites", kwargs={"domain_request_pk": new_domain_request_id}) + ) # fill in the form field current_sites_form = current_sites_page.forms[0] self.assertIn("current_sites-0-website", current_sites_form.fields) @@ -2742,7 +2770,9 @@ class DomainRequestTests(TestWithUser, WebTest): # This user should also be forbidden from editing existing ones domain_request = completed_domain_request(user=self.user) - edit_page = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}), expect_errors=True) + edit_page = self.app.get( + reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}), expect_errors=True + ) self.assertEqual(edit_page.status_code, 403) # Cleanup @@ -2767,7 +2797,9 @@ class DomainRequestTests(TestWithUser, WebTest): # This user should also be allowed to edit existing ones domain_request = completed_domain_request(user=self.user) - edit_page = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})).follow() + edit_page = self.app.get( + reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}) + ).follow() self.assertEqual(edit_page.status_code, 200) def test_non_creator_access(self): @@ -2776,14 +2808,18 @@ class DomainRequestTests(TestWithUser, WebTest): other_user = User.objects.create_user(username="other_user", password=p) domain_request = completed_domain_request(user=other_user) - edit_page = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}), expect_errors=True) + edit_page = self.app.get( + reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}), expect_errors=True + ) self.assertEqual(edit_page.status_code, 403) def test_creator_access(self): """Tests that a user can edit a domain request they created""" domain_request = completed_domain_request(user=self.user) - edit_page = self.app.get(reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk})).follow() + edit_page = self.app.get( + reverse("edit-domain-request", kwargs={"domain_request_pk": domain_request.pk}) + ).follow() self.assertEqual(edit_page.status_code, 200) @@ -3202,7 +3238,9 @@ class TestDomainRequestWizard(TestWithUser, WebTest): self.assertContains(detail_page, "usa-current", count=2) # We default to the requesting entity page - expected_url = reverse("domain-request:portfolio_requesting_entity", kwargs={"domain_request_pk": domain_request.id}) + expected_url = reverse( + "domain-request:portfolio_requesting_entity", kwargs={"domain_request_pk": domain_request.id} + ) # This returns the entire url, thus "in" self.assertIn(expected_url, detail_page.request.url) else: diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index 55d9141ab..8f50e16bb 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -342,7 +342,8 @@ class GetRequestsJsonTest(TestWithUser, WebTest): # Test case where action is View self.assertEqual("View", action_labels[i]) self.assertEqual( - reverse("domain-request-status-viewonly", kwargs={"domain_request_pk": expected_domain_request.id}), action_urls[i] + reverse("domain-request-status-viewonly", kwargs={"domain_request_pk": expected_domain_request.id}), + action_urls[i], ) self.assertEqual("visibility", svg_icons[i]) elif status[i] in [ @@ -360,7 +361,8 @@ class GetRequestsJsonTest(TestWithUser, WebTest): # Test case where action is Manage self.assertEqual("Manage", action_labels[i]) self.assertEqual( - reverse("domain-request-status", kwargs={"domain_request_pk": expected_domain_request.id}), action_urls[i] + reverse("domain-request-status", kwargs={"domain_request_pk": expected_domain_request.id}), + action_urls[i], ) self.assertEqual("settings", svg_icons[i]) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 1f0285314..d424bd978 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1240,7 +1240,7 @@ class DomainUsersView(DomainBaseView): """Domain managers page in the domain details.""" template_name = "domain_users.html" - + def get_context_data(self, **kwargs): """The initial value for the form (which is a formset here).""" context = super().get_context_data(**kwargs) diff --git a/src/registrar/views/domain_requests_json.py b/src/registrar/views/domain_requests_json.py index fb0702a58..0533af66b 100644 --- a/src/registrar/views/domain_requests_json.py +++ b/src/registrar/views/domain_requests_json.py @@ -3,7 +3,6 @@ from django.core.paginator import Paginator from registrar.decorators import grant_access, ALL from registrar.models import DomainRequest from django.utils.dateformat import format -from django.contrib.auth.decorators import login_required from django.urls import reverse from django.db.models import Q diff --git a/src/registrar/views/domains_json.py b/src/registrar/views/domains_json.py index f23fdd3b4..676115904 100644 --- a/src/registrar/views/domains_json.py +++ b/src/registrar/views/domains_json.py @@ -3,7 +3,6 @@ from django.http import JsonResponse from django.core.paginator import Paginator from registrar.decorators import grant_access, ALL from registrar.models import UserDomainRole, Domain, DomainInformation, User -from django.contrib.auth.decorators import login_required from django.urls import reverse from django.db.models import Q diff --git a/src/registrar/views/utility/api_views.py b/src/registrar/views/utility/api_views.py index fbbe72f01..6d0a2b5ec 100644 --- a/src/registrar/views/utility/api_views.py +++ b/src/registrar/views/utility/api_views.py @@ -3,8 +3,6 @@ from django.http import JsonResponse from django.forms.models import model_to_dict from registrar.decorators import IS_STAFF, grant_access from registrar.models import FederalAgency, SeniorOfficial, DomainRequest -from django.contrib.admin.views.decorators import staff_member_required -from django.contrib.auth.decorators import login_required from registrar.utility.admin_helpers import get_action_needed_reason_default_email, get_rejection_reason_default_email from registrar.models.portfolio import Portfolio from registrar.utility.constants import BranchChoices From 37c673100164f54012e8043c5558c9158bbf3fc9 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 13 Feb 2025 06:47:11 -0500 Subject: [PATCH 16/17] updated comments --- src/registrar/decorators.py | 60 +++++++++++++++++---------- src/registrar/registrar_middleware.py | 26 ++++++++---- 2 files changed, 56 insertions(+), 30 deletions(-) diff --git a/src/registrar/decorators.py b/src/registrar/decorators.py index 1147fe943..7cb2792f4 100644 --- a/src/registrar/decorators.py +++ b/src/registrar/decorators.py @@ -24,37 +24,43 @@ HAS_PORTFOLIO_MEMBERS_VIEW = "has_portfolio_members_view" def grant_access(*rules): """ - Allows multiple rules in a single decorator call: - @grant_access(IS_STAFF, IS_SUPERUSER, IS_DOMAIN_MANAGER) - or multiple stacked decorators: - @grant_access(IS_SUPERUSER) - @grant_access(IS_DOMAIN_MANAGER) + A decorator that enforces access control based on specified rules. + + Usage: + - Multiple rules in a single decorator: + @grant_access(IS_STAFF, IS_SUPERUSER, IS_DOMAIN_MANAGER) + + - Stacked decorators for separate rules: + @grant_access(IS_SUPERUSER) + @grant_access(IS_DOMAIN_MANAGER) + + The decorator supports both function-based views (FBVs) and class-based views (CBVs). """ def decorator(view): - if isinstance(view, type): # If decorating a class-based view (CBV) - original_dispatch = view.dispatch # save original dispatch method + if isinstance(view, type): # Check if decorating a class-based view (CBV) + original_dispatch = view.dispatch # Store the original dispatch method - @method_decorator(grant_access(*rules)) # apply the decorator to dispatch + @method_decorator(grant_access(*rules)) # Apply the decorator to dispatch def wrapped_dispatch(self, request, *args, **kwargs): if not _user_has_permission(request.user, request, rules, **kwargs): - raise PermissionDenied + raise PermissionDenied # Deny access if the user lacks permission return original_dispatch(self, request, *args, **kwargs) - view.dispatch = wrapped_dispatch # replace dispatch with wrapped version + view.dispatch = wrapped_dispatch # Replace the dispatch method return view else: # If decorating a function-based view (FBV) - view.has_explicit_access = True - existing_rules = getattr(view, "_access_rules", set()) - existing_rules.update(rules) - view._access_rules = existing_rules + view.has_explicit_access = True # Mark the view as having explicit access control + existing_rules = getattr(view, "_access_rules", set()) # Retrieve existing rules + existing_rules.update(rules) # Merge with new rules + view._access_rules = existing_rules # Store updated rules @functools.wraps(view) def wrapper(request, *args, **kwargs): if not _user_has_permission(request.user, request, rules, **kwargs): - raise PermissionDenied - return view(request, *args, **kwargs) + raise PermissionDenied # Deny access if the user lacks permission + return view(request, *args, **kwargs) # Proceed with the original view return wrapper @@ -63,7 +69,21 @@ def grant_access(*rules): def _user_has_permission(user, request, rules, **kwargs): """ - Checks if the user meets the permission requirements. + Determines if the user meets the required permission rules. + + This function evaluates a set of predefined permission rules to check whether a user has access + to a specific view. It supports various access control conditions, including staff status, + domain management roles, and portfolio-related permissions. + + Parameters: + - user: The user requesting access. + - request: The HTTP request object. + - rules: A set of access control rules to evaluate. + - **kwargs: Additional keyword arguments used in specific permission checks. + + Returns: + - True if the user satisfies any of the specified rules. + - False otherwise. """ # Skip authentication if @login_not_required is applied @@ -86,7 +106,7 @@ def _user_has_permission(user, request, rules, **kwargs): (IS_PORTFOLIO_MEMBER, lambda: user.is_org_user(request)), ( HAS_PORTFOLIO_DOMAINS_VIEW_ALL, - lambda: _can_access_domain_via_portfolio_view_all_domains(request, kwargs.get("domain_pk")), + lambda: _has_portfolio_view_all_domains(request, kwargs.get("domain_pk")), ), ( HAS_PORTFOLIO_DOMAINS_ANY_PERM, @@ -268,11 +288,9 @@ def _is_staff_managing_domain(request, **kwargs): return True -def _can_access_domain_via_portfolio_view_all_domains(request, domain_pk): +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.""" - # NOTE: determine if in practice this ever needs to be called on its own - # or if it can be combined with view_managed_domains portfolio = request.session.get("portfolio") if request.user.has_view_all_domains_portfolio_permission(portfolio): if Domain.objects.filter(id=domain_pk).exists(): diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 8d4518e94..8c9c79876 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -177,40 +177,48 @@ class CheckPortfolioMiddleware: class RestrictAccessMiddleware: - """Middleware that blocks all views unless explicitly permitted""" + """ + Middleware that blocks access to all views unless explicitly permitted. + + This middleware enforces authentication by default. Views must explicitly allow access + using access control mechanisms such as the `@grant_access` decorator. Exceptions are made + for Django admin views, explicitly ignored paths, and views that opt out of login requirements. + """ def __init__(self, get_response): self.get_response = get_response + # Compile regex patterns from settings to identify paths that bypass login requirements self.ignored_paths = [re.compile(pattern) for pattern in getattr(settings, "LOGIN_REQUIRED_IGNORE_PATHS", [])] def __call__(self, request): - # Allow Django Debug Toolbar requests + # Allow requests to Django Debug Toolbar if request.path.startswith("/__debug__/"): return self.get_response(request) - # Allow requests that match LOGIN_REQUIRED_IGNORE_PATHS + # Allow requests matching configured ignored paths if any(pattern.match(request.path) for pattern in self.ignored_paths): return self.get_response(request) - # Try to resolve the view function + # Attempt to resolve the request path to a view function try: resolver_match = resolve(request.path_info) view_func = resolver_match.func - app_name = resolver_match.app_name # Get app name of resolved view + app_name = resolver_match.app_name # Get the app name of the resolved view except Exception: + # If resolution fails, allow the request to proceed (avoid blocking non-view routes) return self.get_response(request) - # Auto-allow Django's built-in admin views (but NOT custom /admin/* views) + # Automatically allow access to Django's built-in admin views (excluding custom /admin/* views) if app_name == "admin": return self.get_response(request) - # Skip access restriction if the view explicitly allows unauthenticated access + # Allow access if the view explicitly opts out of login requirements if getattr(view_func, "login_required", True) is False: return self.get_response(request) - # Enforce explicit access fules for other views + # Restrict access to views that do not explicitly declare access rules if not getattr(view_func, "has_explicit_access", False): - raise PermissionDenied + raise PermissionDenied # Deny access if the view lacks explicit permission handling return self.get_response(request) From efab9e495a29986f1e444b0dffb6289e3cc12e09 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 13 Feb 2025 06:51:33 -0500 Subject: [PATCH 17/17] removed unnecessary file --- src/registrar/utility/domain_cache_helper.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 src/registrar/utility/domain_cache_helper.py diff --git a/src/registrar/utility/domain_cache_helper.py b/src/registrar/utility/domain_cache_helper.py deleted file mode 100644 index e69de29bb..000000000