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

Closes Issue #2153 - Limit how a user becomes an admin #2153 #2239

Closed
wants to merge 4 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/api/users/src/routes/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ router.post(
next(createError(409, `user with id ${id} already exists.`));
} else {
const user = new User(body);
// no user can be created as an admin by default
user.isAdmin = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this code into a new middleware function that happens just before the route's logic gets hit. Maybe call it validateUserRights() and it can simply set user.isAdmin to false.

The logic in the route should be focused on talking to the db and returning HTTP results, nothing more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateUserRights created.

await db.doc(id).set(user);
res.status(201).json({ msg: `Added user with id: ${id}` });
}
Expand Down Expand Up @@ -140,6 +142,43 @@ router.put(
const { id } = req.params;
const { body } = req;

try {
const userRef = db.doc(id);
const doc = await userRef.get();

if (!doc.exists) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code seems to be duplicated between a few put calls now. Can you extract it into middleware and share it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplication removed, code moved to updateUser in schema.js

next(createError(404, `user ${id} not found.`));
} else {
const user = new User(body);
// no user can be created as an admin by default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, this can be done via middleware.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

user.isAdmin = false;
// NOTE: doc().update() doesn't use the converter, we have to make a plain object.
await db.doc(id).update(user.toJSON());
res.status(200).json({ msg: `Updated user ${id}` });
}
} catch (err) {
next(err);
}
}
);

// put (update) a user with a supplied id, validated by the schema
// rejected if a user could not be found, otherwise user updated
// this route is only accessible by administrators
// it allows the modification of the isAdmin property
router.put(
'/:id/admin',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, but one question: the naming implies that you are only setting the admin aspect of this record vs. all fields, yet you are allowing other fields to be updated here. I would limit it to just the admin change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also done in updateUser

isAuthenticated(),
validateId(),
validateUser(),
validateEmailHash(),
isAuthorized((req, user) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrow functions with body statements that immediately return a value should be rewritten as inline expressions:

isAuthorized((req, user) => user.roles.includes('admin')),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return user.roles.includes('admin');
}),
async (req, res, next) => {
const { id } = req.params;
const { body } = req;

try {
const userRef = db.doc(id);
const doc = await userRef.get();
Expand Down