search

SiddhantSadangi / st_login_form

Create secure authentication forms for your Streamlit apps - in one-line of code!
https://pypi.org/project/st-login-form/
MIT License
109 stars 22 forks source link

Password Hashing Feature #7

Closed TheAhmedRmdan closed 2 months ago

TheAhmedRmdan commented 2 months ago

Added password hashing (with salting) feature using bcrypt - pip install bcrypt (already added in requirements.txt) Because storing passwords as plain text is a bad practice, all newly created passwords are hashed. Old plaintext passwords stored in supabase database can be hashed using hash_current_passwords()

image

SiddhantSadangi commented 2 months ago

Hey @TheAhmedRmdan !

This is brilliant, thanks! 🎉

I had previously tried a lame attempt to hash passwords using the builtin hash() method: https://github.com/SiddhantSadangi/st_login_form/blob/b023bb80ccfcdd10a89d6d30457310b4f0b8189e/src/st_login_form/__init__.py#L145

This obviously did not work, as the hash would change every session due to its pseudorandom nature, and the passwords would never match. I ended up yanking v0.3.0 after discovering the chaos it caused (lesson for the future 📝)

I also had a local branch to migrate the migration to use Supabase Auth instead of DB, for a more secure and complete auth experience. However, I've been requested to make this library database agnostic, so sticking to pure databases is the way forward ✅

Coming to this PR, the changes work as expected ✅. The requirements.txt file is used by the demo app, not by pip to build the library. So we need to add bcrypt to setup.py's install_requires and remove it from requirements.txt. I've already pushed these changes.

We should also update the README and expose hash_current_passwords()

However, before making these changes, does it make sense to use either argon2id or scrypt, as recommended by bcrypt itself? What do you think?

TheAhmedRmdan commented 2 months ago

You are right! I certainly didn't notice that bcrypt recommended to use either argon2id or scrypt I did my research, and since argon2id won the Password Hashing Competetion, I re-wrote my functions using argon2 for python

Feel free to update the README, you are the original author and I like your explanations. By the way, I removed bccrypt from setup.py, yes you made a typo 😂

I'm looking forward to contributing more to this repo which is one of my favorites. great work!

SiddhantSadangi commented 2 months ago

Hey @TheAhmedRmdan , Thanks for the quick turnaround!

I'll try to review this over the weekend and release if all looks good ✅

Meanwhile, I've three pending features to add, in order of complexity:

  1. logout button for logged-in users
  2. Using st_supabase_connection to connect to Supabase
  3. Making the library database agnostic. For starters, I'd like to support MySQL databases

Let me know if you'd like to pick something up :)

TheAhmedRmdan commented 2 months ago

Hello @SiddhantSadangi I've already implemented a logout button in one of my Streamlit projects. Would you like me to commit it to this pull request #7 or open a new one with the logout button feature?

Please let me know what you prefer.

SiddhantSadangi commented 2 months ago

@TheAhmedRmdan - a new PR would be easier to manage. Thanks 🫶

SiddhantSadangi commented 2 months ago

@TheAhmedRmdan - I've reviewed the changes and added quite few changes of my own.

I'd like to have an extra pair of eyes to test. Could you please go through the changes and let me know if everything works well for you?

In particular, I want to test the below cases:

TheAhmedRmdan commented 2 months ago

Hey @SiddhantSadangi, I hope that you are doing great. Great changes, I appreciate your collaboration efforts. I really liked the progress bar stqdm. I've tested all those cases before committing and tested them again after your changes, and everything works fine. You are good to go!