Skip to content

feat: Add template-admin + user-admin role for managing templates + users #3490

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

Merged
merged 11 commits into from
Aug 12, 2022

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Aug 12, 2022

What this does

Adds a template-admin role that has full CRUD to templates and files.
Adds a user-admin role that has full CRUD on users

Thoughts? Trying to partially solve #2135 by removing the need to promote everyone to admin.

Screenshot from 2022-08-12 16-02-28

Bonus

UI only shows assignable roles. So if you cannot assign a role, it won't appear in the dropdown. (Except for self assigning roles)

TODO:

Next PR will migrate "Admin" -> "Template Admin, User Admin"
And migrate the first user from "Admin" -> "Owner"

Might want to make this an org role in the future.
Also also shows assignable roles in the ui.
@Emyrk Emyrk mentioned this pull request Aug 12, 2022
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me (although, do consider that my RBAC lingo is weak).

I wonder how much this will help towards #2135, though. I imagine #2135 is looking for a user that can also delete workspaces, but not inspect them.

@Emyrk Emyrk changed the title feat: Add template-manager role for managing templates site-wide feat: Add template-admin + user-admin role for managing templates + users Aug 12, 2022
@Emyrk
Copy link
Member Author

Emyrk commented Aug 12, 2022

@mafredri I made a user-admin role as well. Combined, I think they cover what an "admin" wants to be, and blocks workspace execution on other user's workspaces

Comment on lines +177 to +183
Name: "MyWorkspaceInOrgExecution",
// When creating the WithID won't be set, but it does not change the result.
Actions: []rbac.Action{rbac.ActionCreate, rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete},
Resource: rbac.ResourceWorkspaceExecution.InOrg(orgID).WithOwner(currentUser.String()),
AuthorizeMap: map[bool][]authSubject{
true: {admin, orgAdmin, orgMemberMe},
false: {memberMe, otherOrgAdmin, otherOrgMember, templateAdmin, userAdmin},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workspace execution can only be done by admin, orgadmin and the owner of the workspace.

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new role was a nice addition!

@@ -70,7 +70,7 @@ func (api *API) workspaceAgentDial(rw http.ResponseWriter, r *http.Request) {

workspaceAgent := httpmw.WorkspaceAgentParam(r)
workspace := httpmw.WorkspaceParam(r)
if !api.Authorize(r, rbac.ActionUpdate, workspace) {
if !api.Authorize(r, rbac.ActionCreate, workspace.ExecutionRBAC()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't quite catch why this changed from Update to Create. My intuition says it doesn't really matter. So it's probably fine but just checking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't. It's arbitrary.

But what I did change was I added ResourceWorkspaceExecution as a unique resource. So giving CRUD to a workspace does not grant execution via a tunnel. These are now distinct permissions.

@Emyrk Emyrk requested a review from a team as a code owner August 12, 2022 20:57
Emyrk and others added 2 commits August 12, 2022 16:02
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
@ammario ammario linked an issue Aug 12, 2022 that may be closed by this pull request
@Emyrk Emyrk requested review from ammario and Kira-Pilot August 12, 2022 21:14
Copy link
Member

@Kira-Pilot Kira-Pilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nicely done

@Emyrk Emyrk merged commit 40e68cb into main Aug 12, 2022
@Emyrk Emyrk deleted the stevenmasley/2135/roles branch August 12, 2022 22:27
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.

Create lesser admin role
3 participants