-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[Java] [Kotlin] (#14876) fix use of isBasic conditions - do not use HttpBearerAuth (or HttpBasicAuth) for other http auth methods (such as http signature auth or custom schemes) #15220
[Java] [Kotlin] (#14876) fix use of isBasic conditions - do not use HttpBearerAuth (or HttpBasicAuth) for other http auth methods (such as http signature auth or custom schemes) #15220
Conversation
c6315a6
to
516d5f5
Compare
Additional precision regarding target of this PR: some projects might rely on this bug if their openapi.yaml file contains a valid authorization scheme unsupported by the used generator in order to have these auth methods included in the generated code and then have custom workarounds implemented in their project to replace the way the authentication is handled. Because I was not sure of how much this fix could disrupt projects misusing the authorization part, I decided to go against 7.x rather than master. In any case, as I mentioned in the linked issue, I initially observed the bug with version 4.2.3 and it probably existed for a long time already, so it looks like it does not affect a lot of people and can wait a little longer and be marked as "potential breaking change without fallback" in the next major release. |
516d5f5
to
928c086
Compare
928c086
to
97a7d50
Compare
auth = new HttpBearerAuth("{{scheme}}"); | ||
{{/isBasicBearer}} | ||
{{^isBasicBearer}} | ||
throw new RuntimeException("auth name \"" + authName + "\" does not have a supported http scheme type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: that part might be a breaking change for those relying on the bug
auth = new HttpBearerAuth("{{scheme}}"); | ||
{{/isBasicBearer}} | ||
{{^isBasicBearer}} | ||
throw new RuntimeException("auth name \"" + authName + "\" does not have a supported http scheme type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: that part might be a breaking change for those relying on the bug
auth = new HttpBearerAuth("{{scheme}}"); | ||
{{/isBasicBasic}}{{/isBasic}} | ||
{{/isBasicBearer}}{{^isBasicBearer}} | ||
throw new RuntimeException("auth name \"" + authName + "\" does not have a supported http scheme type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: that part might be a breaking change for those relying on the bug
@@ -128,7 +128,7 @@ import okhttp3.MediaType.Companion.toMediaType | |||
) : this(baseUrl, okHttpClientBuilder{{^kotlinx_serialization}}, serializerBuilder{{/kotlinx_serialization}}) { | |||
authNames.forEach { authName -> | |||
val auth = when (authName) { | |||
{{#authMethods}}"{{name}}" -> {{#isBasic}}{{#isBasicBasic}}HttpBasicAuth(){{/isBasicBasic}}{{#isBasicBearer}}HttpBearerAuth("{{scheme}}"){{/isBasicBearer}}{{/isBasic}}{{#isApiKey}}ApiKeyAuth({{#isKeyInHeader}}"header"{{/isKeyInHeader}}{{#isKeyInQuery}}"query"{{/isKeyInQuery}}{{#isKeyInCookie}}"cookie"{{/isKeyInCookie}}, "{{keyParamName}}"){{/isApiKey}}{{#isOAuth}}OAuth(OAuthFlow.{{flow}}, "{{authorizationUrl}}", "{{tokenUrl}}", "{{#scopes}}{{scope}}{{^-last}}, {{/-last}}{{/scopes}}"){{/isOAuth}}{{/authMethods}} | |||
{{#authMethods}}"{{name}}" -> {{#isBasic}}{{#isBasicBasic}}HttpBasicAuth(){{/isBasicBasic}}{{^isBasicBasic}}{{#isBasicBearer}}HttpBearerAuth("{{scheme}}"){{/isBasicBearer}}{{^isBasicBearer}}throw RuntimeException("auth name $authName does not have a supported http scheme type"){{/isBasicBearer}}{{/isBasicBasic}}{{/isBasic}}{{#isApiKey}}ApiKeyAuth({{#isKeyInHeader}}"header"{{/isKeyInHeader}}{{#isKeyInQuery}}"query"{{/isKeyInQuery}}{{#isKeyInCookie}}"cookie"{{/isKeyInCookie}}, "{{keyParamName}}"){{/isApiKey}}{{#isOAuth}}OAuth(OAuthFlow.{{flow}}, "{{authorizationUrl}}", "{{tokenUrl}}", "{{#scopes}}{{scope}}{{^-last}}, {{/-last}}{{/scopes}}"){{/isOAuth}}{{/authMethods}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: that part might be a breaking change for those relying on the bug
@@ -70,7 +70,7 @@ public ApiClient(String[] authNames) { | |||
} else if ("bearer_test".equals(authName)) { | |||
auth = new HttpBearerAuth("bearer"); | |||
} else if ("http_signature_test".equals(authName)) { | |||
auth = new HttpBearerAuth("signature"); | |||
throw new RuntimeException("auth name \"" + authName + "\" does not have a supported http scheme type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: that part might be a breaking change for those relying on the bug
…p auth method ignore unsupported http auth method unless generated code would not compile (in which case, an exception is thrown)
97a7d50
to
97ea4e2
Compare
First small replacement for #14878 to fix #14876
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
(6.3.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)@bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608
from previous PR: @borsch @wing328