elastic / kibana

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

Space Selector should show the global banner instead of the default space banner #140309

Open wayneseymour opened 2 years ago

wayneseymour commented 2 years ago

Kibana version: latest main

Elasticsearch version: latest main

Server OS version:
Mac OS X

Browser version: Latest MSFT Edge

Browser OS version:

Original install method (e.g. download page, yum, from source, etc.): from source Describe the bug: Space Selector shows the global banner set via kbn server args:

    kbnTestServer: {
      ...kibanaFunctionalConfig.get('kbnTestServer'),
      serverArgs: [
        ...kibanaFunctionalConfig.get('kbnTestServer.serverArgs'),
        '--xpack.banners.placement=top',
        '--xpack.banners.textContent="global banner text"',
      ],
    },

Steps to reproduce:

  1. Set "global" banner text via server args as shown above
  2. Create a space, with or w/o it's own banner text
  3. Notice the top banner is not the "global" text, it's the text from the default space

Expected behavior: The global banner should be shown, not the banner from the default space Screenshots (if relevant):

Screenshot 2022-09-08 at 16 57 55

Errors in browser console (if relevant):

Provide logs and/or server output (if relevant):

Any additional context: Affects: https://github.com/elastic/kibana/pull/135783

elasticmachine commented 2 years ago

Pinging @elastic/kibana-security (Team:Security)

legrego commented 2 years ago

@elastic/kibana-core, how do the space & global banners get injected into apps? The space selector screen is registered as a "chromeless" app, but otherwise looks fairly unremarkable in terms of how it registers and renders itself:

https://github.com/elastic/kibana/blob/7360c1dc78901ad6de2559cb3b1c36023fae8e9b/x-pack/plugins/spaces/public/space_selector/space_selector_app.tsx#L19-L45

Is there a way for us to instruct Kibana to prefer one banner over the other?

pgayvallet commented 2 years ago

@legrego even if on the space selector screen, this issue's on us.

pgayvallet commented 2 years ago

Long story short, the global vs space-aware banner content is selected depending on whether the user is authenticated or not.

https://github.com/elastic/kibana/blob/a02c00b8a3e66cc6117aba87b9d64273302f4d29/x-pack/plugins/banners/server/routes/info.ts#L26-L29

And the space selector is within the authenticated area.

https://github.com/elastic/kibana/blob/a02c00b8a3e66cc6117aba87b9d64273302f4d29/x-pack/plugins/spaces/server/routes/views/index.ts#L20-L23

Therefor, given the initial specifications of the banner plugin, this is working as intended (:tm:).

However, I agree that, ideally, it would make more sense for the global banner to appear here rather than the banner for the space the screen is displayed from.

Recategorising as enhancement.

legrego commented 2 years ago

Long story short, the global vs space-aware banner content is selected depending on whether the user is authenticated or not.

And the space selector is within the authenticated area.

Ah, this is an interesting distinction, thanks for explaining. The Space Selector is a bit of an odd-ball because while it is authenticated, it doesn't exist within any one space. It's possible for a user on this page to not have privileges to the Default Space, so it feels a bit incorrect to show them the banner message from that space.

pgayvallet commented 2 years ago

so it feels a bit incorrect to show them the banner message from that space.

Yea, totally agree with that

It's possible for a user on this page to not have privileges to the Default Space

🙈 given technically on this page the user will be bound to the said default space. Also the space selector page can't really be (technically) displayed without being authenticated given we need to resolve the list of spaces the user is authorized to access...

This is tricky... Not sure how to easily address this without adding a concept of 'space-free' zone/app and/or without having Core somehow aware of the banner feature.

We could have the request performed against /api/banners/info to add either the current path or the currently displayed app... but that would kinda only work with apps performing full page reload, so it sounds like just adding more tech debt for when we'll decide to try to move to a full SPA navigation even for the login and space selector pages (and I'm not even sure we always page reload when the user is redirected to the space selector app, are we?)...

lukeelmers commented 2 years ago

We could have the request performed against /api/banners/info to add either the current path or the currently displayed app... but that would kinda only work with apps performing full page reload, so it sounds like just adding more tech debt for when we'll decide to try to move to a full SPA navigation even for the login and space selector pages (and I'm not even sure we always page reload when the user is redirected to the space selector app, are we?)...

So we could take this approach to hack together a short-term solution, but the question is how important we think it is to have this resolved / what we see as the impact of this behavior, especially since it is only evident when using both global banners and space banners.

@arisonl and/or @alexfrancoeur - any thoughts on the level of impact for this & whether we know of any customers for whom it would be a major issue?

pgayvallet commented 2 years ago

Hum, @wayneseymour I think this was wrongly closed?

elasticmachine commented 1 year ago

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