Skip to content

fix: Google authorization fix in dev-demo #1437

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

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,4 @@ dist
package-lock.json
_book
temp
demo/.env
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix the newline ending for this file?

2 changes: 0 additions & 2 deletions docs/content/en/providers/laravel-jwt.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ position: 36
category: Providers
---


[Source Code](https://github.com/nuxt-community/auth-module/blob/dev/src/providers/laravel-jwt.ts)

This provider is for the [Laravel JWT](https://github.com/tymondesigns/jwt-auth). Please make sure to follow our guide on the route configuration on this page to avoid issues on the authentication.
Expand Down Expand Up @@ -114,7 +113,6 @@ Our provider will manage the refresh automatically based on the `token` life.

The default `token` lifetime is `1 hour` and the `refreshToken` is `2 weeks` based on the config. Make sure that your Laravel JWT config matches our Auth Nuxt Laravel JWT config as shown below:


```js
auth: {
strategies: {
Expand Down
2 changes: 2 additions & 0 deletions docs/content/en/providers/laravel-passport.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ this.$auth.loginWith('laravelPassport')
These options are **REQUIRED**. The `url` is the location of your Laravel application. To obtain the `client_id` and `client_secret`, create a new client app in your [Laravel app](https://laravel.com/docs/passport#managing-clients).

### User endpoint

`userInfo` endpoint is used to make requests using axios to fetch user data.

### Token Lifetimes

By default, Passport issues long-lived access tokens that expire after one year. If you change their lifetime, don't forget to update [token max age](../schemes/oauth2#token-2) and [refresh token max age](../schemes/oauth2#refreshtoken).
1 change: 0 additions & 1 deletion docs/content/en/providers/laravel-sanctum.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,3 @@ this.$auth.loginWith('laravelSanctum', {
```

💁 This provider is based on [cookie scheme](../schemes/cookie) and supports all scheme options.

1 change: 1 addition & 0 deletions src/providers/google.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export function google(
): void {
const DEFAULTS: typeof strategy = {
scheme: 'oauth2',
codeChallengeMethod: '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it doesn't work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endpoints: {
authorization: 'https://accounts.google.com/o/oauth2/auth',
userInfo: 'https://www.googleapis.com/oauth2/v3/userinfo'
Expand Down
4 changes: 2 additions & 2 deletions src/schemes/oauth2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export interface Oauth2SchemeOptions
clientId: string | number
scope: string | string[]
state: string
codeChallengeMethod: 'implicit' | 'S256' | 'plain'
codeChallengeMethod: 'implicit' | 'S256' | 'plain' | ''
acrValues: string
audience: string
autoLogout: boolean
Expand Down Expand Up @@ -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')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about this change? How do we know it won't break anyone else's setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't be 100% sure of course, I have seen some issues describing PR #709 was broken, after changing to id_token it worked for me. On Stackoverflow was also something about it, and in Issue #970 as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about lowering the risk by checking for either value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at it later, I'll try to fix the checks of PR #1483 first. 😂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good -- let me know if you get stuck. Thanks again for all your help!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've split this out into its own PR: #1532

opts.nonce = _opts.nonce || randomString(10)
}

Expand Down