Add authenticated property on Kuzzle object #390

Aschen · 2019-04-26T14:57:00Z

What does this PR do?

Add authenticated property on the Kuzzle object
Refactor the Auth controller to keep responsibility for the authentication token

How should this be manually tested?

Step 1 :
Step 2 :
Step 3 :

Other changes

Boyscout

codecov-io · 2019-04-26T15:08:35Z

Codecov Report

Merging #390 into 6-dev will decrease coverage by 0.12%.
The diff coverage is 92%.

@@            Coverage Diff             @@
##            6-dev     #390      +/-   ##
==========================================
- Coverage   96.44%   96.31%   -0.13%     
==========================================
  Files          31       32       +1     
  Lines        1489     1519      +30     
==========================================
+ Hits         1436     1463      +27     
- Misses         53       56       +3

Impacted Files	Coverage Δ
src/controllers/realtime/index.js	`100% <100%> (ø)`	⬆️
src/Kuzzle.js	`94.59% <81.81%> (-0.54%)`	⬇️
src/controllers/auth.js	`92.85% <94.11%> (-0.17%)`	⬇️
src/core/Jwt.js	`95.23% <95.23%> (ø)`

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 ff20858...a800b25. Read the comment docs.

alexandrebouthinon · 2019-04-29T11:33:51Z

src/Kuzzle.js

@@ -239,6 +247,7 @@ class Kuzzle extends KuzzleEventEmitter {

    this.protocol.addListener('tokenExpired', () => {
      this.jwt = undefined;
+      this.jwtExpiresAt = undefined;


It's just question from a JS noob, what is the difference between:

this.jwtExpiresAt = undefined;

and

delete this.jwtExpiresAt;

With this.jwt = undefined the function hasOwnProperty('jwt') will still return true but in this case it's change nothing

…to add-isLoggued

scottinet

👍

I like the handling the token's expiration time. But I think you need to go further, and add a timeout handler triggering a tokenExpired event whence the token expires

👎

isLogged is a bad name, prefer isAuthenticated, or perhaps isLoggedIn
I don't see the point of the isLogged function, since it's specified that the jwt property holds a value if a not-expired token is set. Note that I didn't say anything about the token being valid: the SDK doesn't know if the token is still valid unless a call to the API is made, if auth:checkToken is invoked, or if a realtime subscription is active. So, unfortunately, that new function doesn't bring added value to the already existing jwt getter.

scottinet · 2019-05-06T13:21:01Z

src/Kuzzle.js

+      return false;
+    }
+
+    return this.jwtExpiresAt > Date.now();


This falls short as soon as the jwt property is set with a custom token (i.e. saved from an earlier connection), which is a very, very common situation.

For this to work, you must also update the jwt setter, decode the token's payload and retrieve its timeout value.

scottinet

You need to update the jwt setter function, to maintain the new jwtExpiresAt property value

scottinet · 2019-05-07T07:20:00Z

src/Kuzzle.js

@@ -203,6 +203,10 @@ class Kuzzle extends KuzzleEventEmitter {
    return this.protocol.sslConnection;
  }

+  get authenticated () {
+    return this.jwt !== null && this.jwt !== undefined;


(nitpicking) according to the jwt setter code, that property can never be null

scottinet

Minor issues aside, this looks great 👍

src/Kuzzle.js

scottinet · 2019-05-17T09:37:28Z

src/controllers/auth.js

@@ -24,11 +58,15 @@ class AuthController extends BaseController {
   * @param  {string}   token     The jwt token to check
   * @return {Promise|*|PromiseLike<T>|Promise<T>}
   */
-  checkToken (token) {
+  checkToken (token = undefined) {


The default parameter is useless: it'll set token to undefined only if it is... undefined.

scottinet · 2019-05-17T09:40:17Z

src/controllers/auth.js

@@ -24,11 +58,15 @@ class AuthController extends BaseController {
   * @param  {string}   token     The jwt token to check
   * @return {Promise|*|PromiseLike<T>|Promise<T>}
   */
-  checkToken (token) {
+  checkToken (token = undefined) {
+    if (token === undefined && this.authenticationToken) {


(to be discussed) Personnally, I would throw if no token is defined or, perhaps better, I would let Kuzzle respond that it needs a token to be verified (like it was done before your changes)

And if users want to verify the stored token, they only need to use it like this: kuzzle.auth.checkToken(kuzzle.jwt)
Don't forget that the SDK automatically does an auth:checkToken on the stored jwt when a network connection is (re-)established.

I know this was the original behaviour but most of the time users want to check if the token stored by the SDK is still valid so IMHO they will understand that if you call this method without a specific token, it will check the SDK internal token.
Also it's more simpler to get ride of the kuzzle.jwt argument.
But maybe I can throw an error if checkToken is called without a token and if there is no internal token

The token is already automatically checked by the SDK, but... why not In that case, you have to document it properly in the checkToken SDK documentation

src/core/Jwt.js

test/mocks/generateJwt.mock.js

Co-Authored-By: Sébastien Cottinet <[email protected]>

…to add-isLoggued

scottinet · 2019-05-17T14:52:57Z

src/controllers/auth.js

@@ -24,11 +58,15 @@ class AuthController extends BaseController {
   * @param  {string}   token     The jwt token to check
   * @return {Promise|*|PromiseLike<T>|Promise<T>}
   */
-  checkToken (token) {
+  checkToken (token = undefined) {
+    if (token === undefined && this.authenticationToken) {


The token is already automatically checked by the SDK, but... why not In that case, you have to document it properly in the checkToken SDK documentation

src/core/Jwt.js

Aschen · 2019-05-20T09:29:19Z

@Yoann-Abbes and @alexandrebouthinon You might want to review this PR again since I extracted the JWT from Kuzzle object

## What does this PR do? Document kuzzleio/sdk-javascript#390 Based on #315 Original PR on v2: #297

Release 6.1.2 Bug fixes [ #398 ] Fix bulk return (Aschen) [ #394 ] Add default values for from/size to document:search (Aschen) [ #384 ] Fix search API: "sort" and "search_after" must be in the requests body (scottinet) Enhancements [ #390 ] Add authenticated property on Kuzzle object (Aschen) [ #395 ] Proxify kuzzle to avoid mistyping error (thomasarbona) [ #389 ] Remove usage of _meta (Aschen) [ #391 ] Add isConnected (Aschen) [ #388 ] Use BaseController class for controllers (Aschen) [ #385 ] Add Security.createRestrictedUser method (Aschen) Others [ #400 ] Fix large document search using scroll (stafyniaksacha) [ #387 ] SearchResult.next returns a new instance (Aschen)

Add isLoggued

011309a

Aschen added the changelog:enhancements label Apr 26, 2019

Aschen self-assigned this Apr 26, 2019

Aschen added 2 commits April 28, 2019 13:00

Offline isLoggued

ea134e1

unset jwtExpiresAt

799a2f2

alexandrebouthinon approved these changes Apr 29, 2019

View reviewed changes

jenow and others added 3 commits May 2, 2019 10:10

Merge branch '6-dev' into add-isLoggued

15e5801

fix tests

3a65501

Merge branch 'add-isLoggued' of github.com:kuzzleio/sdk-javascript in…

7011630

…to add-isLoggued

Yoann-Abbes approved these changes May 6, 2019

View reviewed changes

scottinet suggested changes May 6, 2019

View reviewed changes

rename to authenticated

dad0cb9

Aschen requested review from Yoann-Abbes, scottinet and alexandrebouthinon May 6, 2019 15:37

Aschen added the need-documentation label May 6, 2019

alexandrebouthinon approved these changes May 6, 2019

View reviewed changes

lint

c749274

Yoann-Abbes approved these changes May 7, 2019

View reviewed changes

scottinet suggested changes May 7, 2019

View reviewed changes

Extract jwt from kuzzle object

0f2e49e

Aschen requested review from scottinet, alexandrebouthinon and Yoann-Abbes and removed request for scottinet May 7, 2019 09:29

Aschen changed the title ~~Add isLoggued~~ Add authenticated property on Kuzzle object May 7, 2019

Aschen added 2 commits May 7, 2019 12:01

merge ok

a1bc297

add missing file

7e62b84

Aschen added 2 commits May 9, 2019 17:30

Merge branch '6-dev' into add-isLoggued

747aa08

fix tokenExpired event

404aaf1

scottinet suggested changes May 17, 2019

View reviewed changes

scottinet and others added 4 commits May 17, 2019 11:46

Merge branch '6-dev' into add-isLoggued

61763c3

Update test/mocks/generateJwt.mock.js

36eff29

Co-Authored-By: Sébastien Cottinet <[email protected]>

Throw error with invalid jwt

205840e

Merge branch 'add-isLoggued' of github.com:kuzzleio/sdk-javascript in…

3fe9fe9

…to add-isLoggued

scottinet suggested changes May 17, 2019

View reviewed changes

fix tests

28c869f

scottinet approved these changes May 20, 2019

View reviewed changes

alexandrebouthinon approved these changes May 20, 2019

View reviewed changes

Merge branch '6-dev' into add-isLoggued

a800b25

Aschen mentioned this pull request May 25, 2019

Document authenticated and connected kuzzleio/documentation#297

Closed

Aschen merged commit 69ea980 into 6-dev May 25, 2019

Aschen deleted the add-isLoggued branch May 25, 2019 16:11

Aschen mentioned this pull request Jun 3, 2019

Sdks authenticated connected props kuzzleio/documentation#316

Merged

scottinet pushed a commit to kuzzleio/documentation that referenced this pull request Jun 11, 2019

Sdks authenticated connected props (#316)

7bd8386

## What does this PR do? Document kuzzleio/sdk-javascript#390 Based on #315 Original PR on v2: #297

Aschen mentioned this pull request Jun 14, 2019

Release 6.1.2 #402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authenticated property on Kuzzle object #390

Add authenticated property on Kuzzle object #390

Aschen commented Apr 26, 2019 •

edited

Loading

codecov-io commented Apr 26, 2019 •

edited by codecov bot

Loading

alexandrebouthinon Apr 29, 2019

Aschen May 3, 2019

scottinet left a comment •

edited

Loading

scottinet May 6, 2019

scottinet left a comment

scottinet May 7, 2019

scottinet left a comment

scottinet May 17, 2019

scottinet May 17, 2019

Aschen May 17, 2019

scottinet May 17, 2019

scottinet May 17, 2019

Aschen commented May 20, 2019

Add authenticated property on Kuzzle object #390

Add authenticated property on Kuzzle object #390

Conversation

Aschen commented Apr 26, 2019 • edited Loading

What does this PR do?

How should this be manually tested?

Other changes

Boyscout

codecov-io commented Apr 26, 2019 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottinet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottinet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottinet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aschen commented May 20, 2019

Aschen commented Apr 26, 2019 •

edited

Loading

codecov-io commented Apr 26, 2019 •

edited by codecov bot

Loading

scottinet left a comment •

edited

Loading