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 for two factor authentication #131

Merged
merged 2 commits into from
Jul 17, 2019

Conversation

bhawanaprasain
Copy link
Contributor

Description

The result of last verification request made using OTP can be checked via API.

Related Issue

#38

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

@bhawanaprasain
Copy link
Contributor Author

@dpakach @skshetry @paurakhsharma please review on my PR.

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #131   +/-   ##
=========================================
  Coverage     64.68%   64.68%           
  Complexity       61       61           
=========================================
  Files            13       13           
  Lines           252      252           
=========================================
  Hits            163      163           
  Misses           89       89

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 9dad10b...207901e. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #131   +/-   ##
=========================================
  Coverage     64.68%   64.68%           
  Complexity       61       61           
=========================================
  Files            13       13           
  Lines           252      252           
=========================================
  Hits            163      163           
  Misses           89       89

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 9dad10b...3dfee2c. Read the comment docs.

* @return void
*/
public function sendRequestWithSecretKey($user, $secretKey) {
$response = \TestHelpers\OcsApiHelper::sendRequest(
Copy link
Member

Choose a reason for hiding this comment

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

import OcsApiHelper into the namespace

}

/**
* @When the administrator tries to verify the otp key for user :user using the correct key
Copy link
Member

@skshetry skshetry Jul 16, 2019

Choose a reason for hiding this comment

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

Suggested change
* @When the administrator tries to verify the otp key for user :user using the correct key
* @When the administrator tries to verify with the one-time key generated from the secret key for user :user

}

/**
* @When the administrator tries to verify the otp key for user :user using the wrong key
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
* @When the administrator tries to verify the otp key for user :user using the wrong key
* @When the administrator tries to verify with the random key :key for user :user

Copy link
Member

Choose a reason for hiding this comment

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

Or, maybe this:

Suggested change
* @When the administrator tries to verify the otp key for user :user using the wrong key
* @When the administrator tries to verify with the random key for user :bust_in_silhouette:

I do prefer the first one though.

*
* @return void
*/
public function theAdministratorTriesToVerifyTheOtpKeyForUserAUserThatDoesNotExist() {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this step? You could achieve the same thing with above steps as well

*
* @return void
*/
public function theAdministratorTriesToVerifyTheOtpKeyForUserUsingTheWrongKey($user) {
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
public function theAdministratorTriesToVerifyTheOtpKeyForUserUsingTheWrongKey($user) {
public function theAdministratorTriesToVerifyTheOtpKeyForUserUsingTheWrongKey($user, $key) {

Also, you need to change docs, and the body of the function to use the Skey.

Given user "user0" has logged in using the webUI
And the user has browsed to the personal security settings page
And the user has activated TOTP Second-factor auth but not verified
When the administrator tries to verify with the random key "random" for user "user0"
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
When the administrator tries to verify with the random key "random" for user "user0"
When the administrator tries to verify with the invalid key "random" for user "user0"

@bhawanaprasain
Copy link
Contributor Author

@individual-it @phil-davis please review again

@individual-it individual-it merged commit 47aff5b into owncloud:master Jul 17, 2019
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