Reading event logs and acting when specific event is not present

Question

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.

Answer 1

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;
}

Answer 2

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 };
    }
}

Answer 3

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.

Answer 4

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;