-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NP Security HTTP Interceptors #39477
Changes from 17 commits
d4f7e8e
3ebd52f
16c8e29
7c9f4b8
249bbfa
9041c15
2c6b3aa
007bc82
67af74c
984bf56
f672b72
c7ff2f0
181d951
ba84476
a551a5e
643729a
32d9385
824086c
0384182
acb61ee
41cbc8c
147edbd
ddab529
3b8711e
f37cf88
93206ad
eb860e8
3bd02fb
bae2ef9
da8b846
fb00dc4
51bdb02
837ba11
e23cf8f
2c98227
32751a4
4b0ea10
9432988
e169fbe
3582111
edac0ec
85a655e
a70ac40
248c580
5697aa2
4af0d8a
5a91232
50237cb
40a3697
08aa526
c4ec25c
15ff621
5a9b9a5
43425b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,6 +102,7 @@ export { | |
HttpResponse, | ||
HttpHandler, | ||
HttpBody, | ||
HttpInterceptController, | ||
} from './http'; | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,7 +95,7 @@ export function initUsersApi({ authc: { login }, config }, server) { | |
BasicCredentials.decorateRequest(request, username, password) | ||
); | ||
} catch(err) { | ||
throw Boom.unauthorized(err); | ||
throw Boom.forbidden(err); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason this was changed from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either work in the LP, the significant part was the switch from |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -175,6 +175,7 @@ | |
"ts-loader": "^6.0.4", | ||
"typescript": "3.5.3", | ||
"vinyl-fs": "^3.0.2", | ||
"whatwg-fetch": "^3.0.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I misread this file, turns out we duplicate dependencies. |
||
"xml-crypto": "^0.10.1", | ||
"yargs": "4.8.1" | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,5 +4,5 @@ | |
"kibanaVersion": "kibana", | ||
"configPath": ["xpack", "security"], | ||
"server": true, | ||
"ui": false | ||
"ui": true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { AnonymousPaths } from './anonymous_paths'; | ||
import { BasePath } from 'src/core/public/http/base_path_service'; | ||
|
||
describe('#isAnonymous', () => { | ||
it('returns true for initialPaths', () => { | ||
const basePath = new BasePath('/foo'); | ||
const anonymousPaths = new AnonymousPaths(basePath, ['/bar', '/baz']); | ||
expect(anonymousPaths.isAnonymous('/foo/bar')).toBe(true); | ||
expect(anonymousPaths.isAnonymous('/foo/baz')).toBe(true); | ||
}); | ||
|
||
it('returns true for registered paths', () => { | ||
const basePath = new BasePath('/foo'); | ||
const anonymousPaths = new AnonymousPaths(basePath, []); | ||
anonymousPaths.register('/bar'); | ||
expect(anonymousPaths.isAnonymous('/foo/bar')).toBe(true); | ||
}); | ||
|
||
it('returns false for other paths', () => { | ||
const basePath = new BasePath('/foo'); | ||
const anonymousPaths = new AnonymousPaths(basePath, ['/bar', '/baz']); | ||
anonymousPaths.register('/qux'); | ||
expect(anonymousPaths.isAnonymous('/foo/quux')).toBe(false); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { HttpSetup } from 'src/core/public'; | ||
|
||
export class AnonymousPaths { | ||
private paths: Set<string>; | ||
|
||
constructor(private basePath: HttpSetup['basePath'], initialAnonymousPaths: string[]) { | ||
this.paths = new Set(initialAnonymousPaths); | ||
} | ||
|
||
public isAnonymous(path: string): boolean { | ||
return this.paths.has(this.basePath.remove(path)); | ||
} | ||
|
||
public register(path: string) { | ||
this.paths.add(path); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { PluginInitializer } from 'src/core/public'; | ||
import { SecurityPlugin, SecurityPluginSetup, SecurityPluginStart } from './plugin'; | ||
|
||
export const plugin: PluginInitializer<SecurityPluginSetup, SecurityPluginStart> = () => | ||
new SecurityPlugin(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { Plugin, CoreSetup } from 'src/core/public'; | ||
import { AnonymousPaths } from './anonymous_paths'; | ||
import { SessionExpired } from './session_expired'; | ||
import { SessionTimeout } from './session_timeout'; | ||
import { SessionTimeoutInterceptor } from './session_timeout_interceptor'; | ||
import { UnauthorizedResponseInterceptor } from './unauthorized_response_interceptor'; | ||
|
||
export class SecurityPlugin implements Plugin<SecurityPluginSetup, SecurityPluginStart> { | ||
public setup(core: CoreSetup, deps: {}) { | ||
kobelb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const { http, notifications } = core; | ||
const sessionExpired = new SessionExpired(http.basePath); | ||
const anonymousPaths = new AnonymousPaths(http.basePath, [ | ||
'/login', | ||
'/logout', | ||
'/logged_out', | ||
'/status', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd much prefer to not have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, core (server side at least) has a notion of authentication, why can't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point! I'll open up a separate PR to add this directly to the NP to see how they feel. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving anonymous paths to the NP is turning into a bit of work. before I continue down this path, @elastic/kibana-platform do you mind confirming whether or not it makes sense to move this to the NP? Also, do you think it makes sense for it to be it's own "top-level service" or integrated within one of the existing services? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would make sense to be on the frontend's HTTP service. The data flow would probably be something like:
Once the ui_render_mixin is moved to the New Platform we could eliminate some of the threading here, but until then this probably makes the most sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, these are "frontend paths" where we ignore all 401s. For example the login page which is available at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I understand now. I think since this behavior (handling of 401s) is only used by Security (I think?) then wouldn't it be fine to keep this in the Security plugin and have the Security plugin register any OSS-owned paths itself? This is similar to what we do with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is essentially replacing this LP AngularJS service: https://github.com/elastic/kibana/blob/7ac8e4d9ccc2a9e064d8a5621cd62453d1006164/x-pack/legacy/plugins/xpack_main/public/services/path.js. The LP service has a hard-coded list of paths. This means that plugins themselves can't register their own paths, and instead this list must be modified. We've primarily addressed this in the NP version by allowing paths to be registered; however, the It's also relevant that security isn't the only plugin which consumes the LP AngularJS service. It's also consumed by telemetry and reporting. Since the NP core already introduces the concept of "auth" via facilities like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's used by other plugins, then I think moving it to the OSS/Core HttpService on the client side makes sense then 👍. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @joshdover! |
||
]); | ||
http.intercept(new UnauthorizedResponseInterceptor(sessionExpired, anonymousPaths)); | ||
|
||
const sessionTimeout = new SessionTimeout(1.5 * 60 * 1000, notifications, sessionExpired, http); | ||
http.intercept(new SessionTimeoutInterceptor(sessionTimeout, anonymousPaths)); | ||
|
||
return { | ||
anonymousPaths, | ||
sessionTimeout, | ||
}; | ||
} | ||
|
||
public start() {} | ||
} | ||
|
||
export type SecurityPluginSetup = ReturnType<SecurityPlugin['setup']>; | ||
export type SecurityPluginStart = ReturnType<SecurityPlugin['start']>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { BasePath } from 'src/core/public/http/base_path_service'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @azasypkin @legrego how do you all feel about us writing tests using the NP core services, instead of mocking them out? When it's painful, or impossible, to use a concrete instance, I definitely think it makes sense to use a mock. However, in situations like these it really isn't that painful, and seems to add a level of additional confidence that the code actually works. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm in favor of that approach. I prefer concrete instances when it's not too painful to create or maintain, for the same reasons as you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both approaches sound good to me too at this stage, but only because of The issue here is that not all core services are exposed to plugins as is, moreover they may have different shape for different life-cycle events: what you can do with HTTP service in Having said that, core should provide mocks for all of its services and keep them in sync. If we don't have a mock for the core service that is used in a plugin or it's hard to use it in our tests (e.g. it doesn't have reasonable defaults for mock return values) - platform team should treat it as a bug 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I share this fear as well. It seems like a lot of the existing NP tests use the mocks which the NP provides.
It definitely felt like I was reaching "too far" into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we're supposed to get all the mocks from either import { coreMock } from 'src/core/public/mocks';
const basePath = coreMock.createSetup().http.basePath; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @azasypkin! I'll change this to use the mocks and reevaluate the other tests as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just re-wrote the tests for this using only the mocks, and it's significantly "uglier" and harder to understand... @joshdover how do you feel about us using a concrete instance of Concrete instance: https://github.com/elastic/kibana/blob/32d938591cdfbfd3651068e97cc82aa613f66e0c/x-pack/plugins/security/public/session_expired.test.ts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The basePath APIs are so small and don't interact with anything meaningful. It seems to me that we should replace this with the real implementation in Normally, I'd say it's bad to mix concrete implementations and mocks like this, but given how little code we're talking about and how common I think this scenario is, I think this mixing would be fine. In summary: delete the basePath mock completely, replace it with the real implementation, allow coreMock to take a basePath argument. |
||
import { SessionExpired } from './session_expired'; | ||
|
||
const mockCurrentUrl = (url: string) => window.history.pushState({}, '', url); | ||
|
||
it('redirects user to "/logout" when there is no basePath', async () => { | ||
mockCurrentUrl('/foo/bar?baz=quz#quuz'); | ||
const sessionExpired = new SessionExpired(new BasePath()); | ||
const newUrlPromise = new Promise<string>(resolve => { | ||
jest.spyOn(window.location, 'assign').mockImplementation(url => { | ||
resolve(url); | ||
}); | ||
}); | ||
|
||
sessionExpired.logout(); | ||
|
||
const url = await newUrlPromise; | ||
expect(url).toBe( | ||
`/logout?next=${encodeURIComponent('/foo/bar?baz=quz#quuz')}&msg=SESSION_EXPIRED` | ||
); | ||
}); | ||
|
||
it('redirects user to "/${basePath}/logout" and removes basePath from next parameter when there is a basePath', async () => { | ||
mockCurrentUrl('/foo/bar?baz=quz#quuz'); | ||
const sessionExpired = new SessionExpired(new BasePath('/foo')); | ||
const newUrlPromise = new Promise<string>(resolve => { | ||
jest.spyOn(window.location, 'assign').mockImplementation(url => { | ||
resolve(url); | ||
}); | ||
}); | ||
|
||
sessionExpired.logout(); | ||
|
||
const url = await newUrlPromise; | ||
expect(url).toBe( | ||
`/foo/logout?next=${encodeURIComponent('/bar?baz=quz#quuz')}&msg=SESSION_EXPIRED` | ||
); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { HttpSetup } from 'src/core/public'; | ||
|
||
export class SessionExpired { | ||
constructor(private basePath: HttpSetup['basePath']) {} | ||
|
||
public logout() { | ||
const next = this.basePath.remove( | ||
`${window.location.pathname}${window.location.search}${window.location.hash}` | ||
); | ||
window.location.assign( | ||
this.basePath.prepend(`/logout?next=${encodeURIComponent(next)}&msg=SESSION_EXPIRED`) | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already part of the OSS jest setup, but it wasn't here.