Conversation
router.go(0) calls the browser's native history.go(0) under the hood, which forces a full page reload. This resets the vuex store back to its initial state, ensuring the correct user data is displayed after login. Without this, there is currently no explicit store reset on logout, which caused the previous user's data to persist into the next session.
…ime updating operation
…handling method, and streamline loginManagement method
…g user data retrieval
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?
| }, | ||
| { | ||
| 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?
| // 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.
| */ | ||
| 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
| 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?
| <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
| "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
2 participants
Main Description
This merge request introduces 2fa implementation and third-party login methods.
New User Features
Note
Known Limitations