-
Notifications
You must be signed in to change notification settings - Fork 262
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
.signOut improvements #1439
.signOut improvements #1439
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -261,7 +261,7 @@ export function mixinOAuth | |
if (!idToken) { | ||
return ''; | ||
} | ||
if (!postLogoutRedirectUri) { | ||
if (postLogoutRedirectUri === undefined) { | ||
postLogoutRedirectUri = this.options.postLogoutRedirectUri; | ||
} | ||
|
||
|
@@ -285,16 +285,23 @@ export function mixinOAuth | |
options = Object.assign({}, options); | ||
|
||
// postLogoutRedirectUri must be whitelisted in Okta Admin UI | ||
var defaultUri = window.location.origin; | ||
var currentUri = window.location.href; | ||
var postLogoutRedirectUri = options.postLogoutRedirectUri | ||
const defaultUri = window.location.origin; | ||
const currentUri = window.location.href; | ||
// Fix for issue/1410 - allow for no postLogoutRedirectUri to be passed, resulting in /logout default behavior | ||
// "If no Okta session exists, this endpoint has no effect and the browser is redirected immediately to the | ||
// Okta sign-in page or the post_logout_redirect_uri (if specified)." | ||
// - https://developer.okta.com/docs/reference/api/oidc/#logout | ||
const postLogoutRedirectUri = options.postLogoutRedirectUri === null ? null : | ||
(options.postLogoutRedirectUri | ||
|| this.options.postLogoutRedirectUri | ||
|| defaultUri; | ||
|| defaultUri); | ||
Comment on lines
+295
to
+297
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be careful -- |
||
const state = options?.state; | ||
|
||
|
||
var accessToken = options.accessToken; | ||
var refreshToken = options.refreshToken; | ||
var revokeAccessToken = options.revokeAccessToken !== false; | ||
var revokeRefreshToken = options.revokeRefreshToken !== false; | ||
let accessToken = options.accessToken; | ||
let refreshToken = options.refreshToken; | ||
const revokeAccessToken = options.revokeAccessToken !== false; | ||
const revokeRefreshToken = options.revokeRefreshToken !== false; | ||
|
||
if (revokeRefreshToken && typeof refreshToken === 'undefined') { | ||
refreshToken = this.tokenManager.getTokensSync().refreshToken as RefreshToken; | ||
|
@@ -321,15 +328,18 @@ export function mixinOAuth | |
// Fallback to XHR signOut, then simulate a redirect to the post logout uri | ||
if (!logoutUri) { | ||
// local tokens are cleared once session is closed | ||
return this.closeSession() // can throw if the user cannot be signed out | ||
.then(function(sessionClosed) { | ||
if (postLogoutRedirectUri === currentUri) { | ||
window.location.reload(); // force a hard reload if URI is not changing | ||
} else { | ||
window.location.assign(postLogoutRedirectUri); | ||
} | ||
return sessionClosed; | ||
}); | ||
const sessionClosed = await this.closeSession(); // can throw if the user cannot be signed out | ||
const redirectUri = new URL(postLogoutRedirectUri || defaultUri); // during fallback, redirectUri cannot be null | ||
if (state) { | ||
redirectUri.searchParams.append('state', state); | ||
} | ||
if (postLogoutRedirectUri === currentUri) { | ||
// window.location.reload(); // force a hard reload if URI is not changing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this commented code be removed? |
||
window.location.href = redirectUri.href; | ||
} else { | ||
window.location.assign(redirectUri.href); | ||
} | ||
return sessionClosed; | ||
} else { | ||
if (options.clearTokensBeforeRedirect) { | ||
// Clear all local tokens | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,7 +101,7 @@ export interface IsAuthenticatedOptions { | |
} | ||
|
||
export interface SignoutRedirectUrlOptions { | ||
postLogoutRedirectUri?: string; | ||
postLogoutRedirectUri?: string | null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is null needed here as it's already optional? |
||
idToken?: IDToken; | ||
state?: string; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,8 +46,8 @@ describe('OktaAuth (browser)', function() { | |
delete (global.window as any).location; | ||
global.window.location = { | ||
protocol: 'https:', | ||
hostname: 'somesite.local', | ||
href: 'https://somesite.local', | ||
hostname: 'somesite.local/', | ||
href: 'https://somesite.local/', | ||
replace: jest.fn() | ||
} as unknown as Location; | ||
|
||
|
@@ -197,7 +197,7 @@ describe('OktaAuth (browser)', function() { | |
let encodedOrigin; | ||
|
||
beforeEach(function() { | ||
origin = 'https://somesite.local'; | ||
origin = 'https://somesite.local/'; | ||
href = `${origin}/some-route`; | ||
Comment on lines
+200
to
201
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. idk if this matters but |
||
encodedOrigin = encodeURIComponent(origin); | ||
Object.assign(global.window.location, { | ||
|
@@ -233,7 +233,7 @@ describe('OktaAuth (browser)', function() { | |
expect(auth.revokeAccessToken).toHaveBeenCalledWith(accessToken); | ||
expect(auth.closeSession).not.toHaveBeenCalled(); | ||
expect(window.location.assign).toHaveBeenCalledWith(`${issuer}/oauth2/v1/logout?id_token_hint=${idToken.idToken}&post_logout_redirect_uri=${encodedOrigin}`); | ||
expect(signOutSuccess).toBeTruthy(); | ||
expect(signOutSuccess).toBe(true); | ||
}); | ||
}); | ||
|
||
|
@@ -247,7 +247,7 @@ describe('OktaAuth (browser)', function() { | |
expect(auth.revokeRefreshToken).toHaveBeenCalledWith(refreshToken); | ||
expect(auth.closeSession).not.toHaveBeenCalled(); | ||
expect(window.location.assign).toHaveBeenCalledWith(`${issuer}/oauth2/v1/logout?id_token_hint=${idToken.idToken}&post_logout_redirect_uri=${encodedOrigin}`); | ||
expect(signOutSuccess).toBeTruthy(); | ||
expect(signOutSuccess).toBe(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👌 |
||
}); | ||
}); | ||
|
||
|
@@ -274,7 +274,7 @@ describe('OktaAuth (browser)', function() { | |
|
||
describe('postLogoutRedirectUri', function() { | ||
it('can be set by config', function() { | ||
const postLogoutRedirectUri = 'http://someother'; | ||
const postLogoutRedirectUri = 'http://someother.local/'; | ||
const encodedUri = encodeURIComponent(postLogoutRedirectUri); | ||
auth = new OktaAuth({ | ||
pkce: false, | ||
|
@@ -288,13 +288,20 @@ describe('OktaAuth (browser)', function() { | |
}); | ||
}); | ||
it('can be passed as an option', function() { | ||
const postLogoutRedirectUri = 'http://someother'; | ||
const postLogoutRedirectUri = 'http://someother.local/'; | ||
const encodedUri = encodeURIComponent(postLogoutRedirectUri); | ||
return auth.signOut({ postLogoutRedirectUri }) | ||
.then(function() { | ||
expect(window.location.assign).toHaveBeenCalledWith(`${issuer}/oauth2/v1/logout?id_token_hint=${idToken.idToken}&post_logout_redirect_uri=${encodedUri}`); | ||
}); | ||
}); | ||
|
||
it('can be set to null', function() { | ||
return auth.signOut({ postLogoutRedirectUri: null }) | ||
.then(function() { | ||
expect(window.location.assign).toHaveBeenCalledWith(`${issuer}/oauth2/v1/logout?id_token_hint=${idToken.idToken}`); | ||
}); | ||
}); | ||
}); | ||
|
||
it('Can pass a "state" option', function() { | ||
|
@@ -377,7 +384,7 @@ describe('OktaAuth (browser)', function() { | |
expect(auth.revokeAccessToken).toHaveBeenCalledWith(accessToken); | ||
expect(auth.closeSession).toHaveBeenCalled(); | ||
expect(window.location.assign).toHaveBeenCalledWith(window.location.origin); | ||
expect(signOutSuccess).toBeTruthy(); | ||
expect(signOutSuccess).toBe(true); | ||
}); | ||
}); | ||
|
||
|
@@ -389,8 +396,8 @@ describe('OktaAuth (browser)', function() { | |
expect(auth.tokenManager.getTokensSync).toHaveBeenCalledTimes(4); | ||
expect(auth.revokeAccessToken).toHaveBeenCalledWith(accessToken); | ||
expect(auth.closeSession).toHaveBeenCalled(); | ||
expect(window.location.reload).toHaveBeenCalled(); | ||
expect(signOutSuccess).toBeTruthy(); | ||
// expect(window.location.reload).toHaveBeenCalled(); // TODO: confirm href? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👀 |
||
expect(signOutSuccess).toBe(true); | ||
}); | ||
}); | ||
|
||
|
@@ -411,17 +418,17 @@ describe('OktaAuth (browser)', function() { | |
}); | ||
|
||
it('with postLogoutRedirectUri: will call window.location.assign', function() { | ||
const postLogoutRedirectUri = 'http://someother'; | ||
const postLogoutRedirectUri = 'http://someother.local//'; | ||
spyOn(auth, 'closeSession').and.returnValue(Promise.resolve(true)); | ||
return auth.signOut({ postLogoutRedirectUri }) | ||
.then(function(signOutSuccess) { | ||
expect(window.location.assign).toHaveBeenCalledWith(postLogoutRedirectUri); | ||
expect(signOutSuccess).toBeTruthy(); | ||
expect(signOutSuccess).toBe(true); | ||
}); | ||
}); | ||
|
||
it('with postLogoutRedirectUri: will throw exceptions from closeSession and not call window.location.assign', function() { | ||
const postLogoutRedirectUri = 'http://someother'; | ||
const postLogoutRedirectUri = 'http://someother.local/'; | ||
const testError = new Error('test error'); | ||
spyOn(auth, 'closeSession').and.callFake(function() { | ||
return Promise.reject(testError); | ||
|
@@ -438,22 +445,48 @@ describe('OktaAuth (browser)', function() { | |
}); | ||
|
||
it('if closeSession resolves with true, signOut resolves with true', function() { | ||
const postLogoutRedirectUri = 'http://someother'; | ||
const postLogoutRedirectUri = 'http://someother.local/'; | ||
spyOn(auth, 'closeSession').and.returnValue(Promise.resolve(true)); | ||
return auth.signOut({ postLogoutRedirectUri }) | ||
.then(function(signOutSuccess) { | ||
expect(window.location.assign).toHaveBeenCalledWith(postLogoutRedirectUri); | ||
expect(signOutSuccess).toBeTruthy(); | ||
expect(signOutSuccess).toBe(true); | ||
}); | ||
}); | ||
|
||
it('if closeSession resolves with false (session does not exist), signOut resolves with false', function() { | ||
const postLogoutRedirectUri = 'http://someother'; | ||
const postLogoutRedirectUri = 'http://someother.local/'; | ||
spyOn(auth, 'closeSession').and.returnValue(Promise.resolve(false)); | ||
return auth.signOut({ postLogoutRedirectUri }) | ||
.then(function(signOutSuccess) { | ||
expect(window.location.assign).toHaveBeenCalledWith(postLogoutRedirectUri); | ||
expect(signOutSuccess).toBeFalsy(); | ||
expect(signOutSuccess).toBe(false); | ||
}); | ||
}); | ||
|
||
it('will return state as a query param of the postLogoutRedirectUri after closeSession (true)', function () { | ||
const postLogoutRedirectUri = 'http://someother.local/'; | ||
const state = 'somestatevalue'; | ||
spyOn(auth, 'closeSession').and.returnValue(Promise.resolve(true)); | ||
return auth.signOut({ postLogoutRedirectUri, state }) | ||
.then(function(signOutSuccess) { | ||
const expectedUri = new URL(postLogoutRedirectUri); | ||
expectedUri.searchParams.append('state', state); | ||
expect(window.location.assign).toHaveBeenCalledWith(expectedUri.href); | ||
expect(signOutSuccess).toBe(true); | ||
}); | ||
}); | ||
|
||
it('will return state as a query param of the postLogoutRedirectUri after closeSession (false)', function () { | ||
const postLogoutRedirectUri = 'http://someother.local/'; | ||
const state = 'somestatevalue'; | ||
spyOn(auth, 'closeSession').and.returnValue(Promise.resolve(false)); | ||
return auth.signOut({ postLogoutRedirectUri, state }) | ||
.then(function(signOutSuccess) { | ||
const expectedUri = new URL(postLogoutRedirectUri); | ||
expectedUri.searchParams.append('state', state); | ||
expect(window.location.assign).toHaveBeenCalledWith(expectedUri.href); | ||
expect(signOutSuccess).toBe(false); | ||
}); | ||
}); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add comment why
undefined
is handled hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not likely a real issue but I prefer to check
typeof foo === 'undefined'