Why objects should keep it on the inside

| 0 Comments

So, I'm integrating this POP3 code with my server and the first thing I do is create null message store. I haven't implemented the message store yet, so in order for me to integrate I need a message store that just says yes to everyone being a user and provides mailboxes that are always empty... The test driven development has made this reasonably easy, I have a message store interface so I just need to stub out the methods appropriately. It's then that I find that the interface is bad...

The message store interface looks like this:

class IMessageStore 
{
   public :

      virtual bool IsUserValid(
         const std::string &username) = 0;

      virtual std::string GetPasswordForUser(
         const std::string &username) = 0;

      virtual IMailbox *LockMailbox(
         const std::string &username) = 0;

      virtual void UnlockMailbox(
         const std::string &username) = 0;

   protected :

      virtual ~IMessageStore() {}
};

I've already implemented a mock message store to enable me to test the POP3 server. The mock object can have users added and have messages added for each user and will act appropriately when the server asks it for stuff.

Unfortunately the design of the interface wont allow me to create a null message store that simply allows all users and provides empty mailboxes for them. The reason is that the interface encourages the object to expose its internals and this makes it impossible to create a null object that will do the right thing in all circumstances.

The problem is the way we do password checking. The interface allows the server to obtain the password for a user. The server then checks the value provided by a client's PASS command or creates an MD5 digest to check a cleint's APOP command... How does a null object provide the correct password for everyone? It can't :(

The interface we need could replace the GetPasswordForUser() function with these two functions

      virtual bool CheckPasswordForUser(
         const std::string &username,
         const std::string &password) = 0;

      virtual bool CheckDigestForUser(
         const std::string &username,
         const std::string &timestamp,
         const std::string &digest) = 0;

This would make the message store responsible for validating the password. The message store then simply tells the server that the password is valid. We can create a null message store by simply hardcoding a return value of true from both functions. The server becomes simpler; it no longer needs to know anything about MD5 digests as the message store is now responsible for calculating the digest from the password and timestamp and testing it against the supplied digest...

But we should go further. The message store interface is open to abuse. Someone could write a client of the message store that never bothers to call the password checking functions. This malicious client could just call LockMailbox() and obtain access to the user's mailbox. A more secure interface might combine the password check functions and the lock mailbox function:

      virtual virtual IMailbox *LockMailbox(
         const std::string &username,
         const std::string &password) = 0;

      virtual virtual IMailbox *LockMailbox(
         const std::string &username,
         const std::string &timestamp,
         const std::string &digest) = 0;

Now clients of the message store interface cant circumvent its security. The message store decides if a user can access the mailbox. The policies are stored inside the message store, so we can replace the message store with a mock message store with appropriate policies for testing and a null message store with appropriate policies for early integration...

Of course, if I'd been paying attention in my OO lessons all those years ago I'd have realised that it's always better to have the object on which the operation makes sense actually execute the operation rather than just provide another object with the information that it needs to execute the operation. In summary: don't provide getters and setters ;)

Leave a comment