0
app.post('/api/edit-profile', regularFunctions, async function (req, res) {
    let email = req.body.email
    let password_current = req.body.password_current
    connection.query('SELECT * FROM accounts WHERE id = ?', req.body.id, async function (err, results) {
        if (results.length > 0) {
            bcrypt.compare(password_current, results[0].password, async function (err, isMatch) {
                if (err) {
                    res.send('Unable to save settings')
                    res.end();
                    throw err
                } else if (!isMatch) {
                    res.send('Password doesn\'t match.')
                    res.end();
                } else {
                    let changed = []
                    // Password matches
                    if (req.body.password_new) {
                        let newPassword = req.body.password_new
                        let hashed_password = await hashPassword(newPassword)
                        connection.query('UPDATE accounts SET password = ? WHERE id = ?', [hashed_password, req.body.id], async function (error, results) {
                            if (results.affectedRows && results.affectedRows > 0) {
                                changed.push('password')
                            } else {
                                res.send('Unable to save settings')
                                res.end();
                            }
                        })
                    }
                    if (req.body.license_key) {
                        let newKey = req.body.license_key
                        axios.get(`https://voltcloud.net/api/hosting/check-key/${newKey}`, {
                            headers: {
                                authorization: 'Y1wUo3joP99JHiGM2orji0UYTey9gdqY'
                            }
                        }).then(function (response) {
                            let data = response.data
                            if (typeof data === 'object') {
                                if (data.active === 1) {
                                    axios({
                                        method: 'post',
                                        url: `https://voltcloud.net/api/hosting/activate-key/${newKey}`,
                                        headers: {
                                            authorization: 'Y1wUo3joP99JHiGM2orji0UYTey9gdqY'
                                        }
                                    }).then(async function (response) {
                                        if (response.data === 'Success') {
                                            connection.query('UPDATE accounts SET license_key = ? WHERE id = ?', [newKey, req.body.id], async function (error, results) {
                                                if (results.affectedRows && results.affectedRows > 0) {
                                                    changed.push('license key')
                                                } else {
                                                    res.send('Unable to save settings')
                                                    res.end();
                                                }
                                            })
                                        } else if (data === 'License already active!') {
                                            res.send('License key is already active!')
                                            res.end();
                                        } else if (data === 'Failed to update key.') {
                                            res.send('Unable to save settings')
                                            res.end();
                                        } else {
                                            res.send('Unable to save settings')
                                            res.end();
                                        }
                                    });
                                }
                            }
                        })
                    }
                    connection.query('UPDATE accounts SET email = ? WHERE id = ?', [email,req.body.id], async function (error, results) {
                        if (results.affectedRows && results.affectedRows > 0) {
                            changed.push('email')
                        } else {
                            res.send('Unable to save settings')
                            res.end();
                        }
                    });
                    let finalTxt = 'Successfully changed, '
                    if (changed.length > 1) {
                        changed.forEach(function (txt, index) {
                            if (index === 0) {
                                finalTxt = finalTxt + txt
                            } else if (index === 2) {
                                finalTxt = finalTxt + `and ${txt}.`
                            }
                        })
                    } else if (changed.length === 1) {
                        finalTxt = `Successfully changed ${changed[0]}.`
                    }
                    res.send(finalTxt)
                    res.end();
                }
            })
        }
    })
});}

I know this might seem like a very easy problem to some expert coders, but I am sort of new to this whole async and synchronous thing. Why is it that the "changed" array doesn't update even though it's being pushed to after the functions run? What I'm trying to do is have it only return one string that can be shown on the client-side but it doesn't seem to be changing it and only returning the "Successfully changed, "

Ize_Cubz
  • 37
  • 2
  • 7
  • 1
    This function does _way more than it should_. If you're using express, write middleware functions that take care of a single step in the process, then hand that off to the next function by calling `next()`. so that your route looks like `app.post('/api/edit-profile', regularFunctions, queryDb, verifyCrypto, updateDb, verifyLicense, ... , ... , (req,res) => { res.html(...))` where each step in "what this route should do" is an actual step that can either succeed, moving on to the next one, or fail, ending up in your error handler because you called `next(new Error(...))`; – Mike 'Pomax' Kamermans Oct 07 '20 at 20:53
  • @Mike'Pomax'Kamermans is right, the function body is getting extremely complex and you should be thinking about break down that function right away. Besides that, I see that you're not handling any of the errors that the queries might return, so I'd check that as well. – Gustavo Kawamoto Oct 07 '20 at 20:58
  • Is the `connection` var a PostgreSQL connection? If so, you can move to async/await in there as well, since the `pg` module already implements promises – Gustavo Kawamoto Oct 07 '20 at 20:59
  • No, it is not a PostgreSQL, the server is a MariaDB one and it's connected with the MySQL npm package – Ize_Cubz Oct 07 '20 at 21:49

2 Answers2

0

This function confused me, as it has a lot of responsabilities as Mike 'Pomax' Kamermans pointed out, but I found the problem:

The connection.query method is non-blocking, meaning it will not wait for it's execution to end for it to advance to the next instructions.

When you are using async methods and Promise, it's nice to try and keep consistency over the methods (avoid mixing callback functions and async/await). I've refactored it over what it should look like if using async/await:

app.post('/api/edit-profile', regularFunctions, async function (req, res) {
    let email = req.body.email
    let password_current = req.body.password_current
    let results = await executeQuery(connection, 'SELECT * FROM accounts WHERE id = ?', [req.body.id]);
    if (results.length > 0) {
        let isMatch = await comparePassword(password_current, results[0].password);
        if (!isMatch) {
            throw new Error(`Password doesn't match`);
        }
        let changed = []
        // Password matches
        if (req.body.password_new) {
            let newPassword = req.body.password_new
            let hashed_password = await hashPassword(newPassword)
            let results = await executeQuery(connection, 'UPDATE accounts SET password = ? WHERE id = ?', [hashed_password, req.body.id]);
            if (results.affectedRows && results.affectedRows > 0) {
                changed.push('password')
            } else {
                throw new Error('Unable to save settings');
            }
        }
        if (req.body.license_key) {
            let newKey = req.body.license_key
            let response = await axios.get(`https://voltcloud.net/api/hosting/check-key/${newKey}`, {
                headers: {
                    authorization: '<redacted>'
                }
            });
            let data = response.data
            if (typeof data === 'object') {
                if (data.active === 1) {
                    let response = await axios({
                        method: 'post',
                        url: `https://voltcloud.net/api/hosting/activate-key/${newKey}`,
                        headers: {
                            authorization: '<redacted>'
                        }
                    })
                    if (response.data === 'Success') {
                        let results = await executeQuery(connection, 'UPDATE accounts SET license_key = ? WHERE id = ?', [newKey, req.body.id]);
                        if (results.affectedRows && results.affectedRows > 0) {
                            changed.push('license key')
                        } else {
                            throw new Error('Unable to save settings');
                        }
                    } else if (data === 'License already active!') {
                        throw new Error('License key is already active!');
                    } else if (data === 'Failed to update key.') {
                        throw new Error('Unable to save settings');
                    } else {
                        throw new Error('Unable to save settings');
                    }
                }
            }
        }
        let results = await executeQuery(connection, 'UPDATE accounts SET email = ? WHERE id = ?', [email,req.body.id]);
        if (results.affectedRows && results.affectedRows > 0) {
            changed.push('email')
        } else {
            throw new Error('Unable to save settings');
        }
        let finalTxt = 'Successfully changed, '
        if (changed.length > 1) {
            changed.forEach(function (txt, index) {
                if (index === 0) {
                    finalTxt = finalTxt + txt
                } else if (index === 2) {
                    finalTxt = finalTxt + `and ${txt}.`
                }
            })
        } else if (changed.length === 1) {
            finalTxt = `Successfully changed ${changed[0]}.`
        }
        res.send(finalTxt)
        res.end();
    }
});

function executeQuery(conn, sql, params) {
    return new Promise((resolve, reject) => {
        conn.query(sql, params, function (err, data) {
            if (err != null) {
                return reject(err);
            }
            return resolve(data);
        });
    });
}

function comparePassword(val1, val2) {
    return new Promise((resolve, reject) => {
        bcrypt.compare(val1, val2, function (err, isMatch) {
            if (err != null) {
                return reject(err);
            }
            resolve(isMatch);
        });
    })
}

Notice that we're not using callback functions at all, and even where we don't have native Promise-based functions (i.e. mysql connection), we're delegating to a function that proxies the callback to deliver a Promise and keep consistency over the final implementation.

Gustavo Kawamoto
  • 2,665
  • 18
  • 27
0

The original code isn't waiting for the two if branches to complete before sending the response. It's hard to structure code like this in callbacks, due to the nesting.

Try using async functions and await wherever possible. It allows for much more readable code and error handling is much easier. So this answer is more code review than a simple fix for your issue.

Split out some generic helper code that will be useful in other routes:

// Generate errors for the web, with context
function responseError(message, status, data){
    const error = new Error(message)
    error.status = status
    for (const key in data){
        error[key] = data[key]
    }
    return error
}

// Turn mysql callbacks into promises (or use util.promisify)
async function runQuery(query, values){
    return new Promise((resolve, reject) => {
        connection.query(query, values, function(error, results){
            if (error) return reject(error)
            return resolve(results)
        })
    })
}

async function runUpdateQuery(query, values){
    const results = await runQuery(query, values)
    if (!results) throw responseError('No update result', 500, { query })
    if (!results.affectedRows) throw responseError('No affected rows', 400, { query })
    return results
}

The code from the two if conditions can be easily separated, as well as the other account operations.

async function apiAuthUserId(id, password){
    const results = await runQuery('SELECT * FROM accounts WHERE id = ?', id)
    if (!results.length) throw responseError('No account', 400, { id })
    const isMatch = await bcrypt.compare(password_current, results[0].password)
    if (!isMatch) throw responseError('Password doesn\'t match', 400)
    return true
}

async function apiUpdatePassword(id, password){
    let newPassword = req.body.password_new
    let hashed_password = await hashPassword(newPassword)
    await runUpdateQuery('UPDATE accounts SET password = ? WHERE id = ?', [hashed_password, req.body.id])
    return id
}

async function apiUpdateEmail(id, email){
    await runUpdateQuery('UPDATE accounts SET email = ? WHERE id = ?', [email, id])
    return email
}

async function apiUpdateLicenseKey(id, licenseKey){
    const response_license = await axios.get(`https://voltcloud.net/api/hosting/check-key/${licenseKey}`, {
        headers: {
            authorization: 'somekey'
        }
    })

    const data = response_license.data
    if (!data) {
      throw responseError('No license key response data', 500, { response: response_license })
    }
    if (data.active !== 1) {
      throw responseError('License key not active', 400,  { key: licenseKey })
    }
    const response_activate = await axios({
        method: 'post',
        url: `https://voltcloud.net/api/hosting/activate-key/${licenseKey}`,
        headers: {
            authorization: 'somekey'
        }
    })

    switch (response_activate.data){
        case 'License already active!':
            throw responseError('License key is already active!', 400, { response: response_activate })
        case 'Failed to update key.':
            throw responseError('Unable to save settings!', 400, { response: response_activate })
        case 'Success':
            await runUpdateQuery('UPDATE accounts SET license_key = ? WHERE id = ?', [licenseKey, req.body.id])
            return licenseKey
        default:
            throw responseError('Unable to save settings!', 500, { response: response_activate })
    }
}

Then your route code can be a bit cleaner and show what needs to be done, rather than how to do it all.

app.post('/api/edit-profile', regularFunctions, async function (req, res) {
    const changed = []
    try {
        const { id, email, password_current } = req.body

        await apiAuthUserId(id, password_current)

        // Password matches
        if (req.body.password_new) {
            await apiUpdatePassword(id, req.body.password_new)
            changed.push('password')
        }

        // License key
        if (req.body.license_key) {
            await apiUpdateLicenseKey(id, req.body.license_key)
            changed.push('license key')
        }

        await apiUpdateEmail(id, email)
        changed.push('email')

        let finalTxt = `Successfully changed ${changed.join(' and ')}.`
        res.send(finalTxt)
    }
    catch (error) {
        // If your not using transactions, might need to deal with partial `changed` responses here. 
        console.error('Error /api/edit-profile', error)
        res.status(error.status||500).send(`Error: ${error.message}`)
    }
});
Matt
  • 68,711
  • 7
  • 155
  • 158