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

chrispinkney · 2021-05-14T03:45:56Z

Co-authored-by: Josue manekenpix@fastmail.com

Issue This PR Addresses

Closes #2153

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

This PR limits how a user can become an admin:

It does this by forcing the PUT and POST routes to set user.isAdmin to false.
It also adds another route for admins to modify another user's isAdmin.

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

…ew PUT route for admins

humphd

This also lacks tests, which I'd like to see get added with these changes.

humphd · 2021-05-14T14:44:46Z

src/api/users/src/routes/users.js

@@ -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;


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.

validateUserRights created.

humphd · 2021-05-14T14:45:07Z

src/api/users/src/routes/users.js

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


Same, this can be done via middleware.

humphd · 2021-05-14T14:46:29Z

src/api/users/src/routes/users.js

+      const userRef = db.doc(id);
+      const doc = await userRef.get();
+
+      if (!doc.exists) {


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

Duplication removed, code moved to updateUser in schema.js

humphd · 2021-05-14T14:47:40Z

src/api/users/src/routes/users.js

+// this route is only accessible by administrators
+// it allows the modification of the isAdmin property
+router.put(
+  '/:id/admin',


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.

Also done in updateUser

humphd · 2021-05-14T14:48:41Z

src/api/users/src/routes/users.js

+  validateId(),
+  validateUser(),
+  validateEmailHash(),
+  isAuthorized((req, user) => {


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

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

PedroFonsecaDEV · 2021-05-15T12:12:34Z

Following.

chrispinkney · 2021-05-16T02:59:39Z

Still have to add tests but I think this is heading in the right direction.

humphd · 2021-05-16T14:27:41Z

config/export/firebase-export-metadata.json

@@ -0,0 +1,8 @@
+{


Why did this file, and the other config/export/* files, get added?

This and the change to development.yml add a single user to the emulator up its bootup. This user as the isAdmin 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.

humphd · 2021-05-16T14:28:20Z

docker/development.yml

@@ -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


Why this change?

humphd · 2021-05-16T14:34:14Z

src/api/users/src/models/schema.js

+      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);


This won't allow downgrading a user from Admin to normal, which we should support. We should accept a value {isAdmin: true|false} on the body and use that instead of hard-coding here.

…also allowed users to demote admin status

chrispinkney · 2022-01-05T04:28:53Z

Given #2555, I wonder if this PR should be closed and migrated to a different issue at a later date (once the suprabase work has been merged)?

@manekenpix
@humphd

humphd · 2022-01-05T14:04:30Z

Agree, this can get closed for now; though, I think it's a good starting point for what needs to happen.

Updated POST and PUT routes to make isAdmin false by default, added n…

43328f6

…ew PUT route for admins

chrispinkney added type: bug Something isn't working type: enhancement New feature or request type: security Security concerns area: microservices labels May 14, 2021

chrispinkney added this to the 2.1 Release milestone May 14, 2021

chrispinkney requested review from humphd, HyperTHD and PedroFonsecaDEV May 14, 2021 03:45

chrispinkney self-assigned this May 14, 2021

vercel bot deployed to Preview May 14, 2021 03:46 View deployment

chrispinkney assigned manekenpix May 14, 2021

chrispinkney linked an issue May 14, 2021 that may be closed by this pull request

Limit how a user becomes an admin #2153

Closed

humphd requested changes May 14, 2021

View reviewed changes

chrispinkney added 2 commits May 15, 2021 22:10

admin user data for emulator

55679bf

added middleware functions

191e3b8

vercel bot deployed to Preview May 16, 2021 02:57 View deployment

humphd requested changes May 16, 2021

View reviewed changes

Modified put route to allow only admins to set other users to admin, …

c33c18c

…also allowed users to demote admin status

vercel bot deployed to Preview June 17, 2021 03:47 View deployment

PedroFonsecaDEV requested a review from humphd June 17, 2021 08:28

manekenpix modified the milestones: 2.1 Release, 2.2 Release, 2.1.5 Release Aug 13, 2021

manekenpix removed this from the 2.2.0 Release milestone Oct 8, 2021

humphd mentioned this pull request Jan 5, 2022

Limit how a user becomes an admin #2153

Closed

humphd closed this Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chrispinkney commented May 14, 2021 •

edited

Loading

humphd left a comment

humphd May 14, 2021

chrispinkney May 16, 2021

humphd May 14, 2021

chrispinkney May 16, 2021

humphd May 14, 2021

chrispinkney May 16, 2021

humphd May 14, 2021

chrispinkney May 16, 2021

humphd May 14, 2021

chrispinkney May 16, 2021

PedroFonsecaDEV commented May 15, 2021

chrispinkney commented May 16, 2021

humphd May 16, 2021

chrispinkney May 16, 2021

humphd May 16, 2021

humphd May 16, 2021

chrispinkney commented Jan 5, 2022

humphd commented Jan 5, 2022

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

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

Conversation

chrispinkney commented May 14, 2021 • edited Loading

Issue This PR Addresses

Type of Change

Description

Checklist

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PedroFonsecaDEV commented May 15, 2021

chrispinkney commented May 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrispinkney commented Jan 5, 2022

humphd commented Jan 5, 2022

chrispinkney commented May 14, 2021 •

edited

Loading