Skip to content

Commit

Permalink
Properly handle password change for users authenticated with provider…
Browse files Browse the repository at this point in the history
… other than `basic`. (#55206) (#56831)
  • Loading branch information
azasypkin committed Feb 5, 2020
1 parent b5b610e commit 20b3db1
Show file tree
Hide file tree
Showing 33 changed files with 284 additions and 255 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ test('getUserName() returns a name when security is enabled', async () => {
factory.create(KibanaRequest.from(fakeRequest), fakeRequest);
const constructorCall = jest.requireMock('./alerts_client').AlertsClient.mock.calls[0][0];

securityPluginSetup.authc.getCurrentUser.mockResolvedValueOnce({ username: 'bob' });
securityPluginSetup.authc.getCurrentUser.mockReturnValueOnce({ username: 'bob' });
const userNameResult = await constructorCall.getUserName();
expect(userNameResult).toEqual('bob');
});
Expand Down
19 changes: 3 additions & 16 deletions x-pack/legacy/plugins/reporting/server/lib/get_user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,14 @@

import { Legacy } from 'kibana';
import { KibanaRequest } from '../../../../../../src/core/server';
import { Logger, ServerFacade } from '../../types';
import { ServerFacade } from '../../types';
import { ReportingSetupDeps } from '../plugin';

export function getUserFactory(
server: ServerFacade,
security: ReportingSetupDeps['security'],
logger: Logger
) {
export function getUserFactory(server: ServerFacade, security: ReportingSetupDeps['security']) {
/*
* Legacy.Request because this is called from routing middleware
*/
return async (request: Legacy.Request) => {
if (!security) {
return null;
}

try {
return await security.authc.getCurrentUser(KibanaRequest.from(request));
} catch (err) {
logger.error(err, ['getUser']);
return null;
}
return security?.authc.getCurrentUser(KibanaRequest.from(request)) ?? null;
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const authorizedUserPreRoutingFactory = function authorizedUserPreRouting
plugins: ReportingSetupDeps,
logger: Logger
) {
const getUser = getUserFactory(server, plugins.security, logger);
const getUser = getUserFactory(server, plugins.security);
const config = server.config();

return async function authorizedUserPreRouting(request: Legacy.Request) {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/legacy/plugins/security/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ export const security = kibana =>
);

server.expose({
getUser: request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)),
getUser: async request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)),
});

initLoginView(securityPlugin, server);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export function mockAuthenticatedUser(user: Partial<AuthenticatedUser> = {}) {
enabled: true,
authentication_realm: { name: 'native1', type: 'native' },
lookup_realm: { name: 'native1', type: 'native' },
authentication_provider: 'basic',
...user,
};
}
5 changes: 5 additions & 0 deletions x-pack/plugins/security/common/model/authenticated_user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ export interface AuthenticatedUser extends User {
* The name and type of the Realm where the user information were retrieved from.
*/
lookup_realm: UserRealm;

/**
* Name of the Kibana authentication provider that used to authenticate user.
*/
authentication_provider: string;
}

export function canUserChangePassword(user: AuthenticatedUser) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { AuthenticatedUser } from '../../common/model';
import { AccountManagementPage } from './account_management_page';

import { coreMock } from 'src/core/public/mocks';
import { mockAuthenticatedUser } from '../../common/model/authenticated_user.mock';
import { securityMock } from '../mocks';
import { userAPIClientMock } from '../management/users/index.mock';

Expand All @@ -19,11 +20,10 @@ interface Options {
realm?: string;
}
const createUser = ({ withFullName = true, withEmail = true, realm = 'native' }: Options = {}) => {
return {
return mockAuthenticatedUser({
full_name: withFullName ? 'Casey Smith' : '',
username: 'csmith',
email: withEmail ? 'csmith@domain.com' : '',
enabled: true,
roles: [],
authentication_realm: {
type: realm,
Expand All @@ -33,7 +33,7 @@ const createUser = ({ withFullName = true, withEmail = true, realm = 'native' }:
type: realm,
name: realm,
},
};
});
};

function getSecuritySetupMock({ currentUser }: { currentUser: AuthenticatedUser }) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

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

import Boom from 'boom';
import { duration, Duration } from 'moment';
Expand Down Expand Up @@ -226,75 +226,6 @@ describe('Authenticator', () => {
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).toHaveBeenCalled();
});

describe('stateless login', () => {
it('does not create session even if authentication provider returns state', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();
const authorization = `Basic ${Buffer.from('foo:bar').toString('base64')}`;

mockBasicAuthenticationProvider.login.mockResolvedValue(
AuthenticationResult.succeeded(user, { state: { authorization } })
);

const authenticationResult = await authenticator.login(request, {
provider: 'basic',
value: {},
stateless: true,
});
expect(authenticationResult.succeeded()).toBe(true);
expect(authenticationResult.user).toEqual(user);

expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
expect(mockSessionStorage.get).not.toHaveBeenCalled();
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
});

it('does not clear session even if provider asked to do so.', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();

mockBasicAuthenticationProvider.login.mockResolvedValue(
AuthenticationResult.succeeded(user, { state: null })
);

const authenticationResult = await authenticator.login(request, {
provider: 'basic',
value: {},
stateless: true,
});
expect(authenticationResult.succeeded()).toBe(true);
expect(authenticationResult.user).toEqual(user);

expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
expect(mockSessionStorage.get).not.toHaveBeenCalled();
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
});

it('does not clear session even if provider failed with 401.', async () => {
const request = httpServerMock.createKibanaRequest();

const failureReason = Boom.unauthorized();
mockBasicAuthenticationProvider.login.mockResolvedValue(
AuthenticationResult.failed(failureReason)
);

const authenticationResult = await authenticator.login(request, {
provider: 'basic',
value: {},
stateless: true,
});
expect(authenticationResult.failed()).toBe(true);
expect(authenticationResult.error).toBe(failureReason);

expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
expect(mockSessionStorage.get).not.toHaveBeenCalled();
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
});
});
});

describe('`authenticate` method', () => {
Expand Down
23 changes: 8 additions & 15 deletions x-pack/plugins/security/server/authentication/authenticator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,6 @@ export interface ProviderLoginAttempt {
* Login attempt can have any form and defined by the specific provider.
*/
value: unknown;

/**
* Indicates whether login attempt should be performed in a "stateless" manner. If `true` provider
* performing login will neither be able to retrieve or update existing state if any nor persist
* any new state it may produce as a result of the login attempt. It's `false` by default.
*/
stateless?: boolean;
}

export interface AuthenticatorOptions {
Expand All @@ -108,12 +101,12 @@ const providerMap = new Map<
providerSpecificOptions?: AuthenticationProviderSpecificOptions
) => BaseAuthenticationProvider
>([
['basic', BasicAuthenticationProvider],
['kerberos', KerberosAuthenticationProvider],
['saml', SAMLAuthenticationProvider],
['token', TokenAuthenticationProvider],
['oidc', OIDCAuthenticationProvider],
['pki', PKIAuthenticationProvider],
[BasicAuthenticationProvider.type, BasicAuthenticationProvider],
[KerberosAuthenticationProvider.type, KerberosAuthenticationProvider],
[SAMLAuthenticationProvider.type, SAMLAuthenticationProvider],
[TokenAuthenticationProvider.type, TokenAuthenticationProvider],
[OIDCAuthenticationProvider.type, OIDCAuthenticationProvider],
[PKIAuthenticationProvider.type, PKIAuthenticationProvider],
]);

function assertRequest(request: KibanaRequest) {
Expand Down Expand Up @@ -256,7 +249,7 @@ export class Authenticator {

// If we detect an existing session that belongs to a different provider than the one requested
// to perform a login we should clear such session.
let existingSession = attempt.stateless ? null : await this.getSessionValue(sessionStorage);
let existingSession = await this.getSessionValue(sessionStorage);
if (existingSession && existingSession.provider !== attempt.provider) {
this.logger.debug(
`Clearing existing session of another ("${existingSession.provider}") provider.`
Expand All @@ -283,7 +276,7 @@ export class Authenticator {
(authenticationResult.failed() && getErrorStatusCode(authenticationResult.error) === 401);
if (existingSession && shouldClearSession) {
sessionStorage.clear();
} else if (!attempt.stateless && authenticationResult.shouldUpdateState()) {
} else if (authenticationResult.shouldUpdateState()) {
const { idleTimeoutExpiration, lifespanExpiration } = this.calculateExpiry(existingSession);
sessionStorage.set({
state: authenticationResult.state,
Expand Down
75 changes: 36 additions & 39 deletions x-pack/plugins/security/server/authentication/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ jest.mock('./api_keys');
jest.mock('./authenticator');

import Boom from 'boom';
import { errors } from 'elasticsearch';
import { first } from 'rxjs/operators';

import {
Expand All @@ -27,7 +26,6 @@ import {
AuthToolkit,
IClusterClient,
CoreSetup,
ElasticsearchErrorHelpers,
KibanaRequest,
LoggerFactory,
ScopedClusterClient,
Expand Down Expand Up @@ -332,67 +330,66 @@ describe('setupAuthentication()', () => {
});

describe('getCurrentUser()', () => {
let getCurrentUser: (r: KibanaRequest) => Promise<AuthenticatedUser | null>;
let getCurrentUser: (r: KibanaRequest) => AuthenticatedUser | null;
beforeEach(async () => {
getCurrentUser = (await setupAuthentication(mockSetupAuthenticationParams)).getCurrentUser;
});

it('returns `null` if Security is disabled', async () => {
it('returns `null` if Security is disabled', () => {
mockSetupAuthenticationParams.license.isEnabled.mockReturnValue(false);

await expect(getCurrentUser(httpServerMock.createKibanaRequest())).resolves.toBe(null);
expect(getCurrentUser(httpServerMock.createKibanaRequest())).toBe(null);
});

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

await expect(getCurrentUser(httpServerMock.createKibanaRequest())).rejects.toBe(
failureReason
);
const mockAuthGet = mockSetupAuthenticationParams.http.auth.get as jest.Mock;
mockAuthGet.mockReturnValue({ state: mockUser });

const mockRequest = httpServerMock.createKibanaRequest();
expect(getCurrentUser(mockRequest)).toBe(mockUser);
expect(mockAuthGet).toHaveBeenCalledTimes(1);
expect(mockAuthGet).toHaveBeenCalledWith(mockRequest);
});

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

await expect(getCurrentUser(httpServerMock.createKibanaRequest())).resolves.toBe(mockUser);
const mockRequest = httpServerMock.createKibanaRequest();
expect(getCurrentUser(mockRequest)).toBeNull();
expect(mockAuthGet).toHaveBeenCalledTimes(1);
expect(mockAuthGet).toHaveBeenCalledWith(mockRequest);
});
});

describe('isAuthenticated()', () => {
let isAuthenticated: (r: KibanaRequest) => Promise<boolean>;
let isAuthenticated: (r: KibanaRequest) => boolean;
beforeEach(async () => {
isAuthenticated = (await setupAuthentication(mockSetupAuthenticationParams)).isAuthenticated;
});

it('returns `true` if Security is disabled', async () => {
mockSetupAuthenticationParams.license.isEnabled.mockReturnValue(false);

await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(true);
});
it('returns `true` if request is authenticated', () => {
const mockIsAuthenticated = mockSetupAuthenticationParams.http.auth
.isAuthenticated as jest.Mock;
mockIsAuthenticated.mockReturnValue(true);

it('returns `true` if `authenticate` succeeds.', async () => {
const mockUser = mockAuthenticatedUser();
mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(mockUser);

await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(true);
});

it('returns `false` if `authenticate` fails with 401.', async () => {
const failureReason = ElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error());
mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason);

await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(false);
const mockRequest = httpServerMock.createKibanaRequest();
expect(isAuthenticated(mockRequest)).toBe(true);
expect(mockIsAuthenticated).toHaveBeenCalledTimes(1);
expect(mockIsAuthenticated).toHaveBeenCalledWith(mockRequest);
});

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

await expect(isAuthenticated(httpServerMock.createKibanaRequest())).rejects.toBe(
failureReason
);
const mockRequest = httpServerMock.createKibanaRequest();
expect(isAuthenticated(mockRequest)).toBe(false);
expect(mockIsAuthenticated).toHaveBeenCalledTimes(1);
expect(mockIsAuthenticated).toHaveBeenCalledWith(mockRequest);
});
});

Expand Down
Loading

0 comments on commit 20b3db1

Please sign in to comment.