Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make QR Code Provider a mandatory constructor argument #125

Merged
merged 5 commits into from
Apr 27, 2024
Merged

make QR Code Provider a mandatory constructor argument #125

merged 5 commits into from
Apr 27, 2024

Conversation

NicolasCARPi
Copy link
Collaborator

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.

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
@RobThree
Copy link
Owner

Nice. I so wish I had the time to implement a 'self contained' QR code provider with SVG or PNG. Maybe, someday...

@NicolasCARPi
Copy link
Collaborator Author

@RobThree BTW, would you be so kind as to allow my account write access to this repo? My main argument is that it would save me from having to maintain a fork, but it can also be helpful if I can close issues and review other PRs. It will also help users identify this account as a contributor.

I promise I'm not playing long term game for planting a backdoor ^^

@RobThree
Copy link
Owner

I have no issue with that, but just to keep playing nice I'll have a quick glance at @willpower232 (which, I'm sure, will agree).

I promise I'm not playing long term game for planting a backdoor ^^

Hah, well guess what buddy, now the answer is no! 🤣 Who's laughing now?

docs/getting-started.md Outdated Show resolved Hide resolved
lib/TwoFactorAuth.php Show resolved Hide resolved
@willpower232
Copy link
Collaborator

willpower232 commented Apr 26, 2024

No issues with having another person with write access, as long as all the drastic stuff goes through PR or with @RobThree 's public consent, its all cool

@NicolasCARPi
Copy link
Collaborator Author

as long as all the drastic stuff goes through PR

In fact I'd go as far as locking down the master branch to only Admins, and force PR to merge in master. We have the tools at our disposal to prevent misuse, let's use them!

Copy link
Collaborator

@willpower232 willpower232 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm clicking approve but probably should update the changelog as this counts as another breaking change

@NicolasCARPi
Copy link
Collaborator Author

but probably should update the changelog

Of course, but I prefer to leave it out of commits, and do a dedicated CHANGELOG commit once we have everything we want (which I believe is the case for the next release).

@NicolasCARPi NicolasCARPi merged commit f35f2ae into RobThree:master Apr 27, 2024
12 checks passed
@NicolasCARPi NicolasCARPi deleted the nico-qr branch April 27, 2024 16:58
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.

Security Risk: using QRServerProvider as default provider
3 participants