Skip to content

Conversation

browner12
Copy link

Target branch: 11.3.x
Resolves issue: none

  • It is a Bug fix
  • It is a New feature
  • Breaks BC
  • Includes Deprecations

this will allow the user to set their own secret length, but allow the package to still handle the creation of the string. I think this is a better interface for the user, as this is most likely a large chunk of the reasons someone would want to have a custom secret over the default.

It also helps avoid a (likely) implicit dependency in user code. We are currently using TOTP::createFromSecret(Base32::encode(Str::random(16)))->getSecret() to generate a shorter random secret than the default. Here we are using the Base32 class from ParagonIE, even though it is not an explicit dependency in our code. We probably should be, rather than implicitly depend on it. I'm guessing other people may make this mistake as well.

If there's a better architecture way to handle this, open to suggestions.

this will allow the user to set their own secret length, but allow the package to still handle the creation of the string.  I think this is a better interface for the user, as this is most likely a large chunk of the reasons someone would want to have a custom secret over the default.

It also helps avoid a (likely) implicit dependency in user code. We are currently using `TOTP::createFromSecret(Base32::encode(Str::random(16)))->getSecret()` to generate a shorter random secret than the default. Here we are using the `Base32` class from `ParagonIE`, even though it is not an explicit dependency in our code. We probably should be, rather than implicitly depend on it. I'm guessing other people may make this mistake as well.
@Spomky
Copy link
Member

Spomky commented Jun 12, 2024

Hi,

Many thanks for the suggestion.
However, I do not see any benefits between the existing creation method TOTP::createFromSecret(random_bytes(16)); and TOTP::generate(16); except the line size (not so long to me).

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.

2 participants