Consolidate workspace folder ACL logic and add a writability check#5948
Open
pietern wants to merge 2 commits into
Open
Consolidate workspace folder ACL logic and add a writability check#5948pietern wants to merge 2 commits into
pietern wants to merge 2 commits into
Conversation
The logic for reasoning about a workspace folder's access control was
split across two places: folder_permissions.go owned the traversal that
walks up to the closest existing ancestor, while bundle/permissions owned
the ACL model (WorkspacePathPermissions, ObjectAclToResourcePermissions).
Bring them together in bundle/permissions as FolderACL, so there is one
place that resolves and reasons about folder permissions.
Add a writability check with two entry points:
- FolderACL.CanWrite inspects a readable ACL: does the user hold CAN_EDIT
or higher, directly or via a known group. It is conservative, assuming
write access for workspace admins (who bypass the folder ACL) and when
group membership is not known here.
- CheckWritable resolves the ACL and returns a three-valued result. This
is necessary because reading a folder ACL itself requires manage access:
verified against a live workspace, GET permissions returns 403 for
folders the caller cannot manage, so the common "cannot write" case
surfaces as a permission error on the read, not as a readable ACL that
omits the user. CheckWritable maps that 403 to WritabilityNotWritable, a
readable ACL to writable/not-writable, and anything else to unknown.
Tests use ACLs that mirror the shape of real workspace folder permissions
(a home dir with direct CAN_MANAGE; a shared dir with CAN_MANAGE via a
group the user belongs to) to confirm CanWrite agrees with those grants.
This is the reusable core for a future read-only "remote file path may not
be writable" validation. No validator is wired up yet and existing behavior
is unchanged: folder_permissions now calls ResolveFolderACL but produces the
same diagnostics.
Co-authored-by: Isaac
Cite the workspace ACL model (managing permissions is exclusive to CAN_MANAGE) so the next reader can trace why CheckWritable treats a permission-denied error on the permissions GET as a definite "not writable" without re-probing the API. Co-authored-by: Isaac
Collaborator
Integration test reportCommit: f1c3824
10 interesting tests: 4 RECOVERED, 4 SKIP, 2 flaky
Top 10 slowest tests (at least 2 minutes):
|
andrewnester
approved these changes
Jul 16, 2026
| // "does not have Manage permissions" otherwise), so a 403 means the user cannot | ||
| // manage, and therefore cannot write to, the folder. | ||
| // See https://docs.databricks.com/aws/en/security/auth/access-control/ | ||
| func CheckWritable(ctx context.Context, w workspace.WorkspaceInterface, folderPath string, user *iam.User) Writability { |
Contributor
There was a problem hiding this comment.
It's not called right now, is it expected for it to be called somewhere else later?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Reading
bundle validate's remote-path handling (#5528) surfaced that we have no read-only way to tell a user, early, that they likely can't write to theirworkspace.file_path. Today that only fails at deploy. Reasoning about folder permissions was also split:folder_permissions.goowned the ancestor-walk traversal whilebundle/permissionsowned the ACL model. This lands one place for it and the reusable core for a follow-up writability warning.What
bundle/permissionsasFolderACL(walks up to the closest existing ancestor for a not-yet-created folder).CanWrite(user): CAN_EDIT+ directly or via a known group; conservative for admins and unknown group membership.CheckWritable(...): three-valued (writable / not-writable / unknown), since reading a folder ACL itself requires manage access, so "cannot write" usually surfaces as a 403 on the read.Groundwork only: no validator is wired up yet (the consuming validator is a follow-up, alongside #5528), and existing behavior is unchanged.
ValidateFolderPermissionsnow callsResolveFolderACLbut produces the same diagnostics. Confined tobundle/permissions.This pull request and its description were written by Isaac.