Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have finished my first project using ASP.NET MVC. I did not have to wait long to realise I have essentially violated all possible codes of good programming. Well, experience comes with practice.

One of the styles I completely missed was Dependency Injection Principle. It was far too late I could re-write the project and apply it.

DI took me some time to understand. Based on this very simple project, I would like experience programmers to give me some reviews, whether I have sorted the DI correctly.

This is a very simple task. I want to generate hashed texts based on password provided by users. I know I could sort this in couple of lines but I wanted to do something I could apply DI.

I split the whole solution into two projects. The first project contains hashing algorithms, the second the main function to generate hashed text. I tried to lose all possible couplings based everything on interfaces.

So, GeneratePassword.computetHashedPassword() uses injected object of HashingAlgorithm to make GeneratePassword independent to it. On the other hand, HashingAlgorithm using one of the System interfaces, System.Security.Cryptography.HashAlgorithm. This way I tried to make hashing algorithm also independent.

class Program
{
    static void Main(string[] args)
    {
        IGeneratePassword gp;
        string password = "propiotr";

        Console.WriteLine("By bew class:");
        gp = new GeneratePassword(new XAlgorithm(new SHA256Cng()));
        Console.WriteLine(string.Format("{0,19}: {1}", "HashedPassword", gp.GetHashedPassword(password)));
        Console.WriteLine(string.Format("{0,19}: {1}", "Password", gp.Password));
        Console.WriteLine(string.Format("{0,19}: {1}", "Password Salt", gp.PasswordSalt));
        Console.WriteLine();
    }
}

Hashing Algorithms

public interface IHashAlgorithm
{
    string GetHashedText(string text);
}

public class XAlgorithm : IHashAlgorithm
{
    private System.Security.Cryptography.HashAlgorithm sysHashAlgorithm;

    public XAlgorithm(System.Security.Cryptography.HashAlgorithm hashAlgorithm)
    {
        this.sysHashAlgorithm = hashAlgorithm;
    }

    public string GetHashedText(string text)
    {
        string hashedtext = getHashX(text);
        for (int i = 0; i < 255; i++)
            hashedtext = getHashX(hashedtext);
        return hashedtext;
    }

    private string getHashX(string text)
    {
        byte[] bytes = Encoding.Unicode.GetBytes(text);
        byte[] hash = sysHashAlgorithm.ComputeHash(bytes);
        string hashString = string.Empty;
        foreach (byte x in hash)
        {
            hashString += String.Format("{0:x2}", x);
        }
        return hashString;
    }
}

public class MD5Algorithm : IHashAlgorithm
{
    public string GetHashedText(string text)
    {
        XAlgorithm ha = new XAlgorithm(new MD5Cng());
        return ha.GetHashedText(text);
    }
}

public class Sha256Algorithm : IHashAlgorithm
{
    public string GetHashedText(string text)
    {
        XAlgorithm ha = new XAlgorithm(new SHA256Cng());
        return ha.GetHashedText(text);
    }
}    

PasswordEncryption

public interface IGeneratePassword
{
    string Password { get; set; }
    string HashedPassword { get; }
    string PasswordSalt { get; }
    string GetHashedPassword(string password);
}

public class GeneratePassword : IGeneratePassword
{
    private IHashAlgorithm hashAlgorithm;
    private string password;
    private string passwordSalt;
    private string hashedPassword;

    public string Password
    {
        get { return password; }
        set { this.password = value; }
    }
    public string HashedPassword
    {
        get
        {
            computetHashedPassword();
            return hashedPassword;
        }
    }
    public string PasswordSalt
    {
        get { return passwordSalt; }
    }

    public GeneratePassword(IHashAlgorithm hashAlgorithm)
    {
        this.hashAlgorithm = hashAlgorithm;
        this.passwordSalt = getRandomString(25);
    }

    public string GetHashedPassword(string password)
    {
        this.password = password;
        return HashedPassword;
    }

    private void computetHashedPassword()
    {
        if (string.IsNullOrEmpty(this.password))
            throw new ArgumentNullException("Cannot compute a hashed password for the empty property 'Password'");
        this.hashedPassword = hashAlgorithm.GetHashedText(this.password + this.passwordSalt);
    }

    private static string getRandomString(int number)
    {
        string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
        Random random = new Random();
        return new string(Enumerable.Repeat(chars, number)
                                    .Select(s => s[random.Next(s.Length)])
                                    .ToArray());
    }
}
share|improve this question

1 Answer 1

You've kind of missed the point in dependency injection a bit but you're nearly there.

public class MD5Algorithm : IHashAlgorithm
{
    public string GetHashedText(string text)
    {
        XAlgorithm ha = new XAlgorithm(new MD5Cng());
        return ha.GetHashedText(text);
    }
}

Look at that there - you're newing up an MD5Cng and an XAlgorithm - neither are being injected! You've overcomplicated things a bit here I don't see the point in having a GeneratePassword class. XAlgorithm looks a bit like an adaptor but again, I think it's all a bit overcomplicated.

Basically, you want to move everything that your class depends on into a constructor argument (which is the easiest form of dependency injection to use). Need a Random? Pass it into the class's constructor! The point of DI is to have all your dependent services easily discoverable.

Other notes

All methods should be PascalCase. getRandomString should be GetRandomString

Random isn't particularly random at all - for this sort of thing you'd be better off with System.Security.Cryptography.RandomNumberGenerator.

Autoproperties reduce the amount of code you have to write:

public string Password
{
    get { return password; }
    set { this.password = value; }
}

is exactly equal to

public string Password { get; set; }

Neither MD5 nor SHA256 should be used for password hashing. Use a proper algorithm. Especially not MD5.

share|improve this answer
    
To expand a proper algorithm would be bcrypt or pbkdf2. – WindRaven Sep 10 at 17:59
    
Thanks RobH for pointing out some issues here. I definitely missed Random in terms of DI. I also agree it is overcomplicated, I have mentioned that, but it still serves to me as a practice or an exercise. Also, thanks guys for pointing at right algorithm :) I've already found sopm usefull texts to read on CrackStation: Salted Password Hashing - Doing it Right. Cheers! – Celdor Sep 11 at 7:06
    
@RobH Excuse my ignorance please, but what's wrong with a concrete MD5Algorithm implementation of IHashAlgorithm instantiating an instance of MD5Cng? Surely this is the place to use MD5Cng ? – Neil Moss Sep 16 at 20:54

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.