Skip to content

Fix QR-Code for 2FA#24255

Merged
HLeithner merged 13 commits intojoomla:stagingfrom
bembelimen:2fa-qr
Apr 2, 2019
Merged

Fix QR-Code for 2FA#24255
HLeithner merged 13 commits intojoomla:stagingfrom
bembelimen:2fa-qr

Conversation

@bembelimen
Copy link
Contributor

Pull Request for Issue #24252 .

Summary of Changes

Implements an internal QR-Code generator instead of the google charts.

Testing Instructions

Activate the Google Authenticator 2FA plugin and set up 2FA for one user.

Expected result

QR-Code visible

Actual result

QR-Code broken

@brianteeman
Copy link
Contributor

Thank you for such a speedy response. I will test his in the morning

@ReLater
Copy link
Contributor

ReLater commented Mar 20, 2019

I have tested this item ✅ successfully on 550ddf2

Before patch: No QR code.
After patch: QR code created.

I could not test if QR code is scannable and/or correct. Decide yourself if that's a successful test!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24255.

bembelimen and others added 2 commits March 20, 2019 08:13
Co-Authored-By: bembelimen <bembelimen@users.noreply.github.com>

HTMLHelper::_('script', 'plg_twofactorauth_totp/qrcode.min.js', array('version' => 'auto', 'relative' => true));

$js = "
Copy link
Member

Choose a reason for hiding this comment

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

Can you not put this in a separate file in the media directory so it doesn't need to be done for J4?

Copy link
Member

Choose a reason for hiding this comment

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

Joomla 4 should get a webcomponent for displaying qrcodes.

Copy link
Member

Choose a reason for hiding this comment

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

Even if that's the case, any new additional JS for J3 should be whacked in a separate file so that it makes the migration a lot easier

Copy link
Member

Choose a reason for hiding this comment

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

Github fools me... you mean the inline js should be moved. ok that makes sense

Copy link
Member

Choose a reason for hiding this comment

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

sorry, I probably should be moved my comment down a couple more lines :)

@HLeithner HLeithner requested a review from wilsonge as a code owner April 2, 2019 11:11
@HLeithner HLeithner added this to the Joomla 3.9.5 milestone Apr 2, 2019
@wilsonge
Copy link
Contributor

wilsonge commented Apr 2, 2019

For my own reference when it comes to J4 the library is https://github.com/kazuhikoarase/qrcode-generator/blob/master/js/package.json#L2

@HLeithner HLeithner merged commit f4ef944 into joomla:staging Apr 2, 2019
@HLeithner
Copy link
Member

thx

@deroderuiter
Copy link

I installed 3.9.5 but the fix for the QR-Code for 2FA still does not work on my Joomla installation. Do I have do to something special to get it working again?

@HLeithner
Copy link
Member

@deroderuiter strange I tested it again and it works for me, do you have a screenshot and could you create a new issue.

@deroderuiter
Copy link

There is not much to show. In edit profile I choose the tab Two Factor Authentication, I scan the QR code with the Google Authenticator App on my smartphone and fill in the code and save the page. No error is displayed but the tow factor authentication is not activated.

@deroderuiter
Copy link

I saw I remark of Phil Taylor that Google no longer provides a chart API for QR Code. It was actually deprecated in 2012 and turned off Completely on March 18,2019
My English i not so good and I don't know if I understood this correctly and if it is correct it could explain why 2FA doesn't work anymore.

@brianteeman
Copy link
Contributor

if you see the qr code then the issue you refer to above is resolved as the qr code has been generated by different code

@deroderuiter
Copy link

So probably I have an other problem?

@bembelimen bembelimen deleted the 2fa-qr branch August 10, 2019 11:06
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.

10 participants