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

Add authentication from ims-api #6 #58

Merged
merged 2 commits into from
Nov 18, 2024
Merged

Add authentication from ims-api #6 #58

merged 2 commits into from
Nov 18, 2024

Conversation

joelvdavies
Copy link
Collaborator

@joelvdavies joelvdavies commented Oct 28, 2024

Description

Adds authentication that functions in the same way as inventory-management-system-api does. The majority of this code is copied directly from there. Tested with ral-facilities/inventory-management-system#1028.

Changes I made here have also been made on ims-api: ral-facilities/inventory-management-system-api#408.

Also had a think about the change to using the /verify endpoint instead. The main issue I currently see is handling the e2e tests - we would not be able to run them in the current format without also having ldap-jwt-auth running/a mock endpoint as we would presumably want all requests to be denied if /verify is unreachable. I think this needs further investigation & discussion as it will also effect ral-facilities/inventory-management-system-api#399. So I did not attempt to implement this here yet.

Testing instructions

Add a set up instructions describing how the reviewer should test the code

  • Review code
  • Check Actions build
  • Review changes to test coverage

Agile board tracking

Closes #6

@joelvdavies joelvdavies added the enhancement New feature or request label Oct 28, 2024
@joelvdavies joelvdavies self-assigned this Oct 28, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2024

Codecov Report

Attention: Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.

Project coverage is 98.01%. Comparing base (292e4ec) to head (94ba5ef).

Files with missing lines Patch % Lines
object_storage_api/core/consts.py 75.00% 2 Missing ⚠️
object_storage_api/core/config.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #58      +/-   ##
===========================================
- Coverage    98.70%   98.01%   -0.70%     
===========================================
  Files           19       21       +2     
  Lines          310      352      +42     
===========================================
+ Hits           306      345      +39     
- Misses           4        7       +3     

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

@VKTB
Copy link
Collaborator

VKTB commented Nov 13, 2024

Also had a think about the change to using the /verify endpoint instead. The main issue I currently see is handling the e2e tests - we would not be able to run them in the current format without also having ldap-jwt-auth running/a mock endpoint as we would presumably want all requests to be denied if /verify is unreachable. I think this needs further investigation & discussion as it will also effect ral-facilities/inventory-management-system-api#399. So I did not attempt to implement this here yet.

Good point, thanks for raising this.

@joelvdavies joelvdavies merged commit 448cf8b into develop Nov 18, 2024
3 checks passed
@joelvdavies joelvdavies deleted the add-auth-#6 branch November 18, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add authentication from ldap-jwt-auth
3 participants