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

Generate QR codes locally with JS #487

Merged
merged 3 commits into from
Nov 8, 2022
Merged

Generate QR codes locally with JS #487

merged 3 commits into from
Nov 8, 2022

Conversation

iandunn
Copy link
Member

@iandunn iandunn commented Oct 25, 2022

Fixes #42
Closes #388 - This is an alternate approach, which addresses the concerns I raised there.
Closes #443 - There won't be an external URL to filter anymore. This has filters for the local URL, though

I don't think a PHP fallback is needed, since disabling JS is extremely rare, and the manual secret already serves that purpose.

@iandunn iandunn requested review from kasparsd and dd32 October 25, 2022 16:21
@iandunn iandunn self-assigned this Oct 25, 2022
@jeffpaul jeffpaul added this to the 0.8.0 milestone Oct 25, 2022
@iandunn
Copy link
Member Author

iandunn commented Oct 25, 2022

@kasparsd , @dd32 : I'll be AFK until the 2nd, so don't hesitate to make any changes y'all see fit in my absence. I'm also happy to pick up back up when I return.

@dd32
Copy link
Member

dd32 commented Nov 8, 2022

Additionally, It would be worth changing this from just a QR image, to also linking to the otpauth url.
That allows for those setting up on mobile (...and desktop) to simply click the QR image which loads up their Authenticator app.

Ie.

-<p id="two-factor-qr-placeholder">QR</p>
+<a id="two-factor-qr-placeholder" href="otpauth://....">QR</a>

I've tested this on mobile and desktop, and works as expected with FreeOTP, Yubico Authenticator, and the Apple Password Manager.

@iandunn
Copy link
Member Author

iandunn commented Nov 8, 2022

also linking to the otpauth url

That's a good idea, done in 68ed4f2

@iandunn iandunn merged commit ac63e64 into master Nov 8, 2022
@kasparsd kasparsd deleted the local-js-qr-codes branch November 9, 2022 09:58
<p>
<img src="<?php echo esc_url( $this->get_google_qr_code( $totp_title, $key, $site_name ) ); ?>" id="two-factor-totp-qrcode" />
<p id="two-factor-qr-code">
<a href="<?php echo $totp_url; ?>">
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be escaped with esc_url().

Copy link
Member Author

Choose a reason for hiding this comment

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

It is escaped above (same w/ the other). Normally I'd escape as late as possible, but this one needs the extra param for the otpauth protocol, which would make those echos fairly long and less readable, and duplicated instead of DRY.

I don't feel strongly either way though. I'm happy to change it if you'd prefer.

providers/class-two-factor-totp.php Show resolved Hide resolved
iandunn added a commit that referenced this pull request Nov 9, 2022
brennensmith75 added a commit to brennensmith75/Two-Factor that referenced this pull request Sep 22, 2023
auroravirtuoso added a commit to auroravirtuoso/two-factor that referenced this pull request Jun 13, 2024
frank-verynice added a commit to frank-verynice/two-factor that referenced this pull request Nov 29, 2024
robert-the-lawful-head added a commit to robert-the-lawful-head/two-factor that referenced this pull request Dec 10, 2024
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.

Maybe add a filter hook for the TOTP QR code URL TOTP: Switch to an internal QR Code provider.
4 participants