feathers icon indicating copy to clipboard operation
feathers copied to clipboard

User entity missing on disconnect event

Open rnbrady opened this issue 4 years ago • 5 comments

PR #1889 for issue #1883 has introduced a potential problem whereby connection.user is missing on the disconnect event, for example when a logged in user closes their browser tab.

See here for Slack thread where this arose.

Further complicating things is that this issue depends on a race condition between the disconnect event handlers in channels.ts and @feqathersjs/authentication/src/service/ts.

Steps to reproduce

  1. yarn add @feathersjs/[email protected]

  2. Add the following code to channels.js

  setTimeout(() => {
    app.on('disconnect', (connection: any) => {
      console.log(connection.user
        ? 'User entity found'
        : 'No user entity found'
      );
    });
  }, 100); 

Note: the delay via setTimeout is to ensure this event listener is registered after the event listener in the authentication module. If it is registered first the issue will not manifest.

  1. From a new tab in browser connect and login with client

  2. Close browser tab. Output reads: User entity found.

  3. yarn add @feathersjs/[email protected]

  4. Repeat 3 & 4. Output reads: No user entity found.

Expected behavior

connection.user available in disconnect event handler if user is logged in.

Actual behavior

connection.user is not available.

Notes

  1. I'm curious about the reasons for using disconnect event instead of logout for expired JWTs.
  2. Shouldn't hooks and services use connection.authentication to determine login status rather than connection.user?

rnbrady avatar Jun 19 '20 11:06 rnbrady

Thanks for adding this! Would love to know if anyone has an idea for a potential work around for now

astralmedia avatar Jun 19 '20 17:06 astralmedia

After looking into it a little bit, I think the safest way is to use app.prependListener instead.

app.prependListener('disconnect', (connection: any) => {
      console.log(connection.user
        ? 'User entity found'
        : 'No user entity found'
      );
    });

Should do it. If so we can add it as a note to the docs.

daffl avatar Jul 11 '20 18:07 daffl

Oh wow that's a very useful function to know about and sounds like a good workaround for @astralmedia.

But don't you think app.on('disconnect', ...) should just work? I think there's a conflation of disconnect and logout semantics, possibly here.

If a user disconnects without logging out, the data structures should reflect that. If user logs out without disconnecting, the data structures should reflect that. They are not the same thing.

(I'm just clarifying what I was reporting, I don't mind what you conclude as this issue doesn't impact me or my project).

rnbrady avatar Jul 13 '20 18:07 rnbrady

Thank you for looking into this.

I put the app.prependListener in my channels.js file, and while closing a connected browser does trigger that event, it still does not show any user data...

Just to clarify, when I connect, I hit socket.emit('create', 'authentication', authRequest) to get the users token...then pass that token along with all further requests...

Is this the proper way of handling auth? I am starting to think maybe I am doing something wrong on my end since the socket never knows the user.

astralmedia avatar Jul 13 '20 19:07 astralmedia

I think it's fair to say your problem is not related to this issue. I got that wrong.

when I connect, I hit socket.emit('create', 'authentication', authRequest) to get the users token ...then pass that token along with all further requests...Is this the proper way of handling auth?

That should work. For further requests over socketio you don't need to include the token as the connection itself is authenticated. But if you reconnect after a disconnection you would then do socket.emit('create', 'authentication', { strategy: "jwt", accessToken })

Your best chance to solve this is to step through your code yourself with a debugger. If you're not using it already the VS Code debugger will change your life.

Alternatively make your issue reproducible by others and we can investigate further.

rnbrady avatar Jul 14 '20 12:07 rnbrady