Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: select authenticator auto selects when methodType differs #1470

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions .bacon.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,25 @@ test_suites:
criteria: MERGE
queue_name: small

- name: sample-express-embedded-auth-with-sdk-SPEC
script_path: ../okta-auth-js/scripts/samples
sort_order: '6'
timeout: '30'
script_name: e2e-express-embedded-auth-with-sdk-spec
criteria: MERGE
queue_name: small
- name: sample-express-embedded-auth-with-sdk-FEATURES
# - name: sample-express-embedded-auth-with-sdk-SPEC
# script_path: ../okta-auth-js/scripts/samples
# sort_order: '6'
# timeout: '30'
# script_name: e2e-express-embedded-auth-with-sdk-spec
# criteria: MERGE
# queue_name: small
# - name: sample-express-embedded-auth-with-sdk-FEATURES
# script_path: ../okta-auth-js/scripts/samples
# sort_order: '6'
# timeout: '30'
# script_name: e2e-express-embedded-auth-with-sdk-features
# criteria: MERGE
# queue_name: small
- name: sample-express-embedded-auth-with-sdk
script_path: ../okta-auth-js/scripts/samples
sort_order: '6'
timeout: '30'
script_name: e2e-express-embedded-auth-with-sdk-features
script_name: e2e-express-embedded-auth-with-sdk
criteria: MERGE
queue_name: small
- name: sample-express-web-no-oidc
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ This library uses semantic versioning and follows Okta's [library version policy

## Release Status

:heavy_check_mark: The current stable major version series is: `6.x`
:heavy_check_mark: The current stable major version series is: `7.x`

| Version | Status |
| ------- | -------------------------------- |
Expand Down
5 changes: 3 additions & 2 deletions lib/idx/remediate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,9 @@ export async function remediate(
`);
}

// Return next step to the caller
if (!remediator.canRemediate()) {
// always attempt remediation if `opts.step` is provided
if (!options.step && !remediator.canRemediate()) {
// Return next step to the caller
const nextStep = getNextStep(authClient, remediator, idxResponse);
return {
idxResponse,
Expand Down
20 changes: 15 additions & 5 deletions lib/idx/remediators/Base/SelectAuthenticator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ export class SelectAuthenticator<T extends SelectAuthenticatorValues = SelectAut
// Proceed with provided authenticators
const matchedOption = this.findMatchedOption(authenticators, options!);
if (matchedOption) {

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (this.options.step) {
if (this.options.step || this.values.methodType) {

What do you think about this fix?
Should cover case if user doesn't provide step, eg:

await oktaAuth.idx.authenticate({ username: '...', password: '...' })
await oktaAuth.idx.proceed({ authenticator: 'phone_number', methodType: 'sms' })
await oktaAuth.idx.proceed({ authenticator: 'phone_number', methodType: 'voice' })

return true;
}

// Don't select current authenticator (OKTA-612939)
const isCurrentAuthenticator = context?.currentAuthenticator
&& context?.currentAuthenticator.value.id === matchedOption.relatesTo?.id;
Expand All @@ -81,11 +89,13 @@ export class SelectAuthenticator<T extends SelectAuthenticatorValues = SelectAut

const { options } = remediationValue;
const selectedOption = findMatchedOption(authenticators, options);
this.selectedAuthenticator = selectedOption.relatesTo; // track the selected authenticator
this.selectedOption = selectedOption;
return {
id: selectedOption?.value.form.value.find(({ name }) => name === 'id').value
};
if (selectedOption) {
this.selectedAuthenticator = selectedOption.relatesTo; // track the selected authenticator
this.selectedOption = selectedOption;
return {
id: selectedOption?.value.form.value.find(({ name }) => name === 'id').value
};
}
}

getInputAuthenticator(remediation) {
Expand Down
9 changes: 7 additions & 2 deletions lib/idx/remediators/SelectAuthenticatorUnlockAccount.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,16 @@ export class SelectAuthenticatorUnlockAccount extends SelectAuthenticator<Select
const authenticatorMap = super.mapAuthenticator(remediationValue);
const methodTypeOption = this.selectedOption?.value.form.value.find(({ name }) => name === 'methodType');

const isSingleValue = () => {
if (Array.isArray(methodTypeOption?.options) && methodTypeOption?.options?.length === 1) {
return methodTypeOption?.options?.[0]?.value as string;
}
}

// defaults to 'manually defined' value
// 2nd: option may have pre-defined value, like stateHandle
// 3rd: if only a single OV option is available, default to that option
const methodTypeValue = this.values.methodType ||
methodTypeOption?.value as string || methodTypeOption?.options?.[0]?.value as string;
const methodTypeValue = this.values.methodType || methodTypeOption?.value as string || isSingleValue();

if (methodTypeValue) {
return {
Expand Down
2 changes: 1 addition & 1 deletion lib/oidc/introspect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ 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) {
throw new AuthSdkError('A clientId must be specified in the OktaAuth constructor to introspect a token');
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,8 @@
"samples/generated/*"
],
"nohoist": [
"**/@wdio/**"
"**/@wdio/**",
"**/@cucumber/**"
]
},
"tsd": {
Expand Down
1 change: 1 addition & 0 deletions samples/generated/react-embedded-auth-with-sdk/src/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ function App() {
history.replace('/terminal');
setTransaction(null);
} else if (status === IdxStatus.FAILURE) {
console.log('ERROR STATUS?')
history.replace('/error');
setTransaction(null);
} else if (status === IdxStatus.CANCELED) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,42 @@ const GeneralForm = () => {
}
};

const handleReselect = async e => {
e.preventDefault();
let newTransaction = transaction;

setProcessing(true);
try {
newTransaction = await oktaAuth.idx.proceed({ step: 'select-authenticator-authenticate' });
setTransaction(newTransaction);
setInputValues({});
}
catch (err) {
console.log('proceed threw');
console.log(err);
}
finally {
setProcessing(false);
}
};

const handleReselectPhone = async e => {
e.preventDefault();

setProcessing(true);
try {
const newTransaction = await oktaAuth.idx.proceed({ step: 'select-authenticator-authenticate', authenticator: 'phone_number' });
setTransaction(newTransaction);
setInputValues({});
}
catch (err) {
console.log(err);
}
finally {
setProcessing(false);
}
};

const handleSkip = async () => {
const newTransaction = await oktaAuth.idx.proceed({ skip: true });
setTransaction(newTransaction);
Expand Down Expand Up @@ -120,6 +156,12 @@ const GeneralForm = () => {
<Box paddingTop="s" paddingBottom="s">
<Button wide type="submit" disabled={processing}>Submit</Button>
</Box>
<Box paddingTop="s" paddingBottom="s">
<Button wide variant="secondary" disabled={processing} onClick={handleReselect}>Re-Select</Button>
</Box>
<Box paddingTop="s" paddingBottom="s">
<Button wide variant="secondary" disabled={processing} onClick={handleReselectPhone}>Re-Select Phone</Button>
</Box>
{canRecoverPassword && (
<Box paddingTop="s" paddingBottom="s">
<Link href="#" name="forgotPassword" onClick={handleRecoverPassword}>Forgot password</Link>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ export default function Home() {
<Box display="flex" margin="s">
<Button name="signin" variant="primary" onClick={startIdxFlow('default')}>Sign In</Button>
<Button name="signup" variant="secondary" onClick={startIdxFlow('register')}>Sign Up</Button>
<Button name="unlock" variant="secondary" onClick={startIdxFlow('unlockAccount')}>Unlock</Button>
</Box>
</Box>
)}
Expand Down
5 changes: 5 additions & 0 deletions samples/test/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@
"@typescript-eslint/no-explicit-any": 0,
"@typescript-eslint/no-var-requires": 0,
"node/no-missing-import": ["error", {
"allowModules": ["@cucumber/cucumber"],
"tryExtensions": [".js", ".ts"]
}],
"node/no-extraneous-import": ["error", {
"allowModules": ["@cucumber/cucumber"],
"tryExtensions": [".js", ".ts"]
}],
"new-cap": 0,
Expand Down
42 changes: 17 additions & 25 deletions samples/test/cucumber.wdio.conf.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ const firefoxOptions = {
const maxInstances = process.env.MAX_INSTANCES ? +process.env.MAX_INSTANCES : 1;

if (CI) {
if (process.env.CHROME_BINARY) {
chromeOptions.binary = process.env.CHROME_BINARY;
}
chromeOptions.args = chromeOptions.args.concat([
'--headless',
'--disable-gpu',
Expand All @@ -51,18 +54,6 @@ if (CI) {
]);
}

// driver version must match installed chrome version
// https://chromedriver.storage.googleapis.com/index.html

const CHROMEDRIVER_VERSION = process.env.CHROMEDRIVER_VERSION || '106.0.5249.61';
const drivers = USE_FIREFOX ? {
// Use latest geckodriver
// https://github.com/mozilla/geckodriver/releases
firefox: true,
} : {
chrome: { version: CHROMEDRIVER_VERSION },
};

// If you are using Cucumber you need to specify the location of your step definitions.
const cucumberOpts: WebdriverIO.CucumberOpts = {
// <boolean> show full backtrace for errors
Expand Down Expand Up @@ -107,9 +98,10 @@ const cucumberOpts: WebdriverIO.CucumberOpts = {
// <string> (expression) only execute the features or scenarios with
// tags matching the expression, see
// https://docs.cucumber.io/tag-expressions/
tagExpression: 'not @Pending',
// tagExpression: 'not @Pending',
tags: 'not @Pending and not @quarantined',
// <boolean> add cucumber tags to feature or scenario name
tagsInTitle: false,
// tagsInTitle: false,
// <number> timeout for step definitions
timeout: defaultTimeoutInterval,
};
Expand Down Expand Up @@ -157,7 +149,7 @@ export const config: WebdriverIO.Config = {
// and 30 processes will get spawned. The property handles how many capabilities
// from the same test should run tests.
//
maxInstances: 2,
maxInstances: 1,
//
// If you have trouble getting all important capabilities together, check out the
// Sauce Labs platform configurator - a great tool to configure your capabilities:
Expand Down Expand Up @@ -224,16 +216,16 @@ export const config: WebdriverIO.Config = {
// Services take over a specific job you don't want to take care of. They enhance
// your test setup with almost no effort. Unlike plugins, they don't add new
// commands. Instead, they hook themselves up into the test process.
services: [
['selenium-standalone', {
installArgs: {
drivers
},
args: {
drivers
}
}]
],
// services: [
// ['selenium-standalone', {
// installArgs: {
// drivers
// },
// args: {
// drivers
// }
// }]
// ],
// Framework you want to run your specs with.
// The following are supported: Mocha, Jasmine, and Cucumber
// see also: https://webdriver.io/docs/frameworks.html
Expand Down
2 changes: 2 additions & 0 deletions samples/test/features/identifier-first-auth.feature
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Feature: Login with Identifier First
And a user named "Mary"
And she has an account with "active" state in the org

@quarantined
Scenario: Mary logs in with Email with an OTP
When she clicks the "login" button
Then she is redirected to the "Login" page
Expand All @@ -32,6 +33,7 @@ Feature: Login with Identifier First
And she sees a table with her profile info
And the cell for the value of "email" is shown and contains her "email"

@quarantined
Scenario: Mary Logs in with Email Magic Link on the same Browser
Given the app has Email Verification callback uri defined
When she clicks the "login" button
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Feature: Multi-Factor Authentication with Password and Email Magic Link
And a user named "Mary"
And she has an account with "active" state in the org

@quarantined
Scenario: 2FA Login with Email Magic Link on the same browser
When she clicks the "login" button
Then she is redirected to the "Login" page
Expand Down
2 changes: 2 additions & 0 deletions samples/test/features/mfa-password-and-email.feature
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Feature: Multi-Factor Authentication with Password and Email
And a user named "Mary"
And she has an account with "active" state in the org

@quarantined
Scenario: Mary enters a wrong verification code
When she clicks the "login" button
Then she is redirected to the "Login" page
Expand All @@ -21,6 +22,7 @@ Feature: Multi-Factor Authentication with Password and Email
And she submits the form
Then the sample shows an error message "Invalid code. Try again." on the Sample App

@quarantined
Scenario: 2FA Login with Email
When she clicks the "login" button
Then she is redirected to the "Login" page
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Background:
And a user named "Mary"
And she has an account with "active" state in the org

@quarantined
Scenario: Mary resets her password
When she clicks the "login" button
Then she is redirected to the "Login" page
Expand Down
20 changes: 9 additions & 11 deletions samples/test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,16 @@
"pngjs": "6.0.0",
"jsqr": "1.4.0",
"@babel/plugin-transform-async-to-generator": "^7.5.0",
"@cucumber/cucumber": "^8.5.1",
"@wdio/cli": "^7.31.1",
"@wdio/cucumber-framework": "^7.31.1",
"@wdio/junit-reporter": "^7.31.1",
"@wdio/local-runner": "^7.31.1",
"@wdio/jasmine-framework": "^7.30.2",
"@wdio/sauce-service": "^7.31.1",
"@wdio/selenium-standalone-service": "^7.31.1",
"@wdio/spec-reporter": "^7.31.1",
"ts-node": "^9.1.1",
"@wdio/cli": "^8.20.2",
"@wdio/cucumber-framework": "^8.20.0",
"@wdio/jasmine-framework": "^8.20.0",
"@wdio/junit-reporter": "^8.20.0",
"@wdio/local-runner": "^8.20.0",
"@wdio/sauce-service": "^8.20.0",
"@wdio/spec-reporter": "^8.20.0",
"ts-node": "^10.9.1",
"wait-on": "^6.0.1",
"webdriverio": "^7.20.5",
"webdriverio": "^8.20.0",
"junit-report-merger": "^3.0.5"
}
}
8 changes: 8 additions & 0 deletions samples/test/steps/after.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,18 @@

/* 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
After({ timeout: 3 * 60 * 10000 }, async function(this: ActionContext) {
Expand Down
6 changes: 6 additions & 0 deletions samples/test/steps/before.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@
import { Before } from '@cucumber/cucumber';
import ActionContext from '../support/context';

Before('@flaky', function () {
if (process.env.SKIP_SAMPLE_FLAKY === 'true') {
return 'skipped';
}
});

Before('@smstest', function () {
if (process.env.SKIP_SMS === 'true') {
return 'skipped';
Expand Down
Loading