msc-dtss / super6assignment

First Assignment for the 2019 MSc for Digital & Technology Solutions Specialist
0 stars 0 forks source link

Design and create basic authentication/authorization system #5

Closed neilmusgrove closed 4 years ago

neilmusgrove commented 5 years ago

We need a basic authentication/authorization system with at least the following:

It will need to include a session cookie so that a user remains logged in while using the system. Normally the users password would be stored hashed and salted with SHA-256 or better. It is not vital for this project however it will probably be straightforward.

MikeKeightley commented 5 years ago

@neilmusgrove - is there any particular bits of this I can have a look at building? Just been reviewing what you've done so far and it looks good - fairly self explanatory. I've done some reading on bcrypt, and salting etc so far, but not done any coding for it.

neilmusgrove commented 5 years ago

@MikeKeightley You can have a go at any of the empty functions in the users service file. If you have any questions give me a shout but feel free to dive and and try stuff. I've put descriptions in there to help. I'd probably start with adminExists because the other methods all involve a token which we haven't written code to generate yet, or decided where it will be stored. My thinking on the token is we will store it within a user record since they should have a 1-1 relationship, it's generated on login, used whenever the user is logged in instead of a password (in the background) and terminated when a user logs out.

MikeKeightley commented 5 years ago

@neilmusgrove Sure, will do - hopefully have some time in the office to have a play this afternoon. Storing the token against the user record sounds logical to me too. I'll try and get my database files into the repo tonight, as I think hooking up to that will be a big help for testing the user auth. Just might need a bit of guidance just to ensure I do it right - I've exported the collections as JSON objects already, as you mentioned the other day.

MikeKeightley commented 5 years ago

@neilmusgrove just a brief update, I've started writing some of the functions in the users.js Service, and having a look at creating tokens etc. I've been done some reading on cookieParser, and I see its installed in our package, but doesn't look like we have anything set up yet to create it against the user record? I'd like to have a look at this, and then maybe submit something in the next few days for review before merging into the master branch?

neilmusgrove commented 5 years ago

@MikeKeightley I'm not sure where cookie parser has come from but from what I was reading about cookie parsing in express it's the right package. We don't have anything yet to create a session token or save a session cookie. Basically we need to generate a long random string, save it to the DB against a user and save it to a session cookie that the browser will supply with each request. The session cookie part isn't the responsibility of the service but the token generation is. I'll maybe look at a user creation page or a login page next then and let you have a go at the service.

dosaki commented 5 years ago

Still to do: Authenticate against an actual user. @MikeKeightley will carry on working on this.

MikeKeightley commented 5 years ago

@neilmusgrove I've been fiddling with a few bits here and there, mainly looking at sessions, using express-session. Am I going down the right lines with this, or would you suggest something else/something simpler? Should be able to have some example code ready soon (probably for jst parts of it, not the whole process) - I'd like you guys to have a look before I commit it though :)

neilmusgrove commented 5 years ago

If you want us to look at anything create a branch and commit your changes on there, then when you push it everyone can have a look, even if it's work in progress. I did see express-session, it should do what we need. We don't have to use a session because we could lookup what we need on each request (stateless) however it may make a few things easier so happy to go with that. It's a good thing to understand the two approaches actually so maybe something to chat about on the next call.

MikeKeightley commented 5 years ago

Thanks Neil, will have a look tomorrow at adding it to a new branch (forgot about that to be honest!). I've gone down the session route so far, but I think you're definitely right about learning different approaches too.

MikeKeightley commented 5 years ago

@neilmusgrove I'm getting closer now, but having some trouble with my 'login' route. I am trying to call the userService 'userExists' and 'checkLogin' functions, rather than directly querying the DB as part of the route to check against the form input, and if they match, start a session, and redirect the user to /play screen. I'm not sure if I'm calling the two userServices functions correctly? I'm also getting an error now when i try to log in (using the dummy email and password that is already set up in the users.json file). I've attached a current snippet of the login route, and the error i get.

Screenshot 2019-10-30 at 14 30 18 Screenshot 2019-10-30 at 14 31 35
MikeKeightley commented 5 years ago

Crap. Just seen you comments on the branch @neilmusgrove , just after I created a pull request. Will look at your suggestions tonight!

neilmusgrove commented 5 years ago

@MikeKeightley Looks like you're moving in the right direction anyway from that PR. Try '../login' for the redirect for that error message and without the res.send. You don't want to send content and redirect, one is supplying content to the current page and the other is navigating away to another page.

MikeKeightley commented 5 years ago

Yep, that makes sense. Do I need to define 'login' somewhere (as part of res.session.login)? I'm getting an error relating to that specific line of code now. @neilmusgrove

Pleased I'm atleast moving in the right direction :)

n.b, I've commented out some of the code just to revert back to hard-coded email/password just incase it was something in there contributing to the error....doesn't seem to be though!

Screenshot 2019-10-30 at 18 31 45
neilmusgrove commented 5 years ago

That looks correct how you're trying to use it from a look at the docs.

MikeKeightley commented 5 years ago

Cancel that, it was because i removed my references to cookie-parser, yet the method in index.js for login was referencing the dependancies of cookie-parser

Yeah I thought so @neilmusgrove Although now i'm trying to troubleshoot it, and i'm just getting the below error - I'm not sure whats happened, as I've not altered anything since my last message last night, and I've not touch index.ejs.... any ideas Neil or @dosaki ?

Screenshot 2019-10-31 at 13 40 20

This happens when I try to load any page, or browse to localhost:3000 in my browser

dosaki commented 5 years ago

Sorry, only saw this now!

Glad you managed to sort it out

MikeKeightley commented 4 years ago

Maybe something here we can add as a new issue, but it could be related to the sessions:

When the user logs in, currently it's hard-coded into the header to show the users name at the top. at the moment, all we're capturing from the user is their email address and password. If we extend the 'register' form to capture firstname/surname, and also amend the DB to store these, in theory, we could probably grab the firstname/surname during the log in, and save it into the session. We can then pull from this from the session into the header to display the users name at the top? Not a top priority at the moment, but thought I'd mention whilst it was on my mind. @dosaki @neilmusgrove

dosaki commented 4 years ago

That makes sense.

It shouldn't need much code to do it, so while you're at it, you can just drop that in if you have time :)

MikeKeightley commented 4 years ago

@neilmusgrove As mentioned on whatsapp, I'm currently getting a 404 when attempting to login, using details created and saved in the DB using the registration form. If I browse to localhost:3000/bets/play, the correct screen appears. However, when using /bets/play as the redirect in my /login route presents the 404.

Screenshot 2019-11-05 at 21 14 32

I've also tried some other redirect combinations, but same result.

I've also made a couple of other changes which I am wondering could have had an impact. I have split the userForm.ejs into two separate forms now for Login and Register to account for having different input options on each form. Not sure if that would give me a 404 though?

dosaki commented 4 years ago

What does the URL look like when you end up in /bets/play via the /login route?

MikeKeightley commented 4 years ago

@dosaki when I enter the log in details and click the login button, I get the below;

Screenshot 2019-11-06 at 10 46 43
MikeKeightley commented 4 years ago

@dosaki @neilmusgrove don't worry, fixed it! I have a new problem now with sessions, but i'll investigate that myself before burdening you guys with it.

dosaki commented 4 years ago

Just got home now!

What was the problem?

dosaki commented 4 years ago

@MikeKeightley not sure if you've worked on these already but if you haven't make sure you resolve the TODOs in /users/login route (also below):

router.post("/login", async (req, res, next) => {
    let email = req.body.email;
    let password = req.body.password;
    const db = req.app.get("super6db");
    try {
        await userService.checkLogin(db, email, password);
        //TODO: Assign whatever we need to assign into the session
        res.redirect("/users/play");
    } catch (e) {
        if (e instanceof errors.InvalidCredentialsError) {
            res.redirect("/"); //TODO: Provide feedback to user that login was unsuccessful
        }
    }
});
MikeKeightley commented 4 years ago

@neilmusgrove @dosaki I'm still fiddling with different ideas of how to store user data into the session - and I've got something that works. However, I am wondering if I am using the best method. I am using a new service to query the DB to return all required fields for the session, and setting them in one session it (req.session.users). Will this be ok, does each item need to be its own individual req.session? The query stores them into an array - not sure if that makes any difference either?