Avoid updating password hash when request with simple password scheme

[jiahuili430]

Overview

When using the simple password scheme, the number of iterations is undefined, so the user's password hash is updated every time when a new request is made using user's authentication credentials.

Add a case statement to avoid this situation.

Testing recommendations

make eunit apps=couch suites=couch_passwords_hasher_tests

Related Issues or Pull Requests

Checklist

• Code is written and works correctly
• Changes are covered by tests
• Any new configurable parameters are documented in rel/overlay/etc/default.ini
• Documentation changes were made in the src/docs folder
• Documentation changes were backported (separated PR) to affected branches

[rnewson]

good fine on the root cause of the spurious password "upgrades". I have some style nits to address first though.

[rnewson]

again, not sure why we'd force ourselves over the newer module for this function

[jiahuili430]

For testing only. Use meck:num_calls(couch_password_hasher, upgrade_password_hash, ['_', '_', '_', '_', '_', '_']). to count how many times the function has been called.

[rnewson]

the (careful) use of module-qualified function calls is an important part of live upgrades (I appreciate Cloudant doesn't do them anymore but others might). I don't think it's wise to abuse it for testing.

Perhaps the module needs some rearranging to make it more testable but, for example, you could call upgrade_password_hash/6 in an eunit tests declared in this file rather than the test/eunit hierarchy, and that way we also don't need to export them.

[jiahuili430]

The ?MODULE: and export statements have been removed.
Instead, using meck:called/3 to check if the handle_cast({upgrade_password_hash, ...}, ...) function has been called.

[rnewson]

the (careful) use of module-qualified function calls is an important part of live upgrades (I appreciate Cloudant doesn't do them anymore but others might). I don't think it's wise to abuse it for testing.

Perhaps the module needs some rearranging to make it more testable but, for example, you could call upgrade_password_hash/6 in an eunit tests declared in this file rather than the test/eunit hierarchy, and that way we also don't need to export them.

[rnewson]

I think the needs_upgrade logic is;

case {TargetScheme, TargetIterations, TargetPRF} of
    {CurrentScheme, _, _} when CurrentScheme == <<"simple">> ->
        false;
    {CurrentScheme, CurrentIterations, CurrentPRF} when CurrentScheme == <<"pbkdf2">> ->
        false;
    {_, _, _} ->
        true
end.

1. if the target scheme is simple and the current scheme is simple, you don't need to upgrade (and target iterations and target prf are not relevant).
2. if the target scheme is pbkdf2 and the target scheme is pbkdf2 and the target iterations equal the current iterations and the target prf equals the current prf, you don't need to upgrade
3. otherwise you do need to upgrade.

The original bug (by me) is insisting that TargetIterations matches CurrentIterations. You rightly point out that CurrentIterations will be undefined (which is correct for "simple"), but TargetIterations will be a number (but is only relevant for the pbkdf2 algorithm).