1
\$\begingroup\$

I recently wrote a login function in my express application that does the following:

  1. Verifies the user's email and password are correct
  2. Generates a JWT Access Token with a short expiry date, and then sends it back to the user
  3. Generates a JWT Refresh Token with a long expiry date, and then saves it to MongoDB. I will then create a middleware to check if that access token still exists on each secured endpoint only if the Access token is expired. Then, I will send them a new Access Token.

I am looking to improve the readability and maintainability for this login endpoint.

Here is the login endpoint

// Login to the application
router.post('/login', loginRules, validate, async (req, res, next) => {
    try{
        // verify the email and password
        let user = await Account.findOne({email: req.body.email}).exec();
        if(user == null) {
            return res.status(400).json({error: 'User not found'});
        }

        let isPasswordCorrect = await bcrypt.compareSync(req.body.password, user.password);

        // login was successful
        if(isPasswordCorrect) {
            // generate a jwt token
            let accessToken = jwt.sign({email: user.email}, process.env.JWT_SECRET_KEY, 
                {expiresIn: parseInt(process.env.JWT_ACCESS_TOKEN_EXPIRE_TIME)});
            // generate a refresh token and store it into the database.
            let userIPAddress = req.ip;
            let userBrowser = req.headers['user-agent'];
            let refreshToken = jwt.sign({email: user.email, ip_address: userIPAddress,
                                        user_browser: userBrowser}, process.env.JWT_REFRESH_TOKEN_SECRET_KEY,
                                       {expiresIn: process.env.JWT_REFRESH_TOKEN_EXPIRE_TIME});
            const userDelegate = new Delegate({account: user.id,
                                               refresh_token: refreshToken,
                                               ip_address: userIPAddress,
                                               browser: userBrowser});
            await userDelegate.save();
            // Send the jwt back to the caller if the login was sucessful
            return res.status(200).json({token: accessToken});
        }
        // login was incorrect
        else {
            return res.status(400).json({error: 'Incorrect Login Credentials'});
        }

    }
    catch(e) {
        return res.status(400).json({error: 'Unexpected error happened'});
    }
})

LoginRules middleware

// validation-rules.js
// validation rules for logging into the application
const loginRules = [
    check('email').exists({checkFalsy: true, checkNull: true}).not().isEmpty().isEmail().normalizeEmail(),
    check('password').exists({checkFalsy: true, checkNull: true}).not().isEmpty().isString()
]


module.exports = {
    loginRules
}

validate middleware

// validator.js
const { validationResult } = require('express-validator');

// Check if there wasn't any errors when the user input was validated.
const validate = (req, res, next) => {
    const errors = validationResult(req);
    if(!errors.isEmpty()) {
        return res.status(400).json({errors});
    }
    
    next();
}

module.exports = {validate};
\$\endgroup\$
4
  • \$\begingroup\$ Why use await in await bcrypt.compareSync(...)? It's synchronous, right? It directly returns the value, not a promise, right? \$\endgroup\$
    – jfriend00
    Jun 29, 2021 at 23:45
  • \$\begingroup\$ In your try/catch, you should log the actual exception in the catch because if it starts happening, you will need to know what is actually going wrong in order to troubleshoot. \$\endgroup\$
    – jfriend00
    Jun 29, 2021 at 23:47
  • \$\begingroup\$ @jfriend00 Yes you are 100% correct on both comments. Thank you for the feedback. \$\endgroup\$
    – Tony
    Jun 30, 2021 at 0:22
  • \$\begingroup\$ @jfriend perhaps you were waiting for a response to the question in your first comment but please add an answer instead of a comment. Refer to the section When shouldn't I comment? on Comment everywhere. \$\endgroup\$ Jun 30, 2021 at 14:32

1 Answer 1

0
\$\begingroup\$

I think your middleware is fine. The problem is that you do everything in the controller. The controller should just call the login function from your business layer.

You had to comment the code to explain it. Treat this as a sign that the function is doing too much. You will see, that I have basically just split the code into the functions based on your comments. Everything else is basically just creation of the objects that will be handled by your business logic.

From the controller's point of view all of the complexity is hidden away. Controller just calls the login function and responds with the result of that function.

auth.controller.js:

import { login } from './auth.service.js'

router.post('/login', loginRules, validate, async (req, res, next) => {
    try{
        const credentials = { email: req.body.email, password: req.body.password }
        const refreshTokenOpts = {
            ip: req.ip,
            browser: req.headers['user-agent']
        }

        const accessToken = await login(credentials, refreshTokenOpts)

        return res.status(200).json({ accessToken });
    }
    catch(e) {
        if(e.code === 'user_not_found' || e.code === 'incorrect_password') 
            return res.status(400).json({error: 'Invalid username or password'})

        return res.status(500).json({error: 'Unexpected error happened'});
    }
})

Above I have just defined the behavior of the login: it receives the credentials and parameters that will be used for refresh token, it returns the access token and throws "user not found" and "incorrect password".

How I implement that is now down to auth service and not a concern of the controller.

auth.service.js:

function verifyPassword(actual, expected){
    let isPasswordCorrect = bcrypt.compareSync(actual, expected)
    if(!isPasswordCorrect) throw new Error({code: 'incorrect_password'})
}

function verifyUserCredentials(credentials, user) {
    if(!user) throw new Error({code: 'user_not_found'})

    verifyPassword(credentials.password, user.password)
}

function generateToken(user) {
    const payload = { email: user.email }
    const options = { 
        expiresIn: parseInt(process.env.JWT_ACCESS_TOKEN_EXPIRE_TIME)
    }

    return jwt.sign(
        payload,
        process.env.JWT_SECRET_KEY,
        options
    );
}

function generateRefreshToken(user, ip, browser) {
    const payload = { 
        email: user.email,
        ip_address: ip,
        user_browser: browser
    }
    const options = { expiresIn: process.env.JWT_REFRESH_TOKEN_EXPIRE_TIME }

    return jwt.sign(
         payload,
         process.env.JWT_REFRESH_TOKEN_SECRET_KEY,
         options
    );
}

async function saveRefreshToken(user, ip, browser, refreshToken) {
    new Delegate(
        {
            account: user.id,
            refresh_token: refreshToken,
            ip_address: ip,
            browser
        }
    )
    .save();
}

export async function login(credentials, {ip, browser}) {
    const user = await Account.findOne({ credentials.email }).exec()
    verifyUserCredentials(credentials, user)

    const refreshToken = generateRefreshToken(user, ip, browser)
    await saveRefreshToken(user, ip, browser, refreshToken)

    return generateToken(user)        
}

This is just as far as the code refactoring goes.

As login goes, it is not clear to me, why you are generating the refresh token, but not returning it to the user. And if you are not returning it to the user by design and not by accident, why are you saving it to the database instead of saving the actual data, that you would need to generate another access token.

To me it seems like you are misusing the JWT, as it serves to ensure integrity of the client data not of the data that you have control over, otherwise you would be saving everything with JWT in the database.

\$\endgroup\$
12
  • \$\begingroup\$ To answer your question, I was reading online that you aren't supposed to send the refresh token to the user. Rather, we only send the Access token to the user and then regenerate the new Access token when it's expired by using the refresh token that would be stored in the database. When generating a new access token, I verify that the expired token wasn't used in a CSRF attack by making sure the same IP address and browser that requested the original access token is still the same user. I could be completely wrong in my approach, so if you can send me on the right path that would be good. \$\endgroup\$
    – Tony
    Jul 1, 2021 at 16:58
  • \$\begingroup\$ Also thank you for the response it was very insightful. \$\endgroup\$
    – Tony
    Jul 1, 2021 at 16:58
  • \$\begingroup\$ If we sent both the refresh token and the access token to the user, then both the refresh token and access token could be exposed in a CSRF attack. Also, since the refresh token has a longer expiry time than an access token, an attacker could use that token for a way longer duration of time. Again, i could be wrong in this regard. \$\endgroup\$
    – Tony
    Jul 1, 2021 at 17:07
  • \$\begingroup\$ @Tony But you aren't storing the token in cookies. As far as I know CSRF only works if you store the token in cookies and that can be prevented by using "same site cookie" cheatsheetseries.owasp.org/cheatsheets/…. Also, you are checking the IP and browser to make sure that the request was sent from the same client when you are refreshing the token, so even if someone stole it, he wouldn't be able to reproduce it I think. \$\endgroup\$
    – Blaž Mrak
    Jul 1, 2021 at 17:21
  • \$\begingroup\$ I think that in your case you don't even need a refresh token. Save the access token to the database and expiry time. You can then refresh the token by sending the refresh token to the server and checking if IP, browser and token are the same and if the "right to refresh" didn't expire. I'm not sure about it, but the refresh token is not used anywhere anyway. Also to have the token saved in the database at all might be useless :D I'm not a security expert though, so take everything I say regarding safety with a grain of salt :) \$\endgroup\$
    – Blaž Mrak
    Jul 1, 2021 at 17:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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