mkhorasani / Streamlit-Authenticator

A secure authentication module to manage user access in a Streamlit application.
Other
1.63k stars 254 forks source link

"credentials" is overwritten to the default value each time, so authentication does not succeed when the user name contains uppercase letters #185

Closed rinsa318 closed 1 month ago

rinsa318 commented 3 months ago

First of all, thanks for the great library!

I noticed that v0.3.3 fails to authenticate if the username contains uppercase letters. So I read the code in detail and noticed that the credential initialization was being overwritten each time( L45, in authentication_model.py ) . Therefore, I feel that 'AuthenticationService.init' was not working as you expected.

rinsa318 commented 3 months ago

Sorry for the messy code, but it can be improved by saving it in session_state like this.

class AuthenticationModel:
    """
    This class executes the logic for the login, logout, register user, reset password,
    forgot password, forgot username, and modify user details widgets.
    """

    def __init__(
        self,
        credentials: dict,
        pre_authorized: Optional[List[str]] = None,
        validator: Optional[Validator] = None,
        auto_hash: bool = True,
    ):
        """
        Create a new instance of "AuthenticationService".

        Parameters
        ----------
        credentials: dict
            Dictionary of usernames, names, passwords, emails, and other user data.
        pre-authorized: list, optional
            List of emails of unregistered users who are authorized to register.
        validator: Validator, optional
            Validator object that checks the validity of the username, name, and email fields.
        auto_hash: bool
            Automatic hashing requirement for the passwords,
            True: plain text passwords will be automatically hashed,
            False: plain text passwords will not be automatically hashed.
        """

        self.credentials = credentials
        if "AuthenticationService.__init__.usernames" not in st.session_state:
            st.session_state["AuthenticationService.__init__.usernames"] = None
        if "AuthenticationService.__init__.auto_hash" not in st.session_state:
            st.session_state["AuthenticationService.__init__.auto_hash"] = None

        if self.credentials["usernames"]:
            # convert usernames
            if st.session_state["AuthenticationService.__init__.usernames"] is None:
                print("convert username")
                self.credentials["usernames"] = {
                    key.lower(): value for key, value in self.credentials["usernames"].items()
                }
                st.session_state["AuthenticationService.__init__.usernames"] = self.credentials["usernames"]
            else:
                print("load userneme from session state")
                self.credentials["usernames"] = st.session_state["AuthenticationService.__init__.usernames"]

            if auto_hash:
                # hash password
                if st.session_state["AuthenticationService.__init__.auto_hash"] is None:
                    print("hash password")
                    if len(self.credentials["usernames"]) > params.AUTO_HASH_MAX_USERS:
                        print(
                            f"""Auto hashing in progress. To avoid runtime delays, please manually
                              pre-hash all plain text passwords in the credentials using the
                              Hasher.hash_passwords function, and set auto_hash=False for the
                              Authenticate class. For more information please refer to
                              {params.AUTO_HASH_MAX_USERS_LINK}."""
                        )
                    hashed_pw = {}
                    for username, _ in self.credentials["usernames"].items():
                        if not Hasher._is_hash(self.credentials["usernames"][username]["password"]):
                            self.credentials["usernames"][username]["password"] = Hasher._hash(
                                self.credentials["usernames"][username]["password"]
                            )
                        hashed_pw[username] = self.credentials["usernames"][username]["password"]
                    st.session_state["AuthenticationService.__init__.auto_hash"] = hashed_pw
                else:
                    print("load hased password from session state")
                    hashed_pw = st.session_state["AuthenticationService.__init__.auto_hash"]
                    for username, _ in self.credentials["usernames"].items():
                        self.credentials["usernames"][username]["password"] = hashed_pw[username]

        else:
            self.credentials["usernames"] = {}

        self.pre_authorized = pre_authorized
        self.validator = validator if validator is not None else Validator()
        if "name" not in st.session_state:
            st.session_state["name"] = None
        if "authentication_status" not in st.session_state:
            st.session_state["authentication_status"] = None
        if "username" not in st.session_state:
            st.session_state["username"] = None
        if "logout" not in st.session_state:
            st.session_state["logout"] = None

    def check_credentials(
ocmarus commented 3 months ago

confirm the situation! if username is in case sensitive - it will not let you login. you can find in st.session_state['username'] value - it will be lowered

mkhorasani commented 3 months ago

Dear @ocmarus and @rinsa318, I am unable to recreate this error. Can you please give me the exact steps taken (perhaps with a screen recording) to show me what exactly the problem is?

rinsa318 commented 3 months ago

@mkhorasani Thanks for the response! I will write a list of steps to reproduce the problem.

  1. git clone https://github.com/mkhorasani/Streamlit-Authenticator.git
  2. cd Streamlit-Authenticator/
  3. pip3 install .
  4. change username of jsmith in config.yaml to JSMITH like below
  5. run test cd tests && streamlit run streamlit_authenticator_test.py
  6. try to login as JSMITH with PW(abc) <-- it will not be able to login

modified config.yaml

cookie:
  expiry_days: 30
  key: some_signature_key
  name: some_cookie_name
credentials:
  usernames:
    JSMITH:
      email: jsmith@gmail.com
      failed_login_attempts: 0
      logged_in: true
      name: John Smith
      password: $2b$12$iWlVOac3uujRvTrXDi6wructXftKmo/GyQd6SMu5FmyX306kH.yFO
    dbaldwin:
      email: dbaldwin@gmail.com
      failed_login_attempts: 0
      logged_in: false
      name: David Baldwin
      password: $2b$12$E9/bCaN/r8sN/FV2l8NpgOgspUBAp7UAVU6BgsXJzp/pW8gQWCprC
    rbriggs:
      email: rbriggs@gmail.com
      failed_login_attempts: 0
      logged_in: false
      name: Rebecca Briggs
      password: $2b$12$uNaTgvGPG9rMbzOJHYaPQePw0DUfp1qHBrSq6l4O304qani6pKFpm
    rcouper:
      email: rcouper@gmail.com
      failed_login_attempts: 0
      logged_in: false
      name: Ross Couper
      password: $2b$12$Tir/PbHVmmnt5kgNxgOwMuxNIb2fv2pJ.q71TW8ekvbugCqkye4yu
pre-authorized:
  emails:
  - melsby@gmail.com
mkhorasani commented 3 months ago

Thanks, but I don't see why you would want to manually change the username to uppercase in the config file. What is the use case for it?

rinsa318 commented 3 months ago

I have never used any user registration features through streamlit and have always used config.yaml made by myself in advance. v0.3.2 and earlier allowed uppercase letters to be mixed in with the username. If this library only expects lowercase usernames, there is no problem.

However, I think it would be good to add a note in the README that "usernames are supported in lowercase only".

mkhorasani commented 3 months ago

I have never used any user registration features through streamlit and have always used config.yaml made by myself in advance. v0.3.2 and earlier allowed uppercase letters to be mixed in with the username. If this library only expects lowercase usernames, there is no problem.

However, I think it would be good to add a note in the README that "usernames are supported in lowercase only".

No that's a fair point. It's just that I didn't see a use case for it. I will make sure to address this in the next release which is already in the works!

rinsa318 commented 3 months ago

@mkhorasani That's nice, thanks. Again, if you plan to support case-sensitive usernames, the problem is that the lines after here are not working as expected!

P.S. More to the point, I don't think auto hashed passwords are also stored as you expected, although I haven't checked...

mkhorasani commented 3 months ago

@mkhorasani That's nice, thanks. Again, if you plan to support case-sensitive usernames, the problem is that the lines after here are not working as expected!

P.S. More to the point, I don't think auto hashed passwords are also stored as you expected, although I haven't checked...

Yes, I can see the issue now. Thanks for bringing this to my attention. It is indeed a bug! And no I don't want the username to be case-sensitive, as in it should log in regardless of whether upper or lower cases are being used.

mkhorasani commented 3 months ago

I have already fixed this issue and will push it in the next release! Thanks for bringing this to my attention and please stay tuned.

ocmarus commented 3 months ago

Dear @ocmarus and @rinsa318, I am unable to recreate this error. Can you please give me the exact steps taken (perhaps with a screen recording) to show me what exactly the problem is?

@rinsa318 gave exact instruction to reproduce. I can only add a few notes: my app worded fine with 0.3.2 version. In requirements.txt file there was a line ~=0.3.2 and I found that new package was assembled with 0.3.3 version instead of my local 0.3.2 As result, I have working environment locally, but not in production. accounts.yml file is the same for both variants:

 credentials:
   usernames:
     unitstream:
       password: $2b$12$XK7eE/JZcX//.ub4cH324.8MQk.AF5gTSTNMySXWLg3lditTCvZcK
     Anderson:
        password: $2b$12$UUIGWiHHbKugEsIdNzQzXuCYam0vf3XC7jD0sEyYAXbol7sesxqke

username with "unistream" name is loging successfully for both variantsnts. User "Anderson" got "wrong login / password" in version 0.3.3, but still works fine with 0.3.2 No any other changes requires to reproduce the bug.

mkhorasani commented 3 months ago

Dear @ocmarus and @rinsa318, I am unable to recreate this error. Can you please give me the exact steps taken (perhaps with a screen recording) to show me what exactly the problem is?

@rinsa318 gave exact instruction to reproduce. I can only add a few notes: my app worded fine with 0.3.2 version. In requirements.txt file there was a line ~=0.3.2 and I found that new package was assembled with 0.3.3 version instead of my local 0.3.2 As result, I have working environment locally, but not in production. accounts.yml file is the same for both variants:

credentials: usernames: unitstream: password: $2b$12$XK7eE/JZcX//.ub4cH324.8MQk.AF5gTSTNMySXWLg3lditTCvZcK Anderson: password: $2b$12$UUIGWiHHbKugEsIdNzQzXuCYam0vf3XC7jD0sEyYAXbol7sesxqke

username with "unistream" name is loging successfully for both variantsnts. User "Anderson" got "wrong login / password" in version 0.3.3, but still works fine with 0.3.2 No any other changes requires to reproduce the bug.

Yes, this is an issue that is specific to v0.3.3. And it will be rectified in v0.3.4.

mkhorasani commented 1 month ago

Dear all please check the latest release.