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

Test verification of TOTP through webui #98

Merged
merged 1 commit into from
Feb 11, 2019
Merged

Conversation

skshetry
Copy link
Member

This adds a test for verification of two factor key on the webui. Previously, #88 tested for cli command twofactor_totp for verification.

@codecov
Copy link

codecov bot commented Jan 29, 2019

Codecov Report

Merging #98 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master     #98   +/-   ##
========================================
  Coverage      62.6%   62.6%           
  Complexity       57      57           
========================================
  Files            12      12           
  Lines           230     230           
========================================
  Hits            144     144           
  Misses           86      86

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02894ef...9400b95. Read the comment docs.

@skshetry
Copy link
Member Author

@skshetry skshetry changed the title Test verfiication of TOTP through webui Test verfication of TOTP through webui Jan 29, 2019
@skshetry skshetry changed the title Test verfication of TOTP through webui Test verfiication of TOTP through webui Jan 29, 2019
@skshetry skshetry changed the title Test verfiication of TOTP through webui Test verification of TOTP through webui Jan 29, 2019
@karakayasemi
Copy link
Contributor

@karakayasemi Is this the correct method or is there any better way? Or, try it twice?
twofactor_totp/tests/acceptance/features/bootstrap/TwoFactorTOTPContext.php

Lines 105 to 118 in 09603d7

public function theUserAddsVerficationKeyFromSecretKeyUsingWebUI() {
$remainder = \time() % 30;
if ($remainder % 30 < 10) {
// When we reach the step to add $key, the key might have already changed.
// Let's sleep for a while and then get the key and fill.
// Hopefully, it will be completed within 10 seconds.
$message = "INFORMATION: Hibernating for $remainder seconds.";
echo $message;
\error_log($message);
\sleep($remainder);
}

  $this->personalSecuritySettingsPage->addVerificationKey($this->generateTOTPKey()); 

}

@skshetry as far as I understand, your concern is the key change can cause unwanted fails.
Luckily, we are accepting before and after 3 timeshifts when validating secret as you can see in here https://github.com/owncloud/twofactor_totp/blob/master/lib/Service/Totp.php#L120.
This means every generated key is valid for minimum 90 seconds and maximum 120 seconds. So, the key change should not be a problem.

$verificationMsg,
__METHOD__ . ' The verification msg could not be found'
);
return $verificationMsg->getText() === 'Verified';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return $verificationMsg->getText() === 'Verified';
return ($verificationMsg->getText() === 'Verified');

else {
throw new \Exception(
"TOTP Already used." .
"Multiple key generation not supported due to possibility of same key generation."
Copy link
Member

Choose a reason for hiding this comment

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

please make shorter

@skshetry skshetry force-pushed the add-acceptance-webui branch 2 times, most recently from a325bbb to c490a9c Compare February 8, 2019 11:23
@phil-davis phil-davis merged commit aa9ff07 into master Feb 11, 2019
@delete-merged-branch delete-merged-branch bot deleted the add-acceptance-webui branch February 11, 2019 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants