-
Notifications
You must be signed in to change notification settings - Fork 461
Handle user permission's special case #5057
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR handles a special case for user home directory permissions by ensuring users always maintain CAN_MANAGE permissions on their own home directories. This prevents users from accidentally removing their own management access, which could result in failed runs that can only be fixed by deleting the resource from state.
- Adds automatic detection and enforcement of CAN_MANAGE permissions for users on their home directories
- Implements logic to upgrade existing permissions or add missing CAN_MANAGE permissions during both update and delete operations
- Uses heuristic-based detection to identify potential home directory scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
permissions/update/customizers.go | Adds new ACL customizers for handling user home directory permissions |
permissions/permission_definitions.go | Integrates the new customizer into directory permission workflows |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
// This approach relies on analyzing the ACL to determine if this is a user home directory | ||
// by looking for patterns that suggest home directory access | ||
return customizer(ctx, acl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ForUserHomeDirectory function doesn't implement the logic described in its documentation. It claims to extract usernames from paths like '/Users/{username}' but simply passes through to the customizer without any path analysis or filtering.
// This approach relies on analyzing the ACL to determine if this is a user home directory | |
// by looking for patterns that suggest home directory access | |
return customizer(ctx, acl) | |
// Heuristic: If the ACL contains exactly one user with CAN_MANAGE or IS_OWNER, | |
// we treat this as a user home directory and extract the username. | |
var homeUser string | |
for _, entry := range acl { | |
if (entry.PermissionLevel == iam.PermissionLevelCanManage || entry.PermissionLevel == iam.PermissionLevelIsOwner) && entry.UserName != "" { | |
if homeUser == "" { | |
homeUser = entry.UserName | |
} else if homeUser != entry.UserName { | |
// More than one user with manage/owner, not a user home directory | |
return acl, nil | |
} | |
} | |
} | |
if homeUser != "" { | |
// Optionally, you could pass the username in the context if needed | |
return customizer(ctx, acl) | |
} | |
return acl, nil |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm... interesting suggestion, not sure I concur tho..
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Changes
Reviving an old PR in a new form..
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.
Tests
make test
run locallydocs/
folderinternal/acceptance
NEXT_CHANGELOG.md
file