Skip to content
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

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Add Admin Route #4

wants to merge 3 commits into from

Conversation

Bino26
Copy link
Collaborator

@Bino26 Bino26 commented Dec 2, 2023

Admin Route may help us to change the user role as admin .
We need this because only admin can add and delete quizzes .

Copy link
Collaborator

@in43sh in43sh left a 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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator

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
Copy link
Collaborator

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)
Copy link
Collaborator

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
Copy link
Collaborator

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)
Copy link
Collaborator

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
Copy link
Collaborator

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) => {
Copy link
Collaborator

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);
Copy link
Collaborator

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") {
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants