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

Adding MFA #5664

Closed
wants to merge 1 commit into from
Closed

Adding MFA #5664

wants to merge 1 commit into from

Conversation

Codexnever
Copy link

Description

This pull request introduces a Multi-Factor Authentication (MFA) system to enhance security in the application. The MFA system adds an additional layer of verification during login, requiring users to enter a one-time verification code generated by an authenticator app after successfully providing their username and password.

Changes Made

  • Added MFA setup and authentication routes (/mfa/setup and /mfa/authenticate) to handle the setup process and code verification during login.
  • Implemented MFA setup and authentication logic using the speakeasy library, generating secret keys and QR codes for users to scan during setup.
  • Updated the user data structure to include fields for storing the MFA status and secret key.
  • Added new EJS views (mfa-setup.ejs and mfa-authentication.ejs) to facilitate the MFA setup process and code verification.
  • Updated the README.md file to include instructions for using the MFA system and a list of dependencies.

Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

Thanks @Codexnever for this contribution! I think is a great idea to add support for MFA, thanks for work on it :)

I think that maybe moving this to a separate example will fit better, as that way we can have basic auth and MFA auth covered

package.json Show resolved Hide resolved
Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

I made some additional comments on the dependencies, as well is important to update the test: https://github.com/expressjs/express/blob/master/test/acceptance/auth.js

"qs": "6.11.0",
"range-parser": "~1.2.1",
"safe-buffer": "5.2.1",
"send": "0.18.0",
"serve-static": "1.15.0",
"setprototypeof": "1.2.0",
"speakeasy": "^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Seems like is not maintained anymore: https://github.com/speakeasyjs/speakeasy

Copy link
Author

Choose a reason for hiding this comment

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

i will update other depandency

"path-to-regexp": "0.1.7",
"proxy-addr": "~2.0.7",
"qrcode": "^1.5.3",
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

i will fix it

"merge-descriptors": "1.0.1",
"methods": "~1.1.2",
"on-finished": "2.4.1",
"parseurl": "~1.3.3",
"path": "^0.12.7",
Copy link
Member

Choose a reason for hiding this comment

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

This is an invalid dependency:

This is an exact copy of the NodeJS ’path’ module published to the NPM registry.

https://www.npmjs.com/package/path

"http-errors": "2.0.0",
"i18n": "^0.15.1",
Copy link
Member

Choose a reason for hiding this comment

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

@@ -40,21 +40,28 @@
"encodeurl": "~2.0.0",
"escape-html": "~1.0.3",
"etag": "~1.8.1",
"express": "^4.19.2",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"express": "^4.19.2",

Express itself should not be included

@Codexnever Codexnever closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants