Added ability to create API keys #92610
Conversation
thomheymann
added
v8.0.0
v7.13.0
release_note:feature
|
@thomheymann when an API key is created, we have both the I think that we need to clarify that output, and possibly provide some link to explain how to use it. What do you think? |
@bytebilly What do you have in mind in terms of explanation? There's a few different use cases for API keys so I'm not sure what the best next steps would be for the user or what extra guidance we should offer. In terms of the output, this is printing the |
thomheymann
force-pushed
the
security/create-api-key
branch
from
e87cf17
to
65c4cd2
Compare
In the traditional These docs explain it a bit better: What do you think about exposing both the I think we should display the ID in the table too -- it won't be pretty to look at, but it's the only truly unique identifier that we have for each key (names are not unique) |
Ah, thanks for clarifying, I thought the API key was enough to authenticate. Good idea, I'll add that in. |
Thanks, this is exactly what I was talking about. There were multiple discussions on providing the two terms as separate values or as the base64 final string, but I don't think we have a good answer so far. Looping @philkra and @Mpdreamz to get more feedback from the Clients team. Having both may make sense if it doesn't make the UX confusing.
I'm not against that, but I see it could be a separate iteration if we don't have bandwidth to find a good way to present the |
|
@bytebilly I have updated the success state to show the full and correctly formatted API key. The situation across the stack is a bit of a mess right now and it would be great if we could get Beats and Logstash updated to accept base64 like Elasticsearch and the language clients do (at least the ones I checked). The current situation creates unnecessary confusion and requires relearning for each stack component. I have added a dropdown so that users can select the app they want to use the API key for but it would be even better to be able to remove this once all our apps and clients accept the same format. If there's an app I missed that should be listed in the dropdown let me know.
|
|
Thanks @thomheymann, I like the idea of having multiple output even if making it app specific was not in my initial thoughts. Interesting idea we can consider. What if the list is:
I'm curious what @cristina-eleonora thinks about that as she was looking into it too |
|
Thank you for including me in this conversation, @bytebilly. Since I'm not too versed in the world of Elasticsearch API keys, I have a couple of questions:
Regarding user experience:
|
|
Thank you for the great feedback Cristina! Here are some clarifications.
In the table we just show the
I think the main reason is to make clear who is the user creating the API key, since their permissions affect the final result.
Currently the Cloud ID (numeric) is shown in the table as the creator, since it is the
I'm pretty sure they are.
No, they are not local to the deployment but rather created on the Security Cluster. Users will continue to manage them from the Cloud Console.
No it cannot be reversed. I personally think that the term "remove" may be a bit weak, since it will not just "remove" the API key (from the list?), but rather make it unusable. I suggested "revoke" initially, but "invalidate" has consistency with our current documentation terminology, but it's interesting to discuss more.
I think there may have been a misunderstanding about the different "formats". The generation process is the same for all of them, it's just some post-create elaboration that changes, in order to provide an input that is more suitable for one use case or another. So the "type" is just a visualization thing, not a structural one. |
We are showing only the first 8 characters of the API key. We need to show this so that users can search for and invalidate a an API key. API Keys are different to users or roles in that the name isn't unique. The primary use cases for this page will be creating new API keys or finding an existing one and invalidating it. When finding an existing API key users will start with the API key, most likely copied from a specific application that they want to decommission, so it needs to be easy for them to find that key based on the actual API key and invalidate it. They wouldn't start with the name since that isn't stored anywhere when connecting an application with Elasticsearch and you could have multiple API Keys with the same name.
Unlike other resources (e.g. roles or alerts) in stack management, API keys are scoped and can only be created for the currently logged in user. We wouldn't have to show the username here if the form would be displayed in a personal space like account management but since it sits in stack management the user expectation is that they are managing the stack and not their user account. This could pose a security risk if they don't realise that any API key created by them will have the same permissions as their user.
It would show the user ID (e.g. "3890361203") instead of a username.
Yes, they're both identical.
I don't think it can be reversed. @bytebilly do you know if there's a way to reissue a previously invalidated API key or is that permanent?
The name is just descriptive and will be auto-generated in most cases as well (e.g. alerts). I would actually prefer to change this column to "Description".
An API key doesn't have a format/type. Every API key is the same. The issue is that different Elastic apps currently ask users to format or encode the API key differently in order to configure them. The way we should solve this issue is to agree on a single format and update our existing apps to support that so that we don't even have to show the dropdown menu at all.
Yeh, that would be much nice but unfortunately isn't feasible for this phase. Try and create a new role and have a look at all the options available there. We would have to find a way to cram all that into this form. :(
I had to remove that column due to limited space and felt that it was less important than the other columns. Happy to reconsider if we find a way to display this across all the different breakpoints. |
|
Thank you both for your responses 🙇♀️
Are the first 8 characters always unique? Just want to make sure that a search will always match one item and not several.
The general expectation is that the user who is logged in generates the API key. All possible doubts will be removed when the API key user is displayed in the table. Showing the user in the flyout implies that the user could be changed.
This is something that we definitely need to fix as:
Right now, when a Cloud user authenticates into Kibana, we are able to match the user and the ID.
Perhaps they can be grouped under the same category then, since their separation implies that the format is different.
We should then call the action
If the field becomes the description, then yes, it makes sense to go second. Although it can take a lot of real estate in the table if we don0t put a size limit to it.
++ it would be the ideal user experience,although I imagine it won't have that single format by 7.13, will it? If not, we should try to make more clear what happens there - it looks a bit confusing at the moment. |
|
Regarding default sorting - I wonder what's most important for a user: the time when it was created or the status of the API keys. API keys which is expire soon - do they need to be addressed? When would a user create a temporary API key? |
These characters are an encoded portion of the API key's ID, so the odds of a duplicate are very slim. For prior art, GitHub identifies specific commits by the first 7 characters of its hash, and there are significantly more commits within a repository than there will be API Keys within a deployment. All that said, the current iteration of this screen doesn't show any form of identifier. It just shows the API Key name, which has no uniqueness guarantees.
I think that's fair if the user already understands how Elasticsearch API Keys work. This screen lives within Kibana's general Stack Management app, which itself isn't specific to the current user. I think displaying the username helps to educate/reinforce the distinction, since it behaves differently than other security assets within Stack Management (such as roles and role mappings)
I don't agree with the implication, but I'm not opposed to revisiting this decision, either. We are rendering the username as a readonly piece of text -- not even within a disabled input field. I'm probably too close to this one, but that doesn't make me think it's editable.
I think we all agree, but this is unfortunately the best that we can do today given the information available to us. API Keys only store the |
There was a problem hiding this comment.
First pass through, mostly nits. In addition to the inline comments, we should add a couple functional tests once we're done addressing feedback:
- Accessibility tests within
x-pack/test/accessibility/apps. None currently exist for API Keys, but we should remedy that since we're adding functionality here. - An end-to-end test within
x-pack/test/functional/apps/api_keysto verify that API Keys can be created
Great job on this so far, I really like what you've done!
x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/public/management/api_keys/api_keys_grid/create_api_key_flyout.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/public/management/api_keys/api_keys_grid/create_api_key_flyout.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/public/management/api_keys/api_keys_grid/create_api_key_flyout.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Secondary review --
I love this! Awesome job! I identified a few nits in my comments below.
In addition: this has nothing to do with your changes, but I noticed a couple more things...
-
When you invalidate an API key, it immediately disappears. This was a bit jarring, and my expectation would be that if it's invalidated (but not deleted) I'd still be able to see it on this page. This may also help users troubleshooting if an API key stops working -- navigate to this page and realize that you're using a key that has been invalidated.
Perhaps we should consider changing the HTTP API to stop filtering out invalidated keys, and update the UI to display these with the appropriate status? This might be a follow-on task, but I wanted to bring the topic up here.
-
This fetches all API keys and stores them in memory. I could envision potential performance issues if users have lots of API keys (especially if my suggestion above is implemented). I don't think we have a great alternative right now though.
| <SelectableTokenField | ||
| options={[ |
There was a problem hiding this comment.
I was going to suggest making this full-width:
| <SelectableTokenField | |
| options={[ | |
| <SelectableTokenField | |
| fullWidth={true} | |
| options={[ |
but when I tested it locally, the input does not stretch to fill the entire container:
I'm not sure if this is a problem with EUI, or a problem with your TokenField/SelectableTokenField components. I figured I'd leave this here so you can let me know what you think 😄
There was a problem hiding this comment.
I played around with increasing the size of the field as well but if it's too wide it doesn't look like an input field anymore and the copy button gets lost so I kept it at the standard size.
| { | ||
| key: 'base64', | ||
| value: btoa(concatenated), | ||
| icon: 'empty', | ||
| label: i18n.translate('xpack.security.management.apiKeys.base64Label', { | ||
| defaultMessage: 'Base64', | ||
| }), | ||
| description: i18n.translate( | ||
| 'xpack.security.management.apiKeys.base64Description', | ||
| { | ||
| defaultMessage: 'Format used to authenticate with Elasticsearch.', | ||
| } | ||
| ), | ||
| }, |
There was a problem hiding this comment.
The current options are a mix of formats and purposes. I think it makes sense to focus on purposes, as that will be most intuitive to users.
Since the base64 option is meant to be used for authenticating to Elasticsearch, what do you think about changing the label and icon to match?
| { | |
| key: 'base64', | |
| value: btoa(concatenated), | |
| icon: 'empty', | |
| label: i18n.translate('xpack.security.management.apiKeys.base64Label', { | |
| defaultMessage: 'Base64', | |
| }), | |
| description: i18n.translate( | |
| 'xpack.security.management.apiKeys.base64Description', | |
| { | |
| defaultMessage: 'Format used to authenticate with Elasticsearch.', | |
| } | |
| ), | |
| }, | |
| { | |
| key: 'elasticsearch', | |
| value: btoa(concatenated), | |
| icon: 'logoElasticsearch', | |
| label: i18n.translate('xpack.security.management.apiKeys.elasticsearchLabel', { | |
| defaultMessage: 'Elasticsearch', | |
| }), | |
| description: i18n.translate( | |
| 'xpack.security.management.apiKeys.elasticsearchDescription', | |
| { | |
| defaultMessage: 'Format used to authenticate with Elasticsearch.', | |
| } | |
| ), | |
| }, |
Edit: reading the comments on this issue (there are a lot!) it seems there's some desire to standardize on Base64 format and put more emphasis on that. I'm a bit ambivalent about it now but I still lean towards being more clear that this is the format you need to authenticate to Elasticsearch.
There was a problem hiding this comment.
I like the idea but am worried that it could be confusing to have one option called "Elasticsearch" since they're all API keys for Elasticsearch. "Base64" is referenced throughout our docs as a format which is why I used it as a term here.
Once our products are aligned, I'd like to remove the entire dropdown menu and only show the base64 encoded API key.
| { | ||
| key: 'json', | ||
| value: JSON.stringify(this.state.createdApiKey), | ||
| icon: 'empty', | ||
| label: i18n.translate('xpack.security.management.apiKeys.jsonLabel', { | ||
| defaultMessage: 'JSON', | ||
| }), | ||
| description: i18n.translate( | ||
| 'xpack.security.management.apiKeys.jsonDescription', | ||
| { | ||
| defaultMessage: 'Full API response.', | ||
| } | ||
| ), | ||
| }, |
There was a problem hiding this comment.
If you agree with my comment above about changing the Base64 option (rename it to Elasticsearch), then maybe the JSON option should be moved to the end of the list, as it'll be the only one that is not formatted based on its purpose and it doesn't have an icon. WDYT?
| return ( | ||
| <EuiEmptyPrompt | ||
| iconType="alert" | ||
| body={ | ||
| <p> | ||
| <FormattedMessage | ||
| id="xpack.security.management.apiKeysEmptyPrompt.errorMessage" | ||
| defaultMessage="Could not load API keys." | ||
| /> | ||
| </p> | ||
| } | ||
| actions={children} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Before your changes, the grid would display an error's error (errorTitle), message, and statusCode fields to the user.
I have not tested this, but it looks like your changes will no longer display that information. It also looks like it would still display the button to create a new API key, which we probably don't want to show if we haven't been able to load the API keys at all. Thoughts?
There was a problem hiding this comment.
The page does not show the create button when initial data fetch failed.
I don't think we should show error codes or messages from the API unless they're intended to be displayed in the UI.
There was a problem hiding this comment.
Well, the page showed these details before, and other pages show similar details when we encounter unexpected errors. If we don't include this it will be difficult to troubleshoot in support cases.
There was a problem hiding this comment.
We chatted about this a bit offline, following up here --
To clarify, I tested this in both the current master branch and this PR by adding this bit to throw errors 50% of the time:
diff --git a/x-pack/plugins/security/server/routes/api_keys/get.ts b/x-pack/plugins/security/server/routes/api_keys/get.ts
index 7862401e1b8..0d44fe9acc3 100644
--- a/x-pack/plugins/security/server/routes/api_keys/get.ts
+++ b/x-pack/plugins/security/server/routes/api_keys/get.ts
@@ -29,6 +29,9 @@ export function defineGetApiKeysRoutes({ router }: RouteDefinitionParams) {
},
createLicensedRouteHandler(async (context, request, response) => {
try {
+ if (Math.random() < 0.5) {
+ throw new Error('Test error');
+ }
const isAdmin = request.query.isAdmin === 'true';
const apiResponse = await context.core.elasticsearch.client.asCurrentUser.security.getApiKey<{
api_keys: ApiKey[];Error in current master:
Error in this PR:
So it would be great if we could display the error status code and message in this callout.
Also I inadvertently discovered a bug -- if you encounter an error and click "Reload", even if the request is successful, the UI doesn't change. I tested this change and it fixed the issue:
diff --git a/x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.tsx b/x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.tsx
index 014030a8927..66eed00eb45 100644
--- a/x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.tsx
+++ b/x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.tsx
@@ -681,7 +681,7 @@ export class APIKeysGridPage extends Component<Props, State> {
try {
const { isAdmin } = this.state;
const { apiKeys } = await this.props.apiKeysAPIClient.getApiKeys(isAdmin);
- this.setState({ apiKeys });
+ this.setState({ apiKeys, error: undefined });
} catch (e) {
this.setState({ error: e });
}
x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_empty_prompt.tsx
Show resolved
Hide resolved
| }) | ||
| ); | ||
|
|
||
| describe('APIKeysGridPage', () => { |
There was a problem hiding this comment.
The rewritten tests are great!
There are a couple of code paths that are not exercised and may merit some additional test cases:
- Invalidate one API key (using the button on the right side)
- Select + invalidate 2+ API keys (using the button at the top)
- Switching the format of the API key -- though perhaps it would be better to add separate unit tests for
token_field.tsxinstead
Thoughts?
There was a problem hiding this comment.
Good catch, thanks!
| <EuiHealth color="primary"> | ||
| <FormattedMessage | ||
| id="xpack.security.management.apiKeys.table.statusActive" | ||
| defaultMessage="Active" |
There was a problem hiding this comment.
When I first looked at this page, it was not clear to me that "Active" means the key will never expire.
Perhaps we should reword this? "Active (never expires)" may be sufficient. Thoughts?
There was a problem hiding this comment.
I don't see this as an issue to be honest. I think active/inactive status is a fairly common pattern and shouldn't confuse users. My expectation wouldn't be that API Keys expire (or even have the ability to expire) unless specifically mentioned which is why I'm only calling it out if the API key has been specifically configured to automatically expire.
x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.tsx
Show resolved
Hide resolved
| const apiKey = await getAuthenticationService().apiKeys.create(request, request.body); | ||
|
|
||
| if (!apiKey) { | ||
| return response.badRequest({ body: { message: `API Keys are not available` } }); | ||
| } |
There was a problem hiding this comment.
It was not clear to me at first that the only way you're going to get a null / "falsey" result is if API keys are not enabled.
I don't think you need to add a redundant check for that, but perhaps an extra comment here clarifying would be a good idea.
There was a problem hiding this comment.
Yeh, I'm not sure why this was treated as a null return value rather than an error but the documentation for that belongs in the authentication service, not the route handler, so going to add a comment there instead.
Yeh, I agree, the decision to show expired API Keys but hide invalidated ones seems strange. Happy to make that change but think it should be a future improvement as we'd need to provide a way to hide expired API Keys or filter by status somehow.
I've raised this point too but the API does not allow pagination or server side search / filtering so our options are limited. |
|
@elasticmachine merge upstream |
There was a problem hiding this comment.
Thanks for addressing my feedback, this looks great!
The only thing we have left is to update the docs located at docs/user/security/api-keys/index.asciidoc:
- This includes a screenshot which needs updating to reflect the new design.
- It includes a section instructing users to visit the Kibana Console ("Dev tools") in order to create API Keys. We should instead direct them to the new interface.
There was a problem hiding this comment.
I looked at your most recent changes, looks great!
I have two last minor concerns (error message rendering and page behavior when an error is encountered), I added some details in a comment here: #92610 (comment)
…na into security/create-api-key
|
@jportner I've added a detailed error description accordion after discussing best option with design team.
@legrego As discussed, I have changed all references of "invalidate" to "delete" since invalidated API Keys are deleted automatically after 7 days anyways and there's no way to bring back invalidated keys so there's not much value in showing them to the user.
|
| @@ -5,182 +5,292 @@ | |||
| * 2.0. | |||
| */ | |||
|
|
|||
| import { EuiCallOut } from '@elastic/eui'; | |||
| import type { ReactWrapper } from 'enzyme'; | |||
| import { | |||
There was a problem hiding this comment.
We've noticed that the @testing-library-based tests have been timing out quite a bit, and appears to be the result of the latest test failures here. Do you know what could be causing these tests to run for so long? 15 seconds should be plenty of time for these tests to complete.
There was a problem hiding this comment.
Is the timeout per test or for the entire test suite?
All of them together do take 18-20 seconds on my local machine but individually are much faster:
- loads and displays API keys (163 ms)
- displays callout when API keys are disabled (28 ms)
- displays error when user does not have required permissions (12 ms)
- displays error when fetching API keys fails (58 ms)
- creates API key when submitting form, redirects back and displays base64 (3191 ms)
- creates API key with optional expiration, redirects back and displays base64 (3773 ms)
- deletes api key using cta button (159 ms)
- deletes multiple api keys using bulk select (744 ms)
There was a problem hiding this comment.
Not sure exactly what the timeout is but it just caused CI to fail again:
#97085
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* Added ability to create API keys * Remove hard coded colours * Added unit tests * Fix linting errors * Display full base64 encoded API key * Fix linting errors * Fix more linting error and unit tests * Added suggestions from code review * fix unit tests * move code editor field into separate component * fixed tests * fixed test * Fixed functional tests * replaced theme hook with eui import * Revert to manual theme detection * added storybook * Additional unit and functional tests * Added suggestions from code review * Remove unused translations * Updated docs and added detailed error description * Removed unused messages * Updated unit test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Larry Gregory <larry.gregory@elastic.co> # Conflicts: # x-pack/plugins/security/public/management/api_keys/api_keys_management_app.test.tsx # x-pack/plugins/security/public/management/api_keys/api_keys_management_app.tsx
* Added ability to create API keys (#92610) * Added ability to create API keys * Remove hard coded colours * Added unit tests * Fix linting errors * Display full base64 encoded API key * Fix linting errors * Fix more linting error and unit tests * Added suggestions from code review * fix unit tests * move code editor field into separate component * fixed tests * fixed test * Fixed functional tests * replaced theme hook with eui import * Revert to manual theme detection * added storybook * Additional unit and functional tests * Added suggestions from code review * Remove unused translations * Updated docs and added detailed error description * Removed unused messages * Updated unit test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Larry Gregory <larry.gregory@elastic.co> # Conflicts: # x-pack/plugins/security/public/management/api_keys/api_keys_management_app.test.tsx # x-pack/plugins/security/public/management/api_keys/api_keys_management_app.tsx * Updated translations
legrego
added
the
Team:Security
|
Pinging @elastic/kibana-security (Team:Security) |
Resolves #78222
Summary
Added the ability to create API keys and made UI improvements to the API keys page:
Screenshots
API keys page
Showing 8 first digits of API key alongside metadata.
Create API key flyout
Success state
Showing generated API key ready to be used to authenticate with Elasticsearch. (base64 encoded)
Format selection
We have no unified format for API Keys across the stack with different apps and language clients requiring different formats so added the ability to select the correct one per use case.
Code editor
Updated code editor component to look like an actual input element and improved lazy loading states.
Checklist
Delete any items that are not applicable to this PR.