WCF Duplex service authentication

Question

I have been thinking about a way to implement this and I am not sure that what I have done is correct, because it surely sounds kinda dirty to me.

Basically what I have is a WCF duplex service which multiple clients subscribe to. My problem is the authentication. What I have done is I pass the username and password to the Subscribe method, then check those against the database and add the client's subscription to a Dictionary<UserAccount,ICallback>. Then(just for testing purposes so far) I have a timer which calls a method on each callback every 1 second.

All of this is working fine, with the exception that when I disconnect and connect the same client, the service crashes which I believe is a pretty trivial problem but I just haven't had much time to look into it. I am not sure this is the best way to handle authentication though. I do know about authentication using a custom UserNameValidator and a certificate with that, but last time I tried to do it, it took me a really long time and still couldn't get the client right for some reason, even though the service was working fine with a WCF service testing software(can't remember the name right now, but it was pretty decent IMO). Also I really don't want to have to deploy a certificate to every single client and purchasing a certificate is a big NO. The service is supposedly going to be working in LAN most of the time.

So here is the code:

[ServiceBehavior(InstanceContextMode=InstanceContextMode.Single)]
    public class SchoolTestMakerService : ISchoolTestMakerService
    {
        Dictionary<UserAccount, ISchoolTestMakerCallbackContract> clients =
            new Dictionary<UserAccount, ISchoolTestMakerCallbackContract>();
        IHashGenerator hashGenerator = new SHA1Generator();
        UnitOfWork unitOfWork;
        Timer timer;
        int statusCode;
        public SchoolTestMakerService()
        {
            SchoolTestMakerDbInitializer dbInitializer = new SchoolTestMakerDbInitializer(hashGenerator);
            SchoolTestMakerDbContext context = new SchoolTestMakerDbContext(dbInitializer);
            unitOfWork = new UnitOfWork(context);

            timer = new Timer((s) => ChangeStatus("Status: " + statusCode++),null,0,1000);
        }
        public void Subscribe(string userName,string password)
        {
            string passwordHash = hashGenerator.GenerateHash(password);
            UserAccount userAccount = unitOfWork.Repository<UserAccount>().Get(x => x.Username == userName&&x.PasswordHash==passwordHash);
            if(!clients.ContainsKey(userAccount))
            {
                ISchoolTestMakerCallbackContract callback = OperationContext.Current.GetCallbackChannel<ISchoolTestMakerCallbackContract>();
                clients.Add(userAccount, callback);
            }
        }

        public void Unsubscribe()
        {
            ISchoolTestMakerCallbackContract callback = OperationContext.Current.GetCallbackChannel<ISchoolTestMakerCallbackContract>();
            clients.Remove(clients.First(x => x.Value == callback).Key);
        }

        private void ChangeStatus(string newStatus)
        {
            foreach(var client in clients)
            {
                client.Value.StatusChanged(newStatus);
            }
        }
    }


[ServiceContract(SessionMode=SessionMode.Required,
        CallbackContract=typeof(ISchoolTestMakerCallbackContract))]
    public interface ISchoolTestMakerService
    {
        [OperationContract(IsOneWay=false,IsInitiating=true)]
        void Subscribe(string userName,string password);

        [OperationContract(IsOneWay = false, IsTerminating = true)]
        void Unsubscribe();
    }


public interface ISchoolTestMakerCallbackContract
    {
        [OperationContract(IsOneWay = true)]
        void StatusChanged(string newStatus);
    }

Client code:

using SchoolTestMaker.DataAccess;
using SchoolTestMaker.DataAccess.Utilities;
using SchoolTestMaker.DataAccess.Utilities.Interfaces;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Data.Entity;
using TestClient.SchoolTestMakerServiceProxy;

namespace TestClient
{
    class Program
    {
        static void Main(string[] args)
        {
            string userNameString = "UserName: ";
            string passwordString = "Password: ";
            Console.WriteLine(userNameString);
            Console.WriteLine(passwordString);
            Console.SetCursorPosition(userNameString.Length, 0);
            string userName = Console.ReadLine();
            Console.SetCursorPosition(passwordString.Length, 1);
            string password = Console.ReadLine();
            SchoolTestMakerCallback callback = new SchoolTestMakerCallback();
            SchoolTestMakerServiceClient client = new SchoolTestMakerServiceClient(new System.ServiceModel.InstanceContext(callback));
            client.Open();
            client.Subscribe("Pesho Daskala", "parola123");
            Console.Read();
            client.Unsubscribe();
        }
    }
}

public class SchoolTestMakerCallback:ISchoolTestMakerServiceCallback
    {
        public void StatusChanged(string newStatus)
        {
            Console.WriteLine(newStatus);
        }
    }

The client is basically a dead simple console application(just for testing purposes of course... haven't written the real client yet). The user account is also just a dummy for testing.

SHA1Generator and IHashGenerator:

public class SHA1Generator:IHashGenerator
    {
        public string GenerateHash(string input)
        {
            using (SHA1Managed sha1 = new SHA1Managed())
            {
                var hash = sha1.ComputeHash(Encoding.UTF8.GetBytes(input));
                var sb = new StringBuilder(hash.Length * 2);

                foreach (byte b in hash)
                {
                    sb.Append(b.ToString("X2"));
                }

                return sb.ToString();
            }
        }
    }

public interface IHashGenerator
    {
        string GenerateHash(string input);
    }

By the way, ignore the Session nonsense in the Service code. I forgot to remove it after switching to a single instance.(I am pretty sure it is pointless in that case and does literally nothing).

I'd really appreciate some help. I am pretty new to WCF but not to C# as a whole. I am sorry for the long question. I have tried to provide as much information as possible. If anything is missing please give me a chance to fix it before downvoting :)

Answer 1

Problems

• in Subscribe() you check, without testing for null, if this UserAccount is contained in the dictionary.
• in Unsubscribe() you use the callback to get the Key to call Remove() without checking the success of the call.

Naming and style

Your naming is good. You are using meaningful names and also use the correct casing.

The declaration part of your class variables of SchoolTestMakerService looks kind of ugly (sorry). You should consider to initialize clients inside the constructor.

General

I think the unsubscribe part is problematic, because you are using the callback which is the Value of a KeyValuePairto retrieve the Key.

I would add a class to the service having two properties, the useraccount and the callback like

class UserCallBack
{
    public UserAccount User { get; set; }
    public ISchoolTestMakerCallbackContract CallBack { get; set; }
}  

which could be extended e.g by adding properties like LastActive, TimeOut and a IsValid() method.

Inside the Subscribe() method I would create a hash using the username and the password hash, which I would use as the key and the UserCallBack as the value of the dictionary.

This hash should be returned by the method and for each call of a service method be passed as parameter.

Dictionary<String, UserCallBack> clients = new Dictionary<String, UserCallBack>();

public string Subscribe(string userName,string password)
{
    string passwordHash = hashGenerator.GenerateHash(password);
    UserAccount userAccount = unitOfWork.Repository<UserAccount>().Get(x => x.Username == userName && x.PasswordHash == passwordHash);

    if (userAccount == null) { return String.Empty; }

    string key = CreateKeyHash(userName, passwordHash);
    if (!clients.ContainsKey(key))
    {
         ISchoolTestMakerCallbackContract callback = OperationContext.Current.GetCallbackChannel<ISchoolTestMakerCallbackContract>();
         UserCallBack userCallBack = new UserCallBack() { userAccount, callback };
         clients.Add(key, userCallBack );
    }
    return key;
}

private String CreateKeyHash(string userName, string passwordHash)
{
    return hashGenerator.GenerateHash(userName + passwordHash);
}

public void Unsubscribe(string userKey)
{
    if (userKey == null) { return; }

    clients.Remove(userKey);
}