Skip to content

Commit e9bf1f7

Browse files
smurchingsmurching
authored
Fixed databricks_permissions for calling user to CAN_MANAGE on databricks_mlflow_model (#1507)
Ensure that calling user always has CAN_MANAGE permission on MLflow models. Fixes #1504
1 parent 0e69acb commit e9bf1f7

File tree

2 files changed

+239
-10
lines changed

2 files changed

+239
-10
lines changed

‎permissions/resource_permissions.go‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -124,18 +124,43 @@ func urlPathForObjectID(objectID string) string {
124124
return "/permissions" + objectID
125125
}
126126

127-
// Helper function to select the correct HTTP method depending on the object types.
127+
// As described in https://github.com/databricks/terraform-provider-databricks/issues/1504,
128+
// certain object types require that we explicitly grant the calling user CAN_MANAGE
129+
// permissions when POSTing permissions changes through the REST API, to avoid accidentally
130+
// revoking the calling user's ability to manage the current object.
131+
func (a PermissionsAPI) shouldExplicitlyGrantCallingUserManagePermissions(objectID string) bool {
132+
for _, prefix := range [...]string{"/registered-models/"} {
133+
if strings.HasPrefix(objectID, prefix) {
134+
return true
135+
}
136+
}
137+
return isDbsqlPermissionsWorkaroundNecessary(objectID)
138+
}
139+
140+
func (a PermissionsAPI) ensureCurrentUserCanManageObject(objectID string, objectACL AccessControlChangeList) (AccessControlChangeList, error) {
141+
if !a.shouldExplicitlyGrantCallingUserManagePermissions(objectID) {
142+
return objectACL, nil
143+
}
144+
me, err := scim.NewUsersAPI(a.context, a.client).Me()
145+
if err != nil {
146+
return objectACL, err
147+
}
148+
objectACL.AccessControlList = append(objectACL.AccessControlList, AccessControlChange{
149+
UserName: me.UserName,
150+
PermissionLevel: "CAN_MANAGE",
151+
})
152+
return objectACL, nil
153+
}
154+
155+
// Helper function for applying permissions changes. Ensures that
156+
// we select the correct HTTP method based on the object type and preserve the calling
157+
// user's ability to manage the specified object when applying permissions changes.
128158
func (a PermissionsAPI) put(objectID string, objectACL AccessControlChangeList) error {
159+
objectACL, err := a.ensureCurrentUserCanManageObject(objectID, objectACL)
160+
if err != nil {
161+
return err
162+
}
129163
if isDbsqlPermissionsWorkaroundNecessary(objectID) {
130-
// SQLA entities always have `CAN_MANAGE` permission for the calling user.
131-
me, err := scim.NewUsersAPI(a.context, a.client).Me()
132-
if err != nil {
133-
return err
134-
}
135-
objectACL.AccessControlList = append(objectACL.AccessControlList, AccessControlChange{
136-
UserName: me.UserName,
137-
PermissionLevel: "CAN_MANAGE",
138-
})
139164
// SQLA entities use POST for permission updates.
140165
return a.client.Post(a.context, urlPathForObjectID(objectID), objectACL, nil)
141166
}

‎permissions/resource_permissions_test.go‎

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,210 @@ func TestResourcePermissionsRead_RemovedCluster(t *testing.T) {
121121
}.ApplyNoError(t)
122122
}
123123

124+
func TestResourcePermissionsRead_Mlflow_Model(t *testing.T) {
125+
d, err := qa.ResourceFixture{
126+
// Pass list of API request mocks
127+
Fixtures: []qa.HTTPFixture{
128+
me,
129+
{
130+
Method: http.MethodGet,
131+
Resource: "/api/2.0/permissions/registered-models/fakeuuid123",
132+
Response: ObjectACL{
133+
ObjectID: "/registered-models/fakeuuid123",
134+
ObjectType: "registered-model",
135+
AccessControlList: []AccessControl{
136+
{
137+
UserName: TestingUser,
138+
PermissionLevel: "CAN_READ",
139+
},
140+
{
141+
UserName: TestingAdminUser,
142+
PermissionLevel: "CAN_MANAGE",
143+
},
144+
},
145+
},
146+
},
147+
},
148+
Resource: ResourcePermissions(),
149+
Read: true,
150+
New: true,
151+
ID: "/registered-models/fakeuuid123",
152+
}.Apply(t)
153+
assert.NoError(t, err, err)
154+
assert.Equal(t, "/registered-models/fakeuuid123", d.Id())
155+
ac := d.Get("access_control").(*schema.Set)
156+
require.Equal(t, 1, len(ac.List()))
157+
firstElem := ac.List()[0].(map[string]any)
158+
assert.Equal(t, TestingUser, firstElem["user_name"])
159+
assert.Equal(t, "CAN_READ", firstElem["permission_level"])
160+
}
161+
162+
func TestResourcePermissionsCreate_Mlflow_Model(t *testing.T) {
163+
d, err := qa.ResourceFixture{
164+
Fixtures: []qa.HTTPFixture{
165+
me,
166+
{
167+
Method: http.MethodPut,
168+
Resource: "/api/2.0/permissions/registered-models/fakeuuid123",
169+
ExpectedRequest: AccessControlChangeList{
170+
AccessControlList: []AccessControlChange{
171+
{
172+
UserName: TestingUser,
173+
PermissionLevel: "CAN_READ",
174+
},
175+
{
176+
UserName: TestingAdminUser,
177+
PermissionLevel: "CAN_MANAGE",
178+
},
179+
},
180+
},
181+
},
182+
{
183+
Method: http.MethodGet,
184+
Resource: "/api/2.0/permissions/registered-models/fakeuuid123",
185+
Response: ObjectACL{
186+
ObjectID: "/registered-models/fakeuuid123",
187+
ObjectType: "registered-model",
188+
AccessControlList: []AccessControl{
189+
{
190+
UserName: TestingUser,
191+
PermissionLevel: "CAN_READ",
192+
},
193+
{
194+
UserName: TestingAdminUser,
195+
PermissionLevel: "CAN_MANAGE",
196+
},
197+
},
198+
},
199+
},
200+
},
201+
Resource: ResourcePermissions(),
202+
State: map[string]any{
203+
"registered_model_id": "fakeuuid123",
204+
"access_control": []any{
205+
map[string]any{
206+
"user_name": TestingUser,
207+
"permission_level": "CAN_READ",
208+
},
209+
},
210+
},
211+
Create: true,
212+
}.Apply(t)
213+
assert.NoError(t, err, err)
214+
ac := d.Get("access_control").(*schema.Set)
215+
require.Equal(t, 1, len(ac.List()))
216+
firstElem := ac.List()[0].(map[string]any)
217+
assert.Equal(t, TestingUser, firstElem["user_name"])
218+
assert.Equal(t, "CAN_READ", firstElem["permission_level"])
219+
}
220+
221+
func TestResourcePermissionsUpdate_Mlflow_Model(t *testing.T) {
222+
d, err := qa.ResourceFixture{
223+
Fixtures: []qa.HTTPFixture{
224+
me,
225+
{
226+
Method: http.MethodPut,
227+
Resource: "/api/2.0/permissions/registered-models/fakeuuid123",
228+
ExpectedRequest: AccessControlChangeList{
229+
AccessControlList: []AccessControlChange{
230+
{
231+
UserName: TestingUser,
232+
PermissionLevel: "CAN_READ",
233+
},
234+
{
235+
UserName: TestingAdminUser,
236+
PermissionLevel: "CAN_MANAGE",
237+
},
238+
},
239+
},
240+
},
241+
{
242+
Method: http.MethodGet,
243+
Resource: "/api/2.0/permissions/registered-models/fakeuuid123",
244+
Response: ObjectACL{
245+
ObjectID: "/registered-models/fakeuuid123",
246+
ObjectType: "registered-model",
247+
AccessControlList: []AccessControl{
248+
{
249+
UserName: TestingUser,
250+
PermissionLevel: "CAN_READ",
251+
},
252+
{
253+
UserName: TestingAdminUser,
254+
PermissionLevel: "CAN_MANAGE",
255+
},
256+
},
257+
},
258+
},
259+
},
260+
InstanceState: map[string]string{
261+
"registered_model_id": "fakeuuid123",
262+
},
263+
HCL: `
264+
registered_model_id = "fakeuuid123"
265+
266+
access_control {
267+
user_name = "ben"
268+
permission_level = "CAN_READ"
269+
}
270+
`,
271+
Resource: ResourcePermissions(),
272+
Update: true,
273+
// Removed: true,
274+
ID: "/registered-models/fakeuuid123",
275+
}.Apply(t)
276+
assert.NoError(t, err, err)
277+
assert.Equal(t, "/registered-models/fakeuuid123", d.Id())
278+
ac := d.Get("access_control").(*schema.Set)
279+
require.Equal(t, 1, len(ac.List()))
280+
firstElem := ac.List()[0].(map[string]any)
281+
assert.Equal(t, TestingUser, firstElem["user_name"])
282+
assert.Equal(t, "CAN_READ", firstElem["permission_level"])
283+
}
284+
285+
func TestResourcePermissionsDelete_Mlflow_Model(t *testing.T) {
286+
d, err := qa.ResourceFixture{
287+
Fixtures: []qa.HTTPFixture{
288+
me,
289+
{
290+
Method: http.MethodGet,
291+
Resource: "/api/2.0/permissions/registered-models/fakeuuid123",
292+
Response: ObjectACL{
293+
ObjectID: "/registered-models/fakeuuid123",
294+
ObjectType: "registered-model",
295+
AccessControlList: []AccessControl{
296+
{
297+
UserName: TestingUser,
298+
PermissionLevel: "CAN_READ",
299+
},
300+
{
301+
UserName: TestingAdminUser,
302+
PermissionLevel: "CAN_MANAGE",
303+
},
304+
},
305+
},
306+
},
307+
{
308+
Method: http.MethodPut,
309+
Resource: "/api/2.0/permissions/registered-models/fakeuuid123",
310+
ExpectedRequest: AccessControlChangeList{
311+
AccessControlList: []AccessControlChange{
312+
{
313+
UserName: TestingAdminUser,
314+
PermissionLevel: "CAN_MANAGE",
315+
},
316+
},
317+
},
318+
},
319+
},
320+
Resource: ResourcePermissions(),
321+
Delete: true,
322+
ID: "/registered-models/fakeuuid123",
323+
}.Apply(t)
324+
assert.NoError(t, err, err)
325+
assert.Equal(t, "/registered-models/fakeuuid123", d.Id())
326+
}
327+
124328
func TestResourcePermissionsRead_SQLA_Asset(t *testing.T) {
125329
d, err := qa.ResourceFixture{
126330
Fixtures: []qa.HTTPFixture{

0 commit comments

Comments
 (0)