Merge pull request #2527 from cisagov/dk/2379-portfolio-domain-readonly-permissions

Issue #2379: portfolio domain readonly view
This commit is contained in:
dave-kennedy-ecs 2024-08-01 20:26:19 -04:00 committed by GitHub
commit eb8c7a16c1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
18 changed files with 144 additions and 95 deletions

View file

@ -241,7 +241,6 @@ TEMPLATES = [
"registrar.context_processors.is_demo_site",
"registrar.context_processors.is_production",
"registrar.context_processors.org_user_status",
"registrar.context_processors.add_portfolio_to_context",
"registrar.context_processors.add_path_to_context",
"registrar.context_processors.add_has_profile_feature_flag_to_context",
"registrar.context_processors.portfolio_permissions",

View file

@ -26,7 +26,6 @@ from registrar.views.domain_request import Step
from registrar.views.domain_requests_json import get_domain_requests_json
from registrar.views.domains_json import get_domains_json
from registrar.views.utility import always_404
from registrar.views.portfolios import PortfolioDomainsView, PortfolioDomainRequestsView, PortfolioOrganizationView
from api.views import available, get_current_federal, get_current_full
@ -61,19 +60,19 @@ for step, view in [
urlpatterns = [
path("", views.index, name="home"),
path(
"portfolio/<int:portfolio_id>/domains/",
PortfolioDomainsView.as_view(),
name="portfolio-domains",
"domains/",
views.PortfolioDomainsView.as_view(),
name="domains",
),
path(
"portfolio/<int:portfolio_id>/domain_requests/",
PortfolioDomainRequestsView.as_view(),
name="portfolio-domain-requests",
"requests/",
views.PortfolioDomainRequestsView.as_view(),
name="domain-requests",
),
path(
"portfolio/<int:portfolio_id>/organization/",
PortfolioOrganizationView.as_view(),
name="portfolio-organization",
"organization/",
views.PortfolioOrganizationView.as_view(),
name="organization",
),
path(
"admin/logout/",

View file

@ -50,10 +50,6 @@ def org_user_status(request):
}
def add_portfolio_to_context(request):
return {"portfolio": getattr(request, "portfolio", None)}
def add_path_to_context(request):
return {"path": getattr(request, "path", None)}
@ -70,11 +66,15 @@ def portfolio_permissions(request):
"has_base_portfolio_permission": False,
"has_domains_portfolio_permission": False,
"has_domain_requests_portfolio_permission": False,
"portfolio": None,
"has_organization_feature_flag": False,
}
return {
"has_base_portfolio_permission": request.user.has_base_portfolio_permission(),
"has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(),
"has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(),
"portfolio": request.user.portfolio,
"has_organization_feature_flag": flag_is_active(request, "organization_feature"),
}
except AttributeError:
# Handles cases where request.user might not exist
@ -82,4 +82,6 @@ def portfolio_permissions(request):
"has_base_portfolio_permission": False,
"has_domains_portfolio_permission": False,
"has_domain_requests_portfolio_permission": False,
"portfolio": None,
"has_organization_feature_flag": False,
}

View file

@ -149,10 +149,10 @@ class CheckPortfolioMiddleware:
request.portfolio = portfolio
if request.user.has_domains_portfolio_permission():
portfolio_redirect = reverse("portfolio-domains", kwargs={"portfolio_id": portfolio.id})
portfolio_redirect = reverse("domains")
else:
# View organization is the lowest access
portfolio_redirect = reverse("portfolio-organization", kwargs={"portfolio_id": portfolio.id})
portfolio_redirect = reverse("organization")
return HttpResponseRedirect(portfolio_redirect)

View file

@ -40,39 +40,50 @@
{% include "includes/domain_dates.html" %}
{% if is_portfolio_user and not is_domain_manager %}
<div class="usa-alert usa-alert--info usa-alert--slim">
<div class="usa-alert__body">
<p class="usa-alert__text ">
To manage information for this domain, you must add yourself as a domain manager.
</p>
</div>
</div>
{% endif %}
{% url 'domain-dns-nameservers' pk=domain.id as url %}
{% if domain.nameservers|length > 0 %}
{% include "includes/summary_item.html" with title='DNS name servers' domains='true' value=domain.nameservers list='true' edit_link=url editable=domain.is_editable %}
{% include "includes/summary_item.html" with title='DNS name servers' domains='true' value=domain.nameservers list='true' edit_link=url editable=is_editable %}
{% else %}
{% if domain.is_editable %}
{% if is_editable %}
<h2 class="margin-top-3"> DNS name servers </h2>
<p> No DNS name servers have been added yet. Before your domain can be used well need information about your domain name servers.</p>
<a class="usa-button margin-bottom-1" href="{{url}}"> Add DNS name servers </a>
{% else %}
{% include "includes/summary_item.html" with title='DNS name servers' domains='true' value='' edit_link=url editable=domain.is_editable %}
{% include "includes/summary_item.html" with title='DNS name servers' domains='true' value='' edit_link=url editable=is_editable %}
{% endif %}
{% endif %}
{% url 'domain-org-name-address' pk=domain.id as url %}
{% include "includes/summary_item.html" with title='Organization name and mailing address' value=domain.domain_info address='true' edit_link=url editable=domain.is_editable %}
{% include "includes/summary_item.html" with title='Organization name and mailing address' value=domain.domain_info address='true' edit_link=url editable=is_editable %}
{% url 'domain-senior-official' pk=domain.id as url %}
{% include "includes/summary_item.html" with title='Senior official' value=domain.domain_info.senior_official contact='true' edit_link=url editable=domain.is_editable %}
{% include "includes/summary_item.html" with title='Senior official' value=domain.domain_info.senior_official contact='true' edit_link=url editable=is_editable %}
{# Conditionally display profile #}
{% if not has_profile_feature_flag %}
{% url 'domain-your-contact-information' pk=domain.id as url %}
{% include "includes/summary_item.html" with title='Your contact information' value=request.user contact='true' edit_link=url editable=domain.is_editable %}
{% include "includes/summary_item.html" with title='Your contact information' value=request.user contact='true' edit_link=url editable=is_editable %}
{% endif %}
{% url 'domain-security-email' pk=domain.id as url %}
{% if security_email is not None and security_email not in hidden_security_emails%}
{% include "includes/summary_item.html" with title='Security email' value=security_email edit_link=url editable=domain.is_editable %}
{% include "includes/summary_item.html" with title='Security email' value=security_email edit_link=url editable=is_editable %}
{% else %}
{% include "includes/summary_item.html" with title='Security email' value='None provided' edit_link=url editable=domain.is_editable %}
{% include "includes/summary_item.html" with title='Security email' value='None provided' edit_link=url editable=is_editable %}
{% endif %}
{% url 'domain-users' pk=domain.id as url %}
{% include "includes/summary_item.html" with title='Domain managers' users='true' list=True value=domain.permissions.all edit_link=url editable=domain.is_editable %}
{% include "includes/summary_item.html" with title='Domain managers' users='true' list=True value=domain.permissions.all edit_link=url editable=is_editable %}
</div>
{% endblock %} {# domain_content #}

View file

@ -12,7 +12,7 @@
</a>
</li>
{% if domain.is_editable %}
{% if is_editable %}
<li class="usa-sidenav__item">
{% url 'domain-dns' pk=domain.id as url %}
<a href="{{ url }}" {% if request.path|startswith:url %}class="usa-current"{% endif %}>

View file

@ -1,4 +1,5 @@
{% load static %}
{% load custom_filters %}
<header class="usa-header usa-header--extended">
<div class="usa-navbar">
@ -14,8 +15,8 @@
<ul class="usa-nav__primary usa-accordion">
{% if has_domains_portfolio_permission %}
<li class="usa-nav__primary-item">
{% url 'portfolio-domains' portfolio.id as url %}
<a href="{{ url }}" class="usa-nav-link{% if request.path == url %} usa-current{% endif %}">
{% url 'domains' as url %}
<a href="{{ url }}" class="usa-nav-link{% if 'domain'|in_path:request.path %} usa-current{% endif %}">
Domains
</a>
</li>
@ -27,8 +28,8 @@
</li>
{% if has_domain_requests_portfolio_permission %}
<li class="usa-nav__primary-item">
{% url 'portfolio-domain-requests' portfolio.id as url %}
<a href="{{ url }}" class="usa-nav-link{% if request.path == url %} usa-current{% endif %}">
{% url 'domain-requests' as url %}
<a href="{{ url }}" class="usa-nav-link{% if 'request'|in_path:request.path %} usa-current{% endif %}">
Domain requests
</a>
</li>
@ -39,7 +40,7 @@
</a>
</li>
<li class="usa-nav__primary-item">
{% url 'portfolio-organization' portfolio.id as url %}
{% url 'organization' as url %}
<!-- Move the padding from the a to the span so that the descenders do not get cut off -->
<a href="{{ url }}" class="usa-nav-link padding-y-0">
<span class="ellipsis ellipsis--23 ellipsis--desktop-50 padding-y-1 desktop:padding-y-2">

View file

@ -4,7 +4,7 @@
<nav aria-label="Domain sections">
<ul class="usa-sidenav">
<li class="usa-sidenav__item">
{% url 'portfolio-organization' portfolio_id=portfolio.id as url %}
{% url 'organization' as url %}
<a href="{{ url }}"
{% if request.path == url %}class="usa-current"{% endif %}
>

View file

@ -145,3 +145,8 @@ def format_phone(value):
phone_number = PhoneNumber.from_string(value)
return phone_number.as_national
return value
@register.filter
def in_path(url, path):
return url in path

View file

@ -6,6 +6,7 @@ from django.urls import reverse
from django.contrib.auth import get_user_model
from api.tests.common import less_console_noise_decorator
from registrar.models.portfolio import Portfolio
from .common import MockEppLib, MockSESClient, create_user # type: ignore
from django_webtest import WebTest # type: ignore
import boto3_mocking # type: ignore
@ -138,6 +139,7 @@ class TestWithDomainPermissions(TestWithUser):
Host.objects.all().delete()
Domain.objects.all().delete()
UserDomainRole.objects.all().delete()
Portfolio.objects.all().delete()
except ValueError: # pass if already deleted
pass
super().tearDown()
@ -310,6 +312,33 @@ class TestDomainDetail(TestDomainOverview):
self.assertContains(detail_page, "noinformation.gov")
self.assertContains(detail_page, "Domain missing domain information")
@less_console_noise_decorator
def test_domain_readonly_on_detail_page(self):
"""Test that a domain, which is part of a portfolio, but for which the user is not a domain manager,
properly displays read only"""
portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test org", creator=self.user)
# need to create a different user than self.user because the user needs permission assignments
user = get_user_model().objects.create(
first_name="Test",
last_name="User",
email="bogus@example.gov",
phone="8003111234",
title="test title",
portfolio=portfolio,
portfolio_roles=[User.UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
)
domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov")
DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio)
self.client.force_login(user)
detail_page = self.client.get(f"/domain/{domain.id}")
# Check that alert message displays properly
self.assertContains(
detail_page, "To manage information for this domain, you must add yourself as a domain manager."
)
# Check that user does not have option to Edit domain
self.assertNotContains(detail_page, "Edit")
class TestDomainManagers(TestDomainOverview):
def tearDown(self):

View file

@ -181,7 +181,8 @@ class GetDomainsJsonTest(TestWithUser, WebTest):
# Check svg_icon
svg_icon_expected = (
"visibility"
if expected_domains[i].state
if not user_domain_role_exists
or expected_domains[i].state
in [
Domain.State.DELETED,
Domain.State.ON_HOLD,

View file

@ -111,9 +111,7 @@ class TestPortfolio(WebTest):
with override_flag("organization_feature", active=True):
# This will redirect the user to the portfolio page.
# Follow implicity checks if our redirect is working.
response = self.app.get(
reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk}), status=403
)
response = self.app.get(reverse("domains"), status=403)
# Assert the response is a 403 Forbidden
self.assertEqual(response.status_code, 403)
@ -127,9 +125,7 @@ class TestPortfolio(WebTest):
with override_flag("organization_feature", active=True):
# This will redirect the user to the portfolio page.
# Follow implicity checks if our redirect is working.
response = self.app.get(
reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk}), status=403
)
response = self.app.get(reverse("domain-requests"), status=403)
# Assert the response is a 403 Forbidden
self.assertEqual(response.status_code, 403)
@ -143,9 +139,7 @@ class TestPortfolio(WebTest):
with override_flag("organization_feature", active=True):
# This will redirect the user to the portfolio page.
# Follow implicity checks if our redirect is working.
response = self.app.get(
reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk}), status=403
)
response = self.app.get(reverse("organization"), status=403)
# Assert the response is a 403 Forbidden
self.assertEqual(response.status_code, 403)
@ -169,12 +163,8 @@ class TestPortfolio(WebTest):
self.assertContains(portfolio_page, self.portfolio.organization_name)
self.assertNotContains(portfolio_page, "<h1>Organization</h1>")
self.assertContains(portfolio_page, '<h1 id="domains-header">Domains</h1>')
self.assertContains(
portfolio_page, reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk})
)
self.assertContains(
portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk})
)
self.assertContains(portfolio_page, reverse("domains"))
self.assertContains(portfolio_page, reverse("domain-requests"))
# reducing portfolio permissions to just VIEW_PORTFOLIO, which should remove domains
# and domain requests from nav
@ -187,12 +177,8 @@ class TestPortfolio(WebTest):
self.assertContains(portfolio_page, self.portfolio.organization_name)
self.assertContains(portfolio_page, "<h1>Organization</h1>")
self.assertNotContains(portfolio_page, '<h1 id="domains-header">Domains</h1>')
self.assertNotContains(
portfolio_page, reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk})
)
self.assertNotContains(
portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk})
)
self.assertNotContains(portfolio_page, reverse("domains"))
self.assertNotContains(portfolio_page, reverse("domain-requests"))
class TestPortfolioOrganization(TestPortfolio):
@ -209,7 +195,7 @@ class TestPortfolioOrganization(TestPortfolio):
self.user.save()
self.user.refresh_from_db()
page = self.app.get(reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk}))
page = self.app.get(reverse("organization"))
self.assertContains(
page, "The name of your federal agency will be publicly listed as the domain registrant."
)
@ -228,7 +214,7 @@ class TestPortfolioOrganization(TestPortfolio):
self.portfolio.organization_name = "Hotel California"
self.portfolio.save()
page = self.app.get(reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk}))
page = self.app.get(reverse("organization"))
# Once in the sidenav, once in the main nav, once in the form
self.assertContains(page, "Hotel California", count=3)
@ -246,9 +232,7 @@ class TestPortfolioOrganization(TestPortfolio):
self.portfolio.address_line1 = "1600 Penn Ave"
self.portfolio.save()
portfolio_org_name_page = self.app.get(
reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk})
)
portfolio_org_name_page = self.app.get(reverse("organization"))
session_id = self.app.cookies[settings.SESSION_COOKIE_NAME]
portfolio_org_name_page.form["address_line1"] = "6 Downing st"

View file

@ -17,3 +17,4 @@ from .domain import (
from .user_profile import UserProfileView, FinishProfileSetupView
from .health import *
from .index import *
from .portfolios import *

View file

@ -170,6 +170,17 @@ class DomainView(DomainBaseView):
context["security_email"] = security_email
return context
def can_access_domain_via_portfolio(self, pk):
"""Most views should not allow permission to portfolio users.
If particular views allow permissions, they will need to override
this function."""
if self.request.user.has_domains_portfolio_permission():
if Domain.objects.filter(id=pk).exists():
domain = Domain.objects.get(id=pk)
if domain.domain_info.portfolio == self.request.user.portfolio:
return True
return False
def in_editable_state(self, pk):
"""Override in_editable_state from DomainPermission
Allow detail page to be viewable"""

View file

@ -124,7 +124,7 @@ def serialize_domain(domain, user):
# Check if there is a UserDomainRole for this domain and user
user_domain_role_exists = UserDomainRole.objects.filter(domain_id=domain.id, user=user).exists()
view_only = not user_domain_role_exists or domain.state in [Domain.State.DELETED, Domain.State.ON_HOLD]
return {
"id": domain.id,
"name": domain.name,
@ -133,11 +133,7 @@ def serialize_domain(domain, user):
"state_display": domain.state_display(),
"get_state_help_text": domain.get_state_help_text(),
"action_url": reverse("domain", kwargs={"pk": domain.id}),
"action_label": (
"View"
if not user_domain_role_exists or domain.state in [Domain.State.DELETED, Domain.State.ON_HOLD]
else "Manage"
),
"svg_icon": ("visibility" if domain.state in [Domain.State.DELETED, Domain.State.ON_HOLD] else "settings"),
"action_label": ("View" if view_only else "Manage"),
"svg_icon": ("visibility" if view_only else "settings"),
"suborganization": suborganization_name,
}

View file

@ -1,5 +1,6 @@
import logging
from django.shortcuts import get_object_or_404, render
from django.http import Http404
from django.shortcuts import render
from django.urls import reverse
from django.contrib import messages
from registrar.forms.portfolio import PortfolioOrgAddressForm
@ -9,7 +10,6 @@ from registrar.views.utility.permission_views import (
PortfolioDomainsPermissionView,
PortfolioBasePermissionView,
)
from waffle.decorators import flag_is_active
from django.views.generic import View
from django.views.generic.edit import FormMixin
@ -21,33 +21,18 @@ class PortfolioDomainsView(PortfolioDomainsPermissionView, View):
template_name = "portfolio_domains.html"
def get(self, request, portfolio_id):
context = {}
if self.request.user.is_authenticated:
context["has_profile_feature_flag"] = flag_is_active(request, "profile_feature")
context["has_organization_feature_flag"] = flag_is_active(request, "organization_feature")
portfolio = get_object_or_404(Portfolio, id=portfolio_id)
context["portfolio"] = portfolio
return render(request, "portfolio_domains.html", context)
def get(self, request):
return render(request, "portfolio_domains.html")
class PortfolioDomainRequestsView(PortfolioDomainRequestsPermissionView, View):
template_name = "portfolio_requests.html"
def get(self, request, portfolio_id):
context = {}
def get(self, request):
if self.request.user.is_authenticated:
context["has_profile_feature_flag"] = flag_is_active(request, "profile_feature")
context["has_organization_feature_flag"] = flag_is_active(request, "organization_feature")
portfolio = get_object_or_404(Portfolio, id=portfolio_id)
context["portfolio"] = portfolio
request.session["new_request"] = True
return render(request, "portfolio_requests.html", context)
return render(request, "portfolio_requests.html")
class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin):
@ -63,14 +48,14 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin):
def get_context_data(self, **kwargs):
"""Add additional context data to the template."""
context = super().get_context_data(**kwargs)
# no need to add portfolio to request context here
context["has_profile_feature_flag"] = flag_is_active(self.request, "profile_feature")
context["has_organization_feature_flag"] = flag_is_active(self.request, "organization_feature")
return context
def get_object(self, queryset=None):
"""Get the portfolio object based on the URL parameter."""
return get_object_or_404(Portfolio, id=self.kwargs.get("portfolio_id"))
"""Get the portfolio object based on the request user."""
portfolio = self.request.user.portfolio
if portfolio is None:
raise Http404("No organization found for this user")
return portfolio
def get_form_kwargs(self):
"""Include the instance in the form kwargs."""
@ -107,4 +92,4 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin):
def get_success_url(self):
"""Redirect to the overview page for the portfolio."""
return reverse("portfolio-organization", kwargs={"portfolio_id": self.object.pk})
return reverse("organization")

View file

@ -184,11 +184,17 @@ class DomainPermission(PermissionsLoginMixin):
# user needs to have a role on the domain
if not UserDomainRole.objects.filter(user=self.request.user, domain__id=pk).exists():
return False
return self.can_access_domain_via_portfolio(pk)
# if we need to check more about the nature of role, do it here.
return True
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 in_editable_state(self, pk):
"""Is the domain in an editable state"""

View file

@ -43,6 +43,9 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC):
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"
@ -54,6 +57,22 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC):
return context
def is_editable(self):
"""Returns whether domain is editable in the context of the view"""
logger.info("checking if is_editable")
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
# Abstract property enforces NotImplementedError on an attribute.
@property
@abc.abstractmethod