Ask for new passwords twice, ask for current password on change

[lambdaTotoro]

This PR brings the authenticator up to spec on some very standard security and usability practices.

• From now on, NativeAuthenticator will ask for new passwords twice (on signup, changing your own and admins changing other people's passwords).
• It also asks you your current password when changing it so that nobody's account gets stolen merely because they accidentally left a session open.
• It also improves the layout of the change-password pages (plural, since admin and own password now function differently) and adds a bit of flavour text so the site doesn't appear as barren.

Closes #170. Closes #73.

[consideRatio]

Thanks for your work on this @lambdaTotoro, I agree with the value of this feature!

I think variable names are crucial to code comprehension so I suggest some changes to those. I also think three functions receive the new password confirmation that perhaps shouldn't be bothered with that - but that instead the new password confirmation should be made even before those functions are called, as they would only be if the new passwords provided matched.

To summarize, my suggested changes are:

• Suggested more verbose variable names
  • correctpw, perhaps to correct_password_provided
  • confirmation / con_password, perhaps to new_password_conf
  • match parameter, perhaps to new_password_conf_ok
  • password_prove, perhaps old_password_input
  • password_input, perhaps new_password_input
  • password_confirm, perhaps new_password_conf_input
• Suggested to not pass on the new password confirmation to:
  • user_change_password
  • change_password
  • create_user

[lambdaTotoro]

Happy to change the variable names and such as you suggest, but I'm unsure about (not) handing through the confirmation passwords to the functions in question. It seemed fitting to me because they already contain similar logic (see my comment). What are your thought on that?

[consideRatio]

@lambdaTotoro can you amend the commit to not mention me with @consideRatio ? It will make ge get norifications far into the future as this commit is processed in various locations.

[lambdaTotoro]

You get notifications when anything processes any commit with your handle in it?!
Who thought that was a good idea? Yeah, sure. On it.

[lambdaTotoro]

That should do it, right?

[consideRatio]

This looks almost ready for merge, i made one code suggestion that I think may be critical.

Could you trial it out end-to-end as well? I think our tests wouldn't capture issues like the one I may have spotted so it could be good to do a full end-to-end test of using the related input forms.

[lambdaTotoro]

I have fixed the issue you found and tested the whole thing end-to-end again, it all works as expected now. :)

[consideRatio]

@lambdaTotoro sorry for the confusion I recalled incorrectly that pw was used in UserInfo, but it was password.

class UserInfo(Base):
    """
    This class represents the information that NativeAuthenticator persists in
    JupyterHub's database.
    """

    __tablename__ = "users_info"
    id = Column(Integer, primary_key=True, autoincrement=True)

    # username should be a JupyterHub username, normalized by the Authenticator
    # class normalize_username function.
    username = Column(String(128), nullable=False)

    # password should be a bcrypt generated string that not only contains a
    # hashed password, but also the salt and cost that was used to hash the
    # password. Since bcrypt can extract the salt from this concatenation, this
    # can be used again during validation as salt.
    password = Column(LargeBinary, nullable=False)

So, please exclude the undo refactor (aec917f) commit that I made you add. Further, I think it was not causing issues, but it made it important to have a specific parameter name for the positional argument password in create_user - which we had.

[lambdaTotoro]

I was under the impression it still might affect the object because the kwargs are being updated into to dictionary after it's created with the encoded_password. So if password was part of the kwargs, it would overwrite the encoded password with the unencoded one. But password isn't part of the kwargs, so it's fine. Do I have that right?

[consideRatio]

I think when we call my_func(**some_object) it becomes as if we did my_func(key1=val1, key2=val2, key3=val3), but if you have a function definition like...

def my_func(key1, key2, **kwargs):
    # kwargs will hold key3, but not key1 and key2, I think.

Okay I verified this! So the tricky part was that when we call create_user(**infos) we make infos expand to be both positional arguments and key word arguments, and because password was a positional argument, it could be set without influencing things. But, if you would have refactored the infos dict's key for anything but username and password it would have been trouble.

[lambdaTotoro]

Thanks for verifying that. Anything else left to do on this one?

[consideRatio]

Thanks for taking the time with the review iterations @lambdaTotoro!

[lambdaTotoro]

Thanks for taking the time to review and being patient with me, @consideRatio. This PR really drove home the message of not just merging your own contributions willy-nilly!