So I have a method that, after validation, generates a random password based on the length and characters provided. The UI uses check boxes to select using between lowercase and/or uppercase letters, numbers and special characters to populate the characters parameter. as well as the length of the password and the spacing between each character used (I.E. if character spacing is set to '10', the letter 'A', for example, can only be used again after another 10 unique characters are used first):

The Generator Method:

    private string PasswordGeneration(string characters, int length)
{
var random = Factory.Random(); //Generates new Random() from Factory class.
var availableCharacters = characters; //Preps the available characters with all characters passed.
var output = string.Empty;
var rule = ValidateRule(characters.Length); //Sets character rule to length of parameter "characters" if the unique character spacing is set higher than the number of available characters to use.
for (var position = 0; position < length; position++) //loop until we've met the password's desired length.
{
if (rule != 0 && (characters.Length - availableCharacters.Length == rule)) //if a character rule is set & if we've met the criteria of the number of unique characters asked for...
{
availableCharacters = characters.Where(c => !output.Substring((position - rule) + 1, rule - 1).Contains(c)).Aggregate(availableCharacters, (current, c) => current + (current.Contains(c) ? string.Empty : c.ToString()));
//...we add back all characters for usage other than the last n characters used dictated by the unique character rule.
}
var character = availableCharacters[random.Next(availableCharacters.Length)]; //Grabs a random character from availableCharacters.
availableCharacters = rule == 0 ? availableCharacters : availableCharacters.Replace(character.ToString(), string.Empty); //If a character rule is set, we remove the currently used character from availableCharacters.
output += character; //Append "character" to output.
}
return output;
}


The 'ValidateRule()' Method:

        private int ValidateRule(int length) =>
length - _settings.IdenticalSpacing > 0
? _settings.IdenticalSpacing
: length;


And for readability with what's happening with the LINQ expression within the if statement:

foreach(var c in characters)//...we iterate through the original set of characters...
{
if(output.Substring((position - rule) + 1, rule -1).Contains(c) == false)//...and check to see if the a specific range contains each character. If the range doesn't contain the current character...
{
foreach(var availableCharacter in availableCharacters)//...we iterate through our available characters to see if it doesn't already contains the current character...
{
if(!Equals(availableCharacter, c))
{
availableCharacters += c; //...if not, we make the character available for use again.
}
}
}
}


As stated before, the characters and password length parameters are all validated before being passed to PasswordGeneration(), so I didn't add any null checking or checks for bad inputs other than for the character spacing rule. While everything works perfectly fine with no errors, I feel as if the code looks messy and of course a bit hard to read. I can't help but think there's a much more simple approach without a bunch of loops and conditions or long LINQ expressions. However, I would love to hear some feedback on what you all think and of course, how I can improve this.

var random = Factory.Random(); //Generates new Random() from Factory class.


It's obvious if you read the code.

It's ok to break those lines

availableCharacters = characters
.Where(c => !output.Substring((position - rule) + 1, rule - 1).Contains(c))
.Aggregate(availableCharacters,
(current, c) => (current.Contains(c)
? current
: current + c.ToString()));


Makes it a bit more readable. But still all these +1 -1 does not look very bug safe.

Here's my approach:

private string PasswordGeneration(string characters, int length, int characterSpacing)
{
var random = new Random();
var randomList = new List<int>();
var spacing = Math.Min(characterSpacing, characters.Length);
// Generate indexes
while(randomList.Count < length) {
var num = random.Next(0, characters.Length);
var numNotInUse = randomList.LastIndexOf(num) == -1;
var spacingOk = randomList.LastIndexOf(num) < (randomList.Count - spacing);
if (numNotInUse || spacingOk ) {
}
}
}


Simple is usually better. At least more readable :)

• It's funny, I was actually considering using an index based list before posting here, but got so hung up on the if's being a necessity, that I just couldn't grasp the concept. Very nice execution, simple and clean! One thing I should point out though that while the Math.Min() works, it will hang if the character spacing and password length are set higher than the number available characters to use. However, that shows that it's another input to be validated prior to usage. – Chris P. Bacon Feb 20 at 0:35
• Yep characters.Length - 1 works better. But it ought to throw an exception as it wont work as predicted. I'll leave that as an exercise ;) – tallberg Feb 20 at 7:07

There are lots of ways to improve this hard-to-read code but all of them are unimportant compared to the fact that this is massively insecure against common attacks. You are implementing a component of a security system; you need to build a threat model and understand what you are doing. Implementing a security system is not a beginner task!

An important rule to follow is never use System.Random for a security purpose.

A random number generator needs to be genuinely unpredictable for it to be used in a security system that depends on unpredictability. System.Random is not truly random, and nor is it crypto strength pseudo random.

Why does this matter? Because an attacker who knows anything about the system used to generate the passwords can make good guesses about what passwords it will generate, much better than chance. How does the attacker get information about the system? A generated password leaks information about the state of the system.

That is, given a collection of passwords generated by this algorithm allows an attacker to determine the internal state of System.Random and then from that they can make good guesses as to all past and future passwords that will be generated. That's bad! The whole point of a randomly-generated password is to make it hard to guess.