Provide more flexibility on when to display consent page #1552

MrJovanovic13 · 2024-03-02T15:55:44Z

This PR fixes Issue-1541

Some things worth mentioning.

The Predicate naming is too similar to the private method name, do you think we should find a different name?
The private method is not static anymore since I am checking the predicate inside it and therefore i had to remove the static modifier and I've moved it above the other static methods in the file.
I've added the logic in such a way that if predicate exists it will immediately exit according to its evaluation, without going through further checks(such as if authorization consent is enabled in client settings). I figured if someone sets the predicate they want it to have full control instead of still going through additional checks which are currently in place(scopes, previous consent etc.)

jgrandja · 2024-03-08T20:35:08Z

Thanks @MrJovanovic13. I will review this soon.

I'm trying to finish up gh-101 before we release 1.3.0-M3.

jgrandja

Thanks for the PR @MrJovanovic13.

Please see review comments.

jgrandja · 2024-03-22T13:36:09Z

...erver/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProvider.java

+	 * {@link OAuth2AuthorizationCodeRequestAuthenticationContext#getRegisteredClient()} containing {@link RegisteredClient} used to make the request.
+	 *
+	 * @param requiresAuthorizationConsent the {@link Predicate} that determines if authorization consent is required.
+	 * @since 1.2.3


Update to 1.3

jgrandja · 2024-03-22T13:43:17Z

...erver/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProvider.java

@@ -295,26 +339,6 @@ private static OAuth2TokenContext createAuthorizationCodeTokenContext(
 		return tokenContextBuilder.build();
 	}

-	private static boolean requireAuthorizationConsent(RegisteredClient registeredClient,


We should keep this method and associated logic as-is but change the signature to:

private boolean requireAuthorizationConsent(OAuth2AuthorizationCodeRequestAuthenticationContext context)

We would also need to supply OAuth2AuthorizationCodeRequestAuthenticationContext with OAuth2AuthorizationRequest and OAuth2AuthorizationConsent.

This method would then be the default for:

private Predicate<OAuth2AuthorizationCodeRequestAuthenticationContext> requiresAuthorizationConsent;

Applications could then override this via setRequiresAuthorizationConsent().

Sounds good.

Would you like if OAuth2AuthorizationRequest and OAuth2AuthorizationConsent got treated the same way as RegisteredClient by exposing getRegisteredClient() and a builder method registeredClient(RegisteredClient registeredClient) inside the OAuth2AuthorizationCodeRequestAuthenticationContext?

Or do you want just to use regular .put() and .get() for their supply & access?

I have pushed a commit with a refactor according to your comment.

I attempted to expose getter & builders methods in the OAuth2AuthorizationCodeRequestAuthenticationContext for OAuth2AuthorizationRequest and OAuth2AuthorizationConsent instead of the .put() and .get() approach. However that ended up breaking a lot of tests. If It's important to go in that direction, I will put some more effort into making that work.

Would you like if OAuth2AuthorizationRequest and OAuth2AuthorizationConsent got treated the same way as RegisteredClient

Yes. We should make the getter @Nullable since it may be null when the authenticationValidator is called.

MrJovanovic13 · 2024-03-30T21:43:15Z

Sorry for the slight delay, had a busy week.

I can squash the commits if you guys prefer that.

jgrandja

Thanks for the updates @MrJovanovic13. Please see review comments.

Also, please squash commits. Thanks.

jgrandja · 2024-04-03T16:56:39Z

...server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationContext.java

@@ -63,6 +65,27 @@ public RegisteredClient getRegisteredClient() {
 		return get(RegisteredClient.class);
 	}

+	/**
+	 * Returns the {@link OAuth2AuthorizationRequest oauth2 authorization request}.
+	 *


Add @since 1.3

jgrandja · 2024-04-03T16:56:46Z

...server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationContext.java

+
+	/**
+	 * Returns the {@link OAuth2AuthorizationConsent oauth2 authorization consent}.
+	 *


Add @since 1.3

jgrandja · 2024-04-03T16:58:20Z

...server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationContext.java

+		 * @return the {@link Builder} for further configuration
+		 */
+		@Override
+		public Builder put(Object key, Object value) {


This can be removed since it's inherited by AbstractBuilder

This can be removed since it's inherited by AbstractBuilder

I overrode this method as opposed to using the one in AbstractBuilder because the one in the AbstractBuilder does not accept null values and authorizationConsent can be null.

The alternative is to remove the not null assertion inside the AbstractBuilder, but it's inherited 5 times so I didn't want to cause potential issues in other builders.

If authorizationConsent or authorizationRequest is null it should not be passed into the builder so it's not an issue.

If authorizationConsent or authorizationRequest is null it should not be passed into the builder so it's not an issue.

The authorizationConsent from authorizationConsentService.findById() is @Nullable.

So I added a null check which won't pass it into the builder if it's null. I got rid of the custom put() method.
Let me know if the solution is suitable.

jgrandja · 2024-04-03T17:01:09Z

...server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationContext.java

+		 * @param oAuth2AuthorizationRequest the {@link OAuth2AuthorizationRequest}
+		 * @return the {@link Builder} for further configuration
+		 */
+		public Builder oAuth2AuthorizationRequest(OAuth2AuthorizationRequest oAuth2AuthorizationRequest) {


Rename to authorizationRequest

jgrandja · 2024-04-03T17:01:22Z

...server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationContext.java

+		 * @param oAuth2AuthorizationConsent the {@link OAuth2AuthorizationConsent}
+		 * @return the {@link Builder} for further configuration
+		 */
+		public Builder oAuth2AuthorizationConsent(OAuth2AuthorizationConsent oAuth2AuthorizationConsent) {


Rename to authorizationConsent

jgrandja · 2024-04-15T19:22:49Z

Thanks for the updates @MrJovanovic13.

FYI, I added a polish commit to get this merged before tomorrow's release.

Thanks again!

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 2, 2024

jgrandja self-assigned this Mar 8, 2024

jgrandja added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 8, 2024

jgrandja changed the title ~~Issue-1541: Provide more flexibility on when to display consent page~~ Provide more flexibility on when to display consent page Mar 8, 2024

jgrandja requested changes Mar 22, 2024

View reviewed changes

MrJovanovic13 force-pushed the issue-1541 branch 2 times, most recently from 4d4d80e to 8be811f Compare March 25, 2024 02:07

MrJovanovic13 requested a review from jgrandja March 30, 2024 21:41

jgrandja requested changes Apr 3, 2024

View reviewed changes

MrJovanovic13 force-pushed the issue-1541 branch from 87e1ba1 to 577d392 Compare April 3, 2024 22:44

MrJovanovic13 requested a review from jgrandja April 3, 2024 22:45

Provide more flexibility on when to display consent page

1433393

MrJovanovic13 force-pushed the issue-1541 branch from 577d392 to 1433393 Compare April 4, 2024 16:24

jgrandja added this to the 1.3.0-RC1 milestone Apr 15, 2024

jgrandja added a commit that referenced this pull request Apr 15, 2024

Polish gh-1552

c0446c6

jgrandja closed this Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide more flexibility on when to display consent page #1552

Provide more flexibility on when to display consent page #1552

MrJovanovic13 commented Mar 2, 2024

jgrandja commented Mar 8, 2024

jgrandja left a comment

jgrandja Mar 22, 2024

jgrandja Mar 22, 2024

MrJovanovic13 Mar 24, 2024

MrJovanovic13 Mar 25, 2024

jgrandja Mar 25, 2024

MrJovanovic13 commented Mar 30, 2024

jgrandja left a comment

jgrandja Apr 3, 2024

jgrandja Apr 3, 2024

jgrandja Apr 3, 2024

MrJovanovic13 Apr 3, 2024

jgrandja Apr 4, 2024

MrJovanovic13 Apr 4, 2024

jgrandja Apr 3, 2024

jgrandja Apr 3, 2024

jgrandja commented Apr 15, 2024

Provide more flexibility on when to display consent page #1552

Provide more flexibility on when to display consent page #1552

Conversation

MrJovanovic13 commented Mar 2, 2024

jgrandja commented Mar 8, 2024

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrJovanovic13 commented Mar 30, 2024

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja commented Apr 15, 2024