BasicIdToken files #17

anjuthomas · 2018-05-17T22:20:34Z

No description provided.

bojeil-google · 2018-05-23T08:26:16Z

src/oicMsg/tokenProfiles/basicIdToken.js

@@ -0,0 +1,105 @@
+'use strict';
+const Message = require('../message');


Where is the Message class defined?

It has not been reviewed it. Since you requested to have only one PR at a time I thought I
would start with BasicIdToken.

Hmm, we should have started with Message or Token since this depends on it. It's hard for me to review without knowing anything about these classes. Also your tests will fail and nothing will compile when you merge this PR. Would it be possible to send parallel PRs for these 2 classes.

bojeil-google · 2018-05-23T08:27:00Z

src/oicMsg/tokenProfiles/basicIdToken.js

@@ -0,0 +1,105 @@
+'use strict';
+const Message = require('../message');
+const Token = require('./token');


Where is token defined?

It has not been reviewed it. Since you requested to have only one PR at a time I thought I
would start with BasicIdToken.

Send me the PR for Token to review in parallel.

bojeil-google · 2018-05-23T08:29:16Z

src/oicMsg/tokenProfiles/basicIdToken.js

@@ -0,0 +1,105 @@
+'use strict';
+const Message = require('../message');


Just curious, if you are using ES6, why not use ES6 import instead of require:
import * as Message from '../message';

Good point. Will have to change this in a lot of places through out the library. I can make the change, but is this needed?

It would make sense to be consistent. You already implemented this in es6.
It would also make sense to do that early in the process. It will get harder later on. You can keep it as is for now and do it in a follow up.

Sounds good. On my todo list as need to add additional setting up to enable this.

bojeil-google · 2018-05-23T20:16:27Z

src/oicMsg/tokenProfiles/basicIdToken.js

+ * @class
+ * @constructor
+ * @extends Message
+ * @param {*} iss


The constructor seems to accept one parameter object {iss, sub, iat, jti}. The javadoc here is not correct.
Also shouldn't all these parameter types by string instead of *?

I don't see your change. Did you forget to commit it?

I haven't pushed the changes yet. Will update you once I push them

bojeil-google · 2018-05-23T20:30:16Z

src/oicMsg/tokenProfiles/basicIdToken.js

+    ];
+
+    /** Known required claims */
+    this.knownOptionalClaims = {


This property and the ones below it seem like they could be defined as an array like this.optionsForObjects.
forEach can also be used to iterate over an array.
Is this the format as defined in the Message class?

bojeil-google · 2018-05-23T23:20:27Z

test/basicId-claims-test.js

@@ -0,0 +1,226 @@
+var fs = require('fs');


You seem to be using ES5 for the unit tests. Why not stick to ES6?

Fixed. Will push the commit after the other tasks are complete

bojeil-google · 2018-05-24T00:30:03Z

test/basicId-claims-test.js

@@ -0,0 +1,226 @@
+var fs = require('fs');
+var path = require('path');
+var assert = require('chai').assert;


consider using chai.expect.
It is easier to assert if some call throws an error without having to try/catch.
eg:

expect(() => { failingCallback(); }).to.throw('Some error');

On my todo list

bojeil-google · 2018-05-24T00:36:11Z

test/basicId-claims-test.js

+                      },
+                      {'clockTimestamp': clockTimestamp})
+                } catch (err) {
+                  assert.isNull(err);


Are you expecting this to be called? It is not clear what the expectation here is. I would expect that you are testing that the function call succeeds.
A cleaner way is to use expect().to.throw() here and below

expect does not seem to work well with async functions compared to asserts

bojeil-google · 2018-05-24T00:54:51Z

test/basicId-claims-test.js

+                .catch(function(err) {
+                  assert.isNull(err);
+                });
+            done();


For asynchronous tests, done should be called when the promise completes. You are calling it prematurely ehre and elsewhere.

Fixed. Will push the commit after the other tasks are complete

bojeil-google · 2018-05-24T00:58:36Z

src/oicMsg/tokenProfiles/basicIdToken.js

+    });
+    basicIdToken.addOptionalClaims(optionalClaims);
+    if (options && Object.keys(options).indexOf('algorithm') !== -1 &&
+        options['algorithm'] === 'none') {


Can you also add a unit test for this to confirm algorithm is set to none automatically

BasicIdToken files

34c6450

anjuthomas requested a review from bojeil-google May 17, 2018 22:22

bojeil-google requested changes May 24, 2018

View reviewed changes

bojeil-google assigned anjuthomas May 24, 2018

anjuthomas added 2 commits May 29, 2018 00:27

BasicIdToken revised

57c2500

Revised test

05f86cf

		@@ -0,0 +1,105 @@
		'use strict';
		const Message = require('../message');

BasicIdToken files #17

Are you sure you want to change the base?

BasicIdToken files #17

Uh oh!

Conversation

anjuthomas commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjuthomas May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjuthomas May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjuthomas May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjuthomas May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjuthomas May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjuthomas May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjuthomas May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

anjuthomas May 24, 2018 •

edited

Loading

anjuthomas May 26, 2018 •

edited

Loading

anjuthomas May 26, 2018 •

edited

Loading

anjuthomas May 26, 2018 •

edited

Loading

anjuthomas May 29, 2018 •

edited

Loading

anjuthomas May 29, 2018 •

edited

Loading

anjuthomas May 26, 2018 •

edited

Loading