Refactor AbstractHttpSender to use CredentialFactory to authenticate requests. #8866
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the authentication methods for OAuth to leverage CredentialFactory instead of directly referencing local credential fields, ensuring that both user and client aliases are handled properly.
- Updates tests to use getBasicCredentials() instead of getCredentials()
- Refactors multiple authentication classes to retrieve credentials from CredentialFactory
- Adjusts configuration and validation in AbstractHttpSession and related classes
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| OAuthAccessTokenRequestTest.java | Updated tests to use getBasicCredentials() when creating requests |
| OAuthAccessTokenKeycloakTest.java | Modified sender setup to properly configure OAuth authentication |
| OAuthAccessTokenAuthenticatorTest.java | Adjusted tests to use new credential handling and ensure proper authenticator behavior |
| HttpSenderAuthenticationTest.java | Added tests for client and auth alias based basic authentication |
| ResourceOwnerPasswordCredentialsQueryParameters.java | Replaced direct session calls with new clientId/clientSecret fields |
| ResourceOwnerPasswordCredentialsBasicAuth.java | Updated authorization header creation to use new fields |
| ClientCredentialsQueryParameters.java | Refactored to use member variables instead of session variables |
| ClientCredentialsBasicAuth.java | Similar update to use new credential fields in authorization header creation |
| AbstractResourceOwnerPasswordCredentials.java | Introduced CredentialFactory to obtain both user and client credentials and updated configuration checks |
| AbstractClientCredentials.java | Refactored to retrieve credentials via CredentialFactory and added configuration warnings for authAlias usage |
| AbstractHttpSession.java | Updated credential handling to use CredentialFactory and modified methods to work with getBasicCredentials() |
Files not reviewed (2)
- core/src/test/resources/Management/securityItemsResponse.json: Language not supported
- core/src/test/resources/credentials.properties: Language not supported
nielsm5
requested changes
Apr 24, 2025
core/src/main/java/org/frankframework/http/authentication/ClientCredentialsQueryParameters.java
Outdated
Show resolved
Hide resolved
...in/java/org/frankframework/http/authentication/AbstractResourceOwnerPasswordCredentials.java
Outdated
Show resolved
Hide resolved
...in/java/org/frankframework/http/authentication/AbstractResourceOwnerPasswordCredentials.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/frankframework/http/authentication/ClientCredentialsBasicAuth.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/frankframework/http/authentication/ClientCredentialsBasicAuth.java
Outdated
Show resolved
Hide resolved
…andling to throw aggregated exception
…dentials configuration
nielsm5
approved these changes
May 6, 2025
evandongen
approved these changes
May 7, 2025
core/src/main/java/org/frankframework/http/AbstractHttpSession.java
Outdated
Show resolved
Hide resolved
nielsm5
approved these changes
May 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor the authentication methods for OAuth to use the CredentialFactory instead of using the local clientId, clientSecret, username and password variables. This ensures that an alias can be used for both cases, since setting an alias does not fill in the local fields it represents.
Fixes: #8838