Skip to content

Commit 2080319

Browse files
NicolasCARPiNicolasCARPi
committed
make QR Code Provider a mandatory constructor argument
This change is discussed in #104 Currently, the library defaults to a QR Code Provider using an external service, thus leaking secrets. This change forces the definition of a QR Code Provider in the constructor. It is a breaking change. fixes #104
1 parent cabcf5d commit 2080319

10 files changed

+99
-67
lines changed

‎docs/getting-started.md

+18-5
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,31 @@ or if you have composer installed globally
1717
composer require robthree/twofactorauth
1818
```
1919

20+
**Note:** If you are not using a composer-aware framework, you should [include the composer loader yourself](https://getcomposer.org/doc/01-basic-usage.md#autoloading).
21+
2022
## 2. Create an instance
2123

22-
Now you can create an instance for use with your code
24+
`TwoFactorAuth` constructor requires an object able to provide a QR Code image. It is the only mandatory argument. This lets you select your preferred QR Code generator/library.
25+
26+
See [QR code providers documentation](docs/qr-codes.md) for more information about the different possibilites.
27+
28+
Example code:
2329

2430
```php
2531
use RobThree\Auth\TwoFactorAuth;
26-
27-
$tfa = new TwoFactorAuth();
32+
use RobThree\Auth\Providers\Qr\BaconQrCodeProvider; // if using Bacon
33+
use RobThree\Auth\Providers\Qr\EndroidQrCodeProvider; // if using Endroid
34+
35+
// using Bacon
36+
$tfa = new TwoFactorAuth(new BaconQrCodeProvider());
37+
// using Endroid
38+
$tfa = new TwoFactorAuth(new EndroidQrCodeProvider());
39+
// using a custom object
40+
$tfa = new TwoFactorAuth(new MyQrCodeProvider());
41+
// using named argument and a variable
42+
$tfa = new TwoFactorAuth(qrcodeprovider: $qrGenerator);
2843
```
2944

30-
**Note:** if you are not using a framework that uses composer, you should [include the composer loader yourself](https://getcomposer.org/doc/01-basic-usage.md#autoloading)
31-
3245
## 3. Shared secrets
3346

3447
When your user is setting up two-factor, or multi-factor, authentication in your project, you can create a secret from the instance.

‎docs/qr-codes.md

+21-21
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,6 @@ You can also specify a size as a third argument which is 200 by default.
1818

1919
**Note:** by default, the QR code returned by the instance is generated from a third party across the internet. If the third party is encountering problems or is not available from where you have hosted your code, your user will likely experience a delay in seeing the QR code, if it even loads at all. This can be overcome with offline providers configured when you create the instance.
2020

21-
## Online Providers
22-
23-
[QRServerProvider](qr-codes/qr-server.md) (default)
24-
25-
**Warning:** Whilst it is the default, this provider is not suggested for applications where absolute security is needed, because it uses an external service for the QR code generation. You can make use of the included offline providers listed below which generate locally.
26-
27-
[ImageChartsQRCodeProvider](qr-codes/image-charts.md)
28-
29-
[QRicketProvider](qr-codes/qrickit.md)
30-
3121
## Offline Providers
3222

3323
[EndroidQrCodeProvider](qr-codes/endroid.md) and EndroidQrCodeWithLogoProvider
@@ -38,23 +28,33 @@ You can also specify a size as a third argument which is 200 by default.
3828

3929
## Custom Provider
4030

41-
If you wish to make your own QR Code provider to reference another service or library, it must implement the [IQRCodeProvider interface](https://github.com/RobThree/TwoFactorAuth/blob/master/lib/Providers/Qr/IQRCodeProvider.php).
31+
If you wish to make your own QR Code provider to reference another service or library, it must implement the [IQRCodeProvider interface](../lib/Providers/Qr/IQRCodeProvider.php).
4232

4333
It is recommended to use similar constructor arguments as the included providers to avoid big shifts when trying different providers.
4434

45-
## Using a specific provider
46-
47-
If you do not want to use the default QR code provider, you can specify the one you want to use when you create your instance.
35+
Example:
4836

4937
```php
5038
use RobThree\Auth\TwoFactorAuth;
39+
// using a custom object implementing IQRCodeProvider
40+
$tfa = new TwoFactorAuth(new MyQrCodeProvider());
41+
// using named argument and a variable
42+
$tfa = new TwoFactorAuth(qrcodeprovider: $qrGenerator);
43+
```
5144

52-
$qrCodeProvider = new YourChosenProvider();
45+
## Online Providers
5346

54-
$tfa = new TwoFactorAuth(
55-
issuer: "Your Company Or App Name",
56-
qrcodeprovider: $qrCodeProvider
57-
);
58-
```
47+
**Warning:** Using an external service for generating QR codes encoding authentication secrets is **not** recommended! You should instead make use of the included offline providers listed above.
48+
49+
* Gogr.me: [QRServerProvider](qr-codes/qr-server.md)
50+
* Image Charts: [ImageChartsQRCodeProvider](qr-codes/image-charts.md)
51+
* Qrickit: [QRicketProvider](qr-codes/qrickit.md)
52+
* Google Charts: [GoogleChartsQrCodeProvider](qr-codes/google-charts.md)
53+
54+
Example:
5955

60-
As you create a new instance of your provider, you can supply any extra configuration there.
56+
```php
57+
use RobThree\Auth\TwoFactorAuth;
58+
use RobThree\Auth\Providers\Qr\GoogleChartsQrCodeProvider;
59+
$tfa = new TwoFactorAuth(new GoogleChartsQrCodeProvider());
60+
```

‎docs/qr-codes/google-charts.md

+15
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
layout: post
3+
title: QR GoogleCharts
4+
---
5+
6+
See: https://developers.google.com/chart/infographics/docs/qr_codes
7+
8+
## Optional Configuration
9+
10+
Argument | Default value
11+
------------------------|---------------
12+
`$verifyssl` | `false`
13+
`$errorcorrectionlevel` | `'L'`
14+
`$margin` | `4`
15+
`$encoding` | `'UTF-8'`

‎lib/TwoFactorAuth.php

+3-11
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
use function hash_equals;
88

99
use RobThree\Auth\Providers\Qr\IQRCodeProvider;
10-
use RobThree\Auth\Providers\Qr\QRServerProvider;
1110
use RobThree\Auth\Providers\Rng\CSRNGProvider;
1211
use RobThree\Auth\Providers\Rng\IRNGProvider;
1312
use RobThree\Auth\Providers\Time\HttpTimeProvider;
@@ -29,11 +28,11 @@ class TwoFactorAuth
2928
private static array $_base32lookup = array();
3029

3130
public function __construct(
31+
private IQRCodeProvider $qrcodeprovider,
3232
private readonly ?string $issuer = null,
3333
private readonly int $digits = 6,
3434
private readonly int $period = 30,
3535
private readonly Algorithm $algorithm = Algorithm::Sha1,
36-
private ?IQRCodeProvider $qrcodeprovider = null,
3736
private ?IRNGProvider $rngprovider = null,
3837
private ?ITimeProvider $timeprovider = null
3938
) {
@@ -111,11 +110,10 @@ public function getQRCodeImageAsDataUri(string $label, #[SensitiveParameter] str
111110
throw new TwoFactorAuthException('Size must be > 0');
112111
}
113112

114-
$qrcodeprovider = $this->getQrCodeProvider();
115113
return 'data:'
116-
. $qrcodeprovider->getMimeType()
114+
. $this->qrcodeprovider->getMimeType()
117115
. ';base64,'
118-
. base64_encode($qrcodeprovider->getQRCodeImage($this->getQRText($label, $secret), $size));
116+
. base64_encode($this->qrcodeprovider->getQRCodeImage($this->getQRText($label, $secret), $size));
119117
}
120118

121119
/**
@@ -161,12 +159,6 @@ public function getQRText(string $label, #[SensitiveParameter] string $secret):
161159
. '&digits=' . $this->digits;
162160
}
163161

164-
public function getQrCodeProvider(): IQRCodeProvider
165-
{
166-
// Set default QR Code provider if none was specified
167-
return $this->qrcodeprovider ??= new QRServerProvider();
168-
}
169-
170162
/**
171163
* @throws TwoFactorAuthException
172164
*/

‎tests/Providers/Qr/IQRCodeProviderTest.php

+11-9
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,24 @@
77
use PHPUnit\Framework\TestCase;
88
use RobThree\Auth\Algorithm;
99
use RobThree\Auth\Providers\Qr\HandlesDataUri;
10+
use RobThree\Auth\Providers\Qr\IQRCodeProvider;
1011
use RobThree\Auth\TwoFactorAuth;
1112
use RobThree\Auth\TwoFactorAuthException;
1213

1314
class IQRCodeProviderTest extends TestCase
1415
{
1516
use HandlesDataUri;
1617

17-
public function testTotpUriIsCorrect(): void
18+
protected IQRCodeProvider $qr;
19+
20+
protected function setUp(): void
1821
{
19-
$qr = new TestQrProvider();
22+
$this->qr = new TestQrProvider();
23+
}
2024

21-
$tfa = new TwoFactorAuth('Test&Issuer', 6, 30, Algorithm::Sha1, $qr);
25+
public function testTotpUriIsCorrect(): void
26+
{
27+
$tfa = new TwoFactorAuth($this->qr, 'Test&Issuer', 6, 30, Algorithm::Sha1);
2228
$data = $this->DecodeDataUri($tfa->getQRCodeImageAsDataUri('Test&Label', 'VMR466AB62ZBOKHE'));
2329
$this->assertSame('test/test', $data['mimetype']);
2430
$this->assertSame('base64', $data['encoding']);
@@ -27,14 +33,12 @@ public function testTotpUriIsCorrect(): void
2733

2834
public function testTotpUriIsCorrectNoIssuer(): void
2935
{
30-
$qr = new TestQrProvider();
31-
3236
/**
3337
* The library specifies the issuer is null by default however in PHP 8.1
3438
* there is a deprecation warning for passing null as a string argument to rawurlencode
3539
*/
3640

37-
$tfa = new TwoFactorAuth(null, 6, 30, Algorithm::Sha1, $qr);
41+
$tfa = new TwoFactorAuth($this->qr, null, 6, 30, Algorithm::Sha1);
3842
$data = $this->DecodeDataUri($tfa->getQRCodeImageAsDataUri('Test&Label', 'VMR466AB62ZBOKHE'));
3943
$this->assertSame('test/test', $data['mimetype']);
4044
$this->assertSame('base64', $data['encoding']);
@@ -43,9 +47,7 @@ public function testTotpUriIsCorrectNoIssuer(): void
4347

4448
public function testGetQRCodeImageAsDataUriThrowsOnInvalidSize(): void
4549
{
46-
$qr = new TestQrProvider();
47-
48-
$tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, $qr);
50+
$tfa = new TwoFactorAuth($this->qr, 'Test', 6, 30, Algorithm::Sha1);
4951

5052
$this->expectException(TwoFactorAuthException::class);
5153

‎tests/Providers/Rng/IRNGProviderTest.php

+2-1
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,13 @@
77
use PHPUnit\Framework\TestCase;
88
use RobThree\Auth\Algorithm;
99
use RobThree\Auth\TwoFactorAuth;
10+
use Tests\Providers\Qr\TestQrProvider;
1011

1112
class IRNGProviderTest extends TestCase
1213
{
1314
public function testCreateSecret(): void
1415
{
15-
$tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null);
16+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30, Algorithm::Sha1, null, null);
1617
$this->assertIsString($tfa->createSecret());
1718
}
1819
}

‎tests/Providers/Time/ITimeProviderTest.php

+4-3
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
use RobThree\Auth\Algorithm;
99
use RobThree\Auth\TwoFactorAuth;
1010
use RobThree\Auth\TwoFactorAuthException;
11+
use Tests\Providers\Qr\TestQrProvider;
1112

1213
class ITimeProviderTest extends TestCase
1314
{
@@ -17,7 +18,7 @@ public function testEnsureCorrectTimeDoesNotThrowForCorrectTime(): void
1718
$tpr1 = new TestTimeProvider(123);
1819
$tpr2 = new TestTimeProvider(128);
1920

20-
$tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null, $tpr1);
21+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30, Algorithm::Sha1, null, $tpr1);
2122
$tfa->ensureCorrectTime(array($tpr2)); // 128 - 123 = 5 => within default leniency
2223
}
2324

@@ -26,7 +27,7 @@ public function testEnsureCorrectTimeThrowsOnIncorrectTime(): void
2627
$tpr1 = new TestTimeProvider(123);
2728
$tpr2 = new TestTimeProvider(124);
2829

29-
$tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null, $tpr1);
30+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30, Algorithm::Sha1, null, $tpr1);
3031

3132
$this->expectException(TwoFactorAuthException::class);
3233

@@ -36,7 +37,7 @@ public function testEnsureCorrectTimeThrowsOnIncorrectTime(): void
3637
public function testEnsureDefaultTimeProviderReturnsCorrectTime(): void
3738
{
3839
$this->expectNotToPerformAssertions();
39-
$tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1);
40+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30, Algorithm::Sha1);
4041
$tfa->ensureCorrectTime(array(new TestTimeProvider(time())), 1); // Use a leniency of 1, should the time change between both time() calls
4142
}
4243
}

‎tests/TwoFactorAuthTest.php

+14-13
Original file line numberDiff line numberDiff line change
@@ -11,33 +11,34 @@
1111
use RobThree\Auth\Providers\Time\NTPTimeProvider;
1212
use RobThree\Auth\TwoFactorAuth;
1313
use RobThree\Auth\TwoFactorAuthException;
14+
use Tests\Providers\Qr\TestQrProvider;
1415

1516
class TwoFactorAuthTest extends TestCase
1617
{
1718
public function testConstructorThrowsOnInvalidDigits(): void
1819
{
1920
$this->expectException(TwoFactorAuthException::class);
2021

21-
new TwoFactorAuth('Test', 0);
22+
new TwoFactorAuth(new TestQrProvider(), 'Test', 0);
2223
}
2324

2425
public function testConstructorThrowsOnInvalidPeriod(): void
2526
{
2627
$this->expectException(TwoFactorAuthException::class);
2728

28-
new TwoFactorAuth('Test', 6, 0);
29+
new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 0);
2930
}
3031

3132
public function testGetCodeReturnsCorrectResults(): void
3233
{
33-
$tfa = new TwoFactorAuth('Test');
34+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test');
3435
$this->assertSame('543160', $tfa->getCode('VMR466AB62ZBOKHE', 1426847216));
3536
$this->assertSame('538532', $tfa->getCode('VMR466AB62ZBOKHE', 0));
3637
}
3738

3839
public function testEnsureAllTimeProvidersReturnCorrectTime(): void
3940
{
40-
$tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1);
41+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30, Algorithm::Sha1);
4142
$tfa->ensureCorrectTime(array(
4243
new NTPTimeProvider(), // Uses pool.ntp.org by default
4344
//new \RobThree\Auth\Providers\Time\NTPTimeProvider('time.google.com'), // Somehow time.google.com and time.windows.com make travis timeout??
@@ -50,7 +51,7 @@ public function testEnsureAllTimeProvidersReturnCorrectTime(): void
5051

5152
public function testVerifyCodeWorksCorrectly(): void
5253
{
53-
$tfa = new TwoFactorAuth('Test', 6, 30);
54+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30);
5455
$this->assertTrue($tfa->verifyCode('VMR466AB62ZBOKHE', '543160', 1, 1426847190));
5556
$this->assertTrue($tfa->verifyCode('VMR466AB62ZBOKHE', '543160', 0, 1426847190 + 29)); //Test discrepancy
5657
$this->assertFalse($tfa->verifyCode('VMR466AB62ZBOKHE', '543160', 0, 1426847190 + 30)); //Test discrepancy
@@ -69,7 +70,7 @@ public function testVerifyCodeWorksCorrectly(): void
6970

7071
public function testVerifyCorrectTimeSliceIsReturned(): void
7172
{
72-
$tfa = new TwoFactorAuth('Test', 6, 30);
73+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30);
7374

7475
// We test with discrepancy 3 (so total of 7 codes: c-3, c-2, c-1, c, c+1, c+2, c+3
7576
// Ensure each corresponding timeslice is returned correctly
@@ -95,7 +96,7 @@ public function testVerifyCorrectTimeSliceIsReturned(): void
9596

9697
public function testGetCodeThrowsOnInvalidBase32String1(): void
9798
{
98-
$tfa = new TwoFactorAuth('Test');
99+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test');
99100

100101
$this->expectException(TwoFactorAuthException::class);
101102

@@ -104,7 +105,7 @@ public function testGetCodeThrowsOnInvalidBase32String1(): void
104105

105106
public function testGetCodeThrowsOnInvalidBase32String2(): void
106107
{
107-
$tfa = new TwoFactorAuth('Test');
108+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test');
108109

109110
$this->expectException(TwoFactorAuthException::class);
110111

@@ -124,7 +125,7 @@ public function testKnownBase32DecodeTestVectors(): void
124125
// "In general, you don't want to break any encapsulation for the sake of testing (or as Mom used to say, "don't
125126
// expose your privates!"). Most of the time, you should be able to test a class by exercising its public methods."
126127
// Dave Thomas and Andy Hunt -- "Pragmatic Unit Testing
127-
$tfa = new TwoFactorAuth('Test');
128+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test');
128129

129130
$method = new ReflectionMethod(TwoFactorAuth::class, 'base32Decode');
130131

@@ -144,7 +145,7 @@ public function testKnownBase32DecodeUnpaddedTestVectors(): void
144145
// This test ensures that strings without the padding-char ('=') are also decoded correctly.
145146
// https://tools.ietf.org/html/rfc4648#page-4:
146147
// "In some circumstances, the use of padding ("=") in base-encoded data is not required or used."
147-
$tfa = new TwoFactorAuth('Test');
148+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test');
148149

149150
$method = new ReflectionMethod(TwoFactorAuth::class, 'base32Decode');
150151

@@ -162,7 +163,7 @@ public function testKnownTestVectors_sha1(): void
162163
{
163164
//Known test vectors for SHA1: https://tools.ietf.org/html/rfc6238#page-15
164165
$secret = 'GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ'; //== base32encode('12345678901234567890')
165-
$tfa = new TwoFactorAuth('Test', 8, 30, Algorithm::Sha1);
166+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 8, 30, Algorithm::Sha1);
166167
$this->assertSame('94287082', $tfa->getCode($secret, 59));
167168
$this->assertSame('07081804', $tfa->getCode($secret, 1111111109));
168169
$this->assertSame('14050471', $tfa->getCode($secret, 1111111111));
@@ -175,7 +176,7 @@ public function testKnownTestVectors_sha256(): void
175176
{
176177
//Known test vectors for SHA256: https://tools.ietf.org/html/rfc6238#page-15
177178
$secret = 'GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQGEZA'; //== base32encode('12345678901234567890123456789012')
178-
$tfa = new TwoFactorAuth('Test', 8, 30, Algorithm::Sha256);
179+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 8, 30, Algorithm::Sha256);
179180
$this->assertSame('46119246', $tfa->getCode($secret, 59));
180181
$this->assertSame('68084774', $tfa->getCode($secret, 1111111109));
181182
$this->assertSame('67062674', $tfa->getCode($secret, 1111111111));
@@ -188,7 +189,7 @@ public function testKnownTestVectors_sha512(): void
188189
{
189190
//Known test vectors for SHA512: https://tools.ietf.org/html/rfc6238#page-15
190191
$secret = 'GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQGEZDGNA'; //== base32encode('1234567890123456789012345678901234567890123456789012345678901234')
191-
$tfa = new TwoFactorAuth('Test', 8, 30, Algorithm::Sha512);
192+
$tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 8, 30, Algorithm::Sha512);
192193
$this->assertSame('90693936', $tfa->getCode($secret, 59));
193194
$this->assertSame('25091201', $tfa->getCode($secret, 1111111109));
194195
$this->assertSame('99943326', $tfa->getCode($secret, 1111111111));

0 commit comments

Comments
 (0)