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

Create possibility to perform an internal forward to the consent page #1232

Open
Chr3is opened this issue May 25, 2023 · 6 comments
Open

Create possibility to perform an internal forward to the consent page #1232

Chr3is opened this issue May 25, 2023 · 6 comments
Labels
status: on-hold We can't start working on this issue yet type: enhancement A general enhancement

Comments

@Chr3is
Copy link

Chr3is commented May 25, 2023

Expected Behavior
If the AuthenticationProvider decided, that a user has to give consent, the user is internally forwarded to the consent page without an additional redirect. The URI does not change in the browser.

Current Behavior
If consent is required, the user is redirected to the provided consent url. The browser performs an additional request.

Context
We would like to get rid of this additional redirect and perform an internal forward to the consent controller with the original authorization request.

Possible approach:
Make the redirectStrategy in the OAuth2AuthorizationEndpointFilter configurable to be able to perform a forward if the provided url is the consent url in a custom implementation.

@Chr3is Chr3is added the type: enhancement A general enhancement label May 25, 2023
@jgrandja
Copy link
Collaborator

@Chr3is

We would like to get rid of this additional redirect and perform an internal forward to the consent controller with the original authorization request.

Can you please provide details on why you require a forward? Why is the redirect insufficient for your use case?

@jgrandja jgrandja added status: waiting-for-feedback We need additional information before we can continue and removed type: enhancement A general enhancement labels May 29, 2023
@Chr3is
Copy link
Author

Chr3is commented May 30, 2023

@jgrandja
A user performs the OIDC auth code flow. After the login, the user's original auth request is replayed as authenticated user. The authentication provider decides, that the user has to give explicit scope/claim approval. If the approval is done within the same system, the approval page can be displayed directly in the browser (with an internal forward). In our case this flow is session based and we don't need the clientId, state & scopes as redirect parameters directly. We want to minimize the count of redirects as much as possible. In https://github.com/spring-attic/spring-security-oauth/blob/main/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/endpoint/AuthorizationEndpoint.java#L344 there was no need to perform an explicit redirect to the consent page.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 30, 2023
@jgrandja
Copy link
Collaborator

@Chr3is

We want to minimize the count of redirects as much as possible.

I'm trying to understand why you want to minimize the redirects? Is this causing an issue? Or is it simply a preference?

This is the first time we received this enhancement request so it is unlikely we would provide this enhancement at this point since it wouldn't be widely used.

As an FYI, we try to minimize exposing public API's until there is a greater need for it from our users. Exposing public APIs restricts our flexibility to change things freely and we need to support it over a longer term based on our support cycle. So we only expose APIs if and when there a greater demand from our users.

At this point, it is unlikely we would expose redirectStrategy. Furthermore, an Authorization Request and Authorization Consent Request are distinct request flows so maintaining the Authorization Request parameters (via a forward) at the Authorization Consent screen doesn't make sense. For example, the redirect_uri and code_challenge parameters are not relevant in the context of the Authorization Consent screen.

@jgrandja jgrandja added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels May 30, 2023
@Chr3is
Copy link
Author

Chr3is commented May 31, 2023

@jgrandja It's a preference and we would like to keep the old behavior, which was implemented with the deprecated spring security oauth2 AuthorizationEndpoint, which performed an internal forward. A forward is more performant than a redirect which requires the browser to perform an additional request. At the moment the consent endpoint retrieves the clientId, state and scope. With the state we can perform the lookup for the stored authorization which was created by the AuthRequestAuthprovider. However if custom handling like claims where implemented, they are missing in the consent redirect and this is in my opinion not coherent. It should be at least possible for the developer how the consent handling is done. And this includes how the consent (if required) is handled e.g. with a redirect (with additional parameters) or an internal forward.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 31, 2023
@jgrandja
Copy link
Collaborator

@Chr3is

It's a preference and we would like to keep the old behavior, which was implemented with the deprecated spring security oauth2 AuthorizationEndpoint, which performed an internal forward

I can appreciate that you would prefer to keep with the existing logic but as you are aware Spring Authorization Server is a complete re-write from the legacy Authorization Server so changes are inevitable when migrating to Spring Authorization Server.

With the state we can perform the lookup for the stored authorization which was created by the AuthRequestAuthprovider. However if custom handling like claims where implemented, they are missing in the consent redirect and this is in my opinion not coherent.

All the information related to the "in-flight" authorization is available to the consent controller after the redirect. Please see AuthorizationConsentController:

It demonstrates retrieval of OAuth2AuthorizationConsent for customizing the consent screen.

For your use case, you would inject OAuth2AuthorizationService and lookup the OAuth2Authorization via authorizationService.findByToken(state, new OAuth2TokenType(OAuth2ParameterNames.STATE)). Everything you need will be available in OAuth2Authorization to perform custom processing.

I'll leave this open for now and we'll see if other user's are looking for this capability.

@jgrandja jgrandja added status: on-hold We can't start working on this issue yet type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels May 31, 2023
@Chr3is
Copy link
Author

Chr3is commented May 31, 2023

@jgrandja Yes, there's no problem in implementing the consent controller. However, for me, it feels a little bit inconsistent that the scope is passed as redirect parameter but other additional params (claims for example) have to be taken from the in-flight authorization. Whats the purpose of scope redirect param at all? I like to optimize things where possible and at this point, a forward could do that. But thanks for the discussion! I appreciate that. Lets keep this open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: on-hold We can't start working on this issue yet type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants