API key enhancements

[itisAliRH]

Closes #13328 and Closes #10742

This pr migrates the API key endpoint to Fast API, vuetify the client, and the user can mark the API key as deleted.

• Client
• • List the latest created key
• • Create a new key when there is no key available
• • Delete a key
• • Add route
• • Add to user preferences list
• • Delete old UI files
• Backend
• • Create managers
• • Create services
• • Add endpoints
• • Delete old endpoints

API changes

We made some retro-compatible API changes around the API key management endpoints:

• The APIKeys model now has a deleted column. It has been added to the galaxy model and the toolshed model.
• Dropped the GET /api/users/{id}/api_key/inputs that returned additional data for rendering the API key in the UI.
• Dropped the PUT /api/users/{id}/api_key/inputs that created a new API key and returned data for rendering the UI.
• Added the GET /api/users/{id}/api_key/detailed that now returns the APIKeyModel or 204 status code if there's no key.

  class APIKeyModel(BaseModel):
      key: str = Field(Required, title="Key", description="API key to interact with the Galaxy API")
      create_time: datetime = Field(
          Required, title="Create Time", description="The time and date this API key was created."
      )

• Added DELETE /api/users/{user_id}/api_key to delete/revoke (all*) current API key.
• The rest of the related endpoints preserve their usual behaviour.

Important Note (*): Since the old button to generate an API was always enabled, a user might have multiple API keys in the database if it was used multiple times. Deleting the API key will mark all existing API keys as deleted.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these contributions under Galaxy's current license.
• I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to galaxy-committers@lists.galaxyproject.org.

[mvdbeek]

and user can has more than one API key in same time

I think that's an idea we need to discuss separately, can you move that change into a separate PR ? Maybe also outline why this would be needed, what the usecase is (keep in mind we have no way to give a different set of permissions to API keys, which is what you'd use multiple api keys or tokens for)

[itisAliRH]

and user can has more than one API key in same time

I think that's an idea we need to discuss separately, can you move that change into a separate PR ? Maybe also outline why this would be needed, what the usecase is (keep in mind we have no way to give a different set of permissions to API keys, which is what you'd use multiple api keys or tokens for)

Sure, I'll make a new PR and put changes related to multiple keys there.

With having different permissions, this feature is even more helpful(any plan/issue/pr?). Still, even without it, the user can have other keys for various projects and, if needed, and if one of them must be deleted, there is no need to change all the project keys (each key should have a description or name).

[mvdbeek]

Still, even without it, the user can have other keys for various projects and, if needed, and if one of them must be deleted,

... at which point you need to regenerate all of them, since you can generate sessions and look at other API keys or even create new ones. Essentially having one would equate to having all keys. We can work around that, but then we've halfway implemented token scopes, and I think we'd be better off doing that properly.

[bgruening]

xref: #1524

[itisAliRH]

Still, even without it, the user can have other keys for various projects and, if needed, and if one of them must be deleted,

... at which point you need to regenerate all of them, since you can generate sessions and look at other API keys or even create new ones. Essentially having one would equate to having all keys. We can work around that, but then we've halfway implemented token scopes, and I think we'd be better off doing that properly.

I agree with you. I read the mentioned issue, and we can discuss it later.
Thus, this pr will migrate API to Fast API, vutify and add a delete button.

[davelopez]

Are the API changes in the PR description desirable or should we try to maintain the old API exactly as it is and create alternative endpoints or media types?

[mvdbeek]

So removing one API key will obviously just show the next available one.

That's problematic, I suggest we mark all old keys as deleted.

I would not change the return value for API keys, I don't think we'll introduce "scoped" api keys, this should use a different infrastructure for generating tokens, with a different endpoint.

[davelopez]

Thanks for having a look Marius!

I would not change the return value for API keys, I don't think we'll introduce "scoped" api keys, this should use a different infrastructure for generating tokens, with a different endpoint.

Ok, that makes sense. So then, do you mean we should return the API key as plain text instead of the model with key and create_time?
The create_time was just nice to have as a reference in the UI

But it certainly changes the old API behavior... since it always returned an API key as plain text (it created a new one if it didn't exist) and now it returns the model or 204 if there is no API key explicitly generated.

Please note that the old API was dealing with lists of APIKeys too, just it always returned the [0] index of the list which is what we are doing in UsersService.get_api_key function.

What about the /api/users/{id}/api_key/inputs endpoints? Should we drop or restore them? They seem to be coupled to the UI.

That's problematic, I suggest we mark all old keys as deleted.

The APIKeys database model doesn't have a deleted column so we cannot mark them as deleted. Still, we can try to directly remove all the user's keys whenever the delete endpoint is called (so no key path parameter is needed). Would that be acceptable?

[mvdbeek]

The APIKeys database model doesn't have a deleted column so we cannot mark them as deleted

Yes, we should add this column if we really want to delete API keys (as opposed to generating a new API key). We can't delete a key and then activate an old API key, that's a no-go (say you've created a new API key after exposing the old one in a screencast or similar, now the next time you delete an API key the leaked API key will be valid). This wasn't previously a problem, because we'd just generate a new one. Which, to me, was absolutely fine.

So then, do you mean we should return the API key as plain text instead of the model with key and create_time?
The create_time was just nice to have as a reference in the UI

I agree it looks nice, but I don't think it's worth breaking backwards compatibility over this. In fact I don't think we should even change the GET being a get-or-create, cause again, people may rely on this. Maybe put the new behavior under /api/users/{id}/api_key/v2 ... I don't like it, but I see that this would be required for a more reasonable UX.

What about the /api/users/{id}/api_key/inputs endpoints? Should we drop or restore them?

That's fine with me, I doubt they've been used in scripts.

[davelopez]

This one should be ready to go :)
The PR description is updated with the current API changes after addressing the comments.
We are happy to tweak it further if needed 👍

[nsoranzo]

I'm not sure I understand the use case for adding the deleted column instead of just getting rid of the API key row from the database? Marius already hinted at this in a previous comment, I think, but undeleting an API key is something you don't want to allow, so these rows just unnecessarily occupy space in the database.

[mvdbeek]

It's for auditing, and the logic problem of just using the previous key, which will fall back to an old key of you just straight up delete the API key from the database. The alternative is a migration that drops all non-latest keys, but I am unsure if that is really the best approach.

[nsoranzo]

If it's for auditing then I think it's fine, thanks!

[github-actions]

This PR was merged without a "kind/" label, please correct.