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
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 :)
| @@ -48,9 +46,7 @@ public static byte[] ToBytes(string input) | |||
|
|
|||
| //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
| @@ -101,40 +95,32 @@ public static string ToString(byte[] input) | |||
|
|
|||
| private static int CharToValue(char c) | |||
| { | |||
| int value = (int)c; | |||
| 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
| return value - 65; | ||
| } | ||
|
|
||
| //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
| @@ -21,5 +22,22 @@ public void BasicAuthCodeTest() | |||
|
|
|||
| 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.
|
Fix for issue # 65;
Support for .net5;
Small refactoring.