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

Allow setting role when inviting new user #1025

Merged
merged 3 commits into from
Dec 28, 2023
Merged

Conversation

jjcarstens
Copy link
Collaborator

@jjcarstens jjcarstens commented Sep 5, 2023

Adds role to the invite stored for new users so they are created with the expected role.

image

@fhunleth
Copy link
Contributor

fhunleth commented Sep 5, 2023

What's the difference between the write, delete and admin roles?

@jjcarstens
Copy link
Collaborator Author

It just depends. They are defined in each controller. I think the next bigger task is to really define that and have it more central so it's easy to overview what each can/cannot do.

In the meantime, the typical means are:

  • read - mostly show and index pages. Allows creation of users items
  • delete - Like read, but allows deleting user items
  • write - CRUD actions for more shared items (device, CA certificate, etc)
  • admin - All the things, especially altering org details (invite users, create new products, etc)

@fhunleth
Copy link
Contributor

fhunleth commented Sep 5, 2023

Since these are "roles" and we have a "role" called "admin", would different words make sense? I find read, write, and delete a little confusing - especially since "read" allows creation?

Since our immediate requirement is to support a user that has access to device status and nothing else, can the roles be "admin" and "status"? Or another word besides "status"? And then delete the "read", "delete", and "write" roles?

@jjcarstens
Copy link
Collaborator Author

I misspoke. I went through some controllers and read does not seem able to create.

Yes, I do think that makes sense, but I also think it's a much bigger change than this PR.role in the DB is an Enum type and that is just difficult enough to change that it requires thought and I'd rather it be separate.

@jjcarstens
Copy link
Collaborator Author

Thought right after submitting - the quick hack could be displaying the role as status, but not changing the Enum value in the DB. I don't love how that disconnects the view and code handling so much, but I could be convinced

@fhunleth
Copy link
Contributor

fhunleth commented Sep 6, 2023

Wouldn't it be better if there was a layer of indirection here? I.e., the role would point to a set of fine grained permissions that would determine what could be done. Perhaps the set is trivially small now and having "if" statements all over won't be terrible.

I'm ok with whatever you all think best on this. My desire would be to expose fewer roles while we still can so that we don't accidentally lock in confusing names as people start using them.

@jjcarstens
Copy link
Collaborator Author

Chatted offline. Going to only display the Admin and Read roles for now until all the permissions are adjusted (see e0594f4)

@jjcarstens jjcarstens force-pushed the invite-user-with-role branch from e0594f4 to 658b121 Compare September 6, 2023 14:37
@joshk
Copy link
Collaborator

joshk commented Dec 16, 2023

Upon reflection, I think we should use admin, manage, and view as the roles.

manage is a good middle ground for cases where you want to restrict actions like 'delete a product'

I also think these names are more descriptive.

@joshk
Copy link
Collaborator

joshk commented Dec 18, 2023

@fhunleth @jjcarstens

What are your thoughts on three user roles:

admin : can create and delete products, and can invite users and set roles
manage : do everything except the above
view : can view all information

@jjcarstens
Copy link
Collaborator Author

I like those role names and these basic starting restrictions

@fhunleth
Copy link
Contributor

Same. Those roles sound good to me.

@joshk joshk force-pushed the invite-user-with-role branch from b020f33 to 612be74 Compare December 22, 2023 00:10
@joshk
Copy link
Collaborator

joshk commented Dec 23, 2023

@fhunleth @jjcarstens this is ready for another review.

@joshk joshk force-pushed the invite-user-with-role branch from 612be74 to 1586cc7 Compare December 27, 2023 21:13
@joshk
Copy link
Collaborator

joshk commented Dec 27, 2023

from a PR review by @jjcarstens

Josh and I brainstormed this a bit ultimately deciding that our future selves and those migrating would prob prefer a much simpler setup changing the roles to simple string field in the DB.

The main benefits we get from Enum types in the DB is database validation and maybe more efficient lookups. However, we don't look up the role field individually as it comes with the whole user field so it seems worth the trade off to handle validation in the application instead to make it much simpler to maintain going forward

jjcarstens and others added 3 commits December 27, 2023 15:11
Adds `role` to the invite stored for new users so they are created with the expected role.
Roles and permissions are going through an overhaul. In the meantime,
this restricts the roles to `Admin` or `Read` permissions to reduce
roles available for use until they are redefined
and switch to varchars instead of Postgres Enums
@jjcarstens jjcarstens force-pushed the invite-user-with-role branch from 1586cc7 to d8245da Compare December 27, 2023 22:11
@jjcarstens jjcarstens merged commit ff90726 into main Dec 28, 2023
2 checks passed
@jjcarstens jjcarstens deleted the invite-user-with-role branch December 28, 2023 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants