Skip to content

use toString() instead of cast String or it will fail when using Spring configtree#10980

Merged
abstractj merged 3 commits into
keycloak:mainfrom
ianwallen:patch-1
May 25, 2022
Merged

use toString() instead of cast String or it will fail when using Spring configtree#10980
abstractj merged 3 commits into
keycloak:mainfrom
ianwallen:patch-1

Conversation

@ianwallen

Copy link
Copy Markdown
Contributor

Fix for #10979

By using toString() instead of cast (String), it is able to convert the configtree value correctly to a string value.

@abstractj abstractj self-assigned this May 12, 2022
@Override
public void init(KeycloakDeployment deployment, Object config) {
clientSecret = (String) config;
clientSecret = config.toString();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ianwallen thanks for submitting those changes. But I believe we need a better alternative here. Those changes will fix part of the problem, but if config is null we will get an NPE.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made a change that should resolve the potential NPE issue.

@abstractj abstractj requested a review from ahus1 May 12, 2022 12:28
Fix potential NPE issue if config was null.
@abstractj abstractj requested a review from pedroigor May 16, 2022 22:36

@abstractj abstractj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor changes LGTM, but I would like your opinion before merging it.

@ahus1 ahus1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used config tree with Spring Boot yet. Looking at the APIs, this will return a ConfigTreePropertySource.Value which implements both CharSequence and InputStreamSource.

To retrieve a String, toString() with the null check looks fine to me. At the same time there are several other casts to String in this module. I suspect some of them might fail in a similar way.

I think it is OK to merge this, and I'd encourage the community to investigate if there are other similar issues. An example repository with a ready-to-test example app would be helpful.

@abstractj abstractj merged commit 78b8876 into keycloak:main May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants