2
\$\begingroup\$

I have a small application which is basically a FileSystemWatcher which performs some operations when a file is updated.

What I need it to do is query the event logs and check whether a specific event fired.

To achieve this I created the EventLogHelper class:

class EventLogHelper
{
    private readonly int Timespan; 
    private readonly string PcName;
    private readonly string Filter;

    public EventLogHelper()
    {
        Timespan = 30000;
        PcName = Environment.MachineName;
        Filter = $"*[System[(EventID='5061' or EventID='5058') and TimeCreated[timediff(@SystemTime) <= {Timespan}]]]";
    }

    /// <summary>
    /// Checks the event logs for remote pc and returns true if any of the events we are interested in fired. 
    /// This will try checking 30 times (or until the event is found) with a 1 second wait after each check. 
    /// </summary>
    public bool CheckEvents()
    {
        var query = BuildQuery(PcName, Filter);

        for (var i = 0; i < 30; i++)
        {
            var logs = QueryEvents(query);
            var events = ReadLogs(logs);

            if (events > 0)
            {
                return true;
            }

            System.Threading.Thread.Sleep(1000);
        }

        return false;
    }

    /// <summary>
    /// Builds an EventLogQuery for the given pcname and filter. This user needs to be in the Event Log Readers security group. 
    /// </summary>
    private static EventLogQuery BuildQuery(string pcName, string filter)
    {
        var session = new EventLogSession();
        using (var pw = GetPassword())
        {
            session = new EventLogSession(
            pcName,
            "DOMAIN",
            "SystemAccount",
            pw,
            SessionAuthentication.Default);
        }

        return new EventLogQuery("Security", PathType.LogName, filter)
            { Session = session };
    }

    /// <summary>
    /// Execute the given EventLogQuery
    /// </summary>
    private static EventLogReader QueryEvents(EventLogQuery query)
    {
        try
        {
            return new EventLogReader(query);
        }
        catch (Exception ex)
        {
            Email.Send($"Error occured when instantiating a new EventLogReader \n\n Exception: {ex.Message} \n\n");
            Environment.Exit(Environment.ExitCode);
        }

        return new EventLogReader(query);
    }

    /// <summary>
    /// Read the given EventLogReader and return the amount of events that match the IDs we are looking for
    /// </summary>
    private static int ReadLogs(EventLogReader logReader)
    {
        var count5058 = 0;
        var count5061 = 0;
        EventRecord entry;

        while ((entry = logReader.ReadEvent()) != null)
        {
            if (entry.Id == 5058)
            {
                count5058++;
            }
            else
            {
                count5061++;
            }
        }

        return count5058 + count5061;
    }


    /// <summary>
    /// Return the password stored in an encrypted bytearray. 
    /// </summary>
    private static SecureString GetPassword()
    {
        byte[] data = { 14, 105, 212, 35, 167, 212, 91, 78, 1, 216, 136, 25, 80, 60, 164, 160 };
        var crypto = new SimpleAES();
        var outval = crypto.Decrypt(data);

        var ss = new SecureString();

        foreach (char c in outval)
        {
            ss.AppendChar(c);
        }

        return ss;
    }

I am fairly happy with this class and the method in general, as I can call it like so in my event handler (when the file gets updated):

var eventHelper = new EventLogHelper();
var result = eventHelper.CheckEvents();

if (!result)
{
    // The event didn't fire - we need to act!
}

However there are a couple of things which I am not sure about, for example my pattern in CheckEvents seems reasonable as I only call BuildQuery once, but do I really need to call QueryEvents on every iteration?


I also have concerns about the QueryEvents method itself, where I have the same return statement twice. This was originally written as:

private static EventLogReader QueryEvents(EventLogQuery query)
{
    return new EventLogReader(query);
}

But this can throw an Unauthorised Access error, so I need to handle that. Is there a better way of writing this statement where I don't need to write the return statement twice?

I thought about adding EventLogReader as a property, but then I don't know how to pass in the eventQuery after the reader has already been initialised.

\$\endgroup\$
  • 1
    \$\begingroup\$ Have you actually run this? QueryEvents() will terminate the application every time it's called. \$\endgroup\$ – 404 Jul 28 '16 at 11:37
  • \$\begingroup\$ @eurotrash Sorry about that - the finally block was meant to be removed, I have updated my question \$\endgroup\$ – Bassie Jul 28 '16 at 11:39
  • \$\begingroup\$ @eurotrash the idea with that was that since the application will exit in the catch due to Environment.Exit, the final return will never be called (which makes me feel that it is redundant) - but the method has to return something! \$\endgroup\$ – Bassie Jul 28 '16 at 11:42
  • \$\begingroup\$ Have you tried that now after the change? I think VS should allow you to remove that second return now. \$\endgroup\$ – 404 Jul 28 '16 at 11:45
  • 2
    \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – Mast Jul 28 '16 at 12:01
4
\$\begingroup\$

Your method ReadLogs is a little ridiculous

private static int ReadLogs(EventLogReader logReader)
{
    var count5058 = 0;
    var count5061 = 0;
    EventRecord entry;

    while ((entry = logReader.ReadEvent()) != null)
    {
        if (entry.Id == 5058)
        {
            count5058++;
        }
        else
        {
            count5061++;
        }
    }

    return count5058 + count5061;
}

Isn't it the same as the following ?!

private static int ReadLogs(EventLogReader logReader)
{
    var count = 0;
    EventRecord entry;

    while ((entry = logReader.ReadEvent()) != null)
    {
       count++;
    }

    return count;
}
\$\endgroup\$
  • \$\begingroup\$ Yes you are absolutely right - I updated my code to reflect what I was actually trying to do. You see I need both events to fire, so without checking the ID I risk count returning as 2 where its just the same ID firing twice (this is quite common in this scenario) thank you for spotting that!¬ \$\endgroup\$ – Bassie Jul 28 '16 at 11:59
2
\$\begingroup\$

This code here is a little redundant

private static EventLogQuery BuildQuery(string pcName, string filter)
{
    var session = new EventLogSession();
    using (var pw = GetPassword())
    {
        session = new EventLogSession(
        pcName,
        "DOMAIN",
        "SystemAccount",
        pw,
        SessionAuthentication.Default);
    }

    return new EventLogQuery("Security", PathType.LogName, filter)
        { Session = session };
}

you create a new EventLogSession twice when you only need to do that once, doing this you will have to return from inside the using statement, but that is perfectly okay.

You should also indent the parameters that you are passing into the constructor method, so that you can see that these are not new commands inside the using statement.

private static EventLogQuery BuildQuery(string pcName, string filter)
{
    using (var pw = GetPassword())
    {
        var session = new EventLogSession(
            pcName,
            "DOMAIN",
            "SystemAccount",
            pw,
            SessionAuthentication.Default);
        return new EventLogQuery("Security", PathType.LogName, filter)
            { Session = session };
    }
}
\$\endgroup\$
2
\$\begingroup\$

This one is redundant too:

private static EventLogReader QueryEvents(EventLogQuery query)
{
    try
    {
        return new EventLogReader(query);
    }
    catch (Exception ex)
    {
        Email.Send($"Error occured when instantiating a new EventLogReader \n\n Exception: {ex.Message} \n\n");
        Environment.Exit(Environment.ExitCode);
    }

    return new EventLogReader(query);
}

the last line will never be reached so you may just return a null in the catch to satisfy the compiler.

\$\endgroup\$
  • \$\begingroup\$ Thank you, this is what I wanted to know! I foolishly assumed that because some types can't be returned as null that this was the same for all - but it does work in this case. \$\endgroup\$ – Bassie Jul 28 '16 at 12:15
  • 1
    \$\begingroup\$ @Bassie the null works only with reference types this is with everything that is a class see What is the difference between a reference type and value type in c#? \$\endgroup\$ – t3chb0t Jul 28 '16 at 12:19
2
\$\begingroup\$

To add to what others have said:

Use constants!

private readonly int Timespan; 

This is only set in the constructor you can just do:

private const int Timespan = 30000;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.