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

Feature: Added a prompt when failing to rename items requiring additional permissions #14669

Merged

Conversation

ferrariofilippo
Copy link
Contributor

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Bug: No warning is shown when failing to rename files as non admin #8326

Known bugs

  • There are two cases where Edit permissions doesn't work:
    1. You start to rename an item on one page, and then (somehow) navigate to another before clicking Edit permissions
    2. You open the properties window for that item, navigate to a different page and then rename it

The issue behind these bugs is that the Properties window depends on IShellPage and ListedItem, while RenameAsync doesn't. To get those values, I'm using ContentPageContext, but this doesn't work if you change the working directory while renaming. Do you have any ideas on how I can fix this without messing up the code too much?

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Navigate to C:\Windows\Ssytem32
    2. Try to rename any file/folder (such as Utilman.exe)
    3. When the warning is shown try clicking both Ok and Edit permissions

Screenshots
Screenshot 2024-02-07 210409

@yaira2
Copy link
Member

yaira2 commented Feb 8, 2024

Trusted Installer is just an example, I imagine there can be others cases as well.

@ferrariofilippo
Copy link
Contributor Author

Trusted Installer is just an example, I imagine there can be others cases as well.

I guess it can only be either TrustedInstaller or SYSTEM. Is there any other account with greater privileges than admin?
Anyway, I need to find a way to distinguish the two cases since they both return code 0x80270021 (COPYENGINE_E_ACCESS_DENIED_SRC)

@yaira2
Copy link
Member

yaira2 commented Feb 8, 2024

Is there any other account with greater privileges than admin?

It could also be a non admin account that needs access from admin.

@ferrariofilippo
Copy link
Contributor Author

It could also be a non admin account that needs access from admin.

In that case there's the UAC modal

@ferrariofilippo
Copy link
Contributor Author

I tried to find a way to distinct between TrustedInstaller and SYSTEM, but got nowhere.
Using FileSecurityHelpers.GetAccessControlList() and FileSecurityHelpers.GetOwner(), I can't get anything but generic errors or exceptions (e.g. "Access denied", "NullReference" caused by source.ItemType or by GetOwner()).
I can either use a more general string (e.g. "requires higher permissions") or try again, but in this case I may need some help

@yaira2
Copy link
Member

yaira2 commented Feb 8, 2024

I can either use a more general string (e.g. "requires higher permissions")

I think that's fine

@yaira2 yaira2 changed the title Fix : Fixed a bug where no warning would be shown when renaming an item that requires TrustedInstaller Fix: Fixed an issue where no prompt was displayed when failing to rename admins Feb 11, 2024
@yaira2 yaira2 changed the title Fix: Fixed an issue where no prompt was displayed when failing to rename admins Feature: Added a prompt when failing to rename items requiring additional permissions Feb 11, 2024
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

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

LGTM regarding codebase quaility

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Feb 13, 2024
@yaira2 yaira2 merged commit 6b8bfd9 into files-community:main Feb 13, 2024
6 checks passed
@ferrariofilippo ferrariofilippo deleted the Fix_Rename_As_Admin_Fail_Warning branch November 23, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: No warning is shown when failing to rename files as non admin
3 participants