multi-dimensional javascript array only affecting last array in forEach

Question

I have a multi-dimensional array in JavaScript that holds basic Usernames and to-be hashed passwords. At the moment, when the function to check the credentials is called, the forEach will only check the last array.

const titleText = document.getElementById('loginText');
const usernameField = document.getElementById('usernameField');
const passwordField = document.getElementById('passwordField');

const usernames = [['guido','password'],['ben','test']];

function checkCreds() {
    titleText.textContent = ">> Checking login";

    usernames.forEach(element => {
        if (element[0] === usernameField.value) {

            if (element[1] === passwordField.value) {

                titleText.textContent = '>> Login Valid';
                window.location = "dashboard.html";

            } else {
                titleText.textContent = '>> Password incorrect';
            };
        } else {
            titleText.textContent = '>> Login incorrect';
        };
    });
};

Here, when I type in the credentials: guido and password, it will say that the login is incorrect. But when I type in ben and test, it will proceed as normal. If anyone has an idea on to why this won't work or has better code, please drop an answer. As I say, this will be hashed, salted and not in the file, all that stuff when it's working.

Adam H · Accepted Answer · 2018-08-24T19:10:51.117

4

The problem seems to be that you aren't breaking out of your loop so you are in fact checking all elements in the array but the last element is the one that is sticking. Try braking from your loop, something like this;

const titleText = document.getElementById('loginText');
const usernameField = document.getElementById('usernameField');
const passwordField = document.getElementById('passwordField');

const usernames = [
  ['guido', 'password'],
  ['ben', 'test']
];

function checkCreds() {
  titleText.textContent = ">> Checking login";

  // instead of using Array.forEach use a standard for loop, this allows you to
  // break out of the loop and return. 
  for(let i = 0; i < usernames.length; i++){
    if (usernames[i][0] === usernameField.value){
      if (usernames[i][1] === passwordField.value){
        // show that the login was successful
        titleText.textContent = '>> Login Valid';
        // redirect to the dashboard
        window.location = 'dashboard.html';
        // just return here, there is no need to break out of the loop,
        // returning will end the execution of this function.
        return;
      }
    }
  }
  
  // display the error to the user, we don't want to indicate if the 
  // password or the username were invalid because that tells an attacker
  // they have the correct user name.
  // We also don't have to check a flag because a valid login will result
  // in this code never being hit
    titleText.textContent = '>> Login incorrect';
};

Edit:

Based on the information from Ben West I have updated the solution to use a standard for loop to allow breaking out of the loop.

edited Aug 24 '18 at 19:10

answered Aug 24 '18 at 16:16

Adam H

1,750
1
9
24

Perfect, so my issue was that I wasn't breaking out of the forEach loop? And by having the foundUser variable in there, it better handles the Login Incorrect error. – ben_foster04 Aug 24 '18 at 16:23
You can't break out of a `forEach`. Who lied to you? – ibrahim mahrir Aug 24 '18 at 16:27
1

`return false` doesn't stop `.forEach`. Use a regular `for` loop and `break`, or `.find`. – Ben West Aug 24 '18 at 16:52
@BenWest wow, I did not know that. Thank you for pointing that out. I will update the solution to reflect a pattern I found to deal with that. – Adam H Aug 24 '18 at 19:03
@ben_foster04 Ok, so the solution I gave you worked but is not the correct answer. I have updated the answer so please take a look at that and let me know if you have any more questions or if you want me to expand on anything. – Adam H Aug 24 '18 at 19:12
Nice! jQuery’s `.each` can be stopped by returning false, maybe that’s what you were thinking of? – Ben West Aug 25 '18 at 12:23
1

@BenWest Yeah, I think that's what I was thinking about. – Adam H Aug 27 '18 at 16:30

multi-dimensional javascript array only affecting last array in forEach

1 Answers1