integrate bacon qr code

[willpower232]

To assist with #37 and also to improve my own project, I have written a provider for BaconQrCode.

I have used the current version which relies on PHP 7.1 at minimum. It is also possible to customise the QR code slightly.

Usage:

$provider = new \RobThree\Auth\Providers\Qr\BaconQrCodeProvider();
$provider->setBorderWidth(1);
$provider->setBackgroundColour('#d0d0d0');
$provider->setForegroundColour('#333');

$twoFa = new TwoFactorAuth($issuer, 6, 30, 'sha1', $provider);

// works with standard usage
$twoFa->getQRCodeImageAsDataUri($label, $secret, 400);

// also access an SVG
$provider->getQRCodeSvg($twoFa->getQRText($label, $secret), 400);

As an aside, would you consider allowing setters in the main TwoFactorAuth class to allow setting the providers without declaring all the other options in the constructor?

[RobThree]

Any desired options should be passed to the constructor; please don't use a new public function as this will break when people switch implementations.

See the other constructors for examples:

https://github.com/RobThree/TwoFactorAuth/blob/master/lib/Providers/Qr/ImageChartsQRCodeProvider.php#L11
https://github.com/RobThree/TwoFactorAuth/blob/master/lib/Providers/Qr/QRServerProvider.php#L15
https://github.com/RobThree/TwoFactorAuth/blob/master/lib/Providers/Qr/QRicketProvider.php#L15

[willpower232]

I've made the requested changes however my testing was unable to throw \QRException, I haven't investigated but is it because that file is missing a namespace?

Revised usage:

$borderWidth = 1;
$backgroundColour = '#d0d0d0';
$foregroundColour = '#333';

$provider = new \RobThree\Auth\Providers\Qr\BaconQrCodeProvider(
    $borderWidth,
    $backgroundColour,
    $foregroundColour
);

$twoFa = new TwoFactorAuth($issuer, 6, 30, 'sha1', $provider);

// works with standard usage
$twoFa->getQRCodeImageAsDataUri($label, $secret, 400);

// to access an SVG, or other file type, pass the extension (i.e. svg, jpg)
// as the fourth argument to the provider

[RobThree]

It's better already but I would like to be able to specify the imagerenderer by passing an argument to the constructor. Also: I am really not very much familiar with the whole composer thing; shouldn't this require a reference in the composer.json file as it's a dependency?

[willpower232]

It's better already but I would like to be able to specify the imagerenderer by passing an argument to the constructor

That would make sense, the developer would presumably be able to decide on an image format with the design, would it be better as the first argument in case they were happy with the default border and colours?

Also: I am really not very much familiar with the whole composer thing; shouldn't this require a reference in the composer.json file as it's a dependency?

That is an excellent question. Related to your comment on #40 as well. Arguably, including multiple dependencies to create a QR code would lead to dependency bloat in numerous projects and may even cause conflicts if those dependencies required each other.

I mention that because endroid/qr-code relies on bacon/bacon-qr-code so if endroid required a newer version of bacon than you had specified, it could get messy. FWIW I think a lot of QR-related projects do also rely on bacon.

I guess you have a choice to make in this regard, whether you want this package to have a number of different providers relying on other packages, but those dependencies aren't immediately included in this packages dependencies (to avoid conflicts and/or integrate into users existing dependencies), or to only nominate one provider as the main one and include that dependency as necessary.

Admittedly I haven't written a test for this provider. Including a number of additional dependencies "on the fly" to test with could be complicated and annoying to maintain but not impossible.

Neither PR has included an update to the composer file so we're currently on the everything is optional but difficult to test route.

I hope this makes sense, let me know what you think.

Edit: as a further aside, both provider contenders would require another PHP version bump as bacon requires >= 7.1 and endroid requires >= 7.2

[RobThree]

would it be better as the first argument

If I would've coded it, I think it were the first argument; border and colors are pretty standard black/white etc. usually.

Edit: as a further aside, both provider contenders would require another PHP version bump as bacon requires >= 7.1 and endroid requires >= 7.2

Yes. I'm currently contemplating dropping support for PHP < 7.2 alltogether since they aren't supported anymore anyway. Though I think that would make a lot of people stuck on older versions (for whatever reason) unhappy. A "push" for a newer version can be 'needed' in some cases but in others people are really stuck. And besides the QR code generation requirements in these two PR's there's actually nothing in the code preventing it to run on 5.4 upwards... I only dropped 5.4 and 5.5 today becasue Travis CI doesn't support those anymore (at least not without me putting in some more effort than just removing 5.4 and 5.5 from .travis.yml 😉 )

For the rest of your reply about having a choice to make: we do. Let me sit on that too for a little while...

[willpower232]

I've just reopened the code and remembered that the format is already the last argument as per your other providers haha. I mentioned it in the comment in my last example at the end of the bit.

I'll await your decision.

[melanger]

Hello @RobThree , are you planning to merge this? I will be really grateful because I need a QR code provider which does not send the secret to a remote service. And regarding composer.json - you can add bacon into the suggest block (on the same level as the require block):

"suggest": {
  "bacon/bacon-qr-code": "Needed for BaconQrCodeProvider provider"
}

[RobThree]

are you planning to merge this?

Most likely, yes.

And regarding composer.json

Ah, that looks promising. Thanks.

[RobThree]

I have merged this one, as wel as #40. If someone would like to update the README and make a PR that would be appreciated!

[kevinquinnyo]

What's your release schedule for this library? As it stands, the README makes mention of the Bacon QR provider but the latest release tag does not include it.

As this is the only "baked in" QR code provider that does not send the secret to a 3rd part service over HTTP, it would be nice if a new release was created IMO, so people don't have to implement their own, or use something like 'dev-master' instead of an actual release.

Thanks for your work on this!

[RobThree]

What's your release schedule for this library?

Hmm, I've been busier than I'd like to, so I'm not sure. But I was thinking of pulling in extra people if they're interested to help out (like maybe @willpower232, @MasterOdin , @igorsantos07 or other big(ger) contributors). I'll try and see what I can co short term. If any of the tagged people would be interested, let me know! 👊

[MasterOdin]

@RobThree I'm happy to help maintain this library.

[kevinquinnyo]

@RobThree I'm also happy to help. What needs to be done besides tagging a new release? Are there issues that need to be resolved first?

[igorsantos07]

I prefer to stay out of this, unfortunately. I've compromised with OS packages a couple of times in the past, but my work routine never allows me to actually work on these, so... I stay as a occasional PR writer 😅 Igor Santos -- Desenvolvedor Web [enviado do meu celular]

…

On Wed, Jan 27, 2021, 19:08 Kevin Quinn ***@***.***> wrote: @RobThree <https://github.com/RobThree> I'm also happy to help. What needs to be done besides tagging a new release? Are there issues that need to be resolved first? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEB6S2JNRPBIAYAVQUGVS3S4CFGXANCNFSM4JAG7JNQ> .

[willpower232]

@RobThree I'm happy to help also.

Theres definitely more work that could be done in this project. Whilst not critical, I'd say other than bulking out the test suite and any linting/static analysis checking, testing against PHP 8 would be neat. Also migrating from travis-ci.org to .com or github actions will probably become an issue shortly.

[RobThree]

Agreed @willpower232. Besides that work needs to be done in the QR providers department and the README could use some work too.

For me most important is we try very hard not to break current implementations. The 'core' is, as far as I am concerned, pretty much ok. More providers (QR, Time, RNG) or improvements on the current once are always welcome, again, as long as we don't break stuff willy nilly. There's thousands of people and projects relying on this library.

I have some experience with PHP but I am mostly a .Net developer; I am not very experienced with stuff like composer. PHP8 support would be great but I also would like to try to stay compatible with PHP 5.6 upwards; even though it's EOL there's still many projects that haven't been ported to later PHP versions and as long as there's no good reason to drop support for this then I'd like to keep it (for the forseeable future). That doesn't mean that we can't have providers that require new(er) PHP versions.

One thing I never stopped and took the time for to figure out: ideally the providers are moved to external packages so they have a dependency on this 'core' library. That way providers can be updated "out of band" with TwoFactorAuth and providers can be much more easily added by third parties without having to do pull requests. Also documentation for each of the providers should be moved to the wiki I think, to keep the README short(er).

This is the general direction I'd like to take this project into.

One thing that worries me is having more than one captain on a ship; in my experience this usually ends in the ship sinking. However, I also don't have the time to give this project the attention it deserves. So I vote @willpower232 be added as a collaborator and if needed (we'll discuss this in the open) we can add @MasterOdin and @kevinquinnyo later. Everyone good with that?

I will try to keep the .Net variant of this library 'in tandem' with this PHP version as much as possible.

[willpower232]

Thanks for the vote!

Judging by #59, the code is working fine across all versions, I haven't had to do rewrites in other projects to support PHP 8. Obviously backwards compatibility rules out some of the "syntax sugar" but not a big deal in my book.

Having separate wrapper libraries would definitely resolve the manual requirement to install Bacon or Endroid or whatever so would present a nicer end user experience. I guess you'd have to unmerge these recent PRs to fully accomplish that unless you want to keep them in here.

I mentioned in another issue I think that wiki is great but requires you to provide write access to the repo to allow anyone to edit it so not necessarily what you want in this situation. A github-pages site could be a good alternative so you're back in PR-land.

You could also enable discussions for this repo if you wanted to stop talking in a PR :-P

[RobThree]

A github-pages site could be a good alternative so you're back in PR-land.

That's also a good idea 👍

You could also enable discussions for this repo if you wanted to stop talking in a PR :-P

Done 😉

[RobThree]

@ALL I have added @willpower232 as collaborator. I hope we can make some big steps towards a new and better release. I hope we can improve / solve some minor issues first and fix up some QR code providers and hope to work towards a new major release next in where we move the providers to separate libraries/packages so we they can be released out-of-band.

Thanks to all of you for all your input, efforts and work and good luck and welcome aboard @willpower232 !

[willpower232]

🙏 unless you have any specific minor issues, I might just dive into doc blocks, code formatting, and getting test coverage up where possible

[RobThree]

It's a bit unceremonious but; I'm happy with anything and everything you can co. Let's hold off publishing releases until we've both had a chance to review changes but other than that: I'm happy to have some help. As mentioned earlier, I've been quite busy lately so all help, however small or big, is very welcome!