Issue #65 ValidateTwoFactorPin always returns false, if the secretKey parameter is base32 encoded string - fixed #66
Conversation
…the secretKey parameter is base32 encoded string - fixed Fix for issue # 65; Support for .net5; Small refactoring.
…f the secretKey parameter is base32 encoded string Added parameter secretIsBase32 into ValidateTwoFactorPIN
flytzen
left a comment
flytzen
left a comment
There was a problem hiding this comment.
Thank you for this and for fixing the issue.
I would prefer to leave {} around the body of if/else; it is more verbose but is recommended practice because it prevents subtle errors.
If at all possible, could you add a unit test that would have failed before the fix and passes after? Reason is, the code looks good, but I don't actually know if it works and a unit test would show that.
Thank you again, great work!
| { | ||
| throw new ArgumentNullException("input"); | ||
| } | ||
| throw new ArgumentNullException(nameof(input)); |
There was a problem hiding this comment.
Please don't remove {} here :)
| //if we didn't end with a full byte | ||
| if (arrayIndex != byteCount) | ||
| { | ||
| returnArray[arrayIndex] = curByte; |
There was a problem hiding this comment.
Please don't remove {} here
| { | ||
| throw new ArgumentNullException("input"); | ||
| } | ||
| throw new ArgumentNullException(nameof(input)); |
There was a problem hiding this comment.
Please don't remove {} here
| var value = (int)c; | ||
|
|
||
| //65-90 == uppercase letters | ||
| if (value < 91 && value > 64) |
There was a problem hiding this comment.
Please don't remove {} here
| } | ||
|
|
||
| //50-55 == numbers 2-7 | ||
| if (value < 56 && value > 49) |
There was a problem hiding this comment.
Please don't remove {} here
| } | ||
| else | ||
| { | ||
| // https://github.com/google/google-authenticator/wiki/Conflicting-Accounts |
There was a problem hiding this comment.
Please don't remove these comments
There was a problem hiding this comment.
Sorry, that was not on purpose. Returned it.
| { | ||
| result.Append(symbol); | ||
| } | ||
| if (validChars.IndexOf(symbol) == -1) |
There was a problem hiding this comment.
Please don't remove {} here
| { | ||
| result.Append('%' + String.Format("{0:X2}", (int)symbol)); | ||
| } | ||
| result.Append(symbol); |
There was a problem hiding this comment.
Please don't remove {} here
| bool secretIsBase32, | ||
| int digits = 6) | ||
| => GenerateHashedCode( | ||
| secretIsBase32 ? Base32Encoding.ToBytes(secret):Encoding.UTF8.GetBytes(secret), |
| string twoFactorCodeFromClient, | ||
| TimeSpan timeTolerance, | ||
| bool secretIsBase32 = false) => | ||
| GetCurrentPINs(accountSecretKey, timeTolerance, secretIsBase32).Any(c => c == twoFactorCodeFromClient); |
There was a problem hiding this comment.
I agree this is more compact, but I think it hurts legibility. I am not too bothered, but I prefer the previous form here.
…the secretKey parameter is base32 encoded string - fixes for review
|
|
||
| actual.ShouldBe(expected); | ||
| } | ||
|
|
|
@ahwm LGTM, you happy to merge? |
It will be great if the Nuget package is upgraded! Then I will be able to use it in our product ;) Thank you in advance! |
|
Good stuff here! |
I'm happy to merge. Yes the public API is being changed, but it's been done in a backwards-compatible way (parameters with a default value last in the list) so it's not going to break existing integrations that update to it. So I think we can get by with a minor version increment. So instead of 2.1.2 it would be 2.2.0. @flytzen @BrandonPotter if you're both good with that version number then I'll make the version edit and merge. |
|
@ahwm agreed, thank you 💯 |
|
Agreed! |
|
@awhm could you please update the Nuget package to include these changes? Thank you very much in advance. |
|
@caterina-novak pending #67 |
|
@caterina-novak NuGet is pushed and should be live in a short period. |
|
@ahwm Adam, thank you very much!
Best regards,Caterina mailto: ***@***.***
On Tuesday, 13 July 2021, 04:36:18 EEST, Adam ***@***.***> wrote:
@caterina-novak NuGet is pushed and should be live in a short period.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
4 participants
Fix for issue # 65;
Support for .net5;
Small refactoring.