elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.65k stars 8.23k forks source link

[theme] unauthenticated pages does not respect the `darkMode` uiSettings #127700

Open pgayvallet opened 2 years ago

pgayvallet commented 2 years ago

We're not resolving the 'user provided' uiSettings stored in ES in the rendering service when rendering anonymous pages (via httpResources. renderAnonymousCoreApp)

This is caused by the very valid reason that we're using a scoped ES client to resolve the config SO containing the uiSettings, and that it would throw a 403 for unauthenticated users (when security is enabled)

However, this has the effect to totally ignore the darkMode UI setting, meaning that a Kibana configured with darkMode: true will still display the unauthenticated pages (such as the login page) with the light theme.

https://github.com/elastic/kibana/blob/04d57627207a6d6674df706cd48f56f11049f698/src/core/server/rendering/rendering_service.tsx#L91-L96

Example for the login page: https://github.com/elastic/kibana/blob/6ecffcc57c9a5b0328257cd25b737140a8471dda/x-pack/plugins/security/server/routes/views/login.ts#L47-L56

We should ideally respect the darkTheme uiSetting even for unauthenticated pages. Maybe we need to make an exception on how we resolve this uiSetting from the rendering service and use an uiSetting client bound to an unscoped SO client to always resolve the settings stored in ES.

Joe edit: we should also consider other pages like the Reset Session Page (x-pack/plugins/security/server/authorization/reset_session_page.tsx) and the Unauthenticated Page (x-pack/plugins/security/server/authentication/unauthenticated_page.tsx) that are both rendered server-side.

cc @elastic/kibana-security

elasticmachine commented 2 years ago

Pinging @elastic/kibana-core (Team:Core)

legrego commented 2 years ago

In general I agree that we should allow for dark mode to be respected on unauthenticated pages. I think there is a bit of a chicken and egg problem though: Dark Mode is a space-specific setting -- and "soon" to be a user-specific setting.

Unauthenticated pages don't live within any one space, and unauthenticated pages also don't know anything about the current user, because the user hasn't yet authenticated themselves.

If we wanted an approximation, we could decide that unauthenticated pages follow the preferences of the default space. For something like dark mode, this would probably be good enough, and cover a majority of our user's needs.

jportner commented 2 years ago

If we wanted an approximation, we could decide that unauthenticated pages follow the preferences of the default space. For something like dark mode, this would probably be good enough, and cover a majority of our user's needs.

That sounds reasonable to me!

pgayvallet commented 2 years ago

If we wanted an approximation, we could decide that unauthenticated pages follow the preferences of the default space.

Seems reasonable to me too. That's not like we can really do better atm anyway.

Dark Mode is a space-specific setting -- and "soon" to be a user-specific setting

Full design has yet to be decided, but it is likely that user settings will fallback to space/global setting when not specified/set for the user, so I'd say it should be fine.

legrego commented 2 years ago

Awesome, let's call this decided then

TinaHeiligers commented 2 years ago

likely that user settings will fallback to space/global setting when not specified/set for the user

That was my understanding of the settings-inheritance chain too.

TinaHeiligers commented 2 years ago

Based on the fact that at the moment, the behavior is consistent and that for unauthenticated requests, settings fall back to defaults, I don't think this is a bug, it's expected behavior right now. Should having unauthenticated requests for settings fall back to the settings for the default space be more of an enhancement request as part of the kibana personalization initiative?

pgayvallet commented 2 years ago

Based on the fact that at the moment [...] I don't think this is a bug, it's expected behavior

I won't argue over the bug vs enhancement regarding the technicals, I agree, technically it's working as intended. The fact is, customers can't configure the kibana theme with consistency. From a customer, especially one having/using a single space, POV, it's definitely a UI bug.

Now, it's still low impact for sure.

Should having unauthenticated requests for settings fall back to the settings for the default space

Note, we're speaking very specifically of the darkMode uiSetting for the internal usage within the rendering service only. Allowing access to all 'user' (terribly named, but that's the name atm for settings stored in the config SO) uiSettings (and sending them to the client via the injectedMetadata) is a whole other story which would have more security concerns ihmo (The per-space banner:textContent setting is an example).

TinaHeiligers commented 2 years ago

From a customer, especially one having/using a single space, POV, it's definitely a UI bug.

Thanks for clarifying! Since it's a bug, don't have to wait until later to act here 😄

elasticmachine commented 1 year ago

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