Uniform authentication method for HttpSender by thomaspj10 · Pull Request #7941 · frankframework/frankframework

thomaspj10 · 2024-11-14T12:10:21Z

Closes #3045

… methods

core/src/main/java/org/frankframework/http/AbstractHttpSession.java

core/src/main/java/org/frankframework/http/authentication/ClientCredentialsQueryParameters.java

core/src/main/java/org/frankframework/http/authentication/ClientCredentialsBasicAuth.java

evandongen · 2024-11-20T12:17:24Z

core/src/main/java/org/frankframework/http/AbstractHttpSession.java

+		 */
+		RESOURCE_OWNER_PASSWORD_CREDENTIALS_QUERY_PARAMETERS;
+
+		public IAuthenticator newAuthenticator(AbstractHttpSession session) throws HttpAuthenticationException {


Ik zou dit eerder verwachten in AbstractOauthAuthenticator ipv in de enum

Snap ik. Er zijn veel soorten manieren om te authenticaten, dus AbstractOauthAuthenticator zal niet altijd als superclass gebruikt worden zoals bijvoorbeeld bij saml. Vandaar dat ik het hier neer heb gezet.

Dan nog vind ik een enum hier niet de juiste plaats voor

Waar zou je het dan naar toe verplaatsen?

Een AuthenticatorFactory misschien?

Ik vind het zelf nog niet heel storend om het in de enum te doen maar vind het switch-statement niet mooi.

core/src/main/java/org/frankframework/http/AbstractHttpSession.java

core/src/main/java/org/frankframework/http/authentication/AbstractOauthAuthenticator.java

core/src/main/java/org/frankframework/http/authentication/OauthResponseDto.java

...n/java/org/frankframework/http/authentication/ResourceOwnerPasswordCredentialsBasicAuth.java

.../org/frankframework/http/authentication/ResourceOwnerPasswordCredentialsQueryParameters.java

core/src/test/java/org/frankframework/http/authentication/HttpSenderAuthenticationTest.java

…psender

core/src/main/java/org/frankframework/http/AbstractHttpSession.java

nielsm5 · 2024-11-24T15:05:20Z

core/src/main/java/org/frankframework/http/authentication/AbstractOauthAuthenticator.java

+			UrlEncodedFormEntity body = new UrlEncodedFormEntity(formParameters, DEFAULT_INPUT_STREAM_ENCODING);
+
+			HttpPost request = new HttpPost(uri);
+			request.addHeader(body.getContentType());


Deze call zou niet moeten hoeven dacht ik, de contenttype van de entity is de default? In de HttpSender doen we het iig niet 😃

De Content-Type header wordt niet toegevoegd aan de request als ik die regel weghaal.

Dat zou betekenen dat het in de rest can de (huidige) code fout zou zijn. Overal wordt het content type aan de entity gehangen, en de HttpEntityEnclosingRequestBase (HttpPost) zou deze klakkeloos over moeten nemen 🍓

core/src/main/java/org/frankframework/http/authentication/AbstractOauthAuthenticator.java

core/src/test/java/org/frankframework/http/authentication/HttpSenderAuthenticationTest.java

….java Co-authored-by: Niels Meijer <nielsmeijer@hotmail.com>

…ttps://github.com/frankframework/frankframework into 3045/uniform-authentication-method-for-httpsender

tnleeuw

De meeste opmerkingen die ik heb zijn geloof ik relatief kleinigheden.

tnleeuw · 2024-12-04T10:24:19Z

core/src/main/java/org/frankframework/http/AbstractHttpSession.java

+		 */
+		RESOURCE_OWNER_PASSWORD_CREDENTIALS_QUERY_PARAMETERS;
+
+		public IAuthenticator newAuthenticator(AbstractHttpSession session) throws HttpAuthenticationException {


Een AuthenticatorFactory misschien?

Ik vind het zelf nog niet heel storend om het in de enum te doen maar vind het switch-statement niet mooi.

core/src/main/java/org/frankframework/http/AbstractHttpSession.java

core/src/main/java/org/frankframework/http/authentication/AbstractOauthAuthenticator.java

core/src/main/java/org/frankframework/http/authentication/ClientCredentialsBasicAuth.java

...n/java/org/frankframework/http/authentication/ResourceOwnerPasswordCredentialsBasicAuth.java

core/src/test/java/org/frankframework/http/authentication/HttpSenderAuthenticationTest.java

…psender

core/src/test/java/org/frankframework/http/authentication/OAuthAccessTokenKeycloakTest.java

…hAccessTokenKeycloakTest.java

tnleeuw · 2024-12-10T12:42:05Z

core/src/test/java/org/frankframework/http/authentication/OAuthAccessTokenKeycloakTest.java

-		}
-	}
+	void testGetAccessToken(AbstractHttpSession.OauthAuthenticationMethod oauthAuthenticationMethod, boolean shouldResolveSuccessfully) throws Exception {
+		assumeFalse(TestAssertions.isTestRunningOnGitHub());


Dit statement komt te laat -- het gaat al lang fout in de static test setup, voordat individuele test-methods worden aangeroepen.

…psender

…reated

tnleeuw · 2024-12-10T16:27:37Z

core/src/main/java/org/frankframework/task/TimeoutGuard.java

+			log.warn("Thread [{}] executing task [{}] exceeds timeout of [{}] s, interrupting. Created from source point:",
+				timeoutThread.getName(), description, timeout, source);


@nielsm5 Hier heb ik iets gesleuteld om te kunnen debuggen waarom een test fout ging -- er werd een TimeoutGuard getriggered waarvan we niet wisten waar die vandaan kwam.

Met deze stacktrace konden we dat wel zien.

Willen we deze extra log er in laten zitten bij iedere warning van een TimeoutGuard? Of willen we dat er conditioneel in hebben / alleen bij loglevel DEBUG?

Het zat er al in (alleen bij debug) toch? als het goed draaien we onze tests ook op dit level. je kan in de if(log.isdebugenabled) een log.error gooien, dan is hij iets prominenter.

Ik had een stacktrace toegevoegd aan de warn log.

Ik heb die nu verplaatst naar debug level en de oorspronkelijke warn log hersteld.

tnleeuw · 2024-12-10T16:29:16Z

core/src/main/java/org/frankframework/http/authentication/AbstractOauthAuthenticator.java

+			String responseBody = EntityUtils.toString(response.getEntity());
+
+			if (response.getStatusLine().getStatusCode() != 200) {
+				tg.cancel();


Dit was de missende tg.cancel().
Ik vond het niet de moeite waard om hem te checken en een timeout-exception te gooien, en daarmee dan de status-code line in de exception kwijt te raken.

De tg.cancel() miste omdat ik bij Thomas erop aangedrongen had, het niet in de finally te doen. 🫣

core/src/main/java/org/frankframework/http/AbstractHttpSession.java

….java Co-authored-by: Niels Meijer <nielsmeijer@hotmail.com>

sonarqubecloud · 2024-12-11T08:22:46Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Create initial implementation for all current existing authentication…

8e0db33

… methods

thomaspj10 changed the title ~~Create initial implementation for all current existing authentication methods~~ Uniform authentication method for HttpSender Nov 14, 2024

thomaspj10 added 5 commits November 14, 2024 14:34

Fix tests and remove dead code

037a0ca

Fix more tests and add validation during creation of authenticator

e05bd91

Remove accidental import

f123912

Make an authenticator optional

a7aae28

Fix singular test

163dd83

nielsm5 reviewed Nov 14, 2024

View reviewed changes

Add documentation and implement some code feedback

0104b89

thomaspj10 requested review from evandongen and nielsm5 November 20, 2024 11:26

evandongen reviewed Nov 20, 2024

View reviewed changes

thomaspj10 added 3 commits November 20, 2024 15:20

Implement some reviewer feedback

bffae0d

Implement some more reviewer feedback

6e37555

Fix tests

e8b600a

evandongen approved these changes Nov 21, 2024

View reviewed changes

Merge branch 'master' into 3045/uniform-authentication-method-for-htt…

7b90b1b

…psender

nielsm5 reviewed Nov 24, 2024

View reviewed changes

thomaspj10 and others added 4 commits November 26, 2024 10:31

Update core/src/main/java/org/frankframework/http/AbstractHttpSession…

fe0558a

….java Co-authored-by: Niels Meijer <nielsmeijer@hotmail.com>

Implement some reviewer feedback

c227b38

Merge branch '3045/uniform-authentication-method-for-httpsender' of h…

d7d6d7f

…ttps://github.com/frankframework/frankframework into 3045/uniform-authentication-method-for-httpsender

Add another test

459a70b

thomaspj10 requested a review from nielsm5 November 29, 2024 07:40

nielsm5 approved these changes Dec 3, 2024

View reviewed changes

Move creation of credentials factory behind guard clause

e43aab2

nielsm5 requested a review from tnleeuw December 4, 2024 10:10

tnleeuw requested changes Dec 4, 2024

View reviewed changes

thomaspj10 added 3 commits December 4, 2024 15:53

Implement more reviewer feedback

1fd4f28

Merge branch 'master' into 3045/uniform-authentication-method-for-htt…

2f897a6

…psender

Fix tests

8f301e4

nielsm5 reviewed Dec 9, 2024

View reviewed changes

core/src/test/java/org/frankframework/http/authentication/OAuthAccessTokenKeycloakTest.java Outdated Show resolved Hide resolved

nielsm5 and others added 12 commits December 9, 2024 14:41

Update core/src/test/java/org/frankframework/http/authentication/OAut…

76ce852

…hAccessTokenKeycloakTest.java

Dan toch maar een echte IDE gebruiken...

4c61918

Add log consumer

9fd60c7

Change to custom log consumer

4991b83

Add timeout to sender

00b86de

Change timeout and make container explicitly private

49e9864

Move setup logic to createSender

40cab58

Remove unused import

6093bad

Add wait for strategy

97fffc4

Fix mistake

188883b

Change wait strategy to strategy provided by library

098a203

Disable keycloak test on Github

7550b05

tnleeuw reviewed Dec 10, 2024

View reviewed changes

tnleeuw added 5 commits December 10, 2024 14:04

Merge branch 'master' into 3045/uniform-authentication-method-for-htt…

68556dd

…psender

Add some logging for test

16c16e3

Add some logging to try pinpoint where our breaking TimeoutGuard is c…

86c610e

…reated

Remove an "assume" statement that came at a point too late to matter

2e69d51

Fix a TimeoutGuard that wasn't cancelled.

c88af62

tnleeuw approved these changes Dec 10, 2024

View reviewed changes

tnleeuw requested a review from nielsm5 December 10, 2024 16:29

nielsm5 reviewed Dec 10, 2024

View reviewed changes

core/src/main/java/org/frankframework/http/AbstractHttpSession.java Outdated Show resolved Hide resolved

tnleeuw and others added 3 commits December 10, 2024 19:59

Only debug stacktraces from TimeoutGuard on DEBUG loglevel.

c5b2ec3

Apply review-comment and remove an unused constant.

41f83cf

Update core/src/main/java/org/frankframework/http/AbstractHttpSession…

a00e8ff

….java Co-authored-by: Niels Meijer <nielsmeijer@hotmail.com>

nielsm5 approved these changes Dec 11, 2024

View reviewed changes

nielsm5 merged commit 2897169 into master Dec 11, 2024

nielsm5 deleted the 3045/uniform-authentication-method-for-httpsender branch December 11, 2024 08:40

nielsm5 mentioned this pull request Dec 8, 2025

Use Nimbus to validate OAuth Token flow #10115

Merged

		log.warn("Thread [{}] executing task [{}] exceeds timeout of [{}] s, interrupting. Created from source point:",
		timeoutThread.getName(), description, timeout, source);

Conversation

thomaspj10 commented Nov 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnleeuw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Dec 11, 2024

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants