From 8650351ffec31a4715173997145ceb06625b659a Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Wed, 22 Mar 2023 15:09:15 -0500 Subject: [PATCH 01/22] Add model for domain invitations --- .../migrations/0016_domaininvitation.py | 50 +++++++++++++++ src/registrar/models/__init__.py | 3 + src/registrar/models/domain_invitation.py | 64 +++++++++++++++++++ src/registrar/tests/test_models.py | 28 ++++++++ 4 files changed, 145 insertions(+) create mode 100644 src/registrar/migrations/0016_domaininvitation.py create mode 100644 src/registrar/models/domain_invitation.py diff --git a/src/registrar/migrations/0016_domaininvitation.py b/src/registrar/migrations/0016_domaininvitation.py new file mode 100644 index 000000000..8299044fe --- /dev/null +++ b/src/registrar/migrations/0016_domaininvitation.py @@ -0,0 +1,50 @@ +# Generated by Django 4.1.6 on 2023-03-22 19:55 + +from django.db import migrations, models +import django.db.models.deletion +import django_fsm + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0015_remove_domain_owners_userdomainrole_user_domains_and_more"), + ] + + operations = [ + migrations.CreateModel( + name="DomainInvitation", + 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)), + ("email", models.EmailField(max_length=254)), + ( + "status", + django_fsm.FSMField( + choices=[("sent", "sent"), ("retrieved", "retrieved")], + default="sent", + max_length=50, + protected=True, + ), + ), + ( + "domain", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="registrar.domain", + ), + ), + ], + options={ + "abstract": False, + }, + ), + ] diff --git a/src/registrar/models/__init__.py b/src/registrar/models/__init__.py index 969915969..0fcfeca40 100644 --- a/src/registrar/models/__init__.py +++ b/src/registrar/models/__init__.py @@ -5,6 +5,7 @@ from .domain_application import DomainApplication from .domain import Domain from .host_ip import HostIP from .host import Host +from .domain_invitation import DomainInvitation from .nameserver import Nameserver from .user_domain_role import UserDomainRole from .public_contact import PublicContact @@ -15,6 +16,7 @@ __all__ = [ "Contact", "DomainApplication", "Domain", + "DomainInvitation", "HostIP", "Host", "Nameserver", @@ -27,6 +29,7 @@ __all__ = [ auditlog.register(Contact) auditlog.register(DomainApplication) auditlog.register(Domain) +auditlog.register(DomainInvitation) auditlog.register(HostIP) auditlog.register(Host) auditlog.register(Nameserver) diff --git a/src/registrar/models/domain_invitation.py b/src/registrar/models/domain_invitation.py new file mode 100644 index 000000000..a1598e54e --- /dev/null +++ b/src/registrar/models/domain_invitation.py @@ -0,0 +1,64 @@ +"""People are invited by email to administer domains.""" + +from django.contrib.auth import get_user_model +from django.db import models, IntegrityError + +from django_fsm import FSMField, transition + +from .utility.time_stamped_model import TimeStampedModel +from .user_domain_role import UserDomainRole + + +class DomainInvitation(TimeStampedModel): + SENT = "sent" + RETRIEVED = "retrieved" + + email = models.EmailField( + null=False, + blank=False, + ) + + domain = models.ForeignKey( + "registrar.Domain", + on_delete=models.CASCADE, # delete domain, then get rid of invitations + null=False, + ) + + status = FSMField( + choices=[ + (SENT, SENT), + (RETRIEVED, RETRIEVED), + ], + default=SENT, + protected=True, # can't alter state except through transition methods! + ) + + def __str__(self): + return f"Invitation for {self.email} on {self.domain} is {self.status}" + + @transition(field="status", source=SENT, target=RETRIEVED) + def retrieve(self): + """When an invitation is retrieved, create the corresponding permission.""" + + # get a user with this email address + User = get_user_model() + try: + user = User.objects.get(email=self.email) + except User.DoesNotExist: + # should not happen because a matching user should exist before + # we retrieve this invitation + raise RuntimeError( + "Cannot find the user to retrieve this domain invitation." + ) + + # and create a role for that user on this domain + try: + UserDomainRole.objects.create( + user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN + ) + except IntegrityError: + # should not happen because this user shouldn't retrieve this invitation + # more than once. + raise RuntimeError( + "Invitation would create a role that already exists for this user." + ) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index ef9659cd6..75a43e6d6 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -7,6 +7,7 @@ from registrar.models import ( User, Website, Domain, + DomainInvitation, UserDomainRole, ) from unittest import skip @@ -164,6 +165,33 @@ class TestPermissions(TestCase): self.assertTrue(UserDomainRole.objects.get(user=user, domain=domain)) +class TestInvitations(TestCase): + + """Test the retrieval of invitations.""" + + def setUp(self): + self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") + self.email = "mayor@igorville.gov" + self.invitation, _ = DomainInvitation.objects.get_or_create(email=self.email, domain=self.domain) + self.user, _ = User.objects.get_or_create(email=self.email) + + def test_retrieval_creates_role(self): + self.invitation.retrieve() + self.assertTrue(UserDomainRole.objects.get(user=self.user, domain=self.domain)) + + def test_retrieve_missing_user_error(self): + # get rid of matching users + User.objects.filter(email=self.email).delete() + with self.assertRaises(RuntimeError): + self.invitation.retrieve() + + def test_retrieve_existing_role_error(self): + # make the overlapping role + UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain) + with self.assertRaises(RuntimeError): + self.invitation.retrieve() + + @skip("Not implemented yet.") class TestDomainApplicationLifeCycle(TestCase): def test_application_approval(self): From 69706fbbb78524d7de72d4ee455d8ba411e8c183 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Wed, 22 Mar 2023 16:14:52 -0500 Subject: [PATCH 02/22] Make invitation and show on user management page --- src/registrar/models/domain.py | 3 +++ src/registrar/models/domain_invitation.py | 1 + src/registrar/templates/domain_users.html | 27 ++++++++++++++++++++- src/registrar/views/domain.py | 29 +++++++++++++---------- 4 files changed, 47 insertions(+), 13 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 6697e2e64..09b0fd211 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -281,3 +281,6 @@ class Domain(TimeStampedModel): # ManyToManyField on User creates a "users" member for all of the # users who have some role on this domain + + # ForeignKey on DomainInvitation creates an "invitations" member for + # all of the invitations that have been sent for this domain diff --git a/src/registrar/models/domain_invitation.py b/src/registrar/models/domain_invitation.py index a1598e54e..80fed0486 100644 --- a/src/registrar/models/domain_invitation.py +++ b/src/registrar/models/domain_invitation.py @@ -22,6 +22,7 @@ class DomainInvitation(TimeStampedModel): "registrar.Domain", on_delete=models.CASCADE, # delete domain, then get rid of invitations null=False, + related_name="invitations", ) status = FSMField( diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index a8091fa1c..9b30e7923 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -25,7 +25,7 @@ {% for permission in domain.permissions.all %} - + {{ permission.user.email }} {{ permission.role|title }} @@ -45,4 +45,29 @@ Add another user + {% if domain.invitations.all %} +

Invitations

+ + + + + + + + + + + {% for invitation in domain.invitations.all %} + + + + + + {% endfor %} + +
Domain invitations
EmailDate createdStatus
+ {{ invitation.email }} + {{ invitation.created_at|date }} {{ invitation.status|title }}
+ {% endif %} + {% endblock %} {# domain_content #} diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index dc8ccc369..db46b163d 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -8,7 +8,7 @@ from django.urls import reverse from django.views.generic import DetailView from django.views.generic.edit import FormMixin -from registrar.models import Domain, User, UserDomainRole +from registrar.models import Domain, DomainInvitation, User, UserDomainRole from .utility import DomainPermission @@ -31,15 +31,6 @@ class DomainAddUserForm(DomainPermission, forms.Form): email = forms.EmailField(label="Email") - def clean_email(self): - requested_email = self.cleaned_data["email"] - try: - User.objects.get(email=requested_email) - except User.DoesNotExist: - # TODO: send an invitation email to a non-existent user - raise forms.ValidationError("That user does not exist in this system.") - return requested_email - class DomainAddUserView(DomainPermission, FormMixin, DetailView): template_name = "domain_add_user.html" @@ -53,16 +44,30 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): self.object = self.get_object() form = self.get_form() if form.is_valid(): + # there is a valid email address in the form return self.form_valid(form) else: return self.form_invalid(form) + def _make_invitation(self, email_address): + """Make a Domain invitation for this email and redirect with a message.""" + invitation, created = DomainInvitation.objects.get_or_create(email=email_address, domain=self.object) + if not created: + # that invitation already existed + messages.warning(self.request, f"{email_address} has already been invited to this domain.") + else: + messages.success(self.request, f"Invited {email_address} to this domain.") + return redirect(self.get_success_url()) + def form_valid(self, form): """Add the specified user on this domain.""" requested_email = form.cleaned_data["email"] # look up a user with that email - # they should exist because we checked in clean_email - requested_user = User.objects.get(email=requested_email) + try: + requested_user = User.objects.get(email=requested_email) + except User.DoesNotExist: + # no matching user, go make an invitation + return self._make_invitation(requested_email) try: UserDomainRole.objects.create( From 6f67080e6e8e204ce5643317dd67776dcc543c8d Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Fri, 24 Mar 2023 13:37:50 -0500 Subject: [PATCH 03/22] Create invitations and show/delete from user management view --- src/registrar/config/urls.py | 5 +++ src/registrar/forms/domain.py | 11 ------ .../migrations/0016_domaininvitation.py | 5 ++- src/registrar/models/domain_invitation.py | 2 +- src/registrar/templates/domain_users.html | 5 +++ src/registrar/tests/test_models.py | 4 +- src/registrar/tests/test_views.py | 38 ++++++++++++++++++- src/registrar/views/__init__.py | 7 +++- src/registrar/views/domain.py | 32 ++++++++++------ 9 files changed, 81 insertions(+), 28 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 0d0ec89f5..8674348c8 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -68,6 +68,11 @@ urlpatterns = [ views.DomainAddUserView.as_view(), name="domain-users-add", ), + path( + "invitation//delete", + views.DomainInvitationDeleteView.as_view(http_method_names=["post"]), + name="invitation-delete", + ), ] diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 6a2229961..3d5941eed 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -2,20 +2,9 @@ from django import forms -from registrar.models import User - class DomainAddUserForm(forms.Form): """Form for adding a user to a domain.""" email = forms.EmailField(label="Email") - - def clean_email(self): - requested_email = self.cleaned_data["email"] - try: - User.objects.get(email=requested_email) - except User.DoesNotExist: - # TODO: send an invitation email to a non-existent user - raise forms.ValidationError("That user does not exist in this system.") - return requested_email diff --git a/src/registrar/migrations/0016_domaininvitation.py b/src/registrar/migrations/0016_domaininvitation.py index 8299044fe..f7756ef1d 100644 --- a/src/registrar/migrations/0016_domaininvitation.py +++ b/src/registrar/migrations/0016_domaininvitation.py @@ -1,8 +1,8 @@ -# Generated by Django 4.1.6 on 2023-03-22 19:55 +# Generated by Django 4.1.6 on 2023-03-24 16:56 from django.db import migrations, models import django.db.models.deletion -import django_fsm +import django_fsm # type: ignore class Migration(migrations.Migration): @@ -39,6 +39,7 @@ class Migration(migrations.Migration): "domain", models.ForeignKey( on_delete=django.db.models.deletion.CASCADE, + related_name="invitations", to="registrar.domain", ), ), diff --git a/src/registrar/models/domain_invitation.py b/src/registrar/models/domain_invitation.py index 80fed0486..ba9b6ac03 100644 --- a/src/registrar/models/domain_invitation.py +++ b/src/registrar/models/domain_invitation.py @@ -3,7 +3,7 @@ from django.contrib.auth import get_user_model from django.db import models, IntegrityError -from django_fsm import FSMField, transition +from django_fsm import FSMField, transition # type: ignore from .utility.time_stamped_model import TimeStampedModel from .user_domain_role import UserDomainRole diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index 9b30e7923..976a64a07 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -54,6 +54,7 @@ Email Date created Status + Action @@ -64,6 +65,10 @@ {{ invitation.created_at|date }} {{ invitation.status|title }} +
+ {% csrf_token %} +
+ {% endfor %} diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 75a43e6d6..74298c85a 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -172,7 +172,9 @@ class TestInvitations(TestCase): def setUp(self): self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") self.email = "mayor@igorville.gov" - self.invitation, _ = DomainInvitation.objects.get_or_create(email=self.email, domain=self.domain) + self.invitation, _ = DomainInvitation.objects.get_or_create( + email=self.email, domain=self.domain + ) self.user, _ = User.objects.get_or_create(email=self.email) def test_retrieval_creates_role(self): diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index c621cf986..78427e6b2 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -9,7 +9,15 @@ from django_webtest import WebTest # type: ignore import boto3_mocking # type: ignore -from registrar.models import DomainApplication, Domain, Contact, Website, UserDomainRole +from registrar.models import ( + DomainApplication, + Domain, + DomainInvitation, + Contact, + Website, + UserDomainRole, + User, +) from registrar.views.application import ApplicationWizard, Step from .common import less_console_noise @@ -1130,3 +1138,31 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) success_page = success_result.follow() self.assertContains(success_page, "mayor@igorville.gov") + + def test_domain_invitation_created(self): + """Add user on a nonexistent email creates an invitation.""" + # make sure there is no user with this email + EMAIL = "mayor@igorville.gov" + User.objects.filter(email=EMAIL).delete() + + add_page = self.app.get( + reverse("domain-users-add", kwargs={"pk": self.domain.id}) + ) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + add_page.form["email"] = EMAIL + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + success_result = add_page.form.submit() + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + success_page = success_result.follow() + + self.assertContains(success_page, EMAIL) + self.assertContains(success_page, "Cancel") # link to cancel invitation + self.assertTrue(DomainInvitation.objects.filter(email=EMAIL).exists()) + + def test_domain_invitation_cancel(self): + """Posting to the delete view deletes an invitation.""" + EMAIL = "mayor@igorville.gov" + invitation, _ = DomainInvitation.objects.get_or_create(domain=self.domain, email=EMAIL) + self.client.post(reverse("invitation-delete", kwargs={"pk": invitation.id})) + with self.assertRaises(DomainInvitation.DoesNotExist): + DomainInvitation.objects.get(id=invitation.id) diff --git a/src/registrar/views/__init__.py b/src/registrar/views/__init__.py index e1ae2cc32..0776f70fe 100644 --- a/src/registrar/views/__init__.py +++ b/src/registrar/views/__init__.py @@ -1,5 +1,10 @@ from .application import * -from .domain import DomainView, DomainUsersView, DomainAddUserView +from .domain import ( + DomainView, + DomainUsersView, + DomainAddUserView, + DomainInvitationDeleteView, +) from .health import * from .index import * from .whoami import * diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 0627d92df..c8acded23 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1,15 +1,16 @@ """View for a single Domain.""" -from django import forms from django.contrib import messages +from django.contrib.messages.views import SuccessMessageMixin from django.db import IntegrityError from django.shortcuts import redirect from django.urls import reverse from django.views.generic import DetailView -from django.views.generic.edit import FormMixin +from django.views.generic.edit import DeleteView, FormMixin from registrar.models import Domain, DomainInvitation, User, UserDomainRole +from ..forms import DomainAddUserForm from .utility import DomainPermission @@ -31,13 +32,6 @@ class DomainUsersView(DomainPermission, DetailView): context_object_name = "domain" -class DomainAddUserForm(DomainPermission, forms.Form): - - """Form for adding a user to a domain.""" - - email = forms.EmailField(label="Email") - - class DomainAddUserView(DomainPermission, FormMixin, DetailView): """Inside of a domain's user management, a form for adding users. @@ -64,10 +58,15 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): def _make_invitation(self, email_address): """Make a Domain invitation for this email and redirect with a message.""" - invitation, created = DomainInvitation.objects.get_or_create(email=email_address, domain=self.object) + invitation, created = DomainInvitation.objects.get_or_create( + email=email_address, domain=self.object + ) if not created: # that invitation already existed - messages.warning(self.request, f"{email_address} has already been invited to this domain.") + messages.warning( + self.request, + f"{email_address} has already been invited to this domain.", + ) else: messages.success(self.request, f"Invited {email_address} to this domain.") return redirect(self.get_success_url()) @@ -92,3 +91,14 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): messages.success(self.request, f"Added user {requested_email}.") return redirect(self.get_success_url()) + + +class DomainInvitationDeleteView(SuccessMessageMixin, DeleteView): + model = DomainInvitation + object: DomainInvitation # workaround for type mismatch in DeleteView + + def get_success_url(self): + return reverse("domain-users", kwargs={"pk": self.object.domain.id}) + + def get_success_message(self, cleaned_data): + return f"Successfully canceled invitation for {self.object.email}." From 33059e632e3d33c167ccc56a5f3969804704dd41 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Fri, 24 Mar 2023 13:43:29 -0500 Subject: [PATCH 04/22] Put messages after breadcrumb link --- src/registrar/templates/domain_base.html | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/registrar/templates/domain_base.html b/src/registrar/templates/domain_base.html index 5a6bf75ae..494491190 100644 --- a/src/registrar/templates/domain_base.html +++ b/src/registrar/templates/domain_base.html @@ -20,15 +20,6 @@
- {% if messages %} - {% for message in messages %} -
-
- {{ message }} -
-
- {% endfor %} - {% endif %} + {# messages block is under the back breadcrumb link #} + {% if messages %} + {% for message in messages %} +
+
+ {{ message }} +
+
+ {% endfor %} + {% endif %} + {% block domain_content %}

{{ domain.name }}

From 1e77bd9bf626ba5701494ea1bce30fd023b48543 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Mon, 27 Mar 2023 12:39:58 -0500 Subject: [PATCH 05/22] Fix deprecation warnings --- src/api/tests/common.py | 2 + src/djangooidc/oidc.py | 4 +- src/djangooidc/tests/common.py | 2 + src/registrar/config/settings.py | 3 + src/registrar/models/domain_application.py | 2 +- src/registrar/tests/common.py | 2 + src/registrar/tests/test_models.py | 6 +- src/registrar/tests/test_signals.py | 42 +++---- src/registrar/tests/test_views.py | 122 ++++++++++----------- 9 files changed, 97 insertions(+), 88 deletions(-) diff --git a/src/api/tests/common.py b/src/api/tests/common.py index cdf2f8f30..122965ae8 100644 --- a/src/api/tests/common.py +++ b/src/api/tests/common.py @@ -47,3 +47,5 @@ def less_console_noise(): # restore the streams for handler in handlers.values(): handler.setStream(restore[handler.name]) + # close the file we opened + devnull.close() diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index f4ce070f9..26eb54d3d 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -8,7 +8,7 @@ from django.conf import settings from django.http import HttpResponseRedirect from Cryptodome.PublicKey.RSA import importKey from jwkest.jwk import RSAKey # type: ignore -from oic import oic, rndstr +from oic import oic, rndstr, utils from oic.oauth2 import ErrorResponse from oic.oic import AuthorizationRequest, AuthorizationResponse, RegistrationResponse from oic.oic.message import AccessTokenResponse @@ -56,7 +56,7 @@ class Client(oic.Client): client_id=None, client_authn_method=CLIENT_AUTHN_METHOD, keyjar=keyjar, - verify_ssl=verify_ssl, + settings=utils.settings.OicClientSettings(verify_ssl=verify_ssl), config=None, ) # must be set after client is initialized diff --git a/src/djangooidc/tests/common.py b/src/djangooidc/tests/common.py index cdf2f8f30..122965ae8 100644 --- a/src/djangooidc/tests/common.py +++ b/src/djangooidc/tests/common.py @@ -47,3 +47,5 @@ def less_console_noise(): # restore the streams for handler in handlers.values(): handler.setStream(restore[handler.name]) + # close the file we opened + devnull.close() diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 4043c6991..2947e2a70 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -189,6 +189,9 @@ TEMPLATES = [ }, ] +# Stop using table-based default form renderer which is deprecated +FORM_RENDERER = "django.forms.renderers.DjangoDivFormRenderer" + MESSAGE_STORAGE = "django.contrib.messages.storage.session.SessionStorage" # IS_DEMO_SITE controls whether or not we show our big red "TEST SITE" banner diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index efe87abaf..ff8e8b59e 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -469,7 +469,7 @@ class DomainApplication(TimeStampedModel): nothing. """ if self.submitter is None or self.submitter.email is None: - logger.warn("Cannot send confirmation email, no submitter email address.") + logger.warning("Cannot send confirmation email, no submitter email address.") return try: send_templated_email( diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 182fe324c..332b04c0e 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -52,6 +52,8 @@ def less_console_noise(): # restore the streams for handler in handlers.values(): handler.setStream(restore[handler.name]) + # close the file we opened + devnull.close() class MockUserLogin: diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 74298c85a..e36390e56 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -31,7 +31,7 @@ class TestDomainApplication(TestCase): """Can create with just a creator.""" user, _ = User.objects.get_or_create() application = DomainApplication.objects.create(creator=user) - self.assertEquals(application.status, DomainApplication.STARTED) + self.assertEqual(application.status, DomainApplication.STARTED) def test_full_create(self): """Can create with all fields.""" @@ -116,13 +116,13 @@ class TestDomain(TestCase): def test_minimal_create(self): """Can create with just a name.""" domain = Domain.objects.create(name="igorville.gov") - self.assertEquals(domain.is_active, False) + self.assertEqual(domain.is_active, False) def test_get_status(self): """Returns proper status based on `is_active`.""" domain = Domain.objects.create(name="igorville.gov") domain.save() - self.assertEquals(None, domain.status) + self.assertEqual(None, domain.status) domain.activate() domain.save() self.assertIn("ok", domain.status) diff --git a/src/registrar/tests/test_signals.py b/src/registrar/tests/test_signals.py index ddc47e60d..86350bf08 100644 --- a/src/registrar/tests/test_signals.py +++ b/src/registrar/tests/test_signals.py @@ -19,7 +19,7 @@ class TestUserPostSave(TestCase): def test_user_created_without_matching_contact(self): """Expect 1 Contact containing data copied from User.""" - self.assertEquals(len(Contact.objects.all()), 0) + self.assertEqual(len(Contact.objects.all()), 0) user = get_user_model().objects.create( username=self.username, first_name=self.first_name, @@ -28,14 +28,14 @@ class TestUserPostSave(TestCase): phone=self.phone, ) actual = Contact.objects.get(user=user) - self.assertEquals(actual.first_name, self.first_name) - self.assertEquals(actual.last_name, self.last_name) - self.assertEquals(actual.email, self.email) - self.assertEquals(actual.phone, self.phone) + self.assertEqual(actual.first_name, self.first_name) + self.assertEqual(actual.last_name, self.last_name) + self.assertEqual(actual.email, self.email) + self.assertEqual(actual.phone, self.phone) def test_user_created_with_matching_contact(self): """Expect 1 Contact associated, but with no data copied from User.""" - self.assertEquals(len(Contact.objects.all()), 0) + self.assertEqual(len(Contact.objects.all()), 0) Contact.objects.create( first_name=self.preferred_first_name, last_name=self.preferred_last_name, @@ -49,21 +49,21 @@ class TestUserPostSave(TestCase): email=self.email, ) actual = Contact.objects.get(user=user) - self.assertEquals(actual.first_name, self.preferred_first_name) - self.assertEquals(actual.last_name, self.preferred_last_name) - self.assertEquals(actual.email, self.email) - self.assertEquals(actual.phone, self.preferred_phone) + self.assertEqual(actual.first_name, self.preferred_first_name) + self.assertEqual(actual.last_name, self.preferred_last_name) + self.assertEqual(actual.email, self.email) + self.assertEqual(actual.phone, self.preferred_phone) def test_user_updated_without_matching_contact(self): """Expect 1 Contact containing data copied from User.""" # create the user - self.assertEquals(len(Contact.objects.all()), 0) + self.assertEqual(len(Contact.objects.all()), 0) user = get_user_model().objects.create( username=self.username, first_name="", last_name="", email="", phone="" ) # delete the contact Contact.objects.all().delete() - self.assertEquals(len(Contact.objects.all()), 0) + self.assertEqual(len(Contact.objects.all()), 0) # modify the user user.username = self.username user.first_name = self.first_name @@ -73,15 +73,15 @@ class TestUserPostSave(TestCase): user.save() # test actual = Contact.objects.get(user=user) - self.assertEquals(actual.first_name, self.first_name) - self.assertEquals(actual.last_name, self.last_name) - self.assertEquals(actual.email, self.email) - self.assertEquals(actual.phone, self.phone) + self.assertEqual(actual.first_name, self.first_name) + self.assertEqual(actual.last_name, self.last_name) + self.assertEqual(actual.email, self.email) + self.assertEqual(actual.phone, self.phone) def test_user_updated_with_matching_contact(self): """Expect 1 Contact associated, but with no data copied from User.""" # create the user - self.assertEquals(len(Contact.objects.all()), 0) + self.assertEqual(len(Contact.objects.all()), 0) user = get_user_model().objects.create( username=self.username, first_name=self.first_name, @@ -97,7 +97,7 @@ class TestUserPostSave(TestCase): user.save() # test actual = Contact.objects.get(user=user) - self.assertEquals(actual.first_name, self.first_name) - self.assertEquals(actual.last_name, self.last_name) - self.assertEquals(actual.email, self.email) - self.assertEquals(actual.phone, self.phone) + self.assertEqual(actual.first_name, self.first_name) + self.assertEqual(actual.last_name, self.last_name) + self.assertEqual(actual.email, self.email) + self.assertEqual(actual.phone, self.phone) diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 78427e6b2..10014db8e 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -161,11 +161,11 @@ class DomainApplicationTests(TestWithUser, WebTest): type_result = type_page.form.submit() # should see results in db application = DomainApplication.objects.get() # there's only one - self.assertEquals(application.organization_type, "federal") + self.assertEqual(application.organization_type, "federal") # the post request should return a redirect to the next form in # the application - self.assertEquals(type_result.status_code, 302) - self.assertEquals(type_result["Location"], "/register/organization_federal/") + self.assertEqual(type_result.status_code, 302) + self.assertEqual(type_result["Location"], "/register/organization_federal/") num_pages_tested += 1 # ---- FEDERAL BRANCH PAGE ---- @@ -180,11 +180,11 @@ class DomainApplicationTests(TestWithUser, WebTest): federal_result = federal_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals(application.federal_type, "executive") + self.assertEqual(application.federal_type, "executive") # the post request should return a redirect to the next form in # the application - self.assertEquals(federal_result.status_code, 302) - self.assertEquals(federal_result["Location"], "/register/organization_contact/") + self.assertEqual(federal_result.status_code, 302) + self.assertEqual(federal_result["Location"], "/register/organization_contact/") num_pages_tested += 1 # ---- ORG CONTACT PAGE ---- @@ -209,17 +209,17 @@ class DomainApplicationTests(TestWithUser, WebTest): org_contact_result = org_contact_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals(application.organization_name, "Testorg") - self.assertEquals(application.address_line1, "address 1") - self.assertEquals(application.address_line2, "address 2") - self.assertEquals(application.city, "NYC") - self.assertEquals(application.state_territory, "NY") - self.assertEquals(application.zipcode, "10002") - self.assertEquals(application.urbanization, "URB Royal Oaks") + self.assertEqual(application.organization_name, "Testorg") + self.assertEqual(application.address_line1, "address 1") + self.assertEqual(application.address_line2, "address 2") + self.assertEqual(application.city, "NYC") + self.assertEqual(application.state_territory, "NY") + self.assertEqual(application.zipcode, "10002") + self.assertEqual(application.urbanization, "URB Royal Oaks") # the post request should return a redirect to the next form in # the application - self.assertEquals(org_contact_result.status_code, 302) - self.assertEquals( + self.assertEqual(org_contact_result.status_code, 302) + self.assertEqual( org_contact_result["Location"], "/register/authorizing_official/" ) num_pages_tested += 1 @@ -240,15 +240,15 @@ class DomainApplicationTests(TestWithUser, WebTest): ao_result = ao_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals(application.authorizing_official.first_name, "Testy ATO") - self.assertEquals(application.authorizing_official.last_name, "Tester ATO") - self.assertEquals(application.authorizing_official.title, "Chief Tester") - self.assertEquals(application.authorizing_official.email, "testy@town.com") - self.assertEquals(application.authorizing_official.phone, "(201) 555 5555") + self.assertEqual(application.authorizing_official.first_name, "Testy ATO") + self.assertEqual(application.authorizing_official.last_name, "Tester ATO") + self.assertEqual(application.authorizing_official.title, "Chief Tester") + self.assertEqual(application.authorizing_official.email, "testy@town.com") + self.assertEqual(application.authorizing_official.phone, "(201) 555 5555") # the post request should return a redirect to the next form in # the application - self.assertEquals(ao_result.status_code, 302) - self.assertEquals(ao_result["Location"], "/register/current_sites/") + self.assertEqual(ao_result.status_code, 302) + self.assertEqual(ao_result["Location"], "/register/current_sites/") num_pages_tested += 1 # ---- CURRENT SITES PAGE ---- @@ -263,14 +263,14 @@ class DomainApplicationTests(TestWithUser, WebTest): current_sites_result = current_sites_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals( + self.assertEqual( application.current_websites.filter(website="http://www.city.com").count(), 1, ) # the post request should return a redirect to the next form in # the application - self.assertEquals(current_sites_result.status_code, 302) - self.assertEquals(current_sites_result["Location"], "/register/dotgov_domain/") + self.assertEqual(current_sites_result.status_code, 302) + self.assertEqual(current_sites_result["Location"], "/register/dotgov_domain/") num_pages_tested += 1 # ---- DOTGOV DOMAIN PAGE ---- @@ -285,14 +285,14 @@ class DomainApplicationTests(TestWithUser, WebTest): dotgov_result = dotgov_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals(application.requested_domain.name, "city.gov") - self.assertEquals( + self.assertEqual(application.requested_domain.name, "city.gov") + self.assertEqual( application.alternative_domains.filter(website="city1.gov").count(), 1 ) # the post request should return a redirect to the next form in # the application - self.assertEquals(dotgov_result.status_code, 302) - self.assertEquals(dotgov_result["Location"], "/register/purpose/") + self.assertEqual(dotgov_result.status_code, 302) + self.assertEqual(dotgov_result["Location"], "/register/purpose/") num_pages_tested += 1 # ---- PURPOSE PAGE ---- @@ -307,11 +307,11 @@ class DomainApplicationTests(TestWithUser, WebTest): purpose_result = purpose_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals(application.purpose, "For all kinds of things.") + self.assertEqual(application.purpose, "For all kinds of things.") # the post request should return a redirect to the next form in # the application - self.assertEquals(purpose_result.status_code, 302) - self.assertEquals(purpose_result["Location"], "/register/your_contact/") + self.assertEqual(purpose_result.status_code, 302) + self.assertEqual(purpose_result["Location"], "/register/your_contact/") num_pages_tested += 1 # ---- YOUR CONTACT INFO PAGE ---- @@ -331,15 +331,15 @@ class DomainApplicationTests(TestWithUser, WebTest): your_contact_result = your_contact_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals(application.submitter.first_name, "Testy you") - self.assertEquals(application.submitter.last_name, "Tester you") - self.assertEquals(application.submitter.title, "Admin Tester") - self.assertEquals(application.submitter.email, "testy-admin@town.com") - self.assertEquals(application.submitter.phone, "(201) 555 5556") + self.assertEqual(application.submitter.first_name, "Testy you") + self.assertEqual(application.submitter.last_name, "Tester you") + self.assertEqual(application.submitter.title, "Admin Tester") + self.assertEqual(application.submitter.email, "testy-admin@town.com") + self.assertEqual(application.submitter.phone, "(201) 555 5556") # the post request should return a redirect to the next form in # the application - self.assertEquals(your_contact_result.status_code, 302) - self.assertEquals(your_contact_result["Location"], "/register/other_contacts/") + self.assertEqual(your_contact_result.status_code, 302) + self.assertEqual(your_contact_result["Location"], "/register/other_contacts/") num_pages_tested += 1 # ---- OTHER CONTACTS PAGE ---- @@ -359,7 +359,7 @@ class DomainApplicationTests(TestWithUser, WebTest): other_contacts_result = other_contacts_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals( + self.assertEqual( application.other_contacts.filter( first_name="Testy2", last_name="Tester2", @@ -371,8 +371,8 @@ class DomainApplicationTests(TestWithUser, WebTest): ) # the post request should return a redirect to the next form in # the application - self.assertEquals(other_contacts_result.status_code, 302) - self.assertEquals(other_contacts_result["Location"], "/register/anything_else/") + self.assertEqual(other_contacts_result.status_code, 302) + self.assertEqual(other_contacts_result["Location"], "/register/anything_else/") num_pages_tested += 1 # ---- ANYTHING ELSE PAGE ---- @@ -388,11 +388,11 @@ class DomainApplicationTests(TestWithUser, WebTest): anything_else_result = anything_else_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals(application.anything_else, "Nothing else.") + self.assertEqual(application.anything_else, "Nothing else.") # the post request should return a redirect to the next form in # the application - self.assertEquals(anything_else_result.status_code, 302) - self.assertEquals(anything_else_result["Location"], "/register/requirements/") + self.assertEqual(anything_else_result.status_code, 302) + self.assertEqual(anything_else_result["Location"], "/register/requirements/") num_pages_tested += 1 # ---- REQUIREMENTS PAGE ---- @@ -408,11 +408,11 @@ class DomainApplicationTests(TestWithUser, WebTest): requirements_result = requirements_form.submit() # validate that data from this step are being saved application = DomainApplication.objects.get() # there's only one - self.assertEquals(application.is_policy_acknowledged, True) + self.assertEqual(application.is_policy_acknowledged, True) # the post request should return a redirect to the next form in # the application - self.assertEquals(requirements_result.status_code, 302) - self.assertEquals(requirements_result["Location"], "/register/review/") + self.assertEqual(requirements_result.status_code, 302) + self.assertEqual(requirements_result["Location"], "/register/review/") num_pages_tested += 1 # ---- REVIEW AND FINSIHED PAGES ---- @@ -457,8 +457,8 @@ class DomainApplicationTests(TestWithUser, WebTest): with less_console_noise(): review_result = review_form.submit() - self.assertEquals(review_result.status_code, 302) - self.assertEquals(review_result["Location"], "/register/finished/") + self.assertEqual(review_result.status_code, 302) + self.assertEqual(review_result["Location"], "/register/finished/") num_pages_tested += 1 # following this redirect is a GET request, so include the cookie @@ -494,8 +494,8 @@ class DomainApplicationTests(TestWithUser, WebTest): # the post request should return a redirect to the federal branch # question - self.assertEquals(type_result.status_code, 302) - self.assertEquals(type_result["Location"], "/register/organization_federal/") + self.assertEqual(type_result.status_code, 302) + self.assertEqual(type_result["Location"], "/register/organization_federal/") # and the step label should appear in the sidebar of the resulting page # but the step label for the elections page should not appear @@ -511,8 +511,8 @@ class DomainApplicationTests(TestWithUser, WebTest): federal_result = federal_page.form.submit() # the post request should return a redirect to the contact # question - self.assertEquals(federal_result.status_code, 302) - self.assertEquals(federal_result["Location"], "/register/organization_contact/") + self.assertEqual(federal_result.status_code, 302) + self.assertEqual(federal_result["Location"], "/register/organization_contact/") self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) contact_page = federal_result.follow() self.assertContains(contact_page, "Federal agency") @@ -539,8 +539,8 @@ class DomainApplicationTests(TestWithUser, WebTest): type_result = type_form.submit() # the post request should return a redirect to the elections question - self.assertEquals(type_result.status_code, 302) - self.assertEquals(type_result["Location"], "/register/organization_election/") + self.assertEqual(type_result.status_code, 302) + self.assertEqual(type_result["Location"], "/register/organization_election/") # and the step label should appear in the sidebar of the resulting page # but the step label for the elections page should not appear @@ -556,8 +556,8 @@ class DomainApplicationTests(TestWithUser, WebTest): election_result = election_page.form.submit() # the post request should return a redirect to the contact # question - self.assertEquals(election_result.status_code, 302) - self.assertEquals( + self.assertEqual(election_result.status_code, 302) + self.assertEqual( election_result["Location"], "/register/organization_contact/" ) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) @@ -627,8 +627,8 @@ class DomainApplicationTests(TestWithUser, WebTest): # the post request should return a redirect to the type of work page # if it was successful. - self.assertEquals(contact_result.status_code, 302) - self.assertEquals(contact_result["Location"], "/register/type_of_work/") + self.assertEqual(contact_result.status_code, 302) + self.assertEqual(contact_result["Location"], "/register/type_of_work/") def test_application_type_of_work_special(self): """Special districts have to answer an additional question.""" @@ -893,7 +893,7 @@ class DomainApplicationTests(TestWithUser, WebTest): self.assertIn("current_sites-1-website", current_sites_form.fields) # and it is correctly referenced in the ManyToOne relationship application = DomainApplication.objects.get() # there's only one - self.assertEquals( + self.assertEqual( application.current_websites.filter(website="https://example.com").count(), 1, ) From 8338704315e3446ced1ac09c85072643f8cc8783 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Mon, 27 Mar 2023 15:20:08 -0500 Subject: [PATCH 06/22] Send invitation email when invite is created --- src/registrar/models/domain_application.py | 4 +- .../templates/emails/domain_invitation.txt | 6 +++ src/registrar/tests/test_views.py | 40 ++++++++++++++++--- src/registrar/views/domain.py | 21 +++++++++- 4 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 src/registrar/templates/emails/domain_invitation.txt diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index ff8e8b59e..08343e863 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -469,7 +469,9 @@ class DomainApplication(TimeStampedModel): nothing. """ if self.submitter is None or self.submitter.email is None: - logger.warning("Cannot send confirmation email, no submitter email address.") + logger.warning( + "Cannot send confirmation email, no submitter email address." + ) return try: send_templated_email( diff --git a/src/registrar/templates/emails/domain_invitation.txt b/src/registrar/templates/emails/domain_invitation.txt new file mode 100644 index 000000000..c72730f2d --- /dev/null +++ b/src/registrar/templates/emails/domain_invitation.txt @@ -0,0 +1,6 @@ +You have been invited to manage a domain on get.gov, the registrar for +.gov domain names. + +To accept your invitation, go to <{{ domain_url }}>. + +You will need to log in with a Login.gov account using this email address. diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 10014db8e..8f7a7e54e 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -1,4 +1,5 @@ from unittest import skip +from unittest.mock import MagicMock, ANY from django.conf import settings from django.test import Client, TestCase @@ -557,9 +558,7 @@ class DomainApplicationTests(TestWithUser, WebTest): # the post request should return a redirect to the contact # question self.assertEqual(election_result.status_code, 302) - self.assertEqual( - election_result["Location"], "/register/organization_contact/" - ) + self.assertEqual(election_result["Location"], "/register/organization_contact/") self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) contact_page = election_result.follow() self.assertNotContains(contact_page, "Federal agency") @@ -1139,8 +1138,13 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): success_page = success_result.follow() self.assertContains(success_page, "mayor@igorville.gov") + @boto3_mocking.patching def test_domain_invitation_created(self): - """Add user on a nonexistent email creates an invitation.""" + """Add user on a nonexistent email creates an invitation. + + Adding a non-existent user sends an email as a side-effect, so mock + out the boto3 SES email sending here. + """ # make sure there is no user with this email EMAIL = "mayor@igorville.gov" User.objects.filter(email=EMAIL).delete() @@ -1159,10 +1163,36 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): self.assertContains(success_page, "Cancel") # link to cancel invitation self.assertTrue(DomainInvitation.objects.filter(email=EMAIL).exists()) + @boto3_mocking.patching + def test_domain_invitation_email_sent(self): + """Inviting a non-existent user sends them an email.""" + # make sure there is no user with this email + EMAIL = "mayor@igorville.gov" + User.objects.filter(email=EMAIL).delete() + + 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}) + ) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + add_page.form["email"] = EMAIL + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + add_page.form.submit() + # check the mock instance to see if `send_email` was called right + mock_client_instance.send_email.assert_called_once_with( + FromEmailAddress=settings.DEFAULT_FROM_EMAIL, + Destination={"ToAddresses": [EMAIL]}, + Content=ANY, + ) + def test_domain_invitation_cancel(self): """Posting to the delete view deletes an invitation.""" EMAIL = "mayor@igorville.gov" - invitation, _ = DomainInvitation.objects.get_or_create(domain=self.domain, email=EMAIL) + invitation, _ = DomainInvitation.objects.get_or_create( + domain=self.domain, email=EMAIL + ) self.client.post(reverse("invitation-delete", kwargs={"pk": invitation.id})) with self.assertRaises(DomainInvitation.DoesNotExist): DomainInvitation.objects.get(id=invitation.id) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index c8acded23..b235dd166 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -11,6 +11,7 @@ from django.views.generic.edit import DeleteView, FormMixin from registrar.models import Domain, DomainInvitation, User, UserDomainRole from ..forms import DomainAddUserForm +from ..utility.email import send_templated_email, EmailSendingError from .utility import DomainPermission @@ -56,6 +57,12 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): else: return self.form_invalid(form) + def _domain_abs_url(self): + """Get an absolute URL for this domain.""" + return self.request.build_absolute_uri( + reverse("domain", kwargs={"pk": self.object.id}) + ) + def _make_invitation(self, email_address): """Make a Domain invitation for this email and redirect with a message.""" invitation, created = DomainInvitation.objects.get_or_create( @@ -68,7 +75,19 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): f"{email_address} has already been invited to this domain.", ) else: - messages.success(self.request, f"Invited {email_address} to this domain.") + # created a new invitation in the database, so send an email + try: + send_templated_email( + "emails/domain_invitation.txt", + to_address=email_address, + context={"domain_url": self._domain_abs_url()}, + ) + except EmailSendingError: + messages.warning(self.request, "Could not send email invitation.") + else: + messages.success( + self.request, f"Invited {email_address} to this domain." + ) return redirect(self.get_success_url()) def form_valid(self, form): From e54dda3ddd7d76b421f83a074b106630bf7cd801 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Mon, 27 Mar 2023 16:10:33 -0500 Subject: [PATCH 07/22] Retrieve domain invitations on first login --- src/djangooidc/backends.py | 2 ++ src/registrar/models/user.py | 13 +++++++++++++ src/registrar/tests/test_models.py | 5 +++++ src/registrar/tests/test_views.py | 25 +++++++++++++++++++++++++ 4 files changed, 45 insertions(+) diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index ac65c89d4..08ca47d2e 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -49,6 +49,8 @@ class OpenIdConnectBackend(ModelBackend): user, created = UserModel.objects.update_or_create(**args) if created: user = self.configure_user(user, **kwargs) + # run a newly created user's callback for a first-time login + user.first_login() else: try: user = UserModel.objects.get_by_natural_key(username) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 97f5753b3..a3ab68511 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -1,6 +1,8 @@ from django.contrib.auth.models import AbstractUser from django.db import models +from .domain_invitation import DomainInvitation + from phonenumber_field.modelfields import PhoneNumberField # type: ignore @@ -31,3 +33,14 @@ class User(AbstractUser): return self.email else: return self.username + + def first_login(self): + """Callback when the user is authenticated for the very first time. + + When a user first arrives on the site, we need to retrieve any domain + invitations that match their email address. + """ + for invitation in DomainInvitation.objects.filter( + email=self.email, status=DomainInvitation.SENT + ): + invitation.retrieve() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index e36390e56..b60898e50 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -193,6 +193,11 @@ class TestInvitations(TestCase): with self.assertRaises(RuntimeError): self.invitation.retrieve() + def test_retrieve_on_first_login(self): + """A new user's first_login callback retrieves their invitations.""" + self.user.first_login() + self.assertTrue(UserDomainRole.objects.get(user=self.user, domain=self.domain)) + @skip("Not implemented yet.") class TestDomainApplicationLifeCycle(TestCase): diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 8f7a7e54e..7f3dc6429 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -1196,3 +1196,28 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): self.client.post(reverse("invitation-delete", kwargs={"pk": invitation.id})) with self.assertRaises(DomainInvitation.DoesNotExist): DomainInvitation.objects.get(id=invitation.id) + + @boto3_mocking.patching + def test_domain_invitation_flow(self): + """Send an invitation to a new user, log in and load the dashboard.""" + EMAIL = "mayor@igorville.gov" + User.objects.filter(email=EMAIL).delete() + + add_page = self.app.get( + reverse("domain-users-add", kwargs={"pk": self.domain.id}) + ) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + add_page.form["email"] = EMAIL + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + add_page.form.submit() + + # user was invited, create them + new_user = User.objects.create(username=EMAIL, email=EMAIL) + # log them in to `self.app` + self.app.set_user(new_user.username) + # and manually call the first login callback + new_user.first_login() + + # Now load the home page and make sure our domain appears there + home_page = self.app.get(reverse("home")) + self.assertContains(home_page, self.domain.name) From 2693641f2b35fcc77bb25c34653c31181be1588b Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Tue, 28 Mar 2023 13:53:32 -0500 Subject: [PATCH 08/22] Template email subject lines --- src/registrar/models/domain_application.py | 1 + .../templates/emails/domain_invitation.subject.txt | 1 + .../emails/submission_confirmation.subject.txt | 1 + src/registrar/utility/email.py | 14 ++++++++++---- src/registrar/views/domain.py | 6 +++++- 5 files changed, 18 insertions(+), 5 deletions(-) create mode 100644 src/registrar/templates/emails/domain_invitation.subject.txt create mode 100644 src/registrar/templates/emails/submission_confirmation.subject.txt diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 08343e863..198fffa48 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -476,6 +476,7 @@ class DomainApplication(TimeStampedModel): try: send_templated_email( "emails/submission_confirmation.txt", + "emails/submission_confirmation.subject.txt", self.submitter.email, context={"id": self.id, "domain_name": self.requested_domain.name}, ) diff --git a/src/registrar/templates/emails/domain_invitation.subject.txt b/src/registrar/templates/emails/domain_invitation.subject.txt new file mode 100644 index 000000000..60db880de --- /dev/null +++ b/src/registrar/templates/emails/domain_invitation.subject.txt @@ -0,0 +1 @@ +You are invited to manage {{ domain.name }} on get.gov diff --git a/src/registrar/templates/emails/submission_confirmation.subject.txt b/src/registrar/templates/emails/submission_confirmation.subject.txt new file mode 100644 index 000000000..34b958b3a --- /dev/null +++ b/src/registrar/templates/emails/submission_confirmation.subject.txt @@ -0,0 +1 @@ +Thank you for applying for a .gov domain diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index e6de4c330..6491008cf 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -13,16 +13,22 @@ class EmailSendingError(RuntimeError): pass -def send_templated_email(template_name: str, to_address: str, context={}): +def send_templated_email( + template_name: str, subject_template_name: str, to_address: str, context={} +): """Send an email built from a template to one email address. - template_name is relative to the same template context as Django's HTML - templates. context gives additional information that the template may use. + template_name and subject_template_name are relative to the same template + context as Django's HTML templates. context gives additional information + that the template may use. """ template = get_template(template_name) email_body = template.render(context=context) + subject_template = get_template(subject_template_name) + subject = subject_template.render(context=context) + try: ses_client = boto3.client( "sesv2", @@ -40,7 +46,7 @@ def send_templated_email(template_name: str, to_address: str, context={}): Destination={"ToAddresses": [to_address]}, Content={ "Simple": { - "Subject": {"Data": "Thank you for applying for a .gov domain"}, + "Subject": {"Data": subject}, "Body": {"Text": {"Data": email_body}}, }, }, diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index b235dd166..dcf864cbc 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -79,8 +79,12 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): try: send_templated_email( "emails/domain_invitation.txt", + "emails/domain_invitation.subject.txt", to_address=email_address, - context={"domain_url": self._domain_abs_url()}, + context={ + "domain_url": self._domain_abs_url(), + "domain": self.object, + }, ) except EmailSendingError: messages.warning(self.request, "Could not send email invitation.") From 64553e0d01a9b870f196dff53661abe2e106379b Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Tue, 28 Mar 2023 13:56:59 -0500 Subject: [PATCH 09/22] Tweak templates and save invitation status change --- src/registrar/admin.py | 2 ++ src/registrar/fixtures.py | 4 ++++ src/registrar/models/user.py | 1 + src/registrar/templates/domain_add_user.html | 7 ------- src/registrar/templates/emails/domain_invitation.txt | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index b2d291ce5..30c8e8b89 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -51,7 +51,9 @@ class MyHostAdmin(AuditedAdmin): admin.site.register(models.User, MyUserAdmin) +admin.site.register(models.UserDomainRole, AuditedAdmin) admin.site.register(models.Contact, AuditedAdmin) +admin.site.register(models.DomainInvitation, AuditedAdmin) admin.site.register(models.DomainApplication, AuditedAdmin) admin.site.register(models.Domain, AuditedAdmin) admin.site.register(models.Host, MyHostAdmin) diff --git a/src/registrar/fixtures.py b/src/registrar/fixtures.py index 4edca7cf6..e2ced373d 100644 --- a/src/registrar/fixtures.py +++ b/src/registrar/fixtures.py @@ -39,6 +39,10 @@ class UserFixture: "first_name": "Logan", "last_name": "", }, + { + "username": "2ffe71b0-cea4-4097-8fb6-7a35b901dd70", + "first_name": "Neil", + "last_name": "Martinsen-Burrell", ] @classmethod diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index a3ab68511..963771ec3 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -44,3 +44,4 @@ class User(AbstractUser): email=self.email, status=DomainInvitation.SENT ): invitation.retrieve() + invitation.save() diff --git a/src/registrar/templates/domain_add_user.html b/src/registrar/templates/domain_add_user.html index 1a1c45aa3..87b141570 100644 --- a/src/registrar/templates/domain_add_user.html +++ b/src/registrar/templates/domain_add_user.html @@ -4,13 +4,6 @@ {% block title %}Add another user{% endblock %} {% block domain_content %} -

- - Back to user management - -

Add another user

You can add another user to help manage your domain. They will need to sign diff --git a/src/registrar/templates/emails/domain_invitation.txt b/src/registrar/templates/emails/domain_invitation.txt index c72730f2d..8bfb53933 100644 --- a/src/registrar/templates/emails/domain_invitation.txt +++ b/src/registrar/templates/emails/domain_invitation.txt @@ -1,5 +1,5 @@ -You have been invited to manage a domain on get.gov, the registrar for -.gov domain names. +You have been invited to manage the domain {{ domain.name }} on get.gov, +the registrar for .gov domain names. To accept your invitation, go to <{{ domain_url }}>. From 58977f14f2f72e4bae933bf58ad5e46a9666e5a4 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Tue, 28 Mar 2023 14:33:20 -0500 Subject: [PATCH 10/22] Fix formatting oops --- src/registrar/fixtures.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/fixtures.py b/src/registrar/fixtures.py index e2ced373d..4f56a2336 100644 --- a/src/registrar/fixtures.py +++ b/src/registrar/fixtures.py @@ -43,6 +43,7 @@ class UserFixture: "username": "2ffe71b0-cea4-4097-8fb6-7a35b901dd70", "first_name": "Neil", "last_name": "Martinsen-Burrell", + }, ] @classmethod From a121108506452d337100b40b4e8a26c2bfa804d9 Mon Sep 17 00:00:00 2001 From: igorkorenfeld Date: Wed, 29 Mar 2023 17:21:50 -0400 Subject: [PATCH 11/22] Change title block on base form, add image on review --- .../sass/_theme/_uswds-theme-custom-styles.scss | 4 ---- src/registrar/templates/application_form.html | 4 +++- src/registrar/templates/application_review.html | 12 ++++++++++++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/sass/_theme/_uswds-theme-custom-styles.scss b/src/registrar/assets/sass/_theme/_uswds-theme-custom-styles.scss index 806cacbd3..f1146c14e 100644 --- a/src/registrar/assets/sass/_theme/_uswds-theme-custom-styles.scss +++ b/src/registrar/assets/sass/_theme/_uswds-theme-custom-styles.scss @@ -177,10 +177,6 @@ a.breadcrumb__back { .review__step { margin-top: units(3); - - &:first-of-type { - margin-top: units(4); - } } .review__step hr { diff --git a/src/registrar/templates/application_form.html b/src/registrar/templates/application_form.html index ccbdb4ffb..d1f9598b3 100644 --- a/src/registrar/templates/application_form.html +++ b/src/registrar/templates/application_form.html @@ -41,7 +41,9 @@ {% endfor %} {% endblock %} +{% block form_page_title %}

{{form_titles|get_item:steps.current}}

+{% endblock %} {% block form_instructions %} {% endblock %} @@ -81,4 +83,4 @@
-{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/src/registrar/templates/application_review.html b/src/registrar/templates/application_review.html index 809de33f0..ca306646a 100644 --- a/src/registrar/templates/application_review.html +++ b/src/registrar/templates/application_review.html @@ -5,6 +5,18 @@ {# there are no required fields on this page so don't show this #} {% endblock %} +{% block form_page_title %} + + +

Review and submit your domain request

+ +{% endblock %} + {% block form_fields %} {% for step in steps.all|slice:":-1" %}
From e8c8dc6aaa148071626d8994d6fc016e659b6415 Mon Sep 17 00:00:00 2001 From: igorkorenfeld Date: Thu, 30 Mar 2023 10:47:52 -0400 Subject: [PATCH 12/22] Change submit button to be large and green --- .../sass/_theme/_uswds-theme-custom-styles.scss | 11 +++++++++++ src/registrar/assets/sass/_theme/_uswds-theme.scss | 1 + src/registrar/templates/application_form.html | 2 +- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/registrar/assets/sass/_theme/_uswds-theme-custom-styles.scss b/src/registrar/assets/sass/_theme/_uswds-theme-custom-styles.scss index f1146c14e..135683df4 100644 --- a/src/registrar/assets/sass/_theme/_uswds-theme-custom-styles.scss +++ b/src/registrar/assets/sass/_theme/_uswds-theme-custom-styles.scss @@ -196,6 +196,17 @@ a.breadcrumb__back { margin-bottom: units(0.5); } +.dotgov-button--green { + background-color: color('success-dark'); + + &:hover { + background-color: color('success-darker'); + } + + &:active { + background-color: color('green-80v'); + } +} /** ---- DASHBOARD ---- */ diff --git a/src/registrar/assets/sass/_theme/_uswds-theme.scss b/src/registrar/assets/sass/_theme/_uswds-theme.scss index ee514518a..ba076d845 100644 --- a/src/registrar/assets/sass/_theme/_uswds-theme.scss +++ b/src/registrar/assets/sass/_theme/_uswds-theme.scss @@ -81,6 +81,7 @@ in the form $setting: value, ------------------------------ ## Primary color ----------------------------*/ + $theme-color-primary-darkest: $dhs-blue-80, $theme-color-primary-darker: $dhs-blue-70, $theme-color-primary-dark: $dhs-blue-60, $theme-color-primary: $dhs-blue, diff --git a/src/registrar/templates/application_form.html b/src/registrar/templates/application_form.html index d1f9598b3..3697235f1 100644 --- a/src/registrar/templates/application_form.html +++ b/src/registrar/templates/application_form.html @@ -69,7 +69,7 @@ {% else %} {% endif %} From 793da58a32d2fe06d017804d05879d93f02a266d Mon Sep 17 00:00:00 2001 From: igorkorenfeld Date: Thu, 30 Mar 2023 12:30:52 -0400 Subject: [PATCH 13/22] Add review page graphic (previously ignored) --- .../assets/img/registrar/dotgov_review_magnify.svg | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 src/registrar/assets/img/registrar/dotgov_review_magnify.svg diff --git a/src/registrar/assets/img/registrar/dotgov_review_magnify.svg b/src/registrar/assets/img/registrar/dotgov_review_magnify.svg new file mode 100644 index 000000000..7560ebaf9 --- /dev/null +++ b/src/registrar/assets/img/registrar/dotgov_review_magnify.svg @@ -0,0 +1,11 @@ + + + + + + + + + + + From e61bc5212846b0b87261b5b89e5b9602dd4ae080 Mon Sep 17 00:00:00 2001 From: igorkorenfeld Date: Thu, 30 Mar 2023 14:46:52 -0400 Subject: [PATCH 14/22] Remove non-breaking space, limit title width --- src/registrar/templates/application_review.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/application_review.html b/src/registrar/templates/application_review.html index ca306646a..b9ac97871 100644 --- a/src/registrar/templates/application_review.html +++ b/src/registrar/templates/application_review.html @@ -6,14 +6,14 @@ {% endblock %} {% block form_page_title %} - + -

Review and submit your domain request

+

Review and submit your domain request

{% endblock %} From ba7e97ba18eed43764372beda022452246212945 Mon Sep 17 00:00:00 2001 From: Seamus Johnston Date: Fri, 31 Mar 2023 09:21:02 -0500 Subject: [PATCH 15/22] Begin using requirements.txt --- .../runbooks/update_python_dependencies.md | 17 +++++-- src/Dockerfile | 2 +- src/requirements.txt | 49 +++++++++++++++++++ 3 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 src/requirements.txt diff --git a/docs/operations/runbooks/update_python_dependencies.md b/docs/operations/runbooks/update_python_dependencies.md index 984b22407..16475d3db 100644 --- a/docs/operations/runbooks/update_python_dependencies.md +++ b/docs/operations/runbooks/update_python_dependencies.md @@ -1,8 +1,19 @@ # HOWTO Update Python Dependencies ======================== -1. Check the [Pipfile](./src/Pipfile) for pinned dependencies and manually adjust the version numbers -1. Run `cd src`, `docker-compose up -d`, and `docker-compose exec app pipenv update` to perform the upgrade and generate a new [Pipfile.lock](./src/Pipfile.lock) +1. Check the [Pipfile](../../../src/Pipfile) for pinned dependencies and manually adjust the version numbers + +1. Run + + cd src + docker-compose run app bash -c "pipenv lock && pipenv requirements > requirements.txt" + + This will generate a new [Pipfile.lock](../../../src/Pipfile.lock) and create a new [requirements.txt](../../../src/requirements.txt). It will not install anything. + + It is necessary to use `bash -c` because `run pipenv requirements` will not recognize that it is running non-interactively and will include garbage formatting characters. + + The requirements.txt is used by Cloud.gov. It is needed to work around a bug in the CloudFoundry buildpack version of Pipenv that breaks on installing from a git repository. + 1. (optional) Run `docker-compose stop` and `docker-compose build` to build a new image for local development with the updated dependencies. -The reason for de-coupling the `build` and `update` steps is to increase consistency between builds and reduce "it works on my laptop!". Therefore, `build` uses the lock file as-is; dependencies are never updated except by explicit choice. \ No newline at end of file + The reason for de-coupling the `build` and `lock` steps is to increase consistency between builds--a run of `build` will always get exactly the dependencies listed in `Pipfile.lock`, nothing more, nothing less. \ No newline at end of file diff --git a/src/Dockerfile b/src/Dockerfile index e4cc5230e..00832166c 100644 --- a/src/Dockerfile +++ b/src/Dockerfile @@ -8,4 +8,4 @@ COPY Pipfile Pipfile COPY Pipfile.lock Pipfile.lock RUN pip install pipenv -RUN pipenv install --system --dev +RUN pipenv sync --system --dev diff --git a/src/requirements.txt b/src/requirements.txt new file mode 100644 index 000000000..62220bc36 --- /dev/null +++ b/src/requirements.txt @@ -0,0 +1,49 @@ +-i https://pypi.python.org/simple +asgiref==3.6.0 ; python_version >= '3.7' +boto3==1.26.69 +botocore==1.29.69 ; python_version >= '3.7' +cachetools==5.3.0 +certifi==2022.12.7 ; python_version >= '3.6' +cfenv==0.5.3 +cffi==1.15.1 +charset-normalizer==3.0.1 ; python_version >= '3.6' +cryptography==39.0.1 ; python_version >= '3.6' +defusedxml==0.7.1 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4' +dj-database-url==1.2.0 +dj-email-url==1.0.6 +django==4.1.6 +django-allow-cidr==0.6.0 +django-auditlog==2.2.2 +django-cache-url==3.4.4 +django-csp==3.7 +django-fsm==2.8.1 +django-phonenumber-field[phonenumberslite]==7.0.2 +django-widget-tweaks==1.4.12 +environs[django]==9.5.0 +faker==17.0.0 +furl==2.1.3 +future==0.18.3 ; python_version >= '2.6' and python_version not in '3.0, 3.1, 3.2, 3.3' +gunicorn==20.1.0 +idna==3.4 ; python_version >= '3.5' +jmespath==1.0.1 ; python_version >= '3.7' +mako==1.2.4 ; python_version >= '3.7' +markupsafe==2.1.2 ; python_version >= '3.7' +marshmallow==3.19.0 ; python_version >= '3.7' +oic==1.5.0 +orderedmultidict==1.0.1 +packaging==23.0 ; python_version >= '3.7' +phonenumberslite==8.13.6 +psycopg2-binary==2.9.5 +pycparser==2.21 +pycryptodomex==3.17 +pyjwkest==1.4.2 +python-dateutil==2.8.2 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3' +python-dotenv==0.21.1 ; python_version >= '3.7' +requests==2.28.2 +s3transfer==0.6.0 ; python_version >= '3.7' +setuptools==67.2.0 ; python_version >= '3.7' +six==1.16.0 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3' +sqlparse==0.4.3 ; python_version >= '3.5' +typing-extensions==4.4.0 ; python_version >= '3.7' +urllib3==1.26.14 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5' +whitenoise==6.3.0 From f66df2c931e1488902e517d2b1785779959241bd Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Mon, 3 Apr 2023 15:04:53 -0500 Subject: [PATCH 16/22] Review feedback: name changes --- src/registrar/models/domain_invitation.py | 8 ++++---- src/registrar/models/user.py | 2 +- ...on.subject.txt => submission_confirmation_subject.txt} | 0 3 files changed, 5 insertions(+), 5 deletions(-) rename src/registrar/templates/emails/{submission_confirmation.subject.txt => submission_confirmation_subject.txt} (100%) diff --git a/src/registrar/models/domain_invitation.py b/src/registrar/models/domain_invitation.py index ba9b6ac03..1ad150e4a 100644 --- a/src/registrar/models/domain_invitation.py +++ b/src/registrar/models/domain_invitation.py @@ -10,7 +10,7 @@ from .user_domain_role import UserDomainRole class DomainInvitation(TimeStampedModel): - SENT = "sent" + INVITED = "invited" RETRIEVED = "retrieved" email = models.EmailField( @@ -27,17 +27,17 @@ class DomainInvitation(TimeStampedModel): status = FSMField( choices=[ - (SENT, SENT), + (INVITED, INVITED), (RETRIEVED, RETRIEVED), ], - default=SENT, + default=INVITED, protected=True, # can't alter state except through transition methods! ) def __str__(self): return f"Invitation for {self.email} on {self.domain} is {self.status}" - @transition(field="status", source=SENT, target=RETRIEVED) + @transition(field="status", source=INVITED, target=RETRIEVED) def retrieve(self): """When an invitation is retrieved, create the corresponding permission.""" diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 963771ec3..fb51af30c 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -41,7 +41,7 @@ class User(AbstractUser): invitations that match their email address. """ for invitation in DomainInvitation.objects.filter( - email=self.email, status=DomainInvitation.SENT + email=self.email, status=DomainInvitation.INVITED ): invitation.retrieve() invitation.save() diff --git a/src/registrar/templates/emails/submission_confirmation.subject.txt b/src/registrar/templates/emails/submission_confirmation_subject.txt similarity index 100% rename from src/registrar/templates/emails/submission_confirmation.subject.txt rename to src/registrar/templates/emails/submission_confirmation_subject.txt From ac73a54a9d58dce607dd2378a37c97314ab51ceb Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Tue, 4 Apr 2023 10:24:04 -0500 Subject: [PATCH 17/22] Review feedback: logging, name changes --- src/registrar/models/domain_application.py | 2 +- src/registrar/templates/domain_users.html | 2 +- ...itation.subject.txt => domain_invitation_subject.txt} | 0 src/registrar/views/domain.py | 9 ++++++++- 4 files changed, 10 insertions(+), 3 deletions(-) rename src/registrar/templates/emails/{domain_invitation.subject.txt => domain_invitation_subject.txt} (100%) diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 198fffa48..526e39798 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -476,7 +476,7 @@ class DomainApplication(TimeStampedModel): try: send_templated_email( "emails/submission_confirmation.txt", - "emails/submission_confirmation.subject.txt", + "emails/submission_confirmation_subject.txt", self.submitter.email, context={"id": self.id, "domain_name": self.requested_domain.name}, ) diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index 8f114e48d..f842cff6c 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -38,7 +38,7 @@ Add another user - {% if domain.invitations.all %} + {% if domain.invitations.exists %}

Invitations

diff --git a/src/registrar/templates/emails/domain_invitation.subject.txt b/src/registrar/templates/emails/domain_invitation_subject.txt similarity index 100% rename from src/registrar/templates/emails/domain_invitation.subject.txt rename to src/registrar/templates/emails/domain_invitation_subject.txt diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index dcf864cbc..870b6a676 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1,5 +1,7 @@ """View for a single Domain.""" +import logging + from django.contrib import messages from django.contrib.messages.views import SuccessMessageMixin from django.db import IntegrityError @@ -15,6 +17,9 @@ from ..utility.email import send_templated_email, EmailSendingError from .utility import DomainPermission +logger = logging.getLogger(__name__) + + class DomainView(DomainPermission, DetailView): """Domain detail overview page.""" @@ -79,7 +84,7 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): try: send_templated_email( "emails/domain_invitation.txt", - "emails/domain_invitation.subject.txt", + "emails/domain_invitation_subject.txt", to_address=email_address, context={ "domain_url": self._domain_abs_url(), @@ -88,6 +93,8 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): ) except EmailSendingError: messages.warning(self.request, "Could not send email invitation.") + logger.warn("Could not sent email invitation to %s for domain %s", + email_address, self.object, exc_info=True) else: messages.success( self.request, f"Invited {email_address} to this domain." From ce9c8abd60987aba429fda00d44f690b7e6072ca Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Tue, 4 Apr 2023 10:29:29 -0500 Subject: [PATCH 18/22] Fix linting errors --- src/registrar/views/domain.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 870b6a676..5c199066f 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -93,8 +93,12 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): ) except EmailSendingError: messages.warning(self.request, "Could not send email invitation.") - logger.warn("Could not sent email invitation to %s for domain %s", - email_address, self.object, exc_info=True) + logger.warn( + "Could not sent email invitation to %s for domain %s", + email_address, + self.object, + exc_info=True, + ) else: messages.success( self.request, f"Invited {email_address} to this domain." From e4233870d6ed8d1b7c0ab9e1a98fc05d71ce29f8 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Tue, 4 Apr 2023 14:50:36 -0500 Subject: [PATCH 19/22] Review feedback: exception handling --- src/registrar/models/domain_invitation.py | 25 +++++++++++++++-------- src/registrar/models/user.py | 15 ++++++++++++-- src/registrar/tests/test_models.py | 11 +++++++--- 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/registrar/models/domain_invitation.py b/src/registrar/models/domain_invitation.py index 1ad150e4a..59157349f 100644 --- a/src/registrar/models/domain_invitation.py +++ b/src/registrar/models/domain_invitation.py @@ -1,5 +1,7 @@ """People are invited by email to administer domains.""" +import logging + from django.contrib.auth import get_user_model from django.db import models, IntegrityError @@ -9,6 +11,9 @@ from .utility.time_stamped_model import TimeStampedModel from .user_domain_role import UserDomainRole +logger = logging.getLogger(__name__) + + class DomainInvitation(TimeStampedModel): INVITED = "invited" RETRIEVED = "retrieved" @@ -39,7 +44,11 @@ class DomainInvitation(TimeStampedModel): @transition(field="status", source=INVITED, target=RETRIEVED) def retrieve(self): - """When an invitation is retrieved, create the corresponding permission.""" + """When an invitation is retrieved, create the corresponding permission. + + Raises: + RuntimeError if no matching user can be found. + """ # get a user with this email address User = get_user_model() @@ -54,12 +63,12 @@ class DomainInvitation(TimeStampedModel): # and create a role for that user on this domain try: - UserDomainRole.objects.create( + role = UserDomainRole.objects.get( user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN ) - except IntegrityError: - # should not happen because this user shouldn't retrieve this invitation - # more than once. - raise RuntimeError( - "Invitation would create a role that already exists for this user." - ) + except UserDomainRole.DoesNotExist: + UserDomainRole.objects.create(user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN) + else: + # something strange happened and this role already existed when + # the invitation was retrieved. Log that this occurred. + logger.warn("Invitation %s was retrieved for a role that already exists.", self) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index fb51af30c..3448d712d 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -1,3 +1,5 @@ +import logging + from django.contrib.auth.models import AbstractUser from django.db import models @@ -6,6 +8,9 @@ from .domain_invitation import DomainInvitation from phonenumber_field.modelfields import PhoneNumberField # type: ignore +logger = logging.getLogger(__name__) + + class User(AbstractUser): """ A custom user model that performs identically to the default user model @@ -43,5 +48,11 @@ class User(AbstractUser): for invitation in DomainInvitation.objects.filter( email=self.email, status=DomainInvitation.INVITED ): - invitation.retrieve() - invitation.save() + try: + invitation.retrieve() + invitation.save() + except RuntimeError: + # retrieving should not fail because of a missing user, but + # if it does fail, log the error so a new user can continue + # logging in + logger.warn("Failed to retrieve invitation %s", invitation, exc_info=True) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index b60898e50..5649172b2 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -177,6 +177,9 @@ class TestInvitations(TestCase): ) self.user, _ = User.objects.get_or_create(email=self.email) + # clean out the roles each time + UserDomainRole.objects.all().delete() + def test_retrieval_creates_role(self): self.invitation.retrieve() self.assertTrue(UserDomainRole.objects.get(user=self.user, domain=self.domain)) @@ -187,11 +190,13 @@ class TestInvitations(TestCase): with self.assertRaises(RuntimeError): self.invitation.retrieve() - def test_retrieve_existing_role_error(self): + def test_retrieve_existing_role_no_error(self): # make the overlapping role - UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain) - with self.assertRaises(RuntimeError): + UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.ADMIN) + # this is not an error but does produce a console warning + with less_console_noise(): self.invitation.retrieve() + self.assertEqual(self.invitation.status, DomainInvitation.RETRIEVED) def test_retrieve_on_first_login(self): """A new user's first_login callback retrieves their invitations.""" From 63979afe32fccee74a9a981538b69250b0478523 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Tue, 4 Apr 2023 14:59:04 -0500 Subject: [PATCH 20/22] Fix linting errors --- src/registrar/models/domain_invitation.py | 12 ++++++++---- src/registrar/models/user.py | 4 +++- src/registrar/tests/test_models.py | 4 +++- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/registrar/models/domain_invitation.py b/src/registrar/models/domain_invitation.py index 59157349f..55822c53a 100644 --- a/src/registrar/models/domain_invitation.py +++ b/src/registrar/models/domain_invitation.py @@ -3,7 +3,7 @@ import logging from django.contrib.auth import get_user_model -from django.db import models, IntegrityError +from django.db import models from django_fsm import FSMField, transition # type: ignore @@ -63,12 +63,16 @@ class DomainInvitation(TimeStampedModel): # and create a role for that user on this domain try: - role = UserDomainRole.objects.get( + UserDomainRole.objects.get( user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN ) except UserDomainRole.DoesNotExist: - UserDomainRole.objects.create(user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN) + UserDomainRole.objects.create( + user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN + ) else: # something strange happened and this role already existed when # the invitation was retrieved. Log that this occurred. - logger.warn("Invitation %s was retrieved for a role that already exists.", self) + logger.warn( + "Invitation %s was retrieved for a role that already exists.", self + ) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 3448d712d..4cd8b6c90 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -55,4 +55,6 @@ class User(AbstractUser): # retrieving should not fail because of a missing user, but # if it does fail, log the error so a new user can continue # logging in - logger.warn("Failed to retrieve invitation %s", invitation, exc_info=True) + logger.warn( + "Failed to retrieve invitation %s", invitation, exc_info=True + ) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 5649172b2..42b8803c3 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -192,7 +192,9 @@ class TestInvitations(TestCase): def test_retrieve_existing_role_no_error(self): # make the overlapping role - UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.ADMIN) + UserDomainRole.objects.get_or_create( + user=self.user, domain=self.domain, role=UserDomainRole.Roles.ADMIN + ) # this is not an error but does produce a console warning with less_console_noise(): self.invitation.retrieve() From 57f34280255b84b0dad2b3971b6d6c98a722e04e Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Tue, 4 Apr 2023 15:34:51 -0500 Subject: [PATCH 21/22] Go back to using get_or_create --- src/registrar/models/domain_invitation.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/registrar/models/domain_invitation.py b/src/registrar/models/domain_invitation.py index 55822c53a..7cc2a5432 100644 --- a/src/registrar/models/domain_invitation.py +++ b/src/registrar/models/domain_invitation.py @@ -62,15 +62,10 @@ class DomainInvitation(TimeStampedModel): ) # and create a role for that user on this domain - try: - UserDomainRole.objects.get( - user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN - ) - except UserDomainRole.DoesNotExist: - UserDomainRole.objects.create( - user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN - ) - else: + _, created = UserDomainRole.objects.get_or_create( + user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN + ) + if not created: # something strange happened and this role already existed when # the invitation was retrieved. Log that this occurred. logger.warn( From 64d0312645d02e46e1e2b1fcada3a624f89afd5a Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Tue, 4 Apr 2023 15:45:03 -0500 Subject: [PATCH 22/22] Fix OWASP false positive --- src/zap.conf | 1 + 1 file changed, 1 insertion(+) diff --git a/src/zap.conf b/src/zap.conf index ba0ef6a89..ad01388a7 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -51,6 +51,7 @@ 10038 OUTOFSCOPE http://app:8080/(robots.txt|sitemap.xml|TODO|edit/) 10038 OUTOFSCOPE http://app:8080/users 10038 OUTOFSCOPE http://app:8080/users/add +10038 OUTOFSCOPE http://app:8080/delete # This URL always returns 404, so include it as well. 10038 OUTOFSCOPE http://app:8080/todo # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers
Domain invitations