fix: Google authorization fix in dev-demo #1437
Conversation
|
@bmulholland This is the fix for google auth in demo-app, related to #1393 |
Intevel
changed the title
semantic shit
I have added Spotify auth with the oauth2 scheme. Also added it directly to the demo, and tested everything.
Added the spotify oauth provider to the docs and changed positions
Intevel
changed the title
|
I added the Spotify auth provider to this in the oauth2 schema, he put that in this PR for whatever reason. |
Intevel
changed the title
|
Please separate spotify into a separate PR -- having two things together makes it hard for me to review. You'll need to create a separate branch & PR; anything on your dev branch is going to update this PR here. |
| @@ -12,6 +12,7 @@ export function google( | |||
| ): void { | |||
| const DEFAULTS: typeof strategy = { | |||
| scheme: 'oauth2', | |||
| codeChallengeMethod: '', | |||
There was a problem hiding this comment.
S256 seems to be what Google prefers: https://developers.google.com/identity/protocols/oauth2/native-app
There was a problem hiding this comment.
But it doesn't work.
There was a problem hiding this comment.
What do you mean? Why?
There was a problem hiding this comment.
S256 doesn't work properly in the context, why I don't know. There is a lot on stsckoverflow about this and the only way to solve this is to leave it empty.
There was a problem hiding this comment.
I just checked the app I work on, and we have Google's S256 deployed in production. So it does work, and since Google recommends it, I think S256 should be the default for the google provider.
If you can't get the demo site working with S256 right now, you could turn off code challenge specifically in the demo/nuxt.config. Please try to get "plain" working instead. Ideally the demo config would only turn off code challenge when run locally, so that the code challenge method is still testable on the deployed demo site.
Then we can merge this in, but it's still not ideal: we should get S256 working and testable locally. This may need a secret key set, and maybe that's why you couldn't get it working? In any case, fixing S256 can be filed as a followup issue (please create that on Github) once this change is merged in.
|
And thanks again for the help -- really appreciated! |
|
Thanks for separating Spotify into a separate PR. Could you please also cut this one down to just the Google auth fix? Then I can review only those changes. |
Intevel
changed the title
|
I moved spotify to #1483 |
|
Thank you! |
| @@ -10,3 +10,4 @@ dist | |||
| package-lock.json | |||
| _book | |||
| temp | |||
| demo/.env | |||
There was a problem hiding this comment.
Can you please fix the newline ending for this file?
| @@ -259,7 +259,7 @@ export class Oauth2Scheme< | |||
| // Set Nonce Value if response_type contains id_token to mitigate Replay Attacks | |||
| // More Info: https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes | |||
| // More Info: https://tools.ietf.org/html/draft-ietf-oauth-v2-threatmodel-06#section-4.6.2 | |||
| if (opts.response_type.includes('token')) { | |||
| if (opts.response_type.includes('id_token')) { | |||
There was a problem hiding this comment.
Are you sure about this change? How do we know it won't break anyone else's setup?
There was a problem hiding this comment.
What about lowering the risk by checking for either value?
There was a problem hiding this comment.
I'll have a look at it later, I'll try to fix the checks of PR #1483 first. 😂
There was a problem hiding this comment.
Sounds good -- let me know if you get stuck. Thanks again for all your help!
There was a problem hiding this comment.
I've split this out into its own PR: #1532
This PR belongs to this Issue #1435