Skip to content
This repository has been archived by the owner on May 13, 2023. It is now read-only.

Make nonce optional #68

Merged
merged 3 commits into from
May 9, 2022
Merged

Make nonce optional #68

merged 3 commits into from
May 9, 2022

Conversation

DanMossa
Copy link
Contributor

Dependent on
supabase/auth#430

@DanMossa
Copy link
Contributor Author

I'm not sure why the tests are failing. I can't get them to work on main either.

@dshukertjr
Copy link
Member

@DanMossa

Hmm, not sure why the tests are failing either, but were you able to test this within an app to see if google signin actually works? If not, I can try to test it out when I can!

@DanMossa
Copy link
Contributor Author

@dshukertjr I'll actually set this to draft until the gotrue PR gets merged in, but yeah it should work.

Also, do you happen to know what kind of ETA to expect for the gotrue review? I'm assuming it's a bit longer since it's a more used package

@DanMossa DanMossa marked this pull request as draft March 30, 2022 05:01
@dshukertjr
Copy link
Member

@DanMossa Yeah, there is a lot going on in gotrue repo as it's really high in demand, so it might take some time, but I can see that kangming has some response!

@DanMossa DanMossa marked this pull request as ready for review April 15, 2022 23:54
@DanMossa
Copy link
Contributor Author

DanMossa commented Apr 15, 2022

@dshukertjr Officially got
supabase/auth#430
merged in.

@dshukertjr
Copy link
Member

@DanMossa Amazing! Did you have any chance to try out native Google login with this?

@DanMossa
Copy link
Contributor Author

@dshukertjr I actually haven't just yet. I wasn't sure how long it takes for the gotrue PR merges to reflect on hosted Supabase. I think that regardless nonce should be optional but yeah it makes sense to wait before we merge this in just in case something else needs to be changed.

I'll set it back to draft until I verify.

@DanMossa DanMossa marked this pull request as draft April 17, 2022 04:16
@k0shk0sh
Copy link

k0shk0sh commented May 1, 2022

I can verify, google signIn works with setting nonce as empty string.

@k0shk0sh
Copy link

k0shk0sh commented May 1, 2022

edit: unfortunately, it doesn't work in iOS because the google_signIn return a nonce in idToken which confuses the backend.

having decoded the jwt on client and sending that nonce doesn't work either.

@dshukertjr
Copy link
Member

@k0shk0sh Thank you so much for trying it out on various devices!

I am so sorry, but I haven't had a chance to take a deep look into this PR just yet. Does seem like there needs to be more rework on the backend then?

@DanMossa
Copy link
Contributor Author

DanMossa commented May 2, 2022

@k0shk0sh @dshukertjr
So the PR is definitely working for fixing the sign in with google issue.
But there now needs to be some frontend work to decode the JWT to determine whether or not a nonce is in the JWT.
Or you can just do an if then for if it's apple or google

@DanMossa DanMossa marked this pull request as ready for review May 2, 2022 05:05
@k0shk0sh
Copy link

k0shk0sh commented May 2, 2022

@k0shk0sh @dshukertjr
So the PR is definitely working for fixing the sign in with google issue.
But there now needs to be some frontend work to decode the JWT to determine whether or not a nonce is in the JWT.
Or you can just do an if then for if it's apple or google

I did that, I send the nonce with the api call but can't return invalid nonce.

@DanMossa
Copy link
Contributor Author

DanMossa commented May 2, 2022

Can you send me the code you're using?

And just to clarify this works with Google but not with iOS. And what is the exact error you're getting?

@k0shk0sh
Copy link

k0shk0sh commented May 2, 2022

using jwt_decode: ^0.3.1 lib:

      Map<String, dynamic> payload = Jwt.parseJwt(googleAuth.idToken!);
      final nonce = payload['nonce'] ?? '';
      final result = await _supabase.client.auth.signIn(
        oidc: OpenIDConnectCredentials(
          nonce: nonce,
          idToken: googleAuth.idToken!,
          provider: Provider.google,
        ),
      );

on iOS the nonce is returned and then passed to oidc.

once API call is made then I receive: supabase/auth#412 (comment)

going deeper into this, google SignIn on iOS returns a nonce unfortunately but we can't access this nonce to verify it on backend.
I receive this: Passed nonce and nonce in id_token should either both exist or not.
I tried passing the nonce from the jwt before sending the request but I got this msg:
invalid nonce

@k0shk0sh
Copy link

k0shk0sh commented May 2, 2022

I believe, we should also remove the nonce check against the jwt and also not passing one from clients. then I reckon this will solve it. nonce is always optional even on apple signIn.

@anggoran
Copy link

anggoran commented May 2, 2022

using jwt_decode: ^0.3.1 lib:

      Map<String, dynamic> payload = Jwt.parseJwt(googleAuth.idToken!);
      final nonce = payload['nonce'] ?? '';
      final result = await _supabase.client.auth.signIn(
        oidc: OpenIDConnectCredentials(
          nonce: nonce,
          idToken: googleAuth.idToken!,
          provider: Provider.google,
        ),
      );

on iOS the nonce is returned and then passed to oidc.

once API call is made then I receive: supabase/gotrue#412 (comment)

going deeper into this, google SignIn on iOS returns a nonce unfortunately but we can't access this nonce to verify it on backend.
I receive this: Passed nonce and nonce in id_token should either both exist or not.
I tried passing the nonce from the jwt before sending the request but I got this msg:
invalid nonce

Perhaps empty string is not equal to null, so the backend still check if the empty string is a valid sha256 nonce, also the google_sign_in package doesn't come with nonce setting

@DanMossa
Copy link
Contributor Author

DanMossa commented May 2, 2022

Oh that's exactly the issue. You're passing in an empty string.
I'm making PRs to turn nonce into an optional parameter.

I also know that there's something needed with hashed nonce. I'm not near my computer so I can't tell you definitively

@DanMossa
Copy link
Contributor Author

DanMossa commented May 4, 2022

Testing the official Sign In With Google package.
This is the code I'm using:

    GoogleSignIn _googleSignIn = GoogleSignIn(
      clientId: constants.WEB_GOOGLE_CLIENT_ID,
    );

    GoogleSignInAccount? account = await _googleSignIn.signIn();
    GoogleSignInAuthentication? auth;
    if (account != null) {
      auth = await account.authentication;
    }

    if (auth?.idToken != null) {
      return AuthService.signInWIthOpenID(
          idToken: auth!.idToken!, provider: Provider.google);
    }

On Android: I see that idToken has no nonce. I then pass just idToken and provider and I get a valid accessToken from Supabase.
On iPhone: I see that idToken does have a nonce.

I'm very confused as to where this idToken is coming from. I'm looking through the Sign in with google codebase.

Also, is there a way to set multiple google client id's and secrets? Don't we need to per device?

@bdlukaa
Copy link
Contributor

bdlukaa commented May 4, 2022

@DanMossa what if you make use of auth.serverAuthCode as the nonce? Maybe it works

@DanMossa
Copy link
Contributor Author

DanMossa commented May 4, 2022

@DanMossa what if you make use of auth.serverAuthCode as the nonce? Maybe it works

It's hard to know since I can't even get past the audience verification. How can I set multiple Google client IDs and secrets?

@k0shk0sh
Copy link

k0shk0sh commented May 4, 2022

@DanMossa what if you make use of auth.serverAuthCode as the nonce? Maybe it works

It's hard to know since I can't even get past the audience verification. How can I set multiple Google client IDs and secrets?

Unfortunately that's not possible on supabase, I believe you coming from a firebase project just like me. I haven't figured this part yet tbh, im hitting a roadblock everytime, for now im just gonna stick to an email login/signup which can be very cumbersome.

@k0shk0sh
Copy link

k0shk0sh commented May 4, 2022

@DanMossa what if you make use of auth.serverAuthCode as the nonce? Maybe it works

This wont work, could you elaborate on why you think this will work?

@DanMossa
Copy link
Contributor Author

DanMossa commented May 4, 2022

I think we should merge this PR in because it's correct.
And at the same time we should open a new discussion thread or move this Convo to an existing one.
@dshukertjr @k0shk0sh

@DanMossa
Copy link
Contributor Author

DanMossa commented May 5, 2022

@k0shk0sh
It looks like the backend needs to be the one that validated the audience. Supabase should take a list of client IDs?

https://stackoverflow.com/questions/40806756/google-signin-in-android-and-ios-with-backend-server

https://stackoverflow.com/questions/54021631/google-sign-in-in-ios-with-server

@k0shk0sh
Copy link

k0shk0sh commented May 6, 2022

@k0shk0sh
It looks like the backend needs to be the one that validated the audience. Supabase should take a list of client IDs?

https://stackoverflow.com/questions/40806756/google-signin-in-android-and-ios-with-backend-server

https://stackoverflow.com/questions/54021631/google-sign-in-in-ios-with-server

Yeeep, but since supabase is web focused, they never catered for Mobile I would assume.

@DanMossa
Copy link
Contributor Author

DanMossa commented May 8, 2022

Alrighty.

So next in this rabbit hole is to add support for multiple client IDs lmao.

@dshukertjr Do you wanna go ahead and merge this PR in and I guess I'll eventually start working on that?

@dshukertjr
Copy link
Member

Thanks and sorry for taking time to jump onto this one @DanMossa . I will take a look at it.

Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

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

Thank you @DanMossa for working on bringing native OAuth to supabase-flutter. Thank you @k0shk0sh for hopping on discussions and doing bunch of researches.

It sounds like what we need at this point to fully support all web, Android, and iOS native sign in is to be able to set multiple client IDs on the supabase dashboard and properly handle each client id on the gotrue backend, correct?

Screen Shot 2022-05-09 at 13 31 36

@dshukertjr dshukertjr merged commit e128612 into supabase:main May 9, 2022
@DanMossa
Copy link
Contributor Author

DanMossa commented May 9, 2022

@dshukertjr

Thanks for looking!

At this point sign in with Google works for Android and Web 100%.

But for iOS, the next steps are being able to use multiple client IDs and then figuring out why we're getting a nonce back in iOS.

@k0shk0sh
Copy link

@dshukertjr

Thanks for looking!

At this point sign in with Google works for Android and Web 100%.

But for iOS, the next steps are being able to use multiple client IDs and then figuring out why we're getting a nonce back in iOS.

I believe we getting back nonce on iOS is because its not supported natively, as you can see in iOS google signIn opens actually a web screen, I believe that's the reason.

The nonce is optional and can be ignored completely as how firebase does it.

@DanMossa
Copy link
Contributor Author

I don't really like the idea of ignoring it. The nonce is important and the spec requires you to use the nonce if available to verify the integrity of the idtoken.

I'll dig around in the near future to see if we can generate a nonce to use for iOS

@k0shk0sh
Copy link

I don't really like the idea of ignoring it. The nonce is important and the spec requires you to use the nonce if available to verify the integrity of the idtoken.

I'll dig around in the near future to see if we can generate a nonce to use for iOS

Using existing google signIn in flutter on iOS doesn't provide the ability to supply the nonce.

@DanMossa
Copy link
Contributor Author

DanMossa commented May 10, 2022

@k0shk0sh

Yeah that's true. But neither was passing in a nonce when using Android but I was able to fork their repo and add it in.

I'm going to look to see if I can add in a way to pass in the nonce for iOS and then make a PR for them.

Then we can use my branch until they merge it in.

Who would've thought Google had such garbage code lol

@DanMossa
Copy link
Contributor Author

google/GoogleSignIn-iOS#135

This is where the issue is.

@k0shk0sh @dshukertjr

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants