Skip to content
New issue

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

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

Already on GitHub? # to your account

User entity missing on disconnect event #1995

Closed
rnbrady opened this issue Jun 19, 2020 · 5 comments · Fixed by #2813
Closed

User entity missing on disconnect event #1995

rnbrady opened this issue Jun 19, 2020 · 5 comments · Fixed by #2813

Comments

@rnbrady
Copy link

rnbrady commented Jun 19, 2020

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/authentication@4.5.2

  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/authentication@4.5.3

  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?
@astralmedia
Copy link

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

@daffl
Copy link
Member

daffl commented Jul 11, 2020

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 daffl added the Bug label Jul 11, 2020
@rnbrady
Copy link
Author

rnbrady commented Jul 13, 2020

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).

@astralmedia
Copy link

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.

@rnbrady
Copy link
Author

rnbrady commented Jul 14, 2020

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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants