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

Issue #5064 - the OpenIdCredentials should be serializable #5066

Merged
merged 1 commit into from Jul 23, 2020

Conversation

lachlan-roberts
Copy link
Contributor

Issue #5064

The OpenIdConfiguration now contains an HttpClient so it cannot be stored as a field on the OpenIdCredentials which needs to be serializable.

The OpenIdConfiguration is now passed in on OpenIdCredentials.redeemAuthCode().

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@janbartel
Copy link
Contributor

IMHO it seems a bit odd that something called xxxConfiguration has a HttpClient and some behaviour. Another (non-mandatory) suggestion is to make the class that has the HttpClient a HttpSessionActivationListener, so when you get the sessionWillPassivate() event, you can null out the HttpClient field, and then recreate it as necessary in a sessionDidActivate() event.

@lachlan-roberts
Copy link
Contributor Author

@janbartel I remember having this conversation with Greg previously about the OpenIdConfiguration being more of a "Resources" rather than a "Configuration". But it is probably too late to change the name of it.

I don't think we want to create a HttpClient on every sessionDidActivate() event, it should be shared for all openid authentication requests on the server. So I think its best not to have it as a field on the OpenIdCredentials.

@joakime joakime linked an issue Jul 22, 2020 that may be closed by this pull request
@joakime joakime added this to In progress in Jetty 9.4.31 via automation Jul 22, 2020
@joakime joakime moved this from In progress to Review in progress in Jetty 9.4.31 Jul 22, 2020
@joakime joakime moved this from Review in progress to Reviewer approved in Jetty 9.4.31 Jul 22, 2020
@lachlan-roberts lachlan-roberts merged commit 81f6ec6 into jetty-9.4.x Jul 23, 2020
Jetty 9.4.31 automation moved this from Reviewer approved to Done Jul 23, 2020
@lachlan-roberts lachlan-roberts deleted the jetty-9.4.x-5064-openIdSerializable branch July 23, 2020 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Jetty 9.4.31
  
Done
Development

Successfully merging this pull request may close these issues.

NotSerializableException for OpenIdConfiguration
3 participants