From 645a51bd8acde2186b73e20254b54632b049e5f2 Mon Sep 17 00:00:00 2001 From: Tore Brede <Tore.Brede@uib.no> Date: Fri, 25 Feb 2022 14:57:37 +0100 Subject: [PATCH] GREG-208: Adding constraint to prevent identity duplicates --- frontend/public/locales/en/common.json | 8 +- frontend/public/locales/nb/common.json | 9 +- frontend/public/locales/nn/common.json | 9 +- .../routes/sponsor/guest/guestInfo/index.tsx | 83 +++++++++++++++++-- frontend/src/routes/sponsor/guest/index.tsx | 9 +- .../sponsor/register/stepRegistration.tsx | 11 ++- ...021_identity_identity_type_value_unique.py | 17 ++++ greg/models.py | 6 ++ greg/tests/conftest.py | 2 +- greg/tests/models/test_person.py | 1 + greg/utils.py | 6 ++ .../api/serializers/IdentityDuplicateError.py | 21 +++++ gregui/api/serializers/guest.py | 45 ++++++++++ gregui/api/serializers/identity.py | 3 + gregui/api/serializers/invitation.py | 12 +++ gregui/api/views/person.py | 36 ++++++++ gregui/tests/api/views/test_invitation.py | 40 +++++++++ 17 files changed, 303 insertions(+), 15 deletions(-) create mode 100644 greg/migrations/0021_identity_identity_type_value_unique.py create mode 100644 gregui/api/serializers/IdentityDuplicateError.py diff --git a/frontend/public/locales/en/common.json b/frontend/public/locales/en/common.json index 9a60da89..11c355fa 100644 --- a/frontend/public/locales/en/common.json +++ b/frontend/public/locales/en/common.json @@ -174,7 +174,6 @@ "uib": "Are you sure you want to verify this identity?", "default": "Are you sure you want to verify this identity?" }, - "confirmIdentityTitle": "Confirm?", "cancelInvitation": "Cancel invitation?", "cancelInvitationDescription": "Do you want to cancel the invitation?" @@ -191,12 +190,17 @@ "invitationDataFetchFailed": "Failed to fetch invitation data", "guestRegistrationFailed": "Failed to register your data", "partialSubmitSuccess": "Invite creation partial successful", + "emailUpdateFailed": "E-mail update failed", "codes": { "invalid_invite": "Invalid invite", "invite_expired": "Invite has expired", "cannot_update_fields": "Failed to update data", "update_national_id_not_allowed": "Not allowed to update verified national ID", - "invite_email_failed": "There was a problem sending the invite e-mail. Contact support." + "invite_email_failed": "There was a problem sending the invite e-mail. Contact support.", + "duplicate_private_email": "There is already a guest with the registered e-mail", + "duplicate_private_mobile_number": "There is already a guest with the registered mobile phone number", + "duplicate_private_national_id_number": "There is already a guest with the national ID number given", + "duplicate_private_passport_number": "There is already a guest with the passport number given" } } } diff --git a/frontend/public/locales/nb/common.json b/frontend/public/locales/nb/common.json index 75ffadd0..c4d027a0 100644 --- a/frontend/public/locales/nb/common.json +++ b/frontend/public/locales/nb/common.json @@ -189,13 +189,18 @@ "errorLoadOusRoleType": "Kunne ikke laste organisasjons og/eller rolletype data fra server", "invitationDataFetchFailed": "Klarte ikke å hente invitasjonsdata", "guestRegistrationFailed": "Klarte ikke å registrere dataene dine", - "partialSubmitSuccess": "", + "partialSubmitSuccess": "Opprettelse av invitasjon delvis vellykket", + "emailUpdateFailed": "Oppdatering av e-post feilet", "codes": { "invalid_invite": "Ugyldig invitasjon", "invite_expired": "Invitasjonen har utløpt", "cannot_update_fields": "Klarte ikke å oppdatere dataene", "update_national_id_not_allowed": "Ikke tillatt å endre verifisert fødselsnummer/D-nummer", - "invite_email_failed": "Klarte ikke å sende e-post med invitasjon. Kontakt support." + "invite_email_failed": "Klarte ikke å sende e-post med invitasjon. Kontakt support.", + "duplicate_private_email": "Det eksisterer allerede en gjest med gitt e-postadresse", + "duplicate_private_mobile_number": "Det eksisterer allerede en gjest med gitt mobiltelefonnummer", + "duplicate_private_national_id_number": "Det eksisterer allerede en gjest med gitt fødselsnummer/D-nummer", + "duplicate_private_passport_number": "Det eksisterer allerede en gjest med gitt passnummer" } } } diff --git a/frontend/public/locales/nn/common.json b/frontend/public/locales/nn/common.json index 4eefad7f..df8899e3 100644 --- a/frontend/public/locales/nn/common.json +++ b/frontend/public/locales/nn/common.json @@ -174,7 +174,6 @@ "uib": "Er du sikker på at du vil bekrefte denne identiteten?", "default": "Er du sikker på at du vil bekrefte denne identiteten?" }, - "confirmIdentityTitle": "Bekrefte?", "cancelInvitation": "Kanseller invitasjon?", "cancelInvitationDescription": "Vil du kansellere invitasjonen?" @@ -190,12 +189,18 @@ "errorLoadOusRoleType": "Kunne ikkje laste organisasjons og/eller rolletype data frå server", "invitationDataFetchFailed": "Klarte ikkje å hente invitasjonsdata", "guestRegistrationFailed": "Klarte ikkje å registrere informasjonen din", + "partialSubmitSuccess": "Oppretting av invitasjon delvis vellukka", + "emailUpdateFailed": "Oppdatering av e-post feila", "codes": { "invalid_invite": "Ugyldig invitasjon", "invite_expired": "Invitasjonen har gått ut", "cannot_update_fields": "Klarte ikkje å oppdatere informasjonen din", "update_national_id_not_allowed": "Ikkje tillate å endre verifisert fødselsnummer/D-nummer", - "invite_email_failed": "Klarte ikkje å sende e-post med invitasjon. Kontakt support." + "invite_email_failed": "Klarte ikkje å sende e-post med invitasjon. Kontakt support.", + "duplicate_private_email": "Det eksisterer allereie ein gjest med gjeve e-postadresse", + "duplicate_private_mobile_number": "Det eksisterer allereie ein gjest med gjeve mobiltelefonnummer", + "duplicate_private_national_id_number": "Det eksisterer allereie ein gjest med gjeve fødselsnummer/D-nummer", + "duplicate_private_passport_number": "Det eksisterer allereie ein gjest med gjeve passnummer" } } } diff --git a/frontend/src/routes/sponsor/guest/guestInfo/index.tsx b/frontend/src/routes/sponsor/guest/guestInfo/index.tsx index 6001f018..fc16fb5f 100644 --- a/frontend/src/routes/sponsor/guest/guestInfo/index.tsx +++ b/frontend/src/routes/sponsor/guest/guestInfo/index.tsx @@ -24,10 +24,13 @@ import { } from 'components/table' import { Guest } from 'interfaces' import SponsorInfoButtons from 'routes/components/sponsorInfoButtons' -import { useEffect, useState } from 'react' +import React, { useEffect, useState } from 'react' import { SubmitHandler, useForm } from 'react-hook-form' import IdentityLine from 'components/identityLine' import { isValidEmail, submitJsonOpts } from 'utils' +import ServerErrorReport, { + ServerErrorReportData, +} from '../../../../components/errorReport' type GuestInfoParams = { pid: string @@ -35,7 +38,7 @@ type GuestInfoParams = { type GuestInfoProps = { guest: Guest - updateEmail: (validEmail: string) => void + updateEmail: (validEmail: string, errorCallback: (error: any) => void) => void resend: () => void reloadGuests: () => void reloadGuest: () => void @@ -94,6 +97,8 @@ export default function GuestInfo({ const [t] = useTranslation(['common']) const history = useHistory() const [confirmationDialogOpen, setConfirmationDialogOpen] = useState(false) + const [emailUpdateError, setEmailUpdateError] = + useState<ServerErrorReportData>() // Using useForm even though only the e-mail is allow to change at present, since useForm makes setup and validation easier const { @@ -106,8 +111,67 @@ export default function GuestInfo({ mode: 'onChange', }) + // This function inspects the errorText and tries to + // create an error report based on it by looking for + // the code field, if that does not work a generic + // error report is shown with the raw error text + const createErrorMessageBasedOnText = ( + errorText: string, + statusCode: number | undefined = undefined, + statusText: string | undefined = undefined + ) => { + try { + // Try to extract code from error text + const errorAsJson = JSON.parse(errorText) + if ('code' in errorAsJson) { + setEmailUpdateError({ + errorHeading: t('error.emailUpdateFailed'), + statusCode, + statusText, + errorBodyText: t(`error.codes.${errorAsJson.code}`), + }) + } + } catch (e) { + // Use the text from the error directly if no code could be extracted + setEmailUpdateError({ + errorHeading: t('error.emailUpdateFailed'), + statusCode: undefined, + statusText: undefined, + errorBodyText: errorText, + }) + } + } + const submit: SubmitHandler<Email> = (data) => { - updateEmail(data.email) + setEmailUpdateError(undefined) + updateEmail(data.email, (errorResponse: any) => { + if (typeof errorResponse === 'string') { + createErrorMessageBasedOnText(errorResponse) + } else if (errorResponse instanceof Response) { + // Some not successful response code + errorResponse + .text() + .then((text: string) => { + createErrorMessageBasedOnText( + text, + errorResponse.status, + errorResponse.statusText + ) + }) + .catch((error: any) => { + // Not expected + console.error(error) + }) + } else { + // Not a string in the error response, not expected + setEmailUpdateError({ + errorHeading: t('error.emailUpdateFailed'), + statusCode: undefined, + statusText: undefined, + errorBodyText: errorResponse, + }) + } + }) } const onSubmit = handleSubmit(submit) @@ -173,7 +237,6 @@ export default function GuestInfo({ <TableRow> <TableHeadCell align="left" colSpan={2}> {t('guestInfo.contactInfoTableText')} - </TableHeadCell> </TableRow> </TableHead> @@ -204,7 +267,7 @@ export default function GuestInfo({ defaultValue={guest.email} inputRef={ref} onChange={onChange} - {...inputProps} + {...inputProps} /> {/* If the guest has not completed the registration process, he should have an invitation he has not responded to */} @@ -282,6 +345,16 @@ export default function GuestInfo({ > {t('button.save')} </Button> + {emailUpdateError !== undefined && ( + <Box sx={{ marginTop: '1rem' }}> + <ServerErrorReport + errorHeading={emailUpdateError?.errorHeading} + statusCode={emailUpdateError?.statusCode} + statusText={emailUpdateError?.statusText} + errorBodyText={emailUpdateError?.errorBodyText} + /> + </Box> + )} </Box> </form> <Typography variant="h2">{t('guestInfo.roleInfoHead')}</Typography> diff --git a/frontend/src/routes/sponsor/guest/index.tsx b/frontend/src/routes/sponsor/guest/index.tsx index 37e46d6f..8d36ad2e 100644 --- a/frontend/src/routes/sponsor/guest/index.tsx +++ b/frontend/src/routes/sponsor/guest/index.tsx @@ -17,7 +17,10 @@ function GuestRoutes({ reloadGuests }: GuestRoutesProps) { const { pid } = useParams<GuestInfoParams>() const { guestInfo, reloadGuestInfo } = useGuest(pid) - const updateEmail = (validEmail: string) => { + const updateEmail = ( + validEmail: string, + errorCallback: (error: any) => void + ) => { fetch( `/api/ui/v1/person/${pid}`, submitJsonOpts('PATCH', { email: validEmail }) @@ -27,10 +30,12 @@ function GuestRoutes({ reloadGuests }: GuestRoutesProps) { // Just reload data from the server, it will cause the children // to be rerendered reloadGuestInfo() + } else { + errorCallback(res) } }) .catch((error) => { - console.log('error', error) + errorCallback(error) }) } diff --git a/frontend/src/routes/sponsor/register/stepRegistration.tsx b/frontend/src/routes/sponsor/register/stepRegistration.tsx index d84adc79..479cbb26 100644 --- a/frontend/src/routes/sponsor/register/stepRegistration.tsx +++ b/frontend/src/routes/sponsor/register/stepRegistration.tsx @@ -79,11 +79,20 @@ export default function StepRegistration() { .text() .then((text) => { setSubmitState(SubmitState.SubmitFailure) + + // Some error responses have a code that is used to look up a translated text. + // Attempt to parse the message text and extract the code + const jsonResponse = JSON.parse(text) + const errorText = + jsonResponse.code === undefined + ? text + : t(`error.codes.${jsonResponse.code}`) + setSubmitErrorReport({ errorHeading: t('error.invitationCreationFailedHeader'), statusCode: res.status, statusText: res.statusText, - errorBodyText: text, + errorBodyText: errorText, }) }) .catch((error) => { diff --git a/greg/migrations/0021_identity_identity_type_value_unique.py b/greg/migrations/0021_identity_identity_type_value_unique.py new file mode 100644 index 00000000..46e712ff --- /dev/null +++ b/greg/migrations/0021_identity_identity_type_value_unique.py @@ -0,0 +1,17 @@ +# Generated by Django 4.0 on 2022-02-24 12:08 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('greg', '0020_person_gender'), + ] + + operations = [ + migrations.AddConstraint( + model_name='identity', + constraint=models.UniqueConstraint(fields=('type', 'value'), name='identity_type_value_unique'), + ), + ] diff --git a/greg/models.py b/greg/models.py index a13668b9..43fe9f07 100644 --- a/greg/models.py +++ b/greg/models.py @@ -315,6 +315,12 @@ class Identity(BaseModel): class Meta: verbose_name_plural = "identities" + constraints = ( + models.UniqueConstraint( + fields=["type", "value"], + name="identity_type_value_unique", + ), + ) class ConsentType(BaseModel): diff --git a/greg/tests/conftest.py b/greg/tests/conftest.py index fd6e8b09..d5ab2cfd 100644 --- a/greg/tests/conftest.py +++ b/greg/tests/conftest.py @@ -108,7 +108,7 @@ def person_bar() -> Person: Identity.objects.create( person=person, type=Identity.IdentityType.PRIVATE_MOBILE_NUMBER, - value="+4712345678", + value="+4712345679", ) return Person.objects.get(id=person.id) diff --git a/greg/tests/models/test_person.py b/greg/tests/models/test_person.py index f9fc4958..c6e4d1a2 100644 --- a/greg/tests/models/test_person.py +++ b/greg/tests/models/test_person.py @@ -60,6 +60,7 @@ def verified_national_id_number() -> Identity: return Identity( type=Identity.IdentityType.NORWEGIAN_NATIONAL_ID_NUMBER, verified_at=_a_year_ago, + value="12118913939", ) diff --git a/greg/utils.py b/greg/utils.py index c6480303..e6fa0403 100644 --- a/greg/utils.py +++ b/greg/utils.py @@ -3,6 +3,8 @@ import typing from datetime import date, datetime, timedelta from django.utils import timezone +from greg.models import Identity + def camel_to_snake(s: str) -> str: """Turns `FooBar` into `foo_bar`.""" @@ -113,3 +115,7 @@ def date_to_datetime_midnight(in_date: typing.Union[date, str]) -> datetime: def get_default_invitation_expire_date_from_now(): return timezone.now() + timedelta(days=30) + + +def is_identity_duplicate(identity_type: Identity.IdentityType, value: str) -> bool: + return Identity.objects.filter(type=identity_type).filter(value=value).exists() diff --git a/gregui/api/serializers/IdentityDuplicateError.py b/gregui/api/serializers/IdentityDuplicateError.py new file mode 100644 index 00000000..7b32db68 --- /dev/null +++ b/gregui/api/serializers/IdentityDuplicateError.py @@ -0,0 +1,21 @@ +from rest_framework.exceptions import APIException +from rest_framework import status + +from greg.models import Identity + + +class IdentityDuplicateError(APIException): + """ + Used to signal the there is already an identity registered with the same value + for the type given as input parameter. + """ + + status_code = status.HTTP_400_BAD_REQUEST + default_detail = "Validation error" + _type: Identity.IdentityType + + def __init__(self, identity_type: Identity.IdentityType, code: str): + super().__init__() + self._type = identity_type + self.code = code + self.detail = {"code": code, "message": "Duplicate identity"} diff --git a/gregui/api/serializers/guest.py b/gregui/api/serializers/guest.py index cf4797dc..0cfe4b4c 100644 --- a/gregui/api/serializers/guest.py +++ b/gregui/api/serializers/guest.py @@ -6,6 +6,8 @@ from rest_framework import serializers from rest_framework.exceptions import ValidationError from greg.models import Consent, ConsentChoice, ConsentType, Identity, Person +from greg.utils import is_identity_duplicate +from gregui.api.serializers.IdentityDuplicateError import IdentityDuplicateError from gregui.validation import ( validate_phone_number, validate_norwegian_national_id_number, @@ -132,6 +134,49 @@ class GuestRegisterSerializer(serializers.ModelSerializer): return gender + def validate_private_email(self, private_email: str): + if is_identity_duplicate(Identity.IdentityType.PRIVATE_EMAIL, private_email): + raise IdentityDuplicateError( + Identity.IdentityType.PRIVATE_EMAIL, "duplicate_private_email" + ) + + # Return the validated data + return private_email + + def validate_private_mobile(self, private_mobile: str): + if is_identity_duplicate( + Identity.IdentityType.PRIVATE_MOBILE_NUMBER, private_mobile + ): + raise IdentityDuplicateError( + Identity.IdentityType.PRIVATE_MOBILE_NUMBER, + "duplicate_private_mobile_number", + ) + + # Return the validated data + return private_mobile + + def validate_fnr(self, fnr: str): + if is_identity_duplicate( + Identity.IdentityType.NORWEGIAN_NATIONAL_ID_NUMBER, fnr + ): + raise IdentityDuplicateError( + Identity.IdentityType.NORWEGIAN_NATIONAL_ID_NUMBER, + "duplicate_private_national_id_number", + ) + + # Return the validated data + return fnr + + def validate_passport(self, passport: str): + if is_identity_duplicate(Identity.IdentityType.PASSPORT_NUMBER, passport): + raise IdentityDuplicateError( + Identity.IdentityType.PASSPORT_NUMBER, + "duplicate_private_passport_number", + ) + + # Return the validated data + return passport + class Meta: model = Person fields = ( diff --git a/gregui/api/serializers/identity.py b/gregui/api/serializers/identity.py index 6395cf12..4d792987 100644 --- a/gregui/api/serializers/identity.py +++ b/gregui/api/serializers/identity.py @@ -48,6 +48,9 @@ class IdentitySerializer(serializers.ModelSerializer): Note: Get requests do not use this method, making it safe. """ + + # TODO Check for duplicate value + # Prevent nin verification. (This will only trigger if someone is posting the # requests themselves. The frontend has its own setting disabling the button # used against this endpoint.) diff --git a/gregui/api/serializers/invitation.py b/gregui/api/serializers/invitation.py index 2c95a82f..601694dd 100644 --- a/gregui/api/serializers/invitation.py +++ b/gregui/api/serializers/invitation.py @@ -6,6 +6,8 @@ from django.utils import timezone from rest_framework import serializers from greg.models import Invitation, InvitationLink, Person, Role, Identity +from greg.utils import is_identity_duplicate +from gregui.api.serializers.IdentityDuplicateError import IdentityDuplicateError from gregui.api.serializers.role import InviteRoleSerializerUi from gregui.models import GregUserProfile @@ -40,6 +42,16 @@ class InviteGuestSerializer(serializers.ModelSerializer): ) return person + def validate_email(self, email): + # The e-mail in the invite is the private e-mail + if is_identity_duplicate(Identity.IdentityType.PRIVATE_EMAIL, email): + raise IdentityDuplicateError( + Identity.IdentityType.PRIVATE_EMAIL, "duplicate_private_email" + ) + + # Return the validated data + return email + class Meta: model = Person fields = ( diff --git a/gregui/api/views/person.py b/gregui/api/views/person.py index 622d9ff7..06ea9d3b 100644 --- a/gregui/api/views/person.py +++ b/gregui/api/views/person.py @@ -9,7 +9,9 @@ from rest_framework.viewsets import GenericViewSet from greg.api.serializers.person import SpecialPersonSerializer from greg.models import Identity, Person from greg.permissions import IsSponsor +from greg.utils import is_identity_duplicate from gregui import validation +from gregui.api.serializers.IdentityDuplicateError import IdentityDuplicateError from gregui.api.serializers.guest import create_identity_or_update from gregui.models import GregUserProfile @@ -30,11 +32,45 @@ class PersonViewSet(mixins.RetrieveModelMixin, mixins.UpdateModelMixin, GenericV http_methods = ["get", "patch"] serializer_class = SpecialPersonSerializer + def update(self, request, *args, **kwargs): + """ + Overriding the update-method to be able to return an error response + with a special format if there is a duplicate identity + """ + partial = kwargs.pop("partial", False) + instance = self.get_object() + serializer = self.get_serializer(instance, data=request.data, partial=partial) + serializer.is_valid(raise_exception=True) + + try: + self.perform_update(serializer) + except IdentityDuplicateError as error: + return Response( + status=status.HTTP_400_BAD_REQUEST, + data={ + "code": error.code, + "message": error.detail["message"], + }, + ) + + if getattr(instance, "_prefetched_objects_cache", None): + # If 'prefetch_related' has been applied to a queryset, we need to + # forcibly invalidate the prefetch cache on the instance. + instance._prefetched_objects_cache = {} + + return Response(serializer.data) + def perform_update(self, serializer): """Update email when doing patch""" email = self.request.data.get("email") person = self.get_object() validation.validate_email(email) + + if is_identity_duplicate(Identity.IdentityType.PRIVATE_EMAIL, email): + raise IdentityDuplicateError( + Identity.IdentityType.PRIVATE_EMAIL, "duplicate_private_email" + ) + create_identity_or_update(Identity.IdentityType.PRIVATE_EMAIL, email, person) return super().perform_update(serializer) diff --git a/gregui/tests/api/views/test_invitation.py b/gregui/tests/api/views/test_invitation.py index b1efd36b..81fd90d6 100644 --- a/gregui/tests/api/views/test_invitation.py +++ b/gregui/tests/api/views/test_invitation.py @@ -764,3 +764,43 @@ def test_session_type_id_porten( assert person.first_name == "Updated" assert person.last_name == "Updated2" + + +@pytest.mark.django_db +def test_duplicate_national_id( + client, invited_person, confirmation_template, person_foo +): + person, invitation_link = invited_person + fnr = "11120618212" + + Identity.objects.create( + person=person_foo, + type=Identity.IdentityType.NORWEGIAN_NATIONAL_ID_NUMBER, + value=fnr, + ) + + # Try to respond to an invite with a national ID number that already exists + data = { + "person": { + "private_mobile": "+4797543992", + "private_email": "test@example.com", + "fnr": fnr, + } + } + url = reverse("gregui-v1:invited-info") + + assert person.fnr is None + + session = client.session + session["invite_id"] = str(invitation_link.uuid) + session.save() + + response = client.post(url, data, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST, response.data + response_data = response.json() + assert response_data["code"] == "duplicate_private_national_id_number" + + person.refresh_from_db() + # The national ID number should still be not set since the post request failed + assert person.fnr is None -- GitLab