Skip to content

Conversation

Aschen
Copy link
Contributor

@Aschen Aschen commented Jul 27, 2021

What does this PR do ?

When the SDK subscribe to realtime notification and the authentication expire, Kuzzle send a TokenExpired notification.

When the SDK receive this notification, it unsubscribe from every realtime subscriptions.

A proper re-authentification mechanism should be implemented instead when the authenticator property has been set

@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #653 (8daf2d8) into 7-dev (8c7b354) will decrease coverage by 0.50%.
The diff coverage is 64.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            7-dev     #653      +/-   ##
==========================================
- Coverage   86.16%   85.65%   -0.51%     
==========================================
  Files          36       36              
  Lines        1662     1708      +46     
  Branches      302      309       +7     
==========================================
+ Hits         1432     1463      +31     
- Misses        173      183      +10     
- Partials       57       62       +5     
Impacted Files Coverage Δ
src/controllers/Realtime.ts 87.87% <33.33%> (-2.60%) ⬇️
src/protocols/WebSocket.ts 75.93% <36.00%> (-2.64%) ⬇️
src/Kuzzle.ts 85.58% <78.04%> (-1.46%) ⬇️
src/controllers/Auth.ts 78.35% <100.00%> (+1.75%) ⬆️
src/protocols/abstract/Realtime.ts 97.29% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c7b354...8daf2d8. Read the comment docs.

@Shiranuit
Copy link
Contributor

Shiranuit commented Aug 2, 2021

What has been done

  • Fix loginAttempt event not being emitted when login has failed in cookieAuth mode
  • Added a new event called logoutAttempt that is emitted on logout the same way as loginAttempt
  • Kuzzle now store a state _loggedIn that is usefull to know if the SDK was previously logged in or not
  • Avoid reconnection loop when the SDK is in cookieAuth mode that were triggered by the socket renewal procedure
  • ReAuthentication using authenticator is now only done when we were previously logged in
  • Do not try to reauth when the reconnection is caused by the websocketRenewal

Goal

The ultimate goal of those changes is to only do a reauthentication when we were previously logged in, this is possible thanks to the events loginAttempt and logoutAttempt that can be used to know when a logout or login have been successfuly accomplished.
Thanks to those new event Kuzzle can then store a state that helps know if we were logged in.

Why doing a checkToken when the connected event is emitted ?

Simply because if we were using cookies to store our Token, the SDK will use it directly in every request and if the Token is still valid, every requests will be executed with the right of the user that the Token belong to, without the needs to do a login before.
This why we should check the validity of the token at the connection, because if the Token is still valid it's the same as doing a login before sending requests.

Why doing a checkToken if the loginAttempt has failed instead of just setting the _loggedIn value to false ?

This is also because of the cookieAuth mode, if a previous token is stored in the browser's cookies and we then want to login to another account and the login fails, the SDK will still use the previous token because it has not been replaced and we should verify the token validity.

@Shiranuit Shiranuit force-pushed the handle-token-expired-event branch from 76382cc to 6f90c28 Compare August 2, 2021 15:16
@Shiranuit Shiranuit self-assigned this Aug 2, 2021
@Shiranuit Shiranuit force-pushed the handle-token-expired-event branch from 0b913ed to 95900eb Compare August 2, 2021 16:27
@Shiranuit
Copy link
Contributor

Shiranuit commented Aug 3, 2021

Other Enhancements

Reconnection issue

There was a bug were the reconnection process was executed twice because calling websocketClient.close() in the HeartBeat to close the previous Websocket do call the websocketClient.onclose callback that was calling clientNetworkError since the closing status code was not 1000 this combined with the fact that the HeartBeat was also calling clientNetworkError, the method clientNetworkError was called twice.

clientNetworkError is the method called on connection lost, responsible of starting the reconnection mecanism.

This was the cause of a double reconnected event emitted.

Now the reconnected event is only emitted once.

Disconnected event emitted multiples times

I noticed that the disconnected was emitted multiple times while the SDK was trying to reconnect after a connection loss.
This behaviour was caused by the clientNetworkError emitting a disconnected event each time the connection retry had fail instead of only emitting the event once when a previously established connection was lost.

Now the disconnected event is only emitted once, when a previously established connection is lost.
The networkError event is still emitted at each reconnection attempt because it make sense to know what is the reason that has prevented the establishment of a new connection.

Why those changes ?

To me, it makes sense that the disconnected event is only emitted once, when a previously established connection is either closed or some network error has occured.
But the networkError can still be emitted multiple times because networkError can also occur during the connection process, unlike the disconnected event that can only be emitted after a connection has been established.

Shiranuit and others added 2 commits August 4, 2021 15:49
Co-authored-by: Alexandre Bouthinon <[email protected]>
Co-authored-by: Alexandre Bouthinon <[email protected]>
@Shiranuit Shiranuit merged commit 424e514 into 7-dev Aug 6, 2021
@Shiranuit Shiranuit mentioned this pull request Aug 6, 2021
@Aschen Aschen deleted the handle-token-expired-event branch August 9, 2021 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants