-
Notifications
You must be signed in to change notification settings - Fork 141
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
Add support for audience parameter #600
base: send-audience
Are you sure you want to change the base?
Conversation
|
||
private static boolean isDefaultAudience(OktaOAuth2Properties properties) { | ||
String audience = properties.getAudience(); | ||
return audience == null || audience.isEmpty() || audience.equals("api://default"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that api://default
will never be sent if it's the audience. I tried to make it so it only happens when it's Auth0, but was unable to figure out how to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it should not be a problem sending it for Okta too.
// as of Spring Security 5.4 the default chain uses oauth2Login OR a JWT resource server (NOT both) | ||
// this does the same as both defaults merged together (and provides the previous behavior) | ||
http.authorizeRequests((requests) -> requests.anyRequest().authenticated()); | ||
Okta.configureOAuth2WithPkce(http, clientRegistrationRepository); | ||
Okta.configureOAuth2WithAudience(http, clientRegistrationRepository, oktaOAuth2Properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The audience overrides the PKCE parameters added on the previous line. Is there a way to chain these together so both parameters are added?
@@ -69,13 +64,13 @@ OidcReactiveOAuth2UserService oidcUserService(Collection<AuthoritiesProvider> au | |||
} | |||
|
|||
@Bean | |||
@ConditionalOnBean(ReactiveJwtDecoder.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this was necessary to invoke this configuration when no SecurityConfiguration
exists.
// as of Spring Security 5.4 the default chain uses oauth2Login OR a JWT resource server (NOT both) | ||
// this does the same as both defaults merged together (and provides the previous behavior) | ||
http.authorizeExchange().anyExchange().authenticated(); | ||
Okta.configureOAuth2WithPkce(http, clientRegistrationRepository); | ||
Okta.configureOAuth2WithAudience(http, clientRegistrationRepository, properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line clobbers the previous lines added parameters. We need to figure out a way to chain them together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arvindkrishnakumar-okta It seems like we could create the DefaultServerOAuth2AuthorizationRequestResolver
before passing it to Okta
's static methods, but that would change the method signature for configureOAuth2WithPkce()
. I believe that would be a breaking change and require a major version bump. Do you have any other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mraible I think that is the only way out here to resolve the clobbering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to overload the method with new signature and deprecate the current one?
private static ServerOAuth2AuthorizationRequestResolver authorizeRequestResolver( | ||
ReactiveClientRegistrationRepository clientRegistrationRepository) { | ||
private static ServerOAuth2AuthorizationRequestResolver reactiveAuthzRequestResolver( | ||
ReactiveClientRegistrationRepository clientRegistrationRepository, String audience) { | ||
|
||
DefaultServerOAuth2AuthorizationRequestResolver authorizationRequestResolver = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you get the authorizationRequestResolver
if it already exists? If I could figure that out, I could probably solve the chaining problem where the audience parameters overwrite the PKCE parameters.
No description provided.