Basic password authentication system app

Question

This is a console app I wrote for an end-of-chapter assignment in Jumping into C++. The parameters were for it to have a basic password system, and if you are eager, a multiple password / username system. I am going through this book completely on my own free will. It's not through a school assignment or whatnot.

It works quite well, but as I have a limited programming background, I wanted to post it here so any bad techniques or poor syntax could be brought to my attention.

#include <iostream>

using namespace std;

string usrnam[] = { "admin", "user", "" };
string pasword[] = { "root", "default", ""};

string temp_pass;
string temp_usr;

bool loggedin;

int n = sizeof(usrnam) / sizeof(string);

int findPassword( string str[], int array_size, string query )
{
    for ( int i = 0; i < array_size; i++ )
    {
        if ( str[i] == query )
    {
        return i;
    }
}
}

int commandC()
{
    int stage;
    cout << "Enter '1' for calculator: ";
    cin >> stage;
    switch(stage)
{
    case 1: {
        cout << endl;
        cout << "Not fully developed" << endl << endl;

    }
}

cout << "_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-";
}

void login(string user, string pass){
     if ( pass == pasword[findPassword(usrnam, n, user)] )
     {
        cout << "Logged in as " << user << endl << endl;
        loggedin = true;
        commandC();
     }
}

void registerUser()
{
    string answer;
    cout << "No matches found in the database...would you like to make new account? (Y/N)";
    cin >> answer;
    if (answer == "y" || answer == "Y")
    {
        cout << "Please enter a username: ";
        cin >> usrnam[n-1];
        cout << "Please enter a password: ";
        cin >> pasword[n-1];

        login(usrnam[n-1],pasword[n-1]);
    }
}

int main()
{
    cout << "Enter username: ";
    cin >> temp_usr;
    cout << "Enter password; ";
    cin >> temp_pass;

    login(temp_usr,temp_pass);

    if(!loggedin)
        registerUser();
}

Answer 1

• Try using std::vector instead of arrays. As a result, you'll not be required to keep track of the array_size in a separate variable like n in your case.

  Something like this:

  std::vector<std::string> username;
  username.push_back("admin");
  //other entries go here
  std::vector<std::string> password;
  password.push_back("root");
  //other entries go here
  

  Then, the size of the std::vector is given by size().

• Try to use proper names as variables. Avoid wrong spellings or short-names. Exact variable names make the code easy to understand, as the name represents the actual concept the variable is referring to.

• Also, in registerUser() the variable answer can just be a char. This will help in saving a little memory. Like:

  char answer;
  std::cin>>answer;
  if(answer=='y' || answer=='Y')
  {
      //code goes here
  }
  

• In your code, the method commandC() is defined to return an int. But it doesn't do so. Make the return type void instead.

  void CommandC()
  {
      //code goes here
  }
  

• Also, the main() method is defined to return an int too. It is a good practice to declare it like that, but make sure to return an integer. 0 (zero) is preferred as it refers to successful completion of main().

  int main()
  {
      //code goes here
      return 0; //indicates successful termination
  }
  

Answer 2

You have some compiler warnings:

$ clang++ -o console{,.cpp}
console.cpp:24:1: warning: control may reach end of non-void function [-Wreturn-type]
}
^
console.cpp:41:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
2 warnings generated.

You have a segfault:

Enter username: a
Enter password; b
Segmentation fault: 11

For entering a wrong password, I wouldn't expect to be invited to create an account:

Enter username: user
Enter password; WrongPassword
No matches found in the database...would you like to make new account? (Y/N)y
Please enter a username: admin
Please enter a password: LaxSecurity

If a password contains a space, weird things happen:

Enter username: user
Enter password; correct y horse battery staple
No matches found in the database...would you like to make new account? (Y/N)Please enter a username: Please enter a password: Logged in as horse

Enter '1' for calculator: _-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-

Answer 3

• These are all being used as global variables:

  string usrnam[] = { "admin", "user", "" };
  string pasword[] = { "root", "default", ""};
  
  string temp_pass;
  string temp_usr;
  
  bool loggedin;
  
  int n = sizeof(usrnam) / sizeof(string);
  

  This is bad practice because any of these variables can be modified anywhere at any time, which is a source of bugs and maintenance issues. Both of those initialized strings and n should be const, which will prevent them from being modified further.

  The rest of the variables should be declared in functions and possibly passed to other functions for use. This will allow you to keep track of how they're being used.

• Passing in a C-style array isn't preferred:

  int findPassword( string str[], int array_size, string query ) {}
  

  This will actually deprecate to a pointer, which is not what the user expects to happen. This is also common practice in C, but you're writing in C++.

  As @gargankit mentioned, prefer to use an STL container such as std::vector in place of all the C-style arrays to avoid this deprecation. It'll also no longer be necessary to pass in array_size since all STL containers know their sizes (accessible with the function size()).

• switch seems needless here:

  switch(stage)
  {
      case 1: {
          cout << endl;
          cout << "Not fully developed" << endl << endl;
      }
  }
  

  If you only have one case, just use an if:

  if (stage == 1)
  {
      cout << endl;
      cout << "Not fully developed" << endl << endl;
  }
  

  You could also make the body just one line, and with "\n" instead of std::endl. The latter also flushes the buffer, which takes longer. No need to use it for every newline.

  cout << "\nNot fully developed\n\n";
  

Answer 4

In the following:

int findPassword( string str[], int array_size, string query )
{
    for ( int i = 0; i < array_size; i++ )
    {
        if ( str[i] == query )
    {
        return i;
    }
}
}

If you use std::vector as suggested by gargankit, or you use std::array, you would be able to take advantage of the range-based for loops in C++. Instead of:

for(int i = 0; i < array_size; ++i)
{
    if(str[i] == query)
    {
       return i;
    }
}

you would write

for(const auto & arrayElement : str)
{
    if(arrayElement == query)
    {
        // note in this case arrayElement is a string, not
        // an integer index into the array, so this would
        // require changing the return type of findPassword
        return arrayElement;
    }
}

The benefit of a range-based for loop is that you don't have to be concerned about the potential risk of stepping out of the bounds of the given container, in this case str.

If you are feeling adventurous you might also look into using the standard algorithms included in the STL and calling either std::find_if, or std::any_of to find the array element you are looking for.

For functions that you are writing which take parameters that you will not modify in the body of the function, prefer taking the parameters as const parameters to specify and enforce your intent not to modify those values.

For example in:

void login(string user, string pass){
     if ( pass == pasword[findPassword(usrnam, n, user)] )
     {
        cout << "Logged in as " << user << endl << endl;
        loggedin = true;
        commandC();
     }
}

You never modify the value of pass, and it would be particularly strange in this case if you did modify the password that the user was typing in. In such a case it is more clear and more correct to require pass be const in login:

void login(string user, const string pass){
     if ( pass == pasword[findPassword(usrnam, n, user)] )
     {
        cout << "Logged in as " << user << endl << endl;
        loggedin = true;
        commandC();
     }
}

You can probably also do this to user, but that would require also making user const in the findPassword function.

Also, for any variables that you pass along as const, consider passing them as "const &" so that you do not make any unnecessary copies of that data.

Answer 5

You're using a lot of global variables, such as bool loggedin;. It's best to keep a variable's scope as small as possible. To make the scope smaller for loggedin, for example, remove this global declaration:

bool loggedin;

Do the following to the login function:

bool login(string user, string pass)
{
    bool loggedin; // Make a local copy of this in login

    if(pass == pasword[findPassword(usrnam, n, user)])
    {
        cout << "Logged in as " << user << endl << endl;
        loggedin = true;
        commandC();
    }
    else
    {
        loggedin = false; // Always make sure variables are initialized to something
    }

    return loggedin; // Return this value to calling function
}

Now you can call login like this:

int main()
{  
    cout << "Enter username: ";
    cin >> temp_usr;
    cout << "Enter password; ";
    cin >> temp_pass;

    bool loggedin = login(temp_usr,temp_pass); // Make another local copy of loggedin in main

    if(!loggedin)
        registerUser();
}

Answer 6

In terms of best practice, you should keep in mind that passwords should never be stored raw, only as salted hashes. If you are going to ever write a program that will handle passwords in actual use (not just as a programming exercise), you really need to read Salted Password Hashing - Doing it Right. (Even if not, it's still a good read anyway.)

The reason for this is not because your application necessarily needs to be secure, but to ensure someone doesn't hack your application and steal your users accounts for other services, because most people use the same username and password everywhere.

Also, one other feature that would normally be needed is the ability to store the user data in a file and read it when it starts, so it can persist between sessions, although this too is probably beyond the scope of the excersise.

With respect to other aspects of your code, it seems you are on the right track; everything else I can see has already been said by other users.

Answer 7

you should look at the way you are indenting your code, this piece of code almost confused me for a minute ( or two )

int findPassword( string str[], int array_size, string query )
{
    for ( int i = 0; i < array_size; i++ )
    {
        if ( str[i] == query )
    {
        return i;
    }
}
}

it should look like this

int findPassword( string str[], int array_size, string query )
{
    for ( int i = 0; i < array_size; i++ )
    {
        if ( str[i] == query )
        {
            return i;
        }
    }
}

it is always good to keep your indentations straight (even thought you could write the code without them) it makes the code easier to read.

Answer 8

Usually comment about this line (look it up in every other C++ review).

using namespace std;

Don't use C arrays. The C++ containers are much more useful. Every other answer has suggested you use std::vector<std::string> as a replacement. That again is a terrible idea. Your main usage pattern is searching. Use a data structure that is designed to be searched. std::map<std::string, std::string>

string usrnam[] = { "admin", "", "" };
string pasword[] = { "root", "default", ""};

// I would have used:

std::map<std::string, std::string> password = {{"admin","root"},{"user","default"}};

I can see putting the actual username/password in global scope (as it needs to be used from everywhere (not a great design but acceptable). But no other variables should be in the global scope they should be close to the point of usage.

string temp_pass;
string temp_usr;

This should have been const. Your current design did not allow for modification of the username/password so this should have been const. But if you use a standard C++ container this becomes superfluous.

int n = sizeof(usrnam) / sizeof(string);

// Also note it is more traditional to write this as:
int n = sizeof(usrnam) / sizeof(usrnam[0]);

// This is because if you change the type of `usernam` this code is still
// correct. In your version you would need to change the code in two places.

C++ requires you to return a value at the end of a function:

int findPassword( string str[], int array_size, string query )
// This function does not return anything when the name is not
// found. This will result in hard to find errors.

When dealing with user input read a line at a time then parse the line.

cin >> stage;

// What happens if the user entered 1 12 <enter>
// The next time you are looking for input then you will read 12
// even though it is left over from this input.
// 
// Users interact with the terminal in lines.
// Because there input is not flushed until they hit `enter`
// they will hit enter after their input. The program does not recognize
// this and the operator>> does not remove the resulting '\n' character
// from the input. So if the next interaction with the user is getline()
// you will mysteriously read an empty line even if the user enters text.

Always use a break at the end of case. The exception is so rare that it should be well documented with comments.

Always put a default in a switch. Even if the default (throws an exception). An exception is better than the code blowing up because some other piece of code has changed.

switch(stage)
{
    case 1: {
       cout << endl;
        cout << "Not fully developed" << endl << endl;
    }
}

Password design.
Absolutely NEVER store the plain text version of the password. Always use a one way hash like md5 on the password and store the hash of the password.

Many people use the same password on multiple systems. If you lose control of your passwords then your have compromised your customers security.

So accept the password in plan text but always store it has a hash. You may also want to read up on slating the password first.