Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 177 additions & 0 deletions src/serviceContext.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
const State = require('./state.js').State;

This comment was marked as resolved.

This comment was marked as resolved.

const crypto = require('crypto');
const KeyJar = require('../nodeOIDCMsg/src/oicMsg/keystore/KeyJar').KeyJar;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The module location will change once you're done with the nodeOIDCMsg repo, right?
If so, could you already require the correct and to be expected location of the module here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would the module location change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, this wasn't too clear. What I meant is whether nodeOIDCMsg becomes a dependency of the nodeOIDCService at some point that is listed in the dependency section of the package.json file? If that's the case the path in this in this require statement would slightly change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On my todo list

const assert = require('chai').assert;
Copy link
Collaborator

@haamel haamel May 22, 2018

Choose a reason for hiding this comment

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

Unused. Remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


/**
* @fileoverview This class keeps information that a client needs to be
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that the @fileoverview is needed for this file, as it only contains a single class. Instead I would suggest to make this the class comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* able to talk to a server. Some of this information comes from
* configuration and some from dynamic provider info discovery or client
* registration. But information is also picked up during the
* conversation with a server.
*/

/**
* serviceContext
* @class
* @constructor
Copy link
Collaborator

Choose a reason for hiding this comment

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

The annotations @Class and @constructor are redundant here.

Copy link
Contributor Author

@anjuthomas anjuthomas May 22, 2018

Choose a reason for hiding this comment

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

have removed the @constructor to avoid redundancy

*/
class ServiceContext {
/**
* This class keeps information that a client needs to be able to talk
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the description, as it's the same as above and only keep the @param annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* to a server. Some of this information comes from configuration and some
* from dynamic provider info discovery or client registration.
* But information is also picked up during the conversation with a server.
* @param {KeyJar} keyjar OIDCMsg KeyJar instance that contains the RP signing and encyrpting keys
* @param {Object<string, string>} config Client configuration
* @param {Object<string, string>} params Other attributes that might be needed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mark at least this parameter as an optional one: http://usejsdoc.org/tags-param.html#optional-parameters-and-default-values

The syntax (JSDoc vs. Google Closure Compiler) should be the same as in the other repos you are working on.

*/
constructor(keyjar, config, params) {
this.clientSecret = [this.getClientSecret, this.setClientSecret];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove or what is this meant for?

keyjar = keyjar || null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be removed as it is pointless to initialize keyjar when you initialize this.keyjar properly just 2 lines later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

config = config || null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove. this.config will be initialized properly in line 26 regardless of this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

this.keyjar = keyjar || new KeyJar();
this.providerInfo = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed that several member variables are defined with similar naming, e.g.:

  • providerInfo vs. provider_info,
  • baseUrl vs. base_url,
  • requestDir vs. requests_dir,
  • cId vs. client_id,
  • cSecret vs. clientSecret vs. client_secret,
  • clientPreferences vs. client_prefs

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The providerInfo and provider_info is intentional. One refers to the class object and the other refers to config attr. All other names are unintentional and I have fixed it. Good catch ;)

this.registrationResponse = {};
this.kid = {'sig': {}, 'enc': {}};

this.config = config || {};

this.baseUrl = '';
this.requestDir = '';
this.allow = {};
this.behavior = {};
this.clientPreferences = {};
this.cId = '';
this.cSecret = '';
this.issuer = '';

let serviceContext =
['client_id', 'issuer', 'client_secret', 'base_url', 'requests_dir'];
let defaultVal = '';

if (params) {
for (var i = 0; i < Object.keys(params).length; i++) {
let key = Object.keys(params)[i];
let val = params[key];
this[key] = val;
}
}

for (let i = 0; i < serviceContext.length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason you are doing this through a loop? I find it quite hard to read and would rather prefer something like:

this.client_id = this.config['client_id'] || defaultVal;
this.issuer = this.config['issuer'] || defaultVal;
...

This would also have the benefit that you do not have to initialize the members with their default value, as done above. If that is the intention, due to the naming inconsistencies it is somewhat hard to tell ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Removed the for loop.

let attr = serviceContext[i];
if (attr === 'client_id') {
this.client_id = this.config[attr] || defaultVal;
} else if (attr === 'issuer') {
this.issuer = this.config[attr] || defaultVal;
} else if (attr === 'client_secret') {
this.client_secret = this.config[attr] || defaultVal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can combine this and the following line into a single one:
this.setClientSecret(this.config[attr]);

this.setClientSecret(this.client_secret);
} else if (attr === 'base_url') {
this.base_url = this.config[attr] || defaultVal;
} else if (attr === 'requests_dir') {
this.request_dir = this.config[attr] || defaultVal;
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the semicolon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


let providerInfo =
['allow', 'client_preferences', 'behaviour', 'provider_info'];
defaultVal = {};
for (let i = 0; i < providerInfo.length; i++) {
let attr = providerInfo[i];
if (attr === 'allow') {
this.allow = this.config[attr] || defaultVal;
} else if (attr === 'client_preferences') {
this.client_prefs = this.config[attr] || defaultVal;
} else if (attr === 'behaviour') {
this.behavior = this.config[attr] || defaultVal;
} else if (attr === 'provider_info') {
this.provider_info = this.config[attr] || defaultVal;
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the semicolon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be simplified to this.redirectUris = this.config['redirect_uris'] || [null];.

this.redirectUris = this.config['redirect_uris'];
} catch (err) {
this.redirectUris = [null];
}

try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be simplified to this.callback = this.config['callback'] || {};.

this.callback = this.config['callback'];
} catch (err) {
this.callback = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing semicolon at EOL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}

if (config && Object.keys(config).indexOf('keydefs') !== -1) {
this.keyjar = this.buildKeyJar(config['keydefs'], this.keyjar)[1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

this.buildKeyJar is not defined in this class.

}

return this;
}

getClientSecret() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a JSDoc to this function.

return this.client_secret;
}

setClientSecret(val) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a JSDoc to this function. Also mention in the JSDoc that a symmetric key is added to the keyjar and the reasons for it.

if (!val) {
this.client_secret;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you wanted to have the default value set when there is no 'val'. So, this should be:
this.client_secret = '';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

} else {
this.client_secret = val;
// client uses it for signing
// Server might also use it for signing which means the
// client uses it for verifying server signatures
if (this.keyjar == null) {
this.keyjar = new KeyJar();
}
this.keyjar.addSymmetric('', val.toString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a unit test that verifies that a symmetric key is added to the keyjar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On my TODO list

}
}

/**
* Need to generate a redirect_uri path that is unique for a OP/RP combo
This is to counter the mix-up attack.
Copy link
Collaborator

Choose a reason for hiding this comment

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

{style} Please put a '*' in front of this comment line to make it clear that it belongs to the block comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* @param {string} path Leading path
* @return A list of one unique URL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing the return type.

*/
generateRequestUris(path) {
let m = crypto.createHmac('sha256', '');
try {
m.update(this.providerInfo['issuer']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could probably shorten this try catch to the following:

m.update(this.providerInfo['issuer'] || this.issuer);

} catch (error) {
m.update(this.issuer);
}
m.update(this.baseUrl);
if (!path.startsWith('/')) {
return [this.baseUrl + '/' + path + '/' + m.digest('hex')];
} else {
return [this.baseUrl + path + '/' + m.digest('hex')];
}
}

/**
* A 1<->1 map is maintained between a URL pointing to a file and
* the name of the file in the file system.
*
* As an example if the base_url is 'https://example.com' and a jwks_uri
* is 'https://example.com/jwks_uri.json' then the filename of the
* corresponding file on the local filesystem would be 'jwks_uri'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the comment is not correct here. To match with the given example it should state:
"corresponding file on the local filesystem would be 'jwks_uri.json"

* Relative to the directory from which the RP instance is run.
*
* @param {*} webName
*/
filenameFromWebName(webName) {
if (webName.startsWith(this.baseUrl) == false) {
console.log('ValueError');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this lead to some error or invalid filename? Just logging a not very informative message and proceeding with the filename detection may lead to some unpredictable behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that just logging and still proceeding afterwards is the right thing to do here.
Maybe raise an error here or at least do not proceed further, as it will then lead to unexpected behavior.

}
let name = webName.substring(this.baseUrl.length, webName.length);
if (name.startsWith('/')) {
return name.substring(1, name.length);
} else {
let splitName = name.split('/');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add more unit tests for this function to cover all the various cases to determine the local filename.

Also cover the case, as mentioned above, when the webName doesn't start with the base_url.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On my TODO list

return splitName[splitName.length - 1];
}
}
}

module.exports.ServiceContext = ServiceContext;
Loading