Implement OAuth2 password flow #2397

MugeSo · 2016-09-10T08:44:58Z

This PR is an OAuth2 password flow implementation works with #2014

This PR updates #1853 and #1574
And this closes #807

webron · 2016-09-14T20:45:44Z

@bodnia can you review?

hkulekci · 2016-09-20T08:08:19Z

@MugeSo at least, client_id is necessary for password grant_type request at OAuth2. Right now, Your update is not sending any client information.

hkulekci · 2016-09-20T08:19:44Z

MugeSo#1 I sent you an pr.

MugeSo · 2016-09-20T09:17:02Z

@hkulekci I cannot find what you say though I read RFC6749 again.
I found only descriptions which says that clinent_id is required for client password authentication(for client authentication), authorization code grant or implicit grant.
And password grant does not requires client authentication. it just optional.

MugeSo · 2016-09-20T09:23:30Z

And there is no spec about oauth2 client authentication in Open API specification.
So I sent issue OAI/OpenAPI-Specification#785.

hkulekci · 2016-09-20T09:59:41Z

@MugeSo you are right. RFC don't force. But I don't understand how the api understand the people come which client in this scenario.

To be able to work this scenario, we must take a client token before sending a request for a password credential request.

Update: I am using https://oauth2.thephpleague.com/authorization-server/resource-owner-password-credentials-grant/ this library. And client_id is required for Password Grant Type request.

MugeSo · 2016-09-20T10:44:19Z

@hkulekci Ok, I see what you want.
I think we need to implement client authentication correctly, to solve your problem.

And it also solves #2183.

@webron @fehguy Which do you prefers implement client authentication in this PR or as another PR?

HugoMario · 2016-09-26T18:33:59Z

I edited the PR (still not check in) in order to test this flow with the sample oauth2 provider that it's being developed. Adding next headers:

var requestHeaders = { 'Authorization': 'Basic ' + btoa(username + ':' + password), 'Content-Type': 'application/x-www-form-urlencoded' };

$.ajax({ ... headers: requestHeaders, ...

Helped me to make flow works and tests oauth2 provider properly.

wimpers · 2016-10-31T08:44:53Z

It seems that the milestone to which #807 belongs is closed. Does this mean that the issue is fixed (I assume not as it is still open) or will this PR be merged in a newer version?

pndv · 2016-11-01T20:19:17Z

I too am eagerly watching this PR, this is going to fix so many workarounds we had to implement.

wimpers · 2016-11-04T15:55:25Z

@fehguy , @bodnia I know you guys must be super busy but could it be that this one wasn't merged in v2.1.4 (#807) ? Can it be merged into the latest version?

bundabrg · 2016-11-07T11:48:53Z

Could not client_id and client_secret be optional parameters asked on the same dialogue as the username/password? That's how I'm implementing it and is far closer to how a real client will work. Saves having to hard-code it in.

frol · 2016-11-09T11:40:57Z

@fehguy @bodnia @webron Can we hear at least a word from you? There were already two PRs (#1574, #1853) which got outdated just because of the project refactoring. I don't want to see this one become outdated once again. I beg you to merge it while it has no conflicts.

bodnia · 2016-11-09T12:17:22Z

@wimpers @bundabrg @frol I'll look into it by Monday EOD, sorry for delays.

frol · 2016-11-11T22:02:02Z

@MugeSo @hkulekci The RFC doesn't say a word about client_id and client_secret for Resource Owner Password Credentials Grant, but the example in section (4.3.2) indicates the Authorization: Basic czZCaGRSa3F0MzpnWDFmQmF0M2JW header, which is the base64(cliend_id:client_secret) just like @HugoMario implemented.

bundabrg · 2016-11-12T01:14:41Z

Actually I'm pretty confused here. The RFC doesn't state anything about client_id or client_secret for a Resource Owner Password Credentials Grant. Yet nearly every other page I've researched about OAuth2 grants described this grant with client_id and client_secret as part of the exchange, including oauthlib requiring it.

frol · 2016-11-12T06:58:22Z

Actually, the section 3.2.1 states the following:

Confidential clients or other clients issued client credentials MUST authenticate with the authorization server as described in Section 2.3 when making requests to the token endpoint.

It seems to me that the reason why client_id is specifically mentioned in some of the flows is that the headers cannot be easily set for those requests (e.g. if you want to perform OAuth2 in pure HTML (without JS) you will only need to define a form with a "hidden" client_id field).

MugeSo · 2016-11-12T15:49:32Z

I've implemented client authentication for password and application flow, because some libraries requires it as you say.

frol · 2016-11-12T20:39:08Z

I have just tested the latest changes and both "Basic auth" and "Request body" work for me on my Flask-Oauthlib based OAuth2 server!

@MugeSo Great job!

frol · 2016-11-12T21:08:28Z

@MugeSo Is there and should be there a way to set a default client_id?

MugeSo · 2016-11-13T05:10:39Z

@frol What is a "default" which you say?
Is it the global variable clientId defined in lib/swagger-oauth.js?

frol · 2016-11-13T05:34:08Z

@MugeSo It used to be it, but since that code seems to be dead and mostly useless, I wouldn't mind if the way to configure the defaults gets changed.

frol · 2016-11-13T14:22:26Z

~~Hmm, I am not sure what is the origin of the problems, so it might be out of the scope of this issue, but let me put them here:~~

~~The checkbox selection of the scopes is ignored, clicking the "Authorize" button always results in sending out all available scopes.~~ Selected OAuth2 scopes are not respected on authentication #2497
When I added <div id="auth_container"></div> container to have a top-level "Authorize" button (like on the top bar here: http://petstore.swagger.io/), the listed scopes include a "vendorExtensions - [object Object]" scope at the end of the scopes list. (I don't experience this issue on the demo site passing my swagger.json there.) OAuth2 request adds vendorExtension scope to all auth requests #2483

~~@MugeSo Do you have any guesses?~~

UPDATE: ~~I have found the reported issue #2483 describing exactly what I experience in (2).~~ Fixed.

UPDATE2: ~~It turned out that the behaviour I experienced in (1) is also a general Swagger-UI bug, so I reported it here #2497.~~ Fixed.

wimpers · 2016-11-18T07:45:30Z

@bodnia any update on the issue? It seems more people have chipped and got it working?

danballance · 2016-11-22T13:56:03Z

Hi folks, we could really do with this support as well. Was a solution found? Anything I can do to help?

frol · 2016-11-22T14:00:40Z

@danballance I think you may try and test it against your server (I have tested it against Flask-OAuthlib and everything works for me as far as I can tell, but it is always better to have an extra pair of eyes here, I guess).

@MugeSo @bodnia A conflict in AuthView.js appeared. :(

frol · 2016-11-22T14:06:24Z

By the way, there is a demo API server which bundles this patch rebased on top of a recent master branch: https://github.com/frol/flask-restplus-server-example. The demo is written in Python, but it is really easy to evaluate it if you use Docker:

$ docker run -it --rm --publish 5000:5000 frolvlad/flask-restplus-server-example

http://127.0.0.1:5000/api/v1/ (login: root, password: q)

MugeSo · 2016-11-24T08:39:55Z

@frol Thank you for mention. I resolved it :)

frol · 2016-11-24T09:29:00Z

src/main/javascript/view/Oauth2Model.js

-        scopes: {}
+        scopes: {},
+        isPasswordFlow: false,
+        clientAuthenticationType: 'none'


Can this default be configurable? In fact, I would love to have it configured to be Basic by default as it is what the RFC describes.

frol · 2016-11-25T12:25:55Z

src/main/javascript/view/Oauth2Model.js

+            var flow = this.attributes.flow;
+            this.set('isPasswordFlow', flow === 'password');
+            this.set('requireClientAuthentication', flow === 'application');
+            this.set('clientAuthentication', flow === 'password' || flow === 'application');


It seems to me that clientAuthentication should be available in every OAuth2 Flow.

Yes, it's advisable what you say.
But, this PR does not support client authentication for other flows.
I add client authentication just to make password flow usable.
I just want to merge password-flow implementation quickly!!

wimpers · 2016-11-29T13:47:33Z

@bodnia any update on the issue? It seems more people have chipped and got it working?

danballance · 2016-12-02T16:25:51Z

Any joy with this folks? Is this functionality available in master now or am I getting ahead of myself?

pndv · 2016-12-05T20:18:18Z

Please merge this guys, I am waiting for this fix for a long time :)

ncarlier · 2016-12-06T09:42:29Z

+1

bodnia · 2016-12-06T16:16:25Z

@webron I tested, and this works ok and can be merged

webron · 2016-12-06T16:52:13Z

Thanks for all the work and tweaks, @MugeSo - glad to see such a contribution to the project.

frol · 2016-12-06T16:53:54Z

WOW! Supercalifragilisticexpialidocious!

hkulekci · 2016-12-06T19:52:20Z

woow. Finally merged. Thank you.

VenomAV · 2017-05-03T20:29:05Z

I do not know if this is the right place for this question: is there any way to specify default client_id and client_secret for password flow through Swagger UI configurations?

bodnia · 2017-05-04T07:09:44Z

@VenomAV one of options is from #3010 and one more option is with configs, but not implemented yet

webron assigned bodnia Sep 14, 2016

rosenstrauch mentioned this pull request Oct 20, 2016

Real user Auth/ Access Scopes - now we add some access control to our api acolono/opencounter_api#25

Open

frol mentioned this pull request Nov 13, 2016

OAuth2 request adds vendorExtension scope to all auth requests #2483

Closed

MugeSo added 3 commits November 24, 2016 17:04

Implement OAuth2 password flow

7cdf83a

Add validation for oauth password flow

f2a1caa

Implement OAuth2 client authentication for password and application flow

3494d44

MugeSo force-pushed the implement-oauth2-password-flow branch from 297eb12 to 3494d44 Compare November 24, 2016 08:36

frol reviewed Nov 24, 2016

View reviewed changes

frol reviewed Nov 25, 2016

View reviewed changes

webron merged commit 2ba1c10 into swagger-api:master Dec 6, 2016

pdusen mentioned this pull request Dec 13, 2016

Swashbuckle 5.0 and Password Grant Flow domaindrivendev/Swashbuckle.WebApi#202

Closed

fehguy modified the milestone: v2.2.9 Jan 5, 2017

pdusen mentioned this pull request Jan 24, 2017

Update to newest version of Swagger UI to support Password Flow domaindrivendev/Swashbuckle.WebApi#986

Closed

danballance mentioned this pull request Feb 7, 2017

Best way to handle "application/problem+json" core-api/python-client#114

Closed

iCodr8 mentioned this pull request Mar 12, 2017

Add an example/tutorial for OAuth2 and Swagger api-platform/docs#180

Closed

domaindrivendev mentioned this pull request Mar 14, 2017

OAuth2 Resource Owner Password Credentials domaindrivendev/Swashbuckle.AspNetCore#326

Closed

frol mentioned this pull request Mar 23, 2017

OAuth2 Password Flow is broken in Swagger UI 3.x #2774

Closed

MugeSo mentioned this pull request Jul 21, 2017

OAuth2 Password Flow modes do not match RFC #3227

Open

Implement OAuth2 password flow #2397

Implement OAuth2 password flow #2397

Conversation

MugeSo commented Sep 10, 2016 • edited Loading

webron commented Sep 14, 2016

hkulekci commented Sep 20, 2016

hkulekci commented Sep 20, 2016

MugeSo commented Sep 20, 2016 • edited Loading

MugeSo commented Sep 20, 2016 • edited Loading

hkulekci commented Sep 20, 2016 • edited Loading

MugeSo commented Sep 20, 2016

HugoMario commented Sep 26, 2016

wimpers commented Oct 31, 2016 • edited Loading

pndv commented Nov 1, 2016

wimpers commented Nov 4, 2016

bundabrg commented Nov 7, 2016

frol commented Nov 9, 2016 • edited Loading

bodnia commented Nov 9, 2016

frol commented Nov 11, 2016

bundabrg commented Nov 12, 2016

frol commented Nov 12, 2016

MugeSo commented Nov 12, 2016 • edited Loading

frol commented Nov 12, 2016

frol commented Nov 12, 2016

MugeSo commented Nov 13, 2016

frol commented Nov 13, 2016

frol commented Nov 13, 2016 • edited Loading

wimpers commented Nov 18, 2016

danballance commented Nov 22, 2016

frol commented Nov 22, 2016

frol commented Nov 22, 2016

MugeSo commented Nov 24, 2016

frol Nov 24, 2016 • edited Loading

Choose a reason for hiding this comment

frol Nov 25, 2016

Choose a reason for hiding this comment

MugeSo Nov 28, 2016

Choose a reason for hiding this comment

wimpers commented Nov 29, 2016

danballance commented Dec 2, 2016

pndv commented Dec 5, 2016

ncarlier commented Dec 6, 2016

bodnia commented Dec 6, 2016

webron commented Dec 6, 2016

frol commented Dec 6, 2016

hkulekci commented Dec 6, 2016

VenomAV commented May 3, 2017

bodnia commented May 4, 2017

MugeSo commented Sep 10, 2016 •

edited

Loading

MugeSo commented Sep 20, 2016 •

edited

Loading

MugeSo commented Sep 20, 2016 •

edited

Loading

hkulekci commented Sep 20, 2016 •

edited

Loading

wimpers commented Oct 31, 2016 •

edited

Loading

frol commented Nov 9, 2016 •

edited

Loading

MugeSo commented Nov 12, 2016 •

edited

Loading

frol commented Nov 13, 2016 •

edited

Loading

frol Nov 24, 2016 •

edited

Loading