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

Trim OIDC claim role path #26256

Merged
merged 1 commit into from Jun 21, 2022
Merged

Conversation

sberyozkin
Copy link
Member

Follows #26185.

@gastaldi Sorry, I commented in #26185 that I was also adding a role path trim, but I've found today I forgot to commit

@sberyozkin
Copy link
Member Author

Thanks George

@gsmet
Copy link
Member

gsmet commented Jun 21, 2022

I wonder if it wouldn't be better to handle that at the @ConfigItem level with @ConvertWith(TrimmedStringConverter.class)?

@gastaldi
Copy link
Contributor

I wonder if it wouldn't be better to handle that at the @ConfigItem level with @ConvertWith(TrimmedStringConverter.class)?

That would be much better, +1

@sberyozkin
Copy link
Member Author

Sure

@sberyozkin
Copy link
Member Author

@gastaldi @gsmet Hey, it does not work with a List property, after adding @ConvertWith(TrimmedStringConverter.class) to
https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java#L441 and removing trim() from the code I've got the test failing.

I guess it should only be applied to Strings, I can follow up at some point with reviewing the OIDC docs and adding it where possible

@gastaldi
Copy link
Contributor

gastaldi commented Jun 21, 2022

@gastaldi @gsmet Hey, it does not work with a List property, after adding @ConvertWith(TrimmedStringConverter.class) to main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java#L441 and removing trim() from the code I've got the test failing.

I guess it should only be applied to Strings, I can follow up at some point with reviewing the OIDC docs and adding it where possible

That is weird, maybe that's a bug in the converter? WDYT @radcortez?

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

OK, let's get this in as is for now, then. We can revisit later.

@gsmet gsmet merged commit fd26ffc into quarkusio:main Jun 21, 2022
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jun 21, 2022
@sberyozkin
Copy link
Member Author

Sure

@sberyozkin sberyozkin deleted the trim_oidc_role_path branch June 21, 2022 14:49
@gsmet gsmet modified the milestones: 2.11 - main, 2.10.1.Final Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants