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

Load jwtClientAuthentication in uaa.yml #2758

Merged
merged 1 commit into from
Mar 8, 2024
Merged

Conversation

strehle
Copy link
Member

@strehle strehle commented Mar 4, 2024

By mistake the variable jwtclientAuthentication was used in PR for clientAuthentication.
Documentation explains jwtClientAuthentication as entry

By mistake the variable jwtclientAuthentication was used in PR for clientAuthentication.
Documentation explain jwtClientAuthentication as entry
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187161959

The labels on this github issue will be updated when the story is started.

@peterhaochen47
Copy link
Member

Before I look at this PR, I want to understand the original issue first. See comment.

@strehle strehle changed the title Fix first part of issue #2752 Load jwtClientAuthentication in uaa.yml Mar 7, 2024
Copy link
Member Author

@strehle strehle left a comment

Choose a reason for hiding this comment

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

@peterhaochen47
Copy link
Member

Thanks @strehle! I personally think that fixing the code to be aligned with the correct case-sensitive field jwtClientAuthentication as documented in our API doc (without being compatible with the incorrect jwtclientAuthentication) is okay, as it can be justified as a bug fix (and not as a breaking change).

What do you think? Are there a lot of adoptions of the incorrect jwtclientAuthentication on your side already?

@strehle
Copy link
Member Author

strehle commented Mar 8, 2024

What do you think? Are there a lot of adoptions of the incorrect jwtclientAuthentication on your side already?

No I am sure, that not many adoptions out with incorrect setting, but with this PR both settings would be used

But I asked me to change this PR so that only the correct setting is used?

@peterhaochen47
Copy link
Member

What do you think? Are there a lot of adoptions of the incorrect jwtclientAuthentication on your side already?

No I am sure, that not many adoptions out with incorrect setting, but with this PR both settings would be used

But I asked me to change this PR so that only the correct setting is used?

Simply correcting the code to comply with the doc (without being compatible with the incorrect jwtclientAuthentication) would be my preference, as it's technically a bug fix, rather than an intentional field name migration. But if maintaining compatibility with the incorrect field is important to you, I'm okay with the current code too.

@strehle
Copy link
Member Author

strehle commented Mar 8, 2024

I'm okay with the current code too.

Ok, then I would like to stay with current code. In current code jwtClientAuthentication is used first, so no extra step if correct name is used, but for others jwtclientAuthentication would still work

@strehle strehle merged commit b9348ef into develop Mar 8, 2024
20 checks passed
@strehle strehle deleted the fix/issue/2752/part1 branch March 8, 2024 09:34
@cf-gitbot cf-gitbot added delivered accepted Accepted the issue and removed delivered labels Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted the issue
Projects
Development

Successfully merging this pull request may close these issues.

Support reuse of OIDC identity provider configuration jwtClientAuthentication on a global level
3 participants