init #2

Amri91 · 2017-12-29T17:38:19Z

Fixes #1
Fixes #3
Fixes #4

Avaq

Great! :)
I'll get into reviewing the code once we have settled all meta issues.

Avaq · 2018-01-01T15:17:58Z

.eslintrc.json

@@ -0,0 +1,10 @@
+{


We should add "root": true to prevent extension of unrelated configs.

Avaq · 2018-01-01T15:49:37Z

.eslintrc.json

+{
+  "extends": ["warp/node", "warp/es6"],
+  "env": {
+    "jest": true,


I don't think it's appropriate to state that all code is run with jest. We should move this statement to test/.eslintrc.json.

You are correct, however, I would suggest having one eslintrc and use glob patterns instead so we don't need to write one config file per mocks and tests folders. Check the comment about jest's mocks folder location requirements. What do you think?

Avaq · 2018-01-01T15:49:54Z

.eslintrc.json

+  "extends": ["warp/node", "warp/es6"],
+  "env": {
+    "jest": true,
+    "mongo": true


What does the mongo environment add?

Nothing at all, I forgot to remove it :)

Avaq · 2018-01-01T15:51:35Z

.gitignore

@@ -57,3 +57,5 @@ typings/
 # dotenv environment variables file
 .env

+# .idea
+.idea


We should clean up the .gitignore to:

contain only files and directories specific to this project (eg build or installer output); and

use absolute paths by default.

Avaq · 2018-01-01T15:52:05Z

__mocks__/jsonwebtoken.js

+
+exports.sign = jest.fn(() => 'I am a token');
+
+exports.decode = jest.fn(() => ({userId: 123}));


Should we move the mocks directory under test?

Jest requires the mocks to exist in the same level as the modules to be mocked. Docs. But I see your point, having a mocks directory beside every mocked file might get messy.

Perhaps we can avoid this kind of mocking altogether by using dependency injection (as we discussed offline).

Avaq · 2018-01-01T15:53:02Z

index.js

@@ -0,0 +1,3 @@
+'use strict';
+
+module.exports = require('./jwtPlus');


Why not just have everything in this file? We can always change it once we need to split things up.

Avaq · 2018-01-01T15:54:18Z

package-lock.json

+          "dev": true
+        },
+        "semver": {
+


Using a package lock is not appropriate for libraries. Let's remove it and add ./.npmrc containing package-lock = false.

Avaq · 2018-01-01T15:55:28Z

package.json

@@ -0,0 +1,28 @@
+{
+  "name": "jwt-plus",
+  "version": "1.0.0",


Let's start at version 0.0.0 and increment once we publish. We can use https://github.com/davidchambers/xyz for automating this process. It's very nice and I use it on all my libraries.

Avaq · 2018-01-01T15:55:41Z

package.json

+{
+  "name": "jwt-plus",
+  "version": "1.0.0",
+  "description": "",


Let's think of a description.

miselaytes-anton · 2018-01-02T12:44:36Z

index.js

+
+  set algorithm(v) {
+    assert(v !== 'none', 'Algorithm none is not accepted');
+    this[_algorithm] = v;


interesting ) What benefit does using Symbol here bring?

It makes the property "private by obscurity".

Right, so you can never bypass the set function, nice

You can bypass it, it just takes more effort. It's the same principle as doing this._____________algorithm = ..., except that it's "more" obscure.
I discussed offline with Amri the possibility of using a closure to capture private properties, eg:

function JWTPlus({store, algorithm = 'HS512', expiresIn = regularTokenLife, jwt}) { return { setAlgorithm(x){ assert(v !== 'none', 'Algorithm none is not accepted'); algorithm = x } } }

hm, from what I read Symbol would create a unique value, which you can not know if its not on the scope. How can you bypass it?

const s1 = Symbol('s'); const s2 = Symbol('s'); console.log(s1 === s2);//false const a = {}; a[s1] = 5; console.log(Object.keys(a));//[]

const hasProp = Object.prototype.hasOwnProperty; const specialGet = (prop, object) => { if(hasProp.call(object, prop)) return object[prop]; const symbols = Object.getOwnPropertySymbols(object); return symbols .filter(x => x.toString().slice('Symbol('.length, -1) === prop) .map(x => object[x]) [0]; }; const o = { 'regular': 'public bar', [Symbol('special')]: 'private bar' }; console.log(specialGet('regular', o)) console.log(specialGet('special', o))

haha, well for people ready to go all this way to modify a private property, I mean just let them do it..

I think simply not documenting the property name is obscure enough. If you want a private variable, you can use a closure.

One of the major issues with JWT is its low misuse-resistance and this library tries amend that. If a user wants to get the symbol property that bad, I think that level of misuse-resistance is outside the scope.

However, I do see your point, it makes sense to use an _, a meant-to-be-private variable with the appropriate naming convention might be sufficiently misuse-resistant. I'll remove the use of Symbols and use _ instead.

Further more, Google JavaScript Style Guide warns against using Symbol and promotes the use of an _ along with an annotation for private fields.

Avaq · 2018-01-02T13:58:11Z

index.js

+const _algorithm = Symbol('_algorithm');
+const _expiresIn = Symbol('_expiresIn');
+
+const Payload = t.struct({


I think it's better to use interfaces (t.inter) over structs in most cases.

Avaq · 2018-01-04T13:31:19Z

.gitignore

@@ -57,3 +57,5 @@ typings/
 # dotenv environment variables file
 .env

+# package-lock
+package-lock.json


Let's replace all of the contents in this file by:

/node_modules/

All the other stuff is either environment specific (so belongs in ~/.gitignore) or not related to this project (such as bower_components, what's that doing here).

Avaq · 2018-01-04T13:50:41Z

index.js

+  }
+  get expiresIn() {
+    return this._expiresIn;
+  }


These two pairs of getters and setters are overwritten by plain values upon object construction. They are essentially unreachable code.

Actually, never mind 😳

Avaq · 2018-01-05T15:22:20Z

index.js

+}, 'Payload');
+
+// Todo: pending more secret-related opinions
+const Secret = t.refinement(t.String, s => s.length >= 20);


You could add a name to this refinement for clearer error messages.

Avaq · 2018-01-05T15:23:23Z

index.js

+});
+
+class JWTPlus {
+  // Todo: select default algorithm


What is this TODO for?

Currently, I am using HS512 as the default algorithm, I need to read more about recommended jwt algorithms, perhaps I should create an issue for this as well.

Avaq · 2018-01-05T15:23:55Z

index.js

+  userId: t.Any
+}, 'Payload');
+
+// Todo: pending more secret-related opinions


Let's remove this comment, and if it's still relevant, create a github issue instead.

Avaq · 2018-01-05T15:35:48Z

index.js

+    const refreshToken = generator.generate(256);
+    await this.store.registerTokens(userId, refreshToken, accessToken, ttl);
+    return refreshToken;
+  }


Missing newline

Avaq · 2018-01-05T15:36:41Z

index.js

+  // Todo: select default algorithm
+  /**
+   * Constructor
+   * @param {Object} store


What is the purpose of these comments? Are you going to generate documentation from them? If so, let's at least create an issue for it.

Yes, I am going to generate documentation from these comments, I'll add an issue. 👍

Avaq · 2018-01-05T15:42:56Z

index.js

+    });
+    const ttl = getTTL(rememberMe);
+    return getTokensObj(token,
+      ms(this.expiresIn),


Why do we represent TTL as strings?

Currently, this library accepts strings and numbers for this.expiresIn, I think strings are more readable, i.e. '1h' than the numbers, but the user can pass either. Is that what you meant?
I also refactored a bit so that we accept string and number formats but always use numbers internally. I will push soon.

As discussed offline

Avaq · 2018-01-05T15:44:22Z

index.js

+   * @param {Object} [verifyOptions] Options to pass to jwt.verify.
+   * @returns {Promise<*>}
+   */
+  verify(token, secret, verifyOptions) {


Is it necessary to pass down these options? Or can we provide our own?

I thought about asking the user to provide them once when calling the constructor, but I think these options may change between jwt method calls. I decided to give the user the option to pass them. Do you believe it is better not to?

As discussed offline

Avaq · 2018-01-05T15:45:27Z

index.js

+   * @returns {Promise<*>}
+   */
+  async refresh(refreshToken, oldToken, secret, rememberMe, signOptions) {
+    Secret(secret);


You could also type-check the other arguments, such as oldToken - even if you're just checking whether they are Strings.

Avaq · 2018-01-05T15:47:29Z

index.js

+   * @param {Object} [signOptions] Options passed to jwt.sign
+   * @returns {Promise<*>}
+   */
+  async refresh(refreshToken, oldToken, secret, rememberMe, signOptions) {


I don't think taking rememberMe here makes for a good API. It's puts some party (the front-end or back-end) in charge of remembering whether the user selected "remember me" when they logged in. Perhaps it's better to keep this information in the token, and just carry it forward.

Do we have to pass along the signOptions, or can we provide our own?

That's a great idea. 👍

Avaq · 2018-01-05T15:59:57Z

index.js

+        // Otherwise, put value in the options
+        options[optionMapping[key]] = inputToken[key];
+      }
+    });


You will have to explain why all this is necessary.

I'll add it in the documentations. But the idea is that some options are already stored in the token (we can also use this for rememberMe), so the refresher does not have to do any processing to generate the options again and the new token will almost match the old one.

It turned out that it is not :)

Avaq

It looks good to me. Let's squash the commits. :)

Amri91 · 2018-01-12T10:11:48Z

Fixes #7

Errors thrown now has the same structure as the ones thrown by jsonwebtoken (the default library used)

codecov · 2018-01-12T10:58:48Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f2bae6a). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master     #2   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      1           
  Lines             ?     49           
  Branches          ?      3           
=======================================
  Hits              ?     49           
  Misses            ?      0           
  Partials          ?      0

Impacted Files	Coverage Δ
index.js	`100% <100%> (ø)`

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 f2bae6a...a0d06d0. Read the comment docs.

Amri91 requested review from Avaq and miselaytes-anton December 29, 2017 17:38

Amri91 force-pushed the inital-commit branch from 6756c2e to e221f93 Compare December 29, 2017 18:11

Avaq reviewed Jan 1, 2018

View reviewed changes

miselaytes-anton reviewed Jan 2, 2018

View reviewed changes

Avaq reviewed Jan 2, 2018

View reviewed changes

Avaq requested changes Jan 4, 2018

View reviewed changes

Avaq reviewed Jan 4, 2018

View reviewed changes

Avaq reviewed Jan 5, 2018

View reviewed changes

Amri91 requested a review from dicearr January 8, 2018 12:35

Avaq reviewed Jan 8, 2018

View reviewed changes

inital commit

81da7b7

Amri91 force-pushed the inital-commit branch from a4b50b9 to 81da7b7 Compare January 8, 2018 14:49

conform errors to match jwt's

a0d06d0

Amri91 force-pushed the inital-commit branch from 236605c to a0d06d0 Compare January 12, 2018 10:52

Avaq approved these changes Jan 12, 2018

View reviewed changes

Amri91 merged commit 9e05268 into master Jan 12, 2018

Amri91 deleted the inital-commit branch January 12, 2018 12:25


		exports.sign = jest.fn(() => 'I am a token');

		exports.decode = jest.fn(() => ({userId: 123}));

		@@ -0,0 +1,3 @@
		'use strict';

		module.exports = require('./jwtPlus');

init #2

init #2

Conversation

Amri91 commented Dec 29, 2017 • edited Loading

Avaq 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

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

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

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

miselaytes-anton Jan 2, 2018 • edited Loading

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

Amri91 Jan 4, 2018 • edited Loading

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

Avaq Jan 4, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

Amri91 Jan 5, 2018 • edited Loading

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

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

Avaq left a comment

Choose a reason for hiding this comment

Amri91 commented Jan 12, 2018

codecov bot commented Jan 12, 2018

Codecov Report

Amri91 commented Dec 29, 2017 •

edited

Loading

miselaytes-anton Jan 2, 2018 •

edited

Loading

Amri91 Jan 4, 2018 •

edited

Loading

Avaq Jan 4, 2018 •

edited

Loading

Amri91 Jan 5, 2018 •

edited

Loading