Skip to content

feat: Option to remove WorkspaceExec from owner role #7050

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 19 commits into from
Apr 11, 2023

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Apr 7, 2023

What this does

This provides an option to remove WorkspaceExec from the owner role, preventing access to all workspaces.

How it does this.

I removed the Wildcard option from our RBAC. So we have to explicitly list all permissions. I made helpers to make this easy. The wildcard was out of laziness, this allows more granularity.

Slight Annoyance

Our builtin static roles are global. So to update the roles to remove a permission, I had to make a function ReloadBuiltinRoles that reloads the static roles. This has to be set on each coderd, as authz is not synced across the database.

@Emyrk Emyrk marked this pull request as ready for review April 7, 2023 19:34
@@ -0,0 +1,91 @@
package main
Copy link
Member Author

Choose a reason for hiding this comment

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

I really want to make sure we keep this enum synced. We should probably make a more generic way to maintain enum lists.

@Emyrk Emyrk requested review from johnstcn, a team and coadler and removed request for a team April 10, 2023 21:32
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

100% in favour of the autogen stuff. 👍

I'm slightly more wary of the path we're starting to go down where we have a fairly well-defined set of roles / permissions etc. that we are now having to provide options to customise in various ways. Allowing complete configurability of this would lead to a combinatorial explosion of options. For this small tightly-scoped change it's fine, but we may have to think about how we expose custom RBAC stuff down the line.

@Emyrk
Copy link
Member Author

Emyrk commented Apr 11, 2023

100% in favour of the autogen stuff. +1

I'm slightly more wary of the path we're starting to go down where we have a fairly well-defined set of roles / permissions etc. that we are now having to provide options to customise in various ways. Allowing complete configurability of this would lead to a combinatorial explosion of options. For this small tightly-scoped change it's fine, but we may have to think about how we expose custom RBAC stuff down the line.

Yea, I think if customization become more common we would have to persist roles differently.

@Emyrk Emyrk merged commit 9d39371 into main Apr 11, 2023
@Emyrk Emyrk deleted the stevenmasley/owner_cannot_workspace_exec branch April 11, 2023 13:57
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants