Route tags by mshustov · Pull Request #37344 · elastic/kibana

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-03T15:46:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

eliperelman · 2019-06-03T16:23:08Z

src/core/public/application/capabilities/capabilities_service.mock.ts

 */
 import { CapabilitiesService, CapabilitiesStart } from './capabilities_service';
-import { deepFreeze } from '../../utils/deep_freeze';
+import { deepFreeze } from '../../../utils/';


Remove trailing slash for import consistency.

let's introduce linter rule explicitly if it's important. imo it doesn't make sense to perform manual check on every PR
related #36829 (comment)

eliperelman · 2019-06-03T16:23:23Z

src/core/public/injected_metadata/injected_metadata_service.ts

 import { DiscoveredPlugin, PluginName } from '../../server';
 import { UiSettingsState } from '../ui_settings';
-import { deepFreeze } from '../utils/deep_freeze';
+import { deepFreeze } from '../../utils/';


Remove trailing slash for import consistency.

eliperelman · 2019-06-03T16:24:42Z

src/core/public/utils/integration_tests/__fixtures__/frozen_object_mutation/index.ts

 */

-import { deepFreeze } from '../../../deep_freeze';
+import { deepFreeze } from '../../../../../utils/deep_freeze';


The other imports for deepFreeze pull this directly off of utils instead of utils/deep_freeze; did you want to do that here as well?

this integration test compiles TS and declares its own tsconfig https://github.com/restrry/kibana/blob/5128c9bf3d2fc508d95d27414b912e1d12be5a55/src/core/public/utils/integration_tests/deep_freeze.test.ts
if I import ../../../../../utils/ TS compilation fails due to different setup for the Kibana project and test. I don't think we should keep both in sync, so I just narrowed import to the specific file.

eliperelman · 2019-06-03T16:27:01Z

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

+
+function createRawRequestMock(customization: DeepPartial<Request> = {}) {
+  return merge(
+    {},


Using 2 anonymous objects here is redundant, so you can remove the first one:

return merge( { headers: {}, path: '/', route: { settings: {} }, raw: { req: { url: '/', }, }, }, customization ) as Request;

merge mutates the destination object https://lodash.com/docs/3.10.1#merge
if we mutate default value, the next test could be affected by the result of the previous createRawRequestMock call

eliperelman · 2019-06-03T16:28:03Z

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

-        } as any,
-        undefined
-      );
+      return KibanaRequest.from(httpServerMock.createRawRequest(), undefined);


Typically, passing undefined to a function is redundant, can this just be:

return KibanaRequest.from(httpServerMock.createRawRequest());

KibanaRequest.from declaration marks the second parameter as required
https://github.com/restrry/kibana/blob/5128c9bf3d2fc508d95d27414b912e1d12be5a55/src/core/server/http/router/request.ts#L49
I suspect it was done to force the caller to specify route schema. Although this method is used only by Kibana, so probably we can make it optional. @azasypkin

Although this method is used only by Kibana, so probably we can make it optional.

Yeah, it seems we should do that. And probably mark it as @internal?

there is no way to make a method as truly @internal on @public class. instead we can declare from method as an internal function, not a class static method

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

eliperelman · 2019-06-03T16:38:16Z

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

+      path: this.request.path,
+      method: this.request.method,
+      options: {
+        authRequired: this.request.route.settings.auth !== false,


If auth is always a boolean, could this instead be:

authRequired: this.request.route.settings.auth,

it's not boolean. false | string | RouteOptionsAccess;

we invert the logic. authRequired always true if not specified false explicitly

eliperelman · 2019-06-03T16:40:05Z

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

+ * The set of common HTTP methods supported by Kibana routing.
+ * @public
+ * */
+export type RouteMethod = 'get' | 'post' | 'put' | 'delete';


A couple questions:

Why should these now be lower case? It looks like it would lead to less diff changes in this patch.

Why not add 'patch' | 'options' to this list since they are also used in: method: RouteMethod | 'patch' | 'options';

current codebase relays on method names in lower case, because it's a default in hapi. changing to the upper case would lead to bugs during migration.

the router doesn't support PATCH and OPTIONS methods as of now and typing should reflect this fact

elasticmachine · 2019-06-03T19:08:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

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

eliperelman previously requested changes Jun 3, 2019

View reviewed changes

address comment. remove unnecessary async in route handlers

b260ee4

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

Conversation

mshustov commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin commented May 29, 2019

Uh oh!

elasticmachine commented May 29, 2019

💔 Build Failed

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 3, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jun 3, 2019

Uh oh!

elasticmachine commented Jun 3, 2019

Uh oh!

elasticmachine commented Jun 3, 2019

💔 Build Failed

Uh oh!

mshustov commented Jun 3, 2019

Uh oh!

elasticmachine commented Jun 3, 2019

💔 Build Failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mshustov commented May 29, 2019 •

edited

Loading

mshustov Jun 3, 2019 •

edited

Loading

mshustov Jun 4, 2019 •

edited

Loading

mshustov Jun 4, 2019 •

edited

Loading

kobelb commented Jun 4, 2019 •

edited

Loading