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

OAuth2 metadata url #3594

Closed
wants to merge 2 commits into from
Closed

OAuth2 metadata url #3594

wants to merge 2 commits into from

Conversation

AxelNennker
Copy link
Contributor

@AxelNennker AxelNennker commented Feb 19, 2024

Add oauth2MetadataUrl to oauth2 allows the client to download the OAuth2 client to download the RFC8414 OAuth2 metadata.

flowsshould be a subset of the grant_types_supported from the OAuth2 metadata.

Fixes #3595

Signed-off-by: Axel Nennker <axel.nennker@telekom.de>
Signed-off-by: Axel Nennker <axel.nennker@telekom.de>
@handrews
Copy link
Member

@AxelNennker I'm afraid I owe you an apology- I only noticed the wording clarification on the last PR and did not realize you were trying to add a field. We can't add fields in patch releases, so this would have to go in 3.2.
We're currently discussing exactly what can go in patch and minor releases:

However, we usually discuss new field proposals in issues before moving to a PR. If it works with your time zone, you might want to join the Thursday technical community call and talk about what you're trying to do and how to fit it with the current release plans.

@AxelNennker
Copy link
Contributor Author

no apology needed.

Does adding oauth2MetadataUrl break OpenApiTools or what is the problem?
Last time I tried openIdConnectUrl with codegen there was no variable generated in my Java client code. No problem.
So, I expect that codegen digests new fields like oauth2MetadataUrl, maybe warn about them, that a field is not understood, but continue doing the good stuff. Maybe there is other stuff that breaks when a new field is added?

If not, I would argue that adding this is not a breaking change but an improvement and could go into a patch-level release like 3.0.4. Even if it just documenting the url of the AZ to the client.

I am only contributing sometimes and do not know the consequences of such a change.

9am Thursday is 18:00 Berlin time. That is possible.
Please open an issue if you think specifying the url of the OAuth2 server is helpful for developers.

@handrews
Copy link
Member

Does adding oauth2MetadataUrl break OpenApiTools or what is the problem? Last time I tried openIdConnectUrl with codegen there was no variable generated in my Java client code. No problem.
So, I expect that codegen digests new fields like oauth2MetadataUrl, maybe warn about them, that a field is not understood, but continue doing the good stuff. Maybe there is other stuff that breaks when a new field is added?

The OpenAPI Initiative does not make any tools (except for the future oascomply tool, but that's more for validating that other tools are behaving correctly). The policies on what changes go in which releases have to do with compatibility guarantees. We're currently working on two patch releases (3.0.4 and 3.1.1), a minor release (3.2.0) and a major release (4.0.0) so there's a home for pretty much any potential change.

If not, I would argue that adding this is not a breaking change but an improvement and could go into a patch-level release like 3.0.4. Even if it just documenting the url of the AZ to the client.

Patch releases only fix errors and clarify existing requirements. They never add anything. Minor releases can add compatible changes, so that's where this would go.

Please open an issue if you think specifying the url of the OAuth2 server is helpful for developers.

Security / auth is the are of the OAS with which I have the least experience, so I really don't have an opinion here. Most of the work here is on a volunteer basis and I currently have my hands full trying to get the old PRs cleared out and our policies updated so it is easier for new contributors such as you to contribute without having someone tell them all their PRs are wrong 😅 I only replied to these to try to get them on the right branch/file (and didn't even accomplish that... trying to do too many things at once, I'm afraid).

@AxelNennker
Copy link
Contributor Author

created the issus #3595

Now moving this to 3.2-dev. I guess I have to close this PR and open a new one there.

@handrews handrews added security security: auth Authentication including overlap with authorization labels Feb 20, 2024
@handrews
Copy link
Member

@AxelNennker yeah, I'll go ahead and close this for you since you mentioned it. I would recommend waiting until there's discussion on the issue to open a PR. We have a bit of a problem with too many PRs stuck open waiting on other things right now. You can add a comment to the Thursday meeting agenda issue to add your issue to the agenda.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security: auth Authentication including overlap with authorization security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants