From e32e657cfa297718e837ccda2822c83a50c6f834 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Thu, 9 Nov 2023 16:59:30 -0500 Subject: [PATCH] final pass --- .../remediators/Base/SelectAuthenticator.ts | 24 ++--- lib/oidc/introspect.ts | 1 - .../mfa-password-and-email-magic-link.feature | 2 +- .../features/mfa-password-and-email.feature | 2 + samples/test/steps/after.ts | 7 ++ .../selectors/SelectAuthenticatorMethod.ts | 1 - test/spec/idx/authenticate.ts | 90 ------------------- .../idx/remediators/SelectAuthenticator.ts | 29 +++--- 8 files changed, 33 insertions(+), 123 deletions(-) diff --git a/lib/idx/remediators/Base/SelectAuthenticator.ts b/lib/idx/remediators/Base/SelectAuthenticator.ts index f9bc947c0..ffc0b06df 100644 --- a/lib/idx/remediators/Base/SelectAuthenticator.ts +++ b/lib/idx/remediators/Base/SelectAuthenticator.ts @@ -60,18 +60,19 @@ export class SelectAuthenticator { - // return auth && !auth.value?.resend - // && auth.value.id === matchedOption.relatesTo?.id; - // }; - - // // Don't select current authenticator (OKTA-612939) - // // Follow up: OKTA-646147 - original fix caused different issue - // const isCurrentAuthenticator = isAuthenticatorCurrent(context?.currentAuthenticator); // false - // const isCurrentAuthenticatorEnrollment = isAuthenticatorCurrent(context?.currentAuthenticatorEnrollment); // true - // return !isCurrentAuthenticator && !isCurrentAuthenticatorEnrollment; // false + // fix for OKTA-612939 (below) seems to have caused a bug when trying to re-select authenticators + // with multiple methodTypes. If `options.step` is passed, this remediation is explicitly being + // invoked, therefore do not guard against auto-remediating the selected authenticator (OKTA-646147) + if (this.options.step) { + return true; + } - return true; + // Don't select current authenticator (OKTA-612939) + const isCurrentAuthenticator = context?.currentAuthenticator + && context?.currentAuthenticator.value.id === matchedOption.relatesTo?.id; + const isCurrentAuthenticatorEnrollment = context?.currentAuthenticatorEnrollment + && context?.currentAuthenticatorEnrollment.value.id === matchedOption.relatesTo?.id; + return !isCurrentAuthenticator && !isCurrentAuthenticatorEnrollment; } return false; @@ -112,7 +113,6 @@ export class SelectAuthenticator { return compareAuthenticators(authenticator, this.selectedAuthenticator) !== true; }); - // return { ...this.values, authenticators, authenticator: undefined }; return { ...this.values, authenticators }; } diff --git a/lib/oidc/introspect.ts b/lib/oidc/introspect.ts index af682364d..73fdda60d 100644 --- a/lib/oidc/introspect.ts +++ b/lib/oidc/introspect.ts @@ -44,7 +44,6 @@ export async function oidcIntrospect (sdk, kind: TokenKind, token?: Token) { else { issuer = (token as any)?.claims?.iss; } - // issuer ??= sdk.options.issuer; issuer = issuer || sdk.options.issuer; if (!clientId) { diff --git a/samples/test/features/mfa-password-and-email-magic-link.feature b/samples/test/features/mfa-password-and-email-magic-link.feature index 67501d731..3636c5aaa 100644 --- a/samples/test/features/mfa-password-and-email-magic-link.feature +++ b/samples/test/features/mfa-password-and-email-magic-link.feature @@ -16,7 +16,7 @@ Feature: Multi-Factor Authentication with Password and Email Magic Link And she has inserted her password And her password is correct When she submits the form - Then she is redirected to the "Verify Email" page + Then she is redirected to the "Verify Email" page # fails here When she selects "Email" from the list of methods And she submits the form And she clicks the Email magic link diff --git a/samples/test/features/mfa-password-and-email.feature b/samples/test/features/mfa-password-and-email.feature index 995e952ec..844e20002 100644 --- a/samples/test/features/mfa-password-and-email.feature +++ b/samples/test/features/mfa-password-and-email.feature @@ -14,6 +14,7 @@ Feature: Multi-Factor Authentication with Password and Email When she fills in her username And she fills in her correct password And she submits the form + # fails here Then she is redirected to the "Verify Email" page When she selects "Email" from the list of methods And she submits the form @@ -30,6 +31,7 @@ Feature: Multi-Factor Authentication with Password and Email And she has inserted her password And her password is correct When she submits the form + # fails here Then she is redirected to the "Verify Email" page When she selects "Email" from the list of methods And she submits the form diff --git a/samples/test/steps/after.ts b/samples/test/steps/after.ts index 117ee246f..5b68ed5a7 100644 --- a/samples/test/steps/after.ts +++ b/samples/test/steps/after.ts @@ -12,10 +12,17 @@ /* eslint-disable complexity */ +// import { After, AfterStep, Status } from '@cucumber/cucumber'; import { After } from '@cucumber/cucumber'; import ActionContext from '../support/context'; import deleteSelfEnrolledUser from '../support/management-api/deleteSelfEnrolledUser'; +// NOTE: can be used to debug cucumber tests, just uncomment +// AfterStep(async ({ result }) => { +// if (result.status === Status.FAILED) { +// await browser.debug(); +// } +// }); // Comment out this after hook to persist test context // Extend the hook timeout to fight against org rate limit diff --git a/samples/test/support/selectors/SelectAuthenticatorMethod.ts b/samples/test/support/selectors/SelectAuthenticatorMethod.ts index b812f286d..890a4272b 100644 --- a/samples/test/support/selectors/SelectAuthenticatorMethod.ts +++ b/samples/test/support/selectors/SelectAuthenticatorMethod.ts @@ -17,7 +17,6 @@ class SelectAuthenticatorMethod extends PageWithTitle { title = 'Select authenticator method'; get pageTitle() {return '#page-title-header';} - // get pageTitle() {return '#select-authenticator-page-title-header';} get options() { return '#authenticator-method-options'; } get submit() { return '#verify-form button[type=submit]';} diff --git a/test/spec/idx/authenticate.ts b/test/spec/idx/authenticate.ts index 7d1a4b471..66242de2f 100644 --- a/test/spec/idx/authenticate.ts +++ b/test/spec/idx/authenticate.ts @@ -2524,94 +2524,4 @@ describe('idx/authenticate', () => { }); }); - describe('clears values.authenticator', () => { - - beforeEach(() => { - const selectAuthenticatorResponse = IdxResponseFactory.build({ - neededToProceed: [ - SelectAuthenticatorAuthenticateRemediationFactory.build({ - value: [ - AuthenticatorValueFactory.build({ - options: [ - EmailAuthenticatorOptionFactory.build(), - ] - }) - ] - }) - ], - }); - const verifyAuthenticatorResponse = IdxResponseFactory.build({ - neededToProceed: [ - VerifyEmailRemediationFactory.build() - ], - context: IdxContextFactory.build({ - authenticatorEnrollments: { - type: 'array', - value: [ - EmailAuthenticatorFactory.build() - ] - } - }) - }); - - Object.assign(testContext, { - selectAuthenticatorResponse, - verifyAuthenticatorResponse, - }); - }); - - // OKTA-609234 - specifically confirm these remediation steps - fit('clears values.authenticator after being consumed to prevent auto-remediating', async () => { - const { - authClient, - selectAuthenticatorResponse, - verifyAuthenticatorResponse - } = testContext; - - chainResponses([ - selectAuthenticatorResponse, - verifyAuthenticatorResponse - ]); - - jest.spyOn(selectAuthenticatorResponse, 'proceed'); - jest.spyOn(mocked.introspect, 'introspect').mockResolvedValue(selectAuthenticatorResponse); - - const res = await authenticate(authClient, { - username: 'foo', - authenticator: AuthenticatorKey.OKTA_EMAIL, - methodType: 'email' - }); - - expect(selectAuthenticatorResponse.proceed).toHaveBeenCalledWith('select-authenticator-authenticate', { - authenticator: { id: 'id-email' } - }); - expect(res.status).toBe(IdxStatus.PENDING); - expect(res.nextStep).toEqual({ - name: 'challenge-authenticator', - type: 'email', - authenticator: { - displayName: 'Email', - id: expect.any(String), - key: 'okta_email', - methods: [ - { type: 'email' } - ], - type: 'email', - }, - authenticatorEnrollments: [{ - id: expect.any(String), - displayName: 'Email', - key: 'okta_email', - type: 'email', - methods: [ - { type: 'email' } - ], - }], - inputs: [ - { name: 'verificationCode', type: 'string', label: 'Enter code', required: true }, - ] - }); - }); - }); - }); \ No newline at end of file diff --git a/test/spec/idx/remediators/SelectAuthenticator.ts b/test/spec/idx/remediators/SelectAuthenticator.ts index 7b3eb1ea5..24b2fad8f 100644 --- a/test/spec/idx/remediators/SelectAuthenticator.ts +++ b/test/spec/idx/remediators/SelectAuthenticator.ts @@ -19,7 +19,7 @@ describe('remediators/Base/SelectAuthenticator', () => { AuthenticatorValueFactory.build({ options: [ PhoneAuthenticatorOptionFactory.params({ - // prevent resolving of authenticator by `relatesTo` in purpose + // prevent resolving of authenticator by `relatesTo` on purpose // eslint-disable-next-line @typescript-eslint/no-explicit-any _authenticator: 'cant_be_resolved' as any }).build(), @@ -39,7 +39,7 @@ describe('remediators/Base/SelectAuthenticator', () => { describe('remediators/SelectAuthenticatorEnroll', () => { describe('canRemediate', () => { - xit('retuns false if matched authenticator is already the current one', () => { + it('retuns false if matched authenticator is already the current one', () => { const currentAuthenticator = EmailAuthenticatorFactory.build(); const remediation = SelectAuthenticatorEnrollRemediationFactory.build({ value: [ @@ -69,7 +69,7 @@ describe('remediators/SelectAuthenticatorEnroll', () => { describe('remediators/SelectAuthenticatorAuthenticate', () => { describe('canRemediate', () => { - xit('retuns false if matched authenticator is already the current one', () => { + it('retuns false if matched authenticator is already the current one', () => { const currentAuthenticatorEnrollment = PhoneAuthenticatorFactory.build(); const remediation = SelectAuthenticatorAuthenticateRemediationFactory.build({ value: [ @@ -95,14 +95,15 @@ describe('remediators/SelectAuthenticatorAuthenticate', () => { expect(r.canRemediate()).toBe(true); }); - it('returns true if matched authenticator has a resend form', () => { - const phoneAuthenticator = PhoneAuthenticatorFactory.build(); + // Fix for OKTA-646147 + it('retuns true if `options.step` is explicitly passed', () => { + const currentAuthenticatorEnrollment = PhoneAuthenticatorFactory.build(); const remediation = SelectAuthenticatorAuthenticateRemediationFactory.build({ value: [ AuthenticatorValueFactory.build({ options: [ PhoneAuthenticatorOptionFactory.params({ - _authenticator: phoneAuthenticator, + _authenticator: currentAuthenticatorEnrollment, }).build(), ] }), @@ -110,21 +111,13 @@ describe('remediators/SelectAuthenticatorAuthenticate', () => { }); const context = IdxContextFactory.build({ currentAuthenticatorEnrollment: { - value: { - ...phoneAuthenticator, - resend: ResendAuthenticatorFactory.build(), - } - }, - authenticatorEnrollments: { - value: [phoneAuthenticator] - }, - currentAuthenticator: {} + value: currentAuthenticatorEnrollment + } }); - const authenticators = [ - phoneAuthenticator, + currentAuthenticatorEnrollment, ]; - const r = new SelectAuthenticatorAuthenticate(remediation, { authenticators }); + const r = new SelectAuthenticatorAuthenticate(remediation, { authenticators }, { step: 'select-authenticator-authenticate'}); expect(r.canRemediate(context)).toBe(true); expect(r.canRemediate()).toBe(true); });