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 2fa backup codes app #1171

Merged
merged 3 commits into from
Sep 5, 2016
Merged

add 2fa backup codes app #1171

merged 3 commits into from
Sep 5, 2016

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Aug 29, 2016

integrates https://github.com/ChristophWurst/twofactor_backupcodes and fixes #1108

  • App is always enabled, so users can create backup codes even before an admin randomly enables/enforces a 2FA provider
  • Does not enforce 2FA is this app is the only active 2FA provider
  • Does not show up in the list of selectable 2FA providers
  • Adds a "Use backup code" link underneath the 2FA selection page and 2FA challenge page

TODO:

  • Unit/integration tests for the twofactor_backupcodes app
  • Fix/add tests for the changed server classes

bildschirmfoto von 2016-08-29 19-13-25

@nextcloud/designers any idea how we could make the settings dialog prettier? See the screenshots in https://github.com/ChristophWurst/twofactor_backupcodes for how it looks now.

@mention-bot
Copy link

@ChristophWurst, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bantu, @icewind1991 and @DeepDiver1975 to be potential reviewers

@@ -20,3 +20,11 @@
</p>
</div>
<a class="two-factor-cancel" <?php print_unescaped($_['logout_attribute']); ?>><?php p($l->t('Cancel log in')) ?></a>
<?php if (!is_null($_['backupProvider'])): ?>
<a class="two-factor-cancel" href="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.TwoFactorChallenge.showChallenge',
Copy link
Member Author

@ChristophWurst ChristophWurst Aug 29, 2016

Choose a reason for hiding this comment

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

  • TODO: rename class

@ChristophWurst
Copy link
Member Author

/usr/local/bin/phpunit --configuration phpunit-autotest.xml --exclude-group DB --log-junit autotest-results-sqlite.xml  
PHP Parse error:  syntax error, unexpected 'class' (T_CLASS), expecting identifier (T_STRING) or variable (T_VARIABLE) or '{' or '$' in /drone/src/github.com/nextcloud/server/tests/Core/Command/Config/App/DeleteConfigTest.php on line 46
[info] build failed (exit code 255)

unrelated?

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 2, 2016
@ChristophWurst
Copy link
Member Author

@rullzer @LukasReschke @nicohauger @zkwoob @Alphakilo @jancborchardt @Mar1u5 review and test this please.

The personal settings dialog definitely needs some UI/UX enhancements, that I'd like to postpone to follow-up PRs as this one is already rather big.

@ChristophWurst
Copy link
Member Author

Does PHPUnit automatically find the test of the newly added app or do I have to register them somewhere?

@MorrisJobke
Copy link
Member

$ NOCOVERAGE=true TEST_SELECTION=DB ./autotest.sh sqlite
Using PHP executable /usr/bin/php
Parsing all files in lib/public for the presence of @since or @deprecated on each method...

Using database oc_autotest
Setup environment for sqlite testing on local storage ...
Installing ....
Nextcloud is not installed - only a limited number of commands are available
creating sqlite db
Nextcloud was successfully installed
Testing with sqlite ...
No coverage
/usr/local/bin/phpunit --configuration phpunit-autotest.xml --group DB --log-junit autotest-results-sqlite.xml  
PHPUnit 4.8.24 by Sebastian Bergmann and contributors.

Runtime:    PHP 5.6.22-0+deb8u1
Configuration:  /drone/src/github.com/nextcloud/server/tests/phpunit-autotest.xml
Warning:    Deprecated configuration setting "strict" used

........S....................................................   61 / 4240 (  1%)
PHP Fatal error:  Call to a member function getUpdater() on null in /drone/src/github.com/nextcloud/server/lib/private/Files/View.php on line 695
[info] build failed (exit code 255)

@ChristophWurst
Copy link
Member Author

let me try to reproduce the failing tests locally…

@ChristophWurst
Copy link
Member Author

@MorrisJobke what are the requirements/preconditions to run all tests locally? I get Tests: 4716, Assertions: 12160, Errors: 1359, Failures: 13, Skipped: 286. :-(

@MorrisJobke
Copy link
Member

@MorrisJobke what are the requirements/preconditions to run all tests locally?

drone exec

(see http://readme.drone.io/devs/cli/)

Best is also to remove the running tests from .drone.yml this speeds up the test execution ;)

@ChristophWurst
Copy link
Member Author

stack trace:

Call to a member function getUpdater() on null in /server/lib/private/Files/View.php on line 696
#0  OC\Files\View->unlink() called at [/server/lib/private/Cache/File.php:146]
#1  OC\Cache\File->remove() called at [/server/tests/lib/Cache/FileCacheTest.php:91]
#2  Test\Cache\FileCacheTest->tearDown() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:805]
#3  PHPUnit_Framework_TestCase->runBare() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestResult.php:612]
#4  PHPUnit_Framework_TestResult->run() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:724]
#5  PHPUnit_Framework_TestCase->run() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:747]
#6  PHPUnit_Framework_TestSuite->run() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:747]
#7  PHPUnit_Framework_TestSuite->run() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/TestRunner.php:440]
#8  PHPUnit_TextUI_TestRunner->doRun() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:149]
#9  PHPUnit_TextUI_Command->run() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:100]
#10 PHPUnit_TextUI_Command::main() called at [/usr/local/bin/phpunit:548]

* add backup codes app unit tests
* add integration tests for the backup codes app
@rullzer
Copy link
Member

rullzer commented Sep 5, 2016

Some dark magic. Seems something was not cleaned up properly or so which caused the whole thing to break down.

CI seems a lot happier now ;)

@ChristophWurst
Copy link
Member Author

Thank you very much @rullzer 🚀

@rullzer
Copy link
Member

rullzer commented Sep 5, 2016

CI happy.
Tested in combination with TOTP and worked like a charm.
Code looks good! LGTM!

@MariusBluem
Copy link
Member

Works 👍

@MariusBluem MariusBluem merged commit f8eb7be into master Sep 5, 2016
@MariusBluem MariusBluem deleted the 2fa-backup-codes branch September 5, 2016 10:17
<licence>agpl</licence>
<author>Christoph Wurst</author>
<version>1.0.0</version>
<namespace>TwoFactor_BackupCodes</namespace>
Copy link
Member

Choose a reason for hiding this comment

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

Please, no underscores in PSR-4 Namespaces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2FA: let users create and authenticate via backup codes
7 participants