Handle a special use-case of user home directory permissions #3586

favoretti · 2024-05-16T23:25:21Z

Changes

In a special case when one would want to add rights for extra users to either view or edit things in one's home directory - the user's own CAN_MANAGE permission is implicit.

One can specify it explicitly in the resource, however, during deletion that CAN_MANAGE permission can not be removed, resulting in a failed run that can't be fixed without deleting resource from the state (which leaves other permissions hanging).

Hence here's an attempt to ignore or implicitly add it.

This works when permission is supplied with directory_path that matches the user_name, however, this also doesn't work when we specify user_id...

Therefore as WIP for now..

Tests

make test run locally
relevant change in docs/ folder
covered with integration tests in internal/acceptance
relevant acceptance tests are passing
using Go SDK

codecov-commenter · 2024-05-17T06:49:47Z

Codecov Report

Attention: Patch coverage is 31.25000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 81.56%. Comparing base (2a9379a) to head (531fd13).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3586      +/-   ##
==========================================
- Coverage   81.60%   81.56%   -0.05%     
==========================================
  Files         196      196              
  Lines       19744    19758      +14     
==========================================
+ Hits        16112    16115       +3     
- Misses       2672     2680       +8     
- Partials      960      963       +3

Files	Coverage Δ
permissions/resource_permissions.go	`87.41% <31.25%> (-3.40%)`	⬇️

alexott

We also need to test how it will behave if we'll try to change permissions for user/SP from manage to lower, like, CAN_READ...

alexott · 2024-05-17T06:47:55Z

permissions/resource_permissions.go

@@ -215,6 +215,16 @@ func (a PermissionsAPI) Delete(objectID string) error {
 				accl.AccessControlList = append(accl.AccessControlList, change)
 			}
 		}
+
+		// handle special case when we add extra permission to a user home dir
+		if v, ok := d.GetOk("directory_path"); ok {


We need to think how we will handle this for the case when we specify object ID instead of directory path...

Yeah, that's a tricky one my brain refused to wrap itself around at 3am.. Directory ID won't match user_name of course, but fetching a user record based on a user name seemed like an overkill. Open for any hints here.

Now that I think about it.. Even if I fetch the user record, user id won't match directory id, so there's no real way for me to correlate.. Maybe fetch directory object based on id and get its path? If that's even an option...

yes, it's possible to get the directory path by object iD

favoretti · 2024-05-17T11:13:42Z

We also need to test how it will behave if we'll try to change permissions for user/SP from manage to lower, like, CAN_READ...

This particular use-case is only intended for a single corner-case permission: user X to /Users/X, so just this particular user's home dir. That case of CAN_MANAGE is implicit, you can't lower it I believe.

alexott · 2024-06-14T15:21:14Z

I'm clarifying with the team who is responsible for workspace-level permissions

alexott · 2024-06-14T18:39:15Z

permissions/resource_permissions.go

@@ -215,6 +215,16 @@ func (a PermissionsAPI) Delete(objectID string) error {
 				accl.AccessControlList = append(accl.AccessControlList, change)
 			}
 		}
+
+		// handle special case when we add extra permission to a user home dir
+		if v, ok := d.GetOk("directory_path"); ok {


yes, it's possible to get the directory path by object iD

alexott · 2024-06-14T18:41:29Z

permissions/resource_permissions_test.go

I think that we'll need to expand tests to cover this stuff.

alexott · 2024-06-14T18:45:44Z

permissions/resource_permissions.go

@@ -215,6 +215,16 @@ func (a PermissionsAPI) Delete(objectID string) error {
 				accl.AccessControlList = append(accl.AccessControlList, change)
 			}
 		}
+
+		// handle special case when we add extra permission to a user home dir
+		if v, ok := d.GetOk("directory_path"); ok {


We may need to disallow changes only for the current user - then we'll need to add a check like if me == accessControl.UserName || me == accessControl.ServicePrincipalName { like below. We also need to handle the case of home directory of the service principal, not only ordinary users.

alexott · 2024-06-14T18:46:35Z

I got confirmation from the dev team:

Currently, the DirectoryPermissionsHandler prevents users from changing the CAN_MANAGE permissions of a user on its own home folder.

favoretti · 2024-06-17T12:13:16Z

@alexott So, to summarize what I need to do to get this merged:

Add SPN home directories to the use-case
Handle object_id next to currently implemented directory_path
Write tests

Correct?

alexott · 2024-06-17T12:14:57Z

yes, it looks like. @mgyucht I think it makes sense to discuss in the next office hours

tanmay-db

The logic looks good to me overall, I had a concern on checking CAN_MANAGE with first index of the list.

tanmay-db · 2024-08-08T13:25:41Z

permissions/resource_permissions.go

+
+		// handle special case when we add extra permission to a user home dir
+		if v, ok := d.GetOk("directory_path"); ok {
+			if v.(string) == fmt.Sprintf("/Users/%s", acl.UserName) && acl.AllPermissions[0].PermissionLevel == "CAN_MANAGE" {


Why do we only compare with the first index of the list? Just to confirm, is it necessar that CAN_MANAGE will always be at 0 index?

@tanmay-db That is... a great question, considering that this PR has been hanging here for a while - I'm not sure... I'm unfortunately not that knee-deep in databricks provider code, would you mind suggesting an alternative?

@alexott and me discussed this and we think going ahead with checking all the items in the list would be more safer.

Just to check, after this extending this to SPN and adding tests would be left before we can merge this right?

@tanmay-db Something like this?

Just to check, after this extending this to SPN and adding tests would be left before we can merge this right?

Expanding to SPN homedir you mean? Yeah, I'll need to work on that too, although I have no ideas what the format of SPN's home dir is... is it /Users/<spn_id>? If so, which ID is it, Azure Object ID, Azure app ID or Databricks internal ID? :)

As for tests... I'll do my best, but would really appreciate some help, I'm incredibly new to your codebase :)

it's SPN's Application ID (UUID)

@favoretti, yes, to optimise a little, we can add break in the loop when we find the permission level to CAN_MANAGE.

alexott reviewed May 17, 2024

View reviewed changes

tanmay-db self-requested a review May 21, 2024 13:24

favoretti marked this pull request as ready for review June 14, 2024 11:44

favoretti requested review from a team as code owners June 14, 2024 11:44

alexott requested a review from mgyucht June 14, 2024 15:12

alexott reviewed Jun 14, 2024

View reviewed changes

Handle a special use-case of user home directory permissions

531fd13

favoretti force-pushed the favoretti/special_user_home_permission_case branch from c02fbc4 to 531fd13 Compare June 17, 2024 12:01

tanmay-db reviewed Aug 8, 2024

View reviewed changes

favoretti and others added 4 commits August 8, 2024 16:08

Merge branch 'main' into favoretti/special_user_home_permission_case

88f3551

Rotate over all permissions

4929ab8

Rotate over all permissions

fefc053

Add a break

ceda4c4

alexott mentioned this pull request Aug 15, 2024

[ISSUE] Issue with databricks_permissions resource: Tries to remove access with higher access instead of adding new permission #3899

Open

Merge branch 'main' into favoretti/special_user_home_permission_case

1fc6e00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle a special use-case of user home directory permissions #3586

Handle a special use-case of user home directory permissions #3586

favoretti commented May 16, 2024 •

edited

Loading

codecov-commenter commented May 17, 2024 •

edited

Loading

alexott left a comment

alexott May 17, 2024

favoretti May 17, 2024 •

edited

Loading

alexott Jun 14, 2024

favoretti commented May 17, 2024

alexott commented Jun 14, 2024

alexott Jun 14, 2024

alexott Jun 14, 2024

alexott Jun 14, 2024

alexott commented Jun 14, 2024

favoretti commented Jun 17, 2024

alexott commented Jun 17, 2024

tanmay-db left a comment

tanmay-db Aug 8, 2024

favoretti Aug 8, 2024

tanmay-db Aug 8, 2024

tanmay-db Aug 8, 2024

favoretti Aug 8, 2024

favoretti Aug 8, 2024

alexott Aug 8, 2024

tanmay-db Aug 8, 2024

Handle a special use-case of user home directory permissions #3586

Are you sure you want to change the base?

Handle a special use-case of user home directory permissions #3586

Conversation

favoretti commented May 16, 2024 • edited Loading

Changes

Tests

codecov-commenter commented May 17, 2024 • edited Loading

Codecov Report

alexott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

favoretti May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

favoretti commented May 17, 2024

alexott commented Jun 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexott commented Jun 14, 2024

favoretti commented Jun 17, 2024

alexott commented Jun 17, 2024

tanmay-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

favoretti commented May 16, 2024 •

edited

Loading

codecov-commenter commented May 17, 2024 •

edited

Loading

favoretti May 17, 2024 •

edited

Loading