feat(pedm): netmgmt wrapper to add/remove local users from local groups

[thenextman]

For a "MakeMeAdmin" style EDM mode, we require the ability to check if the user is a local administrator, and add or remove them from the local Administrators group.

Add a WinAPI wrapper over the network management functions NetLocalGroupAddMembers, NetLocalGroupGetMembers, and NetLocalGroupGetMembers. This allows us to query the membership of local groups and add or remove users.

I added functions in devolutions_pedm::utils to demonstrate usage, although that will not be final "home" for this logic. We can query if a user is directly a member of the local administrators (in which case, we may add or remove them) or if they are a member of local administrators by virtue of a nested group.

Changelog: ignore

[github-actions]

Let maintainers know that an action is required on their side

• Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

• Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release

• Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

• Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

[CBenoit]

The code is absolutely looking good to me. Unsafe code is also properly documented 💯
Since you asked for pedantry, let me be pedantic 😉
I'll approve now so you can merge when you see fit!

[CBenoit]

nitpick: I think the code can be made more straightforward and shorter, but there is also personal taste in the mix.

1. Early exit instead of maintaining a state that we modify along the way.
2. Less indentation.
3. Extra if block removed.

if is_token_member_of_administrators_group_directly(user_token)? {
    return Ok(true);
}

let local_admin_group_sid = Sid::from_well_known(
    win_api_wrappers::raw::Win32::Security::WinBuiltinAdministratorsSid,
    None,
)?;
let local_admin_group_account = local_admin_group_sid.account(None)?;
let local_admin_sids = get_local_group_members(local_admin_group_account.account_name)?;
let group_sids_and_attributes = user_token.groups()?.0;

for user_group_sid_and_attributes in group_sids_and_attributes {
    for admin_sid in &local_admin_sids {
        if admin_sid == &user_group_sid_and_attributes.sid {
            return Ok(true);
        }
    }
}

Ok(false)

The second if is_member block seemed not useful to me, as we are already checking just after is_member is modified.

Going further, using iterators you can also turn this

for user_group_sid_and_attributes in group_sids_and_attributes {
    for admin_sid in &local_admin_sids {
        if admin_sid == &user_group_sid_and_attributes.sid {
            return Ok(true);
        }
    }
}

Ok(false)

into this

group_sids_and_attributes
    .flat_map(|local_admin_sids| local_admin_sids.iter())
    .any(|admin_sid| admin_sid == &user_group_sid_and_attributes.sid)

Resulting into this code:

if is_token_member_of_administrators_group_directly(user_token)? {
    return Ok(true);
}

let local_admin_group_sid = Sid::from_well_known(
    win_api_wrappers::raw::Win32::Security::WinBuiltinAdministratorsSid,
    None,
)?;
let local_admin_group_account = local_admin_group_sid.account(None)?;
let local_admin_sids = get_local_group_members(local_admin_group_account.account_name)?;
let group_sids_and_attributes = user_token.groups()?.0;

let is_admin = group_sids_and_attributes
    .flat_map(|local_admin_sids| local_admin_sids.iter())
    .any(|admin_sid| admin_sid == &user_group_sid_and_attributes.sid)

Ok(is_admin)

Which is in my opinion easier to read. Feel free to adjust to your preferences.

(note: Code which does not use iterators may sometimes be easier to read, but as you know there is no easy metric for that 😂 )

[thenextman]

I did revise this per your recommendations, but I left the iterative solution at the end. In this instance where I think it's purely a stylistic thing, I still find that easier to parse than the declarative approach. I expect my mindset to shift here (in C# I generally prefer the declarative approach, but my comfort levels are different there)

[CBenoit]

suggestion: Use the .cast() method for pointer-to-pointer casts (rust-lang/rust-clippy#8017 / https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr)

[thenextman]

Left this as-is for now, because I'm not sure I know the best way to follow your suggestion.

Is it the case that I need to break out an intermediate variable?

i.e.

let group_name_ptr = &group_info as *const LOCALGROUP_MEMBERS_INFO_0;

and then use cast() at the final call site, i.e.

unsafe { NetLocalGroupDelMembers(None, group_name.as_pcwstr(), 0, group_name_ptr.cast(), 1) };

But probably I misunderstand

[CBenoit]

You can wrap with parenthesis:

(&group_info as *const _).cast::<u8>()

But splitting into several lines may be more readable in some cases, especially if we spell out the type like in your example.

[thenextman]

Thanks!

[CBenoit]

note: Since you use anyhow::Result, you can attach a context string to the error.

For instance:

Suggested change

	let user_sid = RawSid::try_from(security_identifier)?;
	let user_sid = RawSid::try_from(security_identifier).context("failed to convert security identifier into a RawSid")?;

You need to have the anyhow::Context trait in scope:

use anyhow::Context as _;

This is just an example, it's not necessary to have context for everything either. Depending on the situation you may want to not use anyhow::Result at all.