From c8647abe47b7e4b94e3a1c18b69db9579c5755aa Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Thu, 22 Feb 2024 12:13:28 +0100 Subject: [PATCH 01/10] [#IOPID-1604] new REQUIRED env variable --- env.example | 3 ++- utils/config.ts | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/env.example b/env.example index 73a1c4c..f73aeb7 100644 --- a/env.example +++ b/env.example @@ -4,6 +4,7 @@ WEBSITE_NODE_DEFAULT_VERSION=10.14.1 AzureWebJobsStorage=${STORAGE_CONNECTION_STRING} StorageConnection=${STORAGE_CONNECTION_STRING} VALIDATION_CALLBACK_URL=localhost +CONFIRM_CHOICE_PAGE_URL=localhost SLOT_TASK_HUBNAME=FNPUBLICTESTMOCK FF_UNIQUE_EMAIL_ENFORCEMENT=NONE @@ -12,4 +13,4 @@ UNIQUE_EMAIL_ENFORCEMENT_USERS=[] PROFILE_EMAIL_STORAGE_CONNECTION_STRING=${STORAGE_CONNECTION_STRING} PROFILE_EMAIL_STORAGE_TABLE_NAME=profileEmails -APPINSIGHTS_INSTRUMENTATIONKEY=InstrumentationKeyHere \ No newline at end of file +APPINSIGHTS_INSTRUMENTATIONKEY=InstrumentationKeyHere diff --git a/utils/config.ts b/utils/config.ts index e67b42e..c9f43e3 100644 --- a/utils/config.ts +++ b/utils/config.ts @@ -17,6 +17,7 @@ import { import { pipe } from "fp-ts/lib/function"; import * as t from "io-ts"; import * as E from "fp-ts/lib/Either"; +import { UrlFromString } from "@pagopa/ts-commons/lib/url"; export const BetaUsers = t.readonlyArray(FiscalCode); export type BetaUsers = t.TypeOf; @@ -47,6 +48,7 @@ export const IConfig = t.type({ UNIQUE_EMAIL_ENFORCEMENT_USERS: BetaUsersFromString, VALIDATION_CALLBACK_URL: NonEmptyString, + CONFIRM_CHOICE_PAGE_URL: UrlFromString, isProduction: t.boolean }); From c0194a78765d3633540423b1605e96853a8006ca Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Thu, 22 Feb 2024 12:14:12 +0100 Subject: [PATCH 02/10] [#IOPID-1604] new optional query param on openapi spec --- openapi/external.yaml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/openapi/external.yaml b/openapi/external.yaml index d93dd20..0b60f53 100644 --- a/openapi/external.yaml +++ b/openapi/external.yaml @@ -17,8 +17,14 @@ paths: name: token required: true schema: - type: integer + type: string minLength: 1 + - in: query + name: confirm + schema: + type: string + enum: + - true responses: "303": description: See Others From 0cc6e25069f442a3c4d492d83855789c8b544980 Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Thu, 22 Feb 2024 12:14:23 +0100 Subject: [PATCH 03/10] [#IOPID-1604] enriched tsconfig --- tsconfig.json | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tsconfig.json b/tsconfig.json index eadb49e..72cbb70 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -9,6 +9,8 @@ "resolveJsonModule": true }, "exclude": [ - "__mocks__" + "**/__mocks__", + "**/__tests__", + "node_modules" ] -} \ No newline at end of file +} From 7faa7b352afec6de8ebf104cfcd335f2d1193257 Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Thu, 22 Feb 2024 12:14:48 +0100 Subject: [PATCH 04/10] [#IOPID-1604] redirect to confirm page based on new optional parameter --- .../__tests__/handler.test.ts | 121 +++++++++++++---- ValidateProfileEmail/handler.ts | 124 +++++++++++++----- ValidateProfileEmail/index.ts | 3 +- 3 files changed, 185 insertions(+), 63 deletions(-) diff --git a/ValidateProfileEmail/__tests__/handler.test.ts b/ValidateProfileEmail/__tests__/handler.test.ts index 47633ab..bbbd9bd 100644 --- a/ValidateProfileEmail/__tests__/handler.test.ts +++ b/ValidateProfileEmail/__tests__/handler.test.ts @@ -12,6 +12,7 @@ import * as TE from "fp-ts/TaskEither"; import * as O from "fp-ts/Option"; import { ProfileModel } from "@pagopa/io-functions-commons/dist/src/models/profile"; import { aFiscalCode, aRetrievedProfile, anEmail } from "../__mocks__/profile"; +import { ValidUrl } from "@pagopa/ts-commons/lib/url"; const VALIDATION_TOKEN = "01DPT9QAZ6N0FJX21A86FRCWB3:8c652f8566ba53bd8cf0b1b9" as TokenQueryParam; @@ -36,21 +37,39 @@ const contextMock = { }; const validationCallbackUrl = { - href: "" + href: "localhost/validation" }; + +const confirmChoiceUrl = { + href: "localhost/confirm-choice" +} as ValidUrl; + const timestampGeneratorMock = () => 1234567890; const errorUrl = ( error: keyof typeof ValidationErrors, timestampGenerator: () => number ) => { - return `?result=failure&error=${error}&time=${timestampGenerator()}`; + return `${ + validationCallbackUrl.href + }?result=failure&error=${error}&time=${timestampGenerator()}`; }; const successUrl = (timestampGenerator: () => number) => { - return `?result=success&time=${timestampGenerator()}`; + return `${ + validationCallbackUrl.href + }?result=success&time=${timestampGenerator()}`; }; +const confirmPageUrl = ( + token: string, + email: EmailString, + timestampGenerator: () => number +) => + `${ + confirmChoiceUrl.href + }?token=${token}&email=${email}&time=${timestampGenerator()}`; + const mockRetrieveEntity = jest .fn() .mockImplementation((_, __, ___, ____, f) => { @@ -95,14 +114,21 @@ const expiredTokenEntity = { }; describe("ValidateProfileEmailHandler", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + it.each` - scenario | expectedError | callbackInputs - ${"GENERIC_ERROR in case the query versus the table storage fails"} | ${"GENERIC_ERROR"} | ${[new Error()]} - ${"INVALID_TOKEN error in case the token if not found in the table"} | ${"INVALID_TOKEN"} | ${[{ code: ResourceNotFoundCode }]} - ${"TOKEN_EXPIRED error in case the token is expired"} | ${"TOKEN_EXPIRED"} | ${[undefined, expiredTokenEntity]} + scenario | expectedError | callbackInputs | isConfirmStep + ${"GENERIC_ERROR in case the query versus the table storage fails during confirm step"} | ${"GENERIC_ERROR"} | ${[new Error()]} | ${true} + ${"GENERIC_ERROR in case the query versus the table storage fails"} | ${"GENERIC_ERROR"} | ${[new Error()]} | ${false} + ${"INVALID_TOKEN error in case the token if not found in the table during confirm step"} | ${"INVALID_TOKEN"} | ${[{ code: ResourceNotFoundCode }]} | ${true} + ${"INVALID_TOKEN error in case the token if not found in the table"} | ${"INVALID_TOKEN"} | ${[{ code: ResourceNotFoundCode }]} | ${false} + ${"TOKEN_EXPIRED error in case the token is expired during confirm step"} | ${"TOKEN_EXPIRED"} | ${[undefined, expiredTokenEntity]} | ${true} + ${"TOKEN_EXPIRED error in case the token is expired"} | ${"TOKEN_EXPIRED"} | ${[undefined, expiredTokenEntity]} | ${false} `( "should return a redirect with a $scenario", - async ({ callbackInputs, expectedError }) => { + async ({ callbackInputs, expectedError, isConfirmStep }) => { mockRetrieveEntity.mockImplementationOnce((_, __, ___, ____, f) => { f(...callbackInputs); }); @@ -114,12 +140,14 @@ describe("ValidateProfileEmailHandler", () => { validationCallbackUrl as any, timestampGeneratorMock, profileEmailReader, - constTrue + constTrue, + confirmChoiceUrl ); const response = await verifyProfileEmailHandler( contextMock as any, - VALIDATION_TOKEN + VALIDATION_TOKEN, + isConfirmStep ? O.some(true) : O.none ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); @@ -132,10 +160,43 @@ describe("ValidateProfileEmailHandler", () => { ); it.each` - scenario | expectedError | isThrowing - ${"should return IResponseSeeOtherRedirect if the e-mail is already taken (unique email enforcement = %uee) WHEN a citizen changes e-mail"} | ${"EMAIL_ALREADY_TAKEN"} | ${undefined} - ${"return 500 WHEN the unique e-mail enforcement check fails"} | ${"GENERIC_ERROR"} | ${true} - `("should $scenario", async ({ expectedError, isThrowing }) => { + scenario | expectedError | isThrowing | isConfirmStep + ${"should return IResponseSeeOtherRedirect if the e-mail is already taken (unique email enforcement = %uee) WHEN a citizen changes e-mail during confirm step"} | ${"EMAIL_ALREADY_TAKEN"} | ${undefined} | ${true} + ${"should return IResponseSeeOtherRedirect if the e-mail is already taken (unique email enforcement = %uee) WHEN a citizen changes e-mail"} | ${"EMAIL_ALREADY_TAKEN"} | ${undefined} | ${false} + ${"return 500 WHEN the unique e-mail enforcement check fails during confirm step"} | ${"GENERIC_ERROR"} | ${true} | ${true} + ${"return 500 WHEN the unique e-mail enforcement check fails"} | ${"GENERIC_ERROR"} | ${true} | ${false} + `( + "should $scenario", + async ({ expectedError, isThrowing, isConfirmStep }) => { + const verifyProfileEmailHandler = ValidateProfileEmailHandler( + tableServiceMock as any, + "", + mockProfileModel, + validationCallbackUrl as any, + timestampGeneratorMock, + { + list: generateProfileEmails(1, isThrowing) + }, + constTrue, + confirmChoiceUrl + ); + + const response = await verifyProfileEmailHandler( + contextMock as any, + VALIDATION_TOKEN, + isConfirmStep ? O.some(true) : O.none + ); + + expect(response.kind).toBe("IResponseSeeOtherRedirect"); + expect(response.detail).toBe( + errorUrl(expectedError, timestampGeneratorMock) + ); + expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); + expect(mockUpdate).not.toBeCalled(); + } + ); + + it("should validate the email in profile if all the condition are verified and we are in the confirm step", async () => { const verifyProfileEmailHandler = ValidateProfileEmailHandler( tableServiceMock as any, "", @@ -143,25 +204,27 @@ describe("ValidateProfileEmailHandler", () => { validationCallbackUrl as any, timestampGeneratorMock, { - list: generateProfileEmails(1, isThrowing) + list: generateProfileEmails(0) }, - constTrue + constTrue, + confirmChoiceUrl ); const response = await verifyProfileEmailHandler( contextMock as any, - VALIDATION_TOKEN + VALIDATION_TOKEN, + O.some(true) ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); - expect(response.detail).toBe( - errorUrl(expectedError, timestampGeneratorMock) - ); + expect(response.detail).toBe(successUrl(timestampGeneratorMock)); expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); - expect(mockUpdate).not.toBeCalled(); + expect(mockUpdate).toBeCalledWith( + expect.objectContaining({ isEmailValidated: true }) + ); }); - it("should validate the email in profile if all the condition are verified", async () => { + it("should NOT validate the email in profile if we are NOT in the confirm step", async () => { const verifyProfileEmailHandler = ValidateProfileEmailHandler( tableServiceMock as any, "", @@ -171,19 +234,21 @@ describe("ValidateProfileEmailHandler", () => { { list: generateProfileEmails(0) }, - constTrue + constTrue, + confirmChoiceUrl ); const response = await verifyProfileEmailHandler( contextMock as any, - VALIDATION_TOKEN + VALIDATION_TOKEN, + O.none ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); - expect(response.detail).toBe(successUrl(timestampGeneratorMock)); - expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); - expect(mockUpdate).toBeCalledWith( - expect.objectContaining({ isEmailValidated: true }) + expect(response.detail).toBe( + confirmPageUrl(VALIDATION_TOKEN, anEmail, timestampGeneratorMock) ); + expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); + expect(mockUpdate).not.toHaveBeenCalled(); }); }); diff --git a/ValidateProfileEmail/handler.ts b/ValidateProfileEmail/handler.ts index 65ae7b3..87c21f6 100644 --- a/ValidateProfileEmail/handler.ts +++ b/ValidateProfileEmail/handler.ts @@ -6,6 +6,9 @@ import { isLeft } from "fp-ts/lib/Either"; import { isNone } from "fp-ts/lib/Option"; import * as t from "io-ts"; +import * as O from "fp-ts/lib/Option"; +import * as TE from "fp-ts/lib/TaskEither"; +import { pipe } from "fp-ts/function"; import { Context } from "@azure/functions"; import { TableService } from "azure-storage"; @@ -16,10 +19,15 @@ import { IResponseSeeOtherRedirect, ResponseSeeOtherRedirect } from "@pagopa/ts-commons/lib/responses"; -import { FiscalCode, PatternString } from "@pagopa/ts-commons/lib/strings"; +import { + EmailString, + FiscalCode, + PatternString +} from "@pagopa/ts-commons/lib/strings"; import { hashFiscalCode } from "@pagopa/ts-commons/lib/hash"; import { RequiredQueryParamMiddleware } from "@pagopa/io-functions-commons/dist/src/utils/middlewares/required_query_param"; +import { OptionalQueryParamMiddleware } from "@pagopa/io-functions-commons/dist/src/utils/middlewares/optional_query_param"; import { ValidationTokenEntity } from "@pagopa/io-functions-commons/dist/src/entities/validation_token"; import { ProfileModel } from "@pagopa/io-functions-commons/dist/src/models/profile"; @@ -35,6 +43,7 @@ import { isEmailAlreadyTaken } from "@pagopa/io-functions-commons/dist/src/utils/unique_email_enforcement"; import { trackEvent } from "../utils/appinsights"; +import { BooleanFromString } from "io-ts-types"; // Tokens are generated by CreateValidationTokenActivity function inside the // io-functions-app project (https://github.com/pagopa/io-functions-app) @@ -47,7 +56,8 @@ export type TokenQueryParam = t.TypeOf; type IValidateProfileEmailHandler = ( context: Context, - token: TokenQueryParam + token: TokenQueryParam, + confirm: O.Option ) => Promise; // Used in the callback @@ -58,6 +68,21 @@ export enum ValidationErrors { EMAIL_ALREADY_TAKEN = "EMAIL_ALREADY_TAKEN" } +/** + * Returns a ValidUrl that represents a successful validation + */ +const confirmChoicePageUrl = ( + url: ValidUrl, + token: TokenQueryParam, + email: EmailString, + timeStampGenerator: () => number +) => + ({ + href: `${ + url.href + }?token=${token}&email=${email}&time=${timeStampGenerator()}` + } as ValidUrl); + /** * Returns a ValidUrl that represents a successful validation */ @@ -90,6 +115,11 @@ const TokenQueryParamMiddleware = RequiredQueryParamMiddleware( TokenQueryParam ); +const ChoiceConfirmQueryParamMiddleware = OptionalQueryParamMiddleware( + "confirm", + BooleanFromString.pipe(t.literal(true)) +); + export const ValidateProfileEmailHandler = ( tableService: TableService, validationTokensTableName: string, @@ -97,10 +127,12 @@ export const ValidateProfileEmailHandler = ( validationCallbackUrl: ValidUrl, timeStampGenerator: () => number, profileEmails: IProfileEmailReader, - FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean + FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean, + confirmChoiceUrl: ValidUrl ): IValidateProfileEmailHandler => async ( context, - token + token, + confirm ): Promise => { const logPrefix = `ValidateProfileEmail|TOKEN=${token}`; const vFailureUrl = (error: keyof typeof ValidationErrors): ValidUrl => @@ -225,33 +257,54 @@ export const ValidateProfileEmailHandler = ( } } - // Update the profile and set isEmailValidated to `true` - const errorOrUpdatedProfile = await profileModel.update({ - ...existingProfile, - isEmailValidated: true - })(); - - if (isLeft(errorOrUpdatedProfile)) { - context.log.error( - `${logPrefix}|Error updating profile|ERROR=${errorOrUpdatedProfile.left}` - ); - return ResponseSeeOtherRedirect( - vFailureUrl(ValidationErrors.GENERIC_ERROR) - ); - } - - trackEvent({ - name: "io.citizen-auth.validate_email", - tagOverrides: { - "ai.user.id": hashFiscalCode(existingProfile.fiscalCode), - samplingEnabled: "false" - } - }); - - context.log.verbose(`${logPrefix}|The profile has been updated`); - return ResponseSeeOtherRedirect( - validationSuccessUrl(validationCallbackUrl, timeStampGenerator) - ); + // Update the profile and set isEmailValidated to `true` only if the confirm parameter is true + // otherwise just redirect to confirm page with token in query param + return await pipe( + confirm, + O.fold( + () => + TE.right( + ResponseSeeOtherRedirect( + confirmChoicePageUrl( + confirmChoiceUrl, + token, + email, + timeStampGenerator + ) + ) + ), + _ => + pipe( + profileModel.update({ + ...existingProfile, + isEmailValidated: true + }), + TE.mapLeft(error => { + context.log.error( + `${logPrefix}|Error updating profile|ERROR=${error}` + ); + return ResponseSeeOtherRedirect( + vFailureUrl(ValidationErrors.GENERIC_ERROR) + ); + }), + TE.map(() => { + trackEvent({ + name: "io.citizen-auth.validate_email", + tagOverrides: { + "ai.user.id": hashFiscalCode(existingProfile.fiscalCode), + samplingEnabled: "false" + } + }); + + context.log.verbose(`${logPrefix}|The profile has been updated`); + return ResponseSeeOtherRedirect( + validationSuccessUrl(validationCallbackUrl, timeStampGenerator) + ); + }) + ) + ), + TE.toUnion + )(); }; /** @@ -265,7 +318,8 @@ export const ValidateProfileEmail = ( validationCallbackUrl: ValidUrl, timeStampGenerator: () => number, profileEmails: IProfileEmailReader, - FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean + FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean, + confirmChoiceUrl: ValidUrl ): express.RequestHandler => { const handler = ValidateProfileEmailHandler( tableService, @@ -274,12 +328,14 @@ export const ValidateProfileEmail = ( validationCallbackUrl, timeStampGenerator, profileEmails, - FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED + FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED, + confirmChoiceUrl ); const middlewaresWrap = withRequestMiddlewares( ContextMiddleware(), - TokenQueryParamMiddleware + TokenQueryParamMiddleware, + ChoiceConfirmQueryParamMiddleware ); return wrapRequestHandler(middlewaresWrap(handler)); }; diff --git a/ValidateProfileEmail/index.ts b/ValidateProfileEmail/index.ts index 1b85c79..41fad66 100644 --- a/ValidateProfileEmail/index.ts +++ b/ValidateProfileEmail/index.ts @@ -68,7 +68,8 @@ app.get( validationCallbackValidUrl, Date.now, profileEmailsReader, - FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED + FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED, + config.CONFIRM_CHOICE_PAGE_URL ) ); From e9da6abf8a37481bb3a1dbe1d5a2b5e0a09bf005 Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Thu, 22 Feb 2024 12:22:13 +0100 Subject: [PATCH 05/10] fix: linter errors --- ValidateProfileEmail/handler.ts | 4 ++-- utils/config.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ValidateProfileEmail/handler.ts b/ValidateProfileEmail/handler.ts index 87c21f6..7ebb150 100644 --- a/ValidateProfileEmail/handler.ts +++ b/ValidateProfileEmail/handler.ts @@ -42,8 +42,8 @@ import { IProfileEmailReader, isEmailAlreadyTaken } from "@pagopa/io-functions-commons/dist/src/utils/unique_email_enforcement"; -import { trackEvent } from "../utils/appinsights"; import { BooleanFromString } from "io-ts-types"; +import { trackEvent } from "../utils/appinsights"; // Tokens are generated by CreateValidationTokenActivity function inside the // io-functions-app project (https://github.com/pagopa/io-functions-app) @@ -76,7 +76,7 @@ const confirmChoicePageUrl = ( token: TokenQueryParam, email: EmailString, timeStampGenerator: () => number -) => +): ValidUrl => ({ href: `${ url.href diff --git a/utils/config.ts b/utils/config.ts index c9f43e3..cd24a32 100644 --- a/utils/config.ts +++ b/utils/config.ts @@ -35,6 +35,7 @@ export const FeatureFlagFromString = withFallback( export const IConfig = t.type({ APPINSIGHTS_INSTRUMENTATIONKEY: NonEmptyString, + CONFIRM_CHOICE_PAGE_URL: UrlFromString, COSMOSDB_KEY: NonEmptyString, COSMOSDB_NAME: NonEmptyString, COSMOSDB_URI: NonEmptyString, @@ -48,7 +49,6 @@ export const IConfig = t.type({ UNIQUE_EMAIL_ENFORCEMENT_USERS: BetaUsersFromString, VALIDATION_CALLBACK_URL: NonEmptyString, - CONFIRM_CHOICE_PAGE_URL: UrlFromString, isProduction: t.boolean }); From ecefbcb0b078a918420b6162db168c82adda1098 Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Fri, 23 Feb 2024 09:52:05 +0100 Subject: [PATCH 06/10] [#IOPID-1604] refactor based on feedback --- .../__tests__/handler.test.ts | 86 ++++----- ValidateProfileEmail/handler.ts | 174 +++++------------- openapi/external.yaml | 9 +- package.json | 1 + utils/middleware.ts | 37 ++++ utils/redirect_url.ts | 44 +++++ utils/validation_errors.ts | 6 + yarn.lock | 5 + 8 files changed, 188 insertions(+), 174 deletions(-) create mode 100644 utils/middleware.ts create mode 100644 utils/redirect_url.ts create mode 100644 utils/validation_errors.ts diff --git a/ValidateProfileEmail/__tests__/handler.test.ts b/ValidateProfileEmail/__tests__/handler.test.ts index bbbd9bd..1b19231 100644 --- a/ValidateProfileEmail/__tests__/handler.test.ts +++ b/ValidateProfileEmail/__tests__/handler.test.ts @@ -1,10 +1,6 @@ import { ResourceNotFoundCode } from "@pagopa/io-functions-commons/dist/src/utils/azure_storage"; -import { - TokenQueryParam, - ValidateProfileEmailHandler, - ValidationErrors -} from "../handler"; +import { ValidateProfileEmailHandler } from "../handler"; import { EmailString, FiscalCode } from "@pagopa/ts-commons/lib/strings"; import { IProfileEmailReader } from "@pagopa/io-functions-commons/dist/src/utils/unique_email_enforcement"; import { constTrue } from "fp-ts/lib/function"; @@ -13,6 +9,12 @@ import * as O from "fp-ts/Option"; import { ProfileModel } from "@pagopa/io-functions-commons/dist/src/models/profile"; import { aFiscalCode, aRetrievedProfile, anEmail } from "../__mocks__/profile"; import { ValidUrl } from "@pagopa/ts-commons/lib/url"; +import { FlowChoiceEnum, TokenQueryParam } from "../../utils/middleware"; +import { + confirmChoicePageUrl, + validationFailureUrl, + validationSuccessUrl +} from "../../utils/redirect_url"; const VALIDATION_TOKEN = "01DPT9QAZ6N0FJX21A86FRCWB3:8c652f8566ba53bd8cf0b1b9" as TokenQueryParam; @@ -38,7 +40,7 @@ const contextMock = { const validationCallbackUrl = { href: "localhost/validation" -}; +} as ValidUrl; const confirmChoiceUrl = { href: "localhost/confirm-choice" @@ -46,30 +48,6 @@ const confirmChoiceUrl = { const timestampGeneratorMock = () => 1234567890; -const errorUrl = ( - error: keyof typeof ValidationErrors, - timestampGenerator: () => number -) => { - return `${ - validationCallbackUrl.href - }?result=failure&error=${error}&time=${timestampGenerator()}`; -}; - -const successUrl = (timestampGenerator: () => number) => { - return `${ - validationCallbackUrl.href - }?result=success&time=${timestampGenerator()}`; -}; - -const confirmPageUrl = ( - token: string, - email: EmailString, - timestampGenerator: () => number -) => - `${ - confirmChoiceUrl.href - }?token=${token}&email=${email}&time=${timestampGenerator()}`; - const mockRetrieveEntity = jest .fn() .mockImplementation((_, __, ___, ____, f) => { @@ -119,16 +97,16 @@ describe("ValidateProfileEmailHandler", () => { }); it.each` - scenario | expectedError | callbackInputs | isConfirmStep - ${"GENERIC_ERROR in case the query versus the table storage fails during confirm step"} | ${"GENERIC_ERROR"} | ${[new Error()]} | ${true} + scenario | expectedError | callbackInputs | isConfirmFlow + ${"GENERIC_ERROR in case the query versus the table storage fails during confirm flow"} | ${"GENERIC_ERROR"} | ${[new Error()]} | ${true} ${"GENERIC_ERROR in case the query versus the table storage fails"} | ${"GENERIC_ERROR"} | ${[new Error()]} | ${false} - ${"INVALID_TOKEN error in case the token if not found in the table during confirm step"} | ${"INVALID_TOKEN"} | ${[{ code: ResourceNotFoundCode }]} | ${true} + ${"INVALID_TOKEN error in case the token if not found in the table during confirm flow"} | ${"INVALID_TOKEN"} | ${[{ code: ResourceNotFoundCode }]} | ${true} ${"INVALID_TOKEN error in case the token if not found in the table"} | ${"INVALID_TOKEN"} | ${[{ code: ResourceNotFoundCode }]} | ${false} - ${"TOKEN_EXPIRED error in case the token is expired during confirm step"} | ${"TOKEN_EXPIRED"} | ${[undefined, expiredTokenEntity]} | ${true} + ${"TOKEN_EXPIRED error in case the token is expired during confirm flow"} | ${"TOKEN_EXPIRED"} | ${[undefined, expiredTokenEntity]} | ${true} ${"TOKEN_EXPIRED error in case the token is expired"} | ${"TOKEN_EXPIRED"} | ${[undefined, expiredTokenEntity]} | ${false} `( "should return a redirect with a $scenario", - async ({ callbackInputs, expectedError, isConfirmStep }) => { + async ({ callbackInputs, expectedError, isConfirmFlow }) => { mockRetrieveEntity.mockImplementationOnce((_, __, ___, ____, f) => { f(...callbackInputs); }); @@ -147,12 +125,16 @@ describe("ValidateProfileEmailHandler", () => { const response = await verifyProfileEmailHandler( contextMock as any, VALIDATION_TOKEN, - isConfirmStep ? O.some(true) : O.none + isConfirmFlow ? FlowChoiceEnum.CONFIRM : FlowChoiceEnum.VALIDATE ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); expect(response.detail).toBe( - errorUrl(expectedError, timestampGeneratorMock) + validationFailureUrl( + validationCallbackUrl, + expectedError, + timestampGeneratorMock + ).href ); expect(mockFindLastVersionByModelId).not.toBeCalled(); expect(mockUpdate).not.toBeCalled(); @@ -160,14 +142,14 @@ describe("ValidateProfileEmailHandler", () => { ); it.each` - scenario | expectedError | isThrowing | isConfirmStep - ${"should return IResponseSeeOtherRedirect if the e-mail is already taken (unique email enforcement = %uee) WHEN a citizen changes e-mail during confirm step"} | ${"EMAIL_ALREADY_TAKEN"} | ${undefined} | ${true} + scenario | expectedError | isThrowing | isConfirmFlow + ${"should return IResponseSeeOtherRedirect if the e-mail is already taken (unique email enforcement = %uee) WHEN a citizen changes e-mail during confirm flow"} | ${"EMAIL_ALREADY_TAKEN"} | ${undefined} | ${true} ${"should return IResponseSeeOtherRedirect if the e-mail is already taken (unique email enforcement = %uee) WHEN a citizen changes e-mail"} | ${"EMAIL_ALREADY_TAKEN"} | ${undefined} | ${false} - ${"return 500 WHEN the unique e-mail enforcement check fails during confirm step"} | ${"GENERIC_ERROR"} | ${true} | ${true} + ${"return 500 WHEN the unique e-mail enforcement check fails during confirm flow"} | ${"GENERIC_ERROR"} | ${true} | ${true} ${"return 500 WHEN the unique e-mail enforcement check fails"} | ${"GENERIC_ERROR"} | ${true} | ${false} `( "should $scenario", - async ({ expectedError, isThrowing, isConfirmStep }) => { + async ({ expectedError, isThrowing, isConfirmFlow }) => { const verifyProfileEmailHandler = ValidateProfileEmailHandler( tableServiceMock as any, "", @@ -184,19 +166,23 @@ describe("ValidateProfileEmailHandler", () => { const response = await verifyProfileEmailHandler( contextMock as any, VALIDATION_TOKEN, - isConfirmStep ? O.some(true) : O.none + isConfirmFlow ? FlowChoiceEnum.CONFIRM : FlowChoiceEnum.VALIDATE ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); expect(response.detail).toBe( - errorUrl(expectedError, timestampGeneratorMock) + validationFailureUrl( + validationCallbackUrl, + expectedError, + timestampGeneratorMock + ).href ); expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); expect(mockUpdate).not.toBeCalled(); } ); - it("should validate the email in profile if all the condition are verified and we are in the confirm step", async () => { + it("should validate the email in profile if all the condition are verified and we are in the confirm flow", async () => { const verifyProfileEmailHandler = ValidateProfileEmailHandler( tableServiceMock as any, "", @@ -213,18 +199,20 @@ describe("ValidateProfileEmailHandler", () => { const response = await verifyProfileEmailHandler( contextMock as any, VALIDATION_TOKEN, - O.some(true) + FlowChoiceEnum.VALIDATE ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); - expect(response.detail).toBe(successUrl(timestampGeneratorMock)); + expect(response.detail).toBe( + validationSuccessUrl(validationCallbackUrl, timestampGeneratorMock).href + ); expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); expect(mockUpdate).toBeCalledWith( expect.objectContaining({ isEmailValidated: true }) ); }); - it("should NOT validate the email in profile if we are NOT in the confirm step", async () => { + it("should NOT validate the email in profile if we are in the CONFIRM flow", async () => { const verifyProfileEmailHandler = ValidateProfileEmailHandler( tableServiceMock as any, "", @@ -241,12 +229,12 @@ describe("ValidateProfileEmailHandler", () => { const response = await verifyProfileEmailHandler( contextMock as any, VALIDATION_TOKEN, - O.none + FlowChoiceEnum.CONFIRM ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); expect(response.detail).toBe( - confirmPageUrl(VALIDATION_TOKEN, anEmail, timestampGeneratorMock) + confirmChoicePageUrl(confirmChoiceUrl, VALIDATION_TOKEN, anEmail).href ); expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); expect(mockUpdate).not.toHaveBeenCalled(); diff --git a/ValidateProfileEmail/handler.ts b/ValidateProfileEmail/handler.ts index 7ebb150..54355d3 100644 --- a/ValidateProfileEmail/handler.ts +++ b/ValidateProfileEmail/handler.ts @@ -5,8 +5,6 @@ import * as express from "express"; import { isLeft } from "fp-ts/lib/Either"; import { isNone } from "fp-ts/lib/Option"; -import * as t from "io-ts"; -import * as O from "fp-ts/lib/Option"; import * as TE from "fp-ts/lib/TaskEither"; import { pipe } from "fp-ts/function"; @@ -19,16 +17,8 @@ import { IResponseSeeOtherRedirect, ResponseSeeOtherRedirect } from "@pagopa/ts-commons/lib/responses"; -import { - EmailString, - FiscalCode, - PatternString -} from "@pagopa/ts-commons/lib/strings"; +import { FiscalCode } from "@pagopa/ts-commons/lib/strings"; import { hashFiscalCode } from "@pagopa/ts-commons/lib/hash"; - -import { RequiredQueryParamMiddleware } from "@pagopa/io-functions-commons/dist/src/utils/middlewares/required_query_param"; -import { OptionalQueryParamMiddleware } from "@pagopa/io-functions-commons/dist/src/utils/middlewares/optional_query_param"; - import { ValidationTokenEntity } from "@pagopa/io-functions-commons/dist/src/entities/validation_token"; import { ProfileModel } from "@pagopa/io-functions-commons/dist/src/models/profile"; import { retrieveTableEntity } from "@pagopa/io-functions-commons/dist/src/utils/azure_storage"; @@ -42,84 +32,27 @@ import { IProfileEmailReader, isEmailAlreadyTaken } from "@pagopa/io-functions-commons/dist/src/utils/unique_email_enforcement"; -import { BooleanFromString } from "io-ts-types"; import { trackEvent } from "../utils/appinsights"; - -// Tokens are generated by CreateValidationTokenActivity function inside the -// io-functions-app project (https://github.com/pagopa/io-functions-app) -// A token is in the following format: -// [tokenId ULID] + ":" + [validatorHash crypto.randomBytes(12)] -export const TokenQueryParam = PatternString( - "^[A-Za-z0-9]{26}:[A-Fa-f0-9]{24}$" -); -export type TokenQueryParam = t.TypeOf; +import { + ChoiceConfirmQueryParamMiddleware, + FlowChoice, + FlowChoiceEnum, + TokenQueryParam, + TokenQueryParamMiddleware +} from "../utils/middleware"; +import { + confirmChoicePageUrl, + validationFailureUrl, + validationSuccessUrl +} from "../utils/redirect_url"; +import { ValidationErrors } from "../utils/validation_errors"; type IValidateProfileEmailHandler = ( context: Context, token: TokenQueryParam, - confirm: O.Option + flowChoice: FlowChoice ) => Promise; -// Used in the callback -export enum ValidationErrors { - GENERIC_ERROR = "GENERIC_ERROR", - INVALID_TOKEN = "INVALID_TOKEN", - TOKEN_EXPIRED = "TOKEN_EXPIRED", - EMAIL_ALREADY_TAKEN = "EMAIL_ALREADY_TAKEN" -} - -/** - * Returns a ValidUrl that represents a successful validation - */ -const confirmChoicePageUrl = ( - url: ValidUrl, - token: TokenQueryParam, - email: EmailString, - timeStampGenerator: () => number -): ValidUrl => - ({ - href: `${ - url.href - }?token=${token}&email=${email}&time=${timeStampGenerator()}` - } as ValidUrl); - -/** - * Returns a ValidUrl that represents a successful validation - */ -const validationSuccessUrl = ( - validationCallbackUrl: ValidUrl, - timeStampGenerator: () => number -): ValidUrl => - ({ - href: `${ - validationCallbackUrl.href - }?result=success&time=${timeStampGenerator()}` - } as ValidUrl); - -/** - * Returns a ValidUrl that represents a failed validation - */ -const validationFailureUrl = ( - validationCallbackUrl: ValidUrl, - error: keyof typeof ValidationErrors, - timeStampGenerator: () => number -): ValidUrl => - ({ - href: `${ - validationCallbackUrl.href - }?result=failure&error=${error}&time=${timeStampGenerator()}` - } as ValidUrl); - -const TokenQueryParamMiddleware = RequiredQueryParamMiddleware( - "token", - TokenQueryParam -); - -const ChoiceConfirmQueryParamMiddleware = OptionalQueryParamMiddleware( - "confirm", - BooleanFromString.pipe(t.literal(true)) -); - export const ValidateProfileEmailHandler = ( tableService: TableService, validationTokensTableName: string, @@ -132,7 +65,7 @@ export const ValidateProfileEmailHandler = ( ): IValidateProfileEmailHandler => async ( context, token, - confirm + flowChoice ): Promise => { const logPrefix = `ValidateProfileEmail|TOKEN=${token}`; const vFailureUrl = (error: keyof typeof ValidationErrors): ValidUrl => @@ -260,49 +193,44 @@ export const ValidateProfileEmailHandler = ( // Update the profile and set isEmailValidated to `true` only if the confirm parameter is true // otherwise just redirect to confirm page with token in query param return await pipe( - confirm, - O.fold( + flowChoice, + TE.fromPredicate( + flow => flow === FlowChoiceEnum.VALIDATE, () => - TE.right( - ResponseSeeOtherRedirect( - confirmChoicePageUrl( - confirmChoiceUrl, - token, - email, - timeStampGenerator - ) - ) - ), - _ => - pipe( - profileModel.update({ - ...existingProfile, - isEmailValidated: true - }), - TE.mapLeft(error => { - context.log.error( - `${logPrefix}|Error updating profile|ERROR=${error}` - ); - return ResponseSeeOtherRedirect( - vFailureUrl(ValidationErrors.GENERIC_ERROR) - ); - }), - TE.map(() => { - trackEvent({ - name: "io.citizen-auth.validate_email", - tagOverrides: { - "ai.user.id": hashFiscalCode(existingProfile.fiscalCode), - samplingEnabled: "false" - } - }); - - context.log.verbose(`${logPrefix}|The profile has been updated`); - return ResponseSeeOtherRedirect( - validationSuccessUrl(validationCallbackUrl, timeStampGenerator) - ); - }) + ResponseSeeOtherRedirect( + confirmChoicePageUrl(confirmChoiceUrl, token, email) ) ), + TE.chain(() => + pipe( + profileModel.update({ + ...existingProfile, + isEmailValidated: true + }), + TE.mapLeft(error => { + context.log.error( + `${logPrefix}|Error updating profile|ERROR=${error}` + ); + return ResponseSeeOtherRedirect( + vFailureUrl(ValidationErrors.GENERIC_ERROR) + ); + }), + TE.map(() => { + trackEvent({ + name: "io.citizen-auth.validate_email", + tagOverrides: { + "ai.user.id": hashFiscalCode(existingProfile.fiscalCode), + samplingEnabled: "false" + } + }); + + context.log.verbose(`${logPrefix}|The profile has been updated`); + return ResponseSeeOtherRedirect( + validationSuccessUrl(validationCallbackUrl, timeStampGenerator) + ); + }) + ) + ), TE.toUnion )(); }; diff --git a/openapi/external.yaml b/openapi/external.yaml index 0b60f53..b80b45c 100644 --- a/openapi/external.yaml +++ b/openapi/external.yaml @@ -20,11 +20,16 @@ paths: type: string minLength: 1 - in: query - name: confirm + name: flow + description: | + CONFIRM -> verify token and on success redirect to confirm page + VALIDATE -> verify token and on success redirect to result page schema: type: string enum: - - true + - "CONFIRM" + - "VALIDATE" + default: "CONFIRM" responses: "303": description: See Others diff --git a/package.json b/package.json index c7f2365..6ec9e9a 100644 --- a/package.json +++ b/package.json @@ -44,6 +44,7 @@ "@pagopa/ts-commons": "^12.5.0", "applicationinsights": "^2.9.2", "azure-storage": "^2.10.7", + "base64url": "^3.0.1", "express": "^4.15.3", "fp-ts": "^2.16.1", "io-ts": "^2.2.21", diff --git a/utils/middleware.ts b/utils/middleware.ts new file mode 100644 index 0000000..4ddcc49 --- /dev/null +++ b/utils/middleware.ts @@ -0,0 +1,37 @@ +import * as t from "io-ts"; +import { PatternString } from "@pagopa/ts-commons/lib/strings"; +import { RequiredQueryParamMiddleware } from "@pagopa/io-functions-commons/dist/src/utils/middlewares/required_query_param"; +import { enumType, withDefault } from "@pagopa/ts-commons/lib/types"; + +// Tokens are generated by CreateValidationTokenActivity function inside the +// io-functions-app project (https://github.com/pagopa/io-functions-app) +// A token is in the following format: +// [tokenId ULID] + ":" + [validatorHash crypto.randomBytes(12)] +export const TokenQueryParam = PatternString( + "^[A-Za-z0-9]{26}:[A-Fa-f0-9]{24}$" +); +export type TokenQueryParam = t.TypeOf; + +export const TokenQueryParamMiddleware = RequiredQueryParamMiddleware( + "token", + TokenQueryParam +); + +// CONFIRM -> verify token and on success redirect to confirm page +// VALIDATE -> verify token and on success redirect to result page +export enum FlowChoiceEnum { + "CONFIRM" = "CONFIRM", + "VALIDATE" = "VALIDATE" +} +export const FlowChoice = enumType( + FlowChoiceEnum, + "FlowChoice" +); +export type FlowChoice = t.TypeOf; + +// even if the query param is optional the withDefault type is covering the absence +// of the value on the URL +export const ChoiceConfirmQueryParamMiddleware = RequiredQueryParamMiddleware( + "flow", + withDefault(FlowChoice, FlowChoiceEnum.CONFIRM) +); diff --git a/utils/redirect_url.ts b/utils/redirect_url.ts new file mode 100644 index 0000000..1e8db63 --- /dev/null +++ b/utils/redirect_url.ts @@ -0,0 +1,44 @@ +import { EmailString } from "@pagopa/ts-commons/lib/strings"; +import { ValidUrl } from "@pagopa/ts-commons/lib/url"; +import base64url from "base64url"; +import { TokenQueryParam } from "./middleware"; +import { ValidationErrors } from "./validation_errors"; + +/** + * Returns a ValidUrl that represents a successful validation + */ +export const confirmChoicePageUrl = ( + url: ValidUrl, + token: TokenQueryParam, + email: EmailString +): ValidUrl => + ({ + href: `${url.href}?token=${token}&email=${base64url(email)}` + } as ValidUrl); + +/** + * Returns a ValidUrl that represents a successful validation + */ +export const validationSuccessUrl = ( + validationCallbackUrl: ValidUrl, + timeStampGenerator: () => number +): ValidUrl => + ({ + href: `${ + validationCallbackUrl.href + }?result=success&time=${timeStampGenerator()}` + } as ValidUrl); + +/** + * Returns a ValidUrl that represents a failed validation + */ +export const validationFailureUrl = ( + validationCallbackUrl: ValidUrl, + error: keyof typeof ValidationErrors, + timeStampGenerator: () => number +): ValidUrl => + ({ + href: `${ + validationCallbackUrl.href + }?result=failure&error=${error}&time=${timeStampGenerator()}` + } as ValidUrl); diff --git a/utils/validation_errors.ts b/utils/validation_errors.ts new file mode 100644 index 0000000..1628978 --- /dev/null +++ b/utils/validation_errors.ts @@ -0,0 +1,6 @@ +export enum ValidationErrors { + GENERIC_ERROR = "GENERIC_ERROR", + INVALID_TOKEN = "INVALID_TOKEN", + TOKEN_EXPIRED = "TOKEN_EXPIRED", + EMAIL_ALREADY_TAKEN = "EMAIL_ALREADY_TAKEN" +} diff --git a/yarn.lock b/yarn.lock index 30a0f3a..503b780 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2282,6 +2282,11 @@ balanced-match@^1.0.0: resolved "https://registry.yarnpkg.com/balanced-match/-/balanced-match-1.0.2.tgz#e83e3a7e3f300b34cb9d87f615fa0cbf357690ee" integrity sha512-3oSeUO0TMV67hN1AmbXsK4yaqU7tjiHlbxRDZOpH0KW9+CeX4bRAaX0Anxt0tx2MrpRpWwQaPwIlISEJhYU5Pw== +base64url@^3.0.1: + version "3.0.1" + resolved "https://registry.yarnpkg.com/base64url/-/base64url-3.0.1.tgz#6399d572e2bc3f90a9a8b22d5dbb0a32d33f788d" + integrity sha512-ir1UPr3dkwexU7FdV8qBBbNDRUhMmIekYMFZfi+C/sLNnRESKPl23nB9b2pltqfOQNnGzsDdId90AEtG5tCx4A== + base@^0.11.1: version "0.11.2" resolved "https://registry.yarnpkg.com/base/-/base-0.11.2.tgz#7bde5ced145b6d551a90db87f83c558b4eb48a8f" From 9481129711a9404f986d5915fc9e48a4de0c9842 Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Fri, 23 Feb 2024 09:56:05 +0100 Subject: [PATCH 07/10] [#IOPID-1604] comment clarification --- ValidateProfileEmail/handler.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ValidateProfileEmail/handler.ts b/ValidateProfileEmail/handler.ts index 54355d3..61fe94d 100644 --- a/ValidateProfileEmail/handler.ts +++ b/ValidateProfileEmail/handler.ts @@ -190,8 +190,8 @@ export const ValidateProfileEmailHandler = ( } } - // Update the profile and set isEmailValidated to `true` only if the confirm parameter is true - // otherwise just redirect to confirm page with token in query param + // Update the profile and set isEmailValidated to `true` ONLY if the flowChoice equals to VALIDATE + // otherwise just redirect to confirm page with token and email(base64url encoded) in query param return await pipe( flowChoice, TE.fromPredicate( From 7d115d231be6393d3b76e2db3ca557f7419a4eab Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Thu, 29 Feb 2024 09:32:31 +0100 Subject: [PATCH 08/10] [#IOPID-1604] applied review suggestions --- .../__tests__/handler.test.ts | 54 +++++++------------ ValidateProfileEmail/handler.ts | 43 +++++++-------- ValidateProfileEmail/index.ts | 9 ++-- utils/middleware.ts | 13 ++--- utils/redirect_url.ts | 14 ++--- 5 files changed, 55 insertions(+), 78 deletions(-) diff --git a/ValidateProfileEmail/__tests__/handler.test.ts b/ValidateProfileEmail/__tests__/handler.test.ts index 1b19231..6b0361a 100644 --- a/ValidateProfileEmail/__tests__/handler.test.ts +++ b/ValidateProfileEmail/__tests__/handler.test.ts @@ -9,7 +9,7 @@ import * as O from "fp-ts/Option"; import { ProfileModel } from "@pagopa/io-functions-commons/dist/src/models/profile"; import { aFiscalCode, aRetrievedProfile, anEmail } from "../__mocks__/profile"; import { ValidUrl } from "@pagopa/ts-commons/lib/url"; -import { FlowChoiceEnum, TokenQueryParam } from "../../utils/middleware"; +import { FlowTypeEnum, TokenQueryParam } from "../../utils/middleware"; import { confirmChoicePageUrl, validationFailureUrl, @@ -42,11 +42,11 @@ const validationCallbackUrl = { href: "localhost/validation" } as ValidUrl; -const confirmChoiceUrl = { +const confirmValidationUrl = { href: "localhost/confirm-choice" } as ValidUrl; -const timestampGeneratorMock = () => 1234567890; +const emailValidationUrls = { confirmValidationUrl, validationCallbackUrl }; const mockRetrieveEntity = jest .fn() @@ -115,26 +115,20 @@ describe("ValidateProfileEmailHandler", () => { tableServiceMock as any, "", mockProfileModel, - validationCallbackUrl as any, - timestampGeneratorMock, + emailValidationUrls, profileEmailReader, - constTrue, - confirmChoiceUrl + constTrue ); const response = await verifyProfileEmailHandler( contextMock as any, VALIDATION_TOKEN, - isConfirmFlow ? FlowChoiceEnum.CONFIRM : FlowChoiceEnum.VALIDATE + isConfirmFlow ? FlowTypeEnum.CONFIRM : FlowTypeEnum.VALIDATE ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); expect(response.detail).toBe( - validationFailureUrl( - validationCallbackUrl, - expectedError, - timestampGeneratorMock - ).href + validationFailureUrl(validationCallbackUrl, expectedError).href ); expect(mockFindLastVersionByModelId).not.toBeCalled(); expect(mockUpdate).not.toBeCalled(); @@ -154,28 +148,22 @@ describe("ValidateProfileEmailHandler", () => { tableServiceMock as any, "", mockProfileModel, - validationCallbackUrl as any, - timestampGeneratorMock, + emailValidationUrls, { list: generateProfileEmails(1, isThrowing) }, - constTrue, - confirmChoiceUrl + constTrue ); const response = await verifyProfileEmailHandler( contextMock as any, VALIDATION_TOKEN, - isConfirmFlow ? FlowChoiceEnum.CONFIRM : FlowChoiceEnum.VALIDATE + isConfirmFlow ? FlowTypeEnum.CONFIRM : FlowTypeEnum.VALIDATE ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); expect(response.detail).toBe( - validationFailureUrl( - validationCallbackUrl, - expectedError, - timestampGeneratorMock - ).href + validationFailureUrl(validationCallbackUrl, expectedError).href ); expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); expect(mockUpdate).not.toBeCalled(); @@ -187,24 +175,22 @@ describe("ValidateProfileEmailHandler", () => { tableServiceMock as any, "", mockProfileModel, - validationCallbackUrl as any, - timestampGeneratorMock, + emailValidationUrls, { list: generateProfileEmails(0) }, - constTrue, - confirmChoiceUrl + constTrue ); const response = await verifyProfileEmailHandler( contextMock as any, VALIDATION_TOKEN, - FlowChoiceEnum.VALIDATE + FlowTypeEnum.VALIDATE ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); expect(response.detail).toBe( - validationSuccessUrl(validationCallbackUrl, timestampGeneratorMock).href + validationSuccessUrl(validationCallbackUrl).href ); expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); expect(mockUpdate).toBeCalledWith( @@ -217,24 +203,22 @@ describe("ValidateProfileEmailHandler", () => { tableServiceMock as any, "", mockProfileModel, - validationCallbackUrl as any, - timestampGeneratorMock, + emailValidationUrls, { list: generateProfileEmails(0) }, - constTrue, - confirmChoiceUrl + constTrue ); const response = await verifyProfileEmailHandler( contextMock as any, VALIDATION_TOKEN, - FlowChoiceEnum.CONFIRM + FlowTypeEnum.CONFIRM ); expect(response.kind).toBe("IResponseSeeOtherRedirect"); expect(response.detail).toBe( - confirmChoicePageUrl(confirmChoiceUrl, VALIDATION_TOKEN, anEmail).href + confirmChoicePageUrl(confirmValidationUrl, VALIDATION_TOKEN, anEmail).href ); expect(mockFindLastVersionByModelId).toBeCalledWith([aFiscalCode]); expect(mockUpdate).not.toHaveBeenCalled(); diff --git a/ValidateProfileEmail/handler.ts b/ValidateProfileEmail/handler.ts index 61fe94d..e65757c 100644 --- a/ValidateProfileEmail/handler.ts +++ b/ValidateProfileEmail/handler.ts @@ -34,9 +34,9 @@ import { } from "@pagopa/io-functions-commons/dist/src/utils/unique_email_enforcement"; import { trackEvent } from "../utils/appinsights"; import { - ChoiceConfirmQueryParamMiddleware, - FlowChoice, - FlowChoiceEnum, + ConfirmEmailFlowQueryParamMiddleware, + FlowType, + FlowTypeEnum, TokenQueryParam, TokenQueryParamMiddleware } from "../utils/middleware"; @@ -50,26 +50,28 @@ import { ValidationErrors } from "../utils/validation_errors"; type IValidateProfileEmailHandler = ( context: Context, token: TokenQueryParam, - flowChoice: FlowChoice + flowChoice: FlowType ) => Promise; export const ValidateProfileEmailHandler = ( tableService: TableService, validationTokensTableName: string, profileModel: ProfileModel, - validationCallbackUrl: ValidUrl, - timeStampGenerator: () => number, + emailValidationUrls: { + confirmValidationUrl: ValidUrl; + validationCallbackUrl: ValidUrl; + }, profileEmails: IProfileEmailReader, - FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean, - confirmChoiceUrl: ValidUrl + FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean ): IValidateProfileEmailHandler => async ( context, token, flowChoice ): Promise => { const logPrefix = `ValidateProfileEmail|TOKEN=${token}`; + const { validationCallbackUrl, confirmValidationUrl } = emailValidationUrls; const vFailureUrl = (error: keyof typeof ValidationErrors): ValidUrl => - validationFailureUrl(validationCallbackUrl, error, timeStampGenerator); + validationFailureUrl(validationCallbackUrl, error); // STEP 1: Find and verify validation token @@ -195,10 +197,10 @@ export const ValidateProfileEmailHandler = ( return await pipe( flowChoice, TE.fromPredicate( - flow => flow === FlowChoiceEnum.VALIDATE, + flow => flow === FlowTypeEnum.VALIDATE, () => ResponseSeeOtherRedirect( - confirmChoicePageUrl(confirmChoiceUrl, token, email) + confirmChoicePageUrl(confirmValidationUrl, token, email) ) ), TE.chain(() => @@ -226,7 +228,7 @@ export const ValidateProfileEmailHandler = ( context.log.verbose(`${logPrefix}|The profile has been updated`); return ResponseSeeOtherRedirect( - validationSuccessUrl(validationCallbackUrl, timeStampGenerator) + validationSuccessUrl(validationCallbackUrl) ); }) ) @@ -243,27 +245,26 @@ export const ValidateProfileEmail = ( tableService: TableService, validationTokensTableName: string, profileModel: ProfileModel, - validationCallbackUrl: ValidUrl, - timeStampGenerator: () => number, + emailValidationUrls: { + confirmValidationUrl: ValidUrl; + validationCallbackUrl: ValidUrl; + }, profileEmails: IProfileEmailReader, - FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean, - confirmChoiceUrl: ValidUrl + FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean ): express.RequestHandler => { const handler = ValidateProfileEmailHandler( tableService, validationTokensTableName, profileModel, - validationCallbackUrl, - timeStampGenerator, + emailValidationUrls, profileEmails, - FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED, - confirmChoiceUrl + FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED ); const middlewaresWrap = withRequestMiddlewares( ContextMiddleware(), TokenQueryParamMiddleware, - ChoiceConfirmQueryParamMiddleware + ConfirmEmailFlowQueryParamMiddleware ); return wrapRequestHandler(middlewaresWrap(handler)); }; diff --git a/ValidateProfileEmail/index.ts b/ValidateProfileEmail/index.ts index 41fad66..ce78a04 100644 --- a/ValidateProfileEmail/index.ts +++ b/ValidateProfileEmail/index.ts @@ -65,11 +65,12 @@ app.get( tableService, VALIDATION_TOKEN_TABLE_NAME, profileModel, - validationCallbackValidUrl, - Date.now, + { + confirmValidationUrl: config.CONFIRM_CHOICE_PAGE_URL, + validationCallbackUrl: validationCallbackValidUrl + }, profileEmailsReader, - FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED, - config.CONFIRM_CHOICE_PAGE_URL + FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED ) ); diff --git a/utils/middleware.ts b/utils/middleware.ts index 4ddcc49..3d12216 100644 --- a/utils/middleware.ts +++ b/utils/middleware.ts @@ -19,19 +19,16 @@ export const TokenQueryParamMiddleware = RequiredQueryParamMiddleware( // CONFIRM -> verify token and on success redirect to confirm page // VALIDATE -> verify token and on success redirect to result page -export enum FlowChoiceEnum { +export enum FlowTypeEnum { "CONFIRM" = "CONFIRM", "VALIDATE" = "VALIDATE" } -export const FlowChoice = enumType( - FlowChoiceEnum, - "FlowChoice" -); -export type FlowChoice = t.TypeOf; +export const FlowType = enumType(FlowTypeEnum, "FlowChoice"); +export type FlowType = t.TypeOf; // even if the query param is optional the withDefault type is covering the absence // of the value on the URL -export const ChoiceConfirmQueryParamMiddleware = RequiredQueryParamMiddleware( +export const ConfirmEmailFlowQueryParamMiddleware = RequiredQueryParamMiddleware( "flow", - withDefault(FlowChoice, FlowChoiceEnum.CONFIRM) + withDefault(FlowType, FlowTypeEnum.CONFIRM) ); diff --git a/utils/redirect_url.ts b/utils/redirect_url.ts index 1e8db63..aaff04f 100644 --- a/utils/redirect_url.ts +++ b/utils/redirect_url.ts @@ -20,13 +20,10 @@ export const confirmChoicePageUrl = ( * Returns a ValidUrl that represents a successful validation */ export const validationSuccessUrl = ( - validationCallbackUrl: ValidUrl, - timeStampGenerator: () => number + validationCallbackUrl: ValidUrl ): ValidUrl => ({ - href: `${ - validationCallbackUrl.href - }?result=success&time=${timeStampGenerator()}` + href: `${validationCallbackUrl.href}?result=success}` } as ValidUrl); /** @@ -34,11 +31,8 @@ export const validationSuccessUrl = ( */ export const validationFailureUrl = ( validationCallbackUrl: ValidUrl, - error: keyof typeof ValidationErrors, - timeStampGenerator: () => number + error: keyof typeof ValidationErrors ): ValidUrl => ({ - href: `${ - validationCallbackUrl.href - }?result=failure&error=${error}&time=${timeStampGenerator()}` + href: `${validationCallbackUrl.href}?result=failure&error=${error}}` } as ValidUrl); From 15f20be219f58c613d113032280261a951832195 Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Thu, 29 Feb 2024 09:50:47 +0100 Subject: [PATCH 09/10] [#IOPID-1604] refactored tests based on review suggestion --- .../__tests__/handler.test.ts | 38 +++++++++++-------- utils/middleware.ts | 2 +- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/ValidateProfileEmail/__tests__/handler.test.ts b/ValidateProfileEmail/__tests__/handler.test.ts index 6b0361a..19a7d7c 100644 --- a/ValidateProfileEmail/__tests__/handler.test.ts +++ b/ValidateProfileEmail/__tests__/handler.test.ts @@ -91,22 +91,26 @@ const expiredTokenEntity = { RowKey: "026c47ead971b9af13353f5d5e563982ebca542f8df3246bdaf1f86e16075072" }; -describe("ValidateProfileEmailHandler", () => { +// Flow types: +// CONFIRM -> verify token and on success redirect to confirm page +// VALIDATE -> verify token and on success update the user data and redirect to result page +describe.each` + isConfirmFlow + ${true} + ${false} +`("ValidateProfileEmailHandler#Errors", ({ isConfirmFlow }) => { beforeEach(() => { jest.clearAllMocks(); }); it.each` - scenario | expectedError | callbackInputs | isConfirmFlow - ${"GENERIC_ERROR in case the query versus the table storage fails during confirm flow"} | ${"GENERIC_ERROR"} | ${[new Error()]} | ${true} - ${"GENERIC_ERROR in case the query versus the table storage fails"} | ${"GENERIC_ERROR"} | ${[new Error()]} | ${false} - ${"INVALID_TOKEN error in case the token if not found in the table during confirm flow"} | ${"INVALID_TOKEN"} | ${[{ code: ResourceNotFoundCode }]} | ${true} - ${"INVALID_TOKEN error in case the token if not found in the table"} | ${"INVALID_TOKEN"} | ${[{ code: ResourceNotFoundCode }]} | ${false} - ${"TOKEN_EXPIRED error in case the token is expired during confirm flow"} | ${"TOKEN_EXPIRED"} | ${[undefined, expiredTokenEntity]} | ${true} - ${"TOKEN_EXPIRED error in case the token is expired"} | ${"TOKEN_EXPIRED"} | ${[undefined, expiredTokenEntity]} | ${false} + scenario | expectedError | callbackInputs + ${"GENERIC_ERROR in case the query versus the table storage fails"} | ${"GENERIC_ERROR"} | ${[new Error()]} + ${"INVALID_TOKEN error in case the token if not found in the table"} | ${"INVALID_TOKEN"} | ${[{ code: ResourceNotFoundCode }]} + ${"TOKEN_EXPIRED error in case the token is expired"} | ${"TOKEN_EXPIRED"} | ${[undefined, expiredTokenEntity]} `( "should return a redirect with a $scenario", - async ({ callbackInputs, expectedError, isConfirmFlow }) => { + async ({ callbackInputs, expectedError }) => { mockRetrieveEntity.mockImplementationOnce((_, __, ___, ____, f) => { f(...callbackInputs); }); @@ -136,11 +140,9 @@ describe("ValidateProfileEmailHandler", () => { ); it.each` - scenario | expectedError | isThrowing | isConfirmFlow - ${"should return IResponseSeeOtherRedirect if the e-mail is already taken (unique email enforcement = %uee) WHEN a citizen changes e-mail during confirm flow"} | ${"EMAIL_ALREADY_TAKEN"} | ${undefined} | ${true} - ${"should return IResponseSeeOtherRedirect if the e-mail is already taken (unique email enforcement = %uee) WHEN a citizen changes e-mail"} | ${"EMAIL_ALREADY_TAKEN"} | ${undefined} | ${false} - ${"return 500 WHEN the unique e-mail enforcement check fails during confirm flow"} | ${"GENERIC_ERROR"} | ${true} | ${true} - ${"return 500 WHEN the unique e-mail enforcement check fails"} | ${"GENERIC_ERROR"} | ${true} | ${false} + scenario | expectedError | isThrowing + ${"should return IResponseSeeOtherRedirect if the e-mail is already taken (unique email enforcement = %uee) WHEN a citizen changes e-mail"} | ${"EMAIL_ALREADY_TAKEN"} | ${undefined} + ${"return 500 WHEN the unique e-mail enforcement check fails"} | ${"GENERIC_ERROR"} | ${true} `( "should $scenario", async ({ expectedError, isThrowing, isConfirmFlow }) => { @@ -169,8 +171,14 @@ describe("ValidateProfileEmailHandler", () => { expect(mockUpdate).not.toBeCalled(); } ); +}); + +describe("ValidateProfileEmailHandler#Happy path", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); - it("should validate the email in profile if all the condition are verified and we are in the confirm flow", async () => { + it("should validate the email in profile if all the condition are verified during VALIDATE flow", async () => { const verifyProfileEmailHandler = ValidateProfileEmailHandler( tableServiceMock as any, "", diff --git a/utils/middleware.ts b/utils/middleware.ts index 3d12216..8745f9e 100644 --- a/utils/middleware.ts +++ b/utils/middleware.ts @@ -18,7 +18,7 @@ export const TokenQueryParamMiddleware = RequiredQueryParamMiddleware( ); // CONFIRM -> verify token and on success redirect to confirm page -// VALIDATE -> verify token and on success redirect to result page +// VALIDATE -> verify token and on success update the user data and redirect to result page export enum FlowTypeEnum { "CONFIRM" = "CONFIRM", "VALIDATE" = "VALIDATE" From fdf4b50771dc874c4c53cf4648a002138f47e856 Mon Sep 17 00:00:00 2001 From: arcogabbo Date: Thu, 29 Feb 2024 09:55:36 +0100 Subject: [PATCH 10/10] [#IOPID-1604] fix: lint errors --- ValidateProfileEmail/handler.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ValidateProfileEmail/handler.ts b/ValidateProfileEmail/handler.ts index e65757c..03c7d9d 100644 --- a/ValidateProfileEmail/handler.ts +++ b/ValidateProfileEmail/handler.ts @@ -58,8 +58,8 @@ export const ValidateProfileEmailHandler = ( validationTokensTableName: string, profileModel: ProfileModel, emailValidationUrls: { - confirmValidationUrl: ValidUrl; - validationCallbackUrl: ValidUrl; + readonly confirmValidationUrl: ValidUrl; + readonly validationCallbackUrl: ValidUrl; }, profileEmails: IProfileEmailReader, FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean @@ -246,8 +246,8 @@ export const ValidateProfileEmail = ( validationTokensTableName: string, profileModel: ProfileModel, emailValidationUrls: { - confirmValidationUrl: ValidUrl; - validationCallbackUrl: ValidUrl; + readonly confirmValidationUrl: ValidUrl; + readonly validationCallbackUrl: ValidUrl; }, profileEmails: IProfileEmailReader, FF_UNIQUE_EMAIL_ENFORCEMENT_ENABLED: (fiscalCode: FiscalCode) => boolean