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

Add counter value when triggering ROTP#provisioning_uri #76

Merged

Conversation

wenderjean
Copy link
Contributor

@wenderjean wenderjean commented Jan 23, 2021

Related to: #74

Changes

  • Add self.otp_counter when calling ROTP#provisioning_uri in order to have it on the final URI

Concerns

  • Even though it seems somethings that never caused issues, reading through the RFC-4226 I noticed some requirements on keeping the counter synchronized between server/client, please, let me know your thoughts regarding that.

Note.: Taking a look at the source it doesn't seem we're caring about the houndci-bot for line length metrics. Did we enable houndci-bot recently?

test/one_time_password_test.rb Outdated Show resolved Hide resolved
test/one_time_password_test.rb Outdated Show resolved Hide resolved
test/one_time_password_test.rb Outdated Show resolved Hide resolved
lib/active_model/one_time_password.rb Outdated Show resolved Hide resolved
Base automatically changed from master to main March 7, 2021 10:58
@pedrofurtado
Copy link
Collaborator

pedrofurtado commented Apr 6, 2021

Hi @wenderjean ! 👋

Thanks for your contribution (and delayed review of my part) 🤝 🍻

That's a great fix! In fact, when we generate the provisioning uri with otp counter previously used (and then with a value greater than zero), the authenticator app and server/backend must be in sync (i.e., with the same counter value).

Please, could you fix the CI warnings and file conflicts? After that, @robertomiranda , I think we are ready to merge it ✅ 🏁

@wenderjean wenderjean force-pushed the bugfix/add-counter-to-hotp-uri branch from 298c93e to bfc030e Compare April 6, 2021 12:26
test/one_time_password_test.rb Outdated Show resolved Hide resolved
test/one_time_password_test.rb Outdated Show resolved Hide resolved
test/one_time_password_test.rb Outdated Show resolved Hide resolved
test/one_time_password_test.rb Outdated Show resolved Hide resolved
@wenderjean wenderjean force-pushed the bugfix/add-counter-to-hotp-uri branch from bfc030e to 09e44e4 Compare April 6, 2021 12:34
test/one_time_password_test.rb Outdated Show resolved Hide resolved
test/one_time_password_test.rb Outdated Show resolved Hide resolved
test/one_time_password_test.rb Outdated Show resolved Hide resolved
test/one_time_password_test.rb Outdated Show resolved Hide resolved
test/one_time_password_test.rb Show resolved Hide resolved
@wenderjean wenderjean force-pushed the bugfix/add-counter-to-hotp-uri branch from 09e44e4 to c0e6775 Compare April 6, 2021 12:43
@wenderjean
Copy link
Contributor Author

I'll take a look on tests asap guys, I'm using a different PC right now for that reason I'm getting those issues.

@pedrofurtado
Copy link
Collaborator

pedrofurtado commented Apr 6, 2021

Thanks a lot @wenderjean 🍻 Let us know when you got tests working well 🤝 We will be glad to review it

Thanks again for your effort on it 🥇

@wenderjean wenderjean force-pushed the bugfix/add-counter-to-hotp-uri branch from c0e6775 to 7af39ca Compare April 7, 2021 11:37
assert_match %r{^otpauth://totp/Example\:roberto\?secret=\w{32}&issuer=Example$}, @visitor.provisioning_uri("roberto", issuer: "Example")
account = %r{
^otpauth://totp/Example\:roberto\?secret=\w{32}&issuer=Example$
}x
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trick for multi-line regex content: }x

@wenderjean
Copy link
Contributor Author

All good @pedrofurtado :)

@pedrofurtado
Copy link
Collaborator

pedrofurtado commented Apr 14, 2021

Thank you for your effort on it, @wenderjean ! 🤝

Our team is planning (especially me and @robertomiranda) to release this and some other improvements/bugfixes in a new release soon 🎉

@robertomiranda robertomiranda merged commit cac2990 into heapsource:main May 7, 2021
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.

None yet

4 participants