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

Add two-factor authentication #473

Merged
merged 15 commits into from
Aug 28, 2023
Merged

Add two-factor authentication #473

merged 15 commits into from
Aug 28, 2023

Conversation

JVT038
Copy link
Collaborator

@JVT038 JVT038 commented Aug 15, 2023

This PR adds 2FA and makes some changes surrounding the password and the login flow.

The password page is renamed to 'security' and so are all the routes related to the former password page.

To enable 2FA, the user can go to the security tab and they can click on a button which opens a modal. When clicking on the button, a post request will be sent to generate a TOTP URI and JS will process the URI to encode it in a QR code, which is shown in the modal. Additionally, the TOTP secret is also just shown in plain text below the QR code, if the user wants to manually copy the secret. (Useful for password managers such as Bitwarden).

The user has to import the TOTP URI into their verification app and enter the correct 6-digit code to enable 2FA.

The user must now first enter their email + password and click on the sign in button. (like usual)
If the user has a filled totp_uri column in the database, Movary will assume they have enabled 2FA. The user will now be redirected back to the login page, but now with the 2FA form instead. They will enter 6 digits and Movary will check if it's correct.

If it is correct, they will be authenticated. Additionally, if they have ticked the 'Remember me' box on the 2FA page, another cookie will be created to ensure they won't have to enter a 2FA code for the next 30 days. Question: Should this cookie be removed upon logout? It's currently not removed when the user logs out.

When the user has entered the code incorrectly, they will be redirected back to the 2FA page and see an error message, telling them it was wrong.

@JVT038 JVT038 requested a review from leepeuker as a code owner August 15, 2023 19:50
Comment on lines 15 to 17
private const REGENERATION_TIME = 30;
private const DIGEST_ALGORITHM = 'sha1';
private const DIGITS = 6;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have chosen for these values as in my experience, most (if not all) verification codes wait 30 seconds before generating a new code, use the sha1 algorithm to generate the secret and have 6 digits for the verification code itself.

@JVT038 JVT038 linked an issue Aug 15, 2023 that may be closed by this pull request
@JVT038
Copy link
Collaborator Author

JVT038 commented Aug 19, 2023

I've also been considering to make a CookieWrapper class that basically centralizes all the cookie-related methods (such as calculating the expiry date) into one class, because it's currently a bit of a mess with cookies IMO.

@leepeuker
Copy link
Owner

I have started looking into the PR a bit, I noticed that I cannot scan the QR code, I am using Aeagis on Android. Entering the code manually works.

@JVT038
Copy link
Collaborator Author

JVT038 commented Aug 27, 2023

Huh? I tried it (I'm also using Aegis on Android) and it works on my side.

Could you check the PHP logs and the browser console logs for any errors?
Also, have you installed the new composer packages and ran the migrations?

Does the QR code appear at all? If so, could you send a screenshot of the QR code and could you run the following command in the browser console: document.getElementById('qrcode').getAttribute('title')? I want to know if the generated QR code is valid and what the contents of the QR code are.

The QR code should contain something like this: otpauth://totp/Movary%3AJVT038?issuer=Movary&secret=<mysecret>

@leepeuker
Copy link
Owner

leepeuker commented Aug 28, 2023

The QR code is displayed, but when scanning it with Aegis simply nothing happens, as if Aegis does not recognize the QR code.

image

the title attribute value is: otpauth://totp/Movary%3Alee?issuer=Movary&secret=XAS2M34SVRCOQMTOPNNA2FOHEIHCKKRUU4B3OI4JUCC4INKXTEYA

@JVT038
Copy link
Collaborator Author

JVT038 commented Aug 28, 2023

The issue was that Aegis was unable to detect the QR code, because of the dark theme.
I have added a 1px solid white border to the QR code div and it's barely noticeable in the dark theme, but noticeable enough so that Aegis can scan the code.

@leepeuker leepeuker merged commit fef4be8 into main Aug 28, 2023
@leepeuker leepeuker deleted the add-totp-authentication branch August 28, 2023 13:43
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.

Add 2FA (Two-Factor Authentication) with time-based code
2 participants