added User model, user route & checkRole controller. Added middleware function. #52
Conversation
| email: String, | ||
| role: String, | ||
| }); | ||
|
|
There was a problem hiding this comment.
The model should include the user's Google ID. role should probably include an enum of acceptable values "user" and "admin". There should also be a one-to-many relationship between User and Appointment established in both models.
const userSchema = new mongoose.Schema({
googleId: { type: String, required: true },
email: { type: String, required: true },
role: { type: String, enum: ["user", "admin"], required: true },
appointments: [{ type: mongoose.Schema.Types.ObjectId, ref: 'Appointment' }]
});
There was a problem hiding this comment.
Ahh true, I think I'll change resident to just user as well. and when I was looking at your PR i thought there'd probably be overlap with appointments. Thanks for this!
client/app/admin-dashboard/page.jsx
Outdated
| ); | ||
| export default async function AdminDashboardView() { | ||
| const session = await getServerSession(authOptions) | ||
| if (session.user.role == "admin") { |
There was a problem hiding this comment.
Use strict equality operator in JS ===
Also note that this line will crash the app if session is null. Until a redirect is in place, use conditional rendering for this scenario.
| const {role} = await res.json() | ||
| const res = await fetch(serverUrl + "user", { | ||
| headers: { | ||
| "x-user-email": profile.email, |
There was a problem hiding this comment.
This is okay for now, but it makes more sense to include the user's google id in their session, and send that to the backend to check roles, fetch forms, etc., instead of their email. Users can change the email associated with their account, but their ID is constant.
There was a problem hiding this comment.
Good point! Will add in next iteration.
| @@ -2,7 +2,6 @@ import GoogleProviders from "next-auth/providers/google"; | |||
| import { serverUrl } from "./constants"; | |||
There was a problem hiding this comment.
This file should probably use the server-only package to ensure it can never be exposed in the browser
| else { | ||
| res.json({ role: "resident" }) | ||
| } | ||
|
|
There was a problem hiding this comment.
It's a good idea to wrap async methods in a try-catch to cleanly handle errors.
This method should be updated later to create a new user if they don't already exist, with a default role of "user" or "resident", and then return the entire user object.
There was a problem hiding this comment.
Ahh that makes sense! Will also make a note to do.
There was a problem hiding this comment.
Overall, impressive work adding role-based authorization to the app! However, I am a little confused by the mix of both middleware and getServerSession hooks? Can this functionality not be consolidated into middleware.js?
I'm also a little unsure why we still have a "/landingPage" view? I have been assuming that page's content would be moved to the index route "/" and replace what's currently there. The auth form could be conditionally rendered based on whether the user has a session. This would eliminate the need for the redirect currently used in middleware.js.
Yeah, I was thinking about this (my To Do no. 3 is related)! I realized after I pushed this that the wrapper seemed a little redundant or unnecessary... It was also kind of code from when I was figuring out things to be honest! I will check if I can just take that out and let the middleware do its thing in the next sprint I suppose! And agree on the landing page situation, I think it does need to replace the index page. Can definitely add to the list of things I/someone in the group can do! |
Sorry for the big PR, I will try to be cognizant of that next time. Make sure you have the Google Client/Secrets I shared in Discord in your client env.
-Server:
-Client:
-To Do: