diff --git a/docs/architecture/decisions/0019-role-based-access-control.md b/docs/architecture/decisions/0019-role-based-access-control.md new file mode 100644 index 000000000..d5bc5ddb3 --- /dev/null +++ b/docs/architecture/decisions/0019-role-based-access-control.md @@ -0,0 +1,29 @@ +# 19. Role-based Access Control + +Date: 2023-03-17 + +## Status + +Approved + +## Context + +In the registrar application, a single user might be associated with many +domains, and they might have different levels of access to view or change +those domains. + +## Decision + +To use a role-based access control system where we have a model of different +roles and an association that links a user to a specific role with a specified +role. Each role would have some associated permissions in the application and +we can enforce those permissions by using decorators on our Django views. + +## Consequences + +There is no enterprise model here of users belonging to an “organization” with +a role on all of its associated domain names. Instead, the association is +per-domain and a user would have to be granted the role on each domain +individually. There is also no process designed yet for how and whether users +can grant other users roles on a domain. + diff --git a/docs/developer/user-permissions.md b/docs/developer/user-permissions.md new file mode 100644 index 000000000..281a58c1b --- /dev/null +++ b/docs/developer/user-permissions.md @@ -0,0 +1,40 @@ +# User Permissions + +In our registrar application, we need authenticated users (via Login.gov) to +be able to access domains that they are authorized to and not to access +domains that they are not authorized to. In our initial MVP design, this +access is controlled at the domain level, there is no "enterprise" or +"organization" layer for assigning permissions in bulk. (See [this +ADR](../architecture/decisions/0019-role-based-access-control.md) for more on +that decision.) + +## Data modeling + +We need a way to associate a particular user with a particular domain and the +role or set of permissions that they have. We use a `UserDomainRole` +[model](../../src/registrar/models/user_domain_role.py) with `ForeignKey`s to +`User` and `Domain` and a `role` field. There are reverse relationships called +`permissions` for a user and for a domain to get a list of all of the +`UserDomainRole`s that involve the user or the domain. In addition, there is a +`User.domains` many-to-many relationship that works through the +`UserDomainRole` link table. + +## Permission decorator + +The Django objects that need to be permission controlled are various views. +For that purpose, we add a very simple permission mixin +[`DomainPermission`](../../src/registrar/views/utility/mixins.py) that can be +added to a view to require that (a) there is a logged-in user and (b) that the +logged in user has a role that permits access to that view. This mixin is the +place where the details of the permissions are enforced. It can allow a view +to load, or deny access with various status codes, e.g. "403 Forbidden". + +## Adding roles + +The current MVP design uses only a single role called +`UserDomainRole.Roles.ADMIN` that has all access on a domain. As such, the +permission mixin doesn't need to examine the `role` field carefully. In the +future, as we add additional roles that our product vision calls for +(read-only? editing only some information?), we need to add conditional +behavior in the permission mixin, or additional mixins that more clearly +express what is allowed for those new roles. diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 287043c06..53cd5c374 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -61,6 +61,7 @@ urlpatterns = [ lambda r: always_404(r, "We forgot to include this link, sorry."), name="todo", ), + path("domain/", views.DomainView.as_view(), name="domain"), ] diff --git a/src/registrar/migrations/0015_remove_domain_owners_userdomainrole_user_domains_and_more.py b/src/registrar/migrations/0015_remove_domain_owners_userdomainrole_user_domains_and_more.py new file mode 100644 index 000000000..2fbc5ab19 --- /dev/null +++ b/src/registrar/migrations/0015_remove_domain_owners_userdomainrole_user_domains_and_more.py @@ -0,0 +1,66 @@ +# Generated by Django 4.1.6 on 2023-03-10 15:32 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0014_user_phone_alter_contact_user"), + ] + + operations = [ + migrations.RemoveField( + model_name="domain", + name="owners", + ), + migrations.CreateModel( + name="UserDomainRole", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("created_at", models.DateTimeField(auto_now_add=True)), + ("updated_at", models.DateTimeField(auto_now=True)), + ("role", models.TextField(choices=[("admin", "Admin")])), + ( + "domain", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="permissions", + to="registrar.domain", + ), + ), + ( + "user", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="permissions", + to=settings.AUTH_USER_MODEL, + ), + ), + ], + ), + migrations.AddField( + model_name="user", + name="domains", + field=models.ManyToManyField( + related_name="users", + through="registrar.UserDomainRole", + to="registrar.domain", + ), + ), + migrations.AddConstraint( + model_name="userdomainrole", + constraint=models.UniqueConstraint( + fields=("user", "domain"), name="unique_user_domain_role" + ), + ), + ] diff --git a/src/registrar/models/__init__.py b/src/registrar/models/__init__.py index 9b7aba971..969915969 100644 --- a/src/registrar/models/__init__.py +++ b/src/registrar/models/__init__.py @@ -6,6 +6,7 @@ from .domain import Domain from .host_ip import HostIP from .host import Host from .nameserver import Nameserver +from .user_domain_role import UserDomainRole from .public_contact import PublicContact from .user import User from .website import Website @@ -17,6 +18,7 @@ __all__ = [ "HostIP", "Host", "Nameserver", + "UserDomainRole", "PublicContact", "User", "Website", @@ -28,6 +30,7 @@ auditlog.register(Domain) auditlog.register(HostIP) auditlog.register(Host) auditlog.register(Nameserver) +auditlog.register(UserDomainRole) auditlog.register(PublicContact) auditlog.register(User) auditlog.register(Website) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index eb89cd387..6697e2e64 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -276,9 +276,8 @@ class Domain(TimeStampedModel): help_text="Domain is live in the registry", ) - # TODO: determine the relationship between this field - # and the domain application's `creator` and `submitter` - owners = models.ManyToManyField( - "registrar.User", - help_text="", - ) + # ForeignKey on UserDomainRole creates a "permissions" member for + # all of the user-roles that are in place for this domain + + # ManyToManyField on User creates a "users" member for all of the + # users who have some role on this domain diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 9a0d5524a..efe87abaf 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -502,6 +502,25 @@ class DomainApplication(TimeStampedModel): # This is a side-effect of the state transition self._send_confirmation_email() + @transition(field="status", source=[SUBMITTED, INVESTIGATING], target=APPROVED) + def approve(self): + """Approve an application that has been submitted. + + This has substantial side-effects because it creates another database + object for the approved Domain and makes the user who created the + application into an admin on that domain. + """ + + # create the domain if it doesn't exist + Domain = apps.get_model("registrar.Domain") + created_domain, _ = Domain.objects.get_or_create(name=self.requested_domain) + + # create the permission for the user + UserDomainRole = apps.get_model("registrar.UserDomainRole") + UserDomainRole.objects.get_or_create( + user=self.creator, domain=created_domain, role=UserDomainRole.Roles.ADMIN + ) + # ## Form policies ### # # These methods control what questions need to be answered by applicants diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 9fc726114..97f5753b3 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -1,4 +1,5 @@ from django.contrib.auth.models import AbstractUser +from django.db import models from phonenumber_field.modelfields import PhoneNumberField # type: ignore @@ -9,6 +10,12 @@ class User(AbstractUser): but can be customized later. """ + domains = models.ManyToManyField( + "registrar.Domain", + through="registrar.UserDomainRole", + related_name="users", + ) + phone = PhoneNumberField( null=True, blank=True, diff --git a/src/registrar/models/user_domain_role.py b/src/registrar/models/user_domain_role.py new file mode 100644 index 000000000..5a5219543 --- /dev/null +++ b/src/registrar/models/user_domain_role.py @@ -0,0 +1,50 @@ +from django.db import models + +from .utility.time_stamped_model import TimeStampedModel + + +class UserDomainRole(TimeStampedModel): + + """This is a linking table that connects a user with a role on a domain.""" + + class Roles(models.TextChoices): + + """The possible roles are listed here. + + Implementation of the named roles for allowing particular operations happens + elsewhere. + """ + + ADMIN = "admin" + + user = models.ForeignKey( + "registrar.User", + null=False, + on_delete=models.CASCADE, # when a user is deleted, permissions are too + related_name="permissions", + ) + + domain = models.ForeignKey( + "registrar.Domain", + null=False, + on_delete=models.CASCADE, # when a domain is deleted, permissions are too + related_name="permissions", + ) + + role = models.TextField( + choices=Roles.choices, + null=False, + blank=False, + ) + + def __str__(self): + return "User {} is {} on domain {}".format(self.user, self.role, self.domain) + + class Meta: + constraints = [ + # a user can have only one role on a given domain, that is, there can + # be only a single row with a certain (user, domain) pair. + models.UniqueConstraint( + fields=["user", "domain"], name="unique_user_domain_role" + ) + ] diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html new file mode 100644 index 000000000..5d0624209 --- /dev/null +++ b/src/registrar/templates/domain_detail.html @@ -0,0 +1,14 @@ +{% extends "dashboard_base.html" %} + + +{% block title %}Domain {{ domain.name }}{% endblock %} + +{% block content %} +
+
+

{{ domain.name }}

+ +

Active: {% if domain.is_active %}Yes{% else %}No{% endif %}

+
+
+{% endblock %} {# content #} diff --git a/src/registrar/templates/home.html b/src/registrar/templates/home.html index 0a753d25c..f15f1d1a8 100644 --- a/src/registrar/templates/home.html +++ b/src/registrar/templates/home.html @@ -16,7 +16,41 @@

Registered domains

+ {% if domains %} + + + + + + + + + + + + {% for domain in domains %} + + + + + + + {% endfor %} + +
Your domain applications
Domain nameDate createdStatusAction
+ {{ domain.name }} + {{ domain.created_time|date }}{{ domain.application_status|title }} + + Edit {{ domain.name }} + +
+
+ {% else %}

You don't have any registered domains yet

+ {% endif %}
diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 911b73086..ef9659cd6 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1,7 +1,14 @@ from django.test import TestCase from django.db.utils import IntegrityError -from registrar.models import Contact, DomainApplication, User, Website, Domain +from registrar.models import ( + Contact, + DomainApplication, + User, + Website, + Domain, + UserDomainRole, +) from unittest import skip import boto3_mocking # type: ignore @@ -139,6 +146,24 @@ class TestDomain(TestCase): d1.activate() +class TestPermissions(TestCase): + + """Test the User-Domain-Role connection.""" + + def test_approval_creates_role(self): + domain, _ = Domain.objects.get_or_create(name="igorville.gov") + user, _ = User.objects.get_or_create() + application = DomainApplication.objects.create( + creator=user, requested_domain=domain + ) + # skip using the submit method + application.status = DomainApplication.SUBMITTED + application.approve() + + # should be a role for this user + self.assertTrue(UserDomainRole.objects.get(user=user, domain=domain)) + + @skip("Not implemented yet.") class TestDomainApplicationLifeCycle(TestCase): def test_application_approval(self): diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index b1cc9c8d1..22f2e2ac9 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -9,7 +9,7 @@ from django_webtest import WebTest # type: ignore import boto3_mocking # type: ignore -from registrar.models import DomainApplication, Domain, Contact, Website +from registrar.models import DomainApplication, Domain, Contact, Website, UserDomainRole from registrar.views.application import ApplicationWizard, Step from .common import less_console_noise @@ -76,6 +76,19 @@ class LoggedInTests(TestWithUser): # clean up application.delete() + def test_home_lists_domains(self): + response = self.client.get("/") + domain, _ = Domain.objects.get_or_create(name="igorville.gov") + self.assertNotContains(response, "igorville.gov") + role, _ = UserDomainRole.objects.get_or_create( + user=self.user, domain=domain, role=UserDomainRole.Roles.ADMIN + ) + response = self.client.get("/") + # count = 2 because it is also in screenreader content + self.assertContains(response, "igorville.gov", count=2) + # clean up + role.delete() + def test_whoami_page(self): """User information appears on the whoami page.""" response = self.client.get("/whoami/") @@ -1003,3 +1016,46 @@ class DomainApplicationTests(TestWithUser, WebTest): # self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) # page = self.app.get(url) # self.assertNotContains(page, "VALUE") + + +class TestDomainPermissions(TestWithUser): + def setUp(self): + super().setUp() + self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") + self.role, _ = UserDomainRole.objects.get_or_create( + user=self.user, domain=self.domain, role=UserDomainRole.Roles.ADMIN + ) + + def tearDown(self): + try: + self.domain.delete() + self.role.delete() + except ValueError: # pass if already deleted + pass + super().tearDown() + + def test_not_logged_in(self): + """Not logged in gets a redirect to Login.""" + response = self.client.get(reverse("domain", kwargs={"pk": self.domain.id})) + self.assertEqual(response.status_code, 302) + + def test_no_domain_role(self): + """Logged in but no role gets 403 Forbidden.""" + self.client.force_login(self.user) + self.role.delete() # user no longer has a role on this domain + with less_console_noise(): + response = self.client.get(reverse("domain", kwargs={"pk": self.domain.id})) + self.assertEqual(response.status_code, 403) + + +class TestDomainDetail(TestDomainPermissions, WebTest): + def setUp(self): + super().setUp() + self.app.set_user(self.user.username) + + def test_domain_detail_link_works(self): + home_page = self.app.get("/") + self.assertContains(home_page, "igorville.gov") + # click the "Edit" link + detail_page = home_page.click("Edit") + self.assertContains(detail_page, "igorville.gov") diff --git a/src/registrar/views/__init__.py b/src/registrar/views/__init__.py index 3941213ea..696c4640d 100644 --- a/src/registrar/views/__init__.py +++ b/src/registrar/views/__init__.py @@ -1,4 +1,5 @@ from .application import * +from .domain import * from .health import * from .index import * from .whoami import * diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py new file mode 100644 index 000000000..e2553ce44 --- /dev/null +++ b/src/registrar/views/domain.py @@ -0,0 +1,13 @@ +"""View for a single Domain.""" + +from django.views.generic import DetailView + +from registrar.models import Domain + +from .utility import DomainPermission + + +class DomainView(DomainPermission, DetailView): + model = Domain + template_name = "domain_detail.html" + context_object_name = "domain" diff --git a/src/registrar/views/index.py b/src/registrar/views/index.py index 6d50b3948..35a67bceb 100644 --- a/src/registrar/views/index.py +++ b/src/registrar/views/index.py @@ -1,3 +1,4 @@ +from django.db.models import F from django.shortcuts import render from registrar.models import DomainApplication @@ -9,4 +10,13 @@ def index(request): if request.user.is_authenticated: applications = DomainApplication.objects.filter(creator=request.user) context["domain_applications"] = applications + + domains = request.user.permissions.values( + "role", + pk=F("domain__id"), + name=F("domain__name"), + created_time=F("domain__created_at"), + application_status=F("domain__domain_application__status"), + ) + context["domains"] = domains return render(request, "home.html", context) diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index cf6ac377a..a492b5cb3 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -1,2 +1,3 @@ from .steps_helper import StepsHelper from .always_404 import always_404 +from .mixins import DomainPermission diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py new file mode 100644 index 000000000..71129a1f6 --- /dev/null +++ b/src/registrar/views/utility/mixins.py @@ -0,0 +1,37 @@ +"""Permissions-related mixin classes.""" + +from django.contrib.auth.mixins import PermissionRequiredMixin + +from registrar.models import UserDomainRole + + +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 DomainPermission(PermissionsLoginMixin): + + """Does the logged-in user have access to this domain?""" + + 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 + + # user needs to have a role on the domain + if not UserDomainRole.objects.filter( + user=self.request.user, domain__id=self.kwargs["pk"] + ).exists(): + return False + + # if we need to check more about the nature of role, do it here. + return True