Add isSystemRequest support to Kibana Platform by joshdover · Pull Request #53734 · elastic/kibana

joshdover · 2019-12-20T21:51:38Z

Summary

Adds two new options:

On the client, core.http.fetch now accepts an asSystemRequest option
On the server, KibanaRequest now has an isSystemRequest boolean property

This change also modifies how request interceptors work so that they now can only modify the HttpFetchOptions rather than the raw Request object. This enables Core to encapsulate its implementation details on how it converts HttpFetchOptions to a Request. It also reduces the API surface a bit by not needing to provide access to all of Request's methods.

This change was helpful to support exposing to request interceptors whether or not a request was called with asSystemRequest. This particular change could be split out as a separate PR if we think that is useful.

In renaming this concept from "system api" to "system request", I also renamed the header that is used from kbn-system-api to kbn-system-request to better match the code. The backend will continue to support kbn-system-api for legacy plugin support and to support the use case where the client is running an older version than the server, where requests may still contain the kbn-system-api header.

Opened #55203 to track removing support for the old header.

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

elasticmachine · 2019-12-20T21:51:40Z

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

kibanamachine · 2019-12-22T03:12:44Z

💔 Build Failed

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

History

💔 Build #17039 failed 831ae85df17c38356b935781ff5bacc2835e610d

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

src/core/public/http/fetch.ts

src/core/public/http/types.ts

src/core/public/http/fetch.test.ts

src/core/utils/constants.ts

src/core/server/http/router/request.ts

kibanamachine · 2019-12-28T00:27:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 2b1a6fcb472ff76b6c8857d3ccc3381daa91762f

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

kibanamachine · 2019-12-28T01:23:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 82f8de7f6649470f587e7fa17de5e6c04d56f724

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

kibanamachine · 2019-12-30T18:26:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 29d03bf5199c09f0f05fdc60fd7378500d2f6921

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

kibanamachine · 2019-12-30T20:13:34Z

💔 Build Failed

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

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

streamich

AppArch changes LGTM.

src/core/public/http/fetch.ts

azasypkin

Security related changes LGTM (I'll migrate Security server side part as soon as this PR is merged).

src/core/public/http/types.ts

src/core/public/http/fetch.test.ts

src/core/public/http/types.ts

src/core/public/http/fetch.ts

src/core/server/http/router/request.test.ts

joshdover · 2020-01-17T00:18:48Z

I'm considering renaming the option / overall concept to "system request" rather than "system API". I think this name is more clear.

However, I think that the on-the-wire implementation should remain kbn-system-api for the duration of 7.x to prevent issues when a user has an older version of the Kibana client in their browser tab, while the server has been upgraded. By keeping the header name consistent, any requests sent from older client -> newer server will still work.

The downside is there will be a mismatch in names. If we think this is big enough of a downside I could make Kibana support kbn-system-api and kbn-system-request on the server-side through 7.x and remove support for kbn-system-api in 8.0.

Thoughts?

mshustov · 2020-01-17T06:57:00Z

The downside is there will be a mismatch in names. If we think this is big enough of a downside I could make Kibana support kbn-system-api and kbn-system-request on the server-side through 7.x and remove support for kbn-system-api in 8.0.

sounds reasonable

kibanamachine · 2020-01-23T22:40:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8400e36

History

💔 Build #21990 failed b465da8
💔 Build #21730 failed 8abf6e661b17cdc98722e9f3f1ef1276ca34616f
💚 Build #21171 succeeded 38b372bc8ec674984d0328427e8165a37e329012
💔 Build #21147 failed a4b43c3ce0daf0441169a0cf79016023004b0c07
💔 Build #21121 failed 6636f1dbd71d300e24174bc0b0eda91fb9fadcf8

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

joshdover added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Dec 20, 2019

joshdover requested a review from a team as a code owner December 20, 2019 21:51

joshdover force-pushed the np/system-api branch from 831ae85 to c36f576 Compare December 22, 2019 01:52

elastic deleted a comment from elasticmachine Dec 22, 2019

mshustov reviewed Dec 24, 2019

View reviewed changes

joshdover mentioned this pull request Dec 27, 2019

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

Merged

7 tasks

joshdover force-pushed the np/system-api branch from c36f576 to 2b1a6fc Compare December 28, 2019 00:19

joshdover requested a review from a team as a code owner December 28, 2019 00:19

joshdover force-pushed the np/system-api branch from 2b1a6fc to 82f8de7 Compare December 28, 2019 00:37

joshdover force-pushed the np/system-api branch from 82f8de7 to 29d03bf Compare December 30, 2019 17:14

joshdover requested a review from a team December 30, 2019 18:57

joshdover requested a review from a team as a code owner December 30, 2019 18:57

joshdover force-pushed the np/system-api branch from e6b3f9c to 9bb0b78 Compare December 30, 2019 20:55

streamich approved these changes Dec 30, 2019

View reviewed changes

mshustov reviewed Dec 31, 2019

View reviewed changes

src/core/public/http/fetch.ts Outdated Show resolved Hide resolved

azasypkin approved these changes Jan 2, 2020

View reviewed changes

src/core/public/http/types.ts Outdated Show resolved Hide resolved

mshustov mentioned this pull request Jan 8, 2020

[New platform] Security plugin integration #33775

Closed

34 tasks

pgayvallet reviewed Jan 9, 2020

View reviewed changes

joshdover force-pushed the np/system-api branch from 9bb0b78 to b8f8522 Compare January 17, 2020 00:15

joshdover added 16 commits January 22, 2020 14:17

Update API docs

238e5ed

PR comments

c953041

Refactor request interceptors to modify HttpFetchOptions

6cbdd4e

Update API docs

f8f5218

Fix removal of KbnHeaders constant

cde396a

Fix tests and types

165f93a

Add MaybePromise to @kbn/utility-types

4169419

PR comments

4316fe2

Rename to isSystemRequest

1fa4e51

Add better runtime validation of overloaded arguments

c17dd38

Rename interceptor error types

3a11c8a

Fix types

5341df2

Un-deprecate

9a7e133

Fix types

49ec443

Make fetch throw if kbn- headers are present in fetch options

39030af

Add smoke tests

b465da8

joshdover force-pushed the np/system-api branch from 8abf6e6 to b465da8 Compare January 22, 2020 21:17

joshdover added v7.7.0 and removed v7.6.0 labels Jan 22, 2020

joshdover added 2 commits January 23, 2020 14:13

Update API docs

758c427

Port usages of kbn-system-api via kfetch to asSystemRequest

8400e36

joshdover merged commit 1bb59af into elastic:master Jan 24, 2020

joshdover deleted the np/system-api branch January 24, 2020 22:20

joshdover mentioned this pull request Jan 24, 2020

[7.x] Add isSystemRequest support to Kibana Platform (#53734) #55927

Merged

joshdover added a commit to joshdover/kibana that referenced this pull request Jan 28, 2020

Add isSystemRequest support to Kibana Platform (elastic#53734)

22f0746

joshdover added a commit that referenced this pull request Jan 28, 2020

Add isSystemRequest support to Kibana Platform (#53734) (#55927)

5bbf2cf

This was referenced Feb 7, 2020

Idle sessions never expire #57119

Closed

Fix detection of "system requests" in plugins #57149

Merged

Conversation

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

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Dec 20, 2019

Uh oh!

kibanamachine commented Dec 22, 2019

💔 Build Failed

History

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Dec 28, 2019

💔 Build Failed

Uh oh!

kibanamachine commented Dec 28, 2019

💔 Build Failed

Uh oh!

kibanamachine commented Dec 30, 2019

💔 Build Failed

Uh oh!

kibanamachine commented Dec 30, 2019

💔 Build Failed

Uh oh!

streamich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joshdover commented Jan 17, 2020

Uh oh!

mshustov commented Jan 17, 2020

Uh oh!

kibanamachine commented Jan 23, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

joshdover commented Dec 20, 2019 •

edited

Loading