Conversation
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
There was a problem hiding this comment.
I check the template related feature and there are a number of things that feel confusing (but maybe I just don't understand this feature well enough):
- the return value of getEmailContent contains fallback.subject, which means this feature couldn't work without fallback. In this sense, it's not a fallback mechanism, it's a dependence, right? I wonder why we didn't save the subject information into the DB, but depend on this two-step procedure of retrieving information required for an email.
CARE/backend/utils/emailHelper.js
Line 78 in 6945b3b
- When I tried to create a new system email, I needed to check several files, modify several places such as templateResolver, create migration, and create fallback files. The procedure feels a bit fragmented. Is it intended like this?
- It’s related to the previous point. if the new system email requires new kinds of placeholder, we need to go through migration, check frontend (to if the frontend component supports this placeholder) and may need to expand the backed logic. It feels a bit complicated from my perspective.
- The placeholder table has the column, type, but it does not mean the type of the placeholder, but the type of template, which makes me feel like it’s a foreignKey, while in reality, it isn’t. The naming is a bit confusing for two reasons:
- If it’s not a foreign key, then there is no restraint upon it. It could be a type that does not exist in Template table and the database wouldn’t give us warning.
- I would actually assume that the type of placeholder takes value such as ‘text’ (what placeholderType does/holds now)and we have another column called, say, templateType.
- On the frontend, the Publish Template feature shows a list of templates published by others, excluding our own public templates. I know there is description explaining this, but I kind of feel it’s not intuitive if we exclude public templates from our own.
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
There was a problem hiding this comment.
Your observation was right. There wasn't the redirect mechanism in place previously.
Resolved it in commit 8bb7777.
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
There was a problem hiding this comment.
To add more context, I organized the strategies as functions rather than a class because this layer is mostly stateless startup logic and only gets initialized once when the server starts up. But if you think this is not a good idea, please let me know.
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