11
\$\begingroup\$

The Authenticate method violates the command-query segregation principle, right? How can I adhere to the command-query segregation principle here?

public class Authenticator : IAuthenticator
{
    public bool Authenticated
    {
        get { return HttpContext.Current.User.Identity.IsAuthenticated; }
    }

    public bool Authenticate(string username, string password)
    {
        var authenticated = FormsAuthentication.Authenticate(username, password);
        if (authenticated)
            FormsAuthentication.SetAuthCookie(username, false);
        return authenticated;
    }

    public void Logout()
    {
       FormsAuthentication.SignOut();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Mark Seemann recently blogged about this. \$\endgroup\$ – Bart van Nierop Aug 27 '14 at 13:02
  • \$\begingroup\$ Where does the IAuthenticator come from? I cannot find it in MSDN. \$\endgroup\$ – abuzittin gillifirca Aug 29 '14 at 6:18
  • \$\begingroup\$ @abuzittingillifirca IAuthenticator is an interface that I defined. It is a header interface. \$\endgroup\$ – Caster Troy Aug 29 '14 at 17:21
  • \$\begingroup\$ Can't you just make Authenticate() return void and then use Authenticated instead? \$\endgroup\$ – svick Aug 31 '14 at 23:20
7
\$\begingroup\$

Since it's been several days and this question has not been answered, I will give it a go. I'm saying this not because making Authenticate follow Command-Query Separation Principle is hard, but rather I think this design may indicate a more serious problem with security configuration, and I'm not up to date with ASP.NET.

Command Query Separation

The Authenticate method violates the command-query segregation principle no?

Since you seem to be familiar with Fowler's terminology, let's look at his definition of Command/Query Separation.

The fundamental idea is that we should divide an object's methods into two sharply separated categories:

  • Queries: Return a result and do not change the observable state of the system (are free of side effects).
  • Commands: Change the state of a system but do not return a value.

Since in this case Authenticate definitely changes state, specifically the session state by setting a cookie, it is a command and should not return a value. You can achieve this, as suggested by @svick:

you [can] just make Authenticate() return void and then use Authenticated instead.

That is:

public void Authenticate(string username, string password)
{
    // unused `authenticated` can be inlined
    if (FormsAuthentication.Authenticate(username, password))
        FormsAuthentication.SetAuthCookie(username, false);
}

And why not following CQS is a problem? Because Authenticate() returns a value, it is not immediately clear to the reader of the code if the writer of the code just checks the validity of the username/password pair or does what it does now, thus makes understanding the code harder. Similarly someone looking at the IAuthenticator, which should look like this as you stated that it's a Header Interface:

public interface IAuthenticator
{
    bool Authenticated {get;}
    bool Authenticate(string username, string password);
    void Logout();
}

faces similar dilemma: Do I use Authenticate to just check for the validity of username/password, or does it do something more?

There is also another problem with returning bool from Authenticate(...), because Authenticate is an imperative word, and an Autheticated getter (read-only property) is also present, it could be that bool return value is an error code for the Authenticate command, that is you MUST check it immediately after calling the method and do something with it other than what you would to with the return value of Authenticated. For example Authenticated == false implies wrong password whereas Authenticate(username, password) == false could mean "Membership database is down". Of course error codes are a no-no.

Now I should also state that apparently MS APIs do not always follow CQS principle, as seen here.

But Command Query Separation should also make us reevaluate if the method underneath follows Single Responsibility Principle. A method like this would be used like this:

if (Authenticator.Authenticate(credentials)) {
   // continue to secured content
} else {
   // return to login page
}

We see that the if in Authenticate is duplicate, and increases the complexity of the code unnecessarily:

if (Authenticator.Validate(credentials)) {
   Authenticator.Login();
   // continue to secured content
} else {
   // return to login page
}

This division requires some further points to be made:

  • Currently Authenticator has two separate responsibilities: Checking if username/password pairs are valid, and keep user logged in across multiple requests.

  • FormsAuthentication.SetAuthCookie requires username, but you do not use it after checking whether username/password is valid, i.e. Authenticator.Login does not need it. However your current implementation would need the username to be passed to Authenticator.Login, therefore you would need to modify the interface because of an implementation detail. That is IAuthenticator would not be a proper abstraction over FormsAuthentication.

  • This is rightly so, because even though Authenticator pretends it's not a static class, it does not encapsulate it's own state. You can observe this, as it does not have any state of its own and all the methods it calls from FormsAuthentication are static methods:

    // Note that the no-argument constructor hides its dependency
    // to `Thread`, `HttpContext` or something. 
    IAuthenticator authenticatorOne = new Authenticator(); 
    IAuthenticator authenticatorTwo = new Authenticator();
    authenticatorOne.Authenticate("Alice", "4L1(3'5 p455\/\/0rD");
    authenticatorTwo.Authenticate("Bob", "808'5 p455\/\/0rD");
    authenticatorTwo.Logout();
    

    As seen here, having multiple instances of Authenticator (in a request? thread?) does not make any sense.

Naming Side Note

It is hard to parse Authenticator.Authenticated mentally, as the object of the Authenticate is absent. For example in IIdentity.IsAuthenticated it is clear that the identity is the authenticated thing. If we change members of IAuthenticator as follows:

public interface IAuthenticator
{
    bool Validate(string username, string password);

    bool SomeUserIsLoggedin {get;}
    void Login();
    void Logout();
}

Now it is clear that Login/Logout are opposite commands. SomeUserIsLoggedin checks whether Login has been called and Logout has not been called yet. And Login/Logut having no parameter matches with Some User being the object of Is Logged In, which altogether indicate that you do not care who the user is and care just that he logged in.

This way Validate is more clearly seen to be separable from the rest of the IAuthenticator members.

Header Interfaces Side Note

If you mean Header Interface as in the Fowler's dichotomy of Header/Role interfaces, it is rather an anti pattern, as they violate the ISP:

Robert Martin talks about this topic as the Interface Segregation Principle: role interfaces follow that principle but header interfaces do not.

\$\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.