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: authn polling on iOS18 devices #1551

Merged
merged 9 commits into from
Dec 5, 2024
Merged
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
65 changes: 63 additions & 2 deletions lib/authn/util/poll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import AuthSdkError from '../../errors/AuthSdkError';
import AuthPollStopError from '../../errors/AuthPollStopError';
import { AuthnTransactionState } from '../types';
import { getStateToken } from './stateToken';
import { isIOS } from '../../features';

interface PollOptions {
delay?: number;
Expand Down Expand Up @@ -82,6 +83,48 @@ export function getPollFn(sdk, res: AuthnTransactionState, ref) {
});
}

const delayNextPoll = (ms) => {
// no need for extra logic in non-iOS environments, just continue polling
if (!isIOS()) {
return delayFn(ms);
}

let timeoutId: ReturnType<typeof setTimeout>;
const cancelableDelay = () => {
return new Promise((resolve) => {
timeoutId = setTimeout(resolve, ms);
});
};

const delayForFocus = () => {
let pageVisibilityHandler;
return new Promise<void>((resolve) => {
let pageDidHide = false;
pageVisibilityHandler = () => {
if (document.hidden) {
jaredperreault-okta marked this conversation as resolved.
Show resolved Hide resolved
clearTimeout(timeoutId);
pageDidHide = true;
}
else if (pageDidHide) {
Comment on lines +106 to +108
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it important for us to track this pageDidHide state?

Copy link
Contributor Author

@jaredperreault-okta jaredperreault-okta Dec 4, 2024

Choose a reason for hiding this comment

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

It was an attempt to prevent changing behavior for authn polling flows when it isn't necessary. The code probably isn't necessary as the Promise.race would have already resolve if pageDidHide is false because the timeout wouldn't have been cleared.

resolve();
}
};

document.addEventListener('visibilitychange', pageVisibilityHandler);
})
.then(() => {
document.removeEventListener('visibilitychange', pageVisibilityHandler);
});
};

return Promise.race([
// this function will never resolve if the page changes to hidden because the timeout gets cleared
cancelableDelay(),
// this function won't resolve until the page becomes visible after being hidden
delayForFocus(),
]);
};

ref.isPolling = true;

var retryCount = 0;
Expand All @@ -90,6 +133,24 @@ export function getPollFn(sdk, res: AuthnTransactionState, ref) {
if (!ref.isPolling) {
return Promise.reject(new AuthPollStopError());
}

// don't trigger polling request if page is hidden wait until window is visible again
if (isIOS() && document.hidden) {
let handler;
return new Promise<void>((resolve) => {
handler = () => {
if (!document.hidden) {
resolve();
}
};
document.addEventListener('visibilitychange', handler);
})
.then(() => {
document.removeEventListener('visibilitychange', handler);
return recursivePoll();
});
}

return pollFn()
.then(function (pollRes) {
// Reset our retry counter on success
Expand All @@ -108,7 +169,7 @@ export function getPollFn(sdk, res: AuthnTransactionState, ref) {
}

// Continue poll
return delayFn(delay).then(recursivePoll);
return delayNextPoll(delay).then(recursivePoll);

} else {
// Any non-waiting result, even if polling was stopped
Expand All @@ -124,7 +185,7 @@ export function getPollFn(sdk, res: AuthnTransactionState, ref) {
retryCount <= 4) {
var delayLength = Math.pow(2, retryCount) * 1000;
retryCount++;
return delayFn(delayLength)
return delayNextPoll(delayLength)
.then(recursivePoll);
}
throw err;
Expand Down
1 change: 1 addition & 0 deletions lib/base/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export interface FeaturesAPI {
isPKCESupported(): boolean;
isIE11OrLess(): boolean;
isDPoPSupported(): boolean;
isIOS(): boolean;
}


Expand Down
7 changes: 7 additions & 0 deletions lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,10 @@ export function isDPoPSupported () {
hasTextEncoder() &&
isWebCryptoSubtleSupported();
}

export function isIOS () {
jaredperreault-okta marked this conversation as resolved.
Show resolved Hide resolved
// iOS detection from: http://stackoverflow.com/a/9039885/177710
return isBrowser() && typeof navigator !== 'undefined' && typeof navigator.userAgent !== 'undefined' &&
// @ts-expect-error - MSStream is not in `window` type, unsurprisingly
(/iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream);
}
224 changes: 223 additions & 1 deletion test/spec/authn/mfa-challenge.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,40 @@
* See the License for the specific language governing permissions and limitations under the License.
*/

/* global document */

jest.mock('../../../lib/util/misc', () => {
jest.mock('lib/util', () => {
const actual = jest.requireActual('../../../lib/util');
return {
...actual,
delay: () => { return Promise.resolve(); }
};
});

jest.mock('lib/http', () => {
const actual = jest.requireActual('../../../lib/http');
return {
...actual,
post: actual.post
};
});
jaredperreault-okta marked this conversation as resolved.
Show resolved Hide resolved

jest.mock('lib/features', () => {
const actual = jest.requireActual('../../../lib/features');
return {
...actual,
isIOS: () => false
};
});
import OktaAuth from '@okta/okta-auth-js';
import util from '@okta/test.support/util';
import { setImmediate } from 'timers';

const mocked = {
http: require('../../../lib/http'),
util: require('../../../lib/util'),
features: require('../../../lib/features')
};

describe('MFA_CHALLENGE', function () {

Expand Down Expand Up @@ -1522,6 +1549,201 @@ describe('MFA_CHALLENGE', function () {
expect(err.errorCauses).toBeUndefined();
}
});

// OKTA-823470: iOS18 polling issue
// NOTE: only run these tests in browser environments
// eslint-disable-next-line no-extra-boolean-cast
(!!global.document ? describe : describe.skip)('iOS18 polling', () => {
const togglePageVisibility = () => {
document.hidden = !document.hidden;
document.dispatchEvent(new Event('visibilitychange'));
};

// see https://stackoverflow.com/a/52196951 for more info about jest/promises/timers
const advanceTestTimers = async () => {
jest.runOnlyPendingTimers();
// flushes promise queue
return new Promise(resolve => setImmediate(resolve));
};

const context = {};

beforeEach(async () => {
jest.useFakeTimers();
document.hidden = false;

// delay must be mocked (essentially to original implementation) because other tests
// mock this function to remove any timer delays
jest.spyOn(mocked.util, 'delay').mockImplementation((ms) => {
return new Promise((resolve) => {
setTimeout(resolve, ms);
});
});

// mocks iOS environment
jest.spyOn(mocked.features, 'isIOS').mockReturnValue(true);

const { response: mfaPush } = await util.generateXHRPair({
uri: 'https://auth-js-test.okta.com'
}, 'mfa-challenge-push', 'https://auth-js-test.okta.com');

const { response: success } = await util.generateXHRPair({
uri: 'https://auth-js-test.okta.com'
}, 'success', 'https://auth-js-test.okta.com');

// mocks flow of wait, wait, wait, success
context.httpSpy = jest.spyOn(mocked.http, 'post')
.mockResolvedValueOnce(mfaPush.response)
.mockResolvedValueOnce(mfaPush.response)
.mockResolvedValueOnce(mfaPush.response)
.mockResolvedValueOnce(success.response);


const oktaAuth = new OktaAuth({
issuer: 'https://auth-js-test.okta.com'
});

context.transaction = oktaAuth.tx.createTransaction(mfaPush.response);
});

afterEach(() => {
jest.runOnlyPendingTimers();
jest.useRealTimers();
});

it('should proceed with flow as normal if document is never hidden', async () => {
const { httpSpy, transaction } = context;
expect(document.hidden).toBe(false);

let count = 0;
const pollPromise = transaction.poll({
delay: 2000,
transactionCallBack: () => {
count += 1;
}
});

for (let i=0; i<4; i++) {
await advanceTestTimers();
}

const result = await pollPromise;

expect(count).toEqual(3);
expect(httpSpy).toHaveBeenCalledTimes(4);
expect(result.status).toEqual('SUCCESS');
});

it('should not proceed with flow if document is hidden', async () => {
const { httpSpy, transaction } = context;
expect(document.hidden).toBe(false);

togglePageVisibility();

let count = 0;
const pollPromise = transaction.poll({
delay: 2000,
transactionCallBack: () => {
count += 1;
}
});

// advance the timers so the flow would have succeed in normal circumstances
for (let i=0; i<4; i++) {
await advanceTestTimers();
}

// ensure flow did not advance, awaits document focus to return
expect(count).toEqual(0);

togglePageVisibility();
for (let i=0; i<4; i++) {
await advanceTestTimers();
}

const result = await pollPromise;

expect(count).toEqual(3);
expect(httpSpy).toHaveBeenCalledTimes(4);
expect(result.status).toEqual('SUCCESS');
});

it('should pause flow is document is hidden amidst polling', async () => {
const { httpSpy, transaction } = context;
expect(document.hidden).toBe(false);

let count = 0;
const pollPromise = transaction.poll({
delay: 2000,
transactionCallBack: () => {
count += 1;
}
});

// advance the timers so the flow would have succeed in normal circumstances
for (let i=0; i<4; i++) {
await advanceTestTimers();
if (i == 1) {
// hide document in middle of flow
togglePageVisibility();
}
}

// ensure flow pauses, awaits document focus to return
expect(count).toEqual(2);

togglePageVisibility();
for (let i=0; i<2; i++) {
await advanceTestTimers();
}

const result = await pollPromise;

expect(count).toEqual(3);
expect(httpSpy).toHaveBeenCalledTimes(4);
expect(result.status).toEqual('SUCCESS');
});

it('should handle document visibility being toggled consistently', async () => {
const { httpSpy, transaction } = context;
expect(document.hidden).toBe(false);

let count = 0;
const pollPromise = transaction.poll({
delay: 2000,
transactionCallBack: () => {
count += 1;
}
});

for (let i=0; i<8; i++) {
if (i % 2 === 0) {
expect(document.hidden).toBe(false);
}
else {
expect(document.hidden).toBe(true);
}

await advanceTestTimers();
togglePageVisibility();

if (i % 2 === 0) {
expect(document.hidden).toBe(true);
}
else {
expect(document.hidden).toBe(false);
}
}

expect(document.hidden).toBe(false);

const result = await pollPromise;

expect(count).toEqual(3);
expect(httpSpy).toHaveBeenCalledTimes(4);
expect(result.status).toEqual('SUCCESS');
});
});
jaredperreault-okta marked this conversation as resolved.
Show resolved Hide resolved
});

describe('trans.prev', function () {
Expand Down
26 changes: 26 additions & 0 deletions test/spec/features/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,30 @@ describe('features (browser)', function() {
expect(OktaAuth.features.isIE11OrLess()).toBe(true);
});
});

describe('isIOS', () => {
it('can succeed', () => {
const iOSAgents = [
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_7 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/92.0.4515.90 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_2 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0.1 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_7 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/92.0.4515.90 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_7_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.1.2 Mobile/15E148 Safari/604.1'
];

for (let userAgent of iOSAgents) {
jest.spyOn(global.navigator, 'userAgent', 'get').mockReturnValue(userAgent);
expect(OktaAuth.features.isIOS()).toBe(true);
}
});

it('returns false if navigator is unavailable', () => {
jest.spyOn(global, 'navigator', 'get').mockReturnValue(undefined as never);
expect(OktaAuth.features.isIOS()).toBe(false);
});

it('returns false if userAgent is unavailable', () => {
jest.spyOn(global.navigator, 'userAgent', 'get').mockReturnValue(undefined as never);
expect(OktaAuth.features.isIOS()).toBe(false);
});
});
});
Loading