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

Token based authentication extension libraries #1

Merged
merged 26 commits into from
Dec 20, 2024
Merged

Token based authentication extension libraries #1

merged 26 commits into from
Dec 20, 2024

Conversation

atakavci
Copy link
Collaborator

@atakavci atakavci commented Nov 6, 2024

This PR contains ;

  • the Core(redis-authx-core) components for token based authentication as a module to be shared among jedis and lettuce
  • A common implementation of authentication(redis-authx-entraid) library to support MS EntraID to help support integration of jedis and lettuce with MS EntraID authentication
  • Reqired workflows and configuration regarding to maintain and test both core and entraid modules

Focused aspects are;

  • providing possible future extensibility with the core library
  • providing generic interfaces/components that allow other identity providers to be implemented.
  • keep allowing configurations that MS EntraID support
  • providing easy to use and stable interfaces for particular requirements of individual client library(jedis and lettuce) .

@atakavci atakavci marked this pull request as draft November 7, 2024 07:26
@atakavci atakavci force-pushed the tba-draft branch 4 times, most recently from 745d9ba to dfa9498 Compare November 18, 2024 11:27
- core snapshot
atakavci and others added 6 commits November 29, 2024 14:43
- add unit and integration tests
- add executor to shutdown in TokenManager (review from Ivo)
…advanced configurations with EntraIDTokenAuthConfigBuilder

- add more unit tests
atakavci and others added 5 commits December 5, 2024 08:23
- fix cert issue
- drop jedis integration tests (move to jedis)
- add unit tests
- change textcontext to load demand
- make config builders generic
- force refresh with managedidentity
- skipcache with confidentialclientapp
- add builder cloners
- fix units tests
- set DEFAULT_EXPIRATION_REFRESH_RATIO in entraid 0.75
@atakavci atakavci marked this pull request as ready for review December 9, 2024 10:45
Copy link

@ggivo ggivo left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I think it is good first version

Copy link

@tishun tishun left a comment

Choose a reason for hiding this comment

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

Aaah a challenging review!

Ali, thanks for all the work, this has been much harder than I personally anticipated.

I generally have few important comments, so none of this is blocking. The main things are:

  • we are missing licensing statements, I've added them as suggestions, if you agree you can batch submit them
  • we are missing a lot of documentation; I know we can add it later, but I always fear we will never come back to that; my personal minium is documentation for all public API, but in this case I would say please explain a few of the parts in the code that are a bit harder to understand (because of complexity)

- licesing statement
- checkout action version
- drop useless file
- attemp to increase readibility and establish a clear seperation of responsibilties via breaking tokenmanager into multiple classes and interfaces.
- added some comments to explain the logic
@atakavci atakavci merged commit 806cdad into main Dec 20, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants