Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ValidateTwoFactorPIN with iterationOffset as parameter #150

Merged
merged 2 commits into from
May 1, 2023

Conversation

RomanSoloweow
Copy link
Contributor

I need to allow code with iterationOffset == 1. But with the current implementation, it's non-obvious how to do this.

If set timeTolerance to 30 seconds, I get iterationOffset 0.
If set timeTolerance to 60 seconds, I get iterationOffset 2.
To obtain iterationOffset == 1, I need to set the time between 30 and 60 seconds. It's strange.

if (timeTolerance.TotalSeconds > 30)
    iterationOffset = Convert.ToInt32(timeTolerance.TotalSeconds / 30.00)

I don't want to break compatibility, so let's just allow the user to specify iterationOffset.

@RomanSoloweow
Copy link
Contributor Author

@flytzen @ahwm What are you think about it?

@flytzen
Copy link
Collaborator

flytzen commented Apr 29, 2023

Thank you, will take a look 👍

@RomanSoloweow
Copy link
Contributor Author

@flytzen I will be very grateful if we can release a new version with this fix as soon as possible

@ahwm
Copy link
Collaborator

ahwm commented May 1, 2023

This looks good to me. I am curious why ConvertSecretToBytes is now public. There are various overloads that accept a byte[] parameter for the secret. Having it public isn't a problem from what I can tell but I am curious about the motivation for the change.

@RomanSoloweow
Copy link
Contributor Author

This looks good to me. I am curious why ConvertSecretToBytes is now public.

To support secretkey in two formats, you have many overloads. It would be much more convenient to give the user a method to translate from one format to another.

These were my thoughts in general, and I think they don't apply to the current request, so I'll revert to the previous one.

@ahwm ahwm merged commit 117ec8f into BrandonPotter:master May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants