fix/MNT-24542 class cast exception #3092
Conversation
code4uuuu
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| final DefaultRefreshTokenTokenResponseClient client = new DefaultRefreshTokenTokenResponseClient(); | ||
| client.setRestOperations(rest); | ||
| return client; | ||
| } | ||
|
|
||
| private static OAuth2AccessTokenResponseClient<OAuth2PasswordGrantRequest> createPasswordClient(RestOperations rest, | ||
| ClientRegistration clientRegistration) | ||
| { | ||
| final DefaultPasswordTokenResponseClient client = new DefaultPasswordTokenResponseClient(); | ||
| client.setRestOperations(rest); | ||
| Optional.of(clientRegistration) | ||
| .map(ClientRegistration::getProviderDetails) | ||
| .map(ProviderDetails::getConfigurationMetadata) | ||
| .map(metadata -> metadata.get(AUDIENCE.getValue())) | ||
| .filter(String.class::isInstance) | ||
| .map(String.class::cast) | ||
| .ifPresent(audienceValue -> { | ||
| final OAuth2PasswordGrantRequestEntityConverter requestEntityConverter = new OAuth2PasswordGrantRequestEntityConverter(); | ||
| requestEntityConverter.addParametersConverter(audienceParameterConverter(audienceValue)); | ||
| client.setRequestEntityConverter(requestEntityConverter); | ||
| }); | ||
| return client; | ||
| } | ||
|
|
||
| private static Converter<OAuth2PasswordGrantRequest, MultiValueMap<String, String>> audienceParameterConverter( | ||
| String audienceValue) | ||
| { | ||
| return (grantRequest) -> { | ||
| MultiValueMap<String, String> parameters = new LinkedMultiValueMap<>(); | ||
| parameters.set("audience", audienceValue); | ||
|
|
||
| return parameters; | ||
| }; | ||
| } | ||
|
|
||
| private static class SpringAccessTokenAuthorization implements AccessTokenAuthorization | ||
| { | ||
| private final OAuth2AccessTokenResponse tokenResponse; | ||
|
|
||
| private SpringAccessTokenAuthorization(OAuth2AccessTokenResponse tokenResponse) | ||
| { | ||
| this.tokenResponse = requireNonNull(tokenResponse); | ||
| } | ||
|
|
||
| @Override | ||
| public AccessToken getAccessToken() | ||
| { | ||
| return new SpringAccessToken(tokenResponse.getAccessToken()); | ||
| } | ||
|
|
||
| @Override | ||
| public String getRefreshTokenValue() | ||
| { | ||
| return Optional.of(tokenResponse) | ||
| .map(OAuth2AccessTokenResponse::getRefreshToken) | ||
| .map(AbstractOAuth2Token::getTokenValue) | ||
| .orElse(null); | ||
| } | ||
| } | ||
|
|
||
| private static class SpringAccessToken implements AccessToken | ||
| { | ||
| private final AbstractOAuth2Token token; | ||
|
|
||
| private SpringAccessToken(AbstractOAuth2Token token) | ||
| { | ||
| this.token = requireNonNull(token); | ||
| } | ||
|
|
||
| @Override | ||
| public String getTokenValue() | ||
| { | ||
| return token.getTokenValue(); | ||
| } | ||
|
|
||
| @Override | ||
| public Instant getExpiresAt() | ||
| { | ||
| return token.getExpiresAt(); | ||
| } | ||
| } | ||
|
|
||
| private static class SpringDecodedAccessToken extends SpringAccessToken implements DecodedAccessToken | ||
| { | ||
| private final Jwt jwt; | ||
|
|
||
| private SpringDecodedAccessToken(Jwt jwt) | ||
| { | ||
| super(jwt); | ||
| this.jwt = jwt; | ||
| } | ||
|
|
||
| @Override | ||
| public Object getClaim(String claim) | ||
| { | ||
| return jwt.getClaim(claim); | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / PMD
High amount of different objects as members denotes a high coupling Warning
|
|
||
| class SpringBasedIdentityServiceFacade implements IdentityServiceFacade | ||
| { | ||
| private static final Log LOGGER = LogFactory.getLog(SpringBasedIdentityServiceFacade.class); |
Check warning
Code scanning / PMD
Logger should be defined private static final and have the correct class Warning
| class SpringBasedIdentityServiceFacade implements IdentityServiceFacade | ||
| { | ||
| private static final Log LOGGER = LogFactory.getLog(SpringBasedIdentityServiceFacade.class); | ||
| private static final Instant SOME_INSIGNIFICANT_DATE_IN_THE_PAST = Instant.MIN.plusSeconds(12345); |
Check warning
Code scanning / PMD
Avoid excessively long variable names like SOME_INSIGNIFICANT_DATE_IN_THE_PAST Warning
| } | ||
| catch (OAuth2AuthorizationException e) | ||
| { | ||
| LOGGER.debug("Failed to authorize against Authorization Server. Reason: " + e.getError() + "."); |
Check warning
Code scanning / PMD
Logger calls should be surrounded by log level guards. Warning
| UserInfoResponse userInfoResponse = UserInfoResponse.parse(httpResponse); | ||
|
|
||
| if (userInfoResponse instanceof UserInfoErrorResponse) | ||
| { | ||
| UserInfoErrorResponse userInfoErrorResponse = (UserInfoErrorResponse) userInfoResponse; | ||
| String errorMessage = userInfoErrorResponse.getErrorObject().getDescription(); | ||
| LOGGER.warn("User Info Request failed: " + errorMessage); | ||
| throw new UserInfoException(errorMessage); | ||
| } | ||
| else | ||
| { | ||
| return Optional.of(userInfoResponse); | ||
| } |
There was a problem hiding this comment.
I have made the following changes for the fix. First we are getting the userInfoResponse after that checking it whether it is instance of UserInfoErrorResponse, if it is then we are extracting errorMessage and throwing UserInfoException otherwise returning as it is
Agreed but If we will not format the whole java file then our pre-commit stage is failing |
| LOGGER.warn("User Info Request failed: " + errorMessage); | ||
| throw new UserInfoException(errorMessage); | ||
| } | ||
| else |
There was a problem hiding this comment.
I don't think we need else here, if the exception is not thrown then by default it will come to this line.
There was a problem hiding this comment.
Done. Removed the else part
Also look to resolve the PMD issues. |
| UserInfoErrorResponse userInfoErrorResponse = (UserInfoErrorResponse) userInfoResponse; | ||
| String errorMessage = userInfoErrorResponse.getErrorObject().getDescription(); | ||
| LOGGER.warn("User Info Request failed: " + errorMessage); | ||
| throw new UserInfoException(errorMessage); |
There was a problem hiding this comment.
Do we need to throw the error exception? Can't we extract the error message and use the LOGGER.warn to show the error and then return the empty Optional value as done for ParseException?
There was a problem hiding this comment.
I agree with @kavitshah-gl .
There was a problem hiding this comment.
Do we need to throw the error exception? Can't we extract the error message and use the LOGGER.warn to show the error and then return the empty Optional value as done for ParseException?
If we send the empty response then the Client SDK is not receiving the error at all which does not help users. Further if we continue with the UserInfoErrorResponse further to the map method then it gives ClassCastException. This is happening since UserInfoResponse can be converted to UserInfoSuccessResponse but UserInfoErrorResponse can't ne converted to UserInfoSuccessResponse and in the map it tries to do that . Thus rather than Spring throw the ClassCastException we are throwing UserInfoException which is of type IdentityServiceFacadeException which is caught gracefully in authenticateImpl method.
There was a problem hiding this comment.
Okay I think throwing UserInfoException is fine since we are catching it.
| if (userInfoResponse instanceof UserInfoErrorResponse) | ||
| { | ||
| UserInfoErrorResponse userInfoErrorResponse = (UserInfoErrorResponse) userInfoResponse; | ||
| String errorMessage = userInfoErrorResponse.getErrorObject().getDescription(); |
There was a problem hiding this comment.
In general, it looks good to me.
Please replace UserInfoErrorResponse userInfoErrorResponse = (UserInfoErrorResponse) userInfoResponse;
with pattern variable:
if (userInfoResponse instanceof UserInfoErrorResponse userInfoErrorResponse).
Moreover, I think we can check if userInfoErrorResponse.getErrorObject() is null.
For example:
String errorMessage = Optional.ofNullable(userInfoErrorResponse.getErrorObject())
.map(ErrorObject::getDescription)
.orElse("No error description found");
There was a problem hiding this comment.
I have addressed the following changes
| UserInfoErrorResponse userInfoErrorResponse = (UserInfoErrorResponse) userInfoResponse; | ||
| String errorMessage = userInfoErrorResponse.getErrorObject().getDescription(); | ||
| LOGGER.warn("User Info Request failed: " + errorMessage); | ||
| throw new UserInfoException(errorMessage); |
There was a problem hiding this comment.
Okay I think throwing UserInfoException is fine since we are catching it.
…ty-repo into fix/MNT-24542-Class-Cast-Exception # Conflicts: # repository/src/main/java/org/alfresco/repo/security/authentication/identityservice/SpringBasedIdentityServiceFacade.java
https://hyland.atlassian.net/browse/MNT-24542
Here in this ticket. We are encountering an issue when running ACS with Keycloak configured without the OpenID scope. Upon running the sample SDK with the attached files and executing the generated JAR, we encounter a ClassCastException.
The root cause appears to be that while a UserInfoResponse can be successfully cast to a UserInfoSuccessResponse, it cannot be cast to a UserInfoErrorResponse. This issue occurs in the map, where such an invalid cast is attempted.
To address this, I have implemented a fix. The updated logic first retrieves the UserInfoResponse and checks if it is an instance of UserInfoErrorResponse. If it is, the error message is extracted, and a UserInfoException is thrown. Otherwise, the response is returned as-is.