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

Properly handle password change for users authenticated with provider other than basic. #55206

Merged
merged 2 commits into from
Feb 5, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
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;
Copy link
Member Author

Choose a reason for hiding this comment

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

note: these changes aren't mandatory, just forgot to remove this cleanup. For the majority of places I tried to not change anything.

};
}
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 @@ -130,7 +130,7 @@ export const security = kibana =>
);

server.expose({
getUser: request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)),
getUser: async request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)),
Copy link
Member Author

Choose a reason for hiding this comment

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

note: for BWC reasons kept legacy API async.

});

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 @@ -225,75 +225,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;

Copy link
Member Author

Choose a reason for hiding this comment

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

note: reducing API surface. This workaround was introduced exactly for this use case and we don't need it anymore.

/**
* 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 @@ -107,12 +100,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 @@ -254,7 +247,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 @@ -281,7 +274,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 @@ -289,67 +287,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