Fix scram.*Client.Step so it returns an "ok" flag consistently

[momeni]

The package documentation asks to call Step method as while as it returns true, so it has to return false only on the first error or if all expected steps were taken previously.
This condition was implemented correctly in the first if-condition where it returned false when there were no more steps or an error was detected.
However, the ultimate return statement and the method godoc used to return false as while as the Step should be called again. This commit resolves this inconsistency and updates the relevant godocs.

-       return c.step > 2 || c.err != nil
+       return c.step <= 2 && c.err == nil

The RFC 5802 example test case is added too.

[arp242]

Does this fix a bug or change the function? This is an exported function, so changing it is a non-starter as it's not a compatible change.

[momeni]

It is a bugfix. Function has two return statements which were coded inconsistently.

[arp242]

Okay. What does it fix? Because the test you added also passes without this change.

scram.go was added in #833, and was taken from https://github.com/go-mgo/mgo/blob/v2/internal/scram/scram.go (also used in the fork https://github.com/globalsign/mgo/blob/master/internal/scram/scram.go). It seems unlikely that it can be used in three different projects for over 10 years and be completely wrong?

[arp242]

Closing this as I'm not really confident this is correct; the scram package is an ad-hoc untested implementation, but it does work, has worked for over 10 years, and I'm hesitant to change it.

I'll also add that using the scram package outside of pq is completely unsupported. This really should have been in internal/scram. When I replace it at some point (#914) I'll mark the entire package as deprecated and will remove it in v2.

[momeni]

I added a test case which fails with old code in 7751823 commit.
At first step, c.step is zero and becomes one by c.step++ statement. So final return c.step > 2 || c.err != nil statement will return false. Therefore, for client.Step(in) { will break early and second step cannot be executed.

Current usage of client.Step(in) does not trigger that bug because it is not called in a for loop. Instead, file conn.go lines 1305, 1327, and 1347 call the sc.Step(nextStep) three times and ignore its boolean return value.