Initial PR for new security kibana plugin #162
Conversation
| '/bundles/app/security-customerror/bootstrap.js', | ||
| '/', | ||
| '/app/login', | ||
| '/app/opendistro_login', |
There was a problem hiding this comment.
Do we need 2 login endpoints?
There was a problem hiding this comment.
those are for my local testing, will remove them
| import { SecurityPluginConfigType } from "../../.."; | ||
| import { SecuritySessionCookie } from "../../../session/security_cookie"; | ||
| import { CoreSetup } from "../../../../../../src/core/server"; | ||
| import _ from 'lodash'; |
There was a problem hiding this comment.
Could we import members we leverage from lodash instead of loading all functions ?
There was a problem hiding this comment.
switched to only import specific function from lodash
| @@ -0,0 +1,115 @@ | |||
| /* | |||
There was a problem hiding this comment.
np: Would be good to have file name and class name to be same to relate.
There was a problem hiding this comment.
I am trying to follow Kibana's convention here, they use underscore for file names and CamelCase for type names
| * permissions and limitations under the License. | ||
| */ | ||
|
|
||
| export class User { |
There was a problem hiding this comment.
Do we intend to have more into this, just trying to understand why can't we leverage type and interface Do we need this instance to have more method in future ?
There was a problem hiding this comment.
changed to type instead of class
| npm-debug.log* | ||
| node_modules | ||
| /build/ | ||
| /public/app.css |
There was a problem hiding this comment.
Are we sure we want to remove this from git ?
There was a problem hiding this comment.
I don't know how it get added to .gitignore, in fact we don't have that file, removed it from .gitignore
| @@ -0,0 +1,9 @@ | |||
| { | |||
| "id": "security", | |||
There was a problem hiding this comment.
What is different between this id and PLUGIN_ID mentioned inside common/index.ts d
There was a problem hiding this comment.
Good catch! it doesn't matter much, but better to make it consistent, changed it to opendistroSecurity in kibana.json
| }, | ||
| async (context, request, response) => { | ||
| try { | ||
| const alternativeHeaders = this.config.basicauth.alternative_login.headers; |
There was a problem hiding this comment.
Will there be a case where one of the member could be null in this.config.basicauth.alternative_login.headers
There was a problem hiding this comment.
get() functions are not available to config types, use optional chaining instead:
const alternativeHeaders = this.config.basicauth?.alternative_login?.headers || [];
There was a problem hiding this comment.
Just to ensure it is available with TS transformation / latest node version. this works. When I mentioned get function I meant lodash.get function. But if optional chain works with the version we use, it should be godo.
| try { | ||
| const alternativeHeaders = this.config.basicauth.alternative_login.headers; | ||
| if (alternativeHeaders && alternativeHeaders.length) { | ||
| let requestHeaders = Object.keys(request.headers).map(header => header.toLowerCase()); |
There was a problem hiding this comment.
np: Could just create object instead of an array to simplify finding in next line.
There was a problem hiding this comment.
thanks for the tip. we decided to move the login page rendering to browser side, so I removed this route handler. Will try to adopt this approach in future cases
| } | ||
| } | ||
|
|
||
| private _handleAuthResponse(request: KibanaRequest, authResponse: AuthResponse, additionalAuthHeaders: any = {}) { |
There was a problem hiding this comment.
np: can we remove _ prefix as the method is private.
There was a problem hiding this comment.
removed prefix '-'
a preview PR for security Kibana plugin.
This plugin is developed based on Kibana new plugin platform .
This version includes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.