3
\$\begingroup\$

I feel like this structure could be better, but I can't realize how.

    internal AuthenticationResult ProcessOpenIdResponse(IAuthenticationResponse response)
    {
        switch (response.Status)
        {
            case AuthenticationStatus.Authenticated:
            {
                string openId = response.ClaimedIdentifier;
                User user = userRepository.GetByOpenId(openId);
                bool userIsNew = false;
                bool connectionIsNew = false;
                if (user == null)
                {
                    connectionIsNew = true;

                    string email = null;
                    string displayName = null;
                    var fetch = response.GetExtension<FetchResponse>();
                    if (fetch != null)
                    {
                        email = fetch.GetAttributeValue(WellKnownAttributes.Contact.Email);
                        displayName = fetch.GetAttributeValue(WellKnownAttributes.Name.FullName);
                    }
                    if (!email.NullOrEmpty()) // maybe they already have an account.
                    {
                        user = userRepository.GetByEmail(email);
                    }
                    if (user == null) // create a brand new account.
                    {
                        user = userRepository.CreateFromOpenId(openId, email, displayName);
                        userIsNew = true;
                    }
                    else // just connect the existing account to their OpenId account.
                    {
                        userRepository.AddOpenIdConnection(user, openId);
                    }
                }
                return new AuthenticationResult
                {
                    Status = ConnectionStatus.Authenticated,
                    UserIsNew = userIsNew,
                    ConnectionIsNew = connectionIsNew,
                    DisplayName = user.DisplayName,
                    UserId = user.UserId
                };
            }
            case AuthenticationStatus.Canceled:
            {
                return new AuthenticationResult
                {
                    Status = ConnectionStatus.Canceled,
                    Message = Common.Resources.Authentication.CanceledAtProvider
                };
            }
            case AuthenticationStatus.Failed:
            {
                return new AuthenticationResult
                {
                    Status = ConnectionStatus.Faulted,
                    Message = response.Exception.Message,
                    Exception = response.Exception
                };
            }
            default:
            {
                return new AuthenticationResult
                {
                    Status = ConnectionStatus.Faulted
                };
            }
        }
    }

and another:

    public AuthenticationResult AuthenticateWithFacebook(string facebookId, string accessToken)
    {
        if (facebookId == null)
        {
            throw new ArgumentNullException("facebookId");
        }
        if (accessToken == null)
        {
            throw new ArgumentNullException("accessToken");
        }
        dynamic response;
        try
        {
            FacebookClient client = new FacebookClient(accessToken);
            response = client.Get("me");
        }
        catch (FacebookApiException exception)
        {
            _log.Info(Common.Resources.Error.FacebookApiException, exception);
            return new AuthenticationResult
            {
                Status = ConnectionStatus.Faulted,
                Message = exception.Message,
                Exception = exception
            };
        }
        if (response == null) // sanity
        {
            return new AuthenticationResult
            {
                Status = ConnectionStatus.Faulted,
                Message = Common.Resources.Error.FacebookApiException
            };
        }
        else if (response.error != null)
        {
            return new AuthenticationResult
            {
                Status = ConnectionStatus.Faulted,
                Message = response.error.message ?? Common.Resources.Error.FacebookApiException
            };
        }
        else if (response.id != facebookId) // validate access token against facebookId for enhanced security.
        {
            return new AuthenticationResult
            {
                Status = ConnectionStatus.InvalidCredentials,
                Message = Common.Resources.Authentication.InvalidCredentials
            };
        }
        else
        {
            User user = userRepository.GetByFacebookGraphId(facebookId);
            bool userIsNew = false;
            bool connectionIsNew = false;
            if (user == null)
            {
                connectionIsNew = true;

                string email = response.email;
                if (!email.NullOrEmpty()) // maybe they already have an account.
                {
                    user = userRepository.GetByEmail(email);
                }
                if (user == null) // create a brand new account.
                {
                    string displayName = response.name;
                    user = userRepository.CreateFromFacebook(facebookId, accessToken, email, displayName);
                    userIsNew = true;
                }
                else // just connect the existing account to their Facebook account.
                {
                    userRepository.AddFacebookConnection(user, facebookId, accessToken);
                }
            }
            return new AuthenticationResult
            {
                Status = ConnectionStatus.Authenticated,
                UserIsNew = userIsNew,
                ConnectionIsNew = connectionIsNew,
                DisplayName = user.DisplayName,
                UserId = user.UserId
            };
        }
    }

Just in case anyone wonders about dependencies from outside the method, here is the class:

public class AuthenticationService
{
    private static readonly ILog _log = LogManager.GetLogger(typeof(AuthenticationService));

    private readonly OpenIdRelyingParty relyingParty;
    private readonly IUserRepository userRepository;
    private readonly IMembershipProvider membershipProvider;

    public AuthenticationService(OpenIdRelyingParty relyingParty, IUserRepository userRepository, IMembershipProvider membershipProvider)
    {
        if (relyingParty == null)
        {
            throw new ArgumentNullException("relyingParty");
        }
        if (userRepository == null)
        {
            throw new ArgumentNullException("userRepository");
        }
        if (membershipProvider == null)
        {
            throw new ArgumentNullException("membershipProvider");
        }
        this.relyingParty = relyingParty;
        this.userRepository = userRepository;
        this.membershipProvider = membershipProvider;
    }

    [...]
}
\$\endgroup\$
2
  • \$\begingroup\$ Can you include the whole method that this code is in? There's much more to improve in this and we need to see the whole picture. \$\endgroup\$ Jul 8, 2012 at 23:29
  • \$\begingroup\$ sure, included both complete methods, thank you. I've also added the ctor so you have a basic idea of the injected dependencies too \$\endgroup\$
    – bevacqua
    Jul 8, 2012 at 23:31

2 Answers 2

5
\$\begingroup\$

I would first start by creating a method for the Authentication failed parts of the code. Something like

private AuthenticationResult AuthenticationFailed(AuthenticationStatus status, string message = "", Exception exception = null)
{
   return new AuthenticationResult
   {
       Status = status,
       Message = message,
       Exception = exception
   }
}

Then your first switch for the FB authentication ends up being something like:

    internal AuthenticationResult ProcessOpenIdResponse(IAuthenticationResponse response)
    {
        switch (response.Status)
        {
            case AuthenticationStatus.Authenticated:
            {
                // successfull authentication
            }
            case AuthenticationStatus.Canceled:
            {
                return AuthenticationFailed(ConnectionStatus.Canceled, Common.Resources.Authentication.CanceledAtProvider);                                
            }
            case AuthenticationStatus.Failed:
            {
                return AuthenticationFailed(ConnectionStatus.Faulted, response.Exception.Message, response.Exception);                
            }
            default:
            {
                return AuthenticationFailed(ConnectionStatus.Faulted, response.Exception.Message, response.Exception);                
            }
        }
    }

I would further refactored the getting of user email into it's own method as that seems to be common across both forms.

private User GetUserByEmail(string email)
{
    return email.NullOrEmpty() ?? null : userRepository.GetByEmail(email);
}

Although this might be overkill I would also consider separating the methods for doing the authentication into their own classes. I believe this may help with being able to do unit testing against each class in isolation as well as not separating the responsibilities out of whichever class is handling identifying which process to use etc. Something like this perhaps.

abstract class AuthenticationPortal
{   
    abstract AuthenticationResult Authenticate();

    protected virtual User GetUserByEmail(string emailAddress)
    {   
        return emailAddress.NullOrEmpty() ?? null : userRepository.GetByEmail(emailAddress);            
    }

    private AuthenticationResult AuthenticationFailed(AuthenticationStatus status, string message = "", Exception exception = null)
    {
       return new AuthenticationResult
       {
           Status = status,
           Message = message,
           Exception = exception
       }
    }
}

internal class OpenIdAuthenticationPortal : AuthenticationPortal
{
   private readonly IAuthenticationResponse authenticationResponse;
   private readonly UserRepository userRepository;

   public OpenIdAuthenticationPortal(IAuthenticationResponse response, UserRepository userRepository)
   {
      this.authenticationResponse = response;
      this.userRepository = userRepository;
   }

   public AuthenticationResult Authenticate()
   {
        switch (authenticationResponse.Status)
        {
            case AuthenticationStatus.Authenticated:
            {
                return Authenticated();
            }
            case AuthenticationStatus.Canceled:
            {
                return AuthenticationFailed(ConnectionStatus.Canceled, Common.Resources.Authentication.CanceledAtProvider);                                
            }
            case AuthenticationStatus.Failed:
            {
                return AuthenticationFailed(ConnectionStatus.Faulted, authenticationResponse.Exception.Message, authenticationResponse.Exception);                
            }
            default:
            {
                return AuthenticationFailed(ConnectionStatus.Faulted, authenticationResponse.Exception.Message, authenticationResponse.Exception);                
            }
        }
   }

   private AuthenticationResult Authenticated()
   {
        string openId = authenticationResponse.ClaimedIdentifier;
        User user = userRepository.GetByOpenId(openId);
        bool userIsNew = false;
        bool connectionIsNew = false;

        if (user == null)
        {
            connectionIsNew = true;

            string email = null;
            string displayName = null;
            var fetch = authenticationResponse.GetExtension<FetchResponse>();

            if (fetch != null)
            {
                email = fetch.GetAttributeValue(WellKnownAttributes.Contact.Email);
                displayName = fetch.GetAttributeValue(WellKnownAttributes.Name.FullName);

                // try to fetch by email first
                user = GetUserByEmail(userRepository, email);

                if (user == null) // create a brand new account.
                {
                    user = userRepository.CreateFromOpenId(openId, email, displayName);
                    userIsNew = true;
                }
                else // just connect the existing account to their OpenId account.
                {
                    userRepository.AddOpenIdConnection(user, openId);               
                }
            }
        }

        return new AuthenticationResult
        {
            Status = ConnectionStatus.Authenticated,
            UserIsNew = userIsNew,
            ConnectionIsNew = connectionIsNew,
            DisplayName = user.DisplayName,
            UserId = user.UserId
        };
    }
}

The Facebook client would follow similiar pattern.

\$\endgroup\$
2
  • \$\begingroup\$ I'm not looking at adding any other forms of authentication in the future, that's not a concern, but I'm a sucker for readability and coding cleanly. Currently the authentication options are twitter/fb (oauth), yahoo/gmail (openid), or straight up email/password \$\endgroup\$
    – bevacqua
    Jul 9, 2012 at 0:11
  • \$\begingroup\$ these was a pretty good idea, making helpers for the returned results significantly reduced code repetition .. \$\endgroup\$
    – bevacqua
    Jul 9, 2012 at 0:34
2
\$\begingroup\$
  1. Move common code into helper methods
  2. Fail early. Prefer this pattern

    if(something_is_wrong) { throw error }
    

    over

    if(every_is_ok) { ... }
    else { throw error }
    
  3. There is little point to use else when the if() before that always throws an error.

  4. When a method gets too long, split it into several helper methods with meaningful names. Prefer

    if(!parameter_x_is_valid(x)) { throw "x is invalid" }
    beginTransaction();
    writeToDatabase(x);
    endTransaction();
    

    over one 150 line method where you'll need comments to understand what is happening.

    That will also make it easier to test the code because short helper methods almost always need less setup.

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