Skip to content

Commit

Permalink
Remove redundant call to _authenticate API after access token is cr…
Browse files Browse the repository at this point in the history 
…eated. (#82980)
  • Loading branch information
@azasypkin
azasypkin authored Nov 10, 2020
1 parent 2830523 commit a63c390
Show file tree
Hide file tree
Showing 23 changed files with 258 additions and 495 deletions.
1 change: 0 additions & 1 deletion .github/CODEOWNERS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@
/x-pack/test/security_api_integration/ @elastic/kibana-security
/x-pack/test/security_functional/ @elastic/kibana-security
/x-pack/test/spaces_api_integration/ @elastic/kibana-security
/x-pack/test/token_api_integration/ @elastic/kibana-security
#CC# /src/legacy/ui/public/capabilities @elastic/kibana-security
#CC# /x-pack/legacy/plugins/encrypted_saved_objects/ @elastic/kibana-security
#CC# /x-pack/plugins/security_solution/ @elastic/kibana-security
Expand Down
19 changes: 15 additions & 4 deletions x-pack/plugins/security/server/authentication/providers/base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
ILegacyClusterClient,
Headers,
} from '../../../../../../src/core/server';
import { AuthenticatedUser } from '../../../common/model';
import type { AuthenticatedUser } from '../../../common/model';
import type { AuthenticationInfo } from '../../elasticsearch';
import { AuthenticationResult } from '../authentication_result';
import { DeauthenticationResult } from '../deauthentication_result';
import { Tokens } from '../tokens';
Expand Down Expand Up @@ -109,10 +110,20 @@ export abstract class BaseAuthenticationProvider {
* @param [authHeaders] Optional `Headers` dictionary to send with the request.
*/
protected async getUser(request: KibanaRequest, authHeaders: Headers = {}) {
return deepFreeze({
...(await this.options.client
return this.authenticationInfoToAuthenticatedUser(
await this.options.client
.asScoped({ headers: { ...request.headers, ...authHeaders } })
.callAsCurrentUser('shield.authenticate')),
.callAsCurrentUser('shield.authenticate')
);
}

/**
* Converts Elasticsearch Authentication result to a Kibana authenticated user.
* @param authenticationInfo Result returned from the `_authenticate` operation.
*/
protected authenticationInfoToAuthenticatedUser(authenticationInfo: AuthenticationInfo) {
return deepFreeze({
...authenticationInfo,
authentication_provider: { type: this.type, name: this.options.name },
} as AuthenticatedUser);
}
Expand Down
70 changes: 11 additions & 59 deletions x-pack/plugins/security/server/authentication/providers/kerberos.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,10 @@ describe('KerberosAuthenticationProvider', () => {
headers: { authorization: 'negotiate spnego' },
});

const mockScopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(user);
mockOptions.client.asScoped.mockReturnValue(mockScopedClusterClient);
mockOptions.client.callAsInternalUser.mockResolvedValue({
access_token: 'some-token',
refresh_token: 'some-refresh-token',
authentication: user,
});

await expect(operation(request)).resolves.toEqual(
Expand All @@ -136,10 +134,7 @@ describe('KerberosAuthenticationProvider', () => {
)
);

expectAuthenticateCall(mockOptions.client, {
headers: { authorization: 'Bearer some-token' },
});

expect(mockOptions.client.asScoped).not.toHaveBeenCalled();
expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.getAccessToken', {
body: { grant_type: '_kerberos', kerberos_ticket: 'spnego' },
});
Expand All @@ -153,13 +148,11 @@ describe('KerberosAuthenticationProvider', () => {
headers: { authorization: 'negotiate spnego' },
});

const mockScopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(user);
mockOptions.client.asScoped.mockReturnValue(mockScopedClusterClient);
mockOptions.client.callAsInternalUser.mockResolvedValue({
access_token: 'some-token',
refresh_token: 'some-refresh-token',
kerberos_authentication_response_token: 'response-token',
authentication: user,
});

await expect(operation(request)).resolves.toEqual(
Expand All @@ -173,10 +166,7 @@ describe('KerberosAuthenticationProvider', () => {
)
);

expectAuthenticateCall(mockOptions.client, {
headers: { authorization: 'Bearer some-token' },
});

expect(mockOptions.client.asScoped).not.toHaveBeenCalled();
expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.getAccessToken', {
body: { grant_type: '_kerberos', kerberos_ticket: 'spnego' },
});
Expand Down Expand Up @@ -250,33 +240,6 @@ describe('KerberosAuthenticationProvider', () => {

expect(request.headers.authorization).toBe('negotiate spnego');
});

it('fails if could not retrieve user using the new access token.', async () => {
const request = httpServerMock.createKibanaRequest({
headers: { authorization: 'negotiate spnego' },
});

const failureReason = LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error());
const mockScopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason);
mockOptions.client.asScoped.mockReturnValue(mockScopedClusterClient);
mockOptions.client.callAsInternalUser.mockResolvedValue({
access_token: 'some-token',
refresh_token: 'some-refresh-token',
});

await expect(operation(request)).resolves.toEqual(AuthenticationResult.failed(failureReason));

expectAuthenticateCall(mockOptions.client, {
headers: { authorization: 'Bearer some-token' },
});

expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.getAccessToken', {
body: { grant_type: '_kerberos', kerberos_ticket: 'spnego' },
});

expect(request.headers.authorization).toBe('negotiate spnego');
});
}

describe('`login` method', () => {
Expand Down Expand Up @@ -381,32 +344,21 @@ describe('KerberosAuthenticationProvider', () => {
expect(request.headers).not.toHaveProperty('authorization');
});

it('succeeds with valid session even if requiring a token refresh', async () => {
it('succeeds with a valid session even if requiring a token refresh', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();
const tokenPair = { accessToken: 'foo', refreshToken: 'bar' };

mockOptions.client.asScoped.mockImplementation((scopeableRequest) => {
if (scopeableRequest?.headers.authorization === `Bearer ${tokenPair.accessToken}`) {
const mockScopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(
LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error())
);
return mockScopedClusterClient;
}

if (scopeableRequest?.headers.authorization === 'Bearer newfoo') {
const mockScopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(user);
return mockScopedClusterClient;
}

throw new Error('Unexpected call');
});
const mockScopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(
LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error())
);
mockOptions.client.asScoped.mockReturnValue(mockScopedClusterClient);

mockOptions.tokens.refresh.mockResolvedValue({
accessToken: 'newfoo',
refreshToken: 'newbar',
authenticationInfo: user,
});

await expect(provider.authenticate(request, tokenPair)).resolves.toEqual(
Expand Down
61 changes: 25 additions & 36 deletions x-pack/plugins/security/server/authentication/providers/kerberos.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ import {
LegacyElasticsearchErrorHelpers,
KibanaRequest,
} from '../../../../../../src/core/server';
import type { AuthenticationInfo } from '../../elasticsearch';
import { AuthenticationResult } from '../authentication_result';
import { DeauthenticationResult } from '../deauthentication_result';
import { HTTPAuthorizationHeader } from '../http_authentication';
import { canRedirectRequest } from '../can_redirect_request';
import { Tokens, TokenPair } from '../tokens';
import { Tokens, TokenPair, RefreshTokenResult } from '../tokens';
import { BaseAuthenticationProvider } from './base';

/**
Expand Down Expand Up @@ -149,6 +150,7 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
access_token: string;
refresh_token: string;
kerberos_authentication_response_token?: string;
authentication: AuthenticationInfo;
};
try {
tokens = await this.options.client.callAsInternalUser('shield.getAccessToken', {
Expand Down Expand Up @@ -203,23 +205,16 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
};
}

try {
// Then attempt to query for the user details using the new token
const authHeaders = {
authorization: new HTTPAuthorizationHeader('Bearer', tokens.access_token).toString(),
};
const user = await this.getUser(request, authHeaders);

this.logger.debug('User has been authenticated with new access token');
return AuthenticationResult.succeeded(user, {
authHeaders,
return AuthenticationResult.succeeded(
this.authenticationInfoToAuthenticatedUser(tokens.authentication),
{
authHeaders: {
authorization: new HTTPAuthorizationHeader('Bearer', tokens.access_token).toString(),
},
authResponseHeaders,
state: { accessToken: tokens.access_token, refreshToken: tokens.refresh_token },
});
} catch (err) {
this.logger.debug(`Failed to authenticate request via access token: ${err.message}`);
return AuthenticationResult.failed(err);
}
}
);
}

/**
Expand Down Expand Up @@ -260,38 +255,32 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
private async authenticateViaRefreshToken(request: KibanaRequest, state: ProviderState) {
this.logger.debug('Trying to refresh access token.');

let refreshedTokenPair: TokenPair | null;
let refreshTokenResult: RefreshTokenResult | null;
try {
refreshedTokenPair = await this.options.tokens.refresh(state.refreshToken);
refreshTokenResult = await this.options.tokens.refresh(state.refreshToken);
} catch (err) {
return AuthenticationResult.failed(err);
}

// If refresh token is no longer valid, let's try to renegotiate new tokens using SPNEGO. We
// allow this because expired underlying token is an implementation detail and Kibana user
// facing session is still valid.
if (refreshedTokenPair === null) {
if (refreshTokenResult === null) {
this.logger.debug('Both access and refresh tokens are expired. Re-authenticating...');
return this.authenticateViaSPNEGO(request, state);
}

try {
const authHeaders = {
authorization: new HTTPAuthorizationHeader(
'Bearer',
refreshedTokenPair.accessToken
).toString(),
};
const user = await this.getUser(request, authHeaders);

this.logger.debug('Request has been authenticated via refreshed token.');
return AuthenticationResult.succeeded(user, { authHeaders, state: refreshedTokenPair });
} catch (err) {
this.logger.debug(
`Failed to authenticate user using newly refreshed access token: ${err.message}`
);
return AuthenticationResult.failed(err);
}
this.logger.debug('Request has been authenticated via refreshed token.');
const { accessToken, refreshToken, authenticationInfo } = refreshTokenResult;
return AuthenticationResult.succeeded(
this.authenticationInfoToAuthenticatedUser(authenticationInfo),
{
authHeaders: {
authorization: new HTTPAuthorizationHeader('Bearer', accessToken).toString(),
},
state: { accessToken, refreshToken },
}
);
}

/**
Expand Down
20 changes: 5 additions & 15 deletions x-pack/plugins/security/server/authentication/providers/oidc.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ describe('OIDCAuthenticationProvider', () => {
const { request, attempt, expectedRedirectURI } = getMocks();

mockOptions.client.callAsInternalUser.mockResolvedValue({
authentication: mockUser,
access_token: 'some-token',
refresh_token: 'some-refresh-token',
});
Expand Down Expand Up @@ -440,25 +441,14 @@ describe('OIDCAuthenticationProvider', () => {
const request = httpServerMock.createKibanaRequest();
const tokenPair = { accessToken: 'expired-token', refreshToken: 'valid-refresh-token' };

mockOptions.client.asScoped.mockImplementation((scopeableRequest) => {
if (scopeableRequest?.headers.authorization === `Bearer ${tokenPair.accessToken}`) {
const mockScopedClusterClientToFail = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClientToFail.callAsCurrentUser.mockRejectedValue(
LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error())
);
return mockScopedClusterClientToFail;
}

if (scopeableRequest?.headers.authorization === 'Bearer new-access-token') {
return mockScopedClusterClient;
}

throw new Error('Unexpected call');
});
mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(
LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error())
);

mockOptions.tokens.refresh.mockResolvedValue({
accessToken: 'new-access-token',
refreshToken: 'new-refresh-token',
authenticationInfo: mockUser,
});

await expect(
Expand Down
Loading

0 comments on commit a63c390

Please sign in to comment.