-
Notifications
You must be signed in to change notification settings - Fork 0
Api key implementation #466
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
base: master
Are you sure you want to change the base?
Changes from 34 commits
636367a
d390724
55ab055
3b2c707
9cfb6ee
cf14ca4
98e5f38
d46f1d1
a24f1f9
99385e2
62ef0c7
a814f92
896d616
0a84054
0e60fad
5be2472
be2b48a
3b22322
c94ec41
5823b20
7007ebc
543e5ad
23f2551
b912261
a6b181d
2f19aa3
bc914f9
fd171e1
2431af6
8e7d33f
c6258a9
0a5fde7
2917a3c
3587ade
862cab8
f403dda
5737cc0
a89d1cd
d1c5a4c
f79467a
351eb39
c14bb3f
7fc0a58
b8f1eb2
4a3ea29
5a732cb
ad04504
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# frozen_string_literal: true | ||
|
||
class ApiKeyAbility | ||
include CanCan::Ability | ||
def initialize(api_key) # rubocop:disable Lint/UnusedMethodArgument | ||
can :read, :all | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
# frozen_string_literal: true | ||
|
||
class Ability | ||
class UserAbility | ||
include CanCan::Ability | ||
|
||
def initialize(user) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
# frozen_string_literal: true | ||
|
||
class ApiKeysController < ApplicationController | ||
include ApiKeyAuthenticatable | ||
include SessionsHelper | ||
|
||
# Require token authentication for index | ||
|
||
def index | ||
@api_keys = ApiKey.accessible_by(current_ability) | ||
authorize! :index, @api_keys | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: differs from how we manage index in user_controller |
||
end | ||
|
||
def new | ||
@api_key = ApiKey.new | ||
authorize! :new, @api_key | ||
end | ||
|
||
def create | ||
@api_key = ApiKey.new(api_key_params) | ||
authorize! :create, @api_key | ||
respond_to do |format| | ||
if @api_key.save | ||
format.html do | ||
AliasXoX marked this conversation as resolved.
Show resolved
Hide resolved
|
||
flash[:success] = "ApiKey added! It is #{@api_key.key}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pass the token separately in a custom flash[:new_api_key] = @api_key.key <% if flash[:new_api_key] %>
<p>Api key created! It will only be displayed once, so copy it now! <code><%= flash[:new_api_key] %></code></p>
<% end %> (https://guides.rubyonrails.org/action_controller_overview.html#the-flash) |
||
redirect_to api_keys_url | ||
end | ||
else | ||
format.html { render 'new', status: :unprocessable_entity } | ||
end | ||
end | ||
end | ||
|
||
def destroy | ||
@api_key = ApiKey.find(params[:id]) | ||
authorize! :destroy, @api_key | ||
@api_key.destroy | ||
flash[:success] = 'ApiKey deleted!' | ||
redirect_to api_keys_url | ||
end | ||
|
||
private | ||
|
||
def api_key_params | ||
params.require(:api_key).permit(:bearer_name) | ||
end | ||
end |
AliasXoX marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
# frozen_string_literal: true | ||
|
||
module ApiKeyAuthenticatable | ||
extend ActiveSupport::Concern | ||
|
||
include ActionController::HttpAuthentication::Basic::ControllerMethods | ||
include ActionController::HttpAuthentication::Token::ControllerMethods | ||
|
||
attr_reader :current_api_key, :current_bearer | ||
|
||
private | ||
|
||
attr_writer :current_api_key, :current_bearer | ||
|
||
def authenticator(http_token) | ||
@current_api_key = ApiKey.authenticate_by_token! http_token | ||
|
||
current_api_key | ||
end | ||
end |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,11 +1,18 @@ | ||||
# frozen_string_literal: true | ||||
|
||||
class SessionsController < ApplicationController | ||||
include ApiKeyAuthenticatable | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'm not sure this is still useful |
||||
def create | ||||
user = User.upsert_from_auth_hash(request.env['omniauth.auth']) | ||||
log_in user | ||||
flash[:success] = 'You are now logged in!' | ||||
redirect_to user_path user | ||||
if request.path == '/auth/api' | ||||
current_bearer = authenticate_or_request_with_http_token { |token, _options| authenticator(token) } | ||||
log_in_api current_bearer | ||||
render json: flash[:success] = 'You are now logged in!' | ||||
else | ||||
user = User.upsert_from_auth_hash(request.env['omniauth.auth']) | ||||
log_in user | ||||
flash[:success] = 'You are now logged in!' | ||||
redirect_to user_path user | ||||
end | ||||
end | ||||
|
||||
def destroy | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# frozen_string_literal: true | ||
|
||
module ApiKeysHelper | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# frozen_string_literal: true | ||
|
||
class ApiKey < ApplicationRecord | ||
HMAC_SECRET_KEY = Rails.application.credentials.api_key_hmac_secret_key! | ||
|
||
validates :bearer_name, presence: true, allow_blank: false | ||
|
||
before_create :generate_token_hmac_digest | ||
|
||
attr_accessor :key | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be better to have the column name as Also stick to one name, between key/api_key, token (in |
||
|
||
def self.authenticate_by_token!(key) | ||
digest = OpenSSL::HMAC.hexdigest 'SHA256', HMAC_SECRET_KEY, key | ||
|
||
find_by! api_key: digest | ||
end | ||
|
||
private | ||
|
||
def generate_token_hmac_digest | ||
@key = SecureRandom.hex(32) | ||
AliasXoX marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
digest = OpenSSL::HMAC.hexdigest 'SHA256', HMAC_SECRET_KEY, @key | ||
self.api_key = digest | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<%# locals: (api_key:) -%> | ||
|
||
<div class="api_key"> | ||
<span><%= api_key.bearer_name %></span> | ||
<span><%=api_key.created_at %></span> | ||
<% if can?(:destroy, api_key) %> | ||
<%= button_to(api_key,method: :delete, data: { turbo_confirm: "Are you sure ?" }, 'aria-label': 'Delete this api key') do %> | ||
<%= svg_icon_tag 'icon_delete' %> | ||
<% end %> | ||
<% end %> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<%# locals: () -%> | ||
|
||
<%= form_with(model: @api_key, class: 'form') do |f| %> | ||
<div> | ||
<%= f.label :bearer_name %> | ||
<%= f.text_field :bearer_name, required: true %> | ||
</div> | ||
<div> | ||
<%= f.submit yield(:button_text) %> | ||
</div> | ||
<% end %> |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,30 @@ | ||||||
<%# locals: () -%> | ||||||
|
||||||
<main> | ||||||
<div class="container"> | ||||||
<div class="card-details-container card-api-keys card-details-api-keys"> | ||||||
<div class="card card-details"> | ||||||
|
||||||
<div class="card-title"> | ||||||
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 448 512"><!--! Font Awesome Pro 6.2.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license (Commercial License) Copyright 2022 Fonticons, Inc. --> | ||||||
<path xmlns="http://www.w3.org/2000/svg" d="M224 256c70.7 0 128-57.3 128-128S294.7 0 224 0S96 57.3 96 128s57.3 128 128 128zm-45.7 48C79.8 304 0 383.8 0 482.3C0 498.7 13.3 512 29.7 512H418.3c16.4 0 29.7-13.3 29.7-29.7C448 383.8 368.2 304 269.7 304H178.3z"/> | ||||||
</svg> | ||||||
Comment on lines
+9
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add the icon to the SVG icons file ( |
||||||
<h2>Api Keys</h2> | ||||||
</div> | ||||||
|
||||||
<%# Need to pass an instance for an ability with block https://github.com/CanCanCommunity/cancancan/blob/develop/docs/define_abilities_with_blocks.md %> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is not required here
Suggested change
|
||||||
<% if can?(:create, ApiKey) %> | ||||||
<%= render "components/buttons/button_primary_create_api", text: "new api key", path: new_api_key_path, aria_label: "Add a new api key" %> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you create a new button component, instead of reusing the |
||||||
<% end %> | ||||||
|
||||||
</div> | ||||||
|
||||||
<div class="divider"></div> | ||||||
</div> | ||||||
<div class="card card-api-key card-content"> | ||||||
<div class="card-content-machines"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<%= render(@api_keys) || "No bearers of an api key" %> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
</div> | ||||||
</div> | ||||||
</div> | ||||||
</main> |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we will in the end remove all JSON responses from the non-api controllers and migrate them to the API namespace (and the API key list already has an API endpoint, created in this Pull Request) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# frozen_string_literal: true | ||
|
||
# locals: () | ||
|
||
json.array! @api_keys |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<%# locals: () -%> | ||
|
||
<% provide :button_text, "Create" %> | ||
|
||
<h1>ApiKey#new</h1> | ||
|
||
<%= render 'form' %> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
<%# locals: (aria_label:, path:, text:) -%> | ||
|
||
<%= button_to(path, method: :get, class: "button-primary", 'aria-label': aria_label) do %> | ||
<%= text %> | ||
<%= svg_icon_tag 'icon_plus' %> | ||
<% end %> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,9 @@ | |
<li> | ||
<%= link_to "Users", users_path %> | ||
</li> | ||
<li> | ||
<%= link_to "Api Keys", api_keys_url %> | ||
</li> | ||
Comment on lines
+13
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be exposed in the "Admin" area? |
||
<% end %> | ||
</ul> | ||
</nav> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
VfC7hNP55EjMyi4vt8jK8lAaRmOkql+wsyAFxSVsQdVT8tbZk1l0dgyELsvoVfHwU9kqzI+wMIFyuH6MoXBhcghZfa6m5g683FM06BDkYPwwDflEVeox0DvWmgGji4qzk3oFe57T9qQT736mz3dWfeQTzvjHnaUpq27gYQTpQHOuELjYwKsMXbFRFiNNMmG5phiG2k0Asc7dqZ8CRPwmhJYPm5aJBc9Bzrz3ebBnhxmZ+JzZ8AsZnnvnAFnylm2jRwgN91kJE9l3fkWVTlF6rm0EoCJ9r3neibTeDu2PtNnkYISp3O7chrBJKo6FS9GFAcOxgkxB8QPiX2TV2s3jPiNyxffxLqUe+jziPP4dDiHdZvKrONIltblTJV2WTzur7/82fCEhQUf2oM7uwavq/yvQNOlay4nUD9TyBGZrs1fyFuulT4Ik2/w4T7usUC6am1xIzB2ITF8IuAgolkT/5EPsIs45gJEpJy9Fqlg1PAv01NN9hhViEl+A--tARvrsUmwyVJ9/XI--ytpULZnEcYeOSqfUYhHAnA== | ||
Ctjn6rzrT8lN8apnt2Pd02BtoVstxzsZTgu7DACFn3A6pXt5rbEb1Z79Tcx5R6t2lB9MbLauB/DwUyS9JdzjzKeQQnGvGDHo0j9/vR1I1N3m3+sEXOuPNihRzzCLgePkh4jYtqNZ8NK1MdjmeG6kfPvB4fYNgLz+LWHudMLDhW98Ee3U6yso/edEMlXolmXyp3C4LeyqW/4qVPqaonUU4LJpYeG+UZndxjifEU8vpIhY4dW10mVpdO5JSnteNbtRBc/ZojuVla69hLEgYyr1uiyoy23ArtztfpyXhZEc6cmuJoXi7gpJd9sIDOZf2/xwasRW1EkI3NgpREj4Gy1t7lSwWLf8vp4j3qpC3/ifnGUUNAoJfBjax18WVOfzf2z051Q1TRN0tMhTknihNcUjAer54eT+JYHv7PLbxACcHs4evI/mnuglBOvI9DggODsUIe/7BqM0oQ0j1lj0/m7FHnexdc8FFHrrZ5IXN3VvZ6E0dAEMXNY8gS66mNjNEf2jfXKRicG/eu0FHYRiR00QgtVKsrggUy5IQnLc5Nj3lAq74Bi53UFDQI/AnaIA6uGQtf2lG3XTyfaadwMA6hOFKSQtVd1/OVJAptFIyJkkMiBiJAcU--rNRzrZK7K/fPFPxc--fNlBrjqZoGt/U748IRtjng== |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
QF2+HIDkGNTPjx81hqlGjQAZyIzZVAp9c7J+dWPqlggPt9u/LWhxWWi6vY7KXGYe3opGafwsWlOgrO0BXtzbOg+RK+Bq9a8rUgsKchqHi44B487Y9/rTvS5bcCTJj5c78AJFdZCsEdOXqUv6khBGHB2i8fGQ5LZyh/SFJhmyt3QqGuL6Ig4MsuD/275o2M94CjrlXT2nNGe6BPl94GurtQv7s67bbgrSy3N2f0Kc+VR4VfrIhTwgkpXxEBv7vk/ok/1WvYpqRbFNuyPhiqkZ6Rj+0oK7JCxtFDL15tBtrIRbdpNuBeWO8BQwf/CfvcgBsJnOGg//LU/AJe+ndMCdUzdfVpuB1QKG4A62lmrLNOEwlw/K3JdPsWqPEh6rSVIr5nAzl4oaxMo0NZs7VK7ZhNpGxkvdNVQEZDxchZkaanQOIFU02240w3nMHYx5aed1MEj2ZpuR26Jg+W85m3K0BvsCDcXAouLTvMCB8kGZ8H6SOWExIdbIhmM3--x6H3t+K9w244qMLp--x+vXvS3cObYjuhqsAb4HYA== | ||
Zmnkjz2oYau2HVmSP0GPQ/fzdTezrNhjFyIiP7okk4UZoOATR8X1mbTwulJF0/1vmtmoCr8jTAPWLfRY+0SwZAidd7yh16vhWCy6mpd5/OMeupQencSQCl3T0wgkkMtIa/J64DbfRav8BPhPBHi4St+1x62FM+WRw0udfj+xA2AUPxtIy/FZXIm+WUHcj6wQKXvIY2xJ1YcpA2WAsmow+1d1Aps1UWzMm0pwysm4SsUCDVS3nloaNTQ25g5+0HZ49biY7nbU6E1IM6dsgwK2/yRrrPxgDpv22L/r3BNp1wv0u4XP5LG+/yfaRfcdaO34WegffGsGEbuBf+hJ/fmxEJ3kq3Uk7DVlbFft94kIfzhHPOU/fi6EX4pWwrS4hExJ43X6/nEMtUY2b6ZXQDLazHv6rI2FcVzsSP+CRQwFc3VU2sjiJbHoe9hh1Jz27d/8rc+BjAwiwwpzZyTsAUlFy92qLZIENJskOhiowZewErJIUmV31L1ImRw/NCCC/Qt+hE8VvTkYmlWFU++9v4wddd5u2GUHzmHh7h7QFUCgKYFeqZiZbi58ihfIn/j7mpdJVk6lc73C+zAMI52rN65bZ6pGySU1/IJMGkhPc181hVC8tdiryw==--ss5nXaT6eOm/wEgh--oHW8aumKNzcCbzU7XFQyXg== |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really don't like this 😅 it could be OK to merge the PR as-is, but we will need to quickly find a way to run tests with hardcoded test values that don't need a full encrypted file (same for the development environment, we could use a hardcoded/simpler way to configure the Keycloak config or this HMAC secret without needing a secret key). |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
151fe6f76326223cfb5f7cef5369c504 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
mrjWeVMW0ZzGnTS1Yw+YRq/mJUTEyb7vTGp781GRYBv7s4XMK4eRkNrcG3jlb6StbugQFUa3yKSSUac1E/LHrtRgDS8FO7lkN3m3gnD/apzZ6QFQ+ub/Gzhgg9htCB2OSTwKwHINeCuog/wlxeGd3VGon0nNltGlYKv8Fet2W8KyMaOBNSFTwf6zv0jPrke1LRGiqAlNuaA=--w7kZ8Fb41WHlHnQB--mk0Xq4zeXvm7j1VYpZ39jQ== |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,8 @@ | ||||||
# frozen_string_literal: true | ||||||
|
||||||
# Path to login via api key | ||||||
AUTH_API_PATH = '/auth/api' | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think it's an artifact of the previous version that uses full connection and not Authorization header |
||||||
# Path to login via SSO authentication | ||||||
AUTH_PATH = '/auth/keycloak' | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
# frozen_string_literal: true | ||
|
||
class ApiKeys < ActiveRecord::Migration[7.0] | ||
def change | ||
create_table :api_keys do |t| | ||
t.integer :bearer_id, null: false, index: { unique: true } | ||
t.string :bearer_name, null: false | ||
t.string :api_key, null: false, index: { unique: true } | ||
t.datetime :api_key_start_at, index: { unique: true } | ||
|
||
t.timestamps | ||
end | ||
end | ||
end |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the branch is not merged yet, could you combine the migrations in a single one? (to avoid having "migration + fix1 + fix2") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# frozen_string_literal: true | ||
|
||
class FixApiKey < ActiveRecord::Migration[7.0] | ||
def change | ||
change_table :api_keys do |t| | ||
t.remove :api_key_start_at, | ||
type: :datetime | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# frozen_string_literal: true | ||
|
||
class FixApiKeyBearerId < ActiveRecord::Migration[7.0] | ||
def change | ||
change_table :api_keys do |t| | ||
t.remove :bearer_id, | ||
type: :integer | ||
end | ||
end | ||
end |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need in a near future to refine the permissions of each key, maybe by listing all the endpoints when creating one and let the user select which he wants the bearer to access (read, create, update, destroy), and change the permissions in the edit view.