supabase-community/gotrue-csharp

SignUp Method Documentation on supabase.org [Create a new user](https://supabase.com/docs/reference/csharp/auth-signup) Is Incorrect

hunsra opened this issue · 15 comments

Improve documentation

The supabase.org documentation page for the SignUp method states:

Confirm email determines if users need to confirm their email address after signing up.
-   If Confirm email is enabled, a user is returned but session is null.
-   If Confirm email is disabled, both a user and a session are returned.

When the user confirms their email address, they are redirected to the [SITE_URL](https://supabase.com/docs/reference/auth/config#site_url) by default. You can modify your SITE_URL or add additional redirect URLs in [your project](https://supabase.com/dashboard/project/_/auth/url-configuration).
If SignUp() is called for an existing confirmed user:
-   If Confirm email is enabled in [your project](https://supabase.com/dashboard/project/_/auth/providers), an obfuscated/fake user object is returned.
-   If Confirm email is disabled, the error message, User already registered is returned.

Link

supabase.org page Create a new user

Describe the problem

The actual results of the SignUp method are different than described. It returns a Session object alone, and the behavior when attempting to sign up an existing user is not described accurately.

Describe the improvement

Update ethe documentation to reflect the actual return type and behavior of the method.

Additional context

In my experience, with confirm email enabled, when the method is used to sign up an existing user, it returns a Session object with an empty Identities list. If the user doesn't already exist and the sign up succeeds, it returns a Session object with a non-empty Identities list.

Agreed - it could use a clarification! Would you be willing to do a PR?

@acupofjose, I thought about that, but I don't feel confident enough about the actual behavior to be able to accurately describe it. If you could point me to a reference somewhere that describes it (even if it's not specific to the C# implementation), I'd be happy to take a shot.

The easiest thing to do would probably be to review the test suites and check against the behavior there...

@wiverson, I'll give that a try and see if I can figure out the full behavior.

@acupofjose and @wiverson, looking for the actual repo to fork that contains the docs. I can't seem to find it. I looked at https://github.com/supabase/supabase and in this repo but could not locate the md/mdx content related to auth sign up. Can you refer me to the proper repo?

The repos are a bit confusing. I think this page will help you find things a bit easier.

The languages are easy, just remember that the JS/TS client is the main official client, and the Flutter client code I often find helpful as it will have implementations of things that are mobile specific.

The more confusing bit are the features. Basically, these are client libraries for specific things, for example a lot of the database-as-REST stuff comes from PostgREST, and the auth stuff comes from GoTrue.

So, for example, (eg support for native Sign in with Apple and Google), you could take a look at the Flutter/Dart GoTrue implementation, and then that gets ported/moved over to the gotrue-csharp library.

The first line client library is basically a wrapper to bring all of those subprojects together into a coherent whole. So, you would check out the supabase-csharp library to do something like update the version of the gotrue-csharp library.

Each project has it's own test suite. So to find the tests for the C# auth you would look in the test directory for the gotrue-csharp project.

FWIW the test suite relies on a docker compose setup for integration testing, and that by default automatically verifies emails. So to fully test out all the functionality you might try pointing test cases at a live Supabase test example w/email verification on. Or if you are feeling extra-badass you could add a second docker compose & test setup with a mock email server or something, which so far I don't think anyone has been ambitious enough to tackle. That said GoTrue is pretty stable and I don't think the behavior would change, so I think for now that's been left as as a TBD.

LMK if that makes sense/is enough to go on!

@wiverson, Thanks! This is very helpful for determining what the behavior is so it can be correctly documented. In addition, I need to find the repo that contains the documentation for the C# "Create a new user" markdown page https://supabase.com/docs/reference/csharp/auth-signup which is what I need to ultimately update and make a PR for. I can't seem to find it in the supabase or gotrue-csharp repos. I looked at https://github.com/supabase/supabase/tree/master/apps/docs/docs/ref/csharp, but the authentication stuff isn't there, and the gotrue-csharp repo doesn't seem to have any documentation content.

Ah - I'm following now!

You're looking for this file to update the live Supabase docs:
https://github.com/supabase/supabase/blob/master/apps/docs/spec/supabase_csharp_v0.yml

And this file to update the generated API docs on the gotrue repo itself:

/// <summary>
/// Signs up a user
/// </summary>
/// <remarks>
/// Calling this method will log out the current user session (if any).
///
/// By default, the user needs to verify their email address before logging in. To turn this off, disable confirm email in your project.
/// Confirm email determines if users need to confirm their email address after signing up.
/// - If Confirm email is enabled, a user is returned but session is null.
/// - If Confirm email is disabled, both a user and a session are returned.
/// When the user confirms their email address, they are redirected to the SITE_URL by default. You can modify your SITE_URL or add additional redirect URLs in your project.
/// If signUp() is called for an existing confirmed user:
/// - If Confirm email is enabled in your project, an obfuscated/fake user object is returned.
/// - If Confirm email is disabled, the error message, User already registered is returned.
/// To fetch the currently logged-in user, refer to <see cref="User"/>.
/// </remarks>
/// <param name="type"></param>
/// <param name="identifier"></param>
/// <param name="password"></param>
/// <param name="options">Object containing redirectTo and optional user metadata (data)</param>
/// <returns></returns>
Task<TSession?> SignUp(SignUpType type, string identifier, string password, SignUpOptions? options = null);

@acupofjose, great! Thanks!

@acupofjose and @wiverson, I'm finally getting to working on updating the documentation and I think there may be a bug to address. My investigation has yielded the following description of the SignUp API's behavior (as it would be documented in Yaml):

      - By default, the user needs to verify their email address before logging in. To turn this off, disable **Confirm email** in [your project](https://supabase.com/dashboard/project/_/auth/providers).
      - **Confirm email** determines if users need to confirm their email address after signing up.
        - If **Confirm email** is enabled, the `User` property of the returned `Session` object is populated with the new user.  The `User' object's `ConfirmationSentAt` property will be set to the date and time the confirmation was sent to the new user, and the `ConfirmedAt` property will be set to `null`.
        - If **Confirm email** is disabled, the `User` property of the returned `Session` object is populated with the new user.  Both the `ConfirmationSentAt` and `ConfirmedAt` properties of the `User' object will be set to `null`.
      - When the user confirms their email address, they are redirected to the [`SITE_URL`](https://supabase.com/docs/reference/auth/config#site_url) by default. You can modify your `SITE_URL` or add additional redirect URLs in [your project](https://supabase.com/dashboard/project/_/auth/url-configuration).
      - If SignUp() is called for an existing confirmed user:
        - When both **Confirm email** and **Confirm phone** (even when phone provider is disabled) are enabled in [your project](/dashboard/project/_/auth/providers), the returned `Session` object of populated with the existing user.  The `Identities` `List` property of the `User` object will contain no items, indicating a user was not created.
        - When either **Confirm email** or **Confirm phone** (even when phone provider is disabled) is disabled, a `GotrueExcpetion` is thrown with `Reason` `UserAlreadyRegistered`.

It seems to me that the behavior of the API when attempting to sign up an existing user should be the same no matter what the state of the Confirm email or Confirm phone options are for the associated Supabase project. The result should always be to throw an exception with Reason set to UserAlreadyRegistered. Do you agree, or am I missing something important?

I'm pretty sure this is just matching the GoTrue behavior?

FWIW there's a lot of debate in security circles about the behavior when it comes to registering a user if the account already exists. For many systems even knowing that the user has an account is considered leaking private data (eg if you pop in an email on a dating site and it says user already registered, that's telling you a lot). This might be part of why stuff like emailing a link to login is a good solution. My guess is that this behavior is based on the GoTrue threat modeling.

@wiverson, thanks for the response.

Looking at the Gotrue documentation at: https://github.com/supabase/gotrue?tab=readme-ov-file#post-signup, it seems like both cases should actually return a 'faux' user in the session object, so I'm not sure this could be expected behavior as there is too much overlap in the two situations that are handled differently for that to make sense. The only difference is that if both confirmation methods (email and phone) are enabled (even if not used), a session/user is returned, but if only one confirmation method (email or phone) is enabled, no session/user is returned and an exception is thrown. Doesn't it make sense to return a 'faux' session/user in cases where any kind of confirmation is enabled, or throw an exception of no confirmation is enabled?

I'm happy to make a PR with the documentation as posted in the previous comment, but I can image it might be as confusing for future developers as it currently is.

From a high level, yeah that makes sense.

I think that would be a breaking change. @acupofjose ?

@hunsra thanks for digging deep into this one! Looks like you found a rabbit hole.

The JS client's documentation expects the faux user information for the both case and an exception message for the either case. Since we're trying to mirror the JS client's functionality while adding some C# specific goodies, I vote leaving the functionality in this client as is, unless the JS client takes a different approach.

I will say that personally, I agree, it seems a little strange that the both/either conditions would return different things since they seem to be very close as use cases. If you believe that's something that should change or should be clarified, definitely open an issue on the main Gotrue repo!

Thanks @wiverson and @acupofjose. I'll make a PR with the modified documentation so, if accepted, it will match the current behavior. I'll also add an issue on the Gotrue repo about the behavior to see if the maintainers agree with the strangeness.