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

feat: introduce auth configuration #4321

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

wolf4ood
Copy link
Contributor

@wolf4ood wolf4ood commented Jul 2, 2024

What this PR changes/adds

Introduces a configuration extension for associating an auth implementation to a web context with configuration.

Introduces an ApiAuthenticationProvider which creates a AuthenticationService based on the input config.

The configuration plugs into the web.http.<context> mechanism adding a nested key for configuring the authentication for the specific <context>. For example for configuring the tokenbased auth for a custom context custom:

web.http.custom.path=/custom
web.http.custom.port=8081
web.http.custom.auth.type=tokenbased
web.http.custom.auth.key=apiKey

Why it does that

flexibility, cleanup, decoupling

Further notes

Some API configuration module explicitly register the AuthenticationRequestFilter and uses custom context name to be used in the ApiAuthenticationRegistry. For backward compatibility that has not been changed. For overriding the AuthenticationService for that specific api context the additional config value context is available

For example for the management context:

web.http.management.auth.context=management-api

As future improvement we should remove ApiAuthenticationProvider and apply directly the AuthenticationRequestFilter with the configured AuthenticationService. We also might want to incorporate
this extension as default behavior in the web extension.

Linked Issue(s)

Closes #4294

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@wolf4ood wolf4ood self-assigned this Jul 2, 2024
@wolf4ood wolf4ood added the enhancement New feature or request label Jul 2, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

Project coverage is 75.30%. Comparing base (7f20ba5) to head (aab71d0).
Report is 388 commits behind head on main.

Files Patch % Lines
...ation/ApiAuthenticationConfigurationExtension.java 91.30% 1 Missing and 1 partial ⚠️
...lipse/edc/api/ApiCoreDefaultServicesExtension.java 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4321      +/-   ##
==========================================
+ Coverage   71.74%   75.30%   +3.56%     
==========================================
  Files         919     1053     +134     
  Lines       18457    21159    +2702     
  Branches     1037     1182     +145     
==========================================
+ Hits        13242    15934    +2692     
+ Misses       4756     4707      -49     
- Partials      459      518      +59     

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

@wolf4ood wolf4ood force-pushed the feat/4294_auth_configuration branch 2 times, most recently from 0098748 to d1f2383 Compare July 3, 2024 08:39
@wolf4ood wolf4ood marked this pull request as ready for review July 3, 2024 09:20
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

Great stuff!
I think it would be beneficial to have a piece of .md in the documentation that explains how to secure the APIs, it would be also good to mark the explicit registration of the filter as deprecated so we could get rid of them at the right time.

@wolf4ood wolf4ood force-pushed the feat/4294_auth_configuration branch 3 times, most recently from b081031 to 66065ce Compare July 5, 2024 10:13
@wolf4ood
Copy link
Contributor Author

wolf4ood commented Jul 5, 2024

@ndr-brt

added some docs on the md file

@paullatzelsperger
I've added in the further notes the next step of refactoring that we could to to remove the AuthRegistry

@wolf4ood wolf4ood force-pushed the feat/4294_auth_configuration branch from 66065ce to aab71d0 Compare July 9, 2024 10:06
@wolf4ood wolf4ood merged commit c17a06b into eclipse-edc:main Jul 9, 2024
22 checks passed
@wolf4ood wolf4ood deleted the feat/4294_auth_configuration branch July 9, 2024 11:17
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.

Refactor auth extensions
4 participants