verifyFunc() callback credentials arg isn't optional as expected

[jmm]

The credentials arg to the verifyFunc() callback is documented as optional like for validateFunc(), but it's not actually. Here's a branch with that arg deleted from the relevant test code, causing failures.

The internal code for verifyFunc() doesn't default to the already decoded credentials like validateFunc() does: credentials || decoded.

Most of the work to fix this is in refactoring the test code to test both scenarios.

[nelsonic]

@jmm well spotted! this is a Documentation issue. credentials are indeed required for verifyFunc ... do you have time to PR a the Doc Change? or is it preferable to update the tests? (thanks!)

[jmm]

Thanks @nelsonic! Ok, I see. My 2 cents is that I don't feel very strongly either way about credentials being required or optional for either validateFunc or verifyFunc (though it would be bc-breaking to make it required for validateFunc, of course), but I lean a bit toward it being consistent by being either optional or required in both cases.

On one hand, omitting it is a very small convenience in either case. On the other hand, I don't see a problem with allowing it to be optional on verifyFunc because you have to pass a separate valid arg anyway. So if you callback like callback(null, true), I think that's a reasonable expression that you consider the decoded credentials valid. Do you see a particular hazard to that approach that I'm overlooking?

So it's up to you. If you think it should be optional for verifyFunc then that should be fixed and the tests updated to cover it, otherwise it can just be corrected in the docs like you said.

[nelsonic]

Yeah, I think my preference would be credentials || decoded so that it is optional.
Need to add another test in that case...