Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NAS-132041 / 25.04 / Allow setting ACL entries by user or group name #13965

Merged
merged 1 commit into from
Oct 30, 2024
Merged

NAS-132041 / 25.04 / Allow setting ACL entries by user or group name #13965

merged 1 commit into from
Oct 30, 2024

Conversation

anodos325
Copy link
Contributor

@anodos325 anodos325 commented Jul 2, 2024 •
edited
Loading

This commit converts filesystem ACL APIs to use new api_method as well as improving the filesystem.setacl API to allow setting
ACL entries by user / group name. This commit also removes several private APIs.

@anodos325 anodos325 force-pushed the allow-setting-acl-who branch 4 times, most recently from 8faea66 to 9a7570e Compare July 3, 2024 20:52
@anodos325 anodos325 force-pushed the allow-setting-acl-who branch 12 times, most recently from 2fac8ff to 4d299d7 Compare October 28, 2024 17:05
@anodos325 anodos325 added the jira label Oct 28, 2024
@anodos325 anodos325 requested review from a team and bmeagherix October 28, 2024 17:06
@bugclerk bugclerk changed the title Allow setting ACL entries by user or group name NAS-132041 / 25.04 / Allow setting ACL entries by user or group name Oct 28, 2024
@bugclerk
Copy link
Contributor

Jira URL: https://ixsystems.atlassian.net/browse/NAS-132041

@anodos325 anodos325 requested a review from mgrimesix October 28, 2024 17:06
@yocalebo yocalebo requested a review from creatorcary October 28, 2024 18:08
@creatorcary
Copy link
Contributor

I won't request this change since solidifying standards for our new api style is still a WIP, but I'll just note that if we go with the currently proposed standard of having one module in api/v25_04_0 for every plugin, then we'll have to split api/v25_04_0/acl.py into filesystem.py and filesystem_acltemplate.py.

@anodos325
Copy link
Contributor Author

anodos325 commented Oct 28, 2024 •
edited
Loading

I won't request this change since solidifying standards for our new api style is still a WIP, but I'll just note that if we go with the currently proposed standard of having one module in api/v25_04_0 for every plugin, then we'll have to split api/v25_04_0/acl.py into filesystem.py and filesystem_acltemplate.py.

I'd rather not do that unless it's required. These two plugins share lots of data structures. Putting ACL into fileystem.py is just going to overload that file with irrelevant information IMHO (since it already will have a lot of models).

bmeagherix
bmeagherix reviewed Oct 29, 2024
View reviewed changes
src/middlewared/middlewared/plugins/filesystem_/acl.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/filesystem_/acl.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/filesystem_/acl.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/filesystem_/acl.py Show resolved Hide resolved
src/middlewared/middlewared/plugins/filesystem_/acl.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/filesystem_/perm_check.py Show resolved Hide resolved
src/middlewared/middlewared/plugins/filesystem_/utils.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/pytest/unit/plugins/test_acl_inherit.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/pytest/unit/plugins/test_acl_inherit.py Show resolved Hide resolved
tests/api2/test_acl_by_who.py Outdated Show resolved Hide resolved
@anodos325 anodos325 requested a review from bmeagherix October 30, 2024 13:50
bmeagherix
bmeagherix approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@bmeagherix bmeagherix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

mgrimesix
mgrimesix approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@mgrimesix mgrimesix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@anodos325 anodos325 force-pushed the allow-setting-acl-who branch from 55e5e1f to 85d7e25 Compare October 30, 2024 14:55
@anodos325
Add ability to set ACL entries by username
ca20c9e 
This commit converts filesystem ACL APIs to use new api_method
as well as improving the filesystem.setacl API to allow setting
ACL entries by name. This commit also removes several private
APIs.
@anodos325 anodos325 force-pushed the allow-setting-acl-who branch from 85d7e25 to ca20c9e Compare October 30, 2024 14:57
@anodos325 anodos325 merged commit d869cd2 into master Oct 30, 2024
2 of 3 checks passed
@anodos325 anodos325 deleted the allow-setting-acl-who branch October 30, 2024 15:01
@bugclerk
Copy link
Contributor

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Oct 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mgrimesix mgrimesix mgrimesix approved these changes

@bmeagherix bmeagherix bmeagherix approved these changes

@creatorcary creatorcary creatorcary approved these changes

Assignees
No one assigned
Labels
jira no-time-tracked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anodos325 @bugclerk @creatorcary @bmeagherix @mgrimesix