Skip to content

Commit

Permalink
Include provider name when calling logout endpoint
Browse files Browse the repository at this point in the history
This partially addresses #22440 by ensuring that session timeout
does not wind up producing a `notHandled` deauthentication result.
Note, only affects `basic` and `token` auth providers.
  • Loading branch information
jportner committed Nov 7, 2019
1 parent c20e94e commit a79bc07
Show file tree
Hide file tree
Showing 9 changed files with 70 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@
import { coreMock } from 'src/core/public/mocks';
import { SessionExpired } from './session_expired';

Object.defineProperty(window, 'sessionStorage', {
value: {
getItem: jest.fn(() => null),
},
writable: true,
});

const mockCurrentUrl = (url: string) => window.history.pushState({}, '', url);

it('redirects user to "/logout" when there is no basePath', async () => {
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/security/public/session/session_expired.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ export class SessionExpired {
`${window.location.pathname}${window.location.search}${window.location.hash}`
);
const msg = isMaximum ? 'SESSION_ENDED' : 'SESSION_EXPIRED';
const providerName = sessionStorage.getItem('session.provider');
const provider = providerName ? `&provider=${providerName}` : '';
window.location.assign(
this.basePath.prepend(`/logout?next=${encodeURIComponent(next)}&msg=${msg}`)
this.basePath.prepend(`/logout?next=${encodeURIComponent(next)}&msg=${msg}${provider}`)
);
}
}
8 changes: 4 additions & 4 deletions x-pack/plugins/security/public/session/session_timeout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export class SessionTimeout {

// subscribe to a broadcast channel for session timeout messages
// this allows us to synchronize the UX across tabs and avoid repetitive API calls
this.channel.onmessage = this.receiveMessage;
this.channel.onmessage = this.handleSessionInfo;
this.elector.awaitLeadership().then(() => {
this.updateTimeouts();
});
Expand Down Expand Up @@ -135,9 +135,10 @@ export class SessionTimeout {
return { timeout, isMaximum };
};

private receiveMessage = (sessionInfo: SessionInfo) => {
private handleSessionInfo = (sessionInfo: SessionInfo) => {
this.sessionInfo = sessionInfo;
this.updateTimeouts();
sessionStorage.setItem('session.provider', sessionInfo.provider);
};

private showWarning = () => {
Expand Down Expand Up @@ -174,8 +175,7 @@ export class SessionTimeout {
try {
const result = await this.http.fetch(path, { method: 'GET', headers });

this.sessionInfo = result;
this.updateTimeouts();
this.handleSessionInfo(result);
this.channel.postMessage(result);
} catch (err) {
// do nothing; 401 errors will be caught by the http interceptor
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ export interface SessionInfo {
now: number;
expires: number | null;
maxExpires: number | null;
provider: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,32 @@ describe('Authenticator', () => {
expect(deauthenticationResult.redirectURL).toBe('some-url');
});

it('if session does not exist but provider name is valid, returns whatever authentication provider returns.', async () => {
const request = httpServerMock.createKibanaRequest({ query: { provider: 'basic' } });
mockSessionStorage.get.mockResolvedValue(null);

mockBasicAuthenticationProvider.logout.mockResolvedValue(
DeauthenticationResult.redirectTo('some-url')
);

const deauthenticationResult = await authenticator.logout(request);

expect(mockBasicAuthenticationProvider.logout).toHaveBeenCalledTimes(1);
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
expect(deauthenticationResult.redirected()).toBe(true);
expect(deauthenticationResult.redirectURL).toBe('some-url');
});

it('returns `notHandled` if session does not exist and provider name is invalid', async () => {
const request = httpServerMock.createKibanaRequest({ query: { provider: 'foo' } });
mockSessionStorage.get.mockResolvedValue(null);

const deauthenticationResult = await authenticator.logout(request);

expect(deauthenticationResult.notHandled()).toBe(true);
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
});

it('only clears session if it belongs to not configured provider.', async () => {
const request = httpServerMock.createKibanaRequest();
const state = { authorization: 'Bearer xxx' };
Expand Down
15 changes: 15 additions & 0 deletions x-pack/plugins/security/server/authentication/authenticator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,18 @@ export class Authenticator {

const sessionStorage = this.options.sessionStorageFactory.asScoped(request);
const sessionValue = await this.getSessionValue(sessionStorage);
const providerName = this.getProviderName(request.query);
if (sessionValue) {
sessionStorage.clear();

return this.providers.get(sessionValue.provider)!.logout(request, sessionValue.state);
} else if (providerName) {
// provider name is passed in a query param and sourced from the browser's local storage;
// hence, we can't assume that this provider exists, so we have to check it
const provider = this.providers.get(providerName);
if (provider) {
return provider.logout(request, null);
}
}

// Normally when there is no active session in Kibana, `logout` method shouldn't do anything
Expand Down Expand Up @@ -450,6 +458,13 @@ export class Authenticator {
}
}

private getProviderName(query: any): string | null {
if (query && query.provider && typeof query.provider === 'string') {
return query.provider;
}
return null;
}

private calculateExpiry(
existingSession: ProviderSession | null
): { expires: number | null; maxExpires: number | null } {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -422,20 +422,17 @@ describe('TokenAuthenticationProvider', () => {
});

describe('`logout` method', () => {
it('returns `notHandled` if state is not presented.', async () => {
it('returns `redirected` if state is not presented.', async () => {
const request = httpServerMock.createKibanaRequest();
const tokenPair = { accessToken: 'foo', refreshToken: 'bar' };

let deauthenticateResult = await provider.logout(request);
expect(deauthenticateResult.notHandled()).toBe(true);
expect(deauthenticateResult.redirected()).toBe(true);

deauthenticateResult = await provider.logout(request, null);
expect(deauthenticateResult.notHandled()).toBe(true);
expect(deauthenticateResult.redirected()).toBe(true);

sinon.assert.notCalled(mockOptions.tokens.invalidate);

deauthenticateResult = await provider.logout(request, tokenPair);
expect(deauthenticateResult.notHandled()).toBe(false);
});

it('fails if `tokens.invalidate` fails', async () => {
Expand Down
20 changes: 9 additions & 11 deletions x-pack/plugins/security/server/authentication/providers/token.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,18 +120,16 @@ export class TokenAuthenticationProvider extends BaseAuthenticationProvider {
public async logout(request: KibanaRequest, state?: ProviderState | null) {
this.logger.debug(`Trying to log user out via ${request.url.path}.`);

if (!state) {
if (state) {
this.logger.debug('Token-based logout has been initiated by the user.');
try {
await this.options.tokens.invalidate(state);
} catch (err) {
this.logger.debug(`Failed invalidating user's access token: ${err.message}`);
return DeauthenticationResult.failed(err);
}
} else {
this.logger.debug('There are no access and refresh tokens to invalidate.');
return DeauthenticationResult.notHandled();
}

this.logger.debug('Token-based logout has been initiated by the user.');

try {
await this.options.tokens.invalidate(state);
} catch (err) {
this.logger.debug(`Failed invalidating user's access token: ${err.message}`);
return DeauthenticationResult.failed(err);
}

const queryString = request.url.search || `?msg=LOGGED_OUT`;
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/security/server/routes/authentication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,15 @@ function defineCommonRoutes({ router, logger, authc, basePath }: RouteDefinition
try {
const sessionInfo = await authc.sessionInfo(request);
// This is an authenticated request, so sessionInfo will always be non-null.
const { expires, maxExpires } = sessionInfo!;
const { expires, maxExpires, provider } = sessionInfo!;
const now = new Date().getTime();
// We can't rely on the client's system clock, so in addition to returning expiration timestamps, we also return
// the current server time -- that way the client can calculate the relative time to expiration.
const body: SessionInfo = {
now,
expires,
maxExpires,
provider,
};
return response.ok({ body });
} catch (err) {
Expand Down

0 comments on commit a79bc07

Please sign in to comment.