Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JWT Api Key #2371

Merged
merged 18 commits into from
Mar 1, 2024
Merged

JWT Api Key #2371

merged 18 commits into from
Mar 1, 2024

Conversation

CDimonaco
Copy link
Member

Description

This PR includes a JWT implementation of the API KEY.
This new implementation allows the rotation of api keys as a result of configurable expiration of api keys.
Expiration could be infinite, where "infinite" is a very reasonable amount of time, because jwt standard wants an expiration timestamp.

This implies the store of JTI, used for token whitelisting and invalidation and creation and expiration date of api tokens, used for key generation, THE JWT KEY IS NOT STORED IN THE DATABASE.

Api key settings are stored into settings table, refactored with current settings as InstallationSettings, following a single table inheritance approach in the settings table.

Installation controller supports operation on the new type of settings, and returns the generated key when needed.

A new Plug for validating api key with jwt, validating issuer and JTI is enabled.

When Trento is installed a default api key with infinite expiration is created

How was this tested?

Automated tests

@CDimonaco CDimonaco added enhancement New feature or request elixir Pull requests that update Elixir code labels Feb 28, 2024
@CDimonaco CDimonaco requested a review from arbulu89 February 28, 2024 16:41
@CDimonaco CDimonaco self-assigned this Feb 28, 2024
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super great work @CDimonaco
I dropped a lot of comments, but most of them are nitpic.
Let me know when the PR is finished so I can do a new review, but most probably everything is alright

  • I'm not sure if we should call the main JWT as APP. In reality it should be API, but I understand that it makes things difficult to differentiate between api and api-key
  • Remove the tab in the docstrings

config/config.exs Show resolved Hide resolved
lib/trento/settings/api_key_settings.ex Show resolved Hide resolved
lib/trento/support/ecto/sti.ex Outdated Show resolved Hide resolved
lib/trento/support/ecto/sti.ex Show resolved Hide resolved
lib/trento/support/ecto/sti.ex Outdated Show resolved Hide resolved
test/trento/settings_test.exs Outdated Show resolved Hide resolved
@CDimonaco CDimonaco marked this pull request as ready for review February 29, 2024 15:37
@CDimonaco CDimonaco force-pushed the jwt_api_key branch 2 times, most recently from 1174e44 to 8fa99d5 Compare February 29, 2024 15:47
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super super great @CDimonaco
Ready to merge from my side, but i put some nitpick comments as always.

lib/trento/settings/api_key_settings.ex Outdated Show resolved Hide resolved
lib/trento_web/auth/api_key.ex Outdated Show resolved Hide resolved
lib/trento_web/auth/api_key.ex Outdated Show resolved Hide resolved
alias TrentoWeb.OpenApi.V1.Schema

use OpenApiSpex.ControllerSpecs
plug OpenApiSpex.Plug.CastAndValidate, json_render_error_v2: true
action_fallback TrentoWeb.FallbackController
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, but deprecated means that it will be removed in some future version, right?

@CDimonaco CDimonaco force-pushed the jwt_api_key branch 2 times, most recently from e112969 to ad43a43 Compare March 1, 2024 09:15
@CDimonaco CDimonaco merged commit 530ced5 into main Mar 1, 2024
24 checks passed
@CDimonaco CDimonaco deleted the jwt_api_key branch March 1, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants