migrate xsrf / version-check / custom-headers handlers to NP by pgayvallet · Pull Request #53684 · elastic/kibana

pgayvallet · 2019-12-20T10:16:40Z

Summary

Fix #50656

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

pgayvallet · 2019-12-20T10:18:38Z

src/core/server/http/http_config.ts

+      customResponseHeaders: schema.recordOf(schema.string(), schema.string(), {
+        defaultValue: {},
+      }),


I kept the same name. It's quite long but I don't really have a better idea. If someone comes with a better name, we could deprecates this one now that deprecations can be handled in NP configs

pgayvallet · 2019-12-20T10:20:07Z

src/core/server/http/http_service.ts

+
+    registerCoreHandlers(serverContract, config, this.env);
+


I did not add a test that ensure this is properly invoked during the service setup phase as the feature is heavily integration-tested. Tell we if we want to still add a test.

pgayvallet · 2019-12-20T10:21:48Z

src/core/server/http/integration_tests/lifecycle_handlers.test.ts

+describe('core lifecycle handlers', () => {
+  let root: ReturnType<typeof kbnTestServer.createRoot>;
+
+  beforeEach(async () => {
+    root = kbnTestServer.createRoot({
+      server: {
+        name: kibanaName,
+        customResponseHeaders: {
+          'some-header': 'some-value',
+        },
+        xsrf: { disableProtection: false, whitelist: [whitelistedTestPath] },
+      },


I'm doing all three handlers integration tests in the same file, so the root creation has settings for all the 3 handlers. Not sure if I should split/isolate per file

pgayvallet · 2019-12-20T10:23:20Z

src/core/server/http/lifecycle/lifecycle_mock.ts

+import { OnPreAuthToolkit } from './on_pre_auth';
+import { LifecycleResponseFactory } from '../router';
+
+type MockToolkit = jest.Mocked<OnPreResponseToolkit & OnPostAuthToolkit & OnPreAuthToolkit>;


All these toolkits have very similar API, in a mock point of view. I generalised this type that can be used on for testing any of the lifecycle handler types.

pgayvallet · 2019-12-20T10:27:00Z

src/legacy/server/config/schema.js

@@ -71,19 +71,6 @@ export default () =>
    server: Joi.object({
      name: Joi.string().default(os.hostname()),


HANDLED_IN_NEW_PLATFORM states This key is handled in the new platform ONLY, however server.name is still used in legacy in some places such as

kibana/src/legacy/ui/ui_render/ui_render_mixin.js

Line 212 in 254b18c

serverName: config.get('server.name'),

so I kept it as is. should I change it to name: HANDLED_IN_NEW_PLATFORM ?

The validation is handled in the new platform, so I'd remove this. The legacy platform can continue using config.get('server.name'), but validation will be delegated to the NP.

TIL we can't. Some plugins are still accessing it from the legacy config, and as we are constructing the legacy config from the raw configuration values, any default from our NP schemas are ignored. had to keep it for now.

elasticmachine · 2019-12-20T15:45:27Z

Pinging @elastic/kibana-platform (Team:Platform)

…-custom-headers

joshdover · 2019-12-20T21:00:44Z

src/core/server/http/lifecycle_handlers.ts

+const VERSION_HEADER = 'kbn-version';
+const XSRF_HEADER = 'kbn-xsrf';
+const KIBANA_NAME_HEADER = 'kbn-name';


What do you think about adding a file for these constants that can be shared across public and server?

While I was implementing the System API behavior, I was considering adding a src/core/utils/constants file for the kbn-system-api and kbn-version header names.

Yes I though about it and was not sure where I was supposed to put the constants file. should it be src/core/utils or src/core/types? Constants are not exactly one or the other.

I could also exports these from src/core/server/http (and src/core/server) and then expose them in src/core/server/types to be eventually used by the client-side?

Do we need this? What benefits does it provide? I don't see a problem to inline them in both server & client within the same domain because:

it simplifies search (Cmd + F) and improves DX. I cannot just copy an error message Request must contain a kbn-xsrf header. and find the place where a request was rejected.

it doesn't clutter code with unnecessary import/export expressions

it makes this API truly private and prevents accidental importing from the plugin code.

The main benefit of sharing that we can easily to audit all internal Kibana headers. 🤔

Other benefit is that if we need to change the header name in the future, we only have to edit it in a single place and don't risk missing a string that needs to be updated (or a string from a yet-to-be-merged PR not getting updated).

That said, I don't imagine these changing often (if at all) or them being used in many more places (if any).

Kind of a toss up on benefits here. I'm fine leaving as-is in this PR and removing my shared module in #53734.

Will keep them as-is for now. Can easily be refactored later on if the benefits appear more significants at some point.

elasticmachine · 2019-12-20T21:32:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a84e4f5

History

💚 Build #16909 succeeded fc25e4b
💔 Build #16889 failed 8a0db0d
💔 Build #16878 failed 873c62e
💔 Build #16869 failed 159e542

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

mshustov · 2019-12-23T12:41:31Z

src/core/server/http/lifecycle_handlers.ts

+const VERSION_HEADER = 'kbn-version';
+const XSRF_HEADER = 'kbn-xsrf';
+const KIBANA_NAME_HEADER = 'kbn-name';


Do we need this? What benefits does it provide? I don't see a problem to inline them in both server & client within the same domain because:

it simplifies search (Cmd + F) and improves DX. I cannot just copy an error message Request must contain a kbn-xsrf header. and find the place where a request was rejected.

it doesn't clutter code with unnecessary import/export expressions

it makes this API truly private and prevents accidental importing from the plugin code.

The main benefit of sharing that we can easily to audit all internal Kibana headers. 🤔

mshustov · 2019-12-23T12:51:15Z

src/core/server/http/integration_tests/lifecycle_handlers.test.ts

+    const testRoute = '/version_check/test/route';
+
+    beforeEach(() => {
+      kbnTestServer.getKbnServer(root).server.route({


I'd rather avoid using raw hapi API in the platform tests.

kibana/src/core/server/http/integration_tests/router.test.ts

Line 51 in a84e4f5

const { server: innerServer, createRouter } = await server.setup(setupDeps);

This allows us to reduce noize in tests when updating hapi version / removing hapi

mshustov · 2019-12-23T12:59:18Z

src/core/server/http/integration_tests/lifecycle_handlers.test.ts

+      await kbnTestServer.request
+        .get(root, testRoute)
+        .set(versionHeader, 'invalid-version')
+        .expect(400, /"Browser client is out of date/);


optional nit: /Browser client is out of date/

mshustov · 2019-12-23T13:01:50Z

src/core/server/http/lifecycle_handlers.test.ts

+
+const createConfig = (partial: Partial<HttpConfig>): HttpConfig => partial as HttpConfig;
+
+const forgeRequest = ({


is there a reason not to use already existing fixtures ?

kibana/src/core/server/http/http_server.mocks.ts

Line 97 in a84e4f5

function createRawRequestMock(customization: DeepPartial<Request> = {}) {

No, just forget we already had a mock request fixture. Will adapt.

mshustov · 2019-12-23T13:06:46Z

src/core/server/http/lifecycle_handlers.test.ts

+    responseFactory = httpServerMock.createLifecycleResponseFactory();
+  });
+
+  it('forward the request to the next interceptor if header matches', () => {


I suspect this functionality is covered by the integration tests. Isn't it? If yes, I'd remove the unit tests because it's hard to write them due to coupling with hapi.

Yes, it's tested in integration tests. However I don't think we are really tightly coupled with hapi here, as all used interfaces/types are from core (even if we mimic hapi's types for some of them). I'm leaning to keep them as this is the only place we can really test them unitarily (integ test have all interceptors registered)

mshustov · 2019-12-23T13:11:00Z

src/legacy/server/config/schema.js

@@ -71,19 +71,6 @@ export default () =>
    server: Joi.object({
      name: Joi.string().default(os.hostname()),


The validation is handled in the new platform, so I'd remove this. The legacy platform can continue using config.get('server.name'), but validation will be delegated to the NP.

mshustov · 2019-12-23T13:13:24Z

src/core/server/http/integration_tests/lifecycle_handlers.test.ts

+      );
+    });
+
+    it('adds the custom headers', async () => {


could you add a test that custom headers are present in error responses as well?

mshustov

some nits, looks good & clean 💯

…-custom-headers

pgayvallet · 2020-01-06T10:06:14Z

retest

…-custom-headers

kibanamachine · 2020-01-06T14:04:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4d88dc7

History

💚 Build #18033 succeeded c64cb06
💔 Build #18013 failed b1286e6
💔 Build #18003 failed b1286e6
💚 Build #17010 succeeded a84e4f5
💚 Build #16909 succeeded fc25e4b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…#53684) * migrate xsrf / version-check / custom-headers handlers to NP * export lifecycleMock to be used by plugins * move toolkit mock to http_server mock * remove legacy config tests on xsrf * fix integration_test http configuration * remove direct HAPI usages from integration tests * nits and comments * add custom headers test in case of server returning error * resolve merge conflicts * restore `server.name` to legacy config

* master: allows Alerts to recover gracefully from Executor errors (elastic#53688) [Console] Fix OSS build (elastic#53885) migrate xsrf / version-check / custom-headers handlers to NP (elastic#53684) use NP deprecations in uiSettings (elastic#53755) adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768)

…53684) (#54016) * migrate xsrf / version-check / custom-headers handlers to NP (#53684) * migrate xsrf / version-check / custom-headers handlers to NP * export lifecycleMock to be used by plugins * move toolkit mock to http_server mock * remove legacy config tests on xsrf * fix integration_test http configuration * remove direct HAPI usages from integration tests * nits and comments * add custom headers test in case of server returning error * resolve merge conflicts * restore `server.name` to legacy config * update snapshots

migrate xsrf / version-check / custom-headers handlers to NP

159e542

pgayvallet commented Dec 20, 2019

View reviewed changes

pgayvallet added 2 commits December 20, 2019 11:29

export lifecycleMock to be used by plugins

b3da5a9

move toolkit mock to http_server mock

873c62e

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0 labels Dec 20, 2019

pgayvallet added 2 commits December 20, 2019 13:12

remove legacy config tests on xsrf

8a0db0d

fix integration_test http configuration

fc25e4b

pgayvallet added Feature:New Platform Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels Dec 20, 2019

pgayvallet marked this pull request as ready for review December 20, 2019 15:45

pgayvallet requested a review from a team as a code owner December 20, 2019 15:45

Merge remote-tracking branch 'upstream/master' into kbn-50656-migrate…

a84e4f5

…-custom-headers

joshdover reviewed Dec 20, 2019

View reviewed changes

mshustov reviewed Dec 23, 2019

View reviewed changes

mshustov approved these changes Dec 24, 2019

View reviewed changes

mshustov mentioned this pull request Dec 24, 2019

Add isSystemRequest support to Kibana Platform #53734

Merged

7 tasks

pgayvallet added 6 commits January 6, 2020 08:48

remove direct HAPI usages from integration tests

82a6684

nits and comments

94c3341

add custom headers test in case of server returning error

d6f0df6

Merge remote-tracking branch 'upstream/master' into kbn-50656-migrate…

d333ee3

…-custom-headers

Merge remote-tracking branch 'upstream/master' into kbn-50656-migrate…

ce34c8e

…-custom-headers

resolve merge conflicts

b1286e6

pgayvallet mentioned this pull request Jan 6, 2020

Allow HTTP routes to programmatically opt out of XSRF protection #53823

Closed

pgayvallet added 2 commits January 6, 2020 12:01

restore server.name to legacy config

c64cb06

Merge remote-tracking branch 'upstream/master' into kbn-50656-migrate…

4d88dc7

…-custom-headers

pgayvallet merged commit 205fbce into elastic:master Jan 6, 2020

pgayvallet mentioned this pull request Jan 6, 2020

[7.x] migrate xsrf / version-check / custom-headers handlers to NP (#53684) #54016

Merged

		@@ -71,19 +71,6 @@ export default () =>
		server: Joi.object({
		name: Joi.string().default(os.hostname()),


		const createConfig = (partial: Partial<HttpConfig>): HttpConfig => partial as HttpConfig;

		const forgeRequest = ({

Conversation

pgayvallet commented Dec 20, 2019

Summary

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Dec 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Dec 20, 2019

💚 Build Succeeded

History

Uh oh!

mshustov Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov left a comment

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Jan 6, 2020

Uh oh!

kibanamachine commented Jan 6, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mshustov Dec 23, 2019 •

edited

Loading

mshustov Dec 23, 2019 •

edited

Loading