Make the Chrome project API "protected" for the Serverless plugin #157307

tsullivan · 2023-05-10T17:43:11Z

Summary

Let's think if there is a way to throw an error when core chrome api are called from invalid plugins (in this cases only the serverless plugin would be allowed.

This PR can be a starting point for discussion on the behavior we really want. This PR has a simple goal to ensure that non-serverless plugins do not call the chrome.projects API. However, it's not complete security, as the compile-time error would be easy to override.

cc @sebelga @Dosant @clintandrewhall

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Internal documentation: https://docs.google.com/document/d/1ew8KYl6ROs_V7jeIXgeP_C9YgkYK2IPtuceo6KVF_jE/edit#

tsullivan · 2023-05-10T17:46:46Z

x-pack/plugins/serverless_search/public/plugin.ts

  ): ServerlessSearchPluginStart {
-    core.chrome.project.setSideNavComponent(createComponent(core));
+    serverless.setSideNavComponent(createComponent(core));


cc @sphilipse @kpatticha

elasticmachine · 2023-05-11T20:02:37Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

clintandrewhall

Great work.

clintandrewhall · 2023-05-15T19:43:10Z

x-pack/plugins/serverless/public/plugin.tsx

@@ -64,7 +63,8 @@ export class ServerlessPlugin

    return {
      setSideNavComponent: (sideNavigationComponent) =>
-        core.chrome.project.setSideNavComponent(sideNavigationComponent),
+        // Casting the "chrome.projects" service to an "internal" type: it's intentional to call the service only from here.
+        (core.chrome as InternalChromeStart).project.setSideNavComponent(sideNavigationComponent),


We should couple this with a throw on these methods if chromeStyle is not set properly.

x-pack/plugins/serverless/public/plugin.tsx

clintandrewhall · 2023-05-15T19:47:24Z

x-pack/plugins/serverless/public/plugin.tsx

@@ -64,7 +63,8 @@ export class ServerlessPlugin

    return {
      setSideNavComponent: (sideNavigationComponent) =>


nit: I've always disliked abbreviations, (and Component feels redundant, as the parameter is a React component)... perhaps we could consider setSidebarNavigation...?

I agree w/ you, but serverless.setSideNavComponent already has presence in the 3 serverless project plugins. Let's find a new name at our next sync and change this in a standalone PR.

setSidebarNavigation() is reserved (™) and will be used when there is no need to provide a component (UI) (define the navigation through an object - see first example in Proposal #157702).
Well actually it will even be shorter: setNavigation() 😊

I do think "Component" is explicit of the intent. It could be "UI" suffix instead.

Co-authored-by: Clint Andrew Hall <clint@clintandrewhall.com>

…otected

kibana-ci · 2023-05-15T23:29:12Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: a012fdf

Failed CI Steps

FTR Configs #12

Test Failures

[job] [logs] FTR Configs #12 / home app add data tutorials directory should redirect to integrations app

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	365.1KB	365.4KB	+314.0B
`serverless`	5.0KB	5.1KB	+10.0B
`serverlessSearch`	23.3KB	23.3KB	-2.0B
total			+322.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-chrome-browser`	136	135	-1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	400	404	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	480	484	+4
total			+6

History

💚 Build #128026 succeeded e065ece
💚 Build #127267 succeeded 060f94a
💚 Build #126951 succeeded 26e462ce91b82572a781785aa4971d4dd07a12e0

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

TattdCodeMonkey

LGTM

@sebelga

…57307) ## Summary Addresses #156600 (comment) > Let's think if there is a way to throw an error when core chrome api are called from invalid plugins (in this cases only the serverless plugin would be allowed. This PR can be a starting point for discussion on the behavior we really want. This PR has a simple goal to ensure that non-serverless plugins do not call the `chrome.projects` API. However, it's not complete security, as the compile-time error would be easy to override. cc @sebelga @Dosant @clintandrewhall --- --- ### Checklist Delete any items that are not applicable to this PR. - [x] Documentation was added for features that require explanation or tutorials Internal documentation: https://docs.google.com/document/d/1ew8KYl6ROs_V7jeIXgeP_C9YgkYK2IPtuceo6KVF_jE/edit# --------- Co-authored-by: Clint Andrew Hall <clint@clintandrewhall.com>

tsullivan changed the title ~~Set the Chrome project API as protected for Serverless plugin~~ Make the Chrome project API as "protected for Serverless plugin May 10, 2023

tsullivan changed the title ~~Make the Chrome project API as "protected for Serverless plugin~~ Make the Chrome project API "protected for usage only by the Serverless plugin May 10, 2023

tsullivan commented May 10, 2023

View reviewed changes

tsullivan force-pushed the serverless/chrome-protected branch from 2bb0753 to 26e462c Compare May 10, 2023 18:02

Set the Chrome project API as protected for Serverless plugin

060f94a

tsullivan force-pushed the serverless/chrome-protected branch from 26e462c to 060f94a Compare May 11, 2023 19:46

tsullivan marked this pull request as ready for review May 11, 2023 19:47

tsullivan requested a review from a team as a code owner May 11, 2023 19:47

tsullivan mentioned this pull request May 11, 2023

[Observability] Add side navigation for serverless observability #157360

Merged

tsullivan added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Project:Serverless Work as part of the Serverless project for its initial release labels May 11, 2023

tsullivan requested review from kpatticha and sphilipse May 11, 2023 20:03

tsullivan changed the title ~~Make the Chrome project API "protected for usage only by the Serverless plugin~~ Make the Chrome project API "protected" for the Serverless plugin May 11, 2023

Dosant approved these changes May 15, 2023

View reviewed changes

clintandrewhall approved these changes May 15, 2023

View reviewed changes

tsullivan and others added 4 commits May 15, 2023 12:59

Update x-pack/plugins/serverless/public/plugin.tsx

73d4449

Co-authored-by: Clint Andrew Hall <clint@clintandrewhall.com>

Merge remote-tracking branch 'elastic/main' into serverless/chrome-pr…

8a32270

…otected

throw in setProject*Nav* on invalid ChromeStyle

e065ece

Merge branch 'main' into serverless/chrome-protected

a012fdf

tsullivan requested a review from a team May 15, 2023 22:24

tsullivan enabled auto-merge (squash) May 15, 2023 22:48

TattdCodeMonkey approved these changes May 16, 2023

View reviewed changes

tsullivan merged commit 4805421 into elastic:main May 16, 2023

kibanamachine added v8.9.0 backport:skip This commit does not require backporting labels May 16, 2023

tsullivan deleted the serverless/chrome-protected branch November 21, 2023 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the Chrome project API "protected" for the Serverless plugin #157307

Make the Chrome project API "protected" for the Serverless plugin #157307

tsullivan commented May 10, 2023 •

edited

Loading

tsullivan May 10, 2023

elasticmachine commented May 11, 2023

clintandrewhall left a comment

clintandrewhall May 15, 2023

tsullivan May 15, 2023 •

edited

Loading

clintandrewhall May 15, 2023

tsullivan May 15, 2023

sebelga May 16, 2023

kibana-ci commented May 15, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

TattdCodeMonkey left a comment

		@@ -64,7 +63,8 @@ export class ServerlessPlugin

		return {
		setSideNavComponent: (sideNavigationComponent) =>

Make the Chrome project API "protected" for the Serverless plugin #157307

Make the Chrome project API "protected" for the Serverless plugin #157307

Conversation

tsullivan commented May 10, 2023 • edited Loading

Summary

Checklist

tsullivan May 10, 2023

Choose a reason for hiding this comment

elasticmachine commented May 11, 2023

clintandrewhall left a comment

Choose a reason for hiding this comment

clintandrewhall May 15, 2023

Choose a reason for hiding this comment

tsullivan May 15, 2023 • edited Loading

Choose a reason for hiding this comment

clintandrewhall May 15, 2023

Choose a reason for hiding this comment

tsullivan May 15, 2023

Choose a reason for hiding this comment

sebelga May 16, 2023

Choose a reason for hiding this comment

kibana-ci commented May 15, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

TattdCodeMonkey left a comment

Choose a reason for hiding this comment

tsullivan commented May 10, 2023 •

edited

Loading

tsullivan May 15, 2023 •

edited

Loading