Together-100Devs / Together

Together is a group calendar application using the MERN stack intended to bring discord communities closer!
https://together.cyclic.app/
MIT License
167 stars 112 forks source link

Update User to have Administrator Field #441

Open Caleb-Cohen opened 10 months ago

Caleb-Cohen commented 10 months ago

Related Issues Project: Admin Dashboard Depends on:

Description

Acceptance Criteria

guel-codes commented 10 months ago

We might want to make the field a boolean type. Is there a reason you want it to be typed as a Number? Either way could work, I was just wondering

vguzman812 commented 10 months ago

I have a solution made for this now. I would love to be assigned this. I also have code ready that prevents access to the adminDashboard route unless user has admin property of 1. However, try as I might, I could not configure the tests to pass. If anyone has more experience with cypress, I would love some help with making the tests pass.

guel-codes commented 10 months ago

Also just brainstorming the data model here a little bit, it might be beneficial to migrate to a "roles" field on the user model that stores an array of roles. That way someone could be an "admin" with all permissions but someone else could be a "moderator" with read-only etc.

And when they login to the Admin dashboard there can be a check of their roles array for what permissions they have

Caleb-Cohen commented 10 months ago

We might want to make the field a boolean type. Is there a reason you want it to be typed as a Number? Either way could work, I was just wondering

I proposed number because we can set certain access roles. For example:

Level 0 = default user / no access Level 1 = Moderator can access and delete Level 2 = Admin can access and approve the delete

The rough idea would allow some future flexibility as well if there's other features like editing current events and such. After reading your additional comment would you prefer an array of roles instead of just a number system?

Caleb-Cohen commented 10 months ago

I have a solution made for this now. I would love to be assigned this. I also have code ready that prevents access to the adminDashboard route unless user has admin property of 1. However, try as I might, I could not configure the tests to pass. If anyone has more experience with cypress, I would love some help with making the tests pass.

Hey there! Welcome in! I can take a look at the test in a few days. I'm currently traveling a bit and it's hard to get time to sit down to review.

cblanken commented 10 months ago

@Caleb-Cohen If we're using the admin field on the User to indicate what level of access a user has, I think it might make sense to reword to security-level, admin-level, or even role. Something to indicate the integer value isn't just treated as a boolean since you might assume that if the field just reads admin or administrator.

It might also be worth considering storing these security levels as strings. I don't imagine the app will ever need such fine control of user privileges to that couldn't be handled by a short list ("user", "moderator", "admin", "superadmin"). This way the values are also more expressive and less easily confused if more roles are added later.

guel-codes commented 10 months ago

@cblanken I agree. It is ideal to have a role field that holds an array of strings and then you can index on that field when querying for role based access. I think the first use case for the admin app could be to assign roles to others.

Caleb-Cohen commented 10 months ago

@Caleb-Cohen If we're using the admin field on the User to indicate what level of access a user has, I think it might make sense to reword to security-level, admin-level, or even role. Something to indicate the integer value isn't just treated as a boolean since you might assume that if the field just reads admin or administrator.

It might also be worth considering storing these security levels as strings. I don't imagine the app will ever need such fine control of user privileges to that couldn't be handled by a short list ("user", "moderator", "admin", "superadmin"). This way the values are also more expressive and less easily confused if more roles are added later.

I agree with this. Thanks for the feedback @cblanken and @guel-codes. @vguzman812 can you update PR to reflect changes?

guel-codes commented 9 months ago

@vguzman812 let me know if you want to pair up on this and we can

vguzman812 commented 9 months ago

@vguzman812 let me know if you want to pair up on this and we can

Already submitted the new PR a few days ago. The testing for it needs to be done if you're interested in that. I don't know enough cypress to add administrative authorization testing.