do not allow multiple uses of the same key #61
Conversation
|
will this only store the last token or multiple of the past? |
|
Once this is merged - we need to setup a prior version ( before mandatory confirmation of otp secret ) and play upgrades to ensure the migrations work correctly and do not lock out users |
|
tested. this fixes the issue, by saving the last token, still its possible to use the pre-last token to login again (obviously within the time limits) |
|
Hmm, we are also validating before and next token. https://github.com/owncloud/twofactor_totp/blob/master/lib/Service/Totp.php#L115 I thought only last used token is enough. Actually, since we are implementing this for the case of violation of user-server communication privacy, preventing consecutive usage of the same token is enough. IMHO, it is not a problem. |
|
@PVince81 @DeepDiver1975 if you have a minute during the conference you maybe could have a look here |
| @@ -39,6 +42,9 @@ class TotpSecret extends Entity { | |||
| /** @var string */ | |||
| protected $secret; | |||
|
|
|||
| /** @var string */ | |||
| protected $lastValidatedKey; | |||
There was a problem hiding this comment.
the file was indented with 4 spaces instead of tab. I opened an issue for php-cs-fixer #62
| $secret = $this->crypto->decrypt($dbSecret->getSecret()); | ||
| $otp = new Otp(); | ||
|
|
||
| if ($otp->checkTotp(Base32::decode($secret), $key, 3) === true) { |
There was a problem hiding this comment.
add some PHPDoc comment, it is not clear what and why we are doing this
if really not possible, please make a technical debt ticket and explain what needs to be refactored is this at least covered by acceptance tests ? @individual-it |
|
@PVince81 |
|
so we have at least four options:
|
thoughts ? |
|
@PVince81 let me refactor it, I will make it today, after work hours. |
8e950a9 to
b20b8a3
Compare
b20b8a3 to
f1ece01
Compare
|
I converted spaces to tabs in the files to follow ownCloud code style, now it became difficult to review. I am closing this PR, and creating new PR with the app's code style. We can fix cs mistakes by introducing php-cs-fixer later. Let's continue in here: #63 |
resolves #59 .
I tested it in my local. I would like to add unit test for Totp service, but, IMHO it needs some bigger refactoring. There is an object that is violating dependency injection.