Skip to content

Conversation

Aschen
Copy link
Contributor

@Aschen Aschen commented Jul 13, 2021

What does this PR do ?

When the SDK reconnect to Kuzzle, it trigger the reconnected event. The Realtime controller will try to resubscribe when this event is triggered.

If the token had expired, then the Realtime controller will try to resubscribe with no authentication and thus the subscriptions request may fail.

This PR include a new authenticator property, this property should contain a function that authenticate the SDK (with auth.login for example).
The SDK will call the function before emitting the reconnected event, if the SDK was authenticated and cannot re-authenticate then the reconnected event will not be emitted and the SDK will be in the disconnected state.

A new reconnectionError has been added and is triggered when the reconnection has failed

How should this be manually tested?

Create an user:

kourou security:createUser '{           
  content: {    
    profileIds: ["admin"]
  },                    
  credentials: {          
    local: {
      username: "test",
      password: "test"
    }
  }
}'

Then run this script:

const { Kuzzle, WebSocket } = require('./index');

const kuzzle = new Kuzzle(new WebSocket('localhost'), { autoResubscribe: true });

kuzzle.authenticator = async () => {
  await kuzzle.auth.login('local', { username: 'test', password: 'test' });
};

;(async () => {
  await kuzzle.connect();
  console.log(await kuzzle.authenticate());

  await kuzzle.realtime.subscribe('test', 'test', {}, () => {
    console.log('Received');
  });

  console.log('Subscribed');
})();

Disconnect your user kourou auth:logout -a global=true --username test --password test

Then stop and restart Kuzzle, the SDK should re-subscribe successfully

@Aschen Aschen changed the title Add authenticator function Add authenticator function used at reconnection Jul 13, 2021
@Aschen Aschen marked this pull request as draft July 13, 2021 08:41
@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #650 (9547360) into 7-dev (ea54b9f) will increase coverage by 0.71%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            7-dev     #650      +/-   ##
==========================================
+ Coverage   85.62%   86.34%   +0.71%     
==========================================
  Files          36       36              
  Lines        1635     1655      +20     
  Branches      297      301       +4     
==========================================
+ Hits         1400     1429      +29     
+ Misses        175      169       -6     
+ Partials       60       57       -3     
Impacted Files Coverage Δ
src/core/searchResult/SearchResultBase.ts 77.58% <ø> (ø)
src/protocols/WebSocket.ts 80.67% <ø> (ø)
src/Kuzzle.ts 87.04% <100.00%> (+4.12%) ⬆️
src/core/Jwt.js 96.15% <100.00%> (ø)

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 b3044e1...9547360. Read the comment docs.

@Aschen Aschen marked this pull request as ready for review July 13, 2021 09:55
Aschen added 2 commits July 13, 2021 20:15
@Aschen
Copy link
Contributor Author

Aschen commented Jul 16, 2021

@Shiranuit I couldn't find a way to know if the SDK was authenticated since with the cookie auth we don't have the JWT property. It was to tricky to put listener in auth.login and auth.logout methods because even with that the token may have expired by itself, or the SDK could have been authenticated with an API key directly set on the SDK instance.

All the re-authentication mechanism will only be executed if the authenticator property has been set

@Shiranuit
Copy link
Contributor

The only way I could think of without puting a listener in auth:login and ath:logout is to use auth:checkToken and compare if the kuid is the kuid of anonymous, if it's not then we can consider that the user was logged in

@Shiranuit
Copy link
Contributor

Shiranuit commented Jul 19, 2021

I was wondering, isn't that Breaking Change ?
Because since it would be mandatory to have the authenticator set otherwise reconnection will not work and will throw a reconnectionError, this means that older script will not work if there is a disconnection after upgrading to the new version unless they set the authenticator function, am I wrong ?

@Aschen
Copy link
Contributor Author

Aschen commented Jul 20, 2021

I was wondering, isn't that Breaking Change ?
Because since it would be mandatory to have the authenticator set otherwise reconnection will not work and will throw a reconnectionError, this means that older script will not work if there is a disconnection after upgrading to the new version unless they set the authenticator function, am I wrong ?

The new behavior will be executed only if the authenticator property is set so existing scripts won't be broken

The only way I could think of without puting a listener in auth:login and ath:logout is to use auth:checkToken and compare if the kuid is the kuid of anonymous, if it's not then we can consider that the user was logged in

But if the token is expired then kuid will be anonymous so we cannot know if the user was really authenticated or not.
That's why the whole re-authentication behavior will happen only if the authenticator property is manually set

@Shiranuit
Copy link
Contributor

The new behavior will be executed only if the authenticator property is set so existing scripts won't be broken

You're right I didn't see you changed that, LGTM

Copy link
Contributor

@Yoann-Abbes Yoann-Abbes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@Yoann-Abbes Yoann-Abbes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which authentication function could be in authenticator property instead of auth:login ?
Could it be interesting to only store strategy/credentials in an object inside authenticator and perform auth:login inside the sdk?

@Aschen
Copy link
Contributor Author

Aschen commented Jul 20, 2021

Which authentication function could be in authenticator property instead of auth:login ?
Could it be interesting to only store strategy/credentials in an object inside authenticator and perform auth:login inside the sdk?

It could be a call to an external API that manages secrets like Hashicorp Vault to request a new API key for example

@Aschen Aschen merged commit 2268913 into 7-dev Jul 21, 2021
@Aschen Aschen deleted the ensure-authentication-at-reconnection branch July 21, 2021 07:11
@Aschen Aschen mentioned this pull request Jul 21, 2021
Aschen added a commit that referenced this pull request Jul 21, 2021
# [7.7.2](https://github.com/kuzzleio/sdk-javascript/releases/tag/7.7.2) (2021-07-21)


#### Bug fixes

- [ [#651](#651) ] Hotfix heartbeat race condition   ([Shiranuit](https://github.com/Shiranuit))
- [ [#648](#648) ] Fix the Jwt.expired getter wrong comparison of micro timestamp and timestamp   ([robingrandval](https://github.com/robingrandval))
- [ [#646](#646) ] Fix usage of SearchResult.next with HTTP   ([Aschen](https://github.com/Aschen))
- [ [#644](#644) ] Correctly reject aborted queued requests   ([scottinet](https://github.com/scottinet))

#### Enhancements

- [ [#650](#650) ] Add authenticator function used at reconnection   ([Aschen](https://github.com/Aschen))
---
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.

3 participants