-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Incorrect scope map fix #12144
Comments
Thanks @DamianFekete. Please see the blog post on cve-2022-31690 and related advisory for details about the fix and why it was applied.
See RFC 6749, Section 5.1 for details on why we assume that all scopes were granted. The unfortunate reality is that we cannot rely on that assumption in all cases, hence the fix. I'll look into a fix for
You can apply this workaround if it is acceptable to query the User Info endpoint on every login. If that's not acceptable in your case, let me know and I'll see about a different workaround. |
Thanks @sjohnr for your fast and helpful response. I was working with @DamianFekete on this issue. The workaround works fine for our application. |
FYI, seeing same issue with 5.7.5 version too. Workaround works for me |
I'm seeing the same behaviour on 5.7.8 while integrating a login with Azure B2C IDM. So, either way, I just wanted to put my fix here after hours of debugging in case anyone has a similar issue: @Configuration
@EnableWebSecurity
public class SecurityConfiguration {
// see https://github.com/spring-projects/spring-security/issues/12144
@Bean
public OAuth2UserService<OidcUserRequest, OidcUser> oidcUserService() {
OidcUserService oidcUserService = new OidcUserService();
oidcUserService.setAccessibleScopes(Collections.emptySet());
return oidcUserService;
}
} However, spring now validates the UserInfo and will complain if you do not have a "sub" property, so make sure your UserInfo endpoint includes a sub property (mine didn't) I apologize if this seems off-topic, but I thought it would be valuable to share my solution here for others experiencing the same problem, even if it's a niche topic. |
Describe the bug
Scope mapping handling changed with #12112.
2915a70#diff-73bd44f873d78e3d71e6a0fa18644a304d562a4a9fd2e303e913f6ed20a0ad16R78-R83
OidcAuthorizationCodeAuthenticationProvider.authenticate()
callsOidcAuthorizationCodeAuthenticationProvider.getResponse()
)DefaultAuthorizationCodeTokenResponseClient.getTokenResponse
does NOT add the scopes anymore. If no scopes are returned by default by the IdP, the scopes list is empty.If AccessTokenResponse.scope is empty, then we assume all requested scopes were granted.
seems to say something completely different.OidcAuthorizationCodeAuthenticationProvider.authenticate()
the user info has to be loaded:this.userService.loadUser()
.OidcUserService.loadUser
callsthis.shouldRetrieveUserInfo(userRequest)
which returnsfalse
now, because the scopes (userRequest.getAccessToken().getScopes()
) is empty.null
) and can't be used for example in theuserAuthoritiesMapper
.To Reproduce
Our scopes are configured like this:
Use the
userAuthoritiesMapper
with a token-uri endpoint that doesn't return a list of scopes.In the authorities mapper try to use the
oidcUserAuthority.getUserInfo()
(which is now null).Expected behavior
oidcUserAuthority.getUserInfo()
should not benull
.Sample
No sample yet.
Ping @sjohnr
The text was updated successfully, but these errors were encountered: