Improved API key handling #3094
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tillprochaska
force-pushed
the
feature/1027-reset-api-key
branch
2 times, most recently
from
3ea2d5a
to
71e7678
Compare
|
Related idea: Is it necessary to create an API key for every user by default? Most users probably do not use the API at all. We could reuse the logic to reset to let users create an initial key on demand. |
tillprochaska
force-pushed
the
feature/1027-reset-api-key
branch
2 times, most recently
from
e135d0d
to
5a88cea
Compare
|
@Rosencrantz As mentioned before I still need to double-check a few technical implementation details, but let me know what you think about the UX. |
tillprochaska
force-pushed
the
feature/1027-reset-api-key
branch
from
5a88cea
to
a9d0f8a
Compare
|
I’ve converted this back to a draft as I started implementing additional changes on this branch to avoid merge conflicts. However, all changes on this branch are ready for review and I’d appreciate a review of what’s already there. |
tillprochaska
force-pushed
the
feature/1027-reset-api-key
branch
from
d42f7a2
to
c20c3d8
Compare
Rosencrantz
reviewed
May 6, 2024
tillprochaska
commented
May 8, 2024
tillprochaska
commented
May 8, 2024
tillprochaska
commented
May 8, 2024
tillprochaska
commented
May 8, 2024
tillprochaska
commented
May 16, 2024
tillprochaska
commented
May 22, 2024
| </h4> | ||
|
|
||
| <p> | ||
| {expiresAt && ( |
There was a problem hiding this comment.
This currently only handles API keys that will expire in the future. There should be a separate case that handles API keys that have expired.
Case 1: User doesn’t have an API key:
You need an API key in order to access the Aleph API. Generate an API key to get started."
Case 2: User has an API key that hasn’t yet expired:
Your API key will expire on {date}. You can regenerate your API key at any time. When you regenerate your API key, your old key will stop working.
Case 3: User has an expired API key.
Your API key expired on {date}. Regenerate your API key to continue using the Aleph API.
Case 4: User has an API key that does not expire:
When you regenerate your API key, your old key will stop working.
tillprochaska
commented
May 25, 2024
tillprochaska
commented
Jun 5, 2024
tillprochaska
commented
Jun 5, 2024
tillprochaska
commented
Jun 5, 2024
tillprochaska
commented
Jun 5, 2024
How API keys are reset and displayed has changed since the initial version of API keys: Users will be able to view an API key exactly once after it has been created/reset. This requires a slightly different user interface. We’re also planning a few more changes to API keys in the future, and these UI changes prepare for that.
The existing settings UI was a little cluttered and unstructured. We’re going to add new settings in this PR and in follow-up PRs, so I took the time to clean up the UI (both visually and implementation-wise).
This is a hacky workaround, but a proper fix would require quite some refactoring. Considering that this hack is pretty isolated and not going to affect any other parts of the UI and that we will need to upgrade to Blueprint 5 at some point anyway, I’ve opted for the quick-and-dirty solution for now.
In the future, roles won’t have an API key by default anymore. As an alternative, we generate session tokens explicitly.
Most users do not need API access so there’s no reason to generate an API key for them by default.
Previously, an API was generate automatically for new users, i.e. every user had an API key. This has now changed, and the settings UI needs to properly handle situations where a user doesn’t yet have an API key. As this increases the complexity of the UI state, I’ve refactored the component to make use of a local reducer.
This method is now also used to generate an initial key for users who do not yet have an API key.
While the logic initially was quite simply, there will be more business logic related to API keys, e.g. sending notifications ahead of and when an API key has expired.
Initially, I added this to the role model as the model to be consistent with the model's `set_password` method. However, as the logic to generate an API token has become more complex, it is clear that it shouldn't live in the model.
Aleph represents both users and groups using the role model. However, some API keys (such as `has_password` or `has_api_key` are not relevant for groups).
We merged a different migration in the meantime (8adf50) and as a result Alembic wasn’t able to figure out how to upgrade the database unambiguously.
452e46 changes the way we handle authentication in tests. Previously, we used API keys to authenticate users. Now, as we don’t generate API keys for users by default anymore, we use session tokens (the same mechanism the UI uses as well). In general, authentication using session tokens and API keys is quite similar: In both cases, the token is passed as a request header. However, there’s one significant difference between session tokens and API keys: When using API keys, data about the user (including group memberships) is loaded from the database on every request. When using session tokens, this data is cached in Redis for the lifetime of the session. For end users, this means they have to sign out and in again after their group memberships have been updated. When I originally implemented this change in our tests, it didn’t affect any of the tests as none of them did rely on updating group memberships. However, I’ve since implemented and merged additional tests in #3865, including one test that covers updating the user’s group memberships after the initial sign in. When I rebased this branch to include these new tests, this particular test failed. The solution for this is the same as for end users: In the test case, we need to reauthenticate the test user after updating the group memberships.
tillprochaska
force-pushed
the
feature/1027-reset-api-key
branch
from
7bd4b1e
to
50b689e
Compare
We merged other migrations in the meantime and as a result Alembic wasn’t able to figure out how to upgrade the database unambiguously.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes a number of changes related to API keys:
aleph reset-api-key-expirationto set the expiration date for these keys.Some additional information about implementation decisions:
I’ve refactored the settings screen, both with regards to the visual appearance as well as the implementation. The settings screen was a little cluttered before, and this PR as well as a few follow-up PRs to come will add even more settings to the screen. This refactoring prepares for that.
It’s quite common to allow users to generate multiple API keys for different purposes, and to be able to revoke/expire them individually. While this has advantages, it also adds complexity to the implementation. Assuming that most users do indeed use the API key as a personal API key to e.g. programmatically search Aleph for a data analysis etc. rather than integrating the API key into ten different external services, I’ve decided to avoid this complexity, at least for now.
Screen.Recording.2024-04-16.at.19.38.19.mov
To do: