supabase / gotrue-dart

A dart client library for GoTrue.
MIT License
46 stars 37 forks source link

Make nonce optional #68

Closed DanMossa closed 2 years ago

DanMossa commented 2 years ago

Dependent on https://github.com/supabase/gotrue/pull/430

DanMossa commented 2 years ago

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

dshukertjr commented 2 years ago

@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 commented 2 years ago

@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

dshukertjr commented 2 years ago

@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 commented 2 years ago

@dshukertjr Officially got https://github.com/supabase/gotrue/pull/430 merged in.

dshukertjr commented 2 years ago

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

DanMossa commented 2 years ago

@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.

k0shk0sh commented 2 years ago

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

k0shk0sh commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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

k0shk0sh commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/supabase/gotrue/issues/412#issuecomment-1114192277

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

DanMossa commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

DanMossa commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

https://github.com/google/GoogleSignIn-iOS/issues/135

This is where the issue is.

@k0shk0sh @dshukertjr