Skip to content

Code Quality: Disable drag & drop when running as admin #15795

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

Merged
merged 1 commit into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/Files.App/Data/Contexts/Window/IWindowContext.cs
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
// Copyright (c) 2024 Files Community
// Licensed under the MIT License. See the LICENSE.

using System.ComponentModel;

namespace Files.App.Data.Contexts
{
public interface IWindowContext : INotifyPropertyChanged
{
bool IsCompactOverlay { get; }

/// <inheritdoc cref="IWindowsSecurityService.IsAppElevated"/>
bool IsRunningAsAdmin { get; }

/// <inheritdoc cref="IWindowsSecurityService.CanDragAndDrop"/>
bool CanDragAndDrop { get; }
}
}
14 changes: 13 additions & 1 deletion src/Files.App/Data/Contexts/Window/WindowContext.cs
Original file line number Diff line number Diff line change
@@ -1,18 +1,30 @@
// Copyright (c) 2024 Files Community
// Licensed under the MIT License. See the LICENSE.

using CommunityToolkit.Mvvm.ComponentModel;
using Microsoft.UI.Windowing;

namespace Files.App.Data.Contexts
{
/// <inheritdoc cref="IWindowContext"/>
internal sealed class WindowContext : ObservableObject, IWindowContext
{
private IWindowsSecurityService WindowsSecurityService = Ioc.Default.GetRequiredService<IWindowsSecurityService>();

private bool isCompactOverlay;
/// <inheritdoc/>
public bool IsCompactOverlay => isCompactOverlay;

/// <inheritdoc/>
public bool IsRunningAsAdmin { get; private set; }

/// <inheritdoc/>
public bool CanDragAndDrop { get; private set; }

public WindowContext()
{
IsRunningAsAdmin = WindowsSecurityService.IsAppElevated();
CanDragAndDrop = WindowsSecurityService.CanDragAndDrop();

MainWindow.Instance.PresenterChanged += Window_PresenterChanged;
}

Expand Down
40 changes: 40 additions & 0 deletions src/Files.App/Data/Contracts/IWindowsSecurityService.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright (c) 2024 Files Community
// Licensed under the MIT License. See the LICENSE.

namespace Files.App.Data.Contracts
{
/// <summary>
/// Provides service for security APIs on Windows.
/// </summary>
public interface IWindowsSecurityService
{
/// <summary>
/// Gets a value that indicates whether the application is elevated.
/// </summary>
/// <returns>Returns true if the application is elevated; otherwise, false.</returns>
bool IsAppElevated();

/// <summary>
/// Gets a value that indicates whether the application can drag &amp; drop.
/// </summary>
/// <remarks>
/// Drag &amp; drop onto an elevated app is not allowed (just crashes) due to UIPI.
/// <br/>
/// <br/>
/// For more info, visit:
/// <br/>
/// <a href="https://github.com/files-community/Files/issues/12390"/>
/// <br/>
/// <a href="https://github.com/microsoft/terminal/issues/12017#issuecomment-1004129669"/>
/// </remarks>
/// <returns>Returns true if the application can drag &amp; drop; otherwise, false.</returns>
bool CanDragAndDrop();

/// <summary>
/// Gets a value that indicates whether the application needs to be elevated for some operations.
/// </summary>
/// <param name="path"></param>
/// <returns>True if the application needs to be elevated for some operations; otherwise, false.</returns>
bool IsElevationRequired(string path);
}
}
1 change: 1 addition & 0 deletions src/Files.App/Helpers/Application/AppLifecycleHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ public static IHost ConfigureHost()
// Services
.AddSingleton<IWindowsIniService, WindowsIniService>()
.AddSingleton<IWindowsWallpaperService, WindowsWallpaperService>()
.AddSingleton<IWindowsSecurityService, WindowsSecurityService>()
.AddSingleton<IAppThemeModeService, AppThemeModeService>()
.AddSingleton<IDialogService, DialogService>()
.AddSingleton<ICommonDialogService, CommonDialogService>()
Expand Down
24 changes: 0 additions & 24 deletions src/Files.App/Helpers/Environment/ElevationHelpers.cs

This file was deleted.

32 changes: 32 additions & 0 deletions src/Files.App/Services/Windows/WindowsSecurityService.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright (c) 2024 Files Community
// Licensed under the MIT License. See the LICENSE.

namespace Files.App.Services
{
/// <inheritdoc cref="IWindowsSecurityService"/>
public sealed class WindowsSecurityService : IWindowsSecurityService
{
/// <inheritdoc/>
public unsafe bool IsAppElevated()
{
var identity = System.Security.Principal.WindowsIdentity.GetCurrent();
var principal = new System.Security.Principal.WindowsPrincipal(identity);
return principal.IsInRole(System.Security.Principal.WindowsBuiltInRole.Administrator);
}

/// <inheritdoc/>
public unsafe bool CanDragAndDrop()
{
return !IsAppElevated();
}

/// <inheritdoc/>
public bool IsElevationRequired(string path)
{
if (string.IsNullOrEmpty(path))
return false;

return Win32PInvoke.IsElevationRequired(path);
}
}
}
13 changes: 5 additions & 8 deletions src/Files.App/UserControls/TabBar/TabBar.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public sealed partial class TabBar : BaseTabBar, INotifyPropertyChanged

private readonly ICommandManager Commands = Ioc.Default.GetRequiredService<ICommandManager>();
private readonly IAppearanceSettingsService AppearanceSettingsService = Ioc.Default.GetRequiredService<IAppearanceSettingsService>();
private readonly IWindowContext WindowContext = Ioc.Default.GetRequiredService<IWindowContext>();

// Fields

Expand All @@ -44,12 +45,8 @@ public sealed partial class TabBar : BaseTabBar, INotifyPropertyChanged
public bool ShowTabActionsButton
=> AppearanceSettingsService.ShowTabActions;

// Dragging makes the app crash when run as admin.
// For more information:
// - https://github.com/files-community/Files/issues/12390
// - https://github.com/microsoft/terminal/issues/12017#issuecomment-1004129669
public bool AllowTabsDrag
=> !ElevationHelpers.IsAppRunAsAdmin();
=> WindowContext.CanDragAndDrop;

public Rectangle DragArea
=> DragAreaRectangle;
Expand Down Expand Up @@ -172,7 +169,7 @@ private void TabView_TabStripDragOver(object sender, DragEventArgs e)
{
if (e.DataView.Properties.ContainsKey(TabPathIdentifier))
{
HorizontalTabView.CanReorderTabs = true && !ElevationHelpers.IsAppRunAsAdmin();
HorizontalTabView.CanReorderTabs = WindowContext.CanDragAndDrop;

e.AcceptedOperation = DataPackageOperation.Move;
e.DragUIOverride.Caption = "TabStripDragAndDropUIOverrideCaption".GetLocalizedResource();
Expand All @@ -187,12 +184,12 @@ private void TabView_TabStripDragOver(object sender, DragEventArgs e)

private void TabView_DragLeave(object sender, DragEventArgs e)
{
HorizontalTabView.CanReorderTabs = true && !ElevationHelpers.IsAppRunAsAdmin();
HorizontalTabView.CanReorderTabs = WindowContext.CanDragAndDrop;
}

private async void TabView_TabStripDrop(object sender, DragEventArgs e)
{
HorizontalTabView.CanReorderTabs = true && !ElevationHelpers.IsAppRunAsAdmin();
HorizontalTabView.CanReorderTabs = WindowContext.CanDragAndDrop;

if (!(sender is TabView tabStrip))
return;
Expand Down
5 changes: 2 additions & 3 deletions src/Files.App/ViewModels/ShellViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public sealed class ShellViewModel : ObservableObject, IDisposable
private readonly IFileTagsSettingsService fileTagsSettingsService = Ioc.Default.GetRequiredService<IFileTagsSettingsService>();
private readonly ISizeProvider folderSizeProvider = Ioc.Default.GetRequiredService<ISizeProvider>();
private readonly IStorageCacheService fileListCache = Ioc.Default.GetRequiredService<IStorageCacheService>();
private readonly IWindowsSecurityService WindowsSecurityService = Ioc.Default.GetRequiredService<IWindowsSecurityService>();

// Only used for Binding and ApplyFilesAndFoldersChangesAsync, don't manipulate on this!
public BulkConcurrentObservableCollection<ListedItem> FilesAndFolders { get; }
Expand Down Expand Up @@ -1268,9 +1269,7 @@ private bool CheckElevationRights(ListedItem item)
if (item.SyncStatusUI.LoadSyncStatus)
return false;

return item.IsShortcut
? ElevationHelpers.IsElevationRequired(((ShortcutItem)item).TargetPath)
: ElevationHelpers.IsElevationRequired(item.ItemPath);
return WindowsSecurityService.IsElevationRequired(item.IsShortcut ? ((ShortcutItem)item).TargetPath : item.ItemPath);
}

public async Task LoadGitPropertiesAsync(GitItem gitItem)
Expand Down
5 changes: 2 additions & 3 deletions src/Files.App/Views/Layouts/BaseLayoutPage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public abstract class BaseLayoutPage : Page, IBaseLayoutPage, INotifyPropertyCha
protected IUserSettingsService UserSettingsService { get; } = Ioc.Default.GetService<IUserSettingsService>()!;
protected ICommandManager Commands { get; } = Ioc.Default.GetRequiredService<ICommandManager>();
public InfoPaneViewModel InfoPaneViewModel { get; } = Ioc.Default.GetRequiredService<InfoPaneViewModel>();
protected readonly IWindowContext WindowContext = Ioc.Default.GetRequiredService<IWindowContext>();

// ViewModels

Expand Down Expand Up @@ -82,10 +83,8 @@ public CurrentInstanceViewModel? InstanceViewModel
public static AppModel AppModel
=> App.AppModel;

// NOTE: Dragging makes the app crash when run as admin. (#12390)
// For more information, visit https://github.com/microsoft/terminal/issues/12017#issuecomment-1004129669
public bool AllowItemDrag
=> !ElevationHelpers.IsAppRunAsAdmin();
=> WindowContext.CanDragAndDrop;

public CommandBarFlyout ItemContextMenuFlyout { get; set; } = new()
{
Expand Down
15 changes: 2 additions & 13 deletions src/Files.App/Views/MainPage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@
using Microsoft.UI.Xaml.Controls;
using Microsoft.UI.Xaml.Input;
using Microsoft.UI.Xaml.Navigation;
using Sentry;
using System.Data;
using Windows.ApplicationModel;
using Windows.ApplicationModel.DataTransfer;
using Windows.Foundation.Metadata;
using Windows.Graphics;
using Windows.Services.Store;
Expand All @@ -28,24 +25,17 @@ public sealed partial class MainPage : Page
{
private IGeneralSettingsService generalSettingsService { get; } = Ioc.Default.GetRequiredService<IGeneralSettingsService>();
public IUserSettingsService UserSettingsService { get; }

private readonly IWindowContext WindowContext = Ioc.Default.GetRequiredService<IWindowContext>();
public ICommandManager Commands { get; }

public IWindowContext WindowContext { get; }

public SidebarViewModel SidebarAdaptiveViewModel { get; }

public MainPageViewModel ViewModel { get; }

public StatusCenterViewModel OngoingTasksViewModel { get; }

public static AppModel AppModel
=> App.AppModel;

private bool keyReleased = true;

private bool isAppRunningAsAdmin => ElevationHelpers.IsAppRunAsAdmin();

private DispatcherQueueTimer _updateDateDisplayTimer;

public MainPage()
Expand All @@ -55,7 +45,6 @@ public MainPage()
// Dependency Injection
UserSettingsService = Ioc.Default.GetRequiredService<IUserSettingsService>();
Commands = Ioc.Default.GetRequiredService<ICommandManager>();
WindowContext = Ioc.Default.GetRequiredService<IWindowContext>();
SidebarAdaptiveViewModel = Ioc.Default.GetRequiredService<SidebarViewModel>();
SidebarAdaptiveViewModel.PaneFlyout = (MenuFlyout)Resources["SidebarContextMenu"];
ViewModel = Ioc.Default.GetRequiredService<MainPageViewModel>();
Expand Down Expand Up @@ -307,7 +296,7 @@ private void Page_Loaded(object sender, RoutedEventArgs e)
if
(
AppLifecycleHelper.AppEnvironment is not AppEnvironment.Dev &&
isAppRunningAsAdmin &&
WindowContext.IsRunningAsAdmin &&
UserSettingsService.ApplicationSettingsService.ShowRunningAsAdminPrompt
)
{
Expand Down
7 changes: 4 additions & 3 deletions src/Files.App/Views/Settings/TagsPage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,17 @@ namespace Files.App.Views.Settings
{
public sealed partial class TagsPage : Page
{
private readonly IWindowContext WindowContext = Ioc.Default.GetRequiredService<IWindowContext>();

private string oldTagName = string.Empty;

// Will be null unless the user has edited any tag
private ListedTagViewModel? editingTag;

private FlyoutBase? deleteItemFlyout;

// See issue #12390 on Github. Dragging makes the app crash when run as admin.
// Further reading: https://github.com/microsoft/terminal/issues/12017#issuecomment-1004129669
public bool AllowItemsDrag => !ElevationHelpers.IsAppRunAsAdmin();
public bool AllowItemsDrag
=> WindowContext.CanDragAndDrop;

public TagsPage()
{
Expand Down