Add user passwords (#301)

[inventor02]

Fixes #301

This PR adds passwords to users and if the right config values are set, prompts them for their password when they use a token they have not used for a while. If configured, users will be forced to set a password upon login if they do not have one already.

It also allows users to login directly with their user ID and password, if the configuration option user:allow_password_login is enabled. To support this, in any case, the user ID is surfaced a few more times around the UI, namely in the edit user dialogs and on the current user information dialog.

Passwords are PBKDF2 hashed + salted with the necessary information to check them stored in the database. This means alternative hashing methods or implementations could be added without breaking previously-set passwords.

Passwords can be changed by anyone with the edit-user-password permission (managers by default). Users can change their own passwords, but only if they have the edit-current-user-password permission (which basic_users do by default).

[sde1000]

Hi! Thank you for looking at this. This implementation looks good to me, at least at first glance...

I think the options you currently set via the command line and tillconfig module would be better off as quicktill.config options — probably declared in the quicktill.user module? The reasoning being, command-line options are for things that affect particular instances of the till software, whereas quicktill.config options affect the entire installation, and I think the password feature needs to be configured for the installation as a whole.

Open questions:

• How should passwords be stored? Plaintext? You're using sha256 of the password, which is reasonable, but would it be better to use an industry-standard password hash instead?
• I'm a bit iffy about the global user.is_password_prompt_displayed — I suspect it's covering up an underlying problem...

[inventor02]

I think the options you currently set via the command line and tillconfig module would be better off as quicktill.config options

Initially I thought it might be desired to configure stuff like this separately for different types of terminal but thinking about it now, yes, I agree.

How should passwords be stored? Plaintext? You're using sha256 of the password, which is reasonable, but would it be better to use an industry-standard password hash instead?

When I was testing this I was storing them in plain text - the idea I had was that however they're stored, the function between passwords and storage mechanisms should be deterministic so that it is possible to identify a user by their password (e.g. if they forget a token, or if passwords are not issued at all, they can log in with a password alone, if enabled).

We could of course also store them in plain text - which sounds bad, but I think with adequate warnings to the user could be doable...

I'm a bit iffy about the global user.is_password_prompt_displayed — I suspect it's covering up an underlying problem...

It is - the issue is that because tokens are handled at a very low level, if tokens are presented multiple times then multiple of the password entry dialogs are shown. I need to look closer at this and work out a nicer way of solving this problem (or if you have any immediate thoughts that'd be helpful too! :-))

[inventor02]

I stumbled across ui.hotkeypress() which when implemented allows us to catch usertoken events, and dismiss the dialog (which marks the end of the horrid global variable, which is nice)

[inventor02]

Don't worry about it - it's been a very busy time of year for me too :-)

I think I've addressed all your feedback? Please have a look/play and let me know what you think.

I agree about adding the UID to the login flow for password-only authentication - I've amended the PR to do this. The UID is shown in various places now, and when logging in with the password-only key, the UID is entered first (and then checked, and the shortname is shown next to it).

The password hashes are now PBKDF2 (Django-style tuple storage in the database) too.

I don't think I missed anything but it's entirely possible so let me know if you spot anything, or find any of it confusing/hard to use!

[sde1000]

Just a few notes now...

In localutils.py you've changed the stockterminal hotkey to create a stock terminal page with the current user, rather than with no user. Although that's valid and probably useful behaviour, it will be confusing to existing till users who expect the stock terminal page to have no permissions. I don't think it's connected to the "user-pins" feature so it should be left unchanged here.

You've added ui.validate_positive_int() for the user ID field, but user IDs start at 1 so the existing ui.validate_positive_nonzero_int() would work better for that.

Something about user.token_password_timeout is bothering me. I don't think having different behaviour for "0" is useful — in practice it would be the same as "1". I think it should be a config.IntervalConfigItem where blank or zero interval disables checks. Perhaps it should be named user.password_check_after to be consistent with things like register.transaction_lock_after?

The user.require_unique_passwords config setting still needs removing.

I'm undecided about whether the password hash generation and checking code should be part of the models.User class, or whether it should be in a separate module. It shouldn't be in user.py, because it needs to be usable by the web interface code as well so we can add the "reset password" facility to the web interface. Maybe create a passwords.py module for this code?

In user.edituser, the user ID doesn't need to be in an editfield; it's immutable, so it can just be drawn as text when setting up the popup.

[sde1000]

Also, I've just realised: if user.force_password_registration is set to False, there should be an option in the UI for users to remove their passwords too.

[sde1000]

One more thing that has just become an issue since I checked in the sqlalchemy-2.0 migration code: all your uses of td.s.query(model).get(pk) in user.py need changing to td.s.get(model, pk) to avoid triggering sqlalchemy deprecation warnings during testing.

[inventor02]

Thanks! Responses inline. Some of these serve me right for doing this at midnight...

In localutils.py you've changed the stockterminal hotkey to create a stock terminal page with the current user, rather than with no user. Although that's valid and probably useful behaviour, it will be confusing to existing till users who expect the stock terminal page to have no permissions. I don't think it's connected to the "user-pins" feature so it should be left unchanged here.

I think I assumed that was a bug - whoops! I've reverted that change.

You've added ui.validate_positive_int() for the user ID field, but user IDs start at 1 so the existing ui.validate_positive_nonzero_int() would work better for that.

Sorted, and removed the ui.validate_positive_int() function.

Something about user.token_password_timeout is bothering me. I don't think having different behaviour for "0" is useful — in practice it would be the same as "1". I think it should be a config.IntervalConfigItem where blank or zero interval disables checks. Perhaps it should be named user.password_check_after to be consistent with things like register.transaction_lock_after?

Makes sense to me. user.password_check_after it is. Blank or zero values disable the check, anything else behaves as you'd expect.

The user.require_unique_passwords config setting still needs removing.

I was confused until I checked my local repository and it turns out I never pushed this, which usually helps.

I'm undecided about whether the password hash generation and checking code should be part of the models.User class, or whether it should be in a separate module. It shouldn't be in user.py, because it needs to be usable by the web interface code as well so we can add the "reset password" facility to the web interface. Maybe create a passwords.py module for this code?

Makes sense, passwords.py created and the hashing and checking logic moved in there.

In user.edituser, the user ID doesn't need to be in an editfield; it's immutable, so it can just be drawn as text when setting up the popup.

Sorted.

Also, I've just realised: if user.force_password_registration is set to False, there should be an option in the UI for users to remove their passwords too.

I've changed it so if force_password_registration is disabled, blank values are accepted and clear the password. Do you think people with the edit-user-password permission (managers) should always be able to clear user passwords, irrespective of the force_user_passwords setting? It could be useful, as if a user forgets their password they could just have it cleared, and then set their own the next time they log in to the till. That way, the manager doesn't know the password - but then again, the user could also just key it in when required as they are being edited...

One more thing that has just become an issue since I checked in the sqlalchemy-2.0 migration code: all your uses of td.s.query(model).get(pk) in user.py need changing to td.s.get(model, pk) to avoid triggering sqlalchemy deprecation warnings during testing.

All done.

[sde1000]

I've changed it so if force_password_registration is disabled, blank values are accepted and clear the password. Do you think people with the edit-user-password permission (managers) should always be able to clear user passwords, irrespective of the force_user_passwords setting? It could be useful, as if a user forgets their password they could just have it cleared, and then set their own the next time they log in to the till.

Yes, I think people with permission to edit other users' passwords should be able to set them to blank, so the user is forced to set a new password next time they visit the till.

I think the logic around when to check the password still needs some thought.

For example, starting with a till database with no passwords set at all I did the following:

• Set user:password_check_after to "5 minutes" but left the other two configuration options at their defaults of "No"
• Started the till and touched in using a red token — this sets UserToken.last_seen but does not alter UserToken.last_successful_login (so it's still null)
• Set my own password to "1234"
• Locked the till
• Used the red token again — this logged me in with no password prompt and did not set UserToken.last_successful_login, which is not what I was expecting!

We have three timestamp fields to think about: User.last_seen, UserToken.last_seen and UserToken.last_successful_login. I think these should be updated as follows:

• User.last_seen: when the user successfully accesses the till, with or without the use of a password
• UserToken.last_seen: whenever the token is presented, successfully or otherwise
• UserToken.last_successful_login: when the user has a password set, and presentation of the token leads to the user successfully accessing the till (with or without being prompted for the password), and not at any other time (apart from being set to null when the token is reassigned)

User.last_seen is currently updated in user.user_from_token(), so it'll be updated even when a password is prompted for and not provided — this needs to change.

I think user.user_from_token() should be removed, and its functionality should be split across user.token_login() and maybe a new function user._finish_login() that takes care of updating User.last_seen and invoking the callback to the register or stock terminal — this would be called directly by user.token_login() if no prompt is needed, or by user.password_prompt() and user.password_login_prompt() if a password is being supplied.

The check in user.should_prompt_for_password() is looking at UserToken.last_seen and ignoring UserToken.last_successful_login — this needs to be the other way around.

It's generally not necessary to call td.s.commit() in most of the till code — alterations to the ORM session will be committed automatically once the current keypress has finished being handled. If you have a search for it you'll see it's really not used very much — only in the squareterminal.py and xero.py modules where we genuinely do need to commit to the database and then continue doing more database operations because we are interacting with external services over the network. The existing uses in barcode.py are down to me not thinking clearly at the time and I really ought to remove them!

[inventor02]

That makes sense - sorry, life and things sort of happened between starting this work and coming back to look at it in the new year and so it has taken me a while to get back up to speed with what I actually did!

I have refactored user.py and removed user_from_token() as suggested. The token information is fetched in token_login() instead. All the login functions now finalise with _finish_login(dbuser, callback) which then passes the (wrapped) user to the till or stock terminal, and updates the last_seen attribute in the users relation.

The usertokens.last_successful_login attribute is now set when the password is checked and is correct, and when the password is not prompted for, but the user does have a password set. usertokens.last_seen is always set whenever a token is presented and it is a valid one to log in with (i.e. it has a user, and the user is enabled). I am not sure about this behaviour - I wonder if it makes sense? Or do we care about tracking when the token was last seen at all, even if it is not assigned to a user?

I've tested this in a few scenarios and combinations of settings and it appears to work - although I don't actually own anything that can read tokens (despite having about n+1 iButtons and RFID things...) so I am limited to debug -> fake token.

Also, I removed all the instances of td.s.commit() I added - that one's down to me not being familiar with SQLAlchemy!

[sde1000]

I got a "force set password" prompt in testing, despite user:force_password_registration being set to "No".

I think the logic for this would be clearer if it was all implemented in user.token_login() — at the moment you call password_prompt if either of should_prompt_for_password() or should_force_set_password() is True, and then password_prompt ignores the state of should_force_set_password() and falls through to change_current_user_password_login_initial().

I think password_prompt should fall through to _finish_login() if the user doesn't have a password set, and the decision to call change_current_user_password_login_initial() should be made in token_login().

You dismiss the password prompt popup if a new usertoken is presented before the password is entered, but you don't dismiss any of the others like the password setting popup and the prompt for setting a new password. I was able to stack these arbitrarily deep by presenting usertokens in sequence with no password set!

I don't think the popups for setting a new password at login should be un-dismissable — the user should always be able to back out with the Clear key (although they would end up not logging in). Having a box stuck on the screen will make users panic and say "the till is broken".

When checking the password, it would be nice if the user didn't have to press Cash/Enter after entering a correct password. You could use the same technique as stock.stockpicker to check the password every time the password field contents change. (Still watch for the user pressing Cash/Enter, though, so you can say "wrong password" explicitly.)

[sde1000]

(Maybe make a mixin class for "dismiss this popup when a usertoken is presented or the LOGIN key is pressed", with the hotkeypress() method you currently use in password_prompt?)

[sde1000]

I've just made what I hope is the final release in the v23 series. If it's ok with you, I'll merge this pull request, fix up the remaining issues, fix some other unrelated issues that require simple schema changes, and get ready for a v24 release.

[inventor02]

Hey - sorry it's been a while, I've not had much free time recently! Feel free to take/update/merge - let me know if you'd like me to do anything/have any questions.

[sde1000]

Here are the changes I made after merging the pull request: bc06249

The changes from v23.15 to that commit (i.e. the whole feature as introduced) are here: v23.15...bc06249

Next time I get a chance to look at this, I'll add the remaining features I think are needed: avoiding the need for the user to press Cash/Enter on password entry, a plugin system for different password hash algorithms, and support for changing passwords through the web interface.

Thank you very much for your contribution! I don't think I'd have got around to implementing it nearly as quickly without your help.