Velle-log / hs-api

0 stars 1 forks source link

Design and Validate models for signup workflow #8

Closed Samy-33 closed 5 years ago

Samy-33 commented 5 years ago

@Velle-log/hs-api-ft FYI

For SignUp Workflow:

Samy-33 commented 5 years ago

OTP Model:

class UserOTP(models.Model):
    one_time_password = models.CharField(max_length=8)
    email = models.EmailField(unique=True)
    expires_at = models.DateTimeField()

    created_at = models.DateTimeField(auto_now_add=True)
    updated_at = models.DateTimeField(auto_now=True)
sk364 commented 5 years ago

At this point, an entry in auth_user table will be created?

API verifies the OTP, sends the user auth_token, refresh_token.

Samy-33 commented 5 years ago

Yes. A new user will be created after successful verification of OTP.
We have to decide how we are going to handle username at this point.

sk364 commented 5 years ago

I think that user should be set as inactive. Username can be the same as email, at that point. Then, in the "Set Password", "Set Username" can also be added.

More questions:

Samy-33 commented 5 years ago

Username can be the same as email, at that point. Then, in the "Set Password", "Set Username" can also be added.

Makes sense.

Strategy against bruteforce attacks?

Restricting user to enter OTP only x number of times would reduce chances of brute force. Also need to be able to resend OTP y number of times. After which the email id will be blacklisted for some predefined time.

Cron Job to delete expired token rows?

Yes. We can have this in two ways.

  1. Create a single Cron Job (runs every n minutes) which deletes UserOTPs which have expired.
  2. Create a Crob Job after storing a single UserOTP entry, which deletes this particular UserOTP.
sk364 commented 5 years ago

X = Y = 3 for the initial run?

Restricting user to enter OTP only x number of times would reduce chances of brute force. Also need to be able to resend OTP y number of times. After which the email id will be blacklisted for some predefined time.

Why not delete the entry after the successful match?

Create a Crob Job after storing a single UserOTP entry, which deletes this particular UserOTP.

Samy-33 commented 5 years ago

OTP can be deleted after successful match. But sometimes user might ask to resend the OTP. In that case should we keep the original OTP or delete it and create a new one? Or resend the same OTP or keep the original as valid OTP and send another.? These are to be considered.

And rest remaining expired OTPs will be deleted by CRONJOB

pranjulps3 commented 5 years ago

OTP can be deleted after successful match. But sometimes user might ask to resend the OTP. In that case should we keep the original OTP or delete it and create a new one? Or resend the same OTP or keep the original as valid OTP and send another.? These are to be considered.

Requesting a new OTP should invalidate the original so deleting that would make sense unless we're thinking of a scenario where he is signing up from two different devices at the same time which in itself is kind of dumb.

pranjulps3 commented 5 years ago

What will be the password field if someone uses Google, Facebook or GitHub OAuth to sign up. Will it be an empty or a random string will be assigned to it? Both of those require a different approach when changing password.

Samy-33 commented 5 years ago

unless we're thinking of a scenario where he is signing up from two different devices at the same time which in itself is kind of dumb.

You are right. But we shouldn't expect our users to be very smart, with high IQ and MIT graduation. It is a case to be handled. won't be good if it breaks our application.

What will be the password field if someone uses Google, Facebook or GitHub OAuth to sign up. Will it be an empty or a random string will be assigned to it? Both of those require a different approach when changing password.

Once the user successfully logs in using google, the stepper skips the Verify email step and shows set password step. And also:

Username can be the same as email, at that point. Then, in the "Set Password", "Set Username" can also be added.

This should be considered.

pranjulps3 commented 5 years ago

You are right. But we shouldn't expect our users to be very smart, with high IQ and MIT graduation. It is a case to be handled. won't be good if it breaks our application.

I can't think of any application that covers the scenario we're talking about and It's not something that occurs very often. Regardless of everything I suggest on not covering that scenario.

Once the user successfully logs in using google, the stepper skips the Verify email step and shows set password step. And also:

That fits fine.

Username can be the same as email, at that point. Then, in the "Set Password", "Set Username" can also be added.

We already need a bunch of other fields, than username. We can set the username with email or something unique at start and then redirect them to the profile page in edit mode where they can change things.

Samy-33 commented 5 years ago

Regardless of everything I suggest on not covering that scenario.

I am okay with not covering. We just have to make sure it doesn't break anything on API. So handling this case would be a good.

We already need a lot of other fields other than username.

But the username field is important. With set password. We can add one more field for adding username.

pranjulps3 commented 5 years ago

it doesn't break anything on API

I think we can see it as a feature not provided. I don't see anything breaking as such.

But the username field is important. With set password. We can add one more field for adding username.

I have no issue adding it but I don't want the initial sign up form to be long.

Samy-33 commented 5 years ago

Okay. Just let us know how it looks with username field added in the last step.
New models for signup workflow:

class UserOTP(models.Model):
    one_time_password = models.CharField(max_length=8)
    email = models.EmailField(unique=True)
    expires_at = models.DateTimeField()

    created_at = models.DateTimeField(auto_now_add=True)
    updated_at = models.DateTimeField(auto_now=True)

class UserOTPInfo(models.Model):
    email = models.EmailField(...)
    attempts_used = models.IntegerField(...)
    resends_used = models.IntegerField(...)

    created_at = models.DateTimeField(auto_now_add=True)
    updated_at = models.DateTimeField(auto_now=True)

Comments and improvements are welcome.

pranjulps3 commented 5 years ago

class UserOTP(models.Model):
    one_time_password = models.CharField(max_length=8)
    email = models.EmailField(unique=True)
    expires_at = models.DateTimeField()

    created_at = models.DateTimeField(auto_now_add=True)
    updated_at = models.DateTimeField(auto_now=True)

I am not sure if there is a use of updated_at field if we're deleting the original OTP on resend.


class UserOTPInfo(models.Model):
    email = models.EmailField(...)
    attempts_used = models.IntegerField(...)
    resends_used = models.IntegerField(...)

    created_at = models.DateTimeField(auto_now_add=True)
    updated_at = models.DateTimeField(auto_now=True)

What are the parameters for resetting this and restricting the flow?

sk364 commented 5 years ago

It is a common practice to keep the same OTP until it is expired. (Example: Any Bank Payment) An UserOTP object will only be deleted when a. Successful match b. Expired

Considering the above, if user decides to register through 2 devices at the same time, it will get the same OTP on both as there will be only and only one object in UserOTP with the same email.

Resend Logic: The idea is to first check if the email already exists in the table, if yes, check the expiry, if it's good then issue that OTP, else update the row with a new OTP and expiry and issue that. if row doesn't exist, create new OTP with the email.

Setting the username can be done either ways. That isn't a big issue, in my opinion.

sk364 commented 5 years ago

There could be another field in UserOTPInfo called blocked_until as DateTimeField which will block the email, if attempts_used or resends_used reach the limit.

This will block the email for a certain time period, under which when the user comes and uses that email, then he will get an error message saying that your email is blocked. (Should we tell him until when?)

A cron job will reset the attempts and resends, if the block time is finished.

Samy-33 commented 5 years ago

I am not sure if there is a use of updated_at field if we're deleting the original OTP on resend.

Noted.

What are the parameters for resetting this and restricting the flow?

3 attempts for OTP , 3 resends allowed. After each resend, reset OTP attempts to 3

Samy-33 commented 5 years ago

then he will get an error message saying that your email is blocked. (Should we tell him until when?)

In that case we'll send a message saying. Too many attempts for this email. Please try after sometime.( Or specific time)

sk364 commented 5 years ago

This will be the case, only when we are recreating the OTP, right?

3 attempts for OTP , 3 resends allowed. After each resend, reset OTP attempts to 3

sk364 commented 5 years ago

I think attempts and resends should be mutually exclusive, but in the end producing the same effect (blocked email), if either reach the limit.

What do you guys think?

Samy-33 commented 5 years ago

I think attempts and resends should be mutually exclusive, but in the end producing the same effect (blocked email), if either reach the limit.

Okay, makes sense. We might then increase the attempts to 4 or 5. Resends be only 3.

Samy-33 commented 5 years ago
class UserOTP(models.Model):
    one_time_password = models.CharField(max_length=8)
    email = models.EmailField(unique=True)
    expires_at = models.DateTimeField()

    created_at = models.DateTimeField(auto_now_add=True)
    updated_at = models.DateTimeField(auto_now=True)

class OTPAttempt(models.Model):
    email = models.EmailField(...)
    attempts_used = models.IntegerField(...)
    resends_used = models.IntegerField(...)
    blocked_until = models.DateTimeField(...)

    created_at = models.DateTimeField(auto_now_add=True)
    updated_at = models.DateTimeField(auto_now=True)

Can we use UserOTP as a foreign key in OTPAttempt model, instead of using email field in it?

sk364 commented 5 years ago

We can have a OneToOneField, but I think it will be a performance overhead on the database to maintain a foreign key index on the table.

Samy-33 commented 5 years ago

Is it okay to choose redundancy over this performance overhead?
As we know that these two models are directly connected to each other ( i.e. There will be only one UserOTP object and only one OTPAttempt object for one email).

Can we merge both the models into one?

pranjulps3 commented 5 years ago

I think email works just fine.

sk364 commented 5 years ago

Merging sounds good.

Samy-33 commented 5 years ago

@pranjulps3, we think keeping one model instead of two will be better. What do you think?
It helps avoiding two db calls to get the whole data.

pranjulps3 commented 5 years ago

Yeah that sounds good. We can go ahead with that