Route tags #37344

mshustov · 2019-05-29T16:00:28Z

Summary

Related discussion that may require changing the whole approach to authorization: #38009

2 main changes

http route can be marked with tags. Tags are used as a part of authorization rules declaration.

const router = new Router('');
router.get({
  path: '/',
  options: {
    tags:  ['access:console'],
  }
}, ...)

KibanaRequest exposes route config

const router = new Router('');
router.get({
  path: '/',
  options: {
    tags:  ['access:my-app']
  }
}, (request, responseToolkit) => {
   const { tags } = request.route.options
   if(tags.includes('access:my-app'){ 
   ...
})

there are tons of changes in docs after I merged master and run yarn core:acceptApiChanges. looks like the reason for this is@microsoft/api-documenter update #37759

Probably it's easier to review commit by commit

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

mshustov · 2019-05-29T16:02:21Z

src/core/server/http/router/route.ts

+  authRequired?: boolean;
+
+  /**
+   * Additional meta-data information to attach fot the route.


typo: fot --> to

azasypkin · 2019-05-29T16:08:33Z

ACK: will take a look on Friday or Monday at the latest.

elasticmachine · 2019-05-29T16:58:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin

Approach looks good to me, but can you please add a usage example into PR description? Just to have a better idea how it's going to be used.

src/core/server/http/router/request.ts

azasypkin · 2019-05-31T14:56:31Z

src/core/server/http/router/route.ts

+  /**
+   * Additional meta-data information to attach fot the route.
+   */
+  tags?: string[];


question: do you think using ReadonlySet<string> would introduce too much unnecessary friction? I usually prefer Map and Set where uniqueness is implied unless it really reduces API ergonomics.

I think using Set could be error-prone because strings are iterable in js

new Set('my:tag') // wrong declaration, don't throw new Set(['my:tag']) // correct declaration, but not intuitive

Yeah, it's arguable and may not be the best choice in some cases, don't have a strong opinion on that, feel free to pick whatever you think is better.

P.S. We decided to move with Set for a similar thing in EcnrytpedSavedObjects to signify uniqueness of the setting. It's not super important, just slowly transforming my coding habits towards string[] ---> Set<string> and Record<string, string> ---> Map<string, string> where appropriate 🙂

src/core/server/http/router/request.ts

src/core/server/http/lifecycle/auth.test.ts

mshustov · 2019-06-03T11:04:18Z

src/core/server/http/http_server.mocks.ts

+import { Request, ResponseToolkit } from 'hapi';
+import { merge } from 'lodash';
+
+type DeepPartial<T> = T extends any[]


@elastic/kibana-platform would be useful to declare some type helpers on project level or add a ready toolkit.

elasticmachine · 2019-06-03T12:15:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-03T14:21:04Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-06-03T14:21:06Z

Pinging @elastic/kibana-security

elasticmachine · 2019-06-03T15:34:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-06-03T15:40:01Z

retest

elasticmachine · 2019-06-04T09:35:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kobelb · 2019-06-04T14:25:20Z

Do we feel that sticking to the way that "tags" are implemented currently and replicating them in the new platform is strategically advantageous? At the time the access:identifier tags were implemented, it was truly all we had because we were stuck declaring these routes directly with HapiJS. If we had a different mechanism of uniquely identifying routes, I could envision a different solution which was prone to issues like #35881

I don't want to completely derail this effort though, if the legacy and new platform's interaction makes using the same approach across both strategically advantageous in the short-term.

mshustov · 2019-06-05T06:15:21Z

retest

elasticmachine · 2019-06-05T07:31:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-06-06T08:15:12Z

@elastic/kibana-platform could someone review, please? @eliperelman is on vacation

rudolf

Looks good. I made a docs language suggestion, but if you accept it you'll need to re-generate the docs.

src/core/server/http/http_server.test.ts

src/core/server/http/router/route.ts

mshustov · 2019-06-06T10:00:05Z

@skaapgif ok, then I will update wording manually. thanks ❤️

elasticmachine · 2019-06-06T11:29:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mshustov · 2019-06-06T11:40:58Z

@kobelb let's merge the current PR, even if we decide on other approach in #38009 we can re-use some work from this PR.
also tags can be used for other purposes.

reviewer is on vacation

@skaapgif

* expose route info in KibanaRequest * update mocks in test * make tags readonly, getRouteInfo is private method * add mocks for hapi internals * mode deepFreeze to core utils level as it env agnostic * freeze route props * fix typo * add tests for route options * fix integration tests. deep_freeze was moved under core utils * add comments, expose public types and regenerate docs * address comment. remove unnecessary async in route handlers * make routeSchema optional instead of union with undefined * @skaapgif improvements * update docs

@skaapgif

* expose route info in KibanaRequest * update mocks in test * make tags readonly, getRouteInfo is private method * add mocks for hapi internals * mode deepFreeze to core utils level as it env agnostic * freeze route props * fix typo * add tests for route options * fix integration tests. deep_freeze was moved under core utils * add comments, expose public types and regenerate docs * address comment. remove unnecessary async in route handlers * make routeSchema optional instead of union with undefined * @skaapgif improvements * update docs

mshustov added 2 commits May 29, 2019 17:56

expose route info in KibanaRequest

60e9a55

update mocks in test

c0bf094

mshustov commented May 29, 2019

View reviewed changes

azasypkin self-requested a review May 29, 2019 16:08

azasypkin reviewed May 31, 2019

View reviewed changes

mshustov added 5 commits June 3, 2019 09:55

Merge branch 'master' into issue-33775-route-tags

5f1c8b8

make tags readonly, getRouteInfo is private method

7c94dc4

add mocks for hapi internals

d8e9b66

mode deepFreeze to core utils level as it env agnostic

555dafe

freeze route props

360da87

mshustov commented Jun 3, 2019

View reviewed changes

fix typo

d733454

mshustov added 3 commits June 3, 2019 15:28

add tests for route options

241620c

fix integration tests. deep_freeze was moved under core utils

e51a2fd

add comments, expose public types and regenerate docs

5128c9b

mshustov added the review label Jun 3, 2019

mshustov marked this pull request as ready for review June 3, 2019 14:52

mshustov requested a review from a team as a code owner June 3, 2019 14:52

mshustov mentioned this pull request Jun 4, 2019

[New platform] Security plugin integration #33775

Closed

34 tasks

mshustov requested a review from eliperelman June 4, 2019 12:37

make routeSchema optional instead of union with undefined

277391a

mshustov mentioned this pull request Jun 6, 2019

disable renovate for developer doc generators. #38239

Merged

2 tasks

mshustov requested a review from a team June 6, 2019 08:14

Merge branch 'master' into issue-33775-route-tags

e25a0da

rudolf approved these changes Jun 6, 2019

View reviewed changes

src/core/server/http/http_server.test.ts Outdated Show resolved Hide resolved

src/core/server/http/router/route.ts Outdated Show resolved Hide resolved

mshustov added 2 commits June 6, 2019 11:55

@skaapgif improvements

89de831

update docs

c5909de

rudolf mentioned this pull request Jun 6, 2019

Update dependency @microsoft/api-extractor to v7.1.6 #37760

Merged

1 task

mshustov merged commit 53b133d into elastic:master Jun 6, 2019

mshustov deleted the issue-33775-route-tags branch June 6, 2019 13:49

mshustov mentioned this pull request Jun 7, 2019

[7.x] Route tags (#37344) #38373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Route tags #37344

Route tags #37344

mshustov commented May 29, 2019 •

edited

Loading

mshustov May 29, 2019

azasypkin commented May 29, 2019

elasticmachine commented May 29, 2019

azasypkin left a comment

azasypkin May 31, 2019

mshustov Jun 3, 2019 •

edited

Loading

azasypkin Jun 3, 2019

mshustov Jun 3, 2019

elasticmachine commented Jun 3, 2019

elasticmachine commented Jun 3, 2019

elasticmachine commented Jun 3, 2019

elasticmachine commented Jun 3, 2019

mshustov commented Jun 3, 2019

elasticmachine commented Jun 4, 2019

kobelb commented Jun 4, 2019 •

edited

Loading

mshustov commented Jun 5, 2019

elasticmachine commented Jun 5, 2019

mshustov commented Jun 6, 2019

rudolf left a comment

mshustov commented Jun 6, 2019

elasticmachine commented Jun 6, 2019

mshustov commented Jun 6, 2019

Route tags #37344

Route tags #37344

Conversation

mshustov commented May 29, 2019 • edited Loading

Summary

Checklist

For maintainers

mshustov May 29, 2019

Choose a reason for hiding this comment

azasypkin commented May 29, 2019

elasticmachine commented May 29, 2019

💔 Build Failed

azasypkin left a comment

Choose a reason for hiding this comment

azasypkin May 31, 2019

Choose a reason for hiding this comment

mshustov Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

azasypkin Jun 3, 2019

Choose a reason for hiding this comment

mshustov Jun 3, 2019

Choose a reason for hiding this comment

elasticmachine commented Jun 3, 2019

💔 Build Failed

elasticmachine commented Jun 3, 2019

elasticmachine commented Jun 3, 2019

elasticmachine commented Jun 3, 2019

💔 Build Failed

mshustov commented Jun 3, 2019

elasticmachine commented Jun 4, 2019

💚 Build Succeeded

kobelb commented Jun 4, 2019 • edited Loading

mshustov commented Jun 5, 2019

elasticmachine commented Jun 5, 2019

💔 Build Failed

mshustov commented Jun 6, 2019

rudolf left a comment

Choose a reason for hiding this comment

mshustov commented Jun 6, 2019

elasticmachine commented Jun 6, 2019

💚 Build Succeeded

mshustov commented Jun 6, 2019

mshustov commented May 29, 2019 •

edited

Loading

mshustov Jun 3, 2019 •

edited

Loading

kobelb commented Jun 4, 2019 •

edited

Loading