-
Notifications
You must be signed in to change notification settings - Fork 189
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
Changes from 3 commits
43328f6
55679bf
191e3b8
c33c18c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"version": "9.10.2", | ||
"firestore": { | ||
"version": "1.11.15", | ||
"path": "firestore_export", | ||
"metadata_file": "firestore_export/firestore_export.overall_export_metadata" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,8 @@ services: | |
volumes: | ||
# Copy firebase.config into the container so we get proper ip/port binding | ||
- ../config/firebase.json:/home/node/firebase.json | ||
command: firebase emulators:start --project telescope --only firestore | ||
- ../config/export:/export | ||
command: firebase emulators:start --project telescope --only firestore --import=/export | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change? |
||
ports: | ||
# Emulator Suite UI | ||
- '4000:4000' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,34 @@ const validateId = () => | |
}, | ||
}); | ||
|
||
// no user can be created as an admin by default | ||
const validateUserRights = () => (req, res, next) => { | ||
req.body.isAdmin = false; | ||
next(); | ||
}; | ||
|
||
const updateUser = (makeAdmin = false) => async (req, res, next) => { | ||
const { id } = req.params; | ||
const { body } = req; | ||
|
||
try { | ||
const userRef = db.doc(id); | ||
const doc = await userRef.get(); | ||
|
||
if (!doc.exists) { | ||
next(createError(404, `user ${id} not found.`)); | ||
} else { | ||
// if makeAdmin is true turn the user into an admin, if not use body to update user. | ||
const user = new User(makeAdmin ? { ...body, isAdmin: true } : body); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This won't allow downgrading a user from Admin to normal, which we should support. We should accept a value |
||
// 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); | ||
} | ||
}; | ||
|
||
// Generate a display name if none given | ||
const generateDisplayName = (parent) => `${parent.firstName} ${parent.lastName}`; | ||
|
||
|
@@ -52,5 +80,7 @@ const validateEmailHash = () => (req, res, next) => { | |
|
||
exports.validateUser = validateUser; | ||
exports.validateId = validateId; | ||
exports.validateUserRights = validateUserRights; | ||
exports.validateEmailHash = validateEmailHash; | ||
exports.validatePagingParams = validatePagingParams; | ||
exports.updateUser = updateUser; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,8 @@ const { | |
validateId, | ||
validateUser, | ||
validateEmailHash, | ||
validateUserRights, | ||
updateUser, | ||
} = require('../models/schema'); | ||
const { User } = require('../models/user'); | ||
const { db, documentId } = require('../services/firestore'); | ||
|
@@ -90,6 +92,7 @@ router.post( | |
isAuthenticated(), | ||
validateId(), | ||
validateUser(), | ||
validateUserRights(), | ||
validateEmailHash(), | ||
isAuthorized((req, user) => { | ||
// A user can add their own data (signup) | ||
|
@@ -128,6 +131,7 @@ router.put( | |
validateId(), | ||
validateUser(), | ||
validateEmailHash(), | ||
validateUserRights(), | ||
isAuthorized((req, user) => { | ||
// A user can update their own data | ||
if (user.sub === req.params.id) { | ||
|
@@ -136,26 +140,21 @@ router.put( | |
// Or an admin, or another authorized microservice | ||
return user.roles.includes('admin') || user.roles.includes('service'); | ||
}), | ||
async (req, res, next) => { | ||
const { id } = req.params; | ||
const { body } = req; | ||
|
||
try { | ||
const userRef = db.doc(id); | ||
const doc = await userRef.get(); | ||
updateUser() | ||
); | ||
|
||
if (!doc.exists) { | ||
next(createError(404, `user ${id} not found.`)); | ||
} else { | ||
const user = new User(body); | ||
// 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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also done in |
||
isAuthenticated(), | ||
validateId(), | ||
validateUser(), | ||
validateEmailHash(), | ||
isAuthorized((req, user) => user.roles.includes('admin')), | ||
updateUser(true) | ||
); | ||
|
||
// delete a user with a supplied id, validated by the schema | ||
|
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.
Why did this file, and the other
config/export/*
files, get added?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 and the change to
development.yml
add a single user to the emulator up its bootup. This user as theisAdmin
flag set to true, as without this we'd have no way to make a user an admin during testing.The problem stems from having no way of creating an admin without first having an admin to create an admin.