Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Socket.io authentication tests and login logout event #324

Merged
merged 4 commits into from Oct 24, 2016

Conversation

Projects
None yet
3 participants
@daffl
Copy link
Member

daffl commented Oct 23, 2016

This pull request adds further tests for Socket.io authentication and implements login and logout events for it. Now it is possible to do this:

app.on('login', function(authResult, info) {
  // authResult -> result from app.authentication.authenticate
  // info -> { socket, connection, provider }
});

app.on('logout', function(authResult, info) {
  // authResult -> result from app.authentication.authenticate
  // info -> { socket, connection, provider }
});

Getting the info object is useful so that we can e.g. keep the connection up to date with the latest user information (or let people do other things outside of the abstracted service flow).

@marshallswain
Copy link
Member

marshallswain left a comment

I had a few questions in the review process.

src/base.js Outdated
@@ -4,6 +4,8 @@ import middlewares from './middleware/authentication';

const debug = Debug('feathers-authentication:authentication:base');

// A basic Authentication class that allows to create and verify JWTs
// and also run through

This comment has been minimized.

@marshallswain

marshallswain Oct 23, 2016

Member

// and also run through what?

src/base.js Outdated
@@ -16,6 +18,7 @@ export default class Authentication {
this._middleware.isInitial = true;
}

// Register on or more handlers for the JWT verification chain

This comment has been minimized.

@marshallswain

marshallswain Oct 23, 2016

Member

on -> one

// Add the token to the connection so that it shows up as `params.token`
connection.token = token;

app.emit('login', result, {

This comment has been minimized.

@marshallswain

marshallswain Oct 23, 2016

Member

How do you filter this? Is there an app.filter() method?

This comment has been minimized.

@marshallswain

marshallswain Oct 23, 2016

Member

Also, these events are already being emitted in the service, here: https://github.com/feathersjs/feathers-authentication/blob/socketio-auth/src/service.js#L17

Would it be better to only emit at only one of these?

This comment has been minimized.

@daffl

daffl Oct 23, 2016

Author Member

We can probably take the ones in the service out since it doesn't have information about the connection and the provider. There is also no need to filter those. They only happen on the server on the app object, they are not service events.

This comment has been minimized.

@marshallswain

marshallswain Oct 23, 2016

Member

Also, on line 75 in this same file it says socket.on(disconnect, logout); Should disconnect be a string?

This comment has been minimized.

@marshallswain

marshallswain Oct 23, 2016

Member

Ok, so to emit the logout message to a single user, would we do this:

app.on('logout', (data, {socket}) => {
  socket.emit('logout', data);
});

This comment has been minimized.

@ekryski

ekryski Oct 24, 2016

Member

@daffl so you don't want to use app.service('authentication').on('login') and app.service('authentication').on('logout')? Is that because they don't have the provider context?

This comment has been minimized.

@daffl

daffl Oct 24, 2016

Author Member

@ekryski Yeah. Those events with the provider context seemed the best way to e.g. support keeping connection.user up to date when it changes during connection (which I'm working on right now).
@marshallswain I wouldn't say those events should be propagated to the outside. I was more thinking using them for things like

app.on('logout', (data, {socket}) => {
  const user = data.user;

  app.service('users').patch(user._id, {
    online: false
  });
});

This comment has been minimized.

@marshallswain

marshallswain Oct 24, 2016

Member

I only ask because it's what is wanted for bitcentive. Will that send to only the authenticating or logging out connection if I do that?

This comment has been minimized.

@ekryski

ekryski Oct 24, 2016

Member

@daffl sounds good. Hopefully, because it's not on a service people won't be confused about it being dispatched outside of the server. I agree with you, I don't think they should be available to a client.

This comment has been minimized.

@ekryski

ekryski Oct 24, 2016

Member

@marshallswain we had talked about that but it's redundant. The client will get an ack for the authenticate event it sends to the server, and same for logout so we don't really need to emit those events to the client as well.

});
});

it('app login and logout events', done => {

This comment has been minimized.

@ekryski

ekryski Oct 24, 2016

Member

I'd prefer to have 1 assertion per it block and have these wrapped in describe functions to group them together. It's a more boilerplate but when the tests are run (and fail) it makes it much easier to discern what the tests are actually doing and what part of the test is failing. Here is a good example of what I mean.

It's something I picked up from some experienced devs when I was at KISSmetrics and with large test suites and many contributors it makes adding new tests much more straightforward for people because the tests are only asserting 1 thing and usually are a lot DRYer.

This comment has been minimized.

@daffl

daffl Oct 24, 2016

Author Member

That test was indeed doing too much, so I split it up. I guess the block of

        expect(result.token).to.exist;
        expect(result.payload).to.exist;
        expect(result.payload.userId).to.equal(0);
        expect(result.authenticated).to.be.ok;
        expect(result.user).to.deep.equal({ id: 0, name: 'Tester' });

        expect(info.provider).to.equal('socketio');
        expect(info.socket).to.exist;

Technically still counts as one assertion since we are verifying the single object we are getting back, right?

This comment has been minimized.

@ekryski

ekryski Oct 24, 2016

Member

I typically still break them up into one expect per it block mostly because if you change the result coming back or remove an attribute it's easier to determine which test needs to change.

However, that only applies if the setup cost is pretty negligible, which it might not be in this particular case. Otherwise repeated setup that is expensive can quickly balloon build times.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Oct 24, 2016

So I think other than those typos @marshallswain pointed out this is good to 🚢

daffl added some commits Oct 24, 2016

delete e.stack;
delete e.type;

expect(e).to.deep.equal({

This comment has been minimized.

@ekryski

ekryski Oct 24, 2016

Member

Minor comment. I usually just check for the error code. That way if we ever change any of that other stuff we don't need to update a ton of tests.

This comment has been minimized.

@daffl

daffl Oct 24, 2016

Author Member

Word. I'll update that as well.

@daffl daffl merged commit 5e5b6a1 into modularization Oct 24, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@daffl daffl deleted the socketio-auth branch Oct 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.