Bugfix/add missing grant type #140

DeepDiver1975 · 2018-10-04T07:44:07Z

No description provided.

JGrancha

@DeepDiver1975 is right. It is necessary to collect and send this parameter to obtain the token with the OAuth2 provider. I need this.

dgrubelic · 2018-10-08T10:50:02Z

src/oauth/oauth2.js

        case 'code':
-          payload[key] = oauth.code
-          break
+            payload[value] = oauth.code


I now see that this is actually not necessary at all because default case will pick code value and set it to request payload.

addressed in https://github.com/dgrubelic/vue-authenticate/pull/139/files#diff-c3849faba68f56a667cf75ea2c02ea81

dgrubelic · 2018-10-08T13:08:48Z

src/oauth/oauth2.js

@@ -78,20 +79,23 @@ export default class OAuth2 {
    let payload = objectExtend({}, userData)

    for (let key in defaultProviderConfig.responseParams) {
-      let value = defaultProviderConfig[key]
+      let value = defaultProviderConfig.responseParams[key]


Actually, we shouldn't use responseParams from defaultProviderConfig but from this.providerConfig instead (because defaultProviderConfig is actually merged into this.providerConfig). That way we could pass responseParams when setting up vue-authenticate provider config which will be included in the exchangeForToken request. I believe that this approach would make much more elegant solution for the reported issue.

addressed in https://github.com/dgrubelic/vue-authenticate/pull/139/files#diff-c3849faba68f56a667cf75ea2c02ea81

DeepDiver1975 · 2018-10-08T18:56:24Z

I'll rebase this PR once we have an agreement onaddressed in #139 - objections? THX

dgrubelic · 2018-10-08T19:19:20Z

Let's merge this into one PR so we can avoid any complications having two fixing the same codebase. Imho, you can do everything in this PR and close the other one.

…is a required parameter

DeepDiver1975 · 2018-10-08T20:03:36Z

@dgrubelic I pushed all my changes into this PR as requested. THX

dgrubelic · 2018-10-08T20:15:16Z

src/oauth/oauth2.js

+    if (this.providerConfig.confidentialClient === true) {
+      config.auth = {
+        username: this.providerConfig.clientId,
+        password: this.providerConfig.clientSecret


Oh, this is a problem. As i said in your other PR #141 , this is a major security issue and i'm not going to merge this into my library. This is probably the reason why Satellizer (which my library is built upon) doesn't even support password oauth flow - because it requires client secret.

Primary task of this library is to provide JWT authentication based on OAuth 1/2 protocols.

As i said in your other PR #141 ,

I missed this - can you please reference your comment? THX

this is a major security issue and i'm not going to merge this into my library.

Well - client secrets are distributed quite publicly in some cases and yes they can no longer be considered that secret ;-)

Would it be valid to give the user of this library the chance to use it in an environment where clients have to authenticate via id+secret?

It's the only comment in #141 PR :)

Somebody manipulating the matrix ..... ?

No, it is not. If you see example and server.js file (https://github.com/dgrubelic/vue-authenticate/blob/master/example/server.js), you can see that you can do full OAuth 2 flow using any of the supported (or custom) OAuth providers. There you can use client secret in exchange code for token request.

This is true if you have your own server implementation - but this does not follow the oauth spec.
If you run the token exchange you are in an authorization grant. And this would require the changes in this PR.

Anyway - as explained below I use implicit grant and this lib works as a charm. THX 🍻

but this does not follow the oauth spec

I think that this actually is not entirely true. Simply because, if you go to this link https://tools.ietf.org/html/rfc6749#section-1.3.1 and see the last change date (October 2012), when OAuth 2 protocol spec last change was written, SPA's were not this popular and basically every web applications developed had a server-side implementation - they actually were classic server-side applications that rendered entire HTML on the server. So, technically, we could consider that authors of OAuth specs actually considered that you will have closed and secured server environment where client secret wasn't exposed as it could be in SPA's. Even though they speak about "client", in my opinion, it is meant as:

client: an application that sends authentication requests (at that time server-side application)

authentication server: a server that contains sensitive user data and actually does authentication exchange and provides the client with auth token.

Only in the last few years, when SPA's became popular, term "client" in OAuth protocol specs can actually be both secure server-side application AND insecure (regarding client secret) single page application. I guess this is causing some confusion.

With this, i think that we can close this argument and PR as well.

For the sake of completness: https://www.oauth.com/oauth2-servers/single-page-apps/

It talks about implicit flow as well as authorization code flow WITHOUT secret. So you are totally right. Only the grant type is missing in the request to fully support this.

Anyway - let's close this PR for now.

Thanks a lot! 🍻

dgrubelic · 2018-10-08T20:19:52Z

src/oauth/oauth2.js

-          payload[key] = oauth.code
-          break
+        case 'grantType':
+            payload[value] = 'authorization_code'


Also, i don't like this solution with hardcoded values. Based on OAuth 2 protocol, grant_type doesn't always have to be authorization_code.

Shall we grab it from the providerConfig? Other flows might even require totally different requests - no?

We could, but then it must be like this: payload[key] = value. Honestly, i don't like this because it introduces confusion because in every other case we are setting value as a payload prop, only in case of grantType we are actually settingkey prop.

BTW, what OAuth flow you're trying to implement in your application?

We are following the RFC: https://tools.ietf.org/html/rfc6749?#section-4.1.3

This is the way we are expecting the request to look like:

POST /token HTTP/1.1 Host: server.example.com Authorization: Basic czZCaGRSa3F0MzpnWDFmQmF0M2JW Content-Type: application/x-www-form-urlencoded grant_type=authorization_code&code=SplxlOBeZQQYbYS6WxSbIA &redirect_uri=https%3A%2F%2Fclient%2Eexample%2Ecom%2Fcb

I'll have to think about the possible solution and get back to you tomorrow.

Thanks for your contribution so far :)

I'll have to think about the possible solution and get back to you tomorrow.

Thanks a lot! Let me know if there are further things which need to be discussed!

DeepDiver1975 · 2018-10-10T11:03:41Z

@dgrubelic I removed now the client secret support. What to do now with grant-type? Where to read the default value from? THX

DeepDiver1975 · 2018-10-11T14:33:07Z

Finally wrapped my brain around all this. This lob works out of the box perfectly if you use implicit grant type. Grant type authorization code is not supported due to claimed security reason (different people might think different about this).

I switched over to implicit grants and I'm fine now. THX.

The code in question (exchangeForToken could be killed from my pov - it is not needed for implicit grants)

JGrancha approved these changes Oct 8, 2018

View reviewed changes

dgrubelic requested changes Oct 8, 2018

View reviewed changes

The values in responseParams are now respected as keys for the payload

c19c4dd

As per https://tools.ietf.org/html/rfc6749?#section-4.1.3 grant_type …

b1fcc20

…is a required parameter

dgrubelic requested changes Oct 8, 2018

View reviewed changes

Build dist & example

827d3c7

DeepDiver1975 closed this Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/add missing grant type #140

Bugfix/add missing grant type #140

DeepDiver1975 commented Oct 4, 2018

JGrancha left a comment

dgrubelic Oct 8, 2018

DeepDiver1975 Oct 8, 2018

dgrubelic Oct 8, 2018 •

edited

DeepDiver1975 Oct 8, 2018

DeepDiver1975 commented Oct 8, 2018

dgrubelic commented Oct 8, 2018

DeepDiver1975 commented Oct 8, 2018

dgrubelic Oct 8, 2018

DeepDiver1975 Oct 8, 2018

DeepDiver1975 Oct 8, 2018

dgrubelic Oct 8, 2018

DeepDiver1975 Oct 8, 2018

dgrubelic Oct 11, 2018

DeepDiver1975 Oct 12, 2018

DeepDiver1975 Oct 12, 2018

dgrubelic Oct 12, 2018

DeepDiver1975 Oct 12, 2018

dgrubelic Oct 8, 2018

DeepDiver1975 Oct 8, 2018

dgrubelic Oct 8, 2018 •

edited

DeepDiver1975 Oct 8, 2018

dgrubelic Oct 8, 2018

dgrubelic Oct 8, 2018

DeepDiver1975 Oct 8, 2018

DeepDiver1975 commented Oct 10, 2018

DeepDiver1975 commented Oct 11, 2018

Bugfix/add missing grant type #140

Bugfix/add missing grant type #140

Conversation

DeepDiver1975 commented Oct 4, 2018

JGrancha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrubelic Oct 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Oct 8, 2018

dgrubelic commented Oct 8, 2018

DeepDiver1975 commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrubelic Oct 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Oct 10, 2018

DeepDiver1975 commented Oct 11, 2018

dgrubelic Oct 8, 2018 •

edited

dgrubelic Oct 8, 2018 •

edited