NP Security HTTP Interceptors #39477
Conversation
kobelb
added
the
Team:Security
|
Pinging @elastic/kibana-security |
kobelb
added
the
Feature:Security/Authentication
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
| * 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.
@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.
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.
Both approaches sound good to me too at this stage, but only because of additional confidence that the code actually works point. I'm afraid using real services may not scale very well and we'll end up with a mock in one test and real service in another.
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 setup may be drastically different from what you can do with it in start (e.g. this or this). That means that some of the core services won't be "public" (btw, if I'm not mistaken we're only supposed to import from src/core/public or src/core/public/mocks, I'm surprised that linter doesn't complain about src/core/public/http/base_path_service).
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.
I'm afraid using real services may not scale very well and we'll end up with a mock in one test and real service in another.
I share this fear as well. It seems like a lot of the existing NP tests use the mocks which the NP provides.
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 setup may be drastically different from what you can do with it in start (e.g. this or this). That means that some of the core services won't be "public" (btw, if I'm not mistaken we're only supposed to import from src/core/public or src/core/public/mocks, I'm surprised that linter doesn't complain about src/core/public/http/base_path_service).
It definitely felt like I was reaching "too far" into /src/core/public to get access to this. Do you happen to have a recommendation for where I should be getting the core mock from?
There was a problem hiding this comment.
I believe we're supposed to get all the mocks from either src/core/server/mocks or src/core/public/mocks. So for base_path it'd be something like this I guess:
import { coreMock } from 'src/core/public/mocks';
const basePath = coreMock.createSetup().http.basePath;There was a problem hiding this comment.
Thanks @azasypkin! I'll change this to use the mocks and reevaluate the other tests as well
There was a problem hiding this comment.
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 BasePath for tests like these?
Concrete instance: https://github.com/elastic/kibana/blob/32d938591cdfbfd3651068e97cc82aa613f66e0c/x-pack/plugins/security/public/session_expired.test.ts
Mocked: https://github.com/elastic/kibana/blob/824086c168aec5cc55c04e5866ceaafdb2ec12f9/x-pack/plugins/security/public/session_expired.test.ts
There was a problem hiding this comment.
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 coreMock and make coreMock configurable so you can pass in a basePath when you create the mock.
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.
Co-Authored-By: Larry Gregory <lgregorydev@gmail.com>
💚 Build Succeeded |
Co-Authored-By: Larry Gregory <lgregorydev@gmail.com>
|
|
||
| import { HttpSetup } from 'src/core/public'; | ||
|
|
||
| export class AnonymousPaths { |
There was a problem hiding this comment.
Shouldn't this implement IAnonymousPaths?
There was a problem hiding this comment.
Yup, I will make it so.
| export class AnonymousPaths { | ||
| private paths: Set<string>; | ||
|
|
||
| constructor(private basePath: HttpSetup['basePath']) { |
There was a problem hiding this comment.
nit: seems simpler to just use IBasePath directly rather than coupling this to HttpSetup
There was a problem hiding this comment.
Agreed, this is a carry-over from AnonymousPaths being a security plugin class. I will update it.
| } | ||
|
|
||
| public register(path: string) { | ||
| if (!path.startsWith('/')) { |
There was a problem hiding this comment.
Could normalizePath handle this?
There was a problem hiding this comment.
It definitely can. It felt somewhat awkward to allow consumers to do anonymousPaths.register('login') and felt more explicit to require them to do anonymousPaths.register('/login'). Requiring the path to start with / makes it more obvious that the paths are from the "root" as opposed to relative to anything. This is largely subjective and since it's a platform API, my opinion is just an opinion and I will make it however you prefer.
There was a problem hiding this comment.
I'm all for making things more robust if it's easy to change. Don't consider it a blocker for this PR though
| public setup(core: CoreSetup, deps: {}) { | ||
| const { http, notifications, injectedMetadata } = core; | ||
| const { basePath, anonymousPaths } = http; | ||
| anonymousPaths.register('/login'); |
There was a problem hiding this comment.
What we could do is have a option on the ApplicationService that specifies an app as "anonymous" and then use that to derive these paths.
Maybe we take a look at that after #41981 is complete for the login, logout, and status apps.
|
|
||
| // if the request was omitting credentials it's to an anonymous endpoint | ||
| // (for example to login) and we don't wish to ever redirect | ||
| // TODO: Why is httpErrorResponse.request potentially undefined? |
Co-Authored-By: Josh Dover <me@joshdover.com>
Co-Authored-By: Josh Dover <me@joshdover.com>
💚 Build Succeeded |
I like that idea!
Good catch, removing... |
💚 Build Succeeded |
|
@legrego - all your feedback should be addressed. |
💚 Build Succeeded |
* We have a NP plugin! :celebration: * Redirecting to login on all 401s * Adding commented out code for when credentials are omitted * Fixing types * Respond 403 when user changes password with incorrect current password * Adding AnonymousPaths where we ignore all 401s * Adding anonymous path tests * Extracted a dedicated SessionExpires class and added tests * Fixing plugin after refactoring to add SessionExpired * Beginning to work on the session timeout interceptor * Fixing UnauthorizedResponseInterceptor anonymous path test * Removing test anonymous path * Trying to improve readability * Displaying session logout warning * Mocking out the base path * Revert "Mocking out the base path" This reverts commit 824086c. * Changing coreMock to use a concrete instance of BasePath * Adding session timeout interceptor tests * Adding session timeout tests * Adding more tests for short session timeouts * Moving some files to a session folder * More thrashing around: renaming and reorganizing * Renaming Interceptor to HttpInterceptor * Fixing some type errors * Fixing legacy chrome API tests * Fixing other tests to use the concrete instance of BasePath * Adjusting some types * Putting DeeplyMocked back, I don't get how DeeplyMockedKeys works * Moving anonymousPaths to public core http * Reading sessionTimeout from injected vars and supporting null timeout * Doesn't extend session when there is no response * Updating docs and snapshots * Casting sessionTimeout injectedVar to "number | null" * Fixing i18n issues * Update x-pack/plugins/security/public/plugin.ts Co-Authored-By: Larry Gregory <lgregorydev@gmail.com> * Adding milliseconds postfix to SessionTimeout private fields * Even better anonymous paths, with some validation * Adjusting public method docs for IAnonymousPaths * Adjusting spelling of base-path to basePath * Update x-pack/plugins/security/public/session/session_timeout.tsx Co-Authored-By: Larry Gregory <lgregorydev@gmail.com> * Update src/core/public/http/anonymous_paths.ts Co-Authored-By: Josh Dover <me@joshdover.com> * Update src/core/public/http/anonymous_paths.ts Co-Authored-By: Josh Dover <me@joshdover.com> * AnonymousPaths implements IAnonymousPaths and uses IBasePath * Removing DeeplyMocked * Removing TODOs * Fixing types... * Now, ever more normal
* We have a NP plugin! :celebration: * Redirecting to login on all 401s * Adding commented out code for when credentials are omitted * Fixing types * Respond 403 when user changes password with incorrect current password * Adding AnonymousPaths where we ignore all 401s * Adding anonymous path tests * Extracted a dedicated SessionExpires class and added tests * Fixing plugin after refactoring to add SessionExpired * Beginning to work on the session timeout interceptor * Fixing UnauthorizedResponseInterceptor anonymous path test * Removing test anonymous path * Trying to improve readability * Displaying session logout warning * Mocking out the base path * Revert "Mocking out the base path" This reverts commit 824086c. * Changing coreMock to use a concrete instance of BasePath * Adding session timeout interceptor tests * Adding session timeout tests * Adding more tests for short session timeouts * Moving some files to a session folder * More thrashing around: renaming and reorganizing * Renaming Interceptor to HttpInterceptor * Fixing some type errors * Fixing legacy chrome API tests * Fixing other tests to use the concrete instance of BasePath * Adjusting some types * Putting DeeplyMocked back, I don't get how DeeplyMockedKeys works * Moving anonymousPaths to public core http * Reading sessionTimeout from injected vars and supporting null timeout * Doesn't extend session when there is no response * Updating docs and snapshots * Casting sessionTimeout injectedVar to "number | null" * Fixing i18n issues * Update x-pack/plugins/security/public/plugin.ts Co-Authored-By: Larry Gregory <lgregorydev@gmail.com> * Adding milliseconds postfix to SessionTimeout private fields * Even better anonymous paths, with some validation * Adjusting public method docs for IAnonymousPaths * Adjusting spelling of base-path to basePath * Update x-pack/plugins/security/public/session/session_timeout.tsx Co-Authored-By: Larry Gregory <lgregorydev@gmail.com> * Update src/core/public/http/anonymous_paths.ts Co-Authored-By: Josh Dover <me@joshdover.com> * Update src/core/public/http/anonymous_paths.ts Co-Authored-By: Josh Dover <me@joshdover.com> * AnonymousPaths implements IAnonymousPaths and uses IBasePath * Removing DeeplyMocked * Removing TODOs * Fixing types... * Now, ever more normal
The following implements a very rudimentary redirect whenever we receive a 401 response. This does not currently address two exceptional situations which are in the LP, where we ignore the 401s:
kibana/x-pack/legacy/plugins/security/public/hacks/on_unauthorized_response.js
Lines 14 to 15 in bd4b7a0
kibana/x-pack/legacy/plugins/xpack_main/public/services/path.js
Line 12 in bd4b7a0
With regard to the hard-coded APIs, we're using this for two endpoints "login" and "change password". For login, what if we were to add an option to the NP http service to allow consumers to specify
credentials: 'omit'as an option. That way when we see a 401 from a request where we're usingcredentials: 'omit'we just don't do the redirect. We don't really want or need the cookie sent to the login endpoint, because they'll be getting a new cookie on a successful login. For the change password endpoint, I think we can just change the 401 we return when the user tries to change their own password and doesn't provide the correct old password to a 403. You're forbidden from changing your password if you don't know the old password. I don't really understand the historical reasons for returning a 401 here in the first place.With regard to the page URLs, these are consumed a number of places. They're generally used to ensure that the end-user is logged in before doing things like showing telemetry banners, or polling for reports, etc. This one I don't have a great answer for. All of the pages except for "/status" are security pages and the consumers are in x-pack. The security plugin could potentially expose an API to allow consumers to determine the user is currently on an "unauthenticated page". I'm open to alternatives here.
Resolves #48858
Depends on new functionality
#41990Blocked by bugs
#42990 #42311 #42307