Skip to content

Commit 9e2f26b

Browse files
shreyas-goenkashreyas-goenka
committed
bundle: warn when a workspace path is in /Workspace/Shared without users CAN_MANAGE
Renames ValidateSharedRootPermissions to ValidateWorkspaceSharedPermissions and extends it to also cover workspace.state_path. It warns when root_path or state_path is in /Workspace/Shared — granting read/write to all workspace users — but the top-level permissions section does not declare that access via group_name: users CAN_MANAGE. The state_path warning is suppressed only when state_path is nested under root_path, since the root warning already covers it. When state_path is a separate shared folder, both warnings fire. Co-authored-by: Shreyas Goenka <shreyas.goenka@databricks.com>
1 parent e92f999 commit 9e2f26b

4 files changed

Lines changed: 200 additions & 49 deletions

File tree

‎bundle/permissions/validate.go‎

Lines changed: 72 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,49 +3,101 @@ package permissions
33
import (
44
"context"
55
"fmt"
6+
"strings"
67

78
"github.com/databricks/cli/bundle"
89
"github.com/databricks/cli/bundle/libraries"
910
"github.com/databricks/cli/libs/diag"
1011
)
1112

12-
type validateSharedRootPermissions struct{}
13+
type validateWorkspaceSharedPermissions struct{}
1314

14-
func ValidateSharedRootPermissions() bundle.Mutator {
15-
return &validateSharedRootPermissions{}
15+
// ValidateWorkspaceSharedPermissions warns when a workspace path is configured
16+
// under /Workspace/Shared — which grants read/write access to all workspace users —
17+
// without the top-level permissions section declaring that broad access via
18+
// group_name: users with CAN_MANAGE.
19+
func ValidateWorkspaceSharedPermissions() bundle.Mutator {
20+
return &validateWorkspaceSharedPermissions{}
1621
}
1722

18-
func (*validateSharedRootPermissions) Name() string {
19-
return "ValidateSharedRootPermissions"
23+
func (*validateWorkspaceSharedPermissions) Name() string {
24+
return "ValidateWorkspaceSharedPermissions"
2025
}
2126

22-
func (*validateSharedRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
23-
if libraries.IsWorkspaceSharedPath(b.Config.Workspace.RootPath) {
24-
return isUsersGroupPermissionSet(b)
25-
}
26-
27-
return nil
28-
}
29-
30-
// isUsersGroupPermissionSet checks that top-level permissions set for bundle contain group_name: users with CAN_MANAGE permission.
31-
func isUsersGroupPermissionSet(b *bundle.Bundle) diag.Diagnostics {
27+
func (*validateWorkspaceSharedPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
3228
var diags diag.Diagnostics
3329

34-
allUsers := false
30+
rootPath := b.Config.Workspace.RootPath
31+
statePath := b.Config.Workspace.StatePath
32+
rootIsShared := libraries.IsWorkspaceSharedPath(rootPath)
33+
34+
// Whether the top-level permissions grant group_name: users CAN_MANAGE, i.e.
35+
// the broad /Workspace/Shared access is intentional and declared.
36+
usersCanManage := false
3537
for _, p := range b.Config.Permissions {
3638
if p.GroupName == "users" && p.Level == CAN_MANAGE {
37-
allUsers = true
39+
usersCanManage = true
3840
break
3941
}
4042
}
4143

42-
if !allUsers {
44+
// root_path is in /Workspace/Shared without users CAN_MANAGE.
45+
if rootIsShared && !usersCanManage {
46+
diags = diags.Append(diag.Diagnostic{
47+
Severity: diag.Warning,
48+
Summary: fmt.Sprintf("the bundle root path %s is writable by all workspace users", rootPath),
49+
Detail: "The bundle root path is in /Workspace/Shared, giving read/write access to all workspace users that is not reflected in the permissions section. If this is intentional, add CAN_MANAGE for 'group_name: users' to your bundle permissions. Otherwise, move the bundle to a restricted path such as /Workspace/Users/<username>.",
50+
})
51+
}
52+
53+
// state_path is in /Workspace/Shared without users CAN_MANAGE. Skip only when
54+
// state_path is nested under root_path, since the root warning above already
55+
// covers it. When state_path is a separate folder, warn about it on its own.
56+
if libraries.IsWorkspaceSharedPath(statePath) && !statePathUnderRootPath(rootPath, statePath) && !usersCanManage {
4357
diags = diags.Append(diag.Diagnostic{
4458
Severity: diag.Warning,
45-
Summary: fmt.Sprintf("the bundle root path %s is writable by all workspace users", b.Config.Workspace.RootPath),
46-
Detail: "The bundle is configured to use /Workspace/Shared, which will give read/write access to all users. If this is intentional, add CAN_MANAGE for 'group_name: users' permission to your bundle configuration. If the deployment should be restricted, move it to a restricted folder such as /Workspace/Users/<username or principal name>.",
59+
Summary: fmt.Sprintf("the bundle state path %s is writable by all workspace users", statePath),
60+
Detail: "The bundle state path is in /Workspace/Shared, giving read/write access to all workspace users that is not reflected in the permissions section. If this is intentional, add CAN_MANAGE for 'group_name: users' to your bundle permissions. Otherwise, move the state path to a restricted location such as /Workspace/Users/<username>.",
4761
})
4862
}
4963

5064
return diags
5165
}
66+
67+
// statePathUnderRootPath returns true when statePath is nested under rootPath, in
68+
// which case permissions applied to root_path also cover the state directory.
69+
//
70+
// By default state_path lives under root_path (it defaults to "${root_path}/state"),
71+
// so we treat it as nested unless both paths are set and root_path is genuinely not a
72+
// prefix of state_path. That keeps us from emitting a separate state warning for the
73+
// common case.
74+
//
75+
// We normalize the /Workspace prefix on both paths before comparing. PrependWorkspacePrefix
76+
// already does this before the mutator runs, but normalizing here too means a missing prefix
77+
// on one path can never make a nested state_path look like a separate folder.
78+
func statePathUnderRootPath(rootPath, statePath string) bool {
79+
if rootPath == "" || statePath == "" {
80+
return true
81+
}
82+
83+
rootPath = withWorkspacePrefix(rootPath)
84+
statePath = withWorkspacePrefix(statePath)
85+
if statePath == rootPath {
86+
return true
87+
}
88+
89+
if !strings.HasSuffix(rootPath, "/") {
90+
rootPath += "/"
91+
}
92+
return strings.HasPrefix(statePath, rootPath)
93+
}
94+
95+
// withWorkspacePrefix prepends "/Workspace" to an absolute workspace path that lacks it,
96+
// mirroring mutator.PrependWorkspacePrefix so path comparisons aren't thrown off by an
97+
// inconsistent prefix. Volumes paths and non-absolute paths are returned unchanged.
98+
func withWorkspacePrefix(path string) string {
99+
if strings.HasPrefix(path, "/") && !strings.HasPrefix(path, "/Workspace/") && !libraries.IsVolumesPath(path) {
100+
return "/Workspace" + path
101+
}
102+
return path
103+
}

‎bundle/permissions/validate_test.go‎

Lines changed: 123 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,18 @@ import (
88
"github.com/databricks/cli/bundle/config/resources"
99
"github.com/databricks/cli/libs/diag"
1010
"github.com/databricks/databricks-sdk-go/experimental/mocks"
11-
"github.com/databricks/databricks-sdk-go/service/jobs"
11+
"github.com/stretchr/testify/assert"
1212
"github.com/stretchr/testify/require"
1313
)
1414

15-
func TestValidateSharedRootPermissionsForShared(t *testing.T) {
15+
func applyValidate(t *testing.T, b *bundle.Bundle) diag.Diagnostics {
16+
t.Helper()
17+
m := mocks.NewMockWorkspaceClient(t)
18+
b.SetWorkpaceClient(m.WorkspaceClient)
19+
return bundle.Apply(t.Context(), b, ValidateWorkspaceSharedPermissions())
20+
}
21+
22+
func TestValidateWorkspaceSharedPermissions_RootShared_NoWarningWithUsersManage(t *testing.T) {
1623
b := &bundle.Bundle{
1724
Config: config.Root{
1825
Workspace: config.Workspace{
@@ -21,23 +28,13 @@ func TestValidateSharedRootPermissionsForShared(t *testing.T) {
2128
Permissions: []resources.Permission{
2229
{Level: CAN_MANAGE, GroupName: "users"},
2330
},
24-
Resources: config.Resources{
25-
Jobs: map[string]*resources.Job{
26-
"job_1": {JobSettings: jobs.JobSettings{Name: "job_1"}},
27-
"job_2": {JobSettings: jobs.JobSettings{Name: "job_2"}},
28-
},
29-
},
3031
},
3132
}
32-
33-
m := mocks.NewMockWorkspaceClient(t)
34-
b.SetWorkpaceClient(m.WorkspaceClient)
35-
36-
diags := bundle.Apply(t.Context(), b, ValidateSharedRootPermissions())
33+
diags := applyValidate(t, b)
3734
require.Empty(t, diags)
3835
}
3936

40-
func TestValidateSharedRootPermissionsForSharedError(t *testing.T) {
37+
func TestValidateWorkspaceSharedPermissions_RootShared_WarnWithoutUsersManage(t *testing.T) {
4138
b := &bundle.Bundle{
4239
Config: config.Root{
4340
Workspace: config.Workspace{
@@ -46,20 +43,122 @@ func TestValidateSharedRootPermissionsForSharedError(t *testing.T) {
4643
Permissions: []resources.Permission{
4744
{Level: CAN_MANAGE, UserName: "foo@bar.test"},
4845
},
49-
Resources: config.Resources{
50-
Jobs: map[string]*resources.Job{
51-
"job_1": {JobSettings: jobs.JobSettings{Name: "job_1"}},
52-
"job_2": {JobSettings: jobs.JobSettings{Name: "job_2"}},
53-
},
46+
},
47+
}
48+
diags := applyValidate(t, b)
49+
require.Len(t, diags, 1)
50+
assert.Equal(t, diag.Warning, diags[0].Severity)
51+
assert.Contains(t, diags[0].Summary, "root path")
52+
assert.Contains(t, diags[0].Summary, "/Workspace/Shared/foo/bar")
53+
}
54+
55+
func TestValidateWorkspaceSharedPermissions_StateShared_WarnWithoutUsersManage(t *testing.T) {
56+
b := &bundle.Bundle{
57+
Config: config.Root{
58+
Workspace: config.Workspace{
59+
RootPath: "/Workspace/Users/user@example.test",
60+
StatePath: "/Workspace/Shared/state",
61+
},
62+
Permissions: []resources.Permission{
63+
{Level: CAN_MANAGE, UserName: "user@example.test"},
5464
},
5565
},
5666
}
67+
diags := applyValidate(t, b)
68+
require.Len(t, diags, 1)
69+
assert.Equal(t, diag.Warning, diags[0].Severity)
70+
assert.Contains(t, diags[0].Summary, "state path")
71+
assert.Contains(t, diags[0].Summary, "/Workspace/Shared/state")
72+
}
5773

58-
m := mocks.NewMockWorkspaceClient(t)
59-
b.SetWorkpaceClient(m.WorkspaceClient)
74+
func TestValidateWorkspaceSharedPermissions_StateShared_NoWarningWithUsersManage(t *testing.T) {
75+
b := &bundle.Bundle{
76+
Config: config.Root{
77+
Workspace: config.Workspace{
78+
RootPath: "/Workspace/Users/user@example.test",
79+
StatePath: "/Workspace/Shared/state",
80+
},
81+
Permissions: []resources.Permission{
82+
{Level: CAN_MANAGE, GroupName: "users"},
83+
},
84+
},
85+
}
86+
diags := applyValidate(t, b)
87+
require.Empty(t, diags)
88+
}
6089

61-
diags := bundle.Apply(t.Context(), b, ValidateSharedRootPermissions())
90+
func TestValidateWorkspaceSharedPermissions_StateUnderSharedRoot_OnlyRootWarning(t *testing.T) {
91+
// state_path is nested under root_path — the root warning already covers it,
92+
// so the state warning must not fire.
93+
b := &bundle.Bundle{
94+
Config: config.Root{
95+
Workspace: config.Workspace{
96+
RootPath: "/Workspace/Shared/bundle",
97+
StatePath: "/Workspace/Shared/bundle/state",
98+
},
99+
Permissions: []resources.Permission{
100+
{Level: CAN_MANAGE, UserName: "user@example.test"},
101+
},
102+
},
103+
}
104+
diags := applyValidate(t, b)
62105
require.Len(t, diags, 1)
63-
require.Equal(t, "the bundle root path /Workspace/Shared/foo/bar is writable by all workspace users", diags[0].Summary)
64-
require.Equal(t, diag.Warning, diags[0].Severity)
106+
assert.Contains(t, diags[0].Summary, "root path")
107+
}
108+
109+
func TestValidateWorkspaceSharedPermissions_BothSharedSeparateFolders_TwoWarnings(t *testing.T) {
110+
// root_path and state_path are both shared but state_path is not under root_path,
111+
// so both warnings fire.
112+
b := &bundle.Bundle{
113+
Config: config.Root{
114+
Workspace: config.Workspace{
115+
RootPath: "/Workspace/Shared/bundle",
116+
StatePath: "/Workspace/Shared/separate-state",
117+
},
118+
Permissions: []resources.Permission{
119+
{Level: CAN_MANAGE, UserName: "user@example.test"},
120+
},
121+
},
122+
}
123+
diags := applyValidate(t, b)
124+
require.Len(t, diags, 2)
125+
assert.Contains(t, diags[0].Summary, "root path")
126+
assert.Contains(t, diags[1].Summary, "state path")
127+
}
128+
129+
func TestValidateWorkspaceSharedPermissions_NoSharedPaths_NoWarning(t *testing.T) {
130+
b := &bundle.Bundle{
131+
Config: config.Root{
132+
Workspace: config.Workspace{
133+
RootPath: "/Workspace/Users/user@example.test/bundle",
134+
StatePath: "/Workspace/Users/user@example.test/other-state",
135+
},
136+
},
137+
}
138+
diags := applyValidate(t, b)
139+
require.Empty(t, diags)
140+
}
141+
142+
func TestStatePathUnderRootPath(t *testing.T) {
143+
cases := []struct {
144+
name string
145+
rootPath string
146+
statePath string
147+
want bool
148+
}{
149+
{name: "default nested state", rootPath: "/Workspace/Users/me/bundle", statePath: "/Workspace/Users/me/bundle/state", want: true},
150+
{name: "equal paths", rootPath: "/Workspace/Users/me/bundle", statePath: "/Workspace/Users/me/bundle", want: true},
151+
{name: "separate folder", rootPath: "/Workspace/Users/me/bundle", statePath: "/Workspace/Shared/state", want: false},
152+
{name: "sibling prefix is not nested", rootPath: "/Workspace/Users/me/bundle", statePath: "/Workspace/Users/me/bundle-2/state", want: false},
153+
{name: "empty state defaults to nested", rootPath: "/Workspace/Users/me/bundle", statePath: "", want: true},
154+
{name: "empty root defaults to nested", rootPath: "", statePath: "/Workspace/Shared/state", want: true},
155+
// Normalization: a missing /Workspace prefix on one side must not make a
156+
// nested path look like a separate folder.
157+
{name: "mismatched prefix still nested", rootPath: "/Users/me/bundle", statePath: "/Workspace/Users/me/bundle/state", want: true},
158+
}
159+
for _, tc := range cases {
160+
t.Run(tc.name, func(t *testing.T) {
161+
assert.Equal(t, tc.want, statePathUnderRootPath(tc.rootPath, tc.statePath))
162+
})
163+
}
65164
}

‎bundle/permissions/workspace_root_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) {
7272
WorkspaceObjectType: "directories",
7373
}).Return(nil, nil)
7474

75-
diags := bundle.ApplySeq(t.Context(), b, ValidateSharedRootPermissions(), ApplyWorkspaceRootPermissions())
75+
diags := bundle.ApplySeq(t.Context(), b, ValidateWorkspaceSharedPermissions(), ApplyWorkspaceRootPermissions())
7676
require.Empty(t, diags)
7777
}
7878

‎bundle/phases/initialize.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -191,10 +191,10 @@ func Initialize(ctx context.Context, b *bundle.Bundle) {
191191
apps.Validate(),
192192

193193
resourcemutator.ValidateTargetMode(),
194-
// Reads (typed): b.Config.Workspace.RootPath (checks if path is in shared workspace)
195-
// Reads (typed): b.Config.Permissions (checks if users group has CAN_MANAGE permission)
196-
// Validates that when using a shared workspace path, appropriate permissions are configured
197-
permissions.ValidateSharedRootPermissions(),
194+
// Reads (typed): b.Config.Workspace.{RootPath,StatePath} and b.Config.Permissions.
195+
// Warns when a workspace path is in /Workspace/Shared without the permissions
196+
// section granting all workspace users CAN_MANAGE.
197+
permissions.ValidateWorkspaceSharedPermissions(),
198198

199199
// Annotate resources with "deployment" metadata.
200200
//

0 commit comments

Comments
 (0)