Gracefully disable favorites if profile is not available #204397

Dosant · 2024-12-16T13:39:44Z

Summary

When a user profile is not available, the favorites (starred) service can't be used. On UI user profile can be not available if security is disabled or for an anonymous user.

This PR improves the handling of starred features for rare cases when a profile is missing:

No unnecessary GET favorites requests that would fail with error and add noise to console/networks
No unhandled errors are thrown
Starred tab in esql is hidden
The Dashboard Starred tab isn't flickering on each attempt to fetch favorites

For this needed to expose userProfile.enabled$ from core, also created #204570

Testing

node scripts/functional_tests_server.js --config test/functional/apps/dashboard/group4/config.ts

localhost:5620

another way is by configuring an anonymous user https://www.elastic.co/guide/en/elasticsearch/reference/current/anonymous-access.html

packages/content-management/favorites/favorites_public/src/favorites_client.ts

…kibana into d/2024-12-16-favorites-edge-case

elasticmachine · 2024-12-17T17:01:06Z

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

packages/core/user-profile/core-user-profile-browser/src/service.ts

Dosant · 2024-12-17T17:02:41Z

packages/core/user-profile/core-user-profile-browser/src/api_provider.ts

  userProfile$: Observable<UserProfileData | null>;
+  enabled$: Observable<boolean>;


Exposing enabled$ from core

Dosant · 2024-12-17T17:06:52Z

src/platform/packages/private/kbn-esql-editor/src/editor_footer/history_starred_queries.tsx

@@ -470,22 +470,30 @@ export function HistoryAndStarredQueriesTabs({
  const kibana = useKibana<ESQLEditorDeps>();
  const { core, usageCollection, storage } = kibana.services;

-  const [starredQueriesService, setStarredQueriesService] = useState<EsqlStarredQueriesService>();
+  const [starredQueriesService, setStarredQueriesService] = useState<
+    EsqlStarredQueriesService | null | undefined


adding null state for tracking when starredQueriesService couldn't initialize and use it to hide the "starred" tab. intentionally showing "starred" tab during undefined (loading) state because it is the most common scenario and we can avoid a content flash

afharo

Core changes LGTM. Code review only.

packages/core/user-profile/core-user-profile-browser/src/service.ts

stratoula · 2024-12-18T07:09:24Z

@Dosant can you provide steps on how to test it?

…ce.ts Co-authored-by: Alejandro Fernández Haro <[email protected]>

Dosant · 2024-12-18T14:47:51Z

@stratoula, imo the simplest would be to run any functional tests server from config from src/ since security is not enabled there.

e.g.

node scripts/functional_tests_server.js --config test/functional/apps/dashboard/group4/config.ts

localhost:5620

another way is by configuring anonymous user https://www.elastic.co/guide/en/elasticsearch/reference/current/anonymous-access.html

stratoula

ES|QL changes LGTM, did a brief testing locally to ensure that it works as expected

azasypkin

LGTM from the Platform Security perspective. Tested locally using anonymous access - favorite controls aren’t visible as expected, and the UI isn’t broken. However, I wish we could avoid throwing an exception when the profile isn’t available, as it’s a normal expected flow for certain configurations (adding a warn favorites-specific log line would be much better, as having all user profile consumers throw errors could create a misleading perception).

Dosant · 2024-12-19T12:54:52Z

However, I wish we could avoid throwing an exception when the profile isn’t available, as it’s a normal expected flow for certain configurations (adding a warn favorites-specific log line would be much better, as having all user profile consumers throw errors could create a misleading perception).

@azasypkin, thanks, I'll double-check. I might have missed something when was changing the check to enabled$

Update: so it works without an error in console for ESQL favorites, but it becomes quite a bit large change to fix this in dashboard because logic to hide Starred tab relies on a thrown error and it is default react-query behaviour to log an error even when it is handled.
I'd like to keep it like this

…vorites-edge-case

eokoneyo · 2024-12-30T11:15:32Z

...platform/packages/private/kbn-esql-editor/src/editor_footer/esql_starred_queries_service.tsx

+    const isAvailable = await client.isAvailable();
+    if (!isAvailable) return null;


is it worth extending this conditional check here to the dashboard list implementation to determine if favorites is enabled here https://github.com/elastic/kibana/blob/v8.17.0/packages/content-management/table_list_view_table/src/services.tsx#L278, especially that when we navigate to the dashboard listing page at attempt is still made to request favorites?

I think it would be ideal, but that check is async so I wanted to avoid additional changes.
I will take another look

I just changed that isFavoritesEnabled to allow for Promise

Also fixes @azasypkin's suggestion: #204397 (review)

eokoneyo

Tested this, UI to star entities doesn't get displayed for both ESQL and dashboard listing when the user profile isn't available, however for dashboard listing an ~~request~~ attempt is still being made to fetch the user's favourites.

verified using the following config to kibana.dev.yml,

xpack.security.authc.providers:
  basic.basic1:
    order: 0
  anonymous.anonymous1:
    order: 1
    credentials:
      username: "elastic"
      password: "changeme"

…vorites-edge-case

elasticmachine · 2024-12-30T15:34:31Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`eventAnnotationListing`	619	622	+3
`filesManagement`	178	181	+3
`graph`	290	293	+3
`maps`	1248	1250	+2
`visualizations`	473	476	+3
total			+14

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/content-management-favorites-public`	42	45	+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	666.3KB	666.6KB	+336.0B
`esql`	202.6KB	203.1KB	+529.0B
`eventAnnotationListing`	229.7KB	231.0KB	+1.3KB
`filesManagement`	121.0KB	122.3KB	+1.3KB
`graph`	413.0KB	414.3KB	+1.3KB
`maps`	3.0MB	3.0MB	+1.1KB
`visualizations`	320.7KB	322.0KB	+1.3KB
total			+7.2KB

Page load bundle

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

id	before	after	diff
`core`	443.9KB	444.0KB	+53.0B
`filesManagement`	3.9KB	4.0KB	+48.0B
total			+101.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/content-management-favorites-public`	43	46	+3
`@kbn/core-user-profile-browser`	29	30	+1
total			+4

History

💚 Build #262126 succeeded e112029
💛 Build #261890 was flaky 17e5354
💚 Build #261513 succeeded deda9d0
💔 Build #261385 failed ac8f87d
💔 Build #261379 failed 430d65e
💔 Build #260886 failed 3ae2a13

kibanamachine · 2024-12-30T15:36:11Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12548645226

) ## Summary When a user profile is not available, the favorites (starred) service can't be used. On UI user profile can be not available if security is disabled or for an anonymous user. This PR improves the handling of starred features for rare cases when a profile is missing: - No unnecessary `GET favorites` requests that would fail with error and add noise to console/networks - No unhandled errors are thrown - Starred tab in esql is hidden - The Dashboard Starred tab isn't flickering on each attempt to fetch favorites For this needed to expose `userProfile.enabled$` from core, also created elastic#204570 ### Testing ``` node scripts/functional_tests_server.js --config test/functional/apps/dashboard/group4/config.ts localhost:5620 ``` another way is by configuring an anonymous user https://www.elastic.co/guide/en/elasticsearch/reference/current/anonymous-access.html (cherry picked from commit 70cf414)

kibanamachine · 2024-12-30T15:41:19Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…) (#205270) # Backport This will backport the following commits from `main` to `8.x`: - [Gracefully disable favorites if profile is not available (#204397)](#204397)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Anton Dosov <[email protected]>

) ## Summary When a user profile is not available, the favorites (starred) service can't be used. On UI user profile can be not available if security is disabled or for an anonymous user. This PR improves the handling of starred features for rare cases when a profile is missing: - No unnecessary `GET favorites` requests that would fail with error and add noise to console/networks - No unhandled errors are thrown - Starred tab in esql is hidden - The Dashboard Starred tab isn't flickering on each attempt to fetch favorites For this needed to expose `userProfile.enabled$` from core, also created elastic#204570 ### Testing ``` node scripts/functional_tests_server.js --config test/functional/apps/dashboard/group4/config.ts localhost:5620 ``` another way is by configuring an anonymous user https://www.elastic.co/guide/en/elasticsearch/reference/current/anonymous-access.html

Dosant and others added 3 commits December 16, 2024 14:30

improve

15a5a91

[CI] Auto-commit changed files from 'node scripts/notice'

7ceef29

fix unit tests

05aac0a

Dosant commented Dec 16, 2024

View reviewed changes

packages/content-management/favorites/favorites_public/src/favorites_client.ts Outdated Show resolved Hide resolved

kibanamachine and others added 4 commits December 16, 2024 14:21

[CI] Auto-commit changed files from 'node scripts/notice'

3ae2a13

improve

fd0d829

Merge branch 'd/2024-12-16-favorites-edge-case' of github.com:Dosant/…

05b8f14

…kibana into d/2024-12-16-favorites-edge-case

improve to rely on userprofile

430d65e

Dosant changed the title ~~improve~~ Gracefull disable favorites if profile is not available Dec 17, 2024

kibanamachine and others added 3 commits December 17, 2024 13:13

[CI] Auto-commit changed files from 'node scripts/notice'

e18ca33

fix

3d7324c

Merge branch 'd/2024-12-16-favorites-edge-case' of github.com:Dosant/…

ac8f87d

…kibana into d/2024-12-16-favorites-edge-case

Dosant changed the title ~~Gracefull disable favorites if profile is not available~~ Gracefully disable favorites if profile is not available Dec 17, 2024

Dosant mentioned this pull request Dec 17, 2024

[UserProfileClient] Improve userProfile$ by removing userProfileLoaded$ #204570

Open

update core snapshots

fec5a6a

Dosant added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 17, 2024

Dosant marked this pull request as ready for review December 17, 2024 17:01

Dosant requested review from a team as code owners December 17, 2024 17:01

Dosant requested review from azasypkin and stratoula December 17, 2024 17:01

Dosant commented Dec 17, 2024

View reviewed changes

cleanup

deda9d0

ThomThomson approved these changes Dec 17, 2024

View reviewed changes

afharo approved these changes Dec 17, 2024

View reviewed changes

packages/core/user-profile/core-user-profile-browser/src/service.ts Outdated Show resolved Hide resolved

Update packages/core/user-profile/core-user-profile-browser/src/servi…

17e5354

…ce.ts Co-authored-by: Alejandro Fernández Haro <[email protected]>

Merge branch 'main' into d/2024-12-16-favorites-edge-case

e112029

stratoula approved these changes Dec 19, 2024

View reviewed changes

azasypkin approved these changes Dec 19, 2024

View reviewed changes

Merge branch 'main' of github.com:elastic/kibana into d/2024-12-16-fa…

3dd2c5e

…vorites-edge-case

eokoneyo reviewed Dec 30, 2024

View reviewed changes

eokoneyo approved these changes Dec 30, 2024

View reviewed changes

Dosant added 3 commits December 30, 2024 14:04

Merge branch 'main' of github.com:elastic/kibana into d/2024-12-16-fa…

e736155

…vorites-edge-case

improve favorites check, but make it async

ed10472

improve codestyle

ab54a2b

Dosant merged commit 70cf414 into elastic:main Dec 30, 2024
9 checks passed

kibanamachine added the v9.0.0 label Dec 30, 2024

kibanamachine mentioned this pull request Dec 30, 2024

[8.x] Gracefully disable favorites if profile is not available (#204397) #205270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gracefully disable favorites if profile is not available #204397

Gracefully disable favorites if profile is not available #204397

Dosant commented Dec 16, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Dec 17, 2024

Dosant Dec 17, 2024

Dosant Dec 17, 2024

afharo left a comment

stratoula commented Dec 18, 2024

Dosant commented Dec 18, 2024 •

edited

Loading

stratoula left a comment

azasypkin left a comment •

edited

Loading

Dosant commented Dec 19, 2024 •

edited

Loading

eokoneyo Dec 30, 2024

Dosant Dec 30, 2024

Dosant Dec 30, 2024

Dosant Dec 30, 2024

eokoneyo left a comment •

edited

Loading

elasticmachine commented Dec 30, 2024

API count

kibanamachine commented Dec 30, 2024

kibanamachine commented Dec 30, 2024

		userProfile$: Observable<UserProfileData \| null>;
		enabled$: Observable<boolean>;

		const isAvailable = await client.isAvailable();
		if (!isAvailable) return null;

Gracefully disable favorites if profile is not available #204397

Gracefully disable favorites if profile is not available #204397

Conversation

Dosant commented Dec 16, 2024 • edited by kibanamachine Loading

Summary

Testing

elasticmachine commented Dec 17, 2024

Dosant Dec 17, 2024

Choose a reason for hiding this comment

Dosant Dec 17, 2024

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

stratoula commented Dec 18, 2024

Dosant commented Dec 18, 2024 • edited Loading

stratoula left a comment

Choose a reason for hiding this comment

azasypkin left a comment • edited Loading

Choose a reason for hiding this comment

Dosant commented Dec 19, 2024 • edited Loading

eokoneyo Dec 30, 2024

Choose a reason for hiding this comment

Dosant Dec 30, 2024

Choose a reason for hiding this comment

Dosant Dec 30, 2024

Choose a reason for hiding this comment

Dosant Dec 30, 2024

Choose a reason for hiding this comment

eokoneyo left a comment • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Dec 30, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

kibanamachine commented Dec 30, 2024

kibanamachine commented Dec 30, 2024

💚 All backports created successfully

Questions ?

Dosant commented Dec 16, 2024 •

edited by kibanamachine

Loading

Dosant commented Dec 18, 2024 •

edited

Loading

azasypkin left a comment •

edited

Loading

Dosant commented Dec 19, 2024 •

edited

Loading

eokoneyo left a comment •

edited

Loading