hackforla / VRMS

Volunteer Relationship Management System: This is an ambitious project to create a system that will help us measure our human capital development, reduce repetitive tasks and processes, and improve outcomes.
GNU Affero General Public License v3.0
40 stars 79 forks source link

Fix Super Admin User Login issue #1802

Open JackHaeg opened 1 month ago

JackHaeg commented 1 month ago

Overview

After implementing the Super Admin User feature in #1747, we identified a bug where the Super Admin User (vrms-admin-perm@hackforla.org) is unable to log in to VRMS. This issue tracks the fix for this bug.

Past Research

Bug summary from Jack Haeger_2024-10-17

Trillium merged the [Super User PR](https://github.com/hackforla/VRMS/pull/1765), rebuilt Dev, and edited the DB to make [vrms-admin-perm@hackforla.org](mailto:vrms-admin-perm@hackforla.org) a super user. The good news is that almost everything is working as expected (the user screen is grayed out and no other admin users can edit this user). The bad news is that we cannot log in with this user to [dev.vrms.io](http://dev.vrms.io/): - When we go to log in with that user ([vrms-admin-perm@hackforla.org](mailto:vrms-admin-perm@hackforla.org)) we are unable to log in and we receive the error message “We don’t recognize your email address. Please, create an account.” - I can confirm that this account already existed due to the fact that 1) the account shows up in [dev.VRMS.io](http://dev.vrms.io/), and 2) the account’s email inbox received VRMS magic links to log in to the [dev.vrms.io](http://dev.vrms.io/) website on September 16, so it clearly existed and was logging in appropriately. - Also, this email address logs in as expected on PROD. ![Screenshot 2024-10-17 at 3 24 47 PM (1)](https://github.com/user-attachments/assets/816c62cb-2b45-4b98-8c53-caa2d79f19b8)

Nikhil's research_2024-10-22

The checkAuth Method has authOrigin Set to LOG_IN ![image (3)](https://github.com/user-attachments/assets/5040057e-28d2-433c-8aab-a1a95b327e3c) It fetches SIGN_IN ![image (4)](https://github.com/user-attachments/assets/d9ddb69b-17b8-4287-88fe-e50beaf9eb15) Which is exposed by this route ![image (5)](https://github.com/user-attachments/assets/23d7637b-8d4d-4eba-a14a-76ccc0c22085) That router works in this way and it checks for a method called verifyUser.isAdminByEmai ![image (6)](https://github.com/user-attachments/assets/c86fd665-ed15-4545-b2c7-d7b8533490fb) And voila ![image (7)](https://github.com/user-attachments/assets/dcef0b9c-d84a-42e7-b561-702bc64a7fa5) if (role === 'admin' || user.managedProjects.length > 0) It is allowing either "admin" or someone who has managedProjects.length>0 Now if we look at the super user Neither are they "admin" and their "managedProjects" empty So they are not able to log in ![image (8)](https://github.com/user-attachments/assets/269e918d-5d78-4c1a-98af-91651fd24637) But when you see Trillium's profile, Jack Haeger told me that when converting Trillium to super-user, he was still able to log in because the managedProjects list is not empty! ![image (9)](https://github.com/user-attachments/assets/d5c8e554-b550-4dc3-8949-079cc45a44a3)

Action Items

change user.middleware.js to
const { User } = require('../models');
function checkDuplicateEmail(req, res, next) {
  User.findOne({ email: req.body.email }).then((user) => {
    if (user) {
      return res.sendStatus(400);
    }
    next();
  });
}
function isAdminByEmail(req, res, next) {
  User.findOne({ email: req.body.email }).then((user) => {
    if (!user) {
      return res.sendStatus(400);
    } else {
      const role = user.accessLevel;
      if (role === 'admin' || role ==='superadmin' ||  user.managedProjects.length > 0) {
        next();
      } else {
        next(res.sendStatus(401));
      }
    }
  });
}
const verifyUser = {
  checkDuplicateEmail,
  isAdminByEmail,
};
module.exports = verifyUser;
"Success" Screenshot

Screenshot 2024-10-22 at 1 34 16 PM

Error message screenshot

![Screenshot 2024-10-17 at 3 24 47 PM (1)](https://github.com/user-attachments/assets/a3588b7f-f345-4ede-a5e1-54c7ed949590)

Resources/Instructions

ntrehan commented 1 month ago

@JackHaeg The good news is that I was indeed able to sign in with the super user with the changes So I was on the right track

But we have another big problem image The entire application runs as if the super user is a "USER"

This is happening because as I checked, a lot of parts of VRMS are only visible to users that have accessLevel = "ADMIN", for example user edit screen, event creation etc.

We have two options, I would like to know your opinions @trillium @jbubar @bkmorgan3

  1. Scan the entire code base and wherever anything is allowed for Admin, should also be allowed for superadmin
  2. Keep accessLevel of the superuser to be "admin", and add a totally separate boolean "isSuperAdmin" in the mongoDB users collection. That way the super user is both admin and super admin. This approach is much simpler and will need less refactoring
JackHaeg commented 3 weeks ago

@trillium Please provide next steps for Nikhil as discussed on call (while considering delivery timeline of 2-3 weeks)

trillium commented 2 weeks ago

I'd prefer us to create an isAdmin() function that takes in the user's access level and returns whathe app currently expects eg:

function isAdmin(user) { 
    user.accessLevel('admin' || 'superadmin') {
        return true
    }
    return false
}

and then swap that function in where the comparisons are added for " === 'admin' " is requested in the app

JackHaeg commented 3 days ago

@ntrehan Just checking in on this issue:

trillium commented 3 days ago

FYI - tested logging in on Dev with an account that includes symbols (+ and -), and was unable to log in. However, if your build is allowing emails with symbols to log in, you can ignore the following screenshots.

image image
trillium commented 1 day ago

Hey all, I'd like us to use a different boolean check for admin status, let's do:

image

Checking if the access level string contains the word "admin" will return true for both admin and superadmin

trillium commented 1 day ago
// change user.middleware.js to

const { User } = require('../models');
function checkDuplicateEmail(req, res, next) {
  User.findOne({ email: req.body.email }).then((user) => {
    if (user) {
      return res.sendStatus(400);
    }
    next();
  });
}
function isAdminByEmail(req, res, next) {
  User.findOne({ email: req.body.email }).then((user) => {
    if (!user) {
      return res.sendStatus(400);
    } else {
      const role = user.accessLevel;
      if (role.includes('admin') ||  user.managedProjects.length > 0) { // changes added here with .includes()
        next();
      } else {
        next(res.sendStatus(401));
      }
    }
  });
}
const verifyUser = {
  checkDuplicateEmail,
  isAdminByEmail,
};
module.exports = verifyUser;