Skip to content

Commit

Permalink
Review#4: remove wrong test comment, add more tests for the `LoginFor…
Browse files Browse the repository at this point in the history 
…m` component and `Authenticator`, add additional integration test for Keberos when client provides certificate, fix other nits.
  • Loading branch information
@azasypkin
azasypkin committed Mar 23, 2020
1 parent 181c6db commit 37c6489
Show file tree
Hide file tree
Showing 7 changed files with 231 additions and 30 deletions.
4 changes: 4 additions & 0 deletions x-pack/plugins/security/public/authentication/login/__snapshots__/login_page.test.tsx.snap
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 11 additions & 2 deletions .../public/authentication/login/components/login_form/__snapshots__/login_form.test.tsx.snap
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 40 additions & 2 deletions ...ck/plugins/security/public/authentication/login/components/login_form/login_form.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ describe('LoginForm', () => {
).toMatchSnapshot();
});

it('renders as expected without login form', async () => {
it('renders as expected without login form for providers with and without description', async () => {
const coreStartMock = coreMock.createStart();
expect(
shallowWithIntl(
Expand All @@ -179,7 +179,7 @@ describe('LoginForm', () => {
enabled: true,
providers: [
{ type: 'saml', name: 'saml1', description: 'Login w/SAML' },
{ type: 'pki', name: 'pki1', description: 'Login w/PKI' },
{ type: 'pki', name: 'pki1' },
],
}}
/>
Expand Down Expand Up @@ -230,5 +230,43 @@ describe('LoginForm', () => {
expect(wrapper.find(EuiCallOut).exists()).toBe(false);
expect(coreStartMock.notifications.toasts.addError).not.toHaveBeenCalled();
});

it('shows error toast if login fails', async () => {
const currentURL = `https://some-host/login?next=${encodeURIComponent(
'/some-base-path/app/kibana#/home?_g=()'
)}`;

const failureReason = new Error('Oh no!');
const coreStartMock = coreMock.createStart({ basePath: '/some-base-path' });
coreStartMock.http.post.mockRejectedValue(failureReason);

window.location.href = currentURL;
const wrapper = mountWithIntl(
<LoginForm
http={coreStartMock.http}
notifications={coreStartMock.notifications}
loginAssistanceMessage=""
showLoginForm={true}
selector={{ enabled: true, providers: [{ type: 'saml', name: 'saml1' }] }}
/>
);

wrapper.findWhere(node => node.key() === 'saml1').simulate('click');

await act(async () => {
await nextTick();
wrapper.update();
});

expect(coreStartMock.http.post).toHaveBeenCalledTimes(1);
expect(coreStartMock.http.post).toHaveBeenCalledWith('/internal/security/login_with', {
body: JSON.stringify({ providerType: 'saml', providerName: 'saml1', currentURL }),
});

expect(window.location.href).toBe(currentURL);
expect(coreStartMock.notifications.toasts.addError).toHaveBeenCalledWith(failureReason, {
title: 'Could not perform login.',
});
});
});
});
128 changes: 114 additions & 14 deletions x-pack/plugins/security/server/authentication/authenticator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,19 @@ function getMockOptions({
session,
providers,
http = {},
selector,
}: {
session?: AuthenticatorOptions['config']['session'];
providers?: Record<string, unknown> | string[];
http?: Partial<AuthenticatorOptions['config']['authc']['http']>;
selector?: AuthenticatorOptions['config']['authc']['selector'];
} = {}) {
return {
clusterClient: elasticsearchServiceMock.createClusterClient(),
basePath: httpServiceMock.createSetupContract().basePath,
loggers: loggingServiceMock.create(),
config: createConfig(
ConfigSchema.validate({ session, authc: { providers, http } }),
ConfigSchema.validate({ session, authc: { selector, providers, http } }),
loggingServiceMock.create().get(),
{ isTLSEnabled: false }
),
Expand All @@ -54,7 +56,7 @@ describe('Authenticator', () => {
beforeEach(() => {
mockBasicAuthenticationProvider = {
login: jest.fn(),
authenticate: jest.fn(),
authenticate: jest.fn().mockResolvedValue(AuthenticationResult.notHandled()),
logout: jest.fn().mockResolvedValue(DeauthenticationResult.notHandled()),
getHTTPAuthenticationScheme: jest.fn(),
};
Expand Down Expand Up @@ -182,6 +184,7 @@ describe('Authenticator', () => {
beforeEach(() => {
mockOptions = getMockOptions({ providers: { basic: { basic1: { order: 0 } } } });
mockSessionStorage = sessionStorageMock.create();
mockSessionStorage.get.mockResolvedValue(null);
mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage);
mockSessVal = {
idleTimeoutExpiration: null,
Expand Down Expand Up @@ -322,6 +325,7 @@ describe('Authenticator', () => {
},
});
mockSessionStorage = sessionStorageMock.create();
mockSessionStorage.get.mockResolvedValue(null);
mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage);

authenticator = new Authenticator(mockOptions);
Expand Down Expand Up @@ -496,6 +500,7 @@ describe('Authenticator', () => {
it('clears session if provider asked to do so.', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();
mockSessionStorage.get.mockResolvedValue(mockSessVal);

mockBasicAuthenticationProvider.login.mockResolvedValue(
AuthenticationResult.succeeded(user, { state: null })
Expand All @@ -508,6 +513,26 @@ describe('Authenticator', () => {
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).toHaveBeenCalled();
});

it('clears legacy session.', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();

// Use string format for the `provider` session value field to emulate legacy session.
mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, provider: 'basic' });

mockBasicAuthenticationProvider.login.mockResolvedValue(AuthenticationResult.succeeded(user));

await expect(
authenticator.login(request, { provider: { type: 'basic' }, value: {} })
).resolves.toEqual(AuthenticationResult.succeeded(user));

expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledTimes(1);
expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);

expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).toHaveBeenCalled();
});
});

describe('`authenticate` method', () => {
Expand All @@ -518,6 +543,7 @@ describe('Authenticator', () => {
beforeEach(() => {
mockOptions = getMockOptions({ providers: { basic: { basic1: { order: 0 } } } });
mockSessionStorage = sessionStorageMock.create<ProviderSession>();
mockSessionStorage.get.mockResolvedValue(null);
mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage);
mockSessVal = {
idleTimeoutExpiration: null,
Expand Down Expand Up @@ -931,14 +957,33 @@ describe('Authenticator', () => {
expect(mockSessionStorage.clear).toHaveBeenCalled();
});

it('clears legacy session.', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();

// Use string format for the `provider` session value field to emulate legacy session.
mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, provider: 'basic' });

mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.succeeded(user)
);

await expect(authenticator.authenticate(request)).resolves.toEqual(
AuthenticationResult.succeeded(user)
);

expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalledTimes(1);
expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalledWith(request, null);

expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).toHaveBeenCalled();
});

it('does not clear session if provider can not handle system API request authentication with active session.', async () => {
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'true' },
});

mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.notHandled()
);
mockSessionStorage.get.mockResolvedValue(mockSessVal);

await expect(authenticator.authenticate(request)).resolves.toEqual(
Expand All @@ -954,9 +999,6 @@ describe('Authenticator', () => {
headers: { 'kbn-system-request': 'false' },
});

mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.notHandled()
);
mockSessionStorage.get.mockResolvedValue(mockSessVal);

await expect(authenticator.authenticate(request)).resolves.toEqual(
Expand All @@ -972,9 +1014,6 @@ describe('Authenticator', () => {
headers: { 'kbn-system-request': 'true' },
});

mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.notHandled()
);
mockSessionStorage.get.mockResolvedValue({
...mockSessVal,
provider: { type: 'token', name: 'token1' },
Expand All @@ -993,9 +1032,6 @@ describe('Authenticator', () => {
headers: { 'kbn-system-request': 'false' },
});

mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.notHandled()
);
mockSessionStorage.get.mockResolvedValue({
...mockSessVal,
provider: { type: 'token', name: 'token1' },
Expand All @@ -1008,6 +1044,70 @@ describe('Authenticator', () => {
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).toHaveBeenCalled();
});

describe('with Login Selector', () => {
beforeEach(() => {
mockOptions = getMockOptions({
selector: { enabled: true },
providers: { basic: { basic1: { order: 0 } } },
});
mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage);

authenticator = new Authenticator(mockOptions);
});

it('does not redirect to Login Selector if there is an active session', async () => {
const request = httpServerMock.createKibanaRequest();
mockSessionStorage.get.mockResolvedValue(mockSessVal);

await expect(authenticator.authenticate(request)).resolves.toEqual(
AuthenticationResult.notHandled()
);
expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalled();
});

it('does not redirect AJAX requests to Login Selector', async () => {
const request = httpServerMock.createKibanaRequest({ headers: { 'kbn-xsrf': 'xsrf' } });

await expect(authenticator.authenticate(request)).resolves.toEqual(
AuthenticationResult.notHandled()
);
expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalled();
});

it('does not redirect to Login Selector if request has `Authorization` header', async () => {
const request = httpServerMock.createKibanaRequest({
headers: { authorization: 'Basic ***' },
});

await expect(authenticator.authenticate(request)).resolves.toEqual(
AuthenticationResult.notHandled()
);
expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalled();
});

it('does not redirect to Login Selector if it is not enabled', async () => {
mockOptions = getMockOptions({ providers: { basic: { basic1: { order: 0 } } } });
mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage);
authenticator = new Authenticator(mockOptions);

const request = httpServerMock.createKibanaRequest();
await expect(authenticator.authenticate(request)).resolves.toEqual(
AuthenticationResult.notHandled()
);
expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalled();
});

it('redirects to the Login Selector when needed.', async () => {
const request = httpServerMock.createKibanaRequest();
await expect(authenticator.authenticate(request)).resolves.toEqual(
AuthenticationResult.redirectTo(
'/mock-server-basepath/login?next=%2Fmock-server-basepath%2Fpath'
)
);
expect(mockBasicAuthenticationProvider.authenticate).not.toHaveBeenCalled();
});
});
});

describe('`logout` method', () => {
Expand Down
24 changes: 17 additions & 7 deletions x-pack/plugins/security/server/authentication/authenticator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,15 @@ function isLoginAttemptWithProviderType(
);
}

/**
* Determines if session value was created by the previous Kibana versions which had a different
* session value format.
* @param sessionValue The session value to check.
*/
function isLegacyProviderSession(sessionValue: any) {
return typeof sessionValue?.provider === 'string';
}

/**
* Instantiates authentication provider based on the provider key from config.
* @param providerType Provider type key.
Expand Down Expand Up @@ -503,18 +512,19 @@ export class Authenticator {
* @param sessionStorage Session storage instance.
*/
private async getSessionValue(sessionStorage: SessionStorage<ProviderSession>) {
let sessionValue = await sessionStorage.get();
const sessionValue = await sessionStorage.get();

// If for some reason we have a session stored for the provider that is not available
// (e.g. when user was logged in with one provider, but then configuration has changed
// and that provider is no longer available), then we should clear session entirely.
const sessionProvider = sessionValue && this.providers.get(sessionValue.provider?.name);
// If we detect that session is in incompatible format or for some reason we have a session
// stored for the provider that is not available anymore (e.g. when user was logged in with one
// provider, but then configuration has changed and that provider is no longer available), then
// we should clear session entirely.
if (
sessionValue &&
(!sessionProvider || sessionProvider.type !== sessionValue?.provider.type)
(isLegacyProviderSession(sessionValue) ||
this.providers.get(sessionValue.provider.name)?.type !== sessionValue.provider.type)
) {
sessionStorage.clear();
sessionValue = null;
return null;
}

return sessionValue;
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security/server/routes/authentication/common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef
const { providerType, providerName, currentURL } = request.body;
logger.info(`Logging in with provider "${providerName}" (${providerType})`);

const redirectURL = parseNext(currentURL ?? '', basePath.serverBasePath);
const redirectURL = parseNext(currentURL, basePath.serverBasePath);
try {
const authenticationResult = await authc.login(request, {
provider: { name: providerName },
Expand Down
Loading

0 comments on commit 37c6489

Please sign in to comment.