Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update User to have Administrator Field #441

Open
Caleb-Cohen opened this issue Aug 16, 2023 · 10 comments
Open

Update User to have Administrator Field #441

Caleb-Cohen opened this issue Aug 16, 2023 · 10 comments
Labels

Comments

@Caleb-Cohen
Copy link
Member

Caleb-Cohen commented Aug 16, 2023

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.
@guel-codes
Copy link
Contributor

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
Copy link
Contributor

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 vguzman812 mentioned this issue Aug 24, 2023
13 tasks
@guel-codes
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

cblanken commented Sep 9, 2023

@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
Copy link
Contributor

@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
Copy link
Member Author

Caleb-Cohen commented Sep 10, 2023

@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
Copy link
Contributor

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

@vguzman812
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants