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

Allow hooks or extensions to OAuth2__GrantType__AuthenticationProviders #1075

Open
Yneth opened this issue Feb 16, 2023 · 7 comments
Open

Allow hooks or extensions to OAuth2__GrantType__AuthenticationProviders #1075

Yneth opened this issue Feb 16, 2023 · 7 comments
Assignees
Labels
status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement

Comments

@Yneth
Copy link

Yneth commented Feb 16, 2023

I am integrating OAuth into existing platform, and we have user sessions. User session is a separate entity, that is living in other service, meaning it is not a HTTP session. I need to add user session creation to TokenEndpoint, such that session is created and its' id is included in token claims. The best place to add session creation is after OAuth2AuthorizationCodeAuthenticationProvider or OAuth2RefreshTokenAuthenticationProvider validation and before token generation.

Unfortunately, there are no points of customisation in that place.

In order to overcome this issue we have to create custom token generator that has extended responsibilities and generates session only on accessToken type and generates tokens.

It would be nice to have some kind of hooks such that users will be able to customize the behaviour and update OAuth2Context if there are any variables to share with other components of the flow.

@Yneth Yneth added the type: enhancement A general enhancement label Feb 16, 2023
@sjohnr
Copy link
Member

sjohnr commented Feb 16, 2023

@Yneth thanks for reaching out!

The best place to add session creation is after OAuth2AuthorizationCodeAuthenticationProvider or OAuth2RefreshTokenAuthenticationProvider validation and before token generation.

Unfortunately, there are no points of customisation in that place.

Both OAuth2TokenGenerator and OAuth2TokenCustomizer are available for this purpose. The reference guide has an example of constructing your own OAuth2TokenGenerator using built-in components.

A common pattern would be to use delegation to customize before/after the token generation process. Another would be to simply use the token customizer. The How-to: Customize the OpenID Connect 1.0 UserInfo response has an example of customizing just the access token (with an if-statement), which seems to be what you want.

In order to overcome this issue we have to create custom token generator that has extended responsibilities and generates session only on accessToken type and generates tokens.

Can you explain more about why you had to do this? Or does the above explanation help you get ideas for improvement?

It would be nice to have some kind of hooks such that users will be able to customize the behaviour and update OAuth2Context if there are any variables to share with other components of the flow.

Can you provide more details on your use case here? It's not clear to me yet what you weren't able to accomplish with the existing customization hooks.

@sjohnr sjohnr self-assigned this Feb 16, 2023
@sjohnr sjohnr added the status: waiting-for-feedback We need additional information before we can continue label Feb 16, 2023
@Yneth
Copy link
Author

Yneth commented Feb 19, 2023

Both OAuth2TokenGenerator and OAuth2TokenCustomizer are available for this purpose. The reference guide has an example of constructing your own OAuth2TokenGenerator using built-in components.

Our case is looking the following way:
during the request, first call of our custom OAuth2TokenGenerator implementation, we create a session and store it in the http request attributes, then sessionId is included in claims of all tokens during generation.

I do not like that we have to:

  • mutate request attributes in order to reuse sessionId;
  • create session in OAuthTokenGenerator, violating single responsibility rule;

The desired use case would look something like this:

// OAuth2AuthorizationCodeAuthenticationProvider.authenticate
// ...
OAuth2Authorization authorization = this.authorizationService.findByToken(
				authorizationCodeAuthentication.getCode(), AUTHORIZATION_CODE_TOKEN_TYPE);
// ...
// ADDITION #1: add custom authentication validation before token generation;
// it allows us to add custom validations that may include calls to other services
// after registeredClient and clientRegistrations are already validated.
// thus reducing unnecessary calls to 3rd party services, when not needed.
customAuthorizationValidator.validate(authorization);

// copy of OAuth2AuthorizationCodeAuthenticationProvider.authenticate
DefaultOAuth2TokenContext.Builder tokenContextBuilder = DefaultOAuth2TokenContext.builder()
				.registeredClient(registeredClient)
				.principal(authorization.getAttribute(Principal.class.getName()))
				.providerContext(ProviderContextHolder.getProviderContext())
				.authorization(authorization)
				.authorizedScopes(authorization.getAttribute(OAuth2Authorization.AUTHORIZED_SCOPE_ATTRIBUTE_NAME))
				.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
				.authorizationGrant(authorizationCodeAuthentication);

// ADDITION #2:  add customizer to token context builder. 
// Also, it would make sense to add the same for authorizationBuilder,
// allowing users to customize some fields of authorization.
// in our case it would be useful to add sessionId to the authorization parameters.
// Those are just ideas, I believe there could be a better abstraction to cover such cases.
tokenContextBuilder = tokenContextBuilderCustomizer.customize(tokenContextBuilder);
// ...

@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 Feb 19, 2023
@sjohnr
Copy link
Member

sjohnr commented Feb 24, 2023

Hi @Yneth! Thanks for the extra information.

I do not like that we have to:

  • mutate request attributes in order to reuse sessionId;

I not quite sure I understand this point, can you clarify what you mean here? Are you using the RequestContextHolder to pass the attributes around currently?

  • create session in OAuthTokenGenerator, violating single responsibility rule;

This isn't really the primary goal of adding customization hooks. The primary goal would be to solve for use cases which are not currently possible. A second goal would be to promote code reuse. There are many ways to achieve a desired outcome, but it becomes much harder for everyone if a good existing option is off the table for this reason alone. Additionally, you can always separate concerns into multiple implementations (classes) and use delegation when needed.

// ADDITION #1: add custom authentication validation before token generation;
// it allows us to add custom validations that may include calls to other services
// after registeredClient and clientRegistrations are already validated.
// thus reducing unnecessary calls to 3rd party services, when not needed.
customAuthorizationValidator.validate(authorization);

I agree that something like this could be quite valuable here! We already have Consumer<OAuth2AuthorizationCodeRequestAuthenticationContext> authenticationValidator in the OAuth2AuthorizationCodeRequestAuthenticationProvider for a similar purpose, so it would probably be very similar (use a Consumer and a context).

// ADDITION #2:  add customizer to token context builder. 
// Also, it would make sense to add the same for authorizationBuilder,
// allowing users to customize some fields of authorization.
// in our case it would be useful to add sessionId to the authorization parameters.
// Those are just ideas, I believe there could be a better abstraction to cover such cases.
tokenContextBuilder = tokenContextBuilderCustomizer.customize(tokenContextBuilder);

I think I see the use case here. You're trying to perform an operation (e.g. create a session) in one of the calls to the token generator, and then share information from that operation (e.g. session id) in each of the calls to the token customizer. Is that a good summary?

@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Mar 16, 2023
@sjohnr
Copy link
Member

sjohnr commented Mar 16, 2023

@Yneth please see my above question.

I not quite sure I understand this point, can you clarify what you mean here? Are you using the RequestContextHolder to pass the attributes around currently?

Also, can you confirm that I'm understanding your use case?

I think I see the use case here. You're trying to perform an operation (e.g. create a session) in one of the calls to the token generator, and then share information from that operation (e.g. session id) in each of the calls to the token customizer. Is that a good summary?

@Yneth
Copy link
Author

Yneth commented Mar 23, 2023

I think I see the use case here. You're trying to perform an operation (e.g. create a session) in one of the calls to the token generator, and then share information from that operation (e.g. session id) in each of the calls to the token customizer. Is that a good summary?

yes, thats correct.

we create session before access token and then need to share sessionId via HttpServletRequest.getAttribute during generation of openId and refresh tokens

I not quite sure I understand this point, can you clarify what you mean here? Are you using the RequestContextHolder to pass the attributes around currently?

we are using HttpServletRequest.getAttribute

@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 Mar 23, 2023
@sjohnr
Copy link
Member

sjohnr commented May 26, 2023

Thanks for confirming @Yneth! I think that adding some more flexible customization options and hooks into the processing flow is needed. I see a customization theme already with a few other issues, such as gh-1003, gh-925, gh-884 and others. While those are not strictly related, I think it's pretty clear that there are numerous spots where we need hooks into the flow.

We've already been thinking about this quite a bit, so I'm going to keep this issue open for the meantime as a reminder about the specific need to add hooks into the processing flow of the AuthenticationProvider itself. I may need to reach out to you (on this issue) when we get closer to a solution so you can confirm that what we come up with is addressing your use case. Sound good?

@sjohnr sjohnr added status: pending-design-work Needs design work before any code can be developed and removed status: feedback-provided Feedback has been provided labels May 26, 2023
@Yneth
Copy link
Author

Yneth commented May 27, 2023

@sjohnr sounds amazing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement
Projects
Status: Planning
Development

No branches or pull requests

3 participants