WCF Duplex service authentication

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. 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 clients =
new Dictionary();
IHashGenerator hashGenerator = new SHA1Generator();
UnitOfWork unitOfWork;
Timer timer;
int statusCode;
public SchoolTestMakerService()
{
SchoolTestMakerDbInitializer dbInitializer = new SchoolTestMakerDbInitializer(hashGenerator);
SchoolTestMakerDbContext context = new SchoolTestMakerDbContext(dbInitializer);
unit

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 clients = new Dictionary();

public string Subscribe(string userName,string password)
{
    string passwordHash = hashGenerator.GenerateHash(password);
    UserAccount userAccount = unitOfWork.Repository().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();
         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);
}

StackExchange Code Review Q#73836, answer score: 2