Problem/Motivation

Google Authenticator link is broken.

Steps to reproduce

  1. Enable TFA
  2. Navigate to view profile
  3. Click on TFA tab
  4. Click on Set up application link
  5. Click on Google Authenticator link

Proposed resolution

  1. Update link to include options for Android and iOS
  2. Change ‘Authy’ to ‘Twilio Authy’ to avoid confusion/make sure people don’t download the wrong app

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork tfa-3386910

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

yeniatencio created an issue. See original summary.

yeniatencio’s picture

StatusFileSize
new2.66 KB

Please review patch.

yeniatencio’s picture

StatusFileSize
new2.71 KB

Please review patch for 2.x version

cmlara’s picture

Version: 8.x-1.2 » 2.x-dev
Status: Active » Needs work
  1. +++ b/src/Plugin/Tfa/TfaHotp.php
    @@ -30,7 +30,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
    + *    "Google Authenticator (Android/iOS)" = "https://support.google.com/accounts/answer/1066447?sjid=8469089794140994367-AP&co=GENIE.Platform%3DAndroid&oco=0",
    

    This link appears to have tracking data that should not be included.

  2. +++ b/src/Plugin/Tfa/TfaTotp.php
    @@ -30,11 +30,12 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
    + *    "Google Authenticator (Android/iOS)" = "https://support.google.com/accounts/answer/1066447?sjid=8469089794140994367-AP&co=GENIE.Platform%3DAndroid&oco=0",
    

    Same as the HOTP comment.

  3. +++ b/src/Plugin/Tfa/TfaTotp.php
    @@ -30,11 +30,12 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
    - *    "GAuth Authenticator (Desktop)" = "https://github.com/gbraadnl/gauth",
    

    Why the removal?

  4. +++ b/src/Plugin/Tfa/TfaTotp.php
    @@ -30,11 +30,12 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
    + *    "Protecc - 2FA Authenticator (Windows)" = "https://apps.microsoft.com/store/detail/protecc-2fa-authenticator-totp/9PJX91M06TZS?hl=en-au&gl=au&rtc=1",
    + *    "Authenticator 2FA | Sentinel (MAC)" = "https://apps.apple.com/au/app/authenticator-2fa-sentinel/id1189922806"
    

    Adding new records should probably be done in a separate feature request. I'm not sure what the existing entries had to go through to get on the list, but considering were talking security tokens I don't personally want to add any new entries without a discussion about them on why they should be on the list (we obviously can't and shouldn't list every token that exists in this help link list.)

yeniatencio’s picture

Thanks cmlara. I have removed the tracking data from the links as point out.
Uploading new patch for version 8.x-1.x

Regarding, adding new links, completely makes sense. However, adding those for a very specific use case on my side.

cmlara’s picture

@yeniatencio It general it is helpful if patches don't have local environment use changes in them.

However, adding those for a very specific use case on my side.

Just in case you mean you are using D.O. to host patches that you use as part of your composer deployment I will note this is generally not recommended for both security and stability reasons. D.O could be compromised at any time meaning patches hosted on D.O. could pose a risk to a site, additionally there have been times during security windows where load has caused D.O. to go offline preventing deployment.

bhanu951’s picture

Status: Needs work » Needs review
StatusFileSize
new1.92 KB

I am resubmitting my patch from #3395254: Wrong domain name for Google Authenticator on TOTP and HOTP setup page as it was closed as duplicate.

Please review the patch. It has different links from the other patches submitted here earlier.

cmlara’s picture

Status: Needs review » Needs work

This will still need the Twillo name change from the original patch as that is in scope.

As for the links:
At first I was inclined for a single link of the first patch to keep the UI less cluttered. however reading the text on the KB page, and acknowledging it is a KB not a product page I think the links provided by @Bhanu951 are the better choice.

Side note:
We recently switched to Fully GitlabCi so we can’t actually test patch files in this project going forward.

bhanu951’s picture

Status: Needs work » Needs review

Updated the patch addressing the review comments and created the new MR.

bhanu951’s picture

Updated the MR addressing the review comments please re-review.

  • cmlara committed a642e6be on 2.x authored by Bhanu951
    Issue #3386910 by Bhanu951, yeniatencio, cmlara: Authenticator links...
cmlara’s picture

Status: Needs review » Patch (to be ported)

Committed to 2.x, doesn't apply cleanly to 8.x-.1.x.

bhanu951’s picture

Version: 2.x-dev » 8.x-1.x-dev

bhanu951’s picture

Status: Patch (to be ported) » Needs review

Ported patch to 8.x-.1.x please review MR #55

  • cmlara committed b69a9b09 on 8.x-1.x authored by Bhanu951
    Issue #3386910 by Bhanu951, yeniatencio, cmlara: Authenticator links...
cmlara’s picture

Status: Needs review » Fixed

LGTM, merged to dev.

Thank you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.