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

Api key implementation #466

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

Api key implementation #466

wants to merge 39 commits into from

Conversation

AliasXoX
Copy link
Contributor

@AliasXoX AliasXoX commented Jul 2, 2024

This pull request implements a new authentication method for lea5 using API Keys.
The authentication procedure using an API key has been documented.
Here's an overview of the changes coming with this pull request :

  • A new controller and view in order for admin to see, create and delete API keys
  • A new rails concern to authorize API key authentication (see api_key_authenticatable.rb) (to understand : https://keygen.sh/blog/how-to-implement-api-key-authentication-in-rails-without-devise/)
  • A new /api endpoint to login as an api key bearer (see routes.rb ; sessions_controller.rb ; sessions_helper.rb ; application_controller.rb)
  • A new rails secret to hash API keys (see api_key.rb)
  • An encoded file for credentials for the test environment
  • Modifying the way Cancancan abilities work in lea5 (see user_ability.rb ; user_ability.rb ; application_controller.rb)
  • Some tests that cover abilities, login with an api key and of course the api key controller's actions

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f5516c8) to head (d1c5a4c).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #466   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        31    +5     
  Lines          337       414   +77     
  Branches        35        44    +9     
=========================================
+ Hits           337       414   +77     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

D0gmaDev
D0gmaDev previously approved these changes Jul 4, 2024
@@ -0,0 +1,21 @@
# API Authentication
Copy link
Member

@Letiste Letiste Jul 11, 2024

Choose a reason for hiding this comment

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

For API authentication with API key, we usually do the authentication only with the Authorization header, and we would include it in all the requests. We don't have an authentication endpoint for APIs.
Why did you decide to use session? Maybe it was discussed somewhere else and I'm just not aware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late reply, I came to this conclusion because I didn't really know how to make the difference between a request coming from a regular user and an API key bearer because they both eventually end up making a GET request to access the website. I'll think of another way to recognize an API call to make it more conventional.

Copy link
Member

Choose a reason for hiding this comment

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

We should rework the way API is functionning. At first, there were a will of having the same link for API and normal pages. But, actually, it would be easier to have special path for api (/api/...).

@D0gmaDev D0gmaDev mentioned this pull request Aug 18, 2024
23 tasks
def initialize(api_key)
return if api_key.blank?

can :read, :all
Copy link
Member

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.


def index
@api_keys = ApiKey.accessible_by(current_ability)
authorize! :index, @api_keys
Copy link
Member

Choose a reason for hiding this comment

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

note: differs from how we manage index in user_controller


private

def still_authenticated?
log_out if should_log_out?
end

def api_auth
return unless request.path[0, 5] == '/api/'
Copy link
Member

Choose a reason for hiding this comment

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

Surely useless comment, but there is hard-coded text right here

def log_in_api(bearer)
reset_session # For security reasons, we clear the session data before login
session[:api_key_id] = bearer.id
session[:expires_at] = Time.current
Copy link
Member

Choose a reason for hiding this comment

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

Can't understand why it's done this way


def current_ability
@current_ability ||= if !session[:api_key_id].nil?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@@ -1,6 +1,7 @@
# frozen_string_literal: true

class SessionsController < ApplicationController
include ApiKeyAuthenticatable
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
include ApiKeyAuthenticatable

I'm not sure this is still useful

Comment on lines +3 to +5
# Path to login via api key
AUTH_API_PATH = '/auth/api'

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Path to login via api key
AUTH_API_PATH = '/auth/api'

I think it's an artifact of the previous version that uses full connection and not Authorization header

Comment on lines +19 to +21
resources :users, controller: :users, as: 'api_users'
resources :api_keys, controller: :api_keys, as: 'api_api_keys'
resources :machines, controller: :machines, as: 'api_machines'
Copy link
Member

Choose a reason for hiding this comment

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

Surely things to do about these endpoints but that's not the goal of this PR

Comment on lines +7 to +9
@api_key = api_keys(:FakeRadius)
@real_key = '5fcdb374f0a70e9ff0675a0ce4acbdf6d21225fe74483319c2766074732d6d80'

Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary ?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe test if a wrong api key is provided ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Reviewer approved
Development

Successfully merging this pull request may close these issues.

5 participants