Support space-specific default routes by legrego · Pull Request #44678 · elastic/kibana

legrego · 2019-09-03T15:52:04Z

Summary

This deprecates the server.defaultRoute kibana.yml setting (#46787) in favor of an Advanced Setting controllable through the UI. By making this an advanced setting, it inherently becomes space-aware, so users can specify a custom default route per space.

Transition

If server.defaultRoute is specified in kibana.yml, it will be mapped to the uiSettings.overrides.defaultRoute setting. This setting tells the UI Settings Service that the defaultRoute setting is locked, and cannot be edited via the UI. From a migration perspective, this is functionally equivalent for users upgrading in a minor release: the setting is only controllable via the yml file.

Functionality

Users who wish to take advantage of the space-aware routes simply need to remove the server.defaultRoute setting from their yml file, if set. If unset, the advanced setting defaults to /app/kibana, which was the previous default value for server.defaultRoute.

If the specified default route (via advanced settings) is invalid (must be a relative path), then the "fallback" default route will be used, which is the advanced setting default of /app/kibana.

Caveats

This PR does not address users with insufficient access. If a default route is set for a space, and the user does not have access to that route, then the page will load with an error message.
This PR does not address default routes which point to disabled features or non-existent locations. Users will be met with an error message.

I think these caveats are acceptable for now, because both of these conditions are possible today using just server.defaultRoute, and the security team has roadmap items to address these conditions in the future.

Examples

No server.defaultRoute set. Default behavior:

# Unset default route
# server.defaultRoute: '/app/kibana'

Set server.defaultRoute. Locked down behavior:

server.defaultRoute: '/app/canvas'

Resolves #26126

…rvice

… functionality

elasticmachine · 2019-09-03T21:11:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

fbaligand · 2019-09-04T04:15:19Z

I would love this feature!

legrego · 2019-09-04T11:37:47Z

@kobelb
@elastic/kibana-platform

I'm looking for feedback on the general approach here (see description above for details). If we're ok with the general approach, I'll start splitting this out and tagging for reviews as appropriate. Thanks!

elasticmachine · 2019-09-04T11:38:17Z

Pinging @elastic/kibana-security

…tRoute

elasticmachine · 2019-09-06T14:16:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-07T03:46:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kobelb · 2019-09-11T23:49:05Z

ACK: Will be reviewing first-thing tomorrow, time got away from me today.

elasticmachine · 2019-09-12T01:15:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kobelb

Forcing validation on consumption of the defaultRoute uiSetting seems rather fragile. This requires us to include this logic in src/legacy/server/http/index.js and then in X-Pack. Additionally, I think that we should be enforcing stricter validation on the uiSettings defaultRoute than we did in the kibana.yml. The kibana.yml is only writable by true systems administrators, while the ui setting is writable by anyone. To protect against "open redirect" issues with the login screen's next query-string parameter, we do the following, which ideally we'd be doing here as well:

kibana/x-pack/legacy/plugins/security/public/lib/parse_next.ts

Lines 24 to 37 in 7291e44

    
           const { protocol, hostname, port, pathname } = parse( 
        
             next, 
        
             false /* parseQueryString */, 
        
             true /* slashesDenoteHost */ 
        
           ); 
        
           // We should explicitly compare `protocol`, `port` and `hostname` to null to make sure these are not 
        
           // detected in the URL at all. For example `hostname` can be empty string for Node URL parser, but 
        
           // browser (because of various bwc reasons) processes URL differently (e.g. `///abc.com` - for browser 
        
           // hostname is `abc.com`, but for Node hostname is an empty string i.e. everything between schema (`//`) 
        
           // and the first slash that belongs to path. 
        
           if (protocol !== null || hostname !== null || port !== null) { 
        
             return `${basePath}/`; 
        
           }

If the ui settings themselves allowed us to provide a custom validation, this would be hugely helpful. If we enforced this same validation when the values are being read, and potentially fell back to the default if it's invalid, it'd make this much easier. I explored adding server-side validation to the ui settings when implementing #15342 but it was pulled out for reasons I decided to exclude from the PR...

As a potential aside, I don't think that we should be adding a defaultRoute method to the SpacesClient. Before the addition of this method, the SpacesClient is only responsible for enforcing the business logic for accessing Spaces themselves. Adding default route logic to the SpacesClient doesn't really fit the responsibilities of this class. It also forces us to make changes so that we can pass the namespace to the uiSettingsService, as opposed to relying on the space restrictions that the default wrapped SOC enforces.

kobelb · 2019-09-12T18:37:43Z

src/legacy/server/config/transform_deprecations.js


 const deprecations = [
  //server
+  rename('server.defaultRoute', 'uiSettings.overrides.defaultRoute'),


TIL about uiSettings.overrides. I really like how you're taking advantage of this.

kobelb · 2019-09-12T18:37:53Z

src/legacy/server/config/transform_deprecations.test.js

      });
    });

+    describe('server.defaultRoute', () => {


…tRoute

Co-Authored-By: Brandon Kobel <brandon.kobel@gmail.com>

…spaces/defaultRoute

elasticmachine · 2019-10-01T14:01:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-10-01T15:20:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-10-01T18:08:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kobelb

This is working great! The additional validation is fantastic.

elasticmachine · 2019-10-01T19:19:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

joshdover

Code LGTM.

One UX suggestion: when the server.defaultRoute setting is specified in the kibana.yml, it might be clearer if the text in Advanced Settings explained which config field overrides this value. Not sure how much work it would be customize that message though.

legrego · 2019-10-02T16:01:33Z

One UX suggestion: when the server.defaultRoute setting is specified in the kibana.yml, it might be clearer if the text in Advanced Settings explained which config field overrides this value. Not sure how much work it would be customize that message though.

I like the idea, but I do think it'd be quite a bit of work to do this correctly

kumar716 · 2022-03-09T07:18:06Z

Hi, in Kibana UI (kibana version 7.6.2) have changed the defaultRoute from /app/kibana to /app/canvas
-- but accessing the url via https://host:port/app/canvas is resulting in this message on the screen "Application Not Found
No application was found at this URL. Try going back or choosing an app from the menu."
-- & while accessing url via https://host:port I get the proper landing page but the url is defaulted to https://host:port/app/kibana instead of https://host:port/app/canvas...
-> Can you please help me achieve this path/Route/uri modification to /app/canvas as this is our requirement, how to achieve this Please

legrego · 2022-03-09T12:00:47Z

Hi @kumar716, this isn't the appropriate forum for troubleshooting. Please start a topic on our discussion boards describing your problem, and we'll be more than happy to help you out there.

It's hard to say what the problem is without seeing more information. I'd ask to double check your user's privileges (if security is enabled), and to ensure that the Canvas application hasn't been disabled in your current space.

kumar716 · 2022-03-09T12:19:18Z

thanks @legrego , opening a thread in the discussion boards

legrego added 9 commits September 3, 2019 10:41

partial POC

38e12db

allow for cross-space retrieval of UI Settings via the UI Settings Se…

7064841

…rvice

adding ui settings service tests

9cc3995

spaces service tests

6ef6b24

updating API tests

f7c882a

make setting deprecation a rename operation to better reflect current…

8ddc0a9

… functionality

doc updates

b038587

simplify fallback route handling

4116857

cleanup

3ed43d0

This comment has been minimized.

Sign in to view

legrego added 2 commits September 3, 2019 13:50

pass namespace to getUpgradeableConfig

2162f2e

fixing mocha unit tests

f47326d

legrego added discuss Feature:Security/Spaces Platform Security - Spaces feature Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v7.5.0 v8.0.0 labels Sep 4, 2019

legrego added the release_note:enhancement label Sep 4, 2019

kobelb requested review from a team and kobelb September 4, 2019 15:19

Merge branch 'master' of github.com:elastic/kibana into spaces/defaul…

b1ac207

…tRoute

kobelb reviewed Sep 12, 2019

View reviewed changes

legrego and others added 4 commits October 1, 2019 07:46

Merge branch 'master' of github.com:elastic/kibana into spaces/defaul…

03b4a55

…tRoute

Update x-pack/test/functional/page_objects/space_selector_page.js

2620057

Co-Authored-By: Brandon Kobel <brandon.kobel@gmail.com>

moving non-common code from common to public

cf01b1c

Merge branch 'spaces/defaultRoute' of github.com:legrego/kibana into …

7fa1aee

…spaces/defaultRoute

fixing jest tests

d3a83ce

legrego added the release_note:deprecation label Oct 1, 2019

adding client-side validation to configured ui settings

bd43645

update snapshot

07160c4

legrego mentioned this pull request Oct 1, 2019

Spaces - server-side to NP plugin #46181

Merged

7 tasks

kobelb approved these changes Oct 1, 2019

View reviewed changes

legrego requested a review from joshdover October 1, 2019 21:32

joshdover approved these changes Oct 2, 2019

View reviewed changes

legrego merged commit 0bfa7ca into elastic:master Oct 2, 2019

legrego deleted the spaces/defaultRoute branch October 2, 2019 16:05

legrego mentioned this pull request Oct 2, 2019

[7.x] Support space-specific default routes (#44678) #47134

Merged

legrego added a commit to legrego/kibana that referenced this pull request Oct 2, 2019

Support space-specific default routes (elastic#44678)

c8ae91c

legrego added a commit that referenced this pull request Oct 2, 2019

[7.x] Support space-specific default routes (#44678) (#47134)

8fb34c0

legrego mentioned this pull request Nov 18, 2019

[Docs] - adding defaultRoute setting docs #50952

Merged

peterpramb mentioned this pull request Dec 3, 2019

Config setting 'server.defaultRoute' ignored #52076

Closed

mshustov mentioned this pull request Dec 5, 2019

make defaultRoute accessible in NP Config #52308

Merged

3 tasks

flash1293 mentioned this pull request Dec 24, 2019

Ability to specify arbitrary default app #52254

Closed

mshustov mentioned this pull request Jan 13, 2020

uiSettings - use validation field for image field maxSize #54522

Merged

4 tasks

	const { protocol, hostname, port, pathname } = parse(
	next,
	false /* parseQueryString */,
	true /* slashesDenoteHost */
	);

	// We should explicitly compare `protocol`, `port` and `hostname` to null to make sure these are not
	// detected in the URL at all. For example `hostname` can be empty string for Node URL parser, but
	// browser (because of various bwc reasons) processes URL differently (e.g. `///abc.com` - for browser
	// hostname is `abc.com`, but for Node hostname is an empty string i.e. everything between schema (`//`)
	// and the first slash that belongs to path.
	if (protocol !== null \|\| hostname !== null \|\| port !== null) {
	return `${basePath}/`;
	}

Conversation

legrego commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Transition

Functionality

Caveats

Examples

Uh oh!

This comment has been minimized.

elasticmachine commented Sep 3, 2019

💚 Build Succeeded

Uh oh!

fbaligand commented Sep 4, 2019

Uh oh!

legrego commented Sep 4, 2019

Uh oh!

elasticmachine commented Sep 4, 2019

Uh oh!

elasticmachine commented Sep 6, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Sep 7, 2019

💔 Build Failed

Uh oh!

kobelb commented Sep 11, 2019

Uh oh!

elasticmachine commented Sep 12, 2019

💔 Build Failed

Uh oh!

kobelb left a comment

Choose a reason for hiding this comment

Uh oh!

kobelb Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

kobelb Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 1, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 1, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Oct 1, 2019

💔 Build Failed

Uh oh!

kobelb left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 1, 2019

💚 Build Succeeded

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

legrego commented Oct 2, 2019

Uh oh!

kumar716 commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legrego commented Mar 9, 2022

Uh oh!

kumar716 commented Mar 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

legrego commented Sep 3, 2019 •

edited

Loading

kumar716 commented Mar 9, 2022 •

edited

Loading