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.

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

Change from Set/GetNamedSecurityInfo to Nt{Query,Set}SecurityInfo #1589

Merged
merged 13 commits into from
Feb 1, 2022

Conversation

adreed-msft
Copy link
Member

This PR changes the way that we were obtaining and setting ACLs to ensure accuracy. Set/GetNamedSecurityInfo would do some merging, which made it not entirely accurate for backups. However, the Nt{Query,Set}SecurityInfo APIs do not do merging, and allow for considerably more accurate preservation of ACLs.

@adreed-msft adreed-msft changed the base branch from main to dev October 15, 2021 01:40
@adreed-msft adreed-msft added this to the 10.14.0 milestone Nov 24, 2021
Copy link
Contributor

@siminsavani-msft siminsavani-msft left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@mohsha-msft mohsha-msft left a comment

Choose a reason for hiding this comment

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

Change looks straight forward. I have some concerns about the package we're using tho.

uint32(len(buf)),
&bufLen)
}, &buf, &bufLen)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we verify that changes are working as expected by any unit test?

@zezha-msft
Copy link
Contributor

TODO: an automated test to double check

Copy link
Contributor

@mohsha-msft mohsha-msft left a comment

Choose a reason for hiding this comment

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

Overall changes look good. Just some Nitpicking.
QQ: On the validation size: E2E test involving Local resource use the changes you've done, correct?

@adreed-msft adreed-msft merged commit 8ac7754 into dev Feb 1, 2022
@adreed-msft adreed-msft deleted the adreed/fix-acl-api branch February 1, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants