Skip to content

Commit 210cb67

Browse files
azasypkinazasypkin
committed
Properly handle password change for users authenticated via token authentication provider.
1 parent 9eb51bf commit 210cb67

30 files changed

Lines changed: 279 additions & 237 deletions

File tree

‎x-pack/legacy/plugins/security/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ export const security = kibana =>
130130
);
131131

132132
server.expose({
133-
getUser: request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)),
133+
getUser: async request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)),
134134
});
135135

136136
initLoginView(securityPlugin, server);

‎x-pack/plugins/security/common/model/authenticated_user.mock.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ export function mockAuthenticatedUser(user: Partial<AuthenticatedUser> = {}) {
1515
enabled: true,
1616
authentication_realm: { name: 'native1', type: 'native' },
1717
lookup_realm: { name: 'native1', type: 'native' },
18+
authentication_provider: 'basic',
1819
...user,
1920
};
2021
}

‎x-pack/plugins/security/common/model/authenticated_user.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ export interface AuthenticatedUser extends User {
2626
* The name and type of the Realm where the user information were retrieved from.
2727
*/
2828
lookup_realm: UserRealm;
29+
30+
/**
31+
* Name of the Kibana authentication provider that used to authenticate user.
32+
*/
33+
authentication_provider: string;
2934
}
3035

3136
export function canUserChangePassword(user: AuthenticatedUser) {

‎x-pack/plugins/security/public/account_management/account_management_page.test.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { AuthenticatedUser } from '../../common/model';
1010
import { AccountManagementPage } from './account_management_page';
1111

1212
import { coreMock } from 'src/core/public/mocks';
13+
import { mockAuthenticatedUser } from '../../common/model/authenticated_user.mock';
1314
import { securityMock } from '../mocks';
1415
import { userAPIClientMock } from '../management/users/index.mock';
1516

@@ -19,11 +20,10 @@ interface Options {
1920
realm?: string;
2021
}
2122
const createUser = ({ withFullName = true, withEmail = true, realm = 'native' }: Options = {}) => {
22-
return {
23+
return mockAuthenticatedUser({
2324
full_name: withFullName ? 'Casey Smith' : '',
2425
username: 'csmith',
2526
email: withEmail ? 'csmith@domain.com' : '',
26-
enabled: true,
2727
roles: [],
2828
authentication_realm: {
2929
type: realm,
@@ -33,7 +33,7 @@ const createUser = ({ withFullName = true, withEmail = true, realm = 'native' }:
3333
type: realm,
3434
name: realm,
3535
},
36-
};
36+
});
3737
};
3838

3939
function getSecuritySetupMock({ currentUser }: { currentUser: AuthenticatedUser }) {

‎x-pack/plugins/security/server/authentication/authenticator.test.ts‎

Lines changed: 1 addition & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* you may not use this file except in compliance with the Elastic License.
55
*/
66

7-
jest.mock('./providers/basic', () => ({ BasicAuthenticationProvider: jest.fn() }));
7+
jest.mock('./providers/basic');
88

99
import Boom from 'boom';
1010
import { duration, Duration } from 'moment';
@@ -225,75 +225,6 @@ describe('Authenticator', () => {
225225
expect(mockSessionStorage.set).not.toHaveBeenCalled();
226226
expect(mockSessionStorage.clear).toHaveBeenCalled();
227227
});
228-
229-
describe('stateless login', () => {
230-
it('does not create session even if authentication provider returns state', async () => {
231-
const user = mockAuthenticatedUser();
232-
const request = httpServerMock.createKibanaRequest();
233-
const authorization = `Basic ${Buffer.from('foo:bar').toString('base64')}`;
234-
235-
mockBasicAuthenticationProvider.login.mockResolvedValue(
236-
AuthenticationResult.succeeded(user, { state: { authorization } })
237-
);
238-
239-
const authenticationResult = await authenticator.login(request, {
240-
provider: 'basic',
241-
value: {},
242-
stateless: true,
243-
});
244-
expect(authenticationResult.succeeded()).toBe(true);
245-
expect(authenticationResult.user).toEqual(user);
246-
247-
expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
248-
expect(mockSessionStorage.get).not.toHaveBeenCalled();
249-
expect(mockSessionStorage.set).not.toHaveBeenCalled();
250-
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
251-
});
252-
253-
it('does not clear session even if provider asked to do so.', async () => {
254-
const user = mockAuthenticatedUser();
255-
const request = httpServerMock.createKibanaRequest();
256-
257-
mockBasicAuthenticationProvider.login.mockResolvedValue(
258-
AuthenticationResult.succeeded(user, { state: null })
259-
);
260-
261-
const authenticationResult = await authenticator.login(request, {
262-
provider: 'basic',
263-
value: {},
264-
stateless: true,
265-
});
266-
expect(authenticationResult.succeeded()).toBe(true);
267-
expect(authenticationResult.user).toEqual(user);
268-
269-
expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
270-
expect(mockSessionStorage.get).not.toHaveBeenCalled();
271-
expect(mockSessionStorage.set).not.toHaveBeenCalled();
272-
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
273-
});
274-
275-
it('does not clear session even if provider failed with 401.', async () => {
276-
const request = httpServerMock.createKibanaRequest();
277-
278-
const failureReason = Boom.unauthorized();
279-
mockBasicAuthenticationProvider.login.mockResolvedValue(
280-
AuthenticationResult.failed(failureReason)
281-
);
282-
283-
const authenticationResult = await authenticator.login(request, {
284-
provider: 'basic',
285-
value: {},
286-
stateless: true,
287-
});
288-
expect(authenticationResult.failed()).toBe(true);
289-
expect(authenticationResult.error).toBe(failureReason);
290-
291-
expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
292-
expect(mockSessionStorage.get).not.toHaveBeenCalled();
293-
expect(mockSessionStorage.set).not.toHaveBeenCalled();
294-
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
295-
});
296-
});
297228
});
298229

299230
describe('`authenticate` method', () => {

‎x-pack/plugins/security/server/authentication/authenticator.ts‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,6 @@ export interface ProviderLoginAttempt {
8080
* Login attempt can have any form and defined by the specific provider.
8181
*/
8282
value: unknown;
83-
84-
/**
85-
* Indicates whether login attempt should be performed in a "stateless" manner. If `true` provider
86-
* performing login will neither be able to retrieve or update existing state if any nor persist
87-
* any new state it may produce as a result of the login attempt. It's `false` by default.
88-
*/
89-
stateless?: boolean;
9083
}
9184

9285
export interface AuthenticatorOptions {
@@ -107,12 +100,12 @@ const providerMap = new Map<
107100
providerSpecificOptions?: AuthenticationProviderSpecificOptions
108101
) => BaseAuthenticationProvider
109102
>([
110-
['basic', BasicAuthenticationProvider],
111-
['kerberos', KerberosAuthenticationProvider],
112-
['saml', SAMLAuthenticationProvider],
113-
['token', TokenAuthenticationProvider],
114-
['oidc', OIDCAuthenticationProvider],
115-
['pki', PKIAuthenticationProvider],
103+
[BasicAuthenticationProvider.type, BasicAuthenticationProvider],
104+
[KerberosAuthenticationProvider.type, KerberosAuthenticationProvider],
105+
[SAMLAuthenticationProvider.type, SAMLAuthenticationProvider],
106+
[TokenAuthenticationProvider.type, TokenAuthenticationProvider],
107+
[OIDCAuthenticationProvider.type, OIDCAuthenticationProvider],
108+
[PKIAuthenticationProvider.type, PKIAuthenticationProvider],
116109
]);
117110

118111
function assertRequest(request: KibanaRequest) {
@@ -254,7 +247,7 @@ export class Authenticator {
254247

255248
// If we detect an existing session that belongs to a different provider than the one requested
256249
// to perform a login we should clear such session.
257-
let existingSession = attempt.stateless ? null : await this.getSessionValue(sessionStorage);
250+
let existingSession = await this.getSessionValue(sessionStorage);
258251
if (existingSession && existingSession.provider !== attempt.provider) {
259252
this.logger.debug(
260253
`Clearing existing session of another ("${existingSession.provider}") provider.`
@@ -281,7 +274,7 @@ export class Authenticator {
281274
(authenticationResult.failed() && getErrorStatusCode(authenticationResult.error) === 401);
282275
if (existingSession && shouldClearSession) {
283276
sessionStorage.clear();
284-
} else if (!attempt.stateless && authenticationResult.shouldUpdateState()) {
277+
} else if (authenticationResult.shouldUpdateState()) {
285278
const { idleTimeoutExpiration, lifespanExpiration } = this.calculateExpiry(existingSession);
286279
sessionStorage.set({
287280
state: authenticationResult.state,

‎x-pack/plugins/security/server/authentication/index.test.ts‎

Lines changed: 36 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ jest.mock('./api_keys');
1010
jest.mock('./authenticator');
1111

1212
import Boom from 'boom';
13-
import { errors } from 'elasticsearch';
1413
import { first } from 'rxjs/operators';
1514

1615
import {
@@ -27,7 +26,6 @@ import {
2726
AuthToolkit,
2827
IClusterClient,
2928
CoreSetup,
30-
ElasticsearchErrorHelpers,
3129
KibanaRequest,
3230
LoggerFactory,
3331
ScopedClusterClient,
@@ -289,67 +287,66 @@ describe('setupAuthentication()', () => {
289287
});
290288

291289
describe('getCurrentUser()', () => {
292-
let getCurrentUser: (r: KibanaRequest) => Promise<AuthenticatedUser | null>;
290+
let getCurrentUser: (r: KibanaRequest) => AuthenticatedUser | null;
293291
beforeEach(async () => {
294292
getCurrentUser = (await setupAuthentication(mockSetupAuthenticationParams)).getCurrentUser;
295293
});
296294

297-
it('returns `null` if Security is disabled', async () => {
295+
it('returns `null` if Security is disabled', () => {
298296
mockSetupAuthenticationParams.license.isEnabled.mockReturnValue(false);
299297

300-
await expect(getCurrentUser(httpServerMock.createKibanaRequest())).resolves.toBe(null);
298+
expect(getCurrentUser(httpServerMock.createKibanaRequest())).toBe(null);
301299
});
302300

303-
it('fails if `authenticate` call fails', async () => {
304-
const failureReason = new Error('Something went wrong');
305-
mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason);
301+
it('returns user from the auth state.', () => {
302+
const mockUser = mockAuthenticatedUser();
306303

307-
await expect(getCurrentUser(httpServerMock.createKibanaRequest())).rejects.toBe(
308-
failureReason
309-
);
304+
const mockAuthGet = mockSetupAuthenticationParams.http.auth.get as jest.Mock;
305+
mockAuthGet.mockReturnValue({ state: mockUser });
306+
307+
const mockRequest = httpServerMock.createKibanaRequest();
308+
expect(getCurrentUser(mockRequest)).toBe(mockUser);
309+
expect(mockAuthGet).toHaveBeenCalledTimes(1);
310+
expect(mockAuthGet).toHaveBeenCalledWith(mockRequest);
310311
});
311312

312-
it('returns result of `authenticate` call.', async () => {
313-
const mockUser = mockAuthenticatedUser();
314-
mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(mockUser);
313+
it('returns null if auth state is not available.', () => {
314+
const mockAuthGet = mockSetupAuthenticationParams.http.auth.get as jest.Mock;
315+
mockAuthGet.mockReturnValue({});
315316

316-
await expect(getCurrentUser(httpServerMock.createKibanaRequest())).resolves.toBe(mockUser);
317+
const mockRequest = httpServerMock.createKibanaRequest();
318+
expect(getCurrentUser(mockRequest)).toBeNull();
319+
expect(mockAuthGet).toHaveBeenCalledTimes(1);
320+
expect(mockAuthGet).toHaveBeenCalledWith(mockRequest);
317321
});
318322
});
319323

320324
describe('isAuthenticated()', () => {
321-
let isAuthenticated: (r: KibanaRequest) => Promise<boolean>;
325+
let isAuthenticated: (r: KibanaRequest) => boolean;
322326
beforeEach(async () => {
323327
isAuthenticated = (await setupAuthentication(mockSetupAuthenticationParams)).isAuthenticated;
324328
});
325329

326-
it('returns `true` if Security is disabled', async () => {
327-
mockSetupAuthenticationParams.license.isEnabled.mockReturnValue(false);
328-
329-
await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(true);
330-
});
330+
it('returns `true` if request is authenticated', () => {
331+
const mockIsAuthenticated = mockSetupAuthenticationParams.http.auth
332+
.isAuthenticated as jest.Mock;
333+
mockIsAuthenticated.mockReturnValue(true);
331334

332-
it('returns `true` if `authenticate` succeeds.', async () => {
333-
const mockUser = mockAuthenticatedUser();
334-
mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(mockUser);
335-
336-
await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(true);
337-
});
338-
339-
it('returns `false` if `authenticate` fails with 401.', async () => {
340-
const failureReason = ElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error());
341-
mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason);
342-
343-
await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(false);
335+
const mockRequest = httpServerMock.createKibanaRequest();
336+
expect(isAuthenticated(mockRequest)).toBe(true);
337+
expect(mockIsAuthenticated).toHaveBeenCalledTimes(1);
338+
expect(mockIsAuthenticated).toHaveBeenCalledWith(mockRequest);
344339
});
345340

346-
it('fails if `authenticate` call fails with unknown reason', async () => {
347-
const failureReason = new errors.BadRequest();
348-
mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason);
341+
it('returns `false` if request is not authenticated', () => {
342+
const mockIsAuthenticated = mockSetupAuthenticationParams.http.auth
343+
.isAuthenticated as jest.Mock;
344+
mockIsAuthenticated.mockReturnValue(false);
349345

350-
await expect(isAuthenticated(httpServerMock.createKibanaRequest())).rejects.toBe(
351-
failureReason
352-
);
346+
const mockRequest = httpServerMock.createKibanaRequest();
347+
expect(isAuthenticated(mockRequest)).toBe(false);
348+
expect(mockIsAuthenticated).toHaveBeenCalledTimes(1);
349+
expect(mockIsAuthenticated).toHaveBeenCalledWith(mockRequest);
353350
});
354351
});
355352

‎x-pack/plugins/security/server/authentication/index.ts‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,12 @@ export async function setupAuthentication({
5555
* Retrieves currently authenticated user associated with the specified request.
5656
* @param request
5757
*/
58-
const getCurrentUser = async (request: KibanaRequest) => {
58+
const getCurrentUser = (request: KibanaRequest) => {
5959
if (!license.isEnabled()) {
6060
return null;
6161
}
6262

63-
return (await clusterClient
64-
.asScoped(request)
65-
.callAsCurrentUser('shield.authenticate')) as AuthenticatedUser;
63+
return (http.auth.get(request).state ?? null) as AuthenticatedUser | null;
6664
};
6765

6866
const isValid = (sessionValue: ProviderSession) => {
@@ -180,18 +178,6 @@ export async function setupAuthentication({
180178
apiKeys.create(request, params),
181179
invalidateAPIKey: (request: KibanaRequest, params: InvalidateAPIKeyParams) =>
182180
apiKeys.invalidate(request, params),
183-
isAuthenticated: async (request: KibanaRequest) => {
184-
try {
185-
await getCurrentUser(request);
186-
} catch (err) {
187-
// Don't swallow server errors.
188-
if (getErrorStatusCode(err) !== 401) {
189-
throw err;
190-
}
191-
return false;
192-
}
193-
194-
return true;
195-
},
181+
isAuthenticated: (request: KibanaRequest) => http.auth.isAuthenticated(request),
196182
};
197183
}

‎x-pack/plugins/security/server/authentication/providers/base.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
IClusterClient,
1212
Headers,
1313
} from '../../../../../../src/core/server';
14+
import { deepFreeze } from '../../../../../../src/core/utils';
1415
import { AuthenticatedUser } from '../../../common/model';
1516
import { AuthenticationResult } from '../authentication_result';
1617
import { DeauthenticationResult } from '../deauthentication_result';
@@ -35,6 +36,11 @@ export type AuthenticationProviderSpecificOptions = Record<string, unknown>;
3536
* Base class that all authentication providers should extend.
3637
*/
3738
export abstract class BaseAuthenticationProvider {
39+
/**
40+
* Type of the provider.
41+
*/
42+
static readonly type: string;
43+
3844
/**
3945
* Logger instance bound to a specific provider context.
4046
*/
@@ -85,8 +91,13 @@ export abstract class BaseAuthenticationProvider {
8591
* @param [authHeaders] Optional `Headers` dictionary to send with the request.
8692
*/
8793
protected async getUser(request: KibanaRequest, authHeaders: Headers = {}) {
88-
return (await this.options.client
89-
.asScoped({ headers: { ...request.headers, ...authHeaders } })
90-
.callAsCurrentUser('shield.authenticate')) as AuthenticatedUser;
94+
return deepFreeze({
95+
...(await this.options.client
96+
.asScoped({ headers: { ...request.headers, ...authHeaders } })
97+
.callAsCurrentUser('shield.authenticate')),
98+
// We use `this.constructor` trick to get access to the static `type` field of the specific
99+
// `BaseAuthenticationProvider` subclass.
100+
authentication_provider: (this.constructor as any).type,
101+
} as AuthenticatedUser);
91102
}
92103
}

0 commit comments

Comments
 (0)