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

Add support for setting IWbemContext #103

Merged

Conversation

samin-cf
Copy link
Contributor

Addresses #102

Motivation

I need some way to programmatically determine the effective firewall status for the Public profile. The MSFT_NetFirewallProfile class returns this information, but the firewall information is scoped to the local policy. If one has firewall rules configured using a group policy, they will not be reflected in the output.

Thanks to this stackoverflow post, it is possible to query the effective firewall status by specifying PolicyStore as ActiveStore using the IWbemContext interface. This does not seem to be officially documented by Microsoft, but it does seem to work as expected.

Some queries require setting named context values using the IWbemContext
interface. For example, MSFT_NetFirewallProfile returns the firewall
status as configured by the local policy. To get the effective policy
based on the local + group policies, one must specify the 'PolicyStore' as
'ActiveStore' using the context interface.
Copy link
Owner

@ohadravid ohadravid left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking the time to implement this!

I think we can simplify the API a little and make it a bit better, but overall this is a great addition and I like the impl very much 🥳

src/context.rs Outdated
use crate::{WMIConnection, WMIResult};

#[derive(Debug, PartialEq, Serialize, Clone)]
#[serde(untagged)]
Copy link
Owner

Choose a reason for hiding this comment

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

Why the untagged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ Copy + Pasted the Variant enum and forgot to update. Thanks!

src/context.rs Outdated
}
}

impl WMIConnection {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it'll be better to try and keep the API closer to the native one, maybe something like

struct WMIContext(IWbemContext);

impl WMIConnection() {
    pub fn ctx() -> &mut WMIContext { .. }
}

impl WMIContext {
   pub fn set_value(&mut self, key: &str, value: impl Into<ContextValueType>) -> WMIResult<()> { ... }
   pub fn delete_all(..) { ... }
}

// Usage should be
// connection.ctx().set_value("IncludeHidden", true)?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice, this is much better. I have updated based on the suggestion, thanks!

use serde::Deserialize;

#[test]
fn verify_ctx_values_used() {
Copy link
Owner

Choose a reason for hiding this comment

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

Super cool!

Copy link
Owner

Choose a reason for hiding this comment

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

Can you also add an async test too? (copy-pasting this is 👌 )

Copy link
Owner

Choose a reason for hiding this comment

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

Also: since get_by_path calls GetObject, we need to pass the ctx there as well. If there's a nice way to test this, that would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the async test.

With respect to GetObject, I updated it to use the context, but I am having a difficult time testing it. I can't seem to find any WMI classes that make use of the context when invoking GetObject. Wish this was documented better...

src/context.rs Outdated

/// Clears all named values from the underlying context object.
pub fn clear_ctx_values(&mut self) -> WMIResult<()> {
unsafe { self.ctx.DeleteAll().map_err(Into::into) }
Copy link
Owner

Choose a reason for hiding this comment

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

I think ? and then doing Ok(()) in the end would also work


#[derive(Debug, PartialEq, Serialize, Clone)]
#[serde(untagged)]
pub enum ContextValueType {
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe add to this a non_exhaustive attribute, so adding more options won't be a breaking change

Copy link
Owner

@ohadravid ohadravid Nov 25, 2024

Choose a reason for hiding this comment

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

We can also replace ContextValueType with Variant / impl Into<Variant>, but then it'll be better to have impl_try_from_variant generate both impls (the new one being impl From<$target_type> for Variant), as well as Into<VARIANT> for Variant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had initially started with using Variant but settled for ContextValueType because only a subset of the VARIANT types are supported as per doc.

I guess the benefit of ContextValueType is the caller can't accidentally pass in an unsupported type whereas they can with Variant

Copy link
Owner

Choose a reason for hiding this comment

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

👍 about the doc - this makes a lot of sense.

@samin-cf samin-cf force-pushed the add-support-for-setting-iwbemcontext branch from fce8f21 to b0a9331 Compare November 25, 2024 20:20

#[derive(Debug, PartialEq, Serialize, Clone)]
#[serde(untagged)]
pub enum ContextValueType {
Copy link
Owner

Choose a reason for hiding this comment

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

👍 about the doc - this makes a lot of sense.

@ohadravid ohadravid merged commit e860c72 into ohadravid:main Nov 26, 2024
4 checks passed
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