Socket reconnect upgrade auth #3
Conversation
This adds a `getSocketMethodName` and moves the `method` variable up a level in the authenticate method, so that sockets can be authenticated inside the `handleResponse` callback. `handleResponse` is probably the best place for this because we only want it to happen after successful auth.
but I can’t even get this simple socket communication to work. What am I doing wrong?
| @@ -132,4 +132,14 @@ describe('Socket.io client authentication', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| it.only('handles upgrade events', done => { | |||
There was a problem hiding this comment.
@marshallswain @daffl do you know how to simulate upgrade events? I can't get this test to pass either but I don't think it is actually testing upgrade functionality anyway.
There was a problem hiding this comment.
Nope. I couldn't figure it out.
…utils to passport class
|
@feathersjs/core-team ready for review. Because I was fighting context issues I've moved all the utils to the In the event that your access token expires while you are disconnected from the server the automatic re-authentication will fail. Since we are using promises to keep the API clean that error would be swallowed. So I've now introduced 2 events you can listen for on the client.
|
|
Wouldn't a single |
|
@daffl I thought about it but when talking to @corymsmith we thought it might be confusing for people because these are special case auth errors and if it was just called These really are edge cases for when your access token becomes invalid when disconnected or your server has a bug in the authentication flow that causes it to continually error. I've added docs to readme about usage. If you can come up with a decent name I'm open to consolidating so you don't need to register 2 listeners. I was thinking maybe |
| passport = new Passport(app, options); | ||
| }); | ||
|
|
||
| describe.skip('getJWT', () => { |
There was a problem hiding this comment.
Are these tests still supposed to be skipped?
There was a problem hiding this comment.
Yes and no. I was being lazy because they need different setup and teardown now that they are part of the Passport class. That whole class needs more unit tests but I figured I would come back to them.
There was a problem hiding this comment.
Gotcha, all good, was more curious if you accidentally left the .skip in there
|
At first glance this all looks good to me, I think maybe we can consolidate those 2 events into one as you suggested, maybe |
|
@corymsmith @daffl consolidated the events now so it is just @corymsmith suggested. |
|
I say 🚢 this bad boy |
|
released as v0.1.1 |
The client needs to authenticate upon reconnect or transport upgrade.
Related to feathersjs-ecosystem/authentication#272
I can't figure out how to test the upgrade message. Can't even get basic socket communication to work. What am I doing wrong?