Skip to content

Conversation

@jenow
Copy link
Contributor

@jenow jenow commented Feb 13, 2019

What does this PR do?

Don't send the jwt when doing a login action. This prevent to be able to do a login after a tokenExpired happen without resetting the jwt (kuzzle.jwt = null)

@jenow jenow self-assigned this Feb 13, 2019
@codecov-io
Copy link

codecov-io commented Feb 13, 2019

Codecov Report

Merging #372 into 6-dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            6-dev     #372   +/-   ##
=======================================
  Coverage   96.61%   96.61%           
=======================================
  Files          28       28           
  Lines        1417     1417           
=======================================
  Hits         1369     1369           
  Misses         48       48
Impacted Files Coverage Δ
src/Kuzzle.js 95.41% <ø> (ø) ⬆️

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 6115d24...deb9e07. Read the comment docs.

@scottinet
Copy link
Contributor

While ignoring the JWT on a login action is not a bad idea, I think there is another problem with the use case you mention.

Namely, the JWT should be unset after a tokenExpired event has been received: https://github.com/kuzzleio/sdk-javascript/blob/6-beta/src/Kuzzle.js#L93

So, if you're right about your use case, then it means that there is a bug with that event.

@scottinet
Copy link
Contributor

Ignore my previous comment: I'll fix that issue in another PR. After discussing the issue internally, not sending the JWT in login requests seems a reasonable thing to do.

@alexandrebouthinon alexandrebouthinon merged commit 6397ca7 into 6-dev Feb 14, 2019
@alexandrebouthinon alexandrebouthinon deleted the no-jwt-login branch February 14, 2019 13:13
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.

5 participants