edmundmok / mealpy

Order your meals on MealPal automatically!
MIT License
28 stars 22 forks source link

Script does not get password from keyring #19

Closed hugo-lyppens closed 5 years ago

hugo-lyppens commented 5 years ago

Fix main thus, otherwise it does not get the password from the keyring: if name == 'main': EMAIL = load_config()['email_address']

if USE_KEYRING:
    # If we're using keyring, we should always grab from keyring instead of holding on to potentially stale password
    PASSWORD = keyring.get_password(KEYRING_SERVICENAME, EMAIL)
    if not PASSWORD:
        PASSWORD = getpass.getpass('Credential not yet stored in keychain, please enter password: ')
        keyring.set_password(
            KEYRING_SERVICENAME,
            EMAIL,
            PASSWORD)
else:
    PASSWORD = getpass.getpass('Enter password: ')
ipwnponies commented 5 years ago

Do you have repro steps? The password is read from keyring at login.

https://github.com/edmundmok/mealpy/blob/6eddaac973abb737d64c378994b9e6fa10ce7ea8/mealpy.py#L54

hugo-lyppens commented 5 years ago

I did not realize it reads from the keyring in two places. I still think it is nicer to remember the keyring password in PASSWORD under if USE_KEYRING: instead of throwing the information away. Up to you whether you want to adopt this suggestion as I see it will work as-is. Thanks for a great script. Worked for me today.

ipwnponies commented 5 years ago

That would cache the PASSWORD. If this script is used as a daily scheduled task and I change my password, I would need to stop the running daemon, to reload the updated credentials. We discuss this a bit in https://github.com/edmundmok/mealpy/pull/16#discussion_r272912572.

A note, the implementation you see is very short-term, interim solution. It will change once we have cli arguments (#16) and can have a separate options to set keyring password. Then the main flow never needs to request password, it'll always use keyring.get_password.