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

fix: add create_user field to otp endpoint #318

Merged
merged 6 commits into from
Jan 19, 2022

Conversation

kangmingtay
Copy link
Member

@kangmingtay kangmingtay commented Dec 28, 2021

What kind of change does this PR introduce?

What is the new behavior?

# POST /otp
{
  "phone": "12456789",
  "create_user" true
}

or 

{
  "email": "newUser@email.com",
  "create_user" true
}

Both will result in a bad request with a message of "Signups not allowed for otp"

Todo

  • write test

@kangmingtay
Copy link
Member Author

@dthyresson pointed out that it's possible for the no_signup flag to be exploited on the client-side so maybe we should set this as a config rather than in the request

@dthyresson
Copy link

dthyresson commented Dec 29, 2021

@dthyresson pointed out that it's possible for the no_signup flag to be exploited on the client-side so maybe we should set this as a config rather than in the request

Or @kangmingtay perhaps instead of a GoTrue config, this rule could be part of Allow/deny user rules.

For examlple:

  • allow users to signup if their email matches a domain (that is, people from “my company”) and logins for confirmed emails only
  • Allow only if the user account already exists

So instead of a if disable sign ups or a config on magic link — include a rule where there is a method and a rule. For magic link on sign up check if user exists.

It is still configuration but less of an envar boolean and more rule based.

Perhaps too involved in near term and a setting will suffice.

@kangmingtay kangmingtay changed the title [WIP] fix: add no_signup field to otp endpoint [WIP] fix: add create_user field to otp endpoint Dec 31, 2021
@dthyresson
Copy link

I'm still uncomfortable with this being completely a client-side controlled flag

flag to be exploited on the client-side so maybe we should set this as a config rather than in the request

Since the /otp endpoint is accessible via the GoTrue client and an anon key:

See: https://github.com/supabase/gotrue-js/blob/d7b334a4283027c65814aa81715ffead262f0bfa/src/GoTrueApi.ts#L236

/**
   * Sends a mobile OTP via SMS. Will register the account if it doesn't already exist
   * @param phone The user's phone number WITH international prefix
   */
  async sendMobileOTP(phone: string): Promise<{ data: {} | null; error: ApiError | null }> {
    try {
      const headers = { ...this.headers }
      const data = await post(this.fetch, `${this.url}/otp`, { phone }, { headers })
      return { data, error: null }
    } catch (e) {
      return { data: null, error: e as ApiError }
    }
  }

Then here the api changes to pass a boolean to create the user or not -- but there's nothing really enforcing the "rule" but rather it is an option.

I guess the question is: is this a rule that enforces users cannot be created for OTP signups or rather this is an option available when signing up?

If the user is not to be created, ma then perhaps signups are just disabled instead? And then admins create accounts and then users simply sign in?

I'd be concerned that if the rulers around this change, then the api has breaking changes rather than enforcing new rules about how users get created, sign in/up and the process.

This seems to be a stop-gap solution for a bigger question: how to whitelist user profiles to ensure only certain individuals can access/use an app.

Auth0 handles something similar with its Rule callbacks that can sit in between sign in and ups and do verification and then stop auth from happening. This is used to only allow signups for accounts with certain email domains -- or lookup the account from an approved list.

@kangmingtay
Copy link
Member Author

I guess the question is: is this a rule that enforces users cannot be created for OTP signups or rather this is an option available when signing up?

Nope, this flag is not to safeguard against those cases, it's merely an option for developers to disable automatic creation of users when using otps. If the developers want to restrict signups, then should use the DISABLE_SIGNUPS env var instead.

If the user is not to be created, ma then perhaps signups are just disabled instead? And then admins create accounts and then users simply sign in?

The user can be created, just not automatically via the otp endpoint. This will enable developers to separate the sign-up and login flows on the client-side where you have createUser=true for sign-up and createUser=false for logins.

I'd be concerned that if the rulers around this change, then the api has breaking changes rather than enforcing new rules about how users get created, sign in/up and the process.

Hmm, i don't really view it as a breaking change since we are adding an optional flag which defaults to the original behaviour if it's not set.

@kangmingtay kangmingtay changed the title [WIP] fix: add create_user field to otp endpoint fix: add create_user field to otp endpoint Jan 16, 2022
@@ -713,16 +713,20 @@ or show an account confirmed/welcome message in the case of `signup`, or direct

One-Time-Password. Will deliver a magiclink or sms otp to the user depending on whether the request body contains an "email" or "phone" key.

If `"create_user": true`, user will not be automatically signed up if the user doesn't exist.
Copy link
Member

Choose a reason for hiding this comment

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

is this right? If create_user == true, then user will not be signed up? seems counter intuitive

Copy link
Member Author

Choose a reason for hiding this comment

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

ah my bad. this is a typo error, it should be if create_user == true, then the user will be automatically signed up

```js
{
"phone": "12345678" // follows the E.164 format
"create_user": true
Copy link
Member

Choose a reason for hiding this comment

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

I read the discussion but still a bit confused about the use case, sorry.

What is the default behavior if omitted?

And the dev still has the ability to prevent signups on these endpoints via some config if they want to enforce login only, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

if omitted, the default behaviour would be the current behaviour - which is that the user will be automatically signed up if they don't have an existing account, and logged in if they do.

Copy link
Member Author

Choose a reason for hiding this comment

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

This flag isn't to restrict signups globally in their app. I think the use case here is for developers to have 2 separate flows - one for sign-up and one for sign-in when using magiclinks / otps.

@awalias awalias self-requested a review January 19, 2022 16:58
Copy link
Member

@awalias awalias left a comment

Choose a reason for hiding this comment

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

lets make "create_user": true into "create_user": false in the README and then good to go!

@kangmingtay kangmingtay merged commit 43d2e39 into master Jan 19, 2022
@kangmingtay kangmingtay deleted the km/fix-no-automatic-signups-for-otps branch January 19, 2022 17:14
@github-actions
Copy link
Contributor

🎉 This PR is included in version 2.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

leynier added a commit to supabase-community/auth-py that referenced this pull request Jan 20, 2022
* feat: add create user param to sign in

ref: supabase/auth#318

* 'Refactored by Sourcery' (#76)

Co-authored-by: Sourcery AI <>

* chore: format code

* chore: format code

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
@kristjanmar
Copy link

I wanted to implement this but I am getting a typescript error when I try to add the "create_user" flag to my signIn function:

"Argument of type '{ email: string; create_user: boolean; }' is not assignable to parameter of type 'UserCredentials'.
Object literal may only specify known properties, and 'create_user' does not exist in type 'UserCredentials'.ts(2345)"

Here's my code:

async function signIn(email: string) {
  const { error } = await supabase.auth.signIn({ email, create_user: false })
  return { error }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "disableSignUp" option for magic links
4 participants