Skip to content

feat: implement acl for workspaces #19094

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 10 commits into from
Jul 30, 2025
Merged

feat: implement acl for workspaces #19094

merged 10 commits into from
Jul 30, 2025

Conversation

aslilac
Copy link
Member

@aslilac aslilac commented Jul 29, 2025

  • Refactors (what is now named) ACLMappingVar to support permissions lists stored in a field rather than directly in the mapping
  • Use that to give workspace resources actual ACL matchers
  • Adds the ACL columns to the workspaces table

@aslilac aslilac requested a review from Emyrk as a code owner July 29, 2025 22:23
@aslilac aslilac requested a review from brettkolodny July 29, 2025 22:23
@aslilac
Copy link
Member Author

aslilac commented Jul 29, 2025

I'm concerned about the security implications of exposing this value too prominently, and we have a lot queries that currently select workspaces.*. Am I over thinking it or should we pare back on some of those to be more selective on the columns they get?

Edit: I guess it doesn't really matter if the db types have this column. We'd have to expose them through the codersdk types manually still.

Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

Wait, not done reviewing! Sorry, finishing up. Misclicked

Comment on lines 62 to 65
// "left" will be a map of group names to actions in rego.
// {
// "all_users": ["read"]
// }
Copy link
Member

Choose a reason for hiding this comment

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

This is not 100% correct anymore. Maybe just note the SubField is "" for this example.

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 think this is still correct tho. by the time it gets to rego it will just be a list of actions because the subfield gets handled in sql right? the rego policy doesn't even know about the indirection

Copy link
Member

Choose a reason for hiding this comment

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

The rego just gets a slice of actions iirc. I will need to double check, we can keep this for now

@aslilac aslilac merged commit eeb0bbe into main Jul 30, 2025
33 checks passed
@aslilac aslilac deleted the lilac/workspaces-acl branch July 30, 2025 23:02
@github-actions github-actions bot locked and limited conversation to collaborators Jul 30, 2025
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.

2 participants