Most of your variables should be static readonly
There is no reason to new up a brand new copy of the arrays of characters each time you run this function. Instead:
public static readonly ReadOnlyCollection<char> AllowedLower = ReadOnlyCollection<char>("abcdefghijklmnopqrstuvwxyz");
public static readonly ReadOnlyCollection<char> AllowedUpper = new ReadOnlyCollection<char>(AllowedLower.Select(x => char.ToUpper(x)));
public static readonly ReadOnlyCollection<char> AllowedDigits = new ReadOnlyCollection<char>("0123456789");
Note that for static, read only lists of things, we use ReadOnlyCollection to prevent modification at run time. There are some alternative implementations of read only collections in .NET depending on your version; any of them is also fine.
While we're at it, we can greatly improve readability by passing just one string, which implements IEnumerable<char>.
Randomness problems
You have two incredibly major problems with randomness:
- The first is that you create a new instance of
Random each time you invoke the function. That is wrong. You must reuse the same instance of a random number generator when generating many random numbers to ensure that it is sufficiently random.
- You're using
Random which doesn't meet the level of randomness required for security algorithms. You need this level of randomness for generating passwords. You should use RNGCryptoServiceProvider.
public static readonly RNGCryptoServiceProvider SecureRNG = new RNGCryptoServiceProvider();
Selecting random characters
Once the minimum requirements for each group of characters is met, you use a two stage random selection to choose the remaining characters. Because each group is not of the same length, this actually reduces the randomness. Any single digit has a higher probability of being selected than any single letter.
To resolve this, combine all of the characters into a single collection:
public static readonly ReadOnlyCollection<char> PasswordAlphabet = new ReadOnlyCollection<char>(AllowedLower.Concat(AllowedUpper).Concat(AllowedDigits));
And then select from that:
generatedPassword += PasswordAlphabet[randomIndex]
Use a more standard shuffle algorithm
Since we can't use Random and RNGCryptoServiceProvider doesn't provide a Next(), using OrderBy to shuffle the password becomes more difficult. We need to ensure that our shuffling doesn't have biases in the order, and we need to do so without using doubles.
Luckily, this is a solved problem. We'll use that solution.
Selecting a random character from the lists
You may have noticed that we skipped over how to generate randomIndex in the code above that selects a random character. That is because RNGCryptoServiceProvider does not readily implement a means of doing so. We'll need to implement the ability to randomly select from a range of values.
The typical way of generating a random integer in a range is to first calculate a random double and then multiply it by the range's length. We can do this with a couple extension methods that we'll implement later.
Prefer LINQ to loops
We don't need a for loop here. We can use LINQ:
generatedPassword += String.Join(
"",
Enumerable.Range(0, length - 3).Select(i => /* Get a random character */)
);
Don't silently replace inputs
While it's good that you restricted your method to require at least an 8 character password, it's not good to return something that the caller did not ask for. Instead of replacing the length, throw an error:
if (length < 8)
{
throw new ArgumentException(paramName: nameof(length), message: "must be at least 8");
}
Higher levels of code may choose to replace user input, but that kind of logic belongs there in the caller, not here in the depths of the implementation.
What about dependency injection?
Should any of this be written in the dependency injection style?
Firstly, I would say that the secure RNG instance should not be injected. A random number generator is one of the few elements of code where doing so is a bad idea. The reason is the very mistake you made in your code: you need a single, permanent instance of the RNG reused over and over. The secure version is notably thread safe, so it's fine to have just a single instance. In fact, I would go so far as to say that if other classes need an RNG, then it should be move out of this class and into a global static one if possible. (It's possible performance requirements may override this advice, but in general, the fewer instances of RNGs you have and the more they're shared, the better.)
Whether the method you're writing needs to be an instance that can be injected will depend on your requirements and the code base surrounding it. I don't see any particular harm in doing so, and I don't have enough context to know whether there's any benefit. The password generator I will present can be trivially converted into a stateless instance that could be injected, though.
Combining everything
First let's put our randomizing utilities in a separate class:
public static class SecureRandom
{
// We should only ever have one instance of this.
// Other classes can use this freely, or methods can be added here.
public static readonly RNGCryptoServiceProvider SecureRNG = new RNGCryptoServiceProvider();
// NextDouble and Next based on https://stackoverflow.com/a/16907577
public static double NextDouble(this RNGCryptoServiceProvider secureRNG)
{
var data = new byte[sizeof(uint)];
secureRNG.GetBytes(data);
var randUint = BitConverter.ToUInt32(data, 0);
return randUint / (uint.MaxValue + 1.0);
}
public static int Next(this RNGCryptoServiceProvider secureRNG, int minValue, int maxValue)
{
if (minValue > maxValue)
{
throw new ArgumentOutOfRangeException();
}
return minValue + (int)((maxValue - minValue) * secureRNG.NextDouble());
}
public static int Next(this RNGCryptoServiceProvider secureRNG, int maxValue)
{
return secureRNG.Next(0, maxValue);
}
private static T SelectElement<T>(this RNGCryptoServiceProvider secureRNG, IList<T> choices)
{
return choices[SecureRNG.Next(choices.Count)];
}
// Based on https://stackoverflow.com/a/1262619
private static void ToRandomOrder<T>(this RNGCryptoServiceProvider secureRNG, IEnumerable<T> sequence)
{
var workingList = new List<T>(sequence);
int n = workingList.Count;
while (n > 1)
{
byte[] box = new byte[1];
do SecureRNG.GetBytes(box);
while (!(box[0] < n * (Byte.MaxValue / n)));
int k = (box[0] % n);
n--;
T value = workingList[k];
workingList[k] = workingList[n];
workingList[n] = value;
}
return workingList;
}
}
With our extension methods above, now we can write the full method:
using static SecureRandom;
public static class SimplePasswordGenerator
{
public static readonly ReadOnlyCollection<char> AllowedLower = ReadOnlyCollection<char>("abcdefghijklmnopqrstuvwxyz");
public static readonly ReadOnlyCollection<char> AllowedUpper = new ReadOnlyCollection<char>(AllowedLower.Select(x => char.ToUpper(x)));
public static readonly ReadOnlyCollection<char> AllowedDigits = new ReadOnlyCollection<char>("0123456789");
public static readonly ReadOnlyCollection<char> PasswordAlphabet = new ReadOnlyCollection<char>(
AllowedLower
.Concat(AllowedUpper)
.Concat(AllowedDigits)
);
public static string GenerateSimplePassword(int length = 8)
{
if (length < 8)
{
throw new ArgumentException(paramName: nameof(length), message: "must be at least 8");
}
IEnumerable<char> passwordCharacters = new [] {
// At least one from each list
SecureRNG.SelectElement(AllowedLower),
SecureRNG.SelectElement(AllowedUpper),
SecureRNG.SelectElement(AllowedDigits)
}
passwordCharacters = passwordCharacters.Concat(
Enumerable.Range(0, length - 3)
.Select(i => SecureRNG.SelectElement(PasswordAlphabet))
);
return String.Join("", SecureRNG.ToRandomOrder(passwordCharacters));
}
