Permissions revamp

[henry-li-06]

Summary

This PR sets up more proper rbac and adds auth middleware to handle the rbac checking. This is obviously very big and I'm not doing this particularly meticulously so it's not well tested.

But - For the most part I like where the idea is at.

Summary of the permission change:

RBAC is role based access control. Within RBAC, there are roles, resources, and actions. Access control is based on which roles are allowed to perform what actions there are on what resources. For the case of IDOL, I think read and write actions are all that are needed.

Introducing auth roles

I added some auth roles here

type AuthLeadTypes =
  | 'dev_lead'
  | 'ops_lead'
  | 'lead'
  | 'design_lead'
  | 'business_lead'
  | 'ci_lead'
  | 'pm_lead';

export type AuthRole =
  | undefined
  | 'admin'
  | 'dev'
  | 'tpm'
  | 'pm'
  | 'designer'
  | 'business'
  | 'lead'
  | AuthLeadTypes;

These roles should be assigned to people as needed in a collection in a db. For the time being, probably only leads to need to be defined.

RBAC Config

The rbac config file has this schema:

export interface RBACConfig {
  resources: {
    [resourceName: string]: {
      read: AuthRole[];
      write: AuthRole[];
    };
  };
}

For each resource, it defines which roles have read and which roles have write permissions to the resource. Being able to read does NOT necessarily mean that role also has write permissions.

The auth middleware

All of the auth stuff that used to be in api.ts got moved to auth.ts. Each of the loginCheckedHttpVerb functions now have been changed to look like:

loginCheckedGet = (
  router: Router,
  path: string,
  handler: (req: Request, user: IdolMember) => Promise<Record<string, unknown>>,
  resource?: string,
  action?: string,
  userHasAccess?: (req: Request, user: IdolMember) => Promise<boolean>
): RequestHandler =>

There are a few parts to this:

• The router param makes it so that the loginChecked can directly be called in the router's file so that we're not defining all the API routes in the same file as before
• The path and handler params stay the same as before
• resource, action, and userHasAccess
  • resource defines the type of resource being requested
  • action defines the type of action being take on the resource, either read or write
  • userHasAccess is a function that takes the request and the logged in user and returns whether or not the user has access to the desired resource. This is configurable by endpoint so when defining the function, you should be able to define a function that given the request and user, you can determine if they should have access. For example, if it's an endpoint for accessing someone's dev portfolio submission, a user has permissions for that resource because email should be a query param which should match the logged in user's email.
    • This is important for making auth more extensible beside just the rbac
    • This defaults to true if it's not passed in
    • For some endpoints that should be accessed only with rbac perms, async () => false needs to be passed. I don't really like this but wasn't sure of a better way. It's done so that endpoints that don't need authorization don't need to specify this.
    • For the most part, something like async (req, user) => req.params.email === user.email || req.query.meta_only ===='true' should cover most cases where it's needed.

What still needs to be done?

I'm glad you asked :D

Make sure everything works!

Well first and foremost, I can't guarantee that I didn't break anything. This is probably a little sloppy so I'm sure there are places where things are broken and need to be fixed. This is to say there needs to be some manual testing for everything to make sure the whole app works. Ideally, tests should be written to test perms for all the endpoints but I acknowledge writing tests is somewhat boring so that probably is never going to happen.

I'm pretty confident some of the route paths are a bit messed up too... 🙃

Lead roles are still a bit scuffed

In terms of the lead roles, my idea was that the lead role encompassed all of the other lead roles (i.e. if a lead had permission for something, then all of lead roles would have that permission). But nobody should actually be assigned this role. All leads should be assigned their specific role lead role (e.g. dev_lead).

When checking for rbac perms here:

const hasRbacPerm = async (
  user: IdolMember,
  rbacConfig: RBACConfig,
  resource?: string,
  action?: string
): Promise<boolean> => {
  if (resource && action) {
    const roleData = await getUserRole(user);
    const userRole = roleData?.role;
    const resourceRBACConfig = rbacConfig.resources[resource];

    if (resourceRBACConfig) {
      return resourceRBACConfig[action].includes(userRole);
    }
  }
  return true;
};

this lead role situation isn't properly accounted for.

The remaining routers...

There are a few routers where rbac isn't setup:

• Candidate decider
• Sign in forms
• Team event images

I think team event images and sign in form APIs originally didn't match the original auth middlware and rbac implementation that relied on using email and meta_only. I wanna say there should be no problem adding rbac middleware to this now.

For candidate decider, there is slightly more complex authorization that is involved. This originally couldn't be handled with that simple rbac logic. But the addition of that userHasAccess function param makes it so that it should be possible to move and define the authorization in that function for the endpoints that need it instead of in the API handlers themselves.

loginCheckedHandler

This just basically doesn't do anything now except handle exception handling. It also still passes the user to the API handlers even though the auth middleware sets the user in res.locals. This can definitely be fixed up.

Tips for dealing with this monstrosity

1. This is PR is definitely big but there are a lot of detailed commits that do things piece by piece. Looking commit by commit will help to understand what's going on.
2. Definitely rebase this on main, the branch is from May 2023. There are lots of merge conflicts but hopefully shouldn't be too bad to resolve.
3. I think (🙏🏻 ) that this was somewhat done in a way that can be split into a handful of smaller PRs. It also helps that the auth middleware and loginChecked functions were implemented/updated in a way that doesn't require the handler functions to buy into rbac (i.e. you can change the loginCheckedFunctions without having to setup rbac for the handlers - you'll still have to start using routers for all the APIs tho).
4. If I'm not wrong about (3) then the RBAC and auth middleware configuration can be done 1 by 1 for all the APIs.

[dti-github-bot]

[diff-counting] Significant lines: 958.

[andrew032011]

Think we need a role for business/PMMs. Probably the same level of permissions as a designer or PM.

[andrew032011]

/deleteShoutout --> /shoutout?

[andrew032011]

I see you have another PR out to make our endpoints more RESTful. Hopefully all our endpoints follow the pattern of "GET" is for read-only, and everything else involves some kind of write operation.

[andrew032011]

I think it would be safer to reject the request if the resource is somehow not found in the config, rather than returning 200. It would alert us if we as the developer made a mistake/typo somewhere, or somehow messed up the config file. We could handle this check in isAuthorized.

[andrew032011]

Cool, I understand this as if someone is requesting data specific to a person and they are that person, then this request overrides any role-based read/read-write permissions in the config. I personally think it reads better to put this check right under the admin userRole check.