feat: add Windows administrator privilege check#9
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Windows administrator privilege checking to match the existing Unix root privilege verification, ensuring the application fails gracefully with a clear error message when run without proper privileges on Windows instead of encountering cryptic OS-level errors.
Changes:
- Implemented Windows administrator privilege check using TokenElevation API in
check_privileges() - Added Windows-specific guidance for the
allow-non-admincommand explaining setuid is unavailable - Added Windows build verification to CI pipeline
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util.rs | Added is_elevated() function using Windows API to check administrator privileges and integrated it into check_privileges() |
| src/command/allow_non_admin.rs | Added Windows-specific message explaining setuid is unavailable, separated from other unsupported OS message |
| src/bin/it.rs | Added conditional Windows hint about running as Administrator when bootit execution fails |
| Cargo.toml | Moved libc to Unix-only dependency and added windows-sys as Windows-specific dependency with required features |
| Cargo.lock | Updated with windows-sys 0.59.0 and its dependencies, version bumped to 0.1.2 |
| .github/workflows/ci.yml | Added Windows build job to verify compilation on Windows platform |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Build | ||
| run: cargo build | ||
|
|
||
| test-windows: |
There was a problem hiding this comment.
The job name 'test-windows' is inconsistent with the existing job name 'test', which runs on Linux. For consistency and clarity, consider renaming the existing job to 'test-linux' or renaming this job to something that better reflects it's a build-only job (like 'build-windows'), since neither job actually runs tests.
| let result = GetTokenInformation( | ||
| token_handle, | ||
| TokenElevation, | ||
| &mut elevation as *mut _ as *mut _, | ||
| size, | ||
| &mut return_length, | ||
| ); | ||
|
|
||
| CloseHandle(token_handle); | ||
|
|
||
| result != 0 && elevation.TokenIsElevated != 0 |
There was a problem hiding this comment.
If GetTokenInformation fails (returns 0), the handle is still closed properly, but the function returns false without distinguishing between "not elevated" and "failed to check elevation". This might be acceptable for this use case, but consider whether failed API calls should be logged or handled differently to aid debugging.
TokenElevationAPIallow-non-admincommandOn Linux,
check_privileges()verifies root viageteuid(). On Windows, no equivalent check existed — running without administrator resulted in cryptic OS-level errors when accessing EFI variables. This change adds a runtime check usingOpenProcessToken+GetTokenInformation, matching the existing Unix pattern.tit