Open Jakousa opened 3 years ago
Addition:
We got a notice from our plagiarism detection. No worries but:
You should add comment with reference where you got the code when copying code from other sources. https://github.com/lelevan3010/2fa-auth/blob/main/server/src/utils/otp.js - https://github.com/OnFrontiers/mfa-demo-node/blob/master/server/otp.js
E.g.
// Implementation of 2fa from https://medium.com/onfrontiers-engineering/two-factor-authentication-flow-with-node-and-react-7cbdf249f13
import crypto from 'crypto';
import base32Decode from 'base32-decode';
I have added the reference. Really appreciate your review!
Hi Jami,
I follow the link on Github to register for Open University to get credits for my study at Metropolia UAS, but the link seemed not to work. What should I do in this case?
Best regards, Van Nguyen
On Wed, Mar 17, 2021 at 1:41 PM Jami Kousa @.***> wrote:
Addition:
We got a notice from our plagiarism detection. No worries but:
You should add comment with reference where you got the code when copying code from other sources. https://github.com/lelevan3010/2fa-auth/blob/main/server/src/utils/otp.js
E.g.
// Implementation of 2fa from https://medium.com/onfrontiers-engineering/two-factor-authentication-flow-with-node-and-react-7cbdf249f13import crypto from 'crypto';import base32Decode from 'base32-decode';
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/lelevan3010/2fa-auth/issues/2#issuecomment-801015138, or unsubscribe https://github.com/notifications/unsubscribe-auth/AH7PHKLGKMSX2M6WBCNUE6DTECIPDANCNFSM4ZKGMUWA .
This is an extension to the previous review, as mentioned in the previous the comments in the review are suggestions that you can use in this project or in your future projects. None of the suggestions are required to get a passing grade.
Registered with a new account
Logged in, Logged out, Logged in
Searched for a plant
The website looks fantastic. Only issue I had was the "missing image" icon when I was logging back in after logout.
2fa worked perfectly. However, I'd still add the text / guide to add 2fa during first login.
It seems you don't have express-async-errors yet. It would enable you to remove 50% of the code in all 3 functions here: https://github.com/lelevan3010/2fa-auth/blob/1bdb33ac4005d5c6d96d08e71a9663b09fba1c32/server/src/resources/history/history.controller.js
Avoid commiting comments that are useless https://github.com/lelevan3010/2fa-auth/blob/main/server/src/index.js#L27-L34 Git is better version control than comments anyway, so just commit that you deleted some code and you can find it at anytime.
The code looks nice, great!
Well done, I hope you keep at it and do the final missing items from your todo list!
Full Stack project review
Here's a short review of your Full Stack course project. The comments in the review are suggestions that you can use in this project or in your future projects. None of the suggestions are required to get a passing grade.
User experience
What did I do??
Registered
Logged in
Logged out and back in
Tried to add an image
Experience
The application looks really nice, login flow was good. You could add "Please read this qr code with your authenticator app" or equivalent to guide user during first login.
After logging out, when going back in I got
And when adding an image the application crashed with
Code
Well structured project.
Read about guard clause here https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html.
Next, let's refactor this part of your code to be a bit more readable: https://github.com/lelevan3010/2fa-auth/blob/627d571800676c485b2ea01bb62faf87a3b1cd4e/server/src/resources/user/user.service.js#L12-L24
A bit easier to read. Next we could move the unauthorized to a separate function.
And instead of doing copy paste of
You can use an error middleware and https://www.npmjs.com/package/express-async-errors
Frontend code also has well structured and short components. Great!
Inline styles is probably ok for a project of this size.
Conclusion
Nice small POC project for your 2fa implementation. Great job