-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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] Add support for Bearer Auth #1930
Conversation
What I am missing here is that both |
I'm not sure I follow your comment. Is your feedback on the direction this
patch has taken, given the current state of DefaultCodegen? Or are you
suggesting further changes to DefaultCodegen?
To be clear, I am trying to get API endpoints with Bearer Authentication
defined as:
userToken:
type: http
scheme: bearer
to produce a working client that submits requests with "Authorization:
Bearer xxxx" instead of today's broken behavior which produces clients that
submits requests with "Authorization: Basic [base64(username+password)]".
What changes would you like to see in this patch to make it more acceptable?
Rather than introduce the BearerAuth object, would you prefer that the
HttpBasicAuth object be "smarter" and made to handle both cases like this:
public class HttpAuth implements Authentication {
private final String scheme;
private String token;
public HttpAuth(String scheme) {
this.scheme = scheme;
}
public String setBearerToken(String token) {
if("basic".equalsIgnoresCase(scheme)){
throw new Exception("cannot set bearerToken for basic http auth");
}
this.token = token;
}
public void setBasicCredentials(String username, String password) {
if(!"basic".equalsIgnoresCase(scheme)){
throw new Exception("cannot set basic credentials for non-basic scheme");
}
String str = (username == null ? "" : username) + ":" + (password == null ? "" : password);
this.token = Base64.encodeToString(str.getBytes("UTF-8"), false);
}
@OverRide
public void applyToParams(List<Pair> queryParams, Map<String, String>
headerParams) {
headerParams.put("Authorization", scheme + " " + token);
}
}
…On Thu, Jan 17, 2019 at 11:14 AM Lorenz Leutgeb ***@***.***> wrote:
What I am missing here is that both Bearer and Basic authentication are
actually closely related and just two variants of HTTP authorization
schemes. This connection is not really clear here. However it is a step in
the right direction and probably a solution that generalises nicely can be
put in place.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1930 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAfikezicgy5NRb4qYMkSjLkGk0MXEU1ks5vEMuSgaJpZM4aEQZs>
.
|
} | ||
|
||
@Override | ||
public void applyToParams(List<Pair> queryParams, Map<String, String> headerParams) { |
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.
Minor suggestions: adding docstrings to these public methods.
c0a24ef
to
a100596
Compare
See OpenAPITools#457 Also OpenAPITools#1446 for typescript, OpenAPITools#1577 for python Specs defined as follows currently generate BasicAuth and send "Authorization: Basic [base64Encode(username + ":" + password)]" components: securitySchemes: bearer: type: http scheme: bearer This change will generate an OAuth header, which will send a "Authorization: Bearer [accessToken]" This is a smaller, less-impactful change than introducing a BearerAuth object, but this change doesn't support scheme values other than bearer See also OpenAPITools#1930
See OpenAPITools#457 Also OpenAPITools#1446 for typescript, OpenAPITools#1577 for python Specs defined as follows currently generate BasicAuth and send an "Authorization: Basic [base64Encode(username + ":" + password)]" header components: securitySchemes: bearer: type: http scheme: bearer This change will generate code which uses a new HttpBearerAuth class, which will send a "Authorization: [scheme] [accessToken]" header. This change is slightly larger and more impactful than simply using OAuth for bearerBearer, but it allows for scheme values other than bearer. This fix was enabled by the recent commit of OpenAPITools@80ca67c This PR is an alternative to OpenAPITools#1972
I've updated the samples to included new files, e.g. samples/client/petstore/java/feign/src/main/java/org/openapitools/client/auth/HttpBearerAuth.java Let's see if the CI tests pass. |
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.
LGTM
@wing328 Any idea when this change will be released? |
I am not a maintainer, but I expect it in the next release on Feb 28
according to this:
#1462
…On Mon, Feb 25, 2019 at 10:28 AM Michael Stead ***@***.***> wrote:
Any idea when this change will be released?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1930 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAfikXOarUjFaaSwUxJOUNQ3ek_8qUqXks5vRCtDgaJpZM4aEQZs>
.
|
* fix OpenAPITools#457 by introducing an HttpBearerAuth object See OpenAPITools#457 Also OpenAPITools#1446 for typescript, OpenAPITools#1577 for python Specs defined as follows currently generate BasicAuth and send an "Authorization: Basic [base64Encode(username + ":" + password)]" header components: securitySchemes: bearer: type: http scheme: bearer This change will generate code which uses a new HttpBearerAuth class, which will send a "Authorization: [scheme] [accessToken]" header. This change is slightly larger and more impactful than simply using OAuth for bearerBearer, but it allows for scheme values other than bearer. This fix was enabled by the recent commit of OpenAPITools@80ca67c This PR is an alternative to OpenAPITools#1972 * update petstore samples * Update HttpBearerAuth mustache templates and samples * correct the expected number of generated java client files * update the retrofit2 HttpBearerAuth template and samples * Add resttemplate-specific HttpBearerAuth mustache and samples * add vertx-specific HttpBearerAuth template and samples * add java webclient-specific HttpBearerAuth template and samples
I just posted an update in #1462 |
Is this supposed to generate Javadocs? I'm seeing this: [WARNING] Javadoc Warnings
[WARNING] /Users/user/company/project/client/java/src/main/java/com/company/auth/HttpBearerAuth.java:33: warning: no @return
[WARNING] public String getBearerToken() {
[WARNING] ^
[WARNING] /Users/user/company/project/client/java/src/main/java/com/company/auth/HttpBearerAuth.java:40: warning: no @param for bearerToken
[WARNING] public void setBearerToken(String bearerToken) {
[WARNING] ^ |
Hmm. This doesn't seem to actually work: generating a Java SDK using master creates docs that say
|
I agree with @intelliot And I see the same result, spec says:
and the code generated is
I was expecting something like
It would be great to have it confirmed that this feature is actually testable ! |
What library are you specifying in your pom and generating code for? From what I can tell, there is going to be some separate work required for each library (to modify the mustache template for ApiClient for your library similar to this code). I admit was focused on trying to fix this for jersey2 for my own selfish purposes. Maybe this will point you in the right direction to submit a pull request or maybe I can take a crack if I get some time. I hope this helps! |
@intelliot |
I added an OAuth scheme expecting to see the setBearerToken method HttpBearerAuth class. |
#2485 has been merged into master to provide better Bearer authentication support on all Java API clients. Please pull the latest master to give it a try. |
Fix proposed for Issue #457
Similar to Issue #1446 for typescript, Issue #1577 for python
Specs defined as follows currently generate
HttpBasicAuth
and send anAuthorization: Basic [base64Encode(username + ":" + password)]
headerThis change will generate code which uses a new
HttpBearerAuth
class, which will send anAuthorization: [scheme] [accessToken]
header.This change is slightly larger and more impactful than reusing the
OAuth
whenCodegenSecurity.bearerBearer
is true, but it allows for scheme values other than bearer.This fix was enabled by the recent commit of 80ca67c
This PR is an alternative to PR #1972
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
.master
,. Default:3.4.x
,4.0.x
master
.Description of the PR
(details of the change, additional tests that have been done, reference to the issue for tracking, etc)
Add a new Auth type for Bearer so that api specs using Bearer Authentication https://swagger.io/docs/specification/authentication/bearer-authentication/ use the correct auth (was BasicAuth).
Copying the Java technical committee as this is a change targeted to Java.
@bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger