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

Impl win permissions #2132

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

Impl win permissions #2132

wants to merge 19 commits into from

Conversation

coddie83
Copy link
Collaborator

@coddie83 coddie83 commented Feb 11, 2025

VPN-2214:file permissions on windows implementation


This change is Reviewable

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


nym-vpn-core/crates/nym-vpn-account-controller/src/storage/credentials/pending_credential_requests/mod.rs line 289 at r1 (raw file):

    // Set file permissions
    unsafe {
        if SetFileSecurityA(

Maybe we should use wide char version SetFileSecurityW and not ANSI

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @coddie83)


nym-vpn-core/crates/nym-vpn-account-controller/src/storage/credentials/pending_credential_requests/mod.rs line 227 at r1 (raw file):

    // FIXME: "SYSTEM" need to be changed to the required account(here and below)
    unsafe {
        LookupAccountNameA(

Let's use widechar version LookupAccountNameW

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @coddie83)


nym-vpn-core/crates/nym-vpn-account-controller/src/storage/credentials/pending_credential_requests/mod.rs line 210 at r1 (raw file):

    let file_path = path.as_ref();
    let c_file_path = CString::new(file_path.to_str().unwrap()).map_err(|_| std::io::Error::from(std::io::ErrorKind::InvalidInput))?;

Maybe we can leverage widestring crate to do this heavy lifting for us.


nym-vpn-core/crates/nym-vpn-account-controller/src/storage/credentials/pending_credential_requests/mod.rs line 298 at r1 (raw file):

    }

    tracing::info!("Successfully set file permissions for {:?}", file_path);

You can use normal formatting with {} instead of debug formatting {:?} by calling it viafile_path.display()

@coddie83 coddie83 force-pushed the impl_win_permissions branch from b7036f5 to f69c63a Compare February 11, 2025 20:23
Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @coddie83)


nym-vpn-core/crates/nym-vpn-account-controller/Cargo.toml line 44 at r2 (raw file):

uuid.workspace = true
zeroize.workspace = true
windows = "0.59.0"

You don't need windows here as you solely use windows-sys

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @coddie83)


nym-vpn-core/crates/nym-vpn-account-controller/Cargo.toml line 46 at r2 (raw file):

windows = "0.59.0"
windows-sys = { version = "0.59.0", features = ["Win32_System_Threading", "Win32_System_SystemServices"] }
widestring = "1.0"

We keep all dependency versions in the workspace to make it easier to update them. widestring is a part of workspace so you can simply replace the version with workspace = true. Same goes for windows-sys, i.e:

widestring.workspace = true
windows-sys = { workspace = true, features = ["Win32_System_Threading", "Win32_System_SystemServices"] }

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @coddie83)

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @coddie83)

@coddie83 coddie83 force-pushed the impl_win_permissions branch 2 times, most recently from fabe4fd to 82a2e96 Compare February 12, 2025 17:35
@coddie83 coddie83 force-pushed the impl_win_permissions branch from 82a2e96 to e1b5f16 Compare February 12, 2025 20:43
Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @coddie83)


nym-vpn-core/crates/nym-vpn-account-controller/Cargo.toml line 45 at r4 (raw file):

uuid.workspace = true
zeroize.workspace = true
windows-sys = { version = "0.59.0", features = ["Win32_System_Threading", "Win32_System_SystemServices"] }

windows-sys should use the version from workspace. Please change version = ... with workspace = true

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @coddie83)

@coddie83 coddie83 requested a review from pronebird February 13, 2025 11:52
Eugene Balabanov added 3 commits February 13, 2025 13:02
@coddie83 coddie83 force-pushed the impl_win_permissions branch from 5c41c6e to 7dc4c68 Compare February 13, 2025 12:32
Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @coddie83)

@pronebird pronebird self-requested a review February 13, 2025 12:42
Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Looks good. Windows CI has some minor issue.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @coddie83)

This reverts commit 7dc4c68.
@coddie83 coddie83 closed this Feb 13, 2025
@coddie83 coddie83 reopened this Feb 13, 2025
@pronebird pronebird self-requested a review February 13, 2025 14:38
@coddie83 coddie83 force-pushed the impl_win_permissions branch from ed3ce4e to b7c207e Compare February 13, 2025 17:05
@coddie83 coddie83 force-pushed the impl_win_permissions branch 3 times, most recently from d888975 to 2c4240c Compare February 14, 2025 03:04
@coddie83 coddie83 force-pushed the impl_win_permissions branch from 2c4240c to f070a75 Compare February 14, 2025 03:14
Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 7 files at r6.
Reviewable status: 3 of 7 files reviewed, 2 unresolved discussions (waiting on @rokas-ambrazevicius)


nym-vpn-core/crates/nym-vpn-account-controller/Cargo.toml line 47 at r6 (raw file):

windows-sys = { workspace = true, features = ["Win32_System_Threading", "Win32_System_SystemServices"] }
widestring.workspace = true
winapi = { version = "0.3" }

Can we please use it from workspace too? All versions we have are defined in workspace Cargo.toml in the root.


nym-vpn-core/crates/nym-vpn-store/Cargo.toml line 26 at r6 (raw file):

windows-sys = { workspace = true, features = ["Win32_System_Threading", "Win32_System_SystemServices"] }
widestring.workspace = true
winapi = { version = "0.3" }

Please use version from workspace. It was moved there recently I think.

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 7 files at r6.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @coddie83 and @rokas-ambrazevicius)

@coddie83 coddie83 requested a review from pronebird February 14, 2025 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants