Skip to content

fix(#390): Add admin elevation check to fail fast with clear error me…#477

Open
Randomlyclueless wants to merge 4 commits into
DotDev262:mainfrom
Randomlyclueless:fix/390-admin-elevation-check
Open

fix(#390): Add admin elevation check to fail fast with clear error me…#477
Randomlyclueless wants to merge 4 commits into
DotDev262:mainfrom
Randomlyclueless:fix/390-admin-elevation-check

Conversation

@Randomlyclueless

@Randomlyclueless Randomlyclueless commented Jun 23, 2026

Copy link
Copy Markdown

Closes #390

Proposed Changes

  • Added src/Infrastructure/Helpers/AdminGuard.cs — a static guard class that checks for Windows administrator privileges using WindowsPrincipal/WindowsIdentity, following the same pattern as the existing RegistryGuard.cs
  • Modified src/Infrastructure/AppRunner.cs to call AdminGuard.EnsureAdministrator() at the start of RunAsync(), before any config loading or system operations
  • Added tests/WinHome.Tests/AdminGuardTests.cs with 3 unit tests covering admin, non-admin, and reset delegate scenarios

Type of Change

  • 🐛 Bug fix
  • 🧪 Testing

Screenshots / Logs (if applicable)

Non-admin shell (before fix would silently fail, now shows clear error):
Uploading prevWinHome.png…

Admin shell (runs normally with full execution):
Uploading Screenshot (12).png…

Testing & Verification

  • I have run dotnet test and all 60+ cross-platform tests passed (537 total: 536 succeeded, 1 skipped).
  • I have verified the changes on a Windows environment (if applicable).
  • I have added new unit tests to cover my changes.

GSSOC 2026 Checklist

  • I have read the Contribution Guidelines.
  • My code is formatted correctly (I have run dotnet format, uv tool run ruff format plugins/, and bun x prettier --write plugins/).
  • I have linked the PR to an approved issue.
  • I understand that a maintainer must apply the gssoc:approved label for this PR to count for points.

@github-actions github-actions Bot added level:intermediate Intermediate level task type:bug Bug fix GSSOC GirlScript Summer of Code 2026 labels Jun 23, 2026
@DotDev262 DotDev262 added the gssoc:approved Approved for GSSOC points (Required) label Jun 23, 2026

@DotDev262 DotDev262 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Compilation error: duplicate using statement

In src/Infrastructure/AppRunner.cs, there is a duplicate using directive at the top of the file:

The using WinHome.Interfaces; appears twice (before and after the new line). This will prevent the code from compiling. Please remove the duplicate and re-push.

Once this is fixed and the build passes, the PR should be good to merge!

@DotDev262 DotDev262 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Compilation error: duplicate using statement

In src/Infrastructure/AppRunner.cs, there is a duplicate using directive at the top of the file:

Lines 1 and 3 both have using WinHome.Interfaces; — this won't compile. Please remove the duplicate.

Once this is fixed and the build passes, the PR should be good to merge!

@DotDev262 DotDev262 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Compilation error: duplicate using statement

In src/Infrastructure/AppRunner.cs, using WinHome.Interfaces; appears twice:

using WinHome.Interfaces;  // line 1 — KEEP
using WinHome.Models;
using WinHome.Services.Logging;
using YamlDotNet.Serialization;
using YamlDotNet.Serialization.NamingConventions;
using WinHome.Infrastructure.Helpers;  // NEW line
using WinHome.Interfaces;              // line 7 — REMOVE THIS (duplicate)

Remove the duplicate using WinHome.Interfaces; on line 7, keep everything else. Once fixed and build passes, this is ready to merge.

@DotDev262 DotDev262 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Your branch is behind main. Please rebase onto latest main and force-push so we can proceed with the review. Thanks!

@Randomlyclueless

Copy link
Copy Markdown
Author

Fixed the duplicate using statement in AppRunner.cs and force-pushed. Build should pass now.

@DotDev262 DotDev262 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Stray git fetch origin line in AppRunner.cs

The duplicate using WinHome.Interfaces; is now fixed ✅, but there's a new issue: a literal git fetch origin command appears on line 8 of AppRunner.cs:

using WinHome.Infrastructure.Helpers;  // <-- add this line
git fetch origin                       // <-- REMOVE THIS LINE

namespace WinHome.Infrastructure;

This line is not valid C# and will cause a compilation error. Please remove it.

Also, the branch is still 4 commits behind main — please rebase onto latest main and force-push after fixing.

@DotDev262

Copy link
Copy Markdown
Owner

Your PR is 9 commits behind main. Please rebase onto the latest main so we can review and merge. Thanks!

@Randomlyclueless

Copy link
Copy Markdown
Author

Sorry for the messy commits! I've fixed the stray git fetch origin line and the duplicate using statements in AppRunner.cs. The file now has clean, correct using directives. Build passes successfully and I've rebased onto the latest main and force-pushed. Ready for review!

@DotDev262

Copy link
Copy Markdown
Owner

Thanks for the contribution. However, there are two issues that need to be resolved:

  1. Cross-Platform Crash: Calling AdminGuard.EnsureAdministrator() unconditionally in AppRunner.RunAsync() will execute WindowsIdentity.GetCurrent(). Because WinHome is cross-platform, this will throw a PlatformNotSupportedException and crash the application on non-Windows environments (Linux/macOS) immediately. You must guard this check using OperatingSystem.IsWindows().
  2. Formatting: Both src/Infrastructure/Helpers/AdminGuard.cs and tests/WinHome.Tests/AdminGuardTests.cs are missing trailing POSIX newlines at the end of the file.

Please address these points, rebase if needed, and push your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gssoc:approved Approved for GSSOC points (Required) GSSOC GirlScript Summer of Code 2026 level:intermediate Intermediate level task type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System-level tasks fail silently when run from non-elevated shell

2 participants