6
\$\begingroup\$

I had to implement an auditing system where the user have to provide a reason he needs to perform a certain operation. When he provides the reason, he can perform freely it for a certain amount of time. Each execution of the operation needs to be logged somewhere.

I implemented it using a Continuation Passing Style, which allows me to cleanly handle atomically permission checking and operation logging.

public class AuditLogger
{
   private readonly TimeSpan permissionDuration;
   private readonly IAuditDatabase auditDatabase;

   private DateTime permissionExpiry = DateTime.MinValue;
   private string accessReason;

   public AuditLogger(TimeSpan permissionDuration, IAuditDatabase auditDatabase)
   {
       this.permissionDuration = permissionDuration;
       this.auditDatabase = auditDatabase;
   }

   private bool CheckPermission()
   {
       return DateTime.UTCNow < permissionExpiry;
   }

   public void SetAccessReason(string reason)
   {
       reason = reason;
       permissionExpiry = DateTime.UTCNow.Add(permissionDuration);
   }

   public void LogAccessAndExecuteOperation(Action operation, Action onPermissionExpired, Action<Exception> onError)
   {
       if(CheckPermission())
           onPermissionExpired();
       try
       {
           auditDatabase.Log(reason);
           operation();            
       }
       catch(Exception exception)
       {
           onError(exception);
       }
   }    
}

What do you think about it?

Here it is some client code to show how it is supposed to be used.

public static void Main(string[] args)
{
    var auditLogger = new AuditLogger(TimeSpan.FromMinutes(15), new AuditingDatabase);
    var reason = args[0]; // Let's assume the user tells us what he needs to do at startup
    auditLogger.SetAccessReason(reason);

    // Now the user keeps doing lots of things    
    while(1)
    {
        auditLogger.LogAccessAndExecuteOperation(
            () => { /* doing nothing here */ Thread.Sleep(1000); },
            () => {
                Console.WriteLine("Time's up. Can you please tell us again what you need to do?");
                reason = Console.ReadLine();
                auditLogger.SetAccessReason(reason);                
            },
            exeption => Console.WriteLine(exception)
            );
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ It seems like to me, two similar, but different responsibilities are being rolled up into one. You are logging user interactions which is also responsible for asking/checking permissions. It makes more sense to me to control permissions somewhere else and then if they don't have permission they never end up auditing that they did something. i.e. can't edit a user, never logged that they edited it. Shouldn't they request permission, someone gets an email, and then approves it indefinitely or for a short time period? Maybe they get notified that request was granted? \$\endgroup\$ Oct 3, 2014 at 19:44
  • \$\begingroup\$ Authorisation is really more for compliance reason that for something else. I need to keep track of why someone is accessing the data but the approval is automatic. In my code, LogAccessAndExecuteOperation logs in the database only the successful attempts. If there is no permission I ask the user to provide a reason to access. The reason I adopted a continuation passing style is to avoid some nasty corner cases when I check for permission just before the expiry time. In such scenario I'd end up having to log the access after the time has expired. In my solution it is an atomic operation \$\endgroup\$ Oct 3, 2014 at 19:53
  • 1
    \$\begingroup\$ That makes more sense if the approval is automatic. From what I can see it looks good. I need to do something similar and will reference this when I get to it. Thanks. \$\endgroup\$ Oct 3, 2014 at 19:55

1 Answer 1

4
\$\begingroup\$

The class unfortunately violates the Single Responsibility Principle because it has to deal with logging as well as expiring access.

You could use the Decorator Pattern here and split the authorization and logging into separate classes as follows:

public interface IAccessWidget
{
   string AccessReason { get; }
   void SetAccessReason(string reason);
   void ExecuteOperation(Action operation, Action onPermissionExpired, Action<Exception> onError);
}

public class AccessWidget
{
    private readonly TimeSpan _permissionDuration;
    private DateTime _permissionExpiry = DateTime.MinValue;

    public string AccessReason { get; private set; }

    public AccessWidget(TimeSpan permissionDuration)
    {
        _permissionDuration = permissionDuration;
    }

    private bool CheckPermission()
    {
        return DateTime.UtcNow < _permissionExpiry;
    }

    public void SetAccessReason(string reason)
    {
        AccessReason = reason;
        _permissionExpiry = DateTime.UtcNow.Add(_permissionDuration);
    }

    public void ExecuteOperation(Action operation, Action onPermissionExpired, Action<Exception> onError)
    {
        if(CheckPermission())
            onPermissionExpired();
        try
        {
            operation();
        }
        catch(Exception exception)
        {
            onError(exception);
        }
    }
}

public class LoggingAccessWidget : IAccessWidget
{
    private readonly IAuditDatabase _auditDatabase;
    private readonly IAccessWidget _widget;

    public string AccessReason { get { return _widget.AccessReason; } }

    public LoggingAccessWidget(IAccessWidget widget, IAuditDatabase auditDatabase)
    {
        _widget = widget;
        _auditDatabase = auditDatabase;
    }

    public void SetAccessReason(string reason)
    {
       _widget.SetAccessReason(reason);
    }

    public void ExecuteOperation(Action operation, Action onPermissionExpired, Action<Exception> onError)
    {
        _auditDatabase.Log(_widget.AccessReason);
        _widget.ExecuteOperation(operation, onPermissionExpired, onError);
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your answer. I didn't think at separating access control and logging but it actually makes sense. Thanks for noticing it \$\endgroup\$ Oct 3, 2014 at 23:27
  • 1
    \$\begingroup\$ Heh, I kinda wish I could review this answer. I'm not a fan of decorator here. +1 though, it's definitely an improvement over the original. \$\endgroup\$ Oct 4, 2014 at 3:02
  • \$\begingroup\$ @BenAaronson you can always write another answer if you feel there are other things I can improve or if you would follow an alternative approach \$\endgroup\$ Oct 4, 2014 at 6:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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