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.

Sign up for GitHub

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

Jump to bottom

(5.0) fix: handleLoginRedirect should not subscribe to auth state changes #730

Closed
wants to merge 3 commits into from
Closed

(5.0) fix: handleLoginRedirect should not subscribe to auth state changes #730

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5cf259a
fix: handleLoginRedirect should not subscribe to auth state changes
aarongranick-okta May 10, 2021
95ba515
Update CHANGELOG.md
aarongranick-okta May 12, 2021
2e329e8
Update CHANGELOG.md
aarongranick-okta May 13, 2021
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
4 changes: 2 additions & 2 deletions .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
{
"type": "node",
"request": "launch",
"name": "jest (jsdom)",
"name": "jest (browser, jsdom)",
"program": "${workspaceFolder}/node_modules/jest/bin/jest",
"args": [
"--runInBand",
Expand All @@ -21,7 +21,7 @@
{
"type": "node",
"request": "launch",
"name": "jest (node)",
"name": "jest (server, node)",
"program": "${workspaceFolder}/node_modules/jest/bin/jest",
"args": [
"--runInBand",
Expand Down
24 changes: 19 additions & 5 deletions lib/AuthStateManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export class AuthStateManager {
return this._authState;
}

updateAuthState(): void {
async updateAuthState(): Promise<AuthState> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should here be considered as a breaking change? Technically, we are exposing authStateManager from authClient (https://github.com/okta/okta-auth-js/blob/master/lib/OktaAuth.ts#L260)

Copy link
Contributor

Choose a reason for hiding this comment

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

probably not a breaking change, as app code won't change because of this signature change.

const { transformAuthState, devMode } = this._sdk.options;

const log = (status) => {
Expand All @@ -92,12 +92,22 @@ export class AuthStateManager {
devMode && log('emitted');
};

const finalPromise = (origPromise) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about making the finalPromise returns a function instead of a promise? In this way, I think we can resolve the concern about the promise may become stale.

return this._pending.updateAuthStatePromise.then(() => {
const curPromise = this._pending.updateAuthStatePromise;
if (curPromise && curPromise !== origPromise) {
return finalPromise(curPromise);
}
return this.getAuthState();
});
};

if (this._pending.updateAuthStatePromise) {
if (this._pending.canceledTimes >= MAX_PROMISE_CANCEL_TIMES) {
// stop canceling then starting a new promise
// let existing promise finish to prevent running into loops
devMode && log('terminated');
return;
return finalPromise(this._pending.updateAuthStatePromise);
} else {
this._pending.updateAuthStatePromise.cancel();
}
Expand All @@ -117,10 +127,12 @@ export class AuthStateManager {
resolve();
return;
}
// emit event and clear pending states
emitAuthStateChange(authState);
this._pending = { ...DEFAULT_PENDING };
// emit event and resolve promise
emitAuthStateChange(authState);
resolve();

// clear pending states after resolve
this._pending = { ...DEFAULT_PENDING };
};

this._sdk.isAuthenticated()
Expand Down Expand Up @@ -154,6 +166,8 @@ export class AuthStateManager {
});
/* eslint-enable complexity */
this._pending.updateAuthStatePromise = cancelablePromise;

return finalPromise(cancelablePromise);
}

subscribe(handler): void {
Expand Down
39 changes: 17 additions & 22 deletions lib/OktaAuth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {
PkceAPI,
SigninOptions,
IdxAPI,
SignoutRedirectUrlOptions
SignoutRedirectUrlOptions,
} from './types';
import {
transactionStatus,
Expand Down Expand Up @@ -554,33 +554,28 @@ class OktaAuth implements SigninAPI, SignoutAPI {
}

async handleLoginRedirect(tokens?: Tokens): Promise<void> {
const handleRedirect = async () => {
// Unsubscribe listener
this.authStateManager.unsubscribe(handleRedirect);

// Get and clear originalUri from storage
const originalUri = this.getOriginalUri();
this.removeOriginalUri();

// Redirect to originalUri
const { restoreOriginalUri } = this.options;
if (restoreOriginalUri) {
await restoreOriginalUri(this, originalUri);
} else {
window.location.replace(originalUri);
}
};

// Handle redirect after authState is updated
this.authStateManager.subscribe(handleRedirect);

// Store tokens and update AuthState by the emitted events
if (tokens) {
this.tokenManager.setTokens(tokens);
} else if (this.isLoginRedirect()) {
await this.storeTokensFromRedirect();
} else {
this.authStateManager.unsubscribe(handleRedirect);
return; // nothing to do
}

// ensure auth state has been updated
await this.authStateManager.updateAuthState();

// Get and clear originalUri from storage
const originalUri = this.getOriginalUri();
this.removeOriginalUri();

// Redirect to originalUri
const { restoreOriginalUri } = this.options;
if (restoreOriginalUri) {
await restoreOriginalUri(this, originalUri);
} else {
window.location.replace(originalUri);
}
}

Expand Down
Loading