Skip to content

Commit c90c685

Browse files
schebscheb
committed
Make getGoogleAuthenticatorUsername and getTotpAuthenticationUsername nullable, #293
No need to return an empty string, can return null to have a record with issuer only in the TOTP app.
1 parent 4e6debf commit c90c685

File tree

7 files changed

+51
-27
lines changed

7 files changed

+51
-27
lines changed

‎doc/providers/google.rst‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ Authenticator for a user, generate a secret code and persist it with the user en
7676
return null !== $this->googleAuthenticatorSecret;
7777
}
7878
79-
public function getGoogleAuthenticatorUsername(): string
79+
public function getGoogleAuthenticatorUsername(): ?string
8080
{
8181
return $this->username;
8282
}
@@ -114,7 +114,7 @@ Authenticator for a user, generate a secret code and persist it with the user en
114114
return null !== $this->googleAuthenticatorSecret;
115115
}
116116
117-
public function getGoogleAuthenticatorUsername(): string
117+
public function getGoogleAuthenticatorUsername(): ?string
118118
{
119119
return $this->username;
120120
}

‎src/google-authenticator/Model/Google/TwoFactorInterface.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ public function isGoogleAuthenticatorEnabled(): bool;
1414
/**
1515
* Return the user name. This is used in QR code generation to display the username in the TOTP app.
1616
*/
17-
public function getGoogleAuthenticatorUsername(): string;
17+
public function getGoogleAuthenticatorUsername(): string|null;
1818

1919
/**
2020
* Return the Google Authenticator secret

‎src/google-authenticator/Security/TwoFactor/Provider/Google/GoogleTotpFactory.php‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use Psr\Clock\ClockInterface;
1010
use Scheb\TwoFactorBundle\Model\Google\TwoFactorInterface;
1111
use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Exception\TwoFactorProviderLogicException;
12+
use function assert;
1213
use function strlen;
1314

1415
/**
@@ -34,13 +35,22 @@ public function createTotpForUser(TwoFactorInterface $user): TOTPInterface
3435
/** @psalm-suppress ArgumentTypeCoercion */
3536
$totp = TOTP::create($secret, 30, 'sha1', $this->digits, clock: $this->clock);
3637

37-
$userAndHost = $user->getGoogleAuthenticatorUsername().(null !== $this->server && $this->server ? '@'.$this->server : '');
38+
$usernameLabel = $user->getGoogleAuthenticatorUsername() ?? '';
39+
$serverLabel = $this->server ?? '';
40+
$userAndHost = $usernameLabel.('' !== $usernameLabel && '' !== $serverLabel ? '@' : '').$serverLabel;
3841
if ('' !== $userAndHost) {
3942
$totp->setLabel($userAndHost);
4043
}
4144

42-
if (null !== $this->issuer && $this->issuer) {
45+
if (null !== $this->issuer && '' !== $this->issuer) {
4346
$totp->setIssuer($this->issuer);
47+
48+
// Omit the issuer parameter, when the issuer is the only value set.
49+
// Otherwise FreeOTP app will show the issuer name twice.
50+
if (null === $totp->getLabel()) {
51+
$totp = $totp->withIssuerIncludedAsParameter(false);
52+
assert($totp instanceof TOTP);
53+
}
4454
}
4555

4656
return $totp;

‎src/totp/Model/Totp/TwoFactorInterface.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ public function isTotpAuthenticationEnabled(): bool;
1515
* Return the user name. This is used in QR code generation to display the username in the TOTP app. Return an
1616
* empty string, if you don't want it to show up in the app.
1717
*/
18-
public function getTotpAuthenticationUsername(): string;
18+
public function getTotpAuthenticationUsername(): string|null;
1919

2020
/**
2121
* Return the configuration for TOTP authentication.

‎src/totp/Security/TwoFactor/Provider/Totp/TotpFactory.php‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use Psr\Clock\ClockInterface;
1010
use Scheb\TwoFactorBundle\Model\Totp\TwoFactorInterface;
1111
use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Exception\TwoFactorProviderLogicException;
12+
use function assert;
1213
use function strlen;
1314

1415
/**
@@ -48,13 +49,22 @@ public function createTotpForUser(TwoFactorInterface $user): TOTPInterface
4849
clock: $this->clock,
4950
);
5051

51-
$userAndHost = $user->getTotpAuthenticationUsername().(null !== $this->server && $this->server ? '@'.$this->server : '');
52+
$usernameLabel = $user->getTotpAuthenticationUsername() ?? '';
53+
$serverLabel = $this->server ?? '';
54+
$userAndHost = $usernameLabel.('' !== $usernameLabel && '' !== $serverLabel ? '@' : '').$serverLabel;
5255
if ('' !== $userAndHost) {
5356
$totp->setLabel($userAndHost);
5457
}
5558

56-
if (null !== $this->issuer && $this->issuer) {
59+
if (null !== $this->issuer && '' !== $this->issuer) {
5760
$totp->setIssuer($this->issuer);
61+
62+
// Omit the issuer parameter, when the issuer is the only value set.
63+
// Otherwise FreeOTP app will show the issuer name twice.
64+
if (null === $totp->getLabel()) {
65+
$totp = $totp->withIssuerIncludedAsParameter(false);
66+
assert($totp instanceof TOTP);
67+
}
5868
}
5969

6070
foreach ($this->customParameters as $key => $value) {

‎tests/Security/TwoFactor/Provider/Google/GoogleTotpFactoryTest.php‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,13 @@ class GoogleTotpFactoryTest extends TestCase
2222
private const int CUSTOM_DIGITS = 8;
2323
private const int DEFAULT_DIGITS = 6;
2424

25-
private function createUserMock(string|null $secret = self::SECRET): MockObject&TwoFactorInterface
25+
private function createUserMock(string|null $secret = self::SECRET, string|null $username = self::USER_NAME): MockObject&TwoFactorInterface
2626
{
2727
$user = $this->createMock(TwoFactorInterface::class);
2828
$user
2929
->expects($this->any())
3030
->method('getGoogleAuthenticatorUsername')
31-
->willReturn(self::USER_NAME);
31+
->willReturn($username);
3232
$user
3333
->expects($this->any())
3434
->method('getGoogleAuthenticatorSecret')
@@ -70,9 +70,9 @@ public function createTotpForUser_factoryCalled_returnTotpObject(): void
7070

7171
#[Test]
7272
#[DataProvider('provideHostnameAndIssuer')]
73-
public function getProvisioningUri_hostnameAndIssuerGiven_returnProvisioningUri(string|null $hostname, string|null $issuer, int $digits, string $expectedUrl): void
73+
public function getProvisioningUri_hostnameAndIssuerGiven_returnProvisioningUri(string|null $username, string|null $hostname, string|null $issuer, int $digits, string $expectedUrl): void
7474
{
75-
$user = $this->createUserMock();
75+
$user = $this->createUserMock(username: $username);
7676
$totp = (new GoogleTotpFactory($hostname, $issuer, $digits))->createTotpForUser($user);
7777

7878
$returnValue = $totp->getProvisioningUri();
@@ -85,11 +85,13 @@ public function getProvisioningUri_hostnameAndIssuerGiven_returnProvisioningUri(
8585
public static function provideHostnameAndIssuer(): array
8686
{
8787
return [
88-
[null, null, self::DEFAULT_DIGITS, 'otpauth://totp/User%20Name?secret=SECRET'],
89-
[self::SERVER, null, self::DEFAULT_DIGITS, 'otpauth://totp/User%20Name%40Server?secret=SECRET'],
90-
[null, self::ISSUER, self::DEFAULT_DIGITS, 'otpauth://totp/Issuer%20Name%3AUser%20Name?issuer=Issuer%20Name&secret=SECRET'],
91-
[self::SERVER, self::ISSUER, self::DEFAULT_DIGITS, 'otpauth://totp/Issuer%20Name%3AUser%20Name%40Server?issuer=Issuer%20Name&secret=SECRET'],
92-
[self::SERVER, self::ISSUER, self::CUSTOM_DIGITS, 'otpauth://totp/Issuer%20Name%3AUser%20Name%40Server?digits=8&issuer=Issuer%20Name&secret=SECRET'],
88+
[null, null, self::ISSUER, self::DEFAULT_DIGITS, 'otpauth://totp/Issuer%20Name?secret=SECRET'],
89+
[null, self::SERVER, self::ISSUER, self::DEFAULT_DIGITS, 'otpauth://totp/Issuer%20Name%3AServer?issuer=Issuer%20Name&secret=SECRET'],
90+
[self::USER_NAME, null, null, self::DEFAULT_DIGITS, 'otpauth://totp/User%20Name?secret=SECRET'],
91+
[self::USER_NAME, self::SERVER, null, self::DEFAULT_DIGITS, 'otpauth://totp/User%20Name%40Server?secret=SECRET'],
92+
[self::USER_NAME, null, self::ISSUER, self::DEFAULT_DIGITS, 'otpauth://totp/Issuer%20Name%3AUser%20Name?issuer=Issuer%20Name&secret=SECRET'],
93+
[self::USER_NAME, self::SERVER, self::ISSUER, self::DEFAULT_DIGITS, 'otpauth://totp/Issuer%20Name%3AUser%20Name%40Server?issuer=Issuer%20Name&secret=SECRET'],
94+
[self::USER_NAME, self::SERVER, self::ISSUER, self::CUSTOM_DIGITS, 'otpauth://totp/Issuer%20Name%3AUser%20Name%40Server?digits=8&issuer=Issuer%20Name&secret=SECRET'],
9395
];
9496
}
9597
}

‎tests/Security/TwoFactor/Provider/Totp/TotpFactoryTest.php‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,13 @@ class TotpFactoryTest extends TestCase
2929
private const int DIGITS = 8;
3030
private const string ALGORITHM = TotpConfiguration::ALGORITHM_SHA256;
3131

32-
private function createUserMock(bool $hasTotpConfiguration = true, string|null $secret = self::SECRET): MockObject&TwoFactorInterface
32+
private function createUserMock(bool $hasTotpConfiguration = true, string|null $secret = self::SECRET, string|null $username = self::USER_NAME): MockObject&TwoFactorInterface
3333
{
3434
$user = $this->createMock(TwoFactorInterface::class);
3535
$user
3636
->expects($this->any())
3737
->method('getTotpAuthenticationUsername')
38-
->willReturn(self::USER_NAME);
38+
->willReturn($username);
3939

4040
$config = $hasTotpConfiguration ? new TotpConfiguration($secret, self::ALGORITHM, self::PERIOD, self::DIGITS) : null;
4141
$user
@@ -86,9 +86,9 @@ public function createTotpForUser_factoryCalled_returnTotpObject(): void
8686
*/
8787
#[Test]
8888
#[DataProvider('provideHostnameAndIssuer')]
89-
public function getProvisioningUri_hostnameAndIssuerGiven_returnProvisioningUri(string|null $hostname, string|null $issuer, array $customParameters, string $expectedUrl): void
89+
public function getProvisioningUri_hostnameAndIssuerGiven_returnProvisioningUri(string|null $username, string|null $hostname, string|null $issuer, array $customParameters, string $expectedUrl): void
9090
{
91-
$user = $this->createUserMock();
91+
$user = $this->createUserMock(username: $username);
9292
$totp = (new TotpFactory($hostname, $issuer, $customParameters))->createTotpForUser($user);
9393

9494
$returnValue = $totp->getProvisioningUri();
@@ -101,12 +101,14 @@ public function getProvisioningUri_hostnameAndIssuerGiven_returnProvisioningUri(
101101
public static function provideHostnameAndIssuer(): array
102102
{
103103
return [
104-
[null, null, [], 'otpauth://totp/User%20Name?algorithm=sha256&digits=8&period=20&secret=SECRET'],
105-
[self::SERVER, null, [], 'otpauth://totp/User%20Name%40Server%20Name?algorithm=sha256&digits=8&period=20&secret=SECRET'],
106-
[null, self::ISSUER, [], 'otpauth://totp/Issuer%20Name%3AUser%20Name?algorithm=sha256&digits=8&issuer=Issuer%20Name&period=20&secret=SECRET'],
107-
[null, null, self::CUSTOM_PARAMETERS, 'otpauth://totp/User%20Name?algorithm=sha256&digits=8&image=logo.png&period=20&secret=SECRET'],
108-
[self::SERVER, self::ISSUER, [], 'otpauth://totp/Issuer%20Name%3AUser%20Name%40Server%20Name?algorithm=sha256&digits=8&issuer=Issuer%20Name&period=20&secret=SECRET'],
109-
[self::SERVER, self::ISSUER, self::CUSTOM_PARAMETERS, 'otpauth://totp/Issuer%20Name%3AUser%20Name%40Server%20Name?algorithm=sha256&digits=8&image=logo.png&issuer=Issuer%20Name&period=20&secret=SECRET'],
104+
[null, null, self::ISSUER, [], 'otpauth://totp/Issuer%20Name?algorithm=sha256&digits=8&period=20&secret=SECRET'],
105+
[null, self::SERVER, self::ISSUER, [], 'otpauth://totp/Issuer%20Name%3AServer%20Name?algorithm=sha256&digits=8&issuer=Issuer%20Name&period=20&secret=SECRET'],
106+
[self::USER_NAME, null, null, [], 'otpauth://totp/User%20Name?algorithm=sha256&digits=8&period=20&secret=SECRET'],
107+
[self::USER_NAME, self::SERVER, null, [], 'otpauth://totp/User%20Name%40Server%20Name?algorithm=sha256&digits=8&period=20&secret=SECRET'],
108+
[self::USER_NAME, null, self::ISSUER, [], 'otpauth://totp/Issuer%20Name%3AUser%20Name?algorithm=sha256&digits=8&issuer=Issuer%20Name&period=20&secret=SECRET'],
109+
[self::USER_NAME, null, null, self::CUSTOM_PARAMETERS, 'otpauth://totp/User%20Name?algorithm=sha256&digits=8&image=logo.png&period=20&secret=SECRET'],
110+
[self::USER_NAME, self::SERVER, self::ISSUER, [], 'otpauth://totp/Issuer%20Name%3AUser%20Name%40Server%20Name?algorithm=sha256&digits=8&issuer=Issuer%20Name&period=20&secret=SECRET'],
111+
[self::USER_NAME, self::SERVER, self::ISSUER, self::CUSTOM_PARAMETERS, 'otpauth://totp/Issuer%20Name%3AUser%20Name%40Server%20Name?algorithm=sha256&digits=8&image=logo.png&issuer=Issuer%20Name&period=20&secret=SECRET'],
110112
];
111113
}
112114
}

0 commit comments

Comments
 (0)