Together icon indicating copy to clipboard operation
Together copied to clipboard

Update User to have Administrator Field

Open Caleb-Cohen opened this issue 1 year ago • 10 comments

Related Issues Project: Admin Dashboard Depends on:

Description

  • Update UserSchema to have a administrator field.
  • field should be typed as a number
  • Update User.create in server/config/passport.js to set value to 0
  • Update current users in production to have 0 (@guel-codes may have some good advice.)

Acceptance Criteria

  • All new users should have an administrator value of 0 when created
  • Ensure user creation works in all aspects, from running Together to testing environment.

Caleb-Cohen avatar Aug 16 '23 03:08 Caleb-Cohen

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

guel-codes avatar Aug 23 '23 01:08 guel-codes

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.

vguzman812 avatar Aug 24 '23 13:08 vguzman812

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

guel-codes avatar Aug 25 '23 16:08 guel-codes

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 avatar Aug 26 '23 00:08 Caleb-Cohen

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.

Caleb-Cohen avatar Aug 26 '23 00:08 Caleb-Cohen

@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.

cblanken avatar Sep 09 '23 01:09 cblanken

@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.

guel-codes avatar Sep 09 '23 22:09 guel-codes

@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?

Caleb-Cohen avatar Sep 10 '23 22:09 Caleb-Cohen

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

guel-codes avatar Sep 16 '23 02:09 guel-codes

@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.

vguzman812 avatar Sep 17 '23 04:09 vguzman812