Unify response interface in handler and request interceptors #42442

mshustov · 2019-08-01T13:36:37Z

Summary

Interceptor interface

The outcome of an interceptor function might be:

perform interceptor specific logic and pass a request further along the handler chain.
redirect an incoming request
reject an incoming request

Each interceptor has a specific toolkit. But what they have in common is the ability to redirect and reject an incoming request.

const preAuthResult = {
  next() => ...
  redirected(url: string) => ...
  rejected(error: Error, options: { statusCode?: number } = {}) => ...

Route interface

In the route handler function, redirection and rejection are parts of ResponseFactory and have completely different interface, providing ability to configure response body and headers.

const reponseFactory = {
  // ...
  redirected: (payload: HttpResponsePayload, options: RedirectResponseOptions) => ...
  badRequest: (error: ResponseError, options: HttpResponseOptions = {}) => ...
  unauthorized: (error: ResponseError, options: HttpResponseOptions = {}) => ...
 //..
};

We want both the mechanism to have a similar interface and feature set.
This PR introduces LifecycleResponseFactory which is a sub-set of ResponseFactory and supports only request redirection/rejection.
Interceptor specific outcome can be specified via toolkit passed as the last argument:

// pre-auth interceptor. toolkit methods - next, rewriteUrl.
registerOnPreAuth((req, res, toolkit) => toolkit.rewriteUrl(url))
// auth interceptor toolkit. toolkit methods - authenticated
registerAuth((req, res, toolkit) => toolkit.authenticated(...))
// post-auth interceptor. toolkit methods - next.
registerOnPostAuth((req, res, toolkit) => toolkit.next())

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

Dev docs

New platform plugins may want to extend Kibana capabilities with implementing a custom logic over an incoming request, before it hits a resource endpoint. For this purpose, KIbana introduced interceptors concept. Interceptors cannot change or mutate request object directly but may redirect, reject or allow a request to pass through. An interceptor may be created for different lifecycle stages of an incoming request:

before a user requesting a resource is authenticated
authentication
after a user was successfully authenticated.

// pre-authentication interceptor.
registerOnPreAuth((req, res, toolkit) => {
  if(req.headers.something) return res.badRequest({ body: '"something" headers is required' });
  return toolkit.next();
})
// authentication interceptor 
registerAuth((req, res, toolkit) => {
  const authResult = authenticate(req.headers.authorization);
  if(authResult.succeeded) return toolkit.authenticated(...);
  return res.unauthorized();
});
// post-authentication interceptor.
registerOnPostAuth((req, res, toolkit) => {
  if(deps.security.getUser(req).roles.length === 0) return res.notFound();
  return toolkit.next();
});

Only route handler can respond with 2xx response. Interceptors may redirect or reject an incoming request.

elasticmachine · 2019-08-01T13:36:39Z

Pinging @elastic/kibana-platform

mshustov · 2019-08-01T13:37:18Z

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

+    registerRouter(router);
+
+    registerOnPreAuth((req, res, t) =>
+      res.redirected(undefined, {


probably we can even go further and pass body and request options as one argument

return res.redirected({ headers: .... }) return res.ok({ body: {}, headers: .... })

@elastic/kibana-platform

Makes sense to me to make this API a bit less awkward. Do you think we should do that for all of these methods for consistency?

Do you think we should do that for all of these methods for consistency?

Yes. I can do this in a separate PR.

mshustov · 2019-08-01T13:45:40Z

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

+      .get('/')
+      .expect(200, 'ok');
+
+    expect(callingOrder).toEqual(['first', 'second']);


I focused more on the integration tests to analyze the observable behavior of the system. Otherwise, our tests are tightly coupled with hapi library.

mshustov · 2019-08-01T13:50:37Z

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

  }

  private toError(kibanaResponse: KibanaResponse<ResponseError>) {
    const { payload } = kibanaResponse;
-    return this.responseToolkit


had to switch to Boom error. hapi doesn't allow to respond from lifecycle event handlers. even for error responses 😞

mshustov · 2019-08-01T13:56:31Z

x-pack/plugins/security/server/authentication/index.ts

@@ -88,7 +88,7 @@ export async function setupAuthentication({
      authenticationResult = await authenticator.authenticate(request);
    } catch (err) {
      authLogger.error(err);
-      return t.rejected(wrapError(err));
+      return response.unauthorized(wrapError(err));


that's not 1-1 mapping. Before we passed on all ES error to the client side. we can add this logic to support rejecting with any error and status or always return something we have control over, say unauthorized/internal error. want to hear your opinion. @elastic/kibana-security @azasypkin

As far as I remember returning 401 will force user to logout, I'd rather return internal server error from this catch as it's unexpected authentication flow.

ok. to note: this changes API behavior. https://github.com/elastic/kibana/pull/42442/files#diff-5fa60b1877c33df27b85f0caac47988dR149

x-pack/plugins/security/server/authentication/index.ts

mshustov · 2019-08-01T14:15:32Z

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

-}
-
-type AuthResult = Authenticated | Rejected | Redirected;
+type AuthResult = Authenticated;


It can be as simple as Authenticated. The current implementation hides details and allows us to introduce additional logic without affecting the interface.

mshustov · 2019-08-01T14:16:09Z

src/core/server/http/lifecycle/on_pre_auth.ts

-      if (preAuthResult.isRedirected(result)) {
-        const { url, forward } = result;
-        if (forward) {
+      if (preAuthResult.isValid(result)) {


TODO: remove this check

elasticmachine · 2019-08-01T14:36:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

eliperelman

Looks good from an implementation standpoint. Have a couple optional nits.

src/core/server/http/integration_tests/core_services.test.ts

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

joshdover · 2019-08-01T23:14:38Z

ack: will get to tomorrow

azasypkin · 2019-08-02T08:09:41Z

x-pack/plugins/security/server/authentication/index.ts

@@ -105,28 +105,25 @@ export async function setupAuthentication({
      // authentication (username and password) or arbitrary external page managed by 3rd party
      // Identity Provider for SSO authentication mechanisms. Authentication provider is the one who
      // decides what location user should be redirected to.
-      return t.redirected(authenticationResult.redirectURL!);
+      return response.redirected(undefined, {


note: feels a bit weird to have a dedicated redirected method, but leverage optional generic headers for the required location header.... But functionally it works for us, so 🤷‍♂️

I kinda agree. However, we did it to be able to send a payload with redirection response and to comply with the other methods signature (payload, options) => KibanaResponse

elasticmachine · 2019-08-02T11:34:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-02T13:21:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…tly.

elasticmachine · 2019-08-02T14:58:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

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

joshdover · 2019-08-05T22:25:57Z

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

+    registerRouter(router);
+
+    registerOnPreAuth((req, res, t) =>
+      res.redirected(undefined, {


Makes sense to me to make this API a bit less awkward. Do you think we should do that for all of these methods for consistency?

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

azasypkin · 2019-08-07T06:53:08Z

x-pack/plugins/security/server/authentication/index.ts

-        // Hapi typings don't support headers that are `string[]`.
-        error.output.headers[headerName] = headerValue as any;
-      }
+      const error = authenticationResult.error;


potential issue: if the change in behavior above is okay (internalError for unexpected errors), here it's a bit different, ideally we should return the same status Elasticsearch gives us during authentication, e.g. 500 or whatever ES may return in the future (since we're basically intermediate party between user and Elasticsearch in the authentication flow). Ideally 401 should only be returned when ES returns error with such status or when we can't handle authentication (notHandled). It feels like we need something like wrapError in the core/responseFactory....

yes, that question I asked here #42442 (comment) ok, I will add customizable error handler

x-pack/plugins/security/server/authentication/index.ts

azasypkin

Changes in Spaces look good, but left a couple of comments/questions in security part.

elasticmachine · 2019-08-07T10:11:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

rudolf

Left a couple of comments on the types, but these aren't directly linked to this PR, so might be best to address in a different PR.

rudolf · 2019-08-07T12:26:17Z

src/core/server/server.api.md

@@ -401,31 +404,22 @@ export interface LogRecord {
 // Warning: (ae-forgotten-export) The symbol "OnPostAuthResult" needs to be exported by the entry point index.d.ts


Shouldn't we export this?

OnPostAuthResult, OnPreAuthResult, RouterRoute
they are internal types of the core, plugins don't have access to them.

rudolf · 2019-08-07T12:26:34Z

src/core/server/server.api.md

-    redirected: (url: string) => OnPostAuthResult;
-    rejected: (error: Error, options?: {
-        statusCode?: number;
-    }) => OnPostAuthResult;
 }

 // Warning: (ae-forgotten-export) The symbol "OnPreAuthResult" needs to be exported by the entry point index.d.ts


Missing export?

rudolf · 2019-08-07T12:27:26Z

src/core/server/server.api.md

@@ -535,16 +529,14 @@ export class Router {
    constructor(path: string);
    delete<P extends ObjectType, Q extends ObjectType, B extends ObjectType>(route: RouteConfig<P, Q, B>, handler: RequestHandler<P, Q, B>): void;
    get<P extends ObjectType, Q extends ObjectType, B extends ObjectType>(route: RouteConfig<P, Q, B>, handler: RequestHandler<P, Q, B>): void;
+    // Warning: (ae-forgotten-export) The symbol "RouterRoute" needs to be exported by the entry point index.d.ts


missing export?

rudolf · 2019-08-07T12:29:36Z

src/core/server/index.ts

@@ -74,6 +74,7 @@ export {
  IsAuthenticated,
  KibanaRequest,
  KibanaRequestRoute,
+  LifecycleResponseFactory,


Shouldn't we prefix these types with Http... to stick with our convention to avoid naming conflicts and make them more discoverable? (Also applies to the other HTTP types, so would probably make sense to do it in a different PR)

babel v7.5+ supports typescript namespaces. probably we can make another attempt to switch to them. for now, we can use manual convention.

Well that sure is exciting.

elasticmachine · 2019-08-07T12:49:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-08-07T13:43:30Z

retest

elasticmachine · 2019-08-07T14:46:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin

Security and Spaces changes LGTM

elasticmachine · 2019-08-08T09:59:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…#42442) * add response factory to the interceptors * adopt x-pack code to the changes * Add a separate response factory for lifecycles. Only route handler can respond with 2xx response. Interceptors may redirect or reject an incoming request. * re-generate docs * response.internal --> response.internalError * use internalError for exceptions in authenticator * before Security plugin proxied ES error status code. now sets explicitly. * provide error via message field of error response for BWC * update docs * add customError response * restore integration test and update unit tests * update docs * support Hapi error format for BWC * add a couple of tests

…#42918) * add response factory to the interceptors * adopt x-pack code to the changes * Add a separate response factory for lifecycles. Only route handler can respond with 2xx response. Interceptors may redirect or reject an incoming request. * re-generate docs * response.internal --> response.internalError * use internalError for exceptions in authenticator * before Security plugin proxied ES error status code. now sets explicitly. * provide error via message field of error response for BWC * update docs * add customError response * restore integration test and update unit tests * update docs * support Hapi error format for BWC * add a couple of tests

elasticmachine · 2019-08-14T12:35:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mshustov added 4 commits August 1, 2019 13:42

add response factory to the interceptors

2ee1d57

adopt x-pack code to the changes

6f6779a

Add a separate response factory for lifecycles.

4c434d6

Only route handler can respond with 2xx response. Interceptors may redirect or reject an incoming request.

re-generate docs

4af2986

mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.4.0 labels Aug 1, 2019

kimjoar mentioned this pull request Aug 1, 2019

[server] Expose HTTP service to plugins #12464

Closed

15 tasks

mshustov commented Aug 1, 2019

View reviewed changes

mshustov marked this pull request as ready for review August 1, 2019 14:39

mshustov requested review from a team as code owners August 1, 2019 14:39

eliperelman approved these changes Aug 1, 2019

View reviewed changes

src/core/server/http/integration_tests/core_services.test.ts Show resolved Hide resolved

src/core/server/http/integration_tests/core_services.test.ts Show resolved Hide resolved

src/core/server/http/lifecycle/auth.ts Outdated Show resolved Hide resolved

azasypkin reviewed Aug 2, 2019

View reviewed changes

mshustov added 2 commits August 2, 2019 12:30

response.internal --> response.internalError

534ad91

use internalError for exceptions in authenticator

6fb0fec

mshustov force-pushed the issue-41996-response-factory-to-interceptors branch from 2cedbb4 to 8f9ecbd Compare August 2, 2019 12:04

mshustov added 2 commits August 2, 2019 15:56

before Security plugin proxied ES error status code. now sets explici…

efe4dae

…tly.

provide error via message field of error response for BWC

fba3b7a

mshustov force-pushed the issue-41996-response-factory-to-interceptors branch from 869cf56 to fba3b7a Compare August 2, 2019 13:56

joshdover added this to Code review in kibana-core [DEPRECATED] Aug 2, 2019

joshdover reviewed Aug 5, 2019

View reviewed changes

Merge branch 'master' into issue-41996-response-factory-to-interceptors

6d4f6f7

azasypkin reviewed Aug 7, 2019

View reviewed changes

x-pack/plugins/security/server/authentication/index.ts Outdated Show resolved Hide resolved

azasypkin reviewed Aug 7, 2019

View reviewed changes

mshustov added 4 commits August 7, 2019 10:42

add customError response

9342185

restore integration test and update unit tests

8634bea

update docs

717d242

Merge branch 'master' into issue-41996-response-factory-to-interceptors

a1c204c

support Hapi error format for BWC

33e46e9

rudolf approved these changes Aug 7, 2019

View reviewed changes

mshustov requested a review from azasypkin August 7, 2019 13:35

joshdover approved these changes Aug 7, 2019

View reviewed changes

azasypkin approved these changes Aug 8, 2019

View reviewed changes

mshustov added 2 commits August 8, 2019 10:52

add a couple of tests

89c80c2

Merge branch 'master' into issue-41996-response-factory-to-interceptors

7383d7d

mshustov merged commit 06adc73 into elastic:master Aug 8, 2019

kibana-core [DEPRECATED] automation moved this from Code review to Needs Backport Aug 8, 2019

mshustov deleted the issue-41996-response-factory-to-interceptors branch August 8, 2019 10:07

mshustov mentioned this pull request Aug 8, 2019

[7.x] Unify response interface in handler and request interceptors (#42442) #42918

Merged

mshustov moved this from Needs Backport to Done (this week) in kibana-core [DEPRECATED] Aug 8, 2019

mshustov added the backported label Aug 8, 2019

This was referenced Aug 8, 2019

Unify ResponseFactory and Interceptor toolkit interfaces #41996

Closed

Response factory parameters as named arguments #43248

Merged

		@@ -401,31 +404,22 @@ export interface LogRecord {
		// Warning: (ae-forgotten-export) The symbol "OnPostAuthResult" needs to be exported by the entry point index.d.ts

Unify response interface in handler and request interceptors #42442

Unify response interface in handler and request interceptors #42442

Conversation

mshustov commented Aug 1, 2019 • edited

Summary

Interceptor interface

Route interface

Checklist

For maintainers

Dev docs

elasticmachine commented Aug 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Aug 1, 2019 • edited

Choose a reason for hiding this comment

mshustov Aug 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Aug 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 1, 2019

💔 Build Failed

eliperelman left a comment

Choose a reason for hiding this comment

joshdover commented Aug 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 2, 2019

💔 Build Failed

elasticmachine commented Aug 2, 2019

💔 Build Failed

elasticmachine commented Aug 2, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

azasypkin Aug 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 7, 2019

💚 Build Succeeded

rudolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Aug 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 7, 2019

💔 Build Failed

mshustov commented Aug 7, 2019

elasticmachine commented Aug 7, 2019

💚 Build Succeeded

azasypkin left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 8, 2019

💚 Build Succeeded

elasticmachine commented Aug 14, 2019

💚 Build Succeeded

mshustov commented Aug 1, 2019 •

edited

mshustov Aug 1, 2019 •

edited

mshustov Aug 1, 2019 •

edited

mshustov Aug 2, 2019 •

edited

azasypkin Aug 7, 2019 •

edited

mshustov Aug 7, 2019 •

edited