Add a DELETE endpoint for API Keys #37051

johnswanson · 2023-12-21T18:27:08Z

Don't actually delete them from the database (to keep the invariant that a user with an :api-key type will always have an associated ApiKey). Just mark them as :archived, and:

make them invisible to the count endpoint
make them invisible to the list endpoint
make them not work for authentication

cypress · 2023-12-21T18:56:54Z

Passing run #1266 ↗︎

0	2201	155	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Add a DELETE endpoint for API Keys
Project: Metabase e2e	Commit: `53c26af6e5`
Status: Passed	Duration: 15:33 💡
Started: Jan 2, 2024 6:50 PM	Ended: Jan 2, 2024 7:05 PM

Review all test suite changes for PR #37051 ↗︎

johnswanson · 2023-12-21T21:02:24Z

Current dependencies on/for this PR:

master
- PR Add updated_by to API Keys #37014
  - PR Add a DELETE endpoint for API Keys #37051 👈
    - PR Return group instead of just group_name for ApiKeys #37061
      - PR Oops - remove API bits from ApiKey model #37085
        
        PR Use simple Secrets for ApiKeys #37205
        
        PR add CopyTextInput component #37021
        
        PR API Keys: add AuthCard and listing page #36258

This stack of pull requests is managed by Graphite.

noahmoss · 2023-12-22T15:47:24Z

What's the advantage of archiving API keys instead of deleting the API Key and the associated user?

Normal users can only be archived, not deleted, but api-key users don't necessarily have to have the same constraint since they're more of an internal mechanism than actual users.

And there's the slight risk that a bug in the future could allow deactivated API keys to still be used for auth. I know you've added checks to prevent that but it's still a potential risk if we're not allowing API keys to be fully deleted.

Don't have a strong opinion necessarily, but just want to make sure we have a good justification for the approach here.

johnswanson · 2023-12-22T15:55:24Z

What's the advantage of archiving API keys instead of deleting the API Key and the associated user?

Normal users can only be archived, not deleted, but api-key users don't necessarily have to have the same constraint since they're more of an internal mechanism than actual users.

Deleting a user would cause other issues - for example, if an API key user creates a dashboard, then deleting that user would break the creator_id of that dashboard (I think there's a foreign key constraint to prevent that).

And there's the slight risk that a bug in the future could allow deactivated API keys to still be used for auth. I know you've added checks to prevent that but it's still a potential risk if we're not allowing API keys to be fully deleted.

This is a good point to consider. What would you think of regenerating the API key as part of "deletion"? That way any existing key becomes unusable even if there's a bug that allows auth with a "deleted" key.

noahmoss · 2023-12-22T16:07:15Z

Gotcha. Yeah I'd be in favor of either:

Overwriting the hashed key in the API Key with a dummy value or just set it to nil (maybe nil would be clearer)
Don't necessarily require that API Key users have an associated API Key, and delete the API Key but leave the user as archived

johnswanson · 2023-12-22T17:19:15Z

Gotcha. Yeah I'd be in favor of either:

Overwriting the hashed key in the API Key with a dummy value or just set it to nil (maybe nil would be clearer)

Don't necessarily require that API Key users have an associated API Key, and delete the API Key but leave the user as archived

One additional point I want to consider is auditability. Say

someone uses an API key to do something malicious
later, for unrelated reasons, that ApiKey is deleted
later, the malicious activity is noticed

We'd have an audit log that showed that User 123, with type :api_key, performed some malicious activity. But we wouldn't have any way of figuring out which ApiKey did it.

I think this makes me lean towards the first option - overwriting the ApiKey with a nil hashed key. That way, if you have a user ID that performed a malicious action, you can always get the associated ApiKey.

Does that sound reasonable? Another option would be to add user_id to the audited fields for ApiKeys, so the deletion audit log event contains the user ID. But this feels a bit less obvious.

EDIT: I'm going with option 2, deleting the ApiKey but leaving the User: https://metaboat.slack.com/archives/C064EB1UE5P/p1703284432369899?thread_ts=1703183375.589429&cid=C064EB1UE5P

noahmoss · 2023-12-27T21:13:34Z

src/metabase/api/api_key.clj

+    (let [api-key (-> (t2/select-one :model/ApiKey id)
+                      (t2/hydrate :group_name))]


Not a huge deal, but might be worth wrapping this in check-404. Even though it's a deletion endpoint, we should probably return a 404 if the caller is trying to delete something that already doesn't exist.

The User update call below would also error in this case anyway, so probably better to catch that at the start and error.

Good call. I'm also going to add this check to the PUT endpoints.

noahmoss · 2023-12-27T21:15:04Z

src/metabase/api/api_key.clj

+      (events/publish-event! :event/api-key-delete
+                             {:object api-key
+                              :user-id api/*current-user-id*})


Here's another case where I don't think the publish-event! call should be in the transaction, since we don't know what downstream DB calls are generated

Oops, thanks for noticing this - sorry about that!

noahmoss · 2023-12-27T21:15:47Z

src/metabase/api/api_key.clj

+      (events/publish-event! :event/api-key-delete
+                             {:object api-key
+                              :user-id api/*current-user-id*})
+      (present-api-key api-key))))


Is there a specific reason we need to return the API key after it's been deleted? If not, you could just return a 204 (api/generic-204-no-content)

Good point, done!

When the endpoint is hit, we delete the ApiKey from the database. The user is left behind to ensure that we don't break foreign keys, and to enhance auditability.

github-actions · 2024-01-02T19:58:21Z

@johnswanson Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

metabase-bot bot assigned johnswanson Dec 21, 2023

metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label Dec 21, 2023

johnswanson force-pushed the add-updated-by-to-api-keys branch from a5a89d4 to 8a9f0f9 Compare December 21, 2023 20:31

johnswanson force-pushed the delete-endpoint-for-api-keys branch from d53d92c to 9b25c1a Compare December 21, 2023 21:02

johnswanson mentioned this pull request Dec 21, 2023

Add updated_by to API Keys #37014

Merged

johnswanson force-pushed the delete-endpoint-for-api-keys branch from 9b25c1a to 90cb001 Compare December 21, 2023 21:04

johnswanson mentioned this pull request Dec 21, 2023

Return group instead of just group_name for ApiKeys #37061

Merged

johnswanson force-pushed the add-updated-by-to-api-keys branch 2 times, most recently from 45993e7 to ecce8b4 Compare December 22, 2023 14:37

johnswanson force-pushed the delete-endpoint-for-api-keys branch from 90cb001 to 7b6fb13 Compare December 22, 2023 14:37

johnswanson added the no-backport Do not backport this PR to any branch label Dec 22, 2023

johnswanson force-pushed the add-updated-by-to-api-keys branch from ecce8b4 to 0964f09 Compare December 22, 2023 14:53

Base automatically changed from add-updated-by-to-api-keys to master December 22, 2023 15:38

johnswanson force-pushed the delete-endpoint-for-api-keys branch from 7b6fb13 to a8f0183 Compare December 22, 2023 15:39

johnswanson force-pushed the delete-endpoint-for-api-keys branch from a8f0183 to cc9db8b Compare December 22, 2023 18:04

johnswanson mentioned this pull request Dec 22, 2023

Oops - remove API bits from ApiKey model #37085

Merged

johnswanson requested a review from a team December 22, 2023 18:17

This was referenced Dec 22, 2023

add CopyTextInput component #37021

Closed

API Keys: add AuthCard and listing page #36258

Closed

shaunlebron force-pushed the delete-endpoint-for-api-keys branch 2 times, most recently from f230bda to 7d65dcd Compare December 22, 2023 22:43

noahmoss approved these changes Dec 27, 2023

View reviewed changes

johnswanson force-pushed the delete-endpoint-for-api-keys branch 2 times, most recently from 9a63671 to 265e21e Compare January 2, 2024 14:44

johnswanson force-pushed the delete-endpoint-for-api-keys branch from 265e21e to 1d1ee6c Compare January 2, 2024 18:12

Add a DELETE endpoint for API Keys

6e93a7a

When the endpoint is hit, we delete the ApiKey from the database. The user is left behind to ensure that we don't break foreign keys, and to enhance auditability.

johnswanson force-pushed the delete-endpoint-for-api-keys branch from 1d1ee6c to 6e93a7a Compare January 2, 2024 18:36

johnswanson merged commit fbb1202 into master Jan 2, 2024
105 checks passed

johnswanson deleted the delete-endpoint-for-api-keys branch January 2, 2024 19:58

johnswanson mentioned this pull request Jan 2, 2024

Use simple Secrets for ApiKeys #37205

Merged

johnswanson added this to the 0.49 milestone Jan 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a DELETE endpoint for API Keys #37051

Add a DELETE endpoint for API Keys #37051

johnswanson commented Dec 21, 2023

cypress bot commented Dec 21, 2023 •

edited

Loading

johnswanson commented Dec 21, 2023 •

edited

Loading

noahmoss commented Dec 22, 2023

johnswanson commented Dec 22, 2023

noahmoss commented Dec 22, 2023

johnswanson commented Dec 22, 2023 •

edited

Loading

noahmoss Dec 27, 2023

johnswanson Jan 1, 2024

noahmoss Dec 27, 2023

johnswanson Jan 1, 2024

noahmoss Dec 27, 2023

johnswanson Jan 1, 2024

github-actions bot commented Jan 2, 2024

		(let [api-key (-> (t2/select-one :model/ApiKey id)
		(t2/hydrate :group_name))]

Add a DELETE endpoint for API Keys #37051

Add a DELETE endpoint for API Keys #37051

Conversation

johnswanson commented Dec 21, 2023

cypress bot commented Dec 21, 2023 • edited Loading

Passing run #1266 ↗︎

Review all test suite changes for PR #37051 ↗︎

johnswanson commented Dec 21, 2023 • edited Loading

noahmoss commented Dec 22, 2023

johnswanson commented Dec 22, 2023

noahmoss commented Dec 22, 2023

johnswanson commented Dec 22, 2023 • edited Loading

noahmoss Dec 27, 2023

Choose a reason for hiding this comment

johnswanson Jan 1, 2024

Choose a reason for hiding this comment

noahmoss Dec 27, 2023

Choose a reason for hiding this comment

johnswanson Jan 1, 2024

Choose a reason for hiding this comment

noahmoss Dec 27, 2023

Choose a reason for hiding this comment

johnswanson Jan 1, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 2, 2024

cypress bot commented Dec 21, 2023 •

edited

Loading

johnswanson commented Dec 21, 2023 •

edited

Loading

johnswanson commented Dec 22, 2023 •

edited

Loading