8
\$\begingroup\$

I am new to C# and develop a configurable PasswordService class. As the code works the next step is to improve the code.

Right now there are the following options:

  • Set alpha characters and the number of alpha characters that has to be in the password.
  • Set numeric and the number of numeric characters that has to be in the password.
  • Set non alphanumeric and the number of non alphanumeric characters that has to be in the password.

My questions:

  • Is the structure fine as it is?

  • Are the thrown Exceptions and it's message well chosen?

  • What do you think about the generate function yet?

  • Would you add or adjust the options?

  • Are the comments c#/xml valid?

  • How can I improve the password algorithm?

  • Did I miss something else?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace PasswordGenerator
{
namespace Utility
{
namespace Service
{
    /// <summary>
    /// This is a configurable password generator.
    /// </summary>
    class PasswordService
    {
        string sAlphaCharacters;
        string sNumericCharacters;
        string sNonAlphanumericCharacters;
        uint uiNumberOfAlphaCharacters;
        uint uiNumberOfNumericCharacters;
        uint uiNumberOfNonAlphanumericCharacters;

        /// <summary>
        /// Initializes class with common configuration.
        /// </summary>
        public PasswordService()
        {
            this.SAlphaCharacters = "abcdefghijklmnopqrstuwvxyABCDEFGHIJKLMNOPQRSTUWVXYYZ";
            this.SNumericCharacters = "0123456789";
            this.SNonAlphanumericCharacters = "!$%&()[]{}=?#";
            this.UiNumberOfAlphaCharacters = 6;
            this.UiNumberOfNumericCharacters = 6;
            this.UiNumberOfNonAlphanumericCharacters = 3;
        }

        // Accessor & modifier
        public string SAlphaCharacters
        {
            get
            {
                return sAlphaCharacters;
            }

            set
            {
                System.Text.RegularExpressions.Regex oRegex = new System.Text.RegularExpressions.Regex("[^a-zA-Z]{1,}");
                if (oRegex.IsMatch(value))
                {
                    throw new ArgumentException("Value must not contain anything but alpha characters.", "SAlphaCharacters.set");
                }
                sAlphaCharacters = value;
            }
        }

        public string SNumericCharacters
        {
            get
            {
                return sNumericCharacters;
            }

            set
            {
                System.Text.RegularExpressions.Regex oRegex = new System.Text.RegularExpressions.Regex("[^0-9]{1,}");
                if (oRegex.IsMatch(value))
                {
                    throw new ArgumentException("Value must not contain anything but numeric characters.", "SNumericCharacters.set");
                }
                sNumericCharacters = value;
            }
        }

        public string SNonAlphanumericCharacters
        {
            get
            {
                return sNonAlphanumericCharacters;
            }

            set
            {
                System.Text.RegularExpressions.Regex oRegex = new System.Text.RegularExpressions.Regex("[a-zA-Z0-9]{1,}");
                if (oRegex.IsMatch(value))
                {
                    throw new ArgumentException("Value must not contain alphanumeric characters.", "SNonAlphanumericCharacters.set");
                }
                sNonAlphanumericCharacters = value;
            }
        }

        public uint UiNumberOfAlphaCharacters
        {
            get
            {
                return uiNumberOfAlphaCharacters;
            }

            set
            {
                uiNumberOfAlphaCharacters = value;
            }
        }

        public uint UiNumberOfNumericCharacters
        {
            get
            {
                return uiNumberOfNumericCharacters;
            }

            set
            {
                uiNumberOfNumericCharacters = value;
            }
        }

        public uint UiNumberOfNonAlphanumericCharacters
        {
            get
            {
                return uiNumberOfNonAlphanumericCharacters;
            }

            set
            {
                uiNumberOfNonAlphanumericCharacters = value;
            }
        }

        /// <summary>
        /// Chooses randomly characters of given group and returns them.
        /// </summary>
        /// <param name="sCharacterGroup">From this character group the chararacters will be choosen.</param>
        /// <param name="uiNumberOfThisGroup">The amount of characters to choose randomly.</param>
        /// <returns>One random character of the character group</returns>
        public string getRandomSubstrOfCharacterGroup(string sCharacterGroup, uint uiNumberOfThisGroup)
        {
            Random rnd = new Random();
            int iPosition = 0;
            string sCharacters = "";

            for (uint ui = 0; ui < uiNumberOfThisGroup; ui++)
            {
                iPosition = iPosition = rnd.Next(0, sCharacterGroup.Length);
                sCharacters += sCharacterGroup.ToCharArray()[iPosition].ToString();
            }

            return sCharacters;
        }

        /// <summary>
        /// Based upon the config a password will be generated and returned.
        /// </summary>
        /// <exception cref="InvalidOperationException">
        /// Thrown when the number of each character group is 0.
        /// </exception>
        /// <returns>
        /// Returns generated password.
        /// </returns>
        public string generate()
        {
            if (this.UiNumberOfAlphaCharacters == 0 && this.UiNumberOfNumericCharacters == 0 && this.UiNumberOfNonAlphanumericCharacters == 0)
            {
                throw new InvalidOperationException("The number of each character group is 0.");
            }

            Random oRand = new Random();
            string sPassword = "";

            sPassword += this.getRandomSubstrOfCharacterGroup(this.SAlphaCharacters, this.uiNumberOfAlphaCharacters);
            sPassword += this.getRandomSubstrOfCharacterGroup(this.SNumericCharacters, this.UiNumberOfNumericCharacters);
            sPassword += this.getRandomSubstrOfCharacterGroup(this.SNonAlphanumericCharacters, this.UiNumberOfNonAlphanumericCharacters);

            sPassword = new string(sPassword.ToCharArray().OrderBy(s => (oRand.Next(2) % 2) == 0).ToArray());

            return sPassword;
        }

    }

}
}
}
\$\endgroup\$
  • \$\begingroup\$ Too bad the comments were removed. Or were they? I see some comments... note that commenting is also something that's reviewable ;-) \$\endgroup\$ – Mathieu Guindon May 24 '15 at 18:22
  • \$\begingroup\$ Oh, yeah that's true but the c# commenting is another independent learning task. Now I want to focus the code itself :) \$\endgroup\$ – AMartinNo1 May 24 '15 at 18:34
  • \$\begingroup\$ I've thought about your words and decided to share comments as well. Next to the comments I have made some changes as well. \$\endgroup\$ – AMartinNo1 May 24 '15 at 21:23
  • 1
    \$\begingroup\$ Don't update your code to incorporate changes. \$\endgroup\$ – CodesInChaos May 25 '15 at 11:12
  • 2
    \$\begingroup\$ That is a very non-standard choice for C#. You'll note for instance that the Length property of a string is an int, despite it never being negative. Use signed integers for everything in C#; use uint only for interoperating with code written in other languages that uses unsigned integers. \$\endgroup\$ – Eric Lippert May 25 '15 at 14:57
11
\$\begingroup\$

I covered the security problem in another answer; in this answer let's look at more subtle problems.

Someone already mentioned that the nested namespaces are unnecessarily verbose, but even with the more compact syntax, things are still not great. We have a class PasswordGenerator.Utility.Service.PasswordService. What is the function of each portion of the namespace? Namespaces are used primarily to organize large bodies of code, and secondarily to mitigate the danger of collisions between your password service and another.

So, first, when I see your namespace there with its three levels I assume that there must be hundreds or thousands of classes that you are organizing, that there is a PasswordGenerator.Utility.Foo and PasswordGenerator.Utility.Bar and PasswordGenerator.Blah.Whatever as well -- in short, that three levels are necessary to organize the mass of code you have in there.

The standard way to structure a namespace is Organization.FunctionalArea.SubArea -- so, Microsoft.CodeAnalysis.CSharp, for example.

The namespace you've come up with seems there only to hold a single class which does exactly what the namespace name says it does. I would be inclined to have a single namespace PasswordUtilities and be done with it.

The name PasswordService is vague. What kind of service? Generation? Verification? Entropy measuring? The class relies upon its namespace to make it clear that it is a generator. That's completely backwards. The namespace should be PasswordUtilities and the class should be Generator.

Others have already commented on your use of System Hungarian, which Joel and I both noted in 2003 is the bad Hungarian. Neither form of Hungarian should be used in C#.

Do not use unsigned integers in C# even for quantities that are logically non-negative. String.Length is signed. Follow its example.

Your test for whether a character is alpha is deeply prejudiced towards American usage. If a Greek person wishes their password to be Κωδικός πρόσβασης shouldn't they be allowed to do so? If you want to check for alpha-ness, use the methods on char that do so.

Follow C# naming conventions

 public string getRandomSubstrOfCharacterGroup

Others have noted that methods begin with a capital letter in C#.

Worse: mthds shd nt cntn abbrvs, u r totes nt a 13yo grl wrtng nts n clss.

Why is this method public? It seems to be an implementation detail.

The methods name is incorrect. It does not return a random substring. "CAB" is not a substring of "ABCDEFG". "CAB" is a substring of "TAXICAB" and "CABERNET".

sCharacters += sCharacterGroup.ToCharArray()[iPosition].ToString();

The ToString is unnecessary. C# automatically calls ToString on your behalf when appending to a string.

Others have suggested that StringBuilder would be the better choice here. If the string were likely to be hundreds or thousands of characters, then yes, always use a builder. But for this little thing that is only six or ten characters, no need to worry about it.

        sPassword = new string(sPassword.ToCharArray().OrderBy(s => (oRand.Next(2) % 2) == 0).ToArray());

I cannot for the life of me figure out what this thing is trying to do. It looks like you're trying to shuffle the string, but why are you shuffling it by flipping a coin (Next(2)) then making sure it was either heads or tails (the remainder), then turning the 0 or 1 into true or false, and then sorting the string by "heads first"? This is a bizarre way to shuffle a string. Can you explain what your logic was here? Maybe there is some good reason for this, and if there is, it should be very clearly explained in the comments.

If I wanted to shuffle a string like this then I'd say

        password = new string(password.ToCharArray().OrderBy(s => random.NextDouble()).ToArray());

That is "generate a random number between 0 and 1 as the sort key".

 System.Text.RegularExpressions.Regex oRegex = new System.Text.RegularExpressions.Regex("[a-zA-Z0-9]{1,}");

Notice how completely uninteresting information dominates this statement. Use a using System.Text.RegularExpressions; directive to bring Regex into scope, and then use var to simplify the declaration:

 var regex = new Regex("[a-zA-Z0-9]{1,}");
\$\endgroup\$
13
\$\begingroup\$

You have asked a lot of questions and omitted the most important question. Is the security system I have developed secure against attacks?

Absolutely it is not. There is an enormous security problem here. Random is pseudo-random, not crypto-strength random, and that means that it is easy for an attacker to make good guesses about the output of this algorithm. In particular if the attacker has a password generated by this algorithm then they have a high likelihood of being able to determine some or all future passwords.

Oh, but wait, it gets worse.

What happens if you try to generate two passwords within the same millisecond? You get the same password both times. You are not re-using the Random instance, which means that it picks a random seed based on the current time. Which means that if I am trying to attack the system I can try to generate a password at the same time as another user, and then I have their password.

If this is an exercise, well, you learned something today: always use crypto strength randomness in security systems. If this is not an exercise then you need to hire a consultant who specializes in security systems. Do not roll your own security system; you will do it wrong.

\$\endgroup\$
  • \$\begingroup\$ Luckily it is an excercise:-) Thanks, I will dig into it and updated the entry post. \$\endgroup\$ – AMartinNo1 May 25 '15 at 7:24
  • \$\begingroup\$ Is the Wikipedia article about crypto randomness fine (including it's links) or do you recommend a different website? \$\endgroup\$ – AMartinNo1 May 25 '15 at 10:51
  • \$\begingroup\$ @AMartinNo1: It is a good place to start, sure. \$\endgroup\$ – Eric Lippert May 25 '15 at 14:55
  • \$\begingroup\$ @AMartinNo1: See also RNGCryptoServiceProvider \$\endgroup\$ – Brian May 26 '15 at 13:00
9
\$\begingroup\$

Everything being initialized in your constructor seems to be a constant.

    public PasswordService()
    {
        this.SAlphaCharacters = "abcdefghijklmnopqrstuwvxyABCDEFGHIJKLMNOPQRSTUWVXYYZ";
        this.SNumericCharacters = "0123456789";
        this.SNonAlphanumericCharacters = "!$%&()[]{}=?#";
        this.UiNumberOfAlphaCharacters = 6;
        this.UiNumberOfNumericCharacters = 6;
        this.UiNumberOfNonAlphanumericCharacters = 3;
    }

So, you should probably declare them as actual const constants. This removes the possibility that you'll mistakenly assign any of them a new value.

Except...

I would probably inject values into the constructor for all of the "number of characters" fields.

class PasswordService
{
    const string sAlphaCharacters = "abcdefghijklmnopqrstuwvxyABCDEFGHIJKLMNOPQRSTUWVXYYZ";
    const string sNumericCharacters = "0123456789";
    const string sNonAlphanumericCharacters = "!$%&()[]{}=?#";
    uint uiNumberOfAlphaCharacters;
    uint uiNumberOfNumericCharacters;
    uint uiNumberOfNonAlphanumericCharacters;

    /// <summary>
    /// Initializes class with common configuration.
    /// </summary>
    public PasswordService(uint numOfAlpha = 6, uint numOfNumeric =6, uint numOfSpecial = 3)
    {

   this.UiNumberOfAlphaCharacters = numOfAlpha;
        this.UiNumberOfNumericCharacters = numOfNumeric;
        this.UiNumberOfNonAlphanumericCharacters = numOfSpecial;
    }

Allowing these values to be specified makes the class much more reusable and specifying default values makes it easy to leave any current client code unmodified.

Which brings me to the fact that I really don't like that client code can arbitrarily mutate these values through the property setters. I really believe these should be immutable. Once the class is instantiated, nothing should be able to change the number of characters that are required. It's the kind of thing that you want one and only one chance to set. If you have a good reason to expose these through a Getter, ok, but I'm doubtful that even that is necessary or useful. I would go so far as to argue that allowing client code to have access to these values breaks encapsulation and fails to hide information that should be hidden.

Once you've removed the setters, go ahead and declare the private fields readonly so the values can't be changed internally as well.

\$\endgroup\$
  • \$\begingroup\$ Nothing about the Hungarian naming? ;-) \$\endgroup\$ – Mathieu Guindon May 25 '15 at 1:52
  • \$\begingroup\$ Ran out of time @Mat'sMug Wanted to hit the structural issues. \$\endgroup\$ – RubberDuck May 25 '15 at 2:05
  • \$\begingroup\$ First I want to thank you for your response. Basically I agree to you - alpha & numeric chars will be changed propably never but special characters may change during runtime. The same goes for the numberOf* vars. Assuming I have a form for a generating a password and the user want to extends the special character group. How do I handle that? \$\endgroup\$ – AMartinNo1 May 25 '15 at 2:34
  • \$\begingroup\$ You only create the instance once you know what values you're going to use for it. \$\endgroup\$ – Mathieu Guindon May 25 '15 at 2:34
  • 1
    \$\begingroup\$ @AMartinNo1 The PasswordService has nothing to do with the UI. The view would collect the values user wants to use in some model class that a presenter (assuming MVP) would validate and use to pass into the PasswordService after the user made their selection. If you then need to send the generated string back to the UI, and the user can make further changes and generate another string, then yes, another string, another instance. \$\endgroup\$ – Mathieu Guindon May 25 '15 at 2:49
8
\$\begingroup\$

I've never seen this:

namespace PasswordGenerator
{
namespace Utility
{
namespace Service
{

I'm guessing what you intended was this:

namespace PasswordGenerator.Utility.Service
{

However if you look at some of the framework's namespaces...

using System.Collections.Generic;
using System.Threading.Tasks;

I think PasswordGenerator.Utilities.Services would be more appropriate.


    string sAlphaCharacters;
    string sNumericCharacters;
    string sNonAlphanumericCharacters;
    uint uiNumberOfAlphaCharacters;
    uint uiNumberOfNumericCharacters;
    uint uiNumberOfNonAlphanumericCharacters;

You're using the abusive form of Hungarian Notation, and this is highly discouraged, especially in a statically typed language like C# - see why.


All public members should be PascalCase. I don't understand why getRandomSubstrOfCharacterGroup isn't called SGetRandomSubstrOf... - you're not being consistent with your own naming conventions, which makes things confusing.

Looking at the function's body...

        string sCharacters = "";

        for (uint ui = 0; ui < uiNumberOfThisGroup; ui++)
        {
            iPosition = iPosition = rnd.Next(0, sCharacterGroup.Length);
            sCharacters += sCharacterGroup.ToCharArray()[iPosition].ToString();
        }

        return sCharacters;

You're building a string here - anytime you need to concatenate strings inside a loop, you should think "hey, that's a job for a StringBuilder!" - and use a StringBuilder instead ;-)


throw new InvalidOperationException("The number of each character group is 0.");

The exception type is correct - the current state of the object doesn't allow this operation. But the exception itself is only "needed" because you allowed this state to happen: if these values were readonly and initialized and validated in the constructor, you wouldn't need this check.

Also, I don't think you need a separate Random instance per method - consider promoting that one to a private field.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply. You're totally right, I will update the namespace part and same goes for hungarian notation. The linked comment is pretty good:-) What's against the usage of +=? \$\endgroup\$ – AMartinNo1 May 25 '15 at 2:49
  • \$\begingroup\$ It's not the string concatenation as much as doing it in a loop that's a problem. Remember a String is an immutable object, so you're creating a new instance everytime you're appending a character - that's definitely better handled with a StringBuilder. \$\endgroup\$ – Mathieu Guindon May 25 '15 at 2:52
  • \$\begingroup\$ Oh alright. Good to know. \$\endgroup\$ – AMartinNo1 May 25 '15 at 2:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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