Skip to content

Use refresh token for any access token error with 401 status code and re-authenticate user for any refresh token error with 400 status code. #33777

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

Merged
azasypkin merged 4 commits into from
Mar 27, 2019
Merged

Use refresh token for any access token error with 401 status code and re-authenticate user for any refresh token error with 400 status code. #33777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
49b52bd
Use refresh token for any access token error with 401 status code and…
azasypkin Mar 25, 2019
8830007
Switch from `expect.js` to `@kbn/expect`.
azasypkin Mar 26, 2019
62299bb
Merge branch 'master' into issue-33646-400-and-401
azasypkin Mar 27, 2019
60e82f9
Review#1: repurpose redundant tests to test token failures with 500 c…
azasypkin Mar 27, 2019
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
2 changes: 1 addition & 1 deletion x-pack/plugins/security/server/lib/authentication/__tests__/authenticator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ describe('Authenticator', () => {
session.clear.resolves();

cluster.callWithRequest
.withArgs(request).rejects({ body: { error: { reason: 'token expired' } } });
.withArgs(request).rejects({ statusCode: 401 });

cluster.callWithInternalUser.withArgs('shield.getAccessToken').rejects(
Boom.badRequest('refresh token expired')
Expand Down
85 changes: 47 additions & 38 deletions x-pack/plugins/security/server/lib/authentication/providers/__tests__/token.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ describe('TokenAuthenticationProvider', () => {

callWithRequest
.withArgs(sinon.match({ headers: { authorization: 'Bearer foo' } }), 'shield.authenticate')
.returns(Promise.reject({ body: { error: { reason: 'token expired' } } }));
.rejects({ statusCode: 401 });

callWithInternalUser
.withArgs('shield.getAccessToken', { body: { grant_type: 'refresh_token', refresh_token: 'bar' } })
Expand Down Expand Up @@ -176,26 +176,6 @@ describe('TokenAuthenticationProvider', () => {
expect(authenticationResult.notHandled()).to.be(true);
});

it('fails if state contains invalid credentials.', async () => {
Copy link
Member Author

@azasypkin azasypkin Mar 27, 2019

Choose a reason for hiding this comment

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

note: we had a similar one below, that I've repurposed to test 500 errors.

const request = requestFixture();
const accessToken = 'foo';
const authorization = `Bearer ${accessToken}`;

const authenticationError = new Error('Forbidden');
callWithRequest
.withArgs(sinon.match({ headers: { authorization } }), 'shield.authenticate')
.returns(Promise.reject(authenticationError));

const authenticationResult = await provider.authenticate(request, { accessToken });

expect(request.headers).to.not.have.property('authorization');
expect(authenticationResult.failed()).to.be(true);
expect(authenticationResult.user).to.be.eql(undefined);
expect(authenticationResult.state).to.be.eql(undefined);
expect(authenticationResult.error).to.be.eql(authenticationError);
sinon.assert.calledOnce(callWithRequest);
});

it('authenticates only via `authorization` header even if state is available.', async () => {
const accessToken = 'foo';
const authorization = `Bearer ${accessToken}`;
Expand Down Expand Up @@ -263,14 +243,14 @@ describe('TokenAuthenticationProvider', () => {
expect(authenticationResult.error).to.be.eql(authenticationError);
});

it('fails when header contains a rejected token', async () => {
it('fails if authentication with token from header fails with unknown error', async () => {
const authorization = `Bearer foo`;
const request = requestFixture({ headers: { authorization } });

const authenticationError = new Error('Forbidden');
const authenticationError = new errors.InternalServerError('something went wrong');
callWithRequest
.withArgs(request, 'shield.authenticate')
.returns(Promise.reject(authenticationError));
.rejects(authenticationError);

const authenticationResult = await provider.authenticate(request);

Expand All @@ -282,14 +262,14 @@ describe('TokenAuthenticationProvider', () => {
expect(authenticationResult.error).to.be.eql(authenticationError);
});

it('fails when session contains a rejected token', async () => {
it('fails if authentication with token from state fails with unknown error.', async () => {
const accessToken = 'foo';
const request = requestFixture();

const authenticationError = new Error('Forbidden');
const authenticationError = new errors.InternalServerError('something went wrong');
callWithRequest
.withArgs(request, 'shield.authenticate')
.returns(Promise.reject(authenticationError));
.withArgs(sinon.match({ headers: { authorization: `Bearer ${accessToken}` } }), 'shield.authenticate')
.rejects(authenticationError);

const authenticationResult = await provider.authenticate(request, { accessToken });

Expand All @@ -302,17 +282,17 @@ describe('TokenAuthenticationProvider', () => {
expect(authenticationResult.error).to.be.eql(authenticationError);
});

it('fails if token refresh is rejected', async () => {
it('fails if token refresh is rejected with unknown error', async () => {
const request = requestFixture();

callWithRequest
.withArgs(sinon.match({ headers: { authorization: 'Bearer foo' } }), 'shield.authenticate')
.returns(Promise.reject({ body: { error: { reason: 'token expired' } } }));
.rejects({ statusCode: 401 });

const authenticationError = new Error('failed to refresh token');
const refreshError = new errors.InternalServerError('failed to refresh token');
callWithInternalUser
.withArgs('shield.getAccessToken', { body: { grant_type: 'refresh_token', refresh_token: 'bar' } })
.returns(Promise.reject(authenticationError));
.rejects(refreshError);

const accessToken = 'foo';
const refreshToken = 'bar';
Expand All @@ -325,15 +305,44 @@ describe('TokenAuthenticationProvider', () => {
expect(authenticationResult.failed()).to.be(true);
expect(authenticationResult.user).to.be.eql(undefined);
expect(authenticationResult.state).to.be.eql(undefined);
expect(authenticationResult.error).to.be.eql(authenticationError);
expect(authenticationResult.error).to.be.eql(refreshError);
});

it('redirects non-AJAX requests to /login and clears session if token document is missing', async () => {
const request = requestFixture({ path: '/some-path' });

callWithRequest
.withArgs(sinon.match({ headers: { authorization: 'Bearer foo' } }), 'shield.authenticate')
.rejects({
statusCode: 500,
body: { error: { reason: 'token document is missing and must be present' } },
});

callWithInternalUser
.withArgs('shield.getAccessToken', { body: { grant_type: 'refresh_token', refresh_token: 'bar' } })
.rejects(new errors.BadRequest('failed to refresh token'));

const accessToken = 'foo';
const refreshToken = 'bar';
const authenticationResult = await provider.authenticate(request, { accessToken, refreshToken });

sinon.assert.calledOnce(callWithRequest);
sinon.assert.calledOnce(callWithInternalUser);

expect(request.headers).to.not.have.property('authorization');
expect(authenticationResult.redirected()).to.be(true);
expect(authenticationResult.redirectURL).to.be('/base-path/login?next=%2Fsome-path');
expect(authenticationResult.user).to.be.eql(undefined);
expect(authenticationResult.state).to.be.eql(null);
expect(authenticationResult.error).to.be.eql(undefined);
});

it('redirects non-AJAX requests to /login and clears session if token refresh fails with 400 error', async () => {
const request = requestFixture({ path: '/some-path' });

callWithRequest
.withArgs(sinon.match({ headers: { authorization: 'Bearer foo' } }), 'shield.authenticate')
.rejects({ body: { error: { reason: 'token expired' } } });
.rejects({ statusCode: 401 });

callWithInternalUser
.withArgs('shield.getAccessToken', { body: { grant_type: 'refresh_token', refresh_token: 'bar' } })
Expand All @@ -359,7 +368,7 @@ describe('TokenAuthenticationProvider', () => {

callWithRequest
.withArgs(sinon.match({ headers: { authorization: 'Bearer foo' } }), 'shield.authenticate')
.rejects({ body: { error: { reason: 'token expired' } } });
.rejects({ statusCode: 401 });

const authenticationError = new errors.BadRequest('failed to refresh token');
callWithInternalUser
Expand All @@ -385,16 +394,16 @@ describe('TokenAuthenticationProvider', () => {

callWithRequest
.withArgs(sinon.match({ headers: { authorization: 'Bearer foo' } }), 'shield.authenticate')
.returns(Promise.reject({ body: { error: { reason: 'token expired' } } }));
.rejects({ statusCode: 401 });

callWithInternalUser
.withArgs('shield.getAccessToken', { body: { grant_type: 'refresh_token', refresh_token: 'bar' } })
.returns(Promise.resolve({ access_token: 'newfoo', refresh_token: 'newbar' }));

const authenticationError = new Error('Some error');
const authenticationError = new errors.AuthenticationException('Some error');
callWithRequest
.withArgs(sinon.match({ headers: { authorization: 'Bearer newfoo' } }), 'shield.authenticate')
.returns(Promise.reject(authenticationError));
.rejects(authenticationError);

const accessToken = 'foo';
const refreshToken = 'bar';
Expand Down
72 changes: 25 additions & 47 deletions x-pack/plugins/security/server/lib/authentication/providers/saml.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ describe('SAMLAuthenticationProvider', () => {
it('fails if token from the state is rejected because of unknown reason.', async () => {
const request = requestFixture();

const failureReason = new Error('Token is not valid!');
const failureReason = { statusCode: 500, message: 'Token is not valid!' };
callWithRequest.withArgs(request, 'shield.authenticate').rejects(failureReason);

const authenticationResult = await provider.authenticate(request, {
Expand All @@ -235,7 +235,7 @@ describe('SAMLAuthenticationProvider', () => {
sinon.match({ headers: { authorization: 'Bearer expired-token' } }),
'shield.authenticate'
)
.rejects({ body: { error: { reason: 'token expired' } } });
.rejects({ statusCode: 401 });

callWithRequest
.withArgs(
Expand Down Expand Up @@ -264,17 +264,20 @@ describe('SAMLAuthenticationProvider', () => {
});
});

it('fails if token from the state is expired and refresh attempt failed too.', async () => {
it('fails if token from the state is expired and refresh attempt failed with unknown reason too.', async () => {
const request = requestFixture();

callWithRequest
.withArgs(
sinon.match({ headers: { authorization: 'Bearer expired-token' } }),
'shield.authenticate'
)
.rejects({ body: { error: { reason: 'token expired' } } });
.rejects({ statusCode: 401 });

const refreshFailureReason = new Error('Something is wrong with refresh token.');
const refreshFailureReason = {
statusCode: 500,
message: 'Something is wrong with refresh token.',
};
callWithInternalUser
.withArgs('shield.getAccessToken', {
body: { grant_type: 'refresh_token', refresh_token: 'invalid-refresh-token' },
Expand All @@ -291,25 +294,25 @@ describe('SAMLAuthenticationProvider', () => {
expect(authenticationResult.error).toBe(refreshFailureReason);
});

it('fails for AJAX requests with user friendly message if refresh token is used more than once.', async () => {
it('fails for AJAX requests with user friendly message if refresh token is expired.', async () => {
const request = requestFixture({ headers: { 'kbn-xsrf': 'xsrf' } });

callWithRequest
.withArgs(
sinon.match({ headers: { authorization: 'Bearer expired-token' } }),
'shield.authenticate'
)
.rejects({ body: { error: { reason: 'token expired' } } });
.rejects({ statusCode: 401 });

callWithInternalUser
.withArgs('shield.getAccessToken', {
body: { grant_type: 'refresh_token', refresh_token: 'invalid-refresh-token' },
body: { grant_type: 'refresh_token', refresh_token: 'expired-refresh-token' },
})
.rejects({ body: { error_description: 'token has already been refreshed' } });
.rejects({ statusCode: 400 });

const authenticationResult = await provider.authenticate(request, {
accessToken: 'expired-token',
refreshToken: 'invalid-refresh-token',
refreshToken: 'expired-refresh-token',
});

expect(request.headers).not.toHaveProperty('authorization');
Expand All @@ -319,7 +322,7 @@ describe('SAMLAuthenticationProvider', () => {
);
});

it('initiates SAML handshake for non-AJAX requests if refresh token is used more than once.', async () => {
it('initiates SAML handshake for non-AJAX requests if access token document is missing.', async () => {
const request = requestFixture({ path: '/some-path', basePath: '/s/foo' });

callWithInternalUser.withArgs('shield.samlPrepare').resolves({
Expand All @@ -332,17 +335,20 @@ describe('SAMLAuthenticationProvider', () => {
sinon.match({ headers: { authorization: 'Bearer expired-token' } }),
'shield.authenticate'
)
.rejects({ body: { error: { reason: 'token expired' } } });
.rejects({
statusCode: 500,
body: { error: { reason: 'token document is missing and must be present' } },
});

callWithInternalUser
.withArgs('shield.getAccessToken', {
body: { grant_type: 'refresh_token', refresh_token: 'invalid-refresh-token' },
body: { grant_type: 'refresh_token', refresh_token: 'expired-refresh-token' },
})
.rejects({ body: { error_description: 'token has already been refreshed' } });
.rejects({ statusCode: 400 });

const authenticationResult = await provider.authenticate(request, {
accessToken: 'expired-token',
refreshToken: 'invalid-refresh-token',
refreshToken: 'expired-refresh-token',
});

sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlPrepare', {
Expand All @@ -359,34 +365,6 @@ describe('SAMLAuthenticationProvider', () => {
});
});

it('fails for AJAX requests with user friendly message if refresh token is expired.', async () => {
const request = requestFixture({ headers: { 'kbn-xsrf': 'xsrf' } });

callWithRequest
.withArgs(
sinon.match({ headers: { authorization: 'Bearer expired-token' } }),
'shield.authenticate'
)
.rejects({ body: { error: { reason: 'token expired' } } });

callWithInternalUser
.withArgs('shield.getAccessToken', {
body: { grant_type: 'refresh_token', refresh_token: 'expired-refresh-token' },
})
.rejects({ body: { error_description: 'refresh token is expired' } });

const authenticationResult = await provider.authenticate(request, {
accessToken: 'expired-token',
refreshToken: 'expired-refresh-token',
});

expect(request.headers).not.toHaveProperty('authorization');
expect(authenticationResult.failed()).toBe(true);
expect(authenticationResult.error).toEqual(
Boom.badRequest('Both access and refresh tokens are expired.')
);
});

it('initiates SAML handshake for non-AJAX requests if refresh token is expired.', async () => {
const request = requestFixture({ path: '/some-path', basePath: '/s/foo' });

Expand All @@ -400,13 +378,13 @@ describe('SAMLAuthenticationProvider', () => {
sinon.match({ headers: { authorization: 'Bearer expired-token' } }),
'shield.authenticate'
)
.rejects({ body: { error: { reason: 'token expired' } } });
.rejects({ statusCode: 401 });

callWithInternalUser
.withArgs('shield.getAccessToken', {
body: { grant_type: 'refresh_token', refresh_token: 'expired-refresh-token' },
})
.rejects({ body: { error_description: 'refresh token is expired' } });
.rejects({ statusCode: 400 });

const authenticationResult = await provider.authenticate(request, {
accessToken: 'expired-token',
Expand Down Expand Up @@ -444,7 +422,7 @@ describe('SAMLAuthenticationProvider', () => {
it('fails if token from `authorization` header is rejected.', async () => {
const request = requestFixture({ headers: { authorization: 'Bearer some-invalid-token' } });

const failureReason = new Error('Token is not valid!');
const failureReason = { statusCode: 401 };
callWithRequest.withArgs(request, 'shield.authenticate').rejects(failureReason);

const authenticationResult = await provider.authenticate(request);
Expand All @@ -457,7 +435,7 @@ describe('SAMLAuthenticationProvider', () => {
const user = { username: 'user' };
const request = requestFixture({ headers: { authorization: 'Bearer some-invalid-token' } });

const failureReason = new Error('Token is not valid!');
const failureReason = { statusCode: 401 };
callWithRequest.withArgs(request, 'shield.authenticate').rejects(failureReason);

callWithRequest
Expand Down
Loading