Skip to content

Fix calculateAll#8

Closed
asiragusa wants to merge 1 commit intoyawn:masterfrom
processout:fix/calculateAll
Closed

Fix calculateAll#8
asiragusa wants to merge 1 commit intoyawn:masterfrom
processout:fix/calculateAll

Conversation

@asiragusa
Copy link
Copy Markdown

@asiragusa asiragusa commented May 17, 2019

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.

@asiragusa
Fix calculateAll
ab3f803 
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.
@asiragusa asiragusa mentioned this pull request May 27, 2019
Merged
@yawn
Copy link
Copy Markdown
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
Copy Markdown
Author

asiragusa commented May 29, 2019

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
Copy Markdown
Owner

yawn commented May 31, 2019

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

@j0hnsmith
Copy link
Copy Markdown
Contributor

j0hnsmith commented Jun 24, 2019

Any updates?

@yawn
Copy link
Copy Markdown
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
@yawn
Added internal dump() function.
c3e1950 
This should hopefully unblock the test in #8.
@yawn
Copy link
Copy Markdown
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
@yawn
Stop being cute with the datastructure.
4e2c7c9 
This should fix the issue from PR #8 while using a single datastructure instead of two.
@yawn
Copy link
Copy Markdown
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.

@yawn yawn closed this Oct 13, 2019
@yawn yawn mentioned this pull request Feb 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@asiragusa @yawn @j0hnsmith