Session storage refactoring by mshustov · Pull Request #37992 · elastic/kibana

mshustov · 2019-06-04T13:04:31Z

Summary

Current implementation passes bound to an incoming request session storage to AuthHandler. This API doesn't allow registerAuth caller to use session storage outside of AuthHandler function.

I changed api in the way when registerAuth returns sessionStorageFactory, therefore it can be used in any place of registerAuth. Security team needs this api to perform logout in route handler

// before
registerAuth(
  (req: KibanaRequest, sessionStorage: SessionStorage, t: authToolkit) => AuthResult, 
  SessionStorageCookieOptions
) => void

// after
registerAuth(
  (req: KibanaRequest, t: authToolkit) => AuthResult, 
  SessionStorageCookieOptions
) => { sessionStorageFactory: SessionStorageFactory }

// in handler
const sessionStorage = sessionStorageFactory.asScoped(request);
sessionStorage.clear();

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

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

as soon as registerAuth refactored to restrict access to hapi Request, CookieSessionStorage won't work with hapi request directly

mshustov · 2019-06-04T13:08:25Z

src/core/server/http/cookie_session_storage.ts


 class ScopedCookieSessionStorage<T extends Record<string, any>> implements SessionStorage<T> {
-  constructor(private readonly server: Server, private readonly request: Request) {}
+  constructor(private readonly server: Server, private readonly request: Readonly<Request>) {}


issue #33775 has point to restrict access to hapi Request object in registerAuth handler. thus we will operate KibanaRequest-ish object here

mshustov · 2019-06-04T13:09:34Z

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

-  }
 }
+
+export const toRawRequest = (request: KibanaRequest) => request[requestSymbol];


this api shouldn't be exposed outside of http service

suggestion: Because this is kind of sensitive, perhaps we should add a comment and mark it with @internal, it's easy to do a refactor like export * from './request' and accidentally expose this

mshustov · 2019-06-04T13:12:54Z

src/core/server/http/cookie_session_storage.ts

  return {
-    asScoped(request: Request) {
-      return new ScopedCookieSessionStorage<T>(server, request);
+    asScoped(request: Readonly<Request> | KibanaRequest) {


As soon as #36509 is implemented we pass bound session storage to a consumer. There is an open question on how to restrict access to this API outside of the plugin boundaries.
#36509 (comment)

elasticmachine · 2019-06-04T13:19:24Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-06-04T13:19:27Z

Pinging @elastic/kibana-security

kobelb · 2019-06-04T15:11:42Z

From the consumption standpoint, this looks great!

To be honest, I didn't realize that you could access the sessionStorageFactory within the registerAuth "callback" before the promise returned from registerAuth returns:

  const { sessionStorageFactory } = await registerAuth<StorageData>((req, t) => {
      const user = { id: '42' };
      const sessionStorage = sessionStorageFactory.asScoped(req);
      sessionStorage.set({ value: user, expires: Date.now() + 1000 });
      return t.authenticated(user);
    }, cookieOptions);

elasticmachine · 2019-06-04T16:07:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

rudolf · 2019-06-05T12:30:25Z

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

-  }
 }
+
+export const toRawRequest = (request: KibanaRequest) => request[requestSymbol];


suggestion: Because this is kind of sensitive, perhaps we should add a comment and mark it with @internal, it's easy to do a refactor like export * from './request' and accidentally expose this

rudolf · 2019-06-05T12:44:54Z

src/core/server/http/session_storage.ts


 export interface SessionStorageFactory<T> {
-  asScoped: (request: Request) => SessionStorage<T>;
+  asScoped: (request: Readonly<Request> | KibanaRequest) => SessionStorage<T>;


I see we use Readonly<Request> in tests, but would a consumer of the API ever get access to a Request to pass it in here?

yes, one of the next actions in #33775 is restrict access to hapi Request object, which is exposed via registerAuth #34631 (comment)

elasticmachine · 2019-06-06T09:45:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

joshdover

The main change here looks good. Not worried about any session leakage since SessionStorageFactory only exposes request-scoped session storage

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

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

src/core/server/server.api.md

elasticmachine · 2019-06-06T17:00:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-07T08:17:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Kibana request keep a reference to raw request. used to bind hapi-cookie * CookieSessionStorage should work with KibanaRequest as soon as registerAuth refactored to restrict access to hapi Request, CookieSessionStorage won't work with hapi request directly * change registerAuth public api * adopt auth lifecycle tests * move lifecycle auth tests from integration to unit and adopt to new api. * mark toRawRequest as internal to prevent exposure * generate docs * reword test cases * mark Request internals in tsdoc

mshustov added 5 commits June 4, 2019 14:29

Kibana request keep a reference to raw request. used to bind hapi-cookie

c26ca80

CookieSessionStorage should work with KibanaRequest

cc4e689

as soon as registerAuth refactored to restrict access to hapi Request, CookieSessionStorage won't work with hapi request directly

change registerAuth public api

db0c258

adopt auth lifecycle tests

440854c

move lifecycle auth tests from integration to unit and adopt to new api.

9cef7fd

mshustov commented Jun 4, 2019

View reviewed changes

mshustov mentioned this pull request Jun 4, 2019

[New platform] Security plugin integration #33775

Closed

34 tasks

mshustov marked this pull request as ready for review June 4, 2019 13:19

mshustov requested a review from a team as a code owner June 4, 2019 13:19

mshustov added the review label Jun 5, 2019

rudolf reviewed Jun 5, 2019

View reviewed changes

mark toRawRequest as internal to prevent exposure

71e7bf5

mshustov requested a review from rudolf June 6, 2019 08:22

mshustov added 2 commits June 6, 2019 16:49

Merge branch 'master' into issue-33775-session-storage-refactoring

181d162

generate docs

c6fe0e1

mshustov force-pushed the issue-33775-session-storage-refactoring branch from 793cd31 to c6fe0e1 Compare June 6, 2019 15:00

joshdover 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/request.ts Outdated Show resolved Hide resolved

src/core/server/server.api.md Outdated Show resolved Hide resolved

mshustov added 3 commits June 7, 2019 08:48

Merge branch 'master' into issue-33775-session-storage-refactoring

4d44555

reword test cases

13188db

mark Request internals in tsdoc

ea82988

mshustov merged commit f753474 into elastic:master Jun 7, 2019

mshustov deleted the issue-33775-session-storage-refactoring branch June 7, 2019 08:18

mshustov mentioned this pull request Jun 7, 2019

[7.x] Session storage refactoring (#37992) #38385

Merged

Conversation

mshustov commented Jun 4, 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!

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!

elasticmachine commented Jun 4, 2019

Uh oh!

elasticmachine commented Jun 4, 2019

Uh oh!

kobelb commented Jun 4, 2019

Uh oh!

elasticmachine commented Jun 4, 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!

elasticmachine commented Jun 6, 2019

💔 Build Failed

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Jun 6, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jun 7, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mshustov commented Jun 4, 2019 •

edited

Loading