-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Account Management: Create API keys #82005
Conversation
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.
I wanted to leave some quick feedback so you had something to look at in the meantime. I need to caveat this review by saying I haven't actually run this locally yet.
Overall this is looking great! I love seeing more "modern" approaches to React development in our codebase, and I think some of the abstractions you've come up with here have a lot of potential for reuse in other areas of the security plugin.
I also really like your attention to detail in the UI here. This is going to be really slick!
x-pack/plugins/security/public/account_management/api_keys_page/api_keys_empty_prompt.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/public/account_management/api_keys_page/invalidate_api_key_modal.tsx
Show resolved
Hide resolved
@@ -199,7 +199,7 @@ | |||
"react-dom": "^16.12.0", | |||
"react-input-range": "^1.3.0", | |||
"react-router": "^5.2.0", | |||
"react-use": "^13.27.0", | |||
"react-use": "^15.3.4", |
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.
@streamich any concerns with updating react-use
from 13.27
to 15.3
?
x-pack/plugins/security/public/account_management/api_keys_page/api_keys_table.tsx
Show resolved
Hide resolved
x-pack/plugins/security/public/account_management/api_keys_page/api_keys_table.tsx
Show resolved
Hide resolved
if (error) { | ||
const { statusCode, message = '' } = (error as any).body ?? {}; | ||
|
||
if (statusCode === 400 && message.indexOf('[feature_not_enabled_exception]') !== -1) { |
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.
Let's chat quick about this logic tomorrow. I have a couple of ideas that might simplify/standardize things a bit
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.
So I've just noticed that the class you recommended me using is server-side code: x-pack/plugins/security/server/authentication/api_keys
so not sure it's expected to be used on the client.
The class also requires a logger and cluster client instance, non of which I have access to in the browser so I will keep this simple for now and use a functional approach directly in the component source.
On top of that I can't even re-use the logic of the function itself as the error object has a different schema and I only get statusCode and message through in the error body but not the disabled features property.
x-pack/plugins/security/public/management/api_keys/api_keys_api_client.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Larry Gregory <[email protected]>
Co-authored-by: Larry Gregory <[email protected]>
…curity/create-api-key
Pinging @elastic/kibana-security (Team:Security) |
A bulk of the increase in bundle size is attributed to the introduction of I don't know if this is the "correct" approach, but in my local testing, changing the import style to reference the necessary functions dropped the bundle size down significantly: diff --git a/x-pack/plugins/security/public/account_management/api_keys_page/api_keys_page.tsx b/x-pack/plugins/security/public/account_management/api_keys_page/api_keys_page.tsx
index 7f70ab228cb..81ebf21ba97 100644
--- a/x-pack/plugins/security/public/account_management/api_keys_page/api_keys_page.tsx
+++ b/x-pack/plugins/security/public/account_management/api_keys_page/api_keys_page.tsx
@@ -6,7 +6,8 @@
import React, { FunctionComponent, useState } from 'react';
import { Route, useHistory } from 'react-router-dom';
-import { useAsyncFn, useMount } from 'react-use';
+import useAsyncFn from 'react-use/lib/useAsyncFn';
+import useMount from 'react-use/lib/useMount';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiButton, EuiCallOut, EuiFlexGroup, EuiFlexItem, EuiSpacer } from '@elastic/eui';
|
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.
Sorry for not including this piece in the initial review: I think we should add a search bar to this page, to help folks filter down the list of API Keys. Many users will only have a handful of API Keys, so search won't be terribly useful for them. However, users who are responsible for setting up the SIEM Detection engine, or who generally take advantage of Alerting could find themselves with many API Keys to manage, and paging through the list without a search function would be tedious:
Bonus points if we support bulk-invalidation, but I consider this outside the scope of your PR
I also think it'd be worthwhile to add a couple of end-to-end tests ("functional tests" in Kibana's parlance) to verify the basic functionality of viewing, creating, and invalidating your API Keys. I can help you get acquainted with the functional test runner so you're not stumbling through this part blindly.
x-pack/plugins/security/public/account_management/api_keys_page/create_api_key_flyout.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/uptime (Team:uptime) |
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.
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.
LGTM! ❤️
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.
AppArch changes LGTM.
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
Unknown metric groups@kbn/ui-shared-deps asset size
async chunk count
History
To update your PR or re-run it, just comment with: |
Closing this since API keys are not attached to a user account so don't belong in the profile page as originally thought. |
This is what I think could be a good first iteration for this issue:
Possible additional features that can be included or postponed to a later release:
What do you think? |
Once we figure out where we want to put API key management,I think this would be good to include in the MVP as well. It should be easy for us to add, and it's a common pain point for users (who can sometimes encode the string incorrectly) |
Summary
This PR adds the ability to manage and create API keys directly from your user profile.
The account management page has been slightly redesigned with user avatar and tabs to cater for the new API keys section.
Empty Prompt
Authorisation error
Disabled error
Unknown error
Create API key form
API key successfully created
Invalidate API key confirmation
Implementation
react-use
dependency since types in previous version where brokenuseKibana
anduseHistory
) instead of manually passing these down the render treeBreadcrumb
component that automatically sets breadcrumbs and page title based on render treeuseForm
,useSubmitHandler
anduseInlineValidation
)ownFocus
functionality inEuiFlyout
is broken so had to render inside a portalChecklist
Delete any items that are not applicable to this PR.
For maintainers