Skip to content

added 2FA TOTP support#1

Open
dhavalsavalia wants to merge 1 commit intokarancode-singh:mainfrom
dhavalsavalia:otp_support
Open

added 2FA TOTP support#1
dhavalsavalia wants to merge 1 commit intokarancode-singh:mainfrom
dhavalsavalia:otp_support

Conversation

@dhavalsavalia
Copy link
Copy Markdown

No description provided.

@karancode-singh
Copy link
Copy Markdown
Owner

Thanks for working on adding the 2FA OTP support. Most of the PR LGTM, however, the jsOTP file is minified and thus difficult to review.
Based on my review of the PR (where I didn't review the minified file), I understand that the user has to manually enter the OTP from their authenticator app on the config page. But the OTP keeps changing every few seconds and by the time the automation process reaches the 2FA page, the OTP might have changed already.
Please correct me if I misunderstood and commit a readable version of the jsOTP file.

@dhavalsavalia
Copy link
Copy Markdown
Author

Hey Karan,

I apologize for poor naming of properties. items['otp'] is actually secret key that user gets when the user first setup TOTP. It can be either QR code or plain-text. IRCC provides both options in their MFA flows.

Based on that secret key and current time of the system it will generate a 6-digit OTP which is done by var otpValue = totp.getOtp(items['otp']); method (ref RFC 6238). This usually reshuffles every 30 seconds, so in theory since your flow is already so fast, and I believe IRCC MFA should have some tolerance to use stale value, newly generated TOTP will never be stale for practical purposes of this plugin.

You can play around this at https://piellardj.github.io/totp-generator/

Thank you reviewing my PR, let me know if I you want to clean up, especially what my auto-formatter did. :)

@dhavalsavalia
Copy link
Copy Markdown
Author

Oh I am sorry to miss jsOTP part. On it.

@karancode-singh
Copy link
Copy Markdown
Owner

Thanks, good stuff! Looks like you got the OTP generation JS from somewhere which is not a problem (you don't have to include a non-minified jsOTP).
I have a concern though, are you aware if a user can have this 2FA setup at 2 places - here and an authenticator app? I don't think a user would be comfortable having their 2FA OTP generated only in this extension. If anything were to happent to this extension, they might lose access to their account altogether which would be catastrophic.
One more small thing, since the plan is publish this to chrome extension store, might be a good idea to give a bit of more info on the config html page as to what needs to be added in the OTP field for a non-techsavvy user.

@dhavalsavalia
Copy link
Copy Markdown
Author

Hey, I am so sorry. My ADHD isn't helping me here lol. No user will use the same key as their existing 2FA (copy from their existing password or 2fa manager).
I will update PR with instructions for a several different password and authenticator apps. Thanks for catching that and extremely sorry for delay.

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.

2 participants