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

fix(job_attachment)!: use username instead of group for Windows file permissions setting #196

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

gahyusuh
Copy link
Contributor

@gahyusuh gahyusuh commented Mar 5, 2024

What was the problem/requirement? (What/Why)

The use of group names in permission settings for Windows file system was identified as unnecessary for our needs. Windows uses ACLs for controlling access to directories and files, and it offers flexibility to assign specific permissions directly to individual trustees (users), so the use of group name seemed to be redundant.

What was the solution? (How)

  1. In the WindowsFileSystemPermissionSettings class, made the os_group field Optional.
    • Later, once the Worker Agent that instantiate this class (here) is updated to not pass the os_group field, we can completely remove this optional field from Job Attachment code.
  2. Updated the permission setting logic to use usernames instead of group names for ACL settings.

What is the impact of this change?

This change simplifies the permission setting process in Windows by removing unnecessary os_group.

How was this change tested?

  • Unit tests: fix a few existing tests to accommodate the changes, ensuring all tests passed.
  • Scripted test: run scripted_tests/set_file_permission_for_windows.py to verify that the specified permissions were correctly applied to files, with the permissions being granted to the specified user.

Was this change documented?

Yes, we have a written document that justifies this change.

Is this a breaking change?

No.

@gahyusuh gahyusuh force-pushed the gahyusuh/windows_no_os_group branch from 26f4a15 to 6768ca0 Compare March 8, 2024 13:40
@gahyusuh gahyusuh changed the title fix(job_attachment): use username instead of group name when setting Windows file permissions fix(job_attachment): use username instead of group for Windows file permissions setting Mar 8, 2024
@gahyusuh gahyusuh force-pushed the gahyusuh/windows_no_os_group branch from 6768ca0 to 2738626 Compare March 8, 2024 15:26
@gahyusuh gahyusuh marked this pull request as ready for review March 8, 2024 15:32
@gahyusuh gahyusuh requested a review from a team as a code owner March 8, 2024 15:32
@epmog
Copy link
Contributor

epmog commented Mar 11, 2024

Swapping from group -> user still has an effect on the behaviour, no? And now we effectively ignore the group? Should this be a breaking change since the permissions are changing in a non-backwards compatible way (even if the API handles it in a backwards compatible way)? Let me know if I'm holding it wrong

…permissions setting

Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
@gahyusuh gahyusuh force-pushed the gahyusuh/windows_no_os_group branch from 2738626 to f99e224 Compare March 11, 2024 19:46
@gahyusuh gahyusuh changed the title fix(job_attachment): use username instead of group for Windows file permissions setting fix(job_attachment)!: use username instead of group for Windows file permissions setting Mar 11, 2024
@gahyusuh
Copy link
Contributor Author

Swapping from group -> user still has an effect on the behaviour, no? And now we effectively ignore the group? Should this be a breaking change since the permissions are changing in a non-backwards compatible way (even if the API handles it in a backwards compatible way)? Let me know if I'm holding it wrong

Yes, we are now ignoring the group name and only using username, and the group name is no longer necessary. I've kept the os_group field optional for backward compatibility. (Let me know please if I am misunderstanding your points.)
And yes you are right, this should be a breaking change.

@gahyusuh gahyusuh merged commit 4c092bb into mainline Mar 11, 2024
18 checks passed
@gahyusuh gahyusuh deleted the gahyusuh/windows_no_os_group branch March 11, 2024 20:02
npmacl pushed a commit that referenced this pull request Mar 21, 2024
…permissions setting (#196)

Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
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.

3 participants