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

Why is label required for the constructor? #60

Closed
lukos opened this issue Dec 6, 2016 · 6 comments
Closed

Why is label required for the constructor? #60

lukos opened this issue Dec 6, 2016 · 6 comments
Assignees
Labels

Comments

@lukos
Copy link

lukos commented Dec 6, 2016

When only verifying a TOTP code, it doesn't look like label is needed anywhere, yet it is a required constructor parameter for the TOTP class.

This means passing in a blank string or random junk for the sake of making it happy.

Can this be changed to make it neater? Otherwise a simple static function could be use that eliminates the need to construct a TOTP object just to call verify.

@Spomky
Copy link
Member

Spomky commented Dec 7, 2016

Hi @lukos,

You are right: the label is only needed when the provisioning URI is computed.
Depending on the schemen (TOTP/HOTP), you just need the secret, the digest, the digits and additional parameters required by the scheme (counter, interval).

I will not add a static method, from my POV it should be easier to modify the setLabel method by allowing a null label and verify that label in the generateURI one.

WDYT?

@Spomky Spomky added the DX label Dec 7, 2016
@Spomky Spomky self-assigned this Dec 7, 2016
@Spomky Spomky closed this as completed in #61 Dec 8, 2016
@Spomky
Copy link
Member

Spomky commented Dec 8, 2016

Tagged v8.3.0

@lukos
Copy link
Author

lukos commented Dec 8, 2016

That is OK but it isn't much better than just passing an empty string to the constructor. The ideal (of course!) is that the constructor should not require the label since it is not conceptually required for a "TOTP" object to be complete but that is obviously not a quick easy fix.

You could also change it so the constructor takes an array of options so you can fill in whatever you want but that is also a breaking change. Thanks anyway.

@Spomky
Copy link
Member

Spomky commented Dec 8, 2016

I agree with you but if I change the constructor it will creates a BC break.
So for the moment and to fix it quickly I found that solution.

In the next major release, I think the constructor will be private and object will be created through static methods only.

@Spomky
Copy link
Member

Spomky commented Dec 8, 2016

I have just created a new branch that implements the static methods I mentioned above.

Example:

<?php

use OTPHP\TOTP;

$totp = TOTP::createTOTP(); //Creates a TOTP object with a random secret and digest='sha1', period=30 and digits=6
$totp = TOTP::createTOTP('JDDK4U6G3BJLEZ7Y', 20, 'sha512', 8); // Creates a TOTP object with selected parameters

WDYT?

@lukos
Copy link
Author

lukos commented Dec 8, 2016

mmmmmmm Factory methods. Nice

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

No branches or pull requests

2 participants