#136 Adding Multiple Admin Roles #146
Conversation
spacing changes, updated headers, added first & last name areas [placeholders for now, need to update database]
layout, name, and interactivity update
There was a problem hiding this comment.
hi guys, code and logic itself looks great and i tested locally! just left a few comments about thoughts I had + error handling i'd like your thoughts on. let's discuss those, resolve conflicts with main branch, and fix some of the lint warnings and this code should be good to go
| const user = req.user as IUser; | ||
| if (user?.isAdmin) { | ||
| export const isSuperAdmin: RequestHandler = (req, res, next) => { | ||
| const user = req.user as IUser | undefined; |
There was a problem hiding this comment.
would there ever be an instance where a user is undefined? just trying to think of when that would be the case otherwise I think we might not need the undefined field or we'd need to do some better error handling to default an undefined user to a basic user to not run into any other general issues
There was a problem hiding this comment.
I think there's maybe a few edge cases (user is logged out and tries to go straight to admin page, session timeout, user is deleted while session is active) but ultimately the main idea of making it potentially undefined is so that we can use the .? selector on the next line, since that would otherwise cause a server-side error if it were actually undefined.
There was a problem hiding this comment.
We talked about this and we're gonna make a landing page yuh
packages/api/src/middleware/auth.ts
Outdated
| return next(); | ||
| } | ||
|
|
||
| next(new HttpError(403, "Admin access only")); |
There was a problem hiding this comment.
Just a thought: would it make sense to specify what type of admin access is needed here? Could help with error handling in the future. This goes for line 30 and 40 asw.
| ); | ||
|
|
||
| router.post( | ||
| "/battery", | ||
| isAdmin, | ||
| isStudyManager, |
There was a problem hiding this comment.
Wouldn't a super admin also be able to do all of these things? How are we also accounting for super admin functionality in these endpoints?
There was a problem hiding this comment.
Yes, the naming is kimnda confusing but we account for this in the actual middleware implementation
| throw new HttpError(404); | ||
| } | ||
|
|
||
| // TODO this needa be figured out lol |
There was a problem hiding this comment.
commenting to add visibility that we need to tackle this
There was a problem hiding this comment.
User admin can't demote super admin
Nobody can demote themselves (including super admin)
|
|
||
| const router = useRouter(); | ||
| const authStore = useAuthStore(); | ||
| const batteryEditingStore = useBatteryEditingStore(); | ||
|
|
||
| if (!authStore.currentUser?.isAdmin) { | ||
| // TODO: Should all admins be able to access this page? |
There was a problem hiding this comment.
but, you bring up a good point about maybe having a general admin page for all admins, something we should think about for an additional ticket
There was a problem hiding this comment.
Shouldn't this page be the one only study manager/super admin can access? It's the admin page regarding the existing studies and batteries. Regardless yea I think we should abstract the functionality here and make it easily customizable which roles can access what
| } | ||
| }; | ||
|
|
||
| // TODO: Should all admins be able to access this page? |
There was a problem hiding this comment.
commenting for visibility so we don't forget ab this question. rn, i'm thinking no: only super admin and user admin
|
TODO: We still gotta do super admin page after this ticket |
Closes #136
isAdminboolean field withrolefield to handle multiple admin typesisAdminfield