Skip to content
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

Full Native Auth support #399

Closed
5 tasks done
DanMossa opened this issue Mar 13, 2023 · 40 comments
Closed
5 tasks done

Full Native Auth support #399

DanMossa opened this issue Mar 13, 2023 · 40 comments
Labels
auth This issue or pull request is related to authentication enhancement New feature or request

Comments

@DanMossa
Copy link
Collaborator

DanMossa commented Mar 13, 2023

The criteria for having a successful native authentication process is the ability to do the following:

  • Sign in on Google working on Android, iOS, and Web
  • Sign in with Apple working on Android, iOS, and Web
    • Sign in with Apple with multiple bundle IDs
  • When creating an account with social auth, you can then link your email with it.
  • When creating an account with email first, you can then link your social auth to it.

Sign in on Google working on Android, iOS, and Web

When using sign in with Google, you're currently limited to having sign in with Google on either Android and Web, or iOS. The reason for this is because on GCP, you create credentials and you choose the type of device that the OAuth client is. The web application credentials work for both Android and Web where as for iOS, you're required to use the iOS credentials. Supabase only has a slot for a single client ID/Secret.

This PR is also required for us to pass in a Nonce when using iOS
google/GoogleSignIn-iOS#244

Sign in with Apple working on Android, iOS, and Web

I was able to get iOS working on iPhone, but I'm having trouble getting Sign in with Apple working on the web as well. These are the instructions I'm following but I'll update this when I / others have better luck setting this up
https://pub.dev/packages/sign_in_with_apple

When creating an account with social auth, you can then link your email with it.

When creating an account with Google the user's email address is registered inside the auth.users table. A user can then link their accounts by following the forgot password route with the same email.

When creating an account with Apple, depending on the user's preferences, either the plain text email or the private email is registered inside auth.users. A user can then link their accounts by following the forgot password route with the same email. This is only possible if the user does not use Apples's private email, but Apple users are aware of this.

When creating an account with email first, you can then link your social auth to it.

This is not possible at the moment. We can an error back stating there's a duplicate of the email already. I've read that toggling isSSO fixes this but I haven't tested it yet.

@DanMossa DanMossa added bug Something isn't working enhancement New feature or request auth This issue or pull request is related to authentication labels Mar 13, 2023
@DanMossa DanMossa mentioned this issue Mar 13, 2023
@DanMossa DanMossa removed the bug Something isn't working label Mar 13, 2023
@dshukertjr
Copy link
Member

Mentioned it here as well, but auth team will be working on a solution to fix the Google login platform problem.

We will probably add a clientId parameter on the OIDC endpoint so that when signing in with Android or iOS we can pass it as a parameter.

@gregorym

This comment was marked as resolved.

@dshukertjr

This comment was marked as resolved.

@medclans

This comment was marked as off-topic.

@elliottetzkorn

This comment was marked as off-topic.

@dshukertjr

This comment was marked as off-topic.

@elliottetzkorn

This comment was marked as off-topic.

@gregorym

This comment was marked as off-topic.

@dshukertjr

This comment was marked as off-topic.

@elliottetzkorn

This comment was marked as off-topic.

@dshukertjr

This comment was marked as off-topic.

@mohsin2596

This comment was marked as off-topic.

@dshukertjr

This comment was marked as off-topic.

@elliottetzkorn
Copy link
Contributor

Google folks have responded to the nonce PR, which is very encouraging! google/GoogleSignIn-iOS#244 (review)

@elliottetzkorn

This comment was marked as off-topic.

@dshukertjr

This comment was marked as off-topic.

@dshukertjr

This comment was marked as off-topic.

@elliottetzkorn
Copy link
Contributor

@elliottetzkorn Apple login on Android is only supported through the web-based login method. You can follow this guide to set it up, and call it like this:

await supabase.auth.signInWithOAuth(Provider.apple);

Thank you!

@elliottetzkorn

This comment was marked as off-topic.

@supabase supabase locked as off-topic and limited conversation to collaborators Jun 23, 2023
@dshukertjr
Copy link
Member

I have locked the conversation here to make it a public discussion board instead of a Q&A issue. If you have any question/ comments about native sign in, please post them here #5.

@dshukertjr
Copy link
Member

dshukertjr commented Jun 23, 2023

@bdlukaa @DanMossa @Vinzent03 I have a few things that I would like to discuss and love to hear your opinions on.

First of all, a good news 🎉 Supporting multiple Client IDs and multiple bundle IDs is finally landing Supabase, and an official announcement is scheduled to go out Friday morning PST. This means that users can finally add Google login for web, Android, and iOS or add Apple logins on multiple iOS apps. The update to allow users to be able to enter multiple client IDs or bundle IDs as comma separated values in the dashboard is being rolled out soon. With this, we should be able to tick all the check boxes on this issue.

Now with this, I want to discuss how to go about implementing native auth methods on supabase_flutter.

Currently, we have signInWithApple() method, that I personally regret implementing for two reasons

  • Adds extra dependencies
  • We would have to create signInWithXXX methods for every native auth providers that we start supporting

The whole reason why I thought adding signInWithApple() is because the process of signing a user in via signInWithIdToken requires generating a nonce and hashing it, which is not so relevant to the developer trying to implement a simple login button. Luckily, signInWithApple() has an experimental flag, so we have a bit of freedom to edit/ remove it.

Native Apple login code 👇
import 'package:crypto/crypto.dart';
import 'package:sign_in_with_apple/sign_in_with_apple.dart' as apple;

final rawNonce = _generateRandomString();
final hashedNonce = sha256.convert(utf8.encode(rawNonce)).toString();

final credential = await apple.SignInWithApple.getAppleIDCredential(
  scopes: [
    apple.AppleIDAuthorizationScopes.email,
    apple.AppleIDAuthorizationScopes.fullName,
  ],
  nonce: hashedNonce,
);

final idToken = credential.identityToken;
if (idToken == null) {
  throw AuthException('Could not find ID Token from generated credential.');
}

await signInWithIdToken(
  provider: Provider.apple,
  idToken: idToken,
  nonce: rawNonce,
);

However, implementing all native login methods is not very realistic, and I wonder what we should do

I think we have a few choices

  1. Keep signInWithApple(), but do not add any other native methods, and add detailed documentations on README.md and supabase.com for them.
  2. Remove signInWithApple() and add detailed documentations on README.md and supabase.com for all the auth providers.
  3. Create a supabase_native_sign_in_helper library (we can think of a better name), which contains AppAuth, Apple sign in, and any future dependencies that we might need, and provide signInWithXXX methods there that takes care of nonce generations and stuff.

I personally want to go with option 2 here, but the steps to implement Google login involves quite a bit of boiler plate code. Because google_sign_in library does not allow users to specify nonce, we have to use another method, and the method I was able to find was using this one using flutter_appauth.

Native Google login code 👇
import 'package:crypto/crypto.dart';
import 'package:flutter_appauth/flutter_appauth.dart';

Future<AuthResponse> signInWithGoogle(String clientId) {
  // Just a random string
  final rawNonce = _generateRandomString();
  final hashedNonce =
      sha256.convert(utf8.encode(rawNonce)).toString();
    
  /// bundle ID of the app
  const bundleId = 'com.supabase.example';
  
  /// fixed for google login
  const redirectUrl = '$bundleId:/google_auth';
  
  /// fixed for google login
  const discoveryUrl =
      'https://accounts.google.com/.well-known/openid-configuration';
  
  // authorize the user by opening the concent page
  final result = await appAuth.authorize(
    AuthorizationRequest(
      clientId,
      redirectUrl,
      discoveryUrl: discoveryUrl,
      nonce: hashedNonce,
      scopes: [
        'openid',
        'email',
      ],
    ),
  );
  
  if (result == null) {
    throw 'No result';
  }
  
  // Request the access and id token to google
  final tokenResult = await appAuth.token(
    TokenRequest(
      clientId,
      redirectUrl,
      authorizationCode: result.authorizationCode,
      discoveryUrl: discoveryUrl,
      codeVerifier: result.codeVerifier,
      nonce: result.nonce,
      scopes: [
        'openid',
        'email',
      ],
    ),
  );
  
  final idToken = tokenResult?.idToken;
  
  if (idToken == null) {
    throw 'No idToken';
  }
  
  return supabase.auth.signInWithIdToken(
    provider: Provider.google,
    idToken: idToken,
    nonce: rawNonce,
  );
}

Do you think as long as we have detailed instruction on how to implement it, it would be okay to have some long boiler plate code, or should we provide something that makes it easier to implement it?

@DanMossa
Copy link
Collaborator Author

@dshukertjr

I'm a big fan of setting up a simple method with documentation on how to use Apple/Google/etc logins.

Like I think we should just have a basic signInWithIdToken with documentation explaining the different fields with examples of how to use Google and Apple.

@dshukertjr
Copy link
Member

@DanMossa So that would be option 2, correct? Right there with you!

While we wait for Bruno and Vinzent to drop what they think, I will prepare a PR to update the README.md to include detailed instructions on how to setup Apple login and Google login with signInWithIdToken method.

@Vinzent03
Copy link
Collaborator

I kinda like to keep the signInWithApple() method, because if we document on how to implement that with signInWithIdToken and it's just copy pasting from the website, I think we can just keep it. The same goes for google and other providers, right? If we document what the dev has to copy paste, it's not a great dx. But I understand the dependency issue, so I would go with the third option. I think that makes the best experience for the dev. If we already document on how to use native auth for these providers, we can provide a ready to go package as well.

@dshukertjr
Copy link
Member

@Vinzent03 Nice! Yeah, it is some unnecessary long code for every developer to copy and paste every single time. Option 3 does take the best of both world off loading the dependency issue and providing better developer experience!

Let's also wait for @bdlukaa to hear what he thinks!

@bdlukaa
Copy link
Collaborator

bdlukaa commented Jun 23, 2023

From a maintainer perspective, keeping the signInWithApple is a dead-hell. Depending on third-party libraries is not an easy task, but it is feasible. From a dev perspective, I would LOVE to have such method built-in, with no extra code.

But keeping it assembles the question: do we need to create a sign in method for every supported third-party authentication method? Keeping the method would also imply in large bundle size on apps that don't make usage of social login.

With that said, I like the second option tho. It should be up to the dev to handle these stuff (like scopes), I believe. There is already parts that the dev will need to do regardless of the solution we adopt, such as configuring intents on Android, so adding a dependency surely will not be an issue.

Some methods, such as generating the nonce, could be built-in tho.

@dshukertjr
Copy link
Member

Thanks everyone! It's so nice to discuss this with all of you.

Why don't we do this. Let's start out with option 2, where we remove the signInWithApple() method, and add an instruction in the docs + README.md. Let's then see how the community reacts to it and if there is a strong push for an simpler way to implement things, we can consider option 3 to create a separate library. This buys us some time to think about how to go about implementing the library, and maybe we can come up with a better way to do it too!

@DanMossa
Copy link
Collaborator Author

@dshukertjr We can also do something where signInWithIdToken can take in a dependency and depending on what that dependency is we can setup auth for the user.

Example:
A user do this

import 'package:sign_in_with_apple/sign_in_with_apple.dart' as apple;

await signInWithIdToken(
  provider: Provider.apple,
  idToken: idToken,
  nonce: rawNonce
  dependency: apple,
);

signInWithIdToken

import 'package:crypto/crypto.dart';

signInWithIdToken(provider, idToken, nonce, dependency){
  final rawNonce = _generateRandomString();
  final hashedNonce = sha256.convert(utf8.encode(rawNonce)).toString();
  
  final credential = await apple.SignInWithApple.getAppleIDCredential(
    scopes: [
      apple.AppleIDAuthorizationScopes.email,
      apple.AppleIDAuthorizationScopes.fullName,
    ],
    nonce: hashedNonce,
  );
  
  final idToken = credential.identityToken;
  if (idToken == null) {
    throw AuthException('Could not find ID Token from generated credential.');
  }

return idToken;
}

I kind of hate this though and would rather just have Option 2.

@dshukertjr
Copy link
Member

@DanMossa Thanks for the suggestion. In that case would the dependency parameter be a type dynamic? If we do that, is there a way for supabase_flutter to tell what type dependency is?

@bdlukaa
Copy link
Collaborator

bdlukaa commented Jun 24, 2023

Well, for sign_in_with_apple package, there is sign_in_with_apple_platform_interface, which doesn't add any extra platform code. This way we would be able to have the dev to add the sign_in_with_apple, if wanted, and keep the helper method. The same implies for google with google_sign_in_platform_interface

@bdlukaa
Copy link
Collaborator

bdlukaa commented Jun 24, 2023

If we do that, is there a way for supabase_flutter to tell what type dependency is?

Currently in dart these is no union types (see dart-lang/language#83), but it is possible to make it dynamic and check if the type matches the provider type.

@dshukertjr
Copy link
Member

@bdlukaa

Currently in dart these is no union types (see dart-lang/language#83), but it is possible to make it dynamic and check if the type matches the provider type.

But in order to check the type, we would do something like dependency is AppleSignIn right? In order to do that, we would need the AppleSignIn class in the code, meaning we would have to add apple_sign_in as the dependencies, don't we?

@Vinzent03
Copy link
Collaborator

I think you are right, because the sign_in_with_apple_platform_interface does only provide the class you get by calling getAppleIDCredential so the platform interface dependency wouldn't work.

@bdlukaa
Copy link
Collaborator

bdlukaa commented Jun 26, 2023

It'd actually look something like the following:

just a POC tho

// the implementation
import 'package:crypto/crypto.dart';

String getAppleNonce() {
  final rawNonce = _generateRandomString();
  final hashedNonce = sha256.convert(utf8.encode(rawNonce)).toString();
  
  return hashedNonce;
}

// idToken optional maybe?
void signInWithIdToken(provider, idToken, dynamic credential){
  if (credential is AuthorizationCredentialAppleID) {
    final idToken = credential.identityToken;
    if (idToken == null) {
      throw AuthException('Could not find ID Token from generated credential.');
    }
  } else if (credential is GoogleSignInAccount) {
    // impl with the provided info
  } 

  return idToken;
}
// the usage

// apple
await signInWithIdToken(
  provider: Provider.apple,
  idToken: idToken,
  dependency: await apple.SignInWithApple.getAppleIDCredential(
    scopes: [
      apple.AppleIDAuthorizationScopes.email,
      apple.AppleIDAuthorizationScopes.fullName,
    ],
    nonce: getAppleNonce(),
  ),
);

// google
await signInWithIdToken(
  provider: Provider.google,
  idToken: idToken,
  dependency: await GoogleSignIn().signIn(),
);

The platform interface for these would work because we only need the data from them. The one responsible for calling the actual platform method (and adding the dependency) is the developer.

@dshukertjr
Copy link
Member

@bdlukaa
In order to have credential is AuthorizationCredentialAppleID through, we would need to add apple_sign_in as the dependencies of supabase_flutter, which defeats the original purpose of getting rid of dependencies.

@bdlukaa
Copy link
Collaborator

bdlukaa commented Jun 27, 2023

Not really. AuthorizationCredentialAppleID comes from sign_in_with_apple_platform_interface, which itself doesn't add any native implementation

See https://pub.dev/documentation/sign_in_with_apple_platform_interface/latest/authorization_credential/AuthorizationCredentialAppleID-class.html

This would make the sign in with apple feature optional to the developer

@dshukertjr
Copy link
Member

@bdlukaa
I see. Yeah, sounds like an idea that is worth exploring!

@Vinzent03
Copy link
Collaborator

To be honest, I would wait with the removal of the signInWithApple method until we have a solution. I find the raw removal and replacement with documentation on how to do it yourself until we are done discussing strange.

I don't think the semi integration proposed here is future-proof, because that way may not always work in the future or for every other provider. Maybe other providers don't even provide a platform interface package. I would rather make it completely inclusive, like creating a new package that holds these native helpers (solution 3), or doing only documentation (solution 2).

@dshukertjr
Copy link
Member

Yeah, we are in a weird position especially for Google login, because the official Google login package isn't a viable option for us, and we have to go with flutter_appauth package. There are a lot of moving parts here. I said it might be worth exploring, because we are just brain storming possible ideas here and I wanted here to be a safe place to talk about whatever ideas that come to our minds.

With the current situations though, the proposal of adding a dependency parameter seems a bit risky as @Vinzent03 has said, and might not be an direction that I would pursue personally. However, anyone is welcome to explore the possibilities, and maybe someone can come up with a cleaver and safe implementation down the line.

For the time being though, I've proposed to go with option 2 for now, and see how the community reacts and if there seems to be a huge demand for a simpler way of implementing native auth, we can consider option 3 here, but do we have a strong objection against this route?

Also maybe instead of getting rid of signInWithApple() immediately, we can mark it as deprecated and leave it for a while.

@dshukertjr
Copy link
Member

dshukertjr commented Jul 3, 2023

I am going to close this issue as all the tasks on this issue is complete.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auth This issue or pull request is related to authentication enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants