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

NP Security HTTP Interceptors #39477

Merged
merged 54 commits into from
Oct 24, 2019
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
d4f7e8e
We have a NP plugin! :celebration:
kobelb Jun 21, 2019
3ebd52f
Redirecting to login on all 401s
kobelb Jun 21, 2019
16c8e29
Merge remote-tracking branch 'upstream/master' into np/security-http-…
kobelb Jul 29, 2019
7c9f4b8
Adding commented out code for when credentials are omitted
kobelb Jul 30, 2019
249bbfa
Merge remote-tracking branch 'upstream/master' into np/security-http-…
kobelb Jul 30, 2019
9041c15
Fixing types
kobelb Jul 31, 2019
2c6b3aa
Respond 403 when user changes password with incorrect current password
kobelb Aug 7, 2019
007bc82
Adding AnonymousPaths where we ignore all 401s
kobelb Aug 7, 2019
67af74c
Adding anonymous path tests
kobelb Aug 7, 2019
984bf56
Extracted a dedicated SessionExpires class and added tests
kobelb Aug 9, 2019
f672b72
Fixing plugin after refactoring to add SessionExpired
kobelb Aug 12, 2019
c7ff2f0
Beginning to work on the session timeout interceptor
kobelb Aug 12, 2019
181d951
Fixing UnauthorizedResponseInterceptor anonymous path test
kobelb Aug 12, 2019
ba84476
Removing test anonymous path
kobelb Aug 14, 2019
a551a5e
Trying to improve readability
kobelb Aug 14, 2019
643729a
Merge remote-tracking branch 'upstream/master' into np/security-http-…
kobelb Aug 14, 2019
32d9385
Displaying session logout warning
kobelb Aug 15, 2019
824086c
Mocking out the base path
kobelb Aug 19, 2019
0384182
Revert "Mocking out the base path"
kobelb Aug 19, 2019
acb61ee
Changing coreMock to use a concrete instance of BasePath
kobelb Aug 19, 2019
41cbc8c
Adding session timeout interceptor tests
kobelb Aug 21, 2019
147edbd
Adding session timeout tests
kobelb Aug 21, 2019
ddab529
Adding more tests for short session timeouts
kobelb Aug 21, 2019
3b8711e
Moving some files to a session folder
kobelb Aug 21, 2019
f37cf88
More thrashing around: renaming and reorganizing
kobelb Aug 21, 2019
93206ad
Renaming Interceptor to HttpInterceptor
kobelb Aug 21, 2019
eb860e8
Merge remote-tracking branch 'upstream/master' into np/security-http-…
kobelb Sep 25, 2019
3bd02fb
Fixing some type errors
kobelb Sep 25, 2019
bae2ef9
Fixing legacy chrome API tests
kobelb Sep 25, 2019
da8b846
Fixing other tests to use the concrete instance of BasePath
kobelb Sep 25, 2019
fb00dc4
Adjusting some types
kobelb Sep 25, 2019
51bdb02
Putting DeeplyMocked back, I don't get how DeeplyMockedKeys works
kobelb Sep 25, 2019
837ba11
Merge remote-tracking branch 'upstream/master' into np/security-http-…
kobelb Oct 18, 2019
e23cf8f
Moving anonymousPaths to public core http
kobelb Oct 18, 2019
2c98227
Reading sessionTimeout from injected vars and supporting null timeout
kobelb Oct 18, 2019
32751a4
Doesn't extend session when there is no response
kobelb Oct 18, 2019
4b0ea10
Updating docs and snapshots
kobelb Oct 18, 2019
9432988
Casting sessionTimeout injectedVar to "number | null"
kobelb Oct 21, 2019
e169fbe
Fixing i18n issues
kobelb Oct 21, 2019
3582111
Merge branch 'master' into np/security-http-interceptors
elasticmachine Oct 21, 2019
edac0ec
Update x-pack/plugins/security/public/plugin.ts
kobelb Oct 22, 2019
85a655e
Adding milliseconds postfix to SessionTimeout private fields
kobelb Oct 22, 2019
a70ac40
Even better anonymous paths, with some validation
kobelb Oct 22, 2019
248c580
Adjusting public method docs for IAnonymousPaths
kobelb Oct 22, 2019
5697aa2
Adjusting spelling of base-path to basePath
kobelb Oct 22, 2019
4af0d8a
Update x-pack/plugins/security/public/session/session_timeout.tsx
kobelb Oct 22, 2019
5a91232
Update src/core/public/http/anonymous_paths.ts
kobelb Oct 22, 2019
50237cb
Update src/core/public/http/anonymous_paths.ts
kobelb Oct 22, 2019
40a3697
AnonymousPaths implements IAnonymousPaths and uses IBasePath
kobelb Oct 22, 2019
08aa526
Removing DeeplyMocked
kobelb Oct 22, 2019
c4ec25c
Removing TODOs
kobelb Oct 22, 2019
15ff621
Fixing types...
kobelb Oct 22, 2019
5a9b9a5
Now, ever more normal
kobelb Oct 23, 2019
43425b1
Merge remote-tracking branch 'upstream/master' into np/security-http-…
kobelb Oct 23, 2019
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
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-public](./kibana-plugin-public.md) &gt; [HttpServiceBase](./kibana-plugin-public.httpservicebase.md) &gt; [anonymousPaths](./kibana-plugin-public.httpservicebase.anonymouspaths.md)

## HttpServiceBase.anonymousPaths property

APIs for denoting certain paths for not requiring authentication

<b>Signature:</b>

```typescript
anonymousPaths: IAnonymousPaths;
```
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export interface HttpServiceBase

| Property | Type | Description |
| --- | --- | --- |
| [anonymousPaths](./kibana-plugin-public.httpservicebase.anonymouspaths.md) | <code>IAnonymousPaths</code> | APIs for denoting certain paths for not requiring authentication |
| [basePath](./kibana-plugin-public.httpservicebase.basepath.md) | <code>IBasePath</code> | APIs for manipulating the basePath on URL segments. |
| [delete](./kibana-plugin-public.httpservicebase.delete.md) | <code>HttpHandler</code> | Makes an HTTP request with the DELETE method. See [HttpHandler](./kibana-plugin-public.httphandler.md) for options. |
| [fetch](./kibana-plugin-public.httpservicebase.fetch.md) | <code>HttpHandler</code> | Makes an HTTP request. Defaults to a GET request unless overriden. See [HttpHandler](./kibana-plugin-public.httphandler.md) for options. |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-public](./kibana-plugin-public.md) &gt; [IAnonymousPaths](./kibana-plugin-public.ianonymouspaths.md) &gt; [isAnonymous](./kibana-plugin-public.ianonymouspaths.isanonymous.md)

## IAnonymousPaths.isAnonymous() method

Determines whether the provided path doesn't require authentication

<b>Signature:</b>

```typescript
isAnonymous(path: string): boolean;
```

## Parameters

| Parameter | Type | Description |
| --- | --- | --- |
| path | <code>string</code> | |

<b>Returns:</b>

`boolean`

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-public](./kibana-plugin-public.md) &gt; [IAnonymousPaths](./kibana-plugin-public.ianonymouspaths.md)

## IAnonymousPaths interface

APIs for denoting paths as not requiring authentication

<b>Signature:</b>

```typescript
export interface IAnonymousPaths
```

## Methods

| Method | Description |
| --- | --- |
| [isAnonymous(path)](./kibana-plugin-public.ianonymouspaths.isanonymous.md) | Determines whether the provided path doesn't require authentication |
| [register(path)](./kibana-plugin-public.ianonymouspaths.register.md) | Register <code>path</code> as not requiring authentication |

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-public](./kibana-plugin-public.md) &gt; [IAnonymousPaths](./kibana-plugin-public.ianonymouspaths.md) &gt; [register](./kibana-plugin-public.ianonymouspaths.register.md)

## IAnonymousPaths.register() method

Register `path` as not requiring authentication

<b>Signature:</b>

```typescript
register(path: string): void;
```

## Parameters

| Parameter | Type | Description |
| --- | --- | --- |
| path | <code>string</code> | |

<b>Returns:</b>

`void`

1 change: 1 addition & 0 deletions docs/development/core/public/kibana-plugin-public.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ The plugin integrates with the core system via lifecycle events: `setup`<!-- -->
| [HttpResponse](./kibana-plugin-public.httpresponse.md) | |
| [HttpServiceBase](./kibana-plugin-public.httpservicebase.md) | |
| [I18nStart](./kibana-plugin-public.i18nstart.md) | I18nStart.Context is required by any localizable React component from @<!-- -->kbn/i18n and @<!-- -->elastic/eui packages and is supposed to be used as the topmost component for any i18n-compatible React tree. |
| [IAnonymousPaths](./kibana-plugin-public.ianonymouspaths.md) | APIs for denoting paths as not requiring authentication |
| [IBasePath](./kibana-plugin-public.ibasepath.md) | APIs for manipulating the basePath on URL segments. |
| [IContextContainer](./kibana-plugin-public.icontextcontainer.md) | An object that handles registration of context providers and configuring handlers with context. |
| [IHttpFetchError](./kibana-plugin-public.ihttpfetcherror.md) | |
Expand Down
81 changes: 81 additions & 0 deletions src/core/public/http/anonymous_paths.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { AnonymousPaths } from './anonymous_paths';
import { BasePath } from './base_path_service';

describe('#register', () => {
it(`throws an error for paths that don't start with '/'`, () => {
const basePath = new BasePath('/foo');
const anonymousPaths = new AnonymousPaths(basePath);
expect(() => anonymousPaths.register('bar')).toThrowErrorMatchingInlineSnapshot(
`"\\"path\\" must start with \\"/\\""`
);
});

it(`allows paths that end with '/'`, () => {
const basePath = new BasePath('/foo');
const anonymousPaths = new AnonymousPaths(basePath);
anonymousPaths.register('/bar/');
});
});

describe('#isAnonymous', () => {
it('returns true for registered paths', () => {
const basePath = new BasePath('/foo');
const anonymousPaths = new AnonymousPaths(basePath);
anonymousPaths.register('/bar');
expect(anonymousPaths.isAnonymous('/foo/bar')).toBe(true);
});

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on adding:

it('returns false for sub-paths of registered paths', () => {
  const basePath = new BasePath('/foo');
  const anonymousPaths = new AnonymousPaths(basePath);
  anonymousPaths.register('/bar');
  expect(anonymousPaths.isAnonymous('/foo/bar/baz')).toBe(false);
});

it('returns true for paths registered with a trailing slash, but call "isAnonymous" with no trailing slash', () => {
const basePath = new BasePath('/foo');
const anonymousPaths = new AnonymousPaths(basePath);
anonymousPaths.register('/bar/');
expect(anonymousPaths.isAnonymous('/foo/bar')).toBe(true);
});

it('returns true for paths registered without a trailing slash, but call "isAnonymous" with a trailing slash', () => {
const basePath = new BasePath('/foo');
const anonymousPaths = new AnonymousPaths(basePath);
anonymousPaths.register('/bar');
expect(anonymousPaths.isAnonymous('/foo/bar/')).toBe(true);
});

it('returns true for paths whose capitalization is different', () => {
const basePath = new BasePath('/foo');
const anonymousPaths = new AnonymousPaths(basePath);
anonymousPaths.register('/BAR');
expect(anonymousPaths.isAnonymous('/foo/bar')).toBe(true);
});

it('returns false for other paths', () => {
const basePath = new BasePath('/foo');
const anonymousPaths = new AnonymousPaths(basePath);
anonymousPaths.register('/bar');
expect(anonymousPaths.isAnonymous('/foo/foo')).toBe(false);
});

it('returns false for sub-paths of registered paths', () => {
const basePath = new BasePath('/foo');
const anonymousPaths = new AnonymousPaths(basePath);
anonymousPaths.register('/bar');
expect(anonymousPaths.isAnonymous('/foo/bar/baz')).toBe(false);
});
});
51 changes: 51 additions & 0 deletions src/core/public/http/anonymous_paths.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { HttpSetup } from 'src/core/public';
kobelb marked this conversation as resolved.
Show resolved Hide resolved

export class AnonymousPaths {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this implement IAnonymousPaths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I will make it so.

private paths: Set<string>;
kobelb marked this conversation as resolved.
Show resolved Hide resolved

constructor(private basePath: HttpSetup['basePath']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: seems simpler to just use IBasePath directly rather than coupling this to HttpSetup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this is a carry-over from AnonymousPaths being a security plugin class. I will update it.

this.paths = new Set();
}

public isAnonymous(path: string): boolean {
const pathWithoutBasePath = this.basePath.remove(path);
return this.paths.has(this.normalizePath(pathWithoutBasePath));
}

public register(path: string) {
if (!path.startsWith('/')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could normalizePath handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely can. It felt somewhat awkward to allow consumers to do anonymousPaths.register('login') and felt more explicit to require them to do anonymousPaths.register('/login'). Requiring the path to start with / makes it more obvious that the paths are from the "root" as opposed to relative to anything. This is largely subjective and since it's a platform API, my opinion is just an opinion and I will make it however you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm all for making things more robust if it's easy to change. Don't consider it a blocker for this PR though

throw new Error('"path" must start with "/"');
}

this.paths.add(this.normalizePath(path));
}

private normalizePath(path: string) {
const lowercased = path.toLowerCase();

if (lowercased.endsWith('/')) {
return lowercased.slice(0, lowercased.length - 1);
}

return lowercased;
}
}
11 changes: 5 additions & 6 deletions src/core/public/http/http_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
import { HttpService } from './http_service';
import { HttpSetup } from './types';
import { BehaviorSubject } from 'rxjs';
import { BasePath } from './base_path_service';
import { AnonymousPaths } from './anonymous_paths';

type ServiceSetupMockType = jest.Mocked<HttpSetup> & {
basePath: jest.Mocked<HttpSetup['basePath']>;
basePath: BasePath;
};

const createServiceMock = ({ basePath = '' } = {}): ServiceSetupMockType => ({
Expand All @@ -34,11 +36,8 @@ const createServiceMock = ({ basePath = '' } = {}): ServiceSetupMockType => ({
patch: jest.fn(),
delete: jest.fn(),
options: jest.fn(),
basePath: {
get: jest.fn(() => basePath),
prepend: jest.fn(path => `${basePath}${path}`),
remove: jest.fn(),
},
basePath: new BasePath(basePath),
anonymousPaths: new AnonymousPaths(new BasePath(basePath)),
addLoadingCount: jest.fn(),
getLoadingCount$: jest.fn().mockReturnValue(new BehaviorSubject(0)),
stop: jest.fn(),
Expand Down
3 changes: 3 additions & 0 deletions src/core/public/http/http_setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { HttpInterceptController } from './http_intercept_controller';
import { HttpFetchError } from './http_fetch_error';
import { HttpInterceptHaltError } from './http_intercept_halt_error';
import { BasePath } from './base_path_service';
import { AnonymousPaths } from './anonymous_paths';

const JSON_CONTENT = /^(application\/(json|x-javascript)|text\/(x-)?javascript|x-json)(;.*)?$/;
const NDJSON_CONTENT = /^(application\/ndjson)(;.*)?$/;
Expand All @@ -57,6 +58,7 @@ export const setup = (
const interceptors = new Set<HttpInterceptor>();
const kibanaVersion = injectedMetadata.getKibanaVersion();
const basePath = new BasePath(injectedMetadata.getBasePath());
const anonymousPaths = new AnonymousPaths(basePath);

function intercept(interceptor: HttpInterceptor) {
interceptors.add(interceptor);
Expand Down Expand Up @@ -318,6 +320,7 @@ export const setup = (
return {
stop,
basePath,
anonymousPaths,
intercept,
removeAllInterceptors,
fetch,
Expand Down
20 changes: 20 additions & 0 deletions src/core/public/http/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ export interface HttpServiceBase {
*/
basePath: IBasePath;

/**
* APIs for denoting certain paths for not requiring authentication
*/
anonymousPaths: IAnonymousPaths;

/**
* Adds a new {@link HttpInterceptor} to the global HTTP client.
* @param interceptor a {@link HttpInterceptor}
Expand Down Expand Up @@ -92,6 +97,21 @@ export interface IBasePath {
remove: (url: string) => string;
}

/**
* APIs for denoting paths as not requiring authentication
*/
export interface IAnonymousPaths {
/**
* Determines whether the provided path doesn't require authentication. `path` should include the current basePath.
*/
isAnonymous(path: string): boolean;

/**
* Register `path` as not requiring authentication. `path` should not include the current basePath.
*/
register(path: string): void;
}

/**
* See {@link HttpServiceBase}
* @public
Expand Down
1 change: 1 addition & 0 deletions src/core/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ export {
HttpHandler,
HttpBody,
IBasePath,
IAnonymousPaths,
IHttpInterceptController,
IHttpFetchError,
InterceptedHttpResponse,
Expand Down
16 changes: 8 additions & 8 deletions src/core/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@
*/
import { applicationServiceMock } from './application/application_service.mock';
import { chromeServiceMock } from './chrome/chrome_service.mock';
import { CoreContext, CoreSetup, CoreStart, PluginInitializerContext } from '.';
import { CoreContext, CoreSetup, CoreStart, PluginInitializerContext, NotificationsSetup } from '.';
import { docLinksServiceMock } from './doc_links/doc_links_service.mock';
import { fatalErrorsServiceMock } from './fatal_errors/fatal_errors_service.mock';
import { httpServiceMock } from './http/http_service.mock';
import { i18nServiceMock } from './i18n/i18n_service.mock';
import { notificationServiceMock } from './notifications/notifications_service.mock';
import { notificationServiceMock, DeeplyMocked } from './notifications/notifications_service.mock';
import { overlayServiceMock } from './overlays/overlay_service.mock';
import { uiSettingsServiceMock } from './ui_settings/ui_settings_service.mock';
import { savedObjectsMock } from './saved_objects/saved_objects_service.mock';
Expand All @@ -41,12 +41,12 @@ export { notificationServiceMock } from './notifications/notifications_service.m
export { overlayServiceMock } from './overlays/overlay_service.mock';
export { uiSettingsServiceMock } from './ui_settings/ui_settings_service.mock';

function createCoreSetupMock() {
const mock: MockedKeys<CoreSetup> = {
function createCoreSetupMock({ basePath = '' } = {}) {
const mock: MockedKeys<CoreSetup> & { notifications: DeeplyMocked<NotificationsSetup> } = {
application: applicationServiceMock.createSetupContract(),
context: contextServiceMock.createSetupContract(),
fatalErrors: fatalErrorsServiceMock.createSetupContract(),
http: httpServiceMock.createSetupContract(),
http: httpServiceMock.createSetupContract({ basePath }),
notifications: notificationServiceMock.createSetupContract(),
uiSettings: uiSettingsServiceMock.createSetupContract(),
injectedMetadata: {
Expand All @@ -57,12 +57,12 @@ function createCoreSetupMock() {
return mock;
}

function createCoreStartMock() {
const mock: MockedKeys<CoreStart> = {
function createCoreStartMock({ basePath = '' } = {}) {
const mock: MockedKeys<CoreStart> & { notifications: DeeplyMocked<NotificationsSetup> } = {
application: applicationServiceMock.createStartContract(),
chrome: chromeServiceMock.createStartContract(),
docLinks: docLinksServiceMock.createStartContract(),
http: httpServiceMock.createStartContract(),
http: httpServiceMock.createStartContract({ basePath }),
i18n: i18nServiceMock.createStartContract(),
notifications: notificationServiceMock.createStartContract(),
overlays: overlayServiceMock.createStartContract(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
} from './notifications_service';
import { toastsServiceMock } from './toasts/toasts_service.mock';

type DeeplyMocked<T> = { [P in keyof T]: jest.Mocked<T[P]> };
export type DeeplyMocked<T> = { [P in keyof T]: jest.Mocked<T[P]> };
kobelb marked this conversation as resolved.
Show resolved Hide resolved

const createSetupContractMock = () => {
const setupContract: DeeplyMocked<NotificationsSetup> = {
Expand Down
Loading