[feat] Super User Privilege Toggle #2111
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| router.patch("/self", authenticate, userValidator.updateUser, users.updateSelf); | ||
| router.get("/", userValidator.getUsers, users.getUsers); | ||
| router.get("/self", authenticate, users.getSelfDetails); | ||
| router.get("/set-super-user-access", authenticate, users.setSuperUserAccessLimiter); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
| router.patch("/self", authenticate, userValidator.updateUser, users.updateSelf); | ||
| router.get("/", userValidator.getUsers, users.getUsers); | ||
| router.get("/self", authenticate, users.getSelfDetails); | ||
| router.get("/set-super-user-access", authenticate, users.setSuperUserAccessLimiter); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
| router.patch("/self", authenticate, userValidator.updateUser, users.updateSelf); | ||
| router.get("/", userValidator.getUsers, users.getUsers); | ||
| router.get("/self", authenticate, users.getSelfDetails); | ||
| router.get("/set-super-user-access", authenticate, users.setSuperUserAccessLimiter); | ||
| router.get("/get-super-user-access-status", authenticate, users.getSuperUserAccessStatus); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
shubhdevelop
force-pushed
the
feat-debug
branch
from
ea36aa4
to
fdaabe5
Compare
| const { userId, superUserAccess } = authService.verifyAuthToken(token); | ||
| const userDoc = await dataAccess.retrieveUsers({ id: userId }); | ||
| let userData; | ||
|
|
||
| // add user data to `req.userData` for further use | ||
| const userData = await dataAccess.retrieveUsers({ id: userId }); | ||
| req.userData = userData.user; | ||
|
|
||
| if (superUserAccess === false) { | ||
| userData = userDoc.user; | ||
| userData.roles.super_user = false; | ||
| userData.superUserAccess = false; | ||
| } else if (superUserAccess === true || superUserAccess === undefined) { | ||
| userData = userDoc.user; | ||
| if (superUserAccess === true) { | ||
| userData.superUserAccess = true; | ||
| } else { | ||
| userData.superUserAccess = undefined; | ||
| } | ||
| } | ||
| req.userData = userData; |
| @@ -19,6 +19,8 @@ router.get("/userId/:userId", users.getUserById); | |||
| router.patch("/self", authenticate, userValidator.updateUser, users.updateSelf); | |||
| router.get("/", userValidator.getUsers, users.getUsers); | |||
| router.get("/self", authenticate, users.getSelfDetails); | |||
| router.get("/set-super-user-access", authenticate, users.setSuperUserAccessLimiter); | |||
There was a problem hiding this comment.
Can you change the route name to be more descriptive?
as this is a GET request and route name is kind on weird
| const setSuperUserAccessLimiter = async (req, res) => { | ||
| try { | ||
| let value; | ||
| switch (req.query.value) { | ||
| case "true": | ||
| value = true; | ||
| break; | ||
| case "false": | ||
| value = false; | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| if (value !== undefined) { | ||
| const token = req.cookies[config.get("userToken.cookieName")]; | ||
| const { userId } = authService.decodeAuthToken(token); | ||
| const newToken = authService.generateAuthToken({ userId, superUserAccess: value }); | ||
| const rdsUiUrl = new URL(config.get("services.rdsUi.baseUrl")); | ||
| res.cookie(config.get("userToken.cookieName"), newToken, { | ||
| domain: rdsUiUrl.hostname, | ||
| expires: new Date(Date.now() + config.get("userToken.ttl") * 1000), | ||
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: "lax", | ||
| }); | ||
| return res.json({ message: "Super User Privilege Access Set!", currentAccess: value }); | ||
| } else { | ||
| return res.boom.badRequest("Wrong value in query param, value can be either true or false"); | ||
| } | ||
| } catch (error) { | ||
| logger.error(`Error while Setting Super Privilege Access Limiter: ${error}`); | ||
| return res.boom.badImplementation({ message: INTERNAL_SERVER_ERROR }); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
This function become complex and not readable so fix this
|
We need to discuss the approach to it. I am not in favor of this approach. Let's discuss about it with the team before processing please |
|
Also, let's not restrict ourselves to superuser privileges. Lets take a step back and discuss about it |
Sure, whenever you free to discuss |
Date: 30 Aug 2024
Developer Name: Shubham Raj
Issue Ticket Number
#2110
Description
This Solution involves the mutating the userData Object returned from the database before adding it to the response object, which then is used in authenticateRoles middleware for authentication of roles.
Added two routes, one that will help us get the current value of
SuperUserAccessas the jwt token in user browser are encrypted2nd route's purpose would be to send an updated jwt token, with the
SuperUserAccessproperty in addition with the id of the user.In the authenticateUser middleware if the value of the
SuperUserAccess === false, we’ll remove the super_user property or either set it to false, from the userData Object this should make the authorizeRoles middleware return an unauthorised user response back to the user.If the cookie’s value is either true or undefined(not present in the request), we shouldn’t be tampering with the user_object and add the returned
userDataobject as it is to the Response object, this should make the authorizeRoles middleware to authorise if the roles exist and call tonext().Depending up the value of
SuperUserAccesswe also modify the user object in the/users/selfand thesuper_uservalue accordingly before sending it to the user.Documentation Updated?
Once this PR gets merged I'll update the the api-contract documentation as well
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screens Capture
Development Testing
`/users/:userId/intro is an routes accessible only when super_user role is present on the user
Screencast.from.30-08-24.02.39.43.PM.IST.webm
Screencast.from.30-08-24.02.43.28.PM.IST.webm
Test Coverage
Screenshot 1
Test coverage for the changes in /users/self route
Test coverage for 2 routes that were added
Additional Notes
For more Detail one can refer to this design doc
https://docs.google.com/document/d/17khsviClkNVNYH4Xy5Pkk98Xvrid1jH1jL9QtfGp7aw/edit?usp=sharing