Add Multi Session and Session Revoke #182

gregfrasco · 2017-10-23T20:47:00Z

In relation to #180

You can have multiple session.
You the same session on the same device with the same ip is not deleted and re issued, it is simply return back.
User revoke there sessions. I am not sure if I did this correctly.
Admin can revoke any session.

Possible Issue: You could revoke your own session and be locked out, until you login onto another system an un-revoke that session. What are your thoughts on this?

jedireza

This is great! Thanks for creating a PR. I've left some minor change requests and brought up some questions. I have not tested locally yet, just a quick code review.

Possible Issue: You could revoke your own session and be locked out, until you login onto another system an un-revoke that session. What are your thoughts on this?

Maybe we can disabled the ability to revoke the current session to avoid this.

jedireza · 2017-10-24T02:41:56Z

package.json

    "nodemailer": "4.x.x",
    "nodemailer-markdown": "1.x.x",
    "object-assign": "4.x.x",
    "slug": "0.9.x",
+    "useragent": "^2.2.1",


"useragent": "2.x.x",

jedireza · 2017-10-24T02:42:30Z

server/api/login.js

+                        });
+                    }
+                    else if (existingSession.revoked) {
+


Remove this empty line.

jedireza · 2017-10-24T02:43:39Z

server/api/login.js

+                    }
+                    else if (existingSession.revoked) {
+
+                        return reply(Boom.forbidden('Session is revoked'));


End sentence with a period.

Ex: return done(Boom.forbidden('Session is revoked.'));

I'm not sure what's better; keeping the revoked session or just deleting them when they're revoked.

I think if we just delete the session, it would turn into a session not found, and we shouldn't to handle these cases.

This probably isn't the best way to do session blocking, since you could easy get around it. I will update it to remove the session, and possibly do session blocking in another pull request down the road.

jedireza · 2017-10-24T02:46:25Z

server/api/signup.js

@@ -142,7 +142,12 @@ internals.applyRoutes = function (server, next) {
                }],
                session: ['linkUser', 'linkAccount', function (results, done) {

-                    Session.create(results.user._id.toString(), done);
+                    const userAgent = Useragent.lookup(request.headers['user-agent']);
+                    const ip = request.info.remoteAddress;


In the case of environments like Heroku, and others that are fronted by a load balancer, we'd want to check that first.

Ex: const ip = request.headers['x-forwarded-for'] || request.info.remoteAddress;

jedireza · 2017-10-24T02:51:07Z

server/auth.js


-                    if (!results.session) {
+                    if (results.session && results.session.revoked) {
+                        return done(Boom.forbidden('Session has been revoked'));


End sentence with a period.

Ex: return done(Boom.forbidden('Session has been revoked.'));

jedireza · 2017-10-24T02:53:05Z

server/models/session.js

@@ -33,7 +33,7 @@ class Session extends MongoModels {
        });
    }

-    static create(userId, callback) {
+    static create(userId, ip, browser, os, callback) {


Instead of passing browser and os, maybe we could pass the useragent string all the way down to where it's used and then use the useragent module to parse it there... or we could send down the parsed useragent object and use the parts of it we're interested in.

jedireza · 2017-11-02T03:40:54Z

server/api/login.js

@@ -83,10 +83,17 @@ internals.applyRoutes = function (server, next) {
                    });
                }
            }, {
-                assign: 'session',
+                assign: 'existingSession',


During a login request I don't think we should be looking up the existing session. As an API consumer, when I create a new login request, I would expect a new session to be created. I'm not sure anything needs to change for this handler.

jedireza · 2017-11-02T03:41:32Z

server/api/sessions.js

@@ -84,6 +84,35 @@ internals.applyRoutes = function (server, next) {

    server.route({
        method: 'DELETE',
+        path: '/sessions/my/{id}',


We're accepting an id url parameter, but we never use it.

jedireza · 2017-11-02T03:42:19Z

server/api/sessions.js

+        },
+        handler: function (request, reply) {
+
+            const id = request.auth.credentials.roles.account._id.toString();


I'm not sure how we're finding a session by the account id of the user. What if they're an admin?

I think you meant to use the id url param from above?

jedireza · 2017-11-02T03:44:53Z

server/auth.js

+                            lastLogin: new Date()
+                        }
+                    };
+                    Session.findByIdAndUpdate(results.session._id.toString(),update, done);


This auth strategy runs for every request, not just during a login event. So I'm not sure that updating the lastLogin field of the session record is appropriate. If this updated something like lastActive that would make more sense. What was the intent?

Yes, that was the intent, lastActive is a better term

jedireza · 2017-11-02T03:45:13Z

server/models/session.js


        const self = this;

        Async.auto({
            keyHash: this.generateKeyHash.bind(this),
            newSession: ['keyHash', function (results, done) {

+                const Useragent = require('useragent');


require statements should be at the top of the file with the others.

jedireza · 2017-11-02T03:49:53Z

server/models/session.js


        const self = this;

        Async.auto({
            keyHash: this.generateKeyHash.bind(this),
            newSession: ['keyHash', function (results, done) {

+                const Useragent = require('useragent');
+                userAgent = Useragent.lookup(userAgent);


Would it be better to create a new variable here (const parsedAgent = Useragent.lookup(userAgent); maybe) instead of clobbering the argument passed in.

jedireza · 2017-11-02T03:51:06Z

server/models/session.js

+    lastLogin: Joi.date().required(),
+    ip: Joi.string().required(),
+    browser: Joi.string().required(),
+    os: Joi.string().required()


How should we handle someone not passing in a user-agent header? Let's say they logged in via Postman or another custom client?

Logged though postman native app, with recent change, returns back:

"browser": "PostmanRuntime/6.4.1", "os": "Other 0.0.0",

Ok maybe Postman was a bad example 😆, I'm curious though what happens if a user-agent header isn't provided.

I think it just says "Other" from the parser as Browser and os is blank

jedireza

This is looking good. I left some comments and asked some questions.

Could you also rebase your work based on the latest master?

Pull from Frame

jedireza · 2017-11-03T03:22:44Z

We should also add a GET /sessions/my endpoint so we can get a list of that users sessions.

jedireza · 2017-11-11T21:06:30Z

Thanks for adding the session/my support. Is this ready for a final review? Are you planning on adding anything else?

gregfrasco · 2017-11-12T02:14:11Z

@jedireza This one is done. If I add another feature, I'll open another pull request.

jedireza

This worked well in my testing. I found a couple things we need to address before merging. Thanks again for working through this. 👍

jedireza · 2017-11-14T06:01:29Z

server/models/session.js

@@ -111,7 +114,11 @@ Session.schema = Joi.object({
    _id: Joi.object(),
    userId: Joi.string().required(),
    key: Joi.string().required(),
-    time: Joi.date().required()
+    time: Joi.date().required(),
+    lastLogin: Joi.date().required(),


This should be lastActive.

jedireza · 2017-11-14T06:02:03Z

server/auth.js

+                        $set: {
+                            lastActive: new Date()
+                        }
+                    };


add new line

This has been added but does not close the comment

jedireza · 2017-11-14T06:02:18Z

server/auth.js

+                            lastActive: new Date()
+                        }
+                    };
+                    Session.findByIdAndUpdate(results.session._id.toString(),update, done);


Add space before update argument.

jedireza · 2017-11-14T06:08:05Z

server/auth.js

+                }],
+                updateSession: ['scope', function (results, done) {
+
+                    if (!results.roles) {


This should be checking for if (!results.scope) { since the scope and roles tasks run in parallel and we can't guarantee results.roles will be around at this point.

I noticed this in my testing after I realized the lastActive timestamp wasn't being updated.

jedireza · 2017-11-14T06:10:00Z

server/api/sessions.js

+        config: {
+            auth: {
+                strategy: 'simple',
+                scope: 'account'


This shouldn't be limited to a scope. Admin users should be able to delete their sessions too.

jedireza · 2017-11-14T06:12:15Z

server/api/sessions.js

+                strategy: 'simple',
+                scope: 'account'
+            }
+        },


I thought we were going to use a pre handler that prevented someone from DELETEing their current session. We should add that.

Note: The conventional way of deleting your current session is to use the /api/logout endpoint.

jedireza · 2017-11-14T06:14:17Z

server/api/signup.js

@@ -4,7 +4,6 @@ const Boom = require('boom');
 const Config = require('../../config');
 const Joi = require('joi');

-


leave this empty line

…feedback

Add Multi Session and Session Revoke

9e5a5b9

gregfrasco changed the title ~~Add Multi Session and Session Revoke #180~~ Add Multi Session and Session Revoke Oct 23, 2017

jedireza reviewed Oct 24, 2017

View reviewed changes

Remove session revoking, updated based on feedback in #182

d4ab66b

jedireza reviewed Nov 2, 2017

View reviewed changes

gregfrasco and others added 2 commits November 2, 2017 09:37

Merge pull request #1 from jedireza/master

c5b024b

Pull from Frame

Fix based on comments in #182

c265a3a

add session/my

a82259a

jedireza reviewed Nov 14, 2017

View reviewed changes

Add error response if your delete your own session. Updated based on …

a038fbe

…feedback

jedireza merged commit 85361f3 into jedireza:master Nov 21, 2017

jedireza mentioned this pull request Nov 21, 2017

Support multiple user sessions #180

Closed

jedireza mentioned this pull request Feb 3, 2018

is it useful ? #196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Multi Session and Session Revoke #182

Add Multi Session and Session Revoke #182

gregfrasco commented Oct 23, 2017 •

edited

Loading

jedireza left a comment •

edited

Loading

jedireza Oct 24, 2017

jedireza Oct 24, 2017

jedireza Oct 24, 2017

jedireza Oct 24, 2017

gregfrasco Oct 24, 2017 •

edited

Loading

jedireza Oct 24, 2017

jedireza Oct 24, 2017

jedireza Oct 24, 2017

jedireza Nov 2, 2017 •

edited

Loading

jedireza Nov 2, 2017

jedireza Nov 2, 2017

gregfrasco Nov 2, 2017

jedireza Nov 2, 2017

gregfrasco Nov 2, 2017

jedireza Nov 2, 2017

jedireza Nov 2, 2017

jedireza Nov 2, 2017

gregfrasco Nov 2, 2017

jedireza Nov 2, 2017

gregfrasco Nov 2, 2017

jedireza left a comment

jedireza commented Nov 3, 2017

jedireza commented Nov 11, 2017

gregfrasco commented Nov 12, 2017

jedireza left a comment

jedireza Nov 14, 2017

jedireza Nov 14, 2017

gregfrasco Nov 17, 2017

jedireza Nov 14, 2017

jedireza Nov 14, 2017

jedireza Nov 14, 2017

jedireza Nov 14, 2017

jedireza Nov 14, 2017

		@@ -4,7 +4,6 @@ const Boom = require('boom');
		const Config = require('../../config');
		const Joi = require('joi');

Add Multi Session and Session Revoke #182

Add Multi Session and Session Revoke #182

Conversation

gregfrasco commented Oct 23, 2017 • edited Loading

jedireza left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregfrasco Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedireza Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedireza left a comment

Choose a reason for hiding this comment

jedireza commented Nov 3, 2017

jedireza commented Nov 11, 2017

gregfrasco commented Nov 12, 2017

jedireza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregfrasco commented Oct 23, 2017 •

edited

Loading

jedireza left a comment •

edited

Loading

gregfrasco Oct 24, 2017 •

edited

Loading

jedireza Nov 2, 2017 •

edited

Loading