Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Fix login failure #49

Merged
merged 2 commits into from
May 9, 2017
Merged

Fix login failure #49

merged 2 commits into from
May 9, 2017

Conversation

ChristophWurst
Copy link
Member

Supersedes #48
Fixes #45

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@Nibbels
Copy link
Collaborator

Nibbels commented May 9, 2017

Nice!
When you are ready some prerelease would be a great thing. I will test the new compilation then.

Greetings

@ChristophWurst ChristophWurst changed the title Fix/login failure Fix login failure May 9, 2017
@ChristophWurst
Copy link
Member Author

Works with all my keys on both FF and Chromium. Chromium apparently doesn't activate the u2f device instantly (the LED isn't blinking right away), it takes a few seconds until it's active.

@ChristophWurst
Copy link
Member Author

cc @go2sh

@ChristophWurst
Copy link
Member Author

You can use this zip for testing:
twofactor_u2f.tar.gz

@Nibbels
Copy link
Collaborator

Nibbels commented May 9, 2017

Ok!

Seems ok.

  • disabled the old app.
  • deleted the apps folder
  • deleted the apps database
  • uploaded the new files from the zip
  • activated the new app

I registered two PlugUp-Keys in Two Accounts (4 registrations)
(Different Account and same names for the USB-Keys or same USB-Keys in general did not make any collision.)

[Windows 10 64-bit]

  • Chrome Version 58.0.3029.96 (64-bit): All 4 Logins worked.
    screenshot_1
  • Firefox 53.0.2 (64-Bit): All 4 Logins worked.
  • Opera 44.0.0.251024618 (PGO): All 4 Logins worked.

I did not see anything bad..!
(As stated in #45 I was never able to see the original bug, i just didnt show up on my system.)

- 5.3
- 5.4
- 5.5
- 5.6
Copy link
Collaborator

@Nibbels Nibbels May 9, 2017

Choose a reason for hiding this comment

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

Why is the Line 5.6 removed here when 5.6 should be allowed.
It just blinked to my eye when I read through the changes. (Note that this comment can be absolutly idiotic, because I do not know the meaning of .travis.yml at all, just a hint to have a look to this again.
screenshot_2
)

Copy link
Member Author

Choose a reason for hiding this comment

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

5.6 is included via the matrix ;-)

It was basically run redundantly, so I removed one build.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@Nibbels Nibbels left a comment

Choose a reason for hiding this comment

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

All the PHP changes look good or have been tested before.

[I cannot tell anything about structural things, but they do not look bad from my point of view. (Changes which are not PHP).]

Copy link
Collaborator

@zauguin zauguin left a comment

Choose a reason for hiding this comment

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

Looks great and works on Chromium and Firefox for me.

@zauguin zauguin mentioned this pull request May 9, 2017
5 tasks
@ChristophWurst ChristophWurst merged commit c25601a into master May 9, 2017
@ChristophWurst ChristophWurst deleted the fix/login-failure branch May 9, 2017 10:32
@ChristophWurst
Copy link
Member Author

Thanks everyone for helping with this critical issue! I've packaged a nighly build https://github.com/nextcloud/twofactor_u2f/releases/tag/nightly-20170509

@coveralls
Copy link

Coverage Status

Coverage remained the same at 59.302% when pulling d59ab69 on fix/login-failure into 936f781 on master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants