Conversation
rate limitation is not implemented on backend, so remove the frontend part
melolw
left a comment
melolw
left a comment
There was a problem hiding this comment.
I also have a general question because I haven't seen something like limiting the amount to tries? Like if someone tries to brute force all 6 digits possibilities, is there something stopping them?
Overall the code is great! I learned a lot along the way, I have limited knowledge about authentification so most of my questions are things I saw in theory only. I also think it's a great addition to the software, 2FA is such an important feature 😎
| /** @type {import('sequelize-cli').Migration} */ | ||
| module.exports = { | ||
| async up(queryInterface, Sequelize) { | ||
| await queryInterface.changeColumn("user", "email", { |
There was a problem hiding this comment.
[QUESTION] should this also be in the down function? changing back the column "email" as it was
Also, why are we allowing null for emails?
There was a problem hiding this comment.
Resolved in commit ee743fc
We are allowing email to be Null now because we added OrcId login method this time, which means users can log in using their orcid account. Orcid users can choose not to share their email with our platform. In this case, we cannot get such information. If we are not allowing email to be null, then we will run into issue when creating user account for orcid users.
| }, | ||
| { | ||
| key: "system.auth.orcid.clientSecret", | ||
| type: "string", |
There was a problem hiding this comment.
[QUESTION] i'm not very knowledgable about security, but is this (and a few others in this file) fine to put as a string in the database?
There was a problem hiding this comment.
Database encryption will be left with Junaid to handle.
| // TOTP for 2FA | ||
| totpSecret: DataTypes.STRING, | ||
| // External login method identifiers | ||
| orcidId: DataTypes.STRING, |
There was a problem hiding this comment.
[question] should we add unique: true here too?
There was a problem hiding this comment.
We usually set the extra rules in the migration files; here we do not need to add unique: true.
backend/utils/auth.js
Outdated
| */ | ||
| exports.generateOTP = function generateOTP() { | ||
| // Generate a random 6-digit number (000000 to 999999) | ||
| const otp = Math.floor(100000 + Math.random() * 900000).toString(); |
There was a problem hiding this comment.
[suggestion] could we use Node's crypto module? i read here (https://www.boot.dev/blog/javascript/node-js-random-number/) that math.random is not safe and here (https://www.geeksforgeeks.org/node-js/node-js-crypto-randomint-method/) and here (https://www.w3schools.com/nodejs/nodejs_crypto.asp) is a method for generating a random number with crypto
There was a problem hiding this comment.
This is a good catch! Yes, since we use Node.js, it's better to use its module. I have updated it in commit 3c25645
| if (typeof raw === "string" && raw.length > 0) { | ||
| this.methods = raw.split(",").filter((m) => !!m); | ||
| } | ||
| // Fallback: just support email/totp if nothing present |
There was a problem hiding this comment.
[question] why email/totp? why not only email?
There was a problem hiding this comment.
Users are directed here only when they have two 2FA methods, so the fallback should be email/totp as well; otherwise, they wouldn't be on this page, but TwoFactorVerifyEmail or TwoFactorVerifyTotp.
| <button | ||
| class="btn btn-primary btn-block" | ||
| type="submit" | ||
| :disabled="!selectedMethod || isSubmitting" |
There was a problem hiding this comment.
[suggestion] could we add that to the cancel and radio button too? so that the user can't cancel when they are already submitting
There was a problem hiding this comment.
This is a very cautious suggestion, but I am not sure if it is necessary. The user will be directed to the next page as soon as they click on "Continue" button. There is little to no time for them to click “Cancel.”
| "A new verification code has been sent to your Email"; | ||
|
|
||
| // Start cooldown timer (60 seconds) | ||
| this.resendCooldown = 60; |
There was a problem hiding this comment.
[question] what if they reload the page? is the cooldown also tracked somewhere in the backend? could not find it
You are right! I have implemented the rate limit to prevent this from happening. |
dennis-zyska
left a comment
dennis-zyska
left a comment
There was a problem hiding this comment.
Really nice implementation, the code is already in a really good shape, but there are some smaller issues, like a missing docstring. It could also be rearranged a bit to make the code more understandable.
backend/webserver/routes/auth.js
Outdated
| const EMAIL_2FA_RESEND_COOLDOWN_MS = 60 * 1000; | ||
| const MAX_2FA_VERIFY_ATTEMPTS = 5; | ||
|
|
||
| function getEmailOtpCooldownInfo(pending) { |
There was a problem hiding this comment.
Here are a few functions without a docstring.
backend/webserver/routes/auth.js
Outdated
| return res.status(500).json({ message: "Session error during 2FA." }); | ||
| } | ||
|
|
||
| return res.status(429).json({ |
There was a problem hiding this comment.
Can we also add here RetryAfter information? (https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Retry-After) Or is it final? If it is final, the 429 error would not apply, and I think it should be possible again after some time, right?
backend/webserver/routes/auth.js
Outdated
| { where: { id: userRecord.id } } | ||
| ); | ||
|
|
||
| await server.sendMail( |
There was a problem hiding this comment.
We should use the email template feature we already integrated. Please have a look here:
CARE/backend/webserver/routes/auth.js
Line 295 in 7cdc0ed
(We don't want to save the mail in the source code, it should be in the database and as file on disk for fallback)
But you can also ask @mohammadsherif0
backend/webserver/routes/auth.js
Outdated
| * @param {string} [options.redirectPath='/dashboard'] - The path to redirect to if mode is 'redirect'. | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async function finalizeLogin(req, res, user, options = { mode: 'json', redirectPath: '/dashboard' }) { |
There was a problem hiding this comment.
What if we open a study URL before logging in to the account? Are we still redirected to that study URL after successful login
backend/webserver/routes/auth.js
Outdated
There was a problem hiding this comment.
This is a huge file, I think we can create a subfolder ./backend/webserver/routes/auth/... and create subfiles for examples ./auth/totp.js for each method.
backend/webserver/Server.js
Outdated
| const SAML_ATTRIBUTE_KEYS = { | ||
| email: [ | ||
| "email", | ||
| "mail", | ||
| "urn:oid:1.2.840.113549.1.9.1", | ||
| "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress", | ||
| ], | ||
| firstName: [ | ||
| "firstName", | ||
| "givenName", | ||
| "urn:oid:2.5.4.42", | ||
| "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname", | ||
| ], | ||
| lastName: [ | ||
| "lastName", | ||
| "sn", | ||
| "surname", | ||
| "familyName", | ||
| "urn:oid:2.5.4.4", | ||
| "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname", | ||
| ], | ||
| }; | ||
|
|
||
| function getFirstPresentValue(source, keys = []) { | ||
| for (const key of keys) { | ||
| const value = source?.[key]; | ||
| if (Array.isArray(value) && value.length > 0 && value[0]) { | ||
| return value[0]; | ||
| } | ||
| if (value) return value; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| function getProvisionedNameParts({ firstName, lastName, email, fullName, fallbackFirstName, fallbackLastName }) { | ||
| const normalizedFirstName = Array.isArray(firstName) ? firstName[0] : firstName; | ||
| const normalizedLastName = Array.isArray(lastName) ? lastName[0] : lastName; | ||
| const toDisplayNamePart = (value, fallback) => { | ||
| if (!value) return fallback; | ||
| return value.charAt(0).toUpperCase() + value.slice(1); | ||
| }; | ||
|
|
||
| if (normalizedFirstName && normalizedLastName) { | ||
| return { firstName: normalizedFirstName, lastName: normalizedLastName }; | ||
| } | ||
|
|
||
| if (email) { | ||
| const localPart = (email || "").split("@")[0] || ""; | ||
| const [rawFirstName, ...rest] = localPart.split(".").filter(Boolean); | ||
| const rawLastName = rest.join("."); | ||
| return { | ||
| firstName: normalizedFirstName || toDisplayNamePart(rawFirstName, fallbackFirstName), | ||
| lastName: normalizedLastName || toDisplayNamePart(rawLastName, fallbackLastName), | ||
| }; | ||
| } | ||
|
|
||
| const parts = (fullName || "").trim().split(/\s+/).filter(Boolean); | ||
| return { | ||
| firstName: normalizedFirstName || toDisplayNamePart(parts[0], fallbackFirstName), | ||
| lastName: normalizedLastName || toDisplayNamePart(parts.slice(1).join(" "), fallbackLastName), | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
The docstrings are missing, can we move this block to some files like ../utils/auth.js , this would be much cleaner
backend/webserver/Server.js
Outdated
| await this.#setupLocalStrategy(); | ||
| await this.#setupOrcidStrategy(); | ||
| await this.#setupLdapStrategy(); | ||
| await this.#setupSamlStrategy(); |
There was a problem hiding this comment.
Maybe we can move those functions as well into an extra file for the loginManagement (like a class Login.js that holds everything that is important regarding the Login). This file is also already really big and it would make it easier to get
frontend/src/router.js
Outdated
| path: "/2fa/verify/email", | ||
| name: "2fa-verify-email", | ||
| component: () => import("@/auth/TwoFactorVerifyEmail.vue"), | ||
| meta: { requiresAuth: false } |
There was a problem hiding this comment.
What is with the additional meta information like hideTopbar and checkLogin, also in the other new routes?
# Conflicts: # backend/webserver/routes/auth.js # frontend/src/components/dashboard/settings/SettingItem.vue
3 participants
Main Description
This merge request introduces 2fa implementation and third-party login methods.
New User Features
Note
Known Limitations