make QR Code Provider a mandatory constructor argument PR #125

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 The public function getQRCodeProvider() has been removed. It is provided by the user in the constructor, so it doesn't make a lot of sense to keep a getter around if we're not using it internally.

Author: NicolasCARPi

docs/getting-started.md

@@ -7,28 +7,33 @@ title: Getting Started
 
 The best way of making use of this project is by installing it with [composer](https://getcomposer.org/doc/01-basic-usage.md).
 
-```
-php composer.phar require robthree/twofactorauth
-```
-
-or if you have composer installed globally
-
 ```
 composer require robthree/twofactorauth
 ```
 
 ## 2. Create an instance
 
-Now you can create an instance for use with your code
+`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.
+
+See [QR code providers documentation](qr-codes.md) for more information about the different possibilites.
+
+Example code:
 
 ```php
 use RobThree\Auth\TwoFactorAuth;
-
-$tfa = new TwoFactorAuth();
+use RobThree\Auth\Providers\Qr\BaconQrCodeProvider; // if using Bacon
+use RobThree\Auth\Providers\Qr\EndroidQrCodeProvider; // if using Endroid
+
+// using Bacon
+$tfa = new TwoFactorAuth(new BaconQrCodeProvider());
+// using Endroid
+$tfa = new TwoFactorAuth(new EndroidQrCodeProvider());
+// using a custom object implementing IQRCodeProvider interface
+$tfa = new TwoFactorAuth(new MyQrCodeProvider());
+// using named argument and a variable
+$tfa = new TwoFactorAuth(qrcodeprovider: $qrGenerator);
 ```
 
-**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)
-
 ## 3. Shared secrets
 
 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

@@ -5,7 +5,7 @@ title: QR Codes
 
 An alternative way of communicating the secret to the user is through the use of [QR Codes](http://en.wikipedia.org/wiki/QR_code) which most if not all authenticator mobile apps can scan.
 
-This can avoid accidental typing errors and also pre-set some text values within the users app.
+This can avoid accidental typing errors and also pre-set some text values within the two factor authentication mobile application.
 
 You can display the QR Code as a base64 encoded image using the instance as follows, supplying the users name or other public identifier as the first argument
 
@@ -16,18 +16,6 @@ You can display the QR Code as a base64 encoded image using the instance as foll
 
 You can also specify a size as a third argument which is 200 by default.
 
-**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.
-
-## Online Providers
-
-[QRServerProvider](qr-codes/qr-server.md) (default)
-
-**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.
-
-[ImageChartsQRCodeProvider](qr-codes/image-charts.md)
-
-[QRicketProvider](qr-codes/qrickit.md)
-
 ## Offline Providers
 
 [EndroidQrCodeProvider](qr-codes/endroid.md) and EndroidQrCodeWithLogoProvider
@@ -38,23 +26,33 @@ You can also specify a size as a third argument which is 200 by default.
 
 ## Custom Provider
 
-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).
+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).
 
 It is recommended to use similar constructor arguments as the included providers to avoid big shifts when trying different providers.
 
-## Using a specific provider
-
-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.
+Example:
 
 ```php
 use RobThree\Auth\TwoFactorAuth;
+// using a custom object implementing IQRCodeProvider
+$tfa = new TwoFactorAuth(new MyQrCodeProvider());
+// using named argument and a variable
+$tfa = new TwoFactorAuth(qrcodeprovider: $qrGenerator);
+```
+
+## Online Providers
 
-$qrCodeProvider = new YourChosenProvider();
+**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.
 
-$tfa = new TwoFactorAuth(
-issuer: "Your Company Or App Name",
-	qrcodeprovider: $qrCodeProvider
-);
-```
+* Gogr.me: [QRServerProvider](qr-codes/qr-server.md)
+* Image Charts: [ImageChartsQRCodeProvider](qr-codes/image-charts.md)
+* Qrickit: [QRicketProvider](qr-codes/qrickit.md)
+* Google Charts: [GoogleChartsQrCodeProvider](qr-codes/google-charts.md)
 
-As you create a new instance of your provider, you can supply any extra configuration there.
+Example:
+
+```php
+use RobThree\Auth\TwoFactorAuth;
+use RobThree\Auth\Providers\Qr\GoogleChartsQrCodeProvider;
+$tfa = new TwoFactorAuth(new GoogleChartsQrCodeProvider());
+```

docs/qr-codes/google-charts.md

@@ -0,0 +1,15 @@
+---
+layout: post
+title: QR GoogleCharts
+---
+
+See: https://developers.google.com/chart/infographics/docs/qr_codes
+
+## Optional Configuration
+
+Argument | Default value
+------------------------|---------------
+`$verifyssl` | `false`
+`$errorcorrectionlevel` | `'L'`
+`$margin` | `4`
+`$encoding` | `'UTF-8'`

lib/Providers/Qr/QRicketProvider.php

@@ -43,6 +43,6 @@ public function getUrl(string $qrText, int $size): string
  'd' => $qrText,
  );
 
- return 'http://qrickit.com/api/qr?' . http_build_query($queryParameters);
+ return 'https://qrickit.com/api/qr?' . http_build_query($queryParameters);
  }
 }

lib/TwoFactorAuth.php

@@ -7,7 +7,6 @@
 use function hash_equals;
 
 use RobThree\Auth\Providers\Qr\IQRCodeProvider;
-use RobThree\Auth\Providers\Qr\QRServerProvider;
 use RobThree\Auth\Providers\Rng\CSRNGProvider;
 use RobThree\Auth\Providers\Rng\IRNGProvider;
 use RobThree\Auth\Providers\Time\HttpTimeProvider;
@@ -29,11 +28,11 @@ class TwoFactorAuth
  private static array $_base32lookup = array();
 
  public function __construct(
+ private IQRCodeProvider $qrcodeprovider,
  private readonly ?string $issuer = null,
  private readonly int $digits = 6,
  private readonly int $period = 30,
  private readonly Algorithm $algorithm = Algorithm::Sha1,
- private ?IQRCodeProvider $qrcodeprovider = null,
  private ?IRNGProvider $rngprovider = null,
  private ?ITimeProvider $timeprovider = null
  ) {
@@ -111,11 +110,10 @@ public function getQRCodeImageAsDataUri(string $label, #[SensitiveParameter] str
  throw new TwoFactorAuthException('Size must be > 0');
  }
 
- $qrcodeprovider = $this->getQrCodeProvider();
  return 'data:'
- . $qrcodeprovider->getMimeType()
+ . $this->qrcodeprovider->getMimeType()
  . ';base64,'
- . base64_encode($qrcodeprovider->getQRCodeImage($this->getQRText($label, $secret), $size));
+ . base64_encode($this->qrcodeprovider->getQRCodeImage($this->getQRText($label, $secret), $size));
  }
 
  /**
@@ -161,12 +159,6 @@ public function getQRText(string $label, #[SensitiveParameter] string $secret):
  . '&digits=' . $this->digits;
  }
 
- public function getQrCodeProvider(): IQRCodeProvider
- {
- // Set default QR Code provider if none was specified
- return $this->qrcodeprovider ??= new QRServerProvider();
- }
-
  /**
  * @throws TwoFactorAuthException
  */

tests/Providers/Qr/IQRCodeProviderTest.php

@@ -7,18 +7,24 @@
 use PHPUnit\Framework\TestCase;
 use RobThree\Auth\Algorithm;
 use RobThree\Auth\Providers\Qr\HandlesDataUri;
+use RobThree\Auth\Providers\Qr\IQRCodeProvider;
 use RobThree\Auth\TwoFactorAuth;
 use RobThree\Auth\TwoFactorAuthException;
 
 class IQRCodeProviderTest extends TestCase
 {
  use HandlesDataUri;
 
- public function testTotpUriIsCorrect(): void
+ protected IQRCodeProvider $qr;
+
+ protected function setUp(): void
  {
- $qr = new TestQrProvider();
+ $this->qr = new TestQrProvider();
+ }
 
- $tfa = new TwoFactorAuth('Test&Issuer', 6, 30, Algorithm::Sha1, $qr);
+ public function testTotpUriIsCorrect(): void
+ {
+ $tfa = new TwoFactorAuth($this->qr, 'Test&Issuer', 6, 30, Algorithm::Sha1);
  $data = $this->DecodeDataUri($tfa->getQRCodeImageAsDataUri('Test&Label', 'VMR466AB62ZBOKHE'));
  $this->assertSame('test/test', $data['mimetype']);
  $this->assertSame('base64', $data['encoding']);
@@ -27,14 +33,12 @@ public function testTotpUriIsCorrect(): void
 
  public function testTotpUriIsCorrectNoIssuer(): void
  {
- $qr = new TestQrProvider();
-
  /**
  * The library specifies the issuer is null by default however in PHP 8.1
  * there is a deprecation warning for passing null as a string argument to rawurlencode
  */
 
- $tfa = new TwoFactorAuth(null, 6, 30, Algorithm::Sha1, $qr);
+ $tfa = new TwoFactorAuth($this->qr, null, 6, 30, Algorithm::Sha1);
  $data = $this->DecodeDataUri($tfa->getQRCodeImageAsDataUri('Test&Label', 'VMR466AB62ZBOKHE'));
  $this->assertSame('test/test', $data['mimetype']);
  $this->assertSame('base64', $data['encoding']);
@@ -43,9 +47,7 @@ public function testTotpUriIsCorrectNoIssuer(): void
 
  public function testGetQRCodeImageAsDataUriThrowsOnInvalidSize(): void
  {
- $qr = new TestQrProvider();
-
- $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, $qr);
+ $tfa = new TwoFactorAuth($this->qr, 'Test', 6, 30, Algorithm::Sha1);
 
  $this->expectException(TwoFactorAuthException::class);
 

tests/Providers/Rng/IRNGProviderTest.php

@@ -7,12 +7,13 @@
 use PHPUnit\Framework\TestCase;
 use RobThree\Auth\Algorithm;
 use RobThree\Auth\TwoFactorAuth;
+use Tests\Providers\Qr\TestQrProvider;
 
 class IRNGProviderTest extends TestCase
 {
  public function testCreateSecret(): void
  {
- $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null);
+ $tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30, Algorithm::Sha1, null, null);
  $this->assertIsString($tfa->createSecret());
  }
 }

tests/Providers/Time/ITimeProviderTest.php

@@ -8,6 +8,7 @@
 use RobThree\Auth\Algorithm;
 use RobThree\Auth\TwoFactorAuth;
 use RobThree\Auth\TwoFactorAuthException;
+use Tests\Providers\Qr\TestQrProvider;
 
 class ITimeProviderTest extends TestCase
 {
@@ -17,7 +18,7 @@ public function testEnsureCorrectTimeDoesNotThrowForCorrectTime(): void
  $tpr1 = new TestTimeProvider(123);
  $tpr2 = new TestTimeProvider(128);
 
- $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null, $tpr1);
+ $tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30, Algorithm::Sha1, null, $tpr1);
  $tfa->ensureCorrectTime(array($tpr2)); // 128 - 123 = 5 => within default leniency
  }
 
@@ -26,7 +27,7 @@ public function testEnsureCorrectTimeThrowsOnIncorrectTime(): void
  $tpr1 = new TestTimeProvider(123);
  $tpr2 = new TestTimeProvider(124);
 
- $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null, $tpr1);
+ $tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30, Algorithm::Sha1, null, $tpr1);
 
  $this->expectException(TwoFactorAuthException::class);
 
@@ -36,7 +37,7 @@ public function testEnsureCorrectTimeThrowsOnIncorrectTime(): void
  public function testEnsureDefaultTimeProviderReturnsCorrectTime(): void
  {
  $this->expectNotToPerformAssertions();
- $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1);
+ $tfa = new TwoFactorAuth(new TestQrProvider(), 'Test', 6, 30, Algorithm::Sha1);
  $tfa->ensureCorrectTime(array(new TestTimeProvider(time())), 1); // Use a leniency of 1, should the time change between both time() calls
  }
 }