Allow users to Update API Keys by kc13greiner · Pull Request #146237 · elastic/kibana

kc13greiner · 2022-11-23T21:33:37Z

Summary

API keys can now be updated via the API Keys Management screen

Release Note

API Keys can now be updated with new Role Descriptors and Metadata via the API Keys Management screen.

Testing Instructions

Login as elastic

Navigate to Roles and create a new role with the read_security cluster privilege:

Create a new user and assign that new role, viewer, and kibana_admin:

Navigate to Dev Tools and run the following:

POST /_security/api_key/grant
{
  "grant_type": "password",
  "username" : "elastic",  
  "password" : "changeme",  
  "run_as": "elastic",  
  "api_key" : {
    "name": "test-expired-key",
    "expiration": "1ms"
  }
}

POST /_security/api_key/grant
{
  "grant_type": "password",
  "username" : "elastic",  
  "password" : "changeme",  
  "run_as": "test_user",  
  "api_key" : {
    "name": "test-user-key",
    "expiration": "1d"
  }
}

The first command will create an API key for the elastic user that expires immediately.

The second command will create an API key for test_user.

Navigate to the API Key page, click the name column links to see a readonly view for the 2 previously created keys as users cannot update an API key that belongs to another user nor an API key that is expired.

Create a new API key:

Click the name link for the newly created API key to see the Update API key flyout.

Update the fields and click submit:

If the update was successful:

Now click the name link again for the updated key and click submit without making changes. You should see a warning:

Logout the elastic user and login as test_user

Navigate to API Keys and click the existing API Key to see a readonly view flyout:

Thanks for reviewing!

kc13greiner · 2022-11-28T12:26:29Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

+    const doesMetadataExist = Object.keys(selectedApiKey.metadata).length > 0;
+    const doCustomPrivilegesExist = Object.keys(selectedApiKey.role_descriptors ?? 0).length > 0;
+    const daysUntilExpiration = moment(selectedApiKey.expiration).diff(moment(), 'days', true);
+    const roundedDaysUntilExpiration =


Question to reviewer: Do you think this is an appropriate expiration value to display?

The expiration summary on the table page is certainly more natural to interpret (i.e. number of hours or mins). I noticed we're limited to days in the flyout (reason?), but I could see this being a more free-form input like we see in other duration/time-based controls (like in discover for example).

If we don't want to change the control to more than just days (e.g. weeks or months), I would suggest displaying either the originally configured lifetime OR the remaining number of whole days (if possible a special case of "<1" for less than 1 day but more than 0). And then always show the "expires in"/"expired" summary text underneath the control (same as what's in the table).

Right now it will also display negative days after a key has expired, which I am not sure was intended. Is it useful to know how long ago a key expired? If so, I would just leave the lifetime at zero and add a text summary (e.g. "expired 3 hours ago"). I think this would be more intuitive than negative floating-point days.

Thank you for the suggestion on this - it looks so much cleaner now with the Status

elasticmachine · 2022-11-30T14:59:32Z

Pinging @elastic/kibana-security (Team:Security)

jeramysoucy

Looks good! As discussed, let's ping Thom to get his thoughts on a couple of things.

jeramysoucy · 2022-12-05T13:24:36Z

docs/user/security/api-keys/index.asciidoc

@@ -62,6 +71,3 @@ created by which user in which realm.
 If you have only the `manage_own_api_key` permission, you see only a list of your own keys.


This seems to also be true if you have the 'read_sec' permission and access to the API key's screen (via kibana admin or viewer role)?

Great catch! I've added a line in the docs to reference the readonly view. @gchaps Would you mind reviewing the new sentence I added?

jeramysoucy · 2022-12-05T13:27:44Z

docs/user/security/api-keys/index.asciidoc

+
+To update an API key, open the main menu, then click *Stack Management > API Keys*, then click on the name of an existing API key.
+
+Only the `Restrict privileges` and `metadata` fields can be updated.


Just for my own knowledge, is it pretty typical (on other platforms/apis) that expiration dates are locked once an API key is created?

The original ticket said only those fields could be updated (that appears to be all ES allows). I think the process if a key is expired is to invalidate it and re- create

If Elasticsearch allows updating expiration time then we should support it in the UI as well

Elasticsearch only allows those fields in the request body

jeramysoucy · 2022-12-05T16:24:12Z

x-pack/plugins/security/public/management/api_keys/api_keys_api_client.test.ts

    });
  });
+
+  it('updateApiKey() calls correct endpoint', async () => {


Complete nit, but the rest of the tests use 'queries the correct endpoint' language.

jeramysoucy · 2022-12-05T16:42:20Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

  defaultValues?: ApiKeyFormValues;
-  onSuccess?: (apiKey: CreateApiKeyResponse) => void;
+  onSuccess?: (
+    createdApiKeyResponse: CreateApiKeyResponse | undefined,


Consistency nit

Suggested change

createdApiKeyResponse: CreateApiKeyResponse | undefined,

createApiKeyResponse: CreateApiKeyResponse | undefined,

jeramysoucy · 2022-12-05T17:06:55Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

+    const doesMetadataExist = Object.keys(selectedApiKey.metadata).length > 0;
+    const doCustomPrivilegesExist = Object.keys(selectedApiKey.role_descriptors ?? 0).length > 0;
+    const daysUntilExpiration = moment(selectedApiKey.expiration).diff(moment(), 'days', true);
+    const roundedDaysUntilExpiration =


The expiration summary on the table page is certainly more natural to interpret (i.e. number of hours or mins). I noticed we're limited to days in the flyout (reason?), but I could see this being a more free-form input like we see in other duration/time-based controls (like in discover for example).

If we don't want to change the control to more than just days (e.g. weeks or months), I would suggest displaying either the originally configured lifetime OR the remaining number of whole days (if possible a special case of "<1" for less than 1 day but more than 0). And then always show the "expires in"/"expired" summary text underneath the control (same as what's in the table).

Right now it will also display negative days after a key has expired, which I am not sure was intended. Is it useful to know how long ago a key expired? If so, I would just leave the lifetime at zero and add a text summary (e.g. "expired 3 hours ago"). I think this would be more intuitive than negative floating-point days.

jeramysoucy · 2022-12-05T17:41:33Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.test.tsx

      expect(await findByText('You do not have permission to create API keys')).toBeInTheDocument();
      expect(queryByText('Create API key')).toBeNull();
    });
-


Should we include cases to check for manage-own vs. manage all view? Test callouts?

Since we are mocking the data from ES it will show whatever we put in the mock. Looking at the routes/api-keys/get.ts it passes an isAdmin value which I believe is the flag that determines if the user gets all the keys or just theirs.

jeramysoucy · 2022-12-05T17:59:09Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

+  }
+
+  return result;
+}


There's no api_key_flyout.test.tsx file. Should we add one that tests the flyout in readonly vs. create vs. edit mode?

EDIT: Looks like this is all tested by the functional tests in x-pack/test/functional/apps/api_keys/home_page.ts

azasypkin · 2022-12-06T09:19:32Z

Tagging @gchaps for the docs changes.

gchaps

LGTM with a few minor comments

docs/user/security/api-keys/index.asciidoc

gchaps · 2022-12-07T00:42:58Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

-                    'xpack.security.accountManagement.createApiKey.customExpirationInputLabel',
+                    'xpack.security.accountManagement.apiKeyFlyout.customExpirationInputLabel',
                    {
                      defaultMessage: 'Lifetime (days)',


Can we remove (days) here as the word "days" is included in the field?

@gchaps Just the days in the label? or on the input box as well?

Just the days in the label.

(days) removed in latest commit!

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

thomheymann

All working well, a few comments below.

The "View API key" button doesn't make any sense (what would it do if it was enabled?) - I would change it to "Close":

I appreciate that you tried to be consistent with the Create API key flow here but that flow is an exception since we need to show the token of the generated API key. Normal updates and errors should be shown using toast notifications so the behaviour is consistent with the rest of Kibana:

The "no update has been made" warning is an unnecessary detail. I would either disable the submit button until a change has been made (similar to user profile form) or show a success toast irregardless of whether the API key actually changed or not:

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.tsx

thomheymann · 2022-12-08T12:28:25Z

Forgot to mention, can we move the "Status" field before or after the two checkboxes so the UI looks a bit neater?

gchaps · 2022-12-12T16:42:56Z

Some comments on copy.

Can we eliminate the back-to-back "API keys" text in the intro? Maybe something like

Manage your API keys, which send requests on behalf of the user.

Open to suggestions for the second half of the sentence.

Also, instead of "View API key" for the title of the flyout, how about "API key details"?

kc13greiner · 2022-12-13T14:42:47Z

All working well, a few comments below.

The "View API key" button doesn't make any sense (what would it do if it was enabled?) - I would change it to "Close":

I appreciate that you tried to be consistent with the Create API key flow here but that flow is an exception since we need to show the token of the generated API key. Normal updates and errors should be shown using toast notifications so the behaviour is consistent with the rest of Kibana:

The "no update has been made" warning is an unnecessary detail. I would either disable the submit button until a change has been made (similar to user profile form) or show a success toast irregardless of whether the API key actually changed or not:

All fixed in latest commit! I chose to leave the 'Update' button clickable and have it return 'Success' even when no updates were made; the Users and Roles screens appear to follow that pattern.

kc13greiner · 2022-12-13T14:43:12Z

Forgot to mention, can we move the "Status" field before or after the two checkboxes so the UI looks a bit neater?

Fixed in latest commit!

kc13greiner · 2022-12-13T14:43:59Z

Some comments on copy.

Can we eliminate the back-to-back "API keys" text in the intro? Maybe something like

Manage your API keys, which send requests on behalf of the user.

Open to suggestions for the second half of the sentence.

Also, instead of "View API key" for the title of the flyout, how about "API key details"?

Fixed in latest commit!

kc13greiner · 2022-12-13T16:23:33Z

@elasticmachine merge upstream

kibana-ci · 2022-12-13T19:52:27Z

💚 Build Succeeded

Buildkite Build
Commit: cb5cc5a

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`security`	90	91	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`security`	549.6KB	553.1KB	+3.5KB

Unknown metric groups

API count

id	before	after	diff
`security`	250	251	+1

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	60	66	+6
`osquery`	109	115	+6
`securitySolution`	445	451	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	69	75	+6
`osquery`	110	117	+7
`securitySolution`	521	527	+6
total			+21

History

💔 Build #94445 failed 5eacf4a
💔 Build #94411 failed 9ecb176
💔 Build #94390 failed 92079c8
💔 Build #94384 failed 1a10fc8
💔 Build #94346 failed 200e57e
💚 Build #93507 succeeded f23230a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

thomheymann

All looking good! 🎉

* main: (21 commits) [Profiling] Remove link to 'Other' bucket (elastic#147523) [Synthetics UI] Add missing configuration options to the add/edit monitor forms (elastic#147265) [DOCS] Updates what's new pages (elastic#147483) [Fleet][Endpoint][RBAC V2] Update fleet router and config to allow API access via RBAC controls (elastic#145361) [Guided onboarding] Update guide IDs (elastic#147348) [Synthetics] Add synthetics settings alerting default (elastic#147339) [Security Solution][Endpoint] Fix Policy form being displayed as Read Only when displayed in Fleet pages (elastic#147212) [Cases] Save draft user comment (elastic#146327) [API Docs] Fix `--plugin` filter (elastic#147500) [Fleet] added a logic to use `destinationId` when tagging imported SOs (elastic#147439) Do not skip UPDATE_TARGET_MAPPINGS if upgrading to a newer stack version (elastic#147503) [Discover] Validate if Data View time field exists on Alert creation / editing (elastic#146324) [Discover] Fix Discover navigation from Lens embeddable (elastic#147000) Allow users to Update API Keys (elastic#146237) Update dependency xstate to ^4.35.0 (main) (elastic#147463) [Behavioral Analytics] Remove feature flag to hide functionality (elastic#147429) [Fleet] Add agent policy `inactivity_timeout`experimental setting (elastic#147432) [APM] Switching service groups from grid to flex layout (elastic#147448) [Fleet] Add missing endpoints to openApi specs (elastic#147452) [AO] Allow providing custom time range for Alert Summary Widget (elastic#147253) ...

## Summary API keys can now be updated via the API Keys Management screen ## Release Note API Keys can now be updated with new Role Descriptors and Metadata via the API Keys Management screen. ## Testing Instructions Login as `elastic` Navigate to Roles and create a new role with the `read_security` cluster privilege: <img width="962" alt="Screen Shot 2022-11-30 at 9 42 31 AM" src="https://user-images.githubusercontent.com/21210601/204826868-a8f6bf03-acf8-404c-90c8-e2b9ab62dc11.png"> Create a new user and assign that new role, `viewer`, and `kibana_admin`: <img width="936" alt="Screen Shot 2022-11-30 at 9 43 10 AM" src="https://user-images.githubusercontent.com/21210601/204827030-e5f97f8e-6676-4c18-8a46-f6afee87ba12.png"> Navigate to Dev Tools and run the following: ```json POST /_security/api_key/grant { "grant_type": "password", "username" : "elastic", "password" : "changeme", "run_as": "elastic", "api_key" : { "name": "test-expired-key", "expiration": "1ms" } } POST /_security/api_key/grant { "grant_type": "password", "username" : "elastic", "password" : "changeme", "run_as": "test_user", "api_key" : { "name": "test-user-key", "expiration": "1d" } } ``` The first command will create an API key for the `elastic` user that expires immediately. The second command will create an API key for `test_user`. Navigate to the API Key page, click the name column links to see a readonly view for the 2 previously created keys as users cannot update an API key that belongs to another user nor an API key that is expired. Create a new API key: <img width="632" alt="Screen Shot 2022-11-30 at 9 44 52 AM" src="https://user-images.githubusercontent.com/21210601/204829114-672c6583-8801-4af0-bfa8-64ae1072ef46.png"> Click the name link for the newly created API key to see the Update API key flyout. Update the fields and click submit: <img width="642" alt="Screen Shot 2022-11-30 at 9 45 59 AM" src="https://user-images.githubusercontent.com/21210601/204829914-9fb1f8e6-8b3f-4acc-b63f-d7e4a0906727.png"> If the update was successful: <img width="904" alt="Screen Shot 2022-11-30 at 9 46 42 AM" src="https://user-images.githubusercontent.com/21210601/204830133-1dcb083b-f945-4980-9e91-19081c224b55.png"> Now click the name link again for the updated key and click submit without making changes. You should see a warning: <img width="895" alt="Screen Shot 2022-11-30 at 9 46 52 AM" src="https://user-images.githubusercontent.com/21210601/204830570-2ca5e2e0-19b6-43ce-b7e4-ae594be6a86b.png"> Logout the `elastic` user and login as `test_user` Navigate to API Keys and click the existing API Key to see a readonly view flyout: <img width="639" alt="Screen Shot 2022-11-30 at 9 58 25 AM" src="https://user-images.githubusercontent.com/21210601/204832019-640ecd2e-4bcb-402b-a164-e8b8eb9f8848.png"> Thanks for reviewing! Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kc13greiner added 2 commits November 18, 2022 15:48

Let users update API Keys

46710bc

Adding tests and readonly

e1478f0

kc13greiner added Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v8.7.0 release_note:feature Makes this part of the condensed release notes labels Nov 23, 2022

kc13greiner added 7 commits November 23, 2022 16:43

Removing debugging logs

d946bfe

Fixing message formatting

96fd016

Adding braces around sub value

61a22e7

Fixing translations

4658de1

plugin test fix

1d493ce

Changing other message IDs

1187d80

Merge branch 'main' into feature/api_keys_update

912f5dd

kc13greiner commented Nov 28, 2022

View reviewed changes

Merge branch 'main' into feature/api_keys_update

38245c7

kc13greiner linked an issue Nov 28, 2022 that may be closed by this pull request

Allow API Keys to be updated #135217

Closed

kc13greiner added 7 commits November 28, 2022 08:03

Updating API Key docs

227d11d

Fixing error message title and adding API test

0e27f9e

Fixing for ES validations

c3d819d

Fixing values sent and username when updating/viewing

6626a74

Merge branch 'main' into feature/api_keys_update

9da296b

Fixing API test

6084e4d

Removing unused services

d317cd4

kc13greiner marked this pull request as ready for review November 30, 2022 14:59

kc13greiner requested a review from a team as a code owner November 30, 2022 14:59

jeramysoucy assigned jeramysoucy and unassigned jeramysoucy Dec 5, 2022

jeramysoucy reviewed Dec 5, 2022

View reviewed changes

azasypkin requested a review from gchaps December 6, 2022 09:18

gchaps approved these changes Dec 7, 2022

View reviewed changes

kc13greiner and others added 5 commits December 7, 2022 22:38

PR Review feedback

7b27465

Update docs/user/security/api-keys/index.asciidoc

f1b81f2

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

Update docs/user/security/api-keys/index.asciidoc

cb41247

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

Adding docs update about readonly mode

337988e

Merge branch 'main' into feature/api_keys_update

f23230a

thomheymann reviewed Dec 8, 2022

View reviewed changes

kc13greiner added 2 commits December 13, 2022 09:36

Changes for PR feedback

1a8c8f7

Merge branch 'main' into feature/api_keys_update

200e57e

kc13greiner added 2 commits December 13, 2022 10:48

Fixing test

1a10fc8

Merge branch 'main' into feature/api_keys_update

92079c8

kibanamachine and others added 3 commits December 13, 2022 11:23

Merge branch 'main' into feature/api_keys_update

9ecb176

Merge branch 'main' into feature/api_keys_update

5eacf4a

Merge branch 'main' into feature/api_keys_update

cb5cc5a

kc13greiner requested a review from thomheymann December 13, 2022 22:04

thomheymann approved these changes Dec 14, 2022

View reviewed changes

kc13greiner merged commit 9a6985e into elastic:main Dec 14, 2022

kibanamachine added the backport:skip This PR does not require backporting label Dec 14, 2022

kc13greiner deleted the feature/api_keys_update branch December 14, 2022 12:58

		@@ -62,6 +71,3 @@ created by which user in which realm.
		If you have only the `manage_own_api_key` permission, you see only a list of your own keys.


		To update an API key, open the main menu, then click Stack Management > API Keys, then click on the name of an existing API key.

		Only the `Restrict privileges` and `metadata` fields can be updated.

	createdApiKeyResponse: CreateApiKeyResponse \| undefined,
	createApiKeyResponse: CreateApiKeyResponse \| undefined,

Conversation

kc13greiner commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Release Note

Testing Instructions

Uh oh!

kc13greiner Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 30, 2022

Uh oh!

jeramysoucy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kc13greiner Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin commented Dec 6, 2022

Uh oh!

gchaps left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomheymann left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomheymann commented Dec 8, 2022

Uh oh!

gchaps commented Dec 12, 2022

Uh oh!

kc13greiner commented Dec 13, 2022

Uh oh!

kc13greiner commented Dec 13, 2022

Uh oh!

kc13greiner commented Nov 23, 2022 •

edited

Loading

kc13greiner Nov 28, 2022 •

edited

Loading

kc13greiner Dec 8, 2022 •

edited

Loading

thomheymann left a comment •

edited

Loading