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

Fixes issue with expiryTime of OIDC cookie that causes refreshToken workflow to be skipped #1990

Merged
Merged
Show file tree
Hide file tree
Changes from 15 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
76 changes: 72 additions & 4 deletions server/auth/types/openid/openid_auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ interface Logger {
fatal(message: string): void;
}

const mockClient = { post: jest.fn() };

jest.mock('@hapi/wreck', () => ({
defaults: jest.fn(() => mockClient),
}));

describe('test OpenId authHeaderValue', () => {
let router: IRouter;
let core: CoreSetup;
Expand Down Expand Up @@ -208,7 +214,7 @@ describe('test OpenId authHeaderValue', () => {
expect(wreckHttpsOptions.passphrase).toBeUndefined();
});

test('Ensure expiryTime is being used to test validity of cookie', async () => {
test('Ensure accessToken expiryTime is being used to test validity of cookie', async () => {
const realDateNow = Date.now.bind(global.Date);
const dateNowStub = jest.fn(() => 0);
global.Date.now = dateNowStub;
Expand All @@ -229,22 +235,83 @@ describe('test OpenId authHeaderValue', () => {
const testCookie: SecuritySessionCookie = {
credentials: {
authHeaderValue: 'Bearer eyToken',
expiry_time: -1,
expiryTime: 200,
},
expiryTime: 2000,
expiryTime: 10000,
username: 'admin',
authType: 'openid',
};

// Credentials are valid because 0 < 200
expect(await openIdAuthentication.isValidCookie(testCookie, {})).toBe(true);
global.Date.now = realDateNow;
});

test('Ensure refreshToken workflow is called if current time is after access token expiry, but before session expiry', async () => {
const realDateNow = Date.now.bind(global.Date);
const dateNowStub = jest.fn(() => 300);
global.Date.now = dateNowStub;
const oidcConfig: unknown = {
openid: {
header: 'authorization',
scope: [],
extra_storage: {
cookie_prefix: 'testcookie',
additional_cookies: 0,
},
},
};

const openIdAuthentication = new OpenIdAuthentication(
oidcConfig as SecurityPluginConfigType,
sessionStorageFactory,
router,
esClient,
core,
logger
);
const testCookie: SecuritySessionCookie = {
credentials: {
authHeaderValue: 'Bearer eyToken',
expiryTime: 200,
refresh_token: 'refreshToken',
},
expiryTime: 10000,
username: 'admin',
authType: 'openid',
};

const mockRequestPayload = JSON.stringify({
grant_type: 'refresh_token',
client_id: 'clientId',
client_secret: 'clientSecret',
refresh_token: 'refreshToken',
});
const mockResponsePayload = JSON.stringify({
id_token: '.eyJleHAiOiIwLjUifQ.', // Translates to {"exp":"0.5"}
access_token: 'accessToken',
refresh_token: 'refreshToken',
});
mockClient.post.mockResolvedValue({
res: { statusCode: 200 },
payload: mockResponsePayload,
});

cwperks marked this conversation as resolved.
Show resolved Hide resolved
expect(await openIdAuthentication.isValidCookie(testCookie, {})).toBe(true);
global.Date.now = realDateNow;
});

test('getKeepAliveExpiry', () => {
const realDateNow = Date.now.bind(global.Date);
const dateNowStub = jest.fn(() => 300);
global.Date.now = dateNowStub;
const oidcConfig: unknown = {
openid: {
scope: [],
},
session: {
ttl: 3600,
},
};

const openIdAuthentication = new OpenIdAuthentication(
Expand All @@ -262,6 +329,7 @@ describe('test OpenId authHeaderValue', () => {
expiryTime: 1000,
};

expect(openIdAuthentication.getKeepAliveExpiry(testCookie, {})).toBe(1000);
expect(openIdAuthentication.getKeepAliveExpiry(testCookie, {})).toBe(3900);
global.Date.now = realDateNow;
});
});
7 changes: 3 additions & 4 deletions server/auth/types/openid/openid_auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,12 +250,11 @@ export class OpenIdAuthentication extends AuthenticationType {
};
}

// OIDC expiry time is set by the IDP and refreshed via refreshTokens
getKeepAliveExpiry(
cookie: SecuritySessionCookie,
request: OpenSearchDashboardsRequest<unknown, unknown, unknown, any>
): number {
return cookie.expiryTime!;
return Date.now() + this.config.session.ttl;
}

// TODO: Add token expiration check here
Expand All @@ -272,7 +271,7 @@ export class OpenIdAuthentication extends AuthenticationType {
return false;
}

if (cookie.expiryTime > Date.now()) {
if (cookie.credentials.expiryTime > Date.now()) {
return true;
}

Expand All @@ -296,8 +295,8 @@ export class OpenIdAuthentication extends AuthenticationType {
cookie.credentials = {
authHeaderValueExtra: true,
refresh_token: refreshTokenResponse.refreshToken,
expiryTime: getExpirationDate(refreshTokenResponse),
};
cookie.expiryTime = getExpirationDate(refreshTokenResponse);

setExtraAuthStorage(
request,
Expand Down
3 changes: 2 additions & 1 deletion server/auth/types/openid/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,10 @@ export class OpenIdAuthRoutes {
username: user.username,
credentials: {
authHeaderValueExtra: true,
expiryTime: getExpirationDate(tokenResponse),
},
authType: AuthType.OPEN_ID,
expiryTime: getExpirationDate(tokenResponse),
expiryTime: Date.now() + this.config.session.ttl,
};
if (this.config.openid?.refresh_tokens && tokenResponse.refreshToken) {
Object.assign(sessionStorage.credentials, {
Expand Down
Loading