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

Fix calculateAll #8

Closed
wants to merge 1 commit into from
Closed

Conversation

asiragusa
Copy link

The algorithm used in calculateAll is wrong, if you have multiple
tokens, some with the touch requirement and some without, the map
produced by calculateAll is not correct: the first n names have the
touchRequired, the following TOTP tokens from other keys. This PR
fixes this issue, using the appropriate data structures.

The algorithm used in calculateAll is wrong, if you have multiple
tokens, some with the touch requirement and some without, the map
produced by calculateAll is not correct: the first n names have the
touchRequired, the following TOTP tokens from other keys.
@yawn
Copy link
Owner

yawn commented May 29, 2019

Oops, good catch. Will you be able to produce a test case for this as well?

@asiragusa
Copy link
Author

I'm struggling to generate the bytecode of the mocks, maybe you can help me or you have a better tool to do it.

In your tests, the order of the credentials is the following:

name: test-test-test-test-s1-6-tr-1e5f2db9-477e-41af-bd2e-60bc569ae871
name: test-test-test-test-s2-6-tr-2a7cbca9-baef-47e3-8ce8-788bc6853e12
name: test-test-test-test-s5-6-tr-b01019ed-2af1-48cc-a64c-fa9b424db993
name: test-test-test-test-s1-8-tr-e62171f0-4cf6-499e-b988-6ef36b213cc6
name: test-test-test-test-s2-8-tr-458af9ee-caaa-4716-bfb8-bd828757955d
name: test-test-test-test-s5-8-tr-2138a991-ec70-48cb-83e6-f80da47c93e4
name: test-test-test-test-s1-6-nt-a70a2520-7e51-45b2-baab-0e35220b06fe
name: test-test-test-test-s2-6-nt-83fe3208-b192-46c2-9cb2-14ee917b4d60
name: test-test-test-test-s5-6-nt-cc9d122e-9b51-435e-b48e-ab1a17157e3c
name: test-test-test-test-s1-8-nt-97a58938-8ea6-4143-ae10-8adb92bdc335
name: test-test-test-test-s2-8-nt-887fd38b-80b3-4d7a-8671-82bef63151a6
name: test-test-test-test-s5-8-nt-daee50d1-7bbf-41e6-a65b-d34046dba287

You can generate a test by putting a touch required credential at the end of this list and running Calculate on it.

@yawn
Copy link
Owner

yawn commented May 31, 2019

I'm on vacation now so I'll get back to it in ~14days.

@j0hnsmith
Copy link
Contributor

Any updates?

@yawn
Copy link
Owner

yawn commented Jul 2, 2019

Sorry, work/live interference. I am having a look today/this week.

yawn added a commit that referenced this pull request Jul 2, 2019
This should hopefully unblock the test in #8.
@yawn
Copy link
Owner

yawn commented Jul 2, 2019

Sorry for making you wait @j0hnsmith & @asiragusa. I've committed the internal dump tool I've used for the previous tests.

If your drop a Dump() here and here you should be able to produce the units tests easily.

Apart from that the PR looks good to me. Could be released tomorrow if you can quickly contribute the tests ...

yawn added a commit that referenced this pull request Oct 13, 2019
This should fix the issue from PR #8 while using a single datastructure instead of two.
@yawn
Copy link
Owner

yawn commented Oct 13, 2019

Closed through 4e2c7c9 while also changing all commands to use the new structure. Thanks again @asiragusa & @j0hnsmith, took me a long time to find the time to fix this properly.

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