nfoert / cardie

An open source business card designer and sharing platform
https://cardie-uwtwy.ondigitalocean.app/
GNU General Public License v3.0
381 stars 24 forks source link

Implement a more secure way to log in and create accounts #32

Open nfoert opened 2 months ago

nfoert commented 2 months ago

Currently the username and password data is stored in the http headers which isn't the most secure. Implement a better way to send these credentials.

Vineet8800 commented 1 week ago

@nfoert I would like to work on this. My suggestion here is to use Django Sessions so that we do not need to keep username and password in headers all the time. Also, I would like to update the sign-in and signup views and move them to class-based views, so that we can inherit Django inbuilt classes.

nfoert commented 1 week ago

My suggestion here is to use Django Sessions so that we do not need to keep username and password in headers all the time.

Django sessions is already being used. My main concern was establishing if there was a more secure way to send the data to the server to set up the session, unless I'm not understanding the meaning of your suggestion?

Also, I would like to update the sign-in and signup views and move them to class-based views, so that we can inherit Django inbuilt classes.

That could be a good idea, however what kind of inbuilt django classes would be used for this?

Vineet8800 commented 1 week ago

@nfoert

Django sessions is already being used. My main concern was establishing if there was a more secure way to send the data to the server to set up the session, unless I'm not understanding the meaning of your suggestion?

To make sign-in more secure you can use MFA or email/sms OTP(for OTP you will need an SMPT service, which today or tomorrow you should have to send new account verification emails. For testing you can use https://sendgrid.com/en-us). MFA will need your users to have an authentication app. In addition to sessions, you can keep track of users being active or not, and based on inactivity, expires the user sessions. This will force the user to login again and will at least rotate the session.

what kind of inbuilt django classes would be used for this?

Well, the classes are not limited to sign-in and signup only but to every API you have. These are:

  1. django.contrib.auth.views.LoginView
  2. django.contrib.auth.views.LogoutView
  3. django.contrib.auth.views.PasswordResetView and few others

You can inherit these and override what you want like template HTML, context, or post success processes, and can redirect from the view itself instead of sending success in response and handling it from the javascript.

OR You can use some more base views like:

  1. django.views.generic.FormView
  2. django.views.generic.CreateView
  3. django.views.generic.DetailView
  4. django.views.generic.ListView
  5. django.views.generic.DeleteView and few others

If you plan to add jquery.js and jquery.validate.js

Also, in authentication.js, I see you have manually added listeners to handle submissions and validate inputs. But you can use <forms> in templates and add validate() on it in javascript and pass validation rules to it.

ar4s commented 1 week ago

We have two problems:

  1. custom authentication via request.session["username"] in views (what is not proper way in Django)
  2. custom User model need (now there are User but is artificial - Django doesn't use that)
  3. Sign in with XYZ a.k.a. authorization via OAuth
  4. MFA/TFA/OTP - I work on it and do some resarch with django-otp https://github.com/nfoert/cardie/issues/69

@Vineet8800 @nfoert I think we should synchronize this issue with https://github.com/nfoert/cardie/issues/51 and @jenyyy4 because OAuth support (by using for instance django-allauth) should solves a majority of above problems or should be included as a result of work on #51.

nfoert commented 1 week ago

I think we should synchronize this issue with https://github.com/nfoert/cardie/issues/51 and @jenyyy4

I think that would be wise, perhaps as a part of implementing OAuth the entire system for authentication could be rewritten to use more conventional Django methods. This would also help with not doing a bunch of changes twice - we can just do all of the reworking at once

I'm also learning that when I first started this project I should have looked into the traditional django authentication methods 😄

Vineet8800 commented 1 week ago

@nfoert Let me know if I could be of any help in this. Thanks

Yes, django-allauth will solve most of the problems.

custom User model need (now there are User but is artificial - Django doesn't use that)

@ar4s Can you elaborate on this, The User table that is currently configured in this project is Django's own. People generally inherit AbstractBaseUser to have more control but still this model is an integral part of the Django ecosystem and has a special setting AUTH_USER_MODEL= and a special retrieval method django.contrib.auth.get_user_model to use it all over the project. So can you explain it a little bit. Thank you.

spline2hg commented 1 week ago

hey @nfoert @ar4s could a authentication system with email and password for verifying ,using a Custom user model work for this ?this could serve as base, then we could work either implement a email verification based login or third party login or both, and btw do we want a email based authentication or google authentication?

nfoert commented 1 week ago

then we could work either implement a email verification based login or third party login or both, and btw do we want a email based authentication or google authentication?

This could definitely work, I think that email verification and SSO would work for now, perhaps 2FA via an authenticator app would be a future additional feature, unless it's something that's easy to implement when the OAuth pieces are added

ar4s commented 1 week ago

The User table that is currently configured in this project is Django's own. People generally inherit AbstractBaseUser to have more control but still this model is an integral part of the Django ecosystem and has a special setting AUTH_USER_MODEL= and a special retrieval method django.contrib.auth.get_user_model to use it all over the project. So can you explain it a little bit.

What I meant was that there is a user model just in the project, but it is not 100% integrated with Django mechanisms (i.e. among other things, lack of relation with AUTH_USER_MODEL) and introduces ambiguity because there is another user model (with django.contrib.auth.models), so you cannot use all the power of the installed batteries in the framework.

Vineet8800 commented 1 week ago

@ar4s Thank you for the reply. Django provides its User models(there are 3 types) to ease your setup. The best thing we can do right now is to inherit django.contrib.base_user.AbstractBaseUser in our User model and set the AUTH_USER_MODEL in settings(Why AbstractBaseUser? Because it is the least restrictive and you can take hints from other django.contrib.models.AbstractUser and django.contrib.models.User to modify yours as per need). Our first focus should be fixing our User model because even with django-allauth or any other tool, you will need a working User model at the end.