-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Admin Route #4
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen it before that MONGO_URI in .env.example is written like mongodb+srv://username:[email protected]/mydatabase
.
@@ -0,0 +1,4 @@ | |||
PORT=8000 | |||
MONGO_URI=mongodb+srv://...cluster | |||
SECRET=SECRET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description is confusing. Could you rephrase it?
/****************************************************** | ||
* @GETADMIN | ||
* @route /api/v1/admin | ||
* @method GET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update what this method returns?
* @returns User Object | ||
******************************************************/ | ||
|
||
const getAdmin = async (req, res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Admin Route may help us to change the user role as admin .
We need this because only admin can add and delete quizzes .