Skip to content

[solana-install-init] Optimize error message for Windows user permission installation#234

Merged
joncinque merged 4 commits intoanza-xyz:masterfrom
WGB5445:patch1
Mar 18, 2024
Merged

[solana-install-init] Optimize error message for Windows user permission installation#234
joncinque merged 4 commits intoanza-xyz:masterfrom
WGB5445:patch1

Conversation

@WGB5445
Copy link
Copy Markdown

@WGB5445 WGB5445 commented Mar 14, 2024

Problem

When users attempt to execute this command without administrator privileges, it results in error code 1314.

Most users find it challenging to promptly recognize that this issue stems from permission limitations.

image

Summary of Changes

Therefore, I propose two possible solutions:

  1. Implement a permission checking function utilizing winapi.
unsafe {
        let mut handle: HANDLE = ptr::null_mut();
        if OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut handle) == 0 {
            return Err(format!("{}", Error::last_os_error()));
        }

        let mut elevation = TOKEN_ELEVATION::default();
        let size = std::mem::size_of::<TOKEN_ELEVATION>() as u32;
        let mut ret_size = size;
        let result = GetTokenInformation(
            handle,
            TokenElevation,
            &mut elevation as *mut _ as *mut _,
            size,
            &mut ret_size,
        );
        if result == 0 {
            return Err(format!("{}", Error::last_os_error()));
        }

        if elevation.TokenIsElevated != 0 {
            Ok(())
        } else {
            Err(
                "You need to run this command with administrator privileges."
                    .parse()
                    .unwrap(),
            )
        }
    }
  1. Check for error code 1314 in the error message.
symlink_dir(
        release_dir.join("solana-release"),
        config.active_release_dir(),
    )
    .map_err(|err| match err.raw_os_error() {
        Some(1314) => "You need to run this command with administrator privileges.".to_string(),
        _ => format!(
            "Unable to symlink {:?} to {:?}: {}",
            release_dir,
            config.active_release_dir(),
            err
        ),
    })?;

Your valuable feedback on these proposed solutions would be greatly appreciated.

@mergify mergify Bot requested a review from a team March 14, 2024 05:07
@WGB5445
Copy link
Copy Markdown
Author

WGB5445 commented Mar 14, 2024

I'd appreciate some assistance, such as which approach is better, or both are equally good.

Thank you very much.

Copy link
Copy Markdown

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I like the approach of catching the error. I've added a little suggestion to help document the behavior a bit more

Comment thread install/src/command.rs Outdated
Copy link
Copy Markdown

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Just two more little things -- this is very close!

Comment thread install/src/command.rs Outdated
Comment thread install/src/command.rs Outdated
Comment on lines +1176 to +1186
#[cfg(windows)]
Some(os_err) => if os_err == winapi::shared::winerror::ERROR_PRIVILEGE_NOT_HELD {
"You need to run this command with administrator privileges.".to_string()
} else {
format!(
"Unable to symlink {:?} to {:?}: {}",
release_dir,
config.active_release_dir(),
err
)
},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can do match with a conditional avoid the copying of the fallback error, ie:

Suggested change
#[cfg(windows)]
Some(os_err) => if os_err == winapi::shared::winerror::ERROR_PRIVILEGE_NOT_HELD {
"You need to run this command with administrator privileges.".to_string()
} else {
format!(
"Unable to symlink {:?} to {:?}: {}",
release_dir,
config.active_release_dir(),
err
)
},
#[cfg(windows)]
Some(os_err) if os_err == winapi::shared::winerror::ERROR_PRIVILEGE_NOT_HELD => {
"You need to run this command with administrator privileges.".to_string()
},

More info at https://doc.rust-lang.org/reference/expressions/match-expr.html#match-guards

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you very much, this is so important.

In fact, I started developing with windows, but I found that these codes could not be compiled correctly on windows because some build.rs would report errors.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

image

@WGB5445 WGB5445 marked this pull request as ready for review March 16, 2024 03:02
@WGB5445 WGB5445 requested review from joncinque and t-nelson March 18, 2024 04:00
@joncinque joncinque added the CI Pull Request is ready to enter CI label Mar 18, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 18, 2024
Copy link
Copy Markdown

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@joncinque joncinque merged commit a71a62f into anza-xyz:master Mar 18, 2024
@WGB5445 WGB5445 deleted the patch1 branch March 18, 2024 17:14
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
…ion installation (anza-xyz#234)

* feat: check user's permissions in Windows

* feat: Remove check fun and check os_err

* fmt and optimize code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants