Conversation
in43sh
left a comment
in43sh
left a comment
There was a problem hiding this comment.
Hi Bino! Thank you for implementing this important feature. It's working properly, but I would like you to address some changes that I put in this review. Let me know what you think about it.
| @@ -0,0 +1,4 @@ | |||
| PORT=8000 | |||
There was a problem hiding this comment.
For security reasons, I would suggest to not use the real backend port.
| @@ -0,0 +1,4 @@ | |||
| PORT=8000 | |||
| MONGO_URI=mongodb+srv://...cluster | |||
There was a problem hiding this comment.
I've seen it before that MONGO_URI in .env.example is written like mongodb+srv://username:password@cluster.example.com/mydatabase.
| @@ -0,0 +1,4 @@ | |||
| PORT=8000 | |||
| MONGO_URI=mongodb+srv://...cluster | |||
| SECRET=SECRET | |||
There was a problem hiding this comment.
Again for security reasons, I would not exposed a real secret key for JWT token. You can replace it with YOUR_SECRET_HERE.
There was a problem hiding this comment.
Also, I would replace SECRET with JWT_SECRET otherwise, it can be confused with hashing/salting secret.
| PORT=8000 | ||
| MONGO_URI=mongodb+srv://...cluster | ||
| SECRET=SECRET | ||
| CLIENT_URL=http://localhost:3000 |
There was a problem hiding this comment.
This may or may not be sensitive, but I would rather avoid using a real port from production in a publicly available file.
| * @GETUSER | ||
| * @route /api/v1/login | ||
| * @method GET | ||
| * @description retrieve user data from mongoDb if user is valid(jwt auth) |
There was a problem hiding this comment.
This description is confusing. Could you rephrase it?
| /****************************************************** | ||
| * @GETADMIN | ||
| * @route /api/v1/admin | ||
| * @method GET |
There was a problem hiding this comment.
In the application, you are using PUT method, so I would change it here too.
| * @GETADMIN | ||
| * @route /api/v1/admin | ||
| * @method GET | ||
| * @description retrieve user data from mongoDb if user is valid(jwt auth) |
There was a problem hiding this comment.
The description here is the same as in the previous method. Could you update it?
| * @route /api/v1/admin | ||
| * @method GET | ||
| * @description retrieve user data from mongoDb if user is valid(jwt auth) | ||
| * @returns User Object |
There was a problem hiding this comment.
Could you update what this method returns?
| * @returns User Object | ||
| ******************************************************/ | ||
|
|
||
| const getAdmin = async (req, res) => { |
There was a problem hiding this comment.
I would change this controller name so that it reflects its purpose. Here's an alternative: grantAdminRole.
| router | ||
| .route("/login") | ||
| .post(loginDataValidate, logIn) | ||
| .get(authenticateUser, getUser); |
There was a problem hiding this comment.
It's confusing to me that when I send a GET request to /login, I get the user info that is logged in. Maybe you could change it? An alternative is /currentuser.
| const adminRole = async (req, res, next) => { | ||
| const { role, userId } = req.user; | ||
| try { | ||
| if (role !== "admin") { |
There was a problem hiding this comment.
If I'm following the logic here correctly, any user can make a PUT request to update their own role to be an admin, is this correct? This would be insecure since any user could create their own account then assign themselves to be an admin.
In general I'd recommend that only admins can create admin users. It'd be good to add logic to this endpoint that the requesting user must have the admin role otherwise return a 403 forbidden.
But then you may ask, how do we create the first admin user? 👀 The first admin user can be created directly in the database. https://www.shellhacks.com/mongodb-create-user-database-admin-root/. You could either proceed with creating any other future admins in the database directly, or expose an endpoint for admin users to be able to create other admin users.
3 participants
Admin Route may help us to change the user role as admin .
We need this because only admin can add and delete quizzes .