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: Fixed an issue where it wasn't possible to edit some file permissions #12142

Merged
merged 49 commits into from May 8, 2023

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Apr 21, 2023

Description

Fix an issue which has been reported.

Motivation and Context

I've been aware of that there're a lot of lack of function. As a part of improving, I fix an issue where some security functionalities are missing.

Validation

  • If you have a file with no ACEs
    • Remove all ACEs
    • Open its properties and see Security again
    • You can see a message that is telling there's no ACE
  • If you have a file whose owner is not current user
    • Change owner to whatever that is not you like SYSTEM, Administrators
    • Open its properties and see Security again
    • You can see a message that is telling you do not have Read Permission access rights and to see ACEs, change owner.

Removal & Its Rationale

  • Removal of functionalities
    • I removed a functionality of changing access control type
    • I removed a functionality of changing access control flags
    • I removed a functionality of changing inheritance flags
    • I removed a functionality of converting inherited ACE to explicit one
    • I removed a functionality of removing inherited ACE
  • Rationale of that removal
    • I will add back them in a further PR. Those functionalities are not at work long time before and so I decided those are might as well not to be.
  • Removal of this message
  • Rationale
    • File Explorer can see ACEs with admin privileges even if the current user doesn't have Read Permission. However, WinUI3 app cannot elevate current instance without no restarting, so there's no meaning that Continue button exists. In this case, in Files, Files suggest that you should take ownership of the object even if the current user can see ACEs with admin privileges like File Explorer do when even admin privilege doesn't work (for example, the owner is SYSTEM or something else that doesn't belong to Admin group).
      image

PR Checklist

Screenshots

Before (File Explorer or previous Files) After
image image
image image
image image

@yaira2
Copy link
Member

yaira2 commented May 1, 2023

@0x5bfa can you link the issue for these changes?

@0x5bfa
Copy link
Member Author

0x5bfa commented May 1, 2023

Done.

@0x5bfa
Copy link
Member Author

0x5bfa commented May 3, 2023

@QuaintMako Could you review?

@QuaintMako
Copy link
Contributor

@QuaintMako Could you review?

I should be able in the upcoming days, probably around Sunday at worst.

@QuaintMako
Copy link
Contributor

Also, I did not test any fonctionnality, I'm not versed at all in the security processes.

QuaintMako
QuaintMako previously approved these changes May 5, 2023
Co-authored-by: Yair <39923744+yaira2@users.noreply.github.com>
0x5bfa and others added 3 commits May 6, 2023 06:57
Co-authored-by: Yair <39923744+yaira2@users.noreply.github.com>
yaira2
yaira2 previously approved these changes May 7, 2023
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 changed the title Fix: Fix an issue where some security functionalities are missing Fix: Fixed an issue where some it wasn't possible to edit some file permissions May 8, 2023
@yaira2 yaira2 changed the title Fix: Fixed an issue where some it wasn't possible to edit some file permissions Fix: Fixed an issue where it wasn't possible to edit some file permissions May 8, 2023
@yaira2 yaira2 merged commit 79754bf into files-community:main May 8, 2023
2 checks passed
@0x5bfa 0x5bfa deleted the 5bfa/fix-security branch May 9, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Cannot add/remove access control entry
3 participants