Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
129 changes: 129 additions & 0 deletions src/serviceContext.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
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


/**
* This class keeps information that a client needs to be 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.
*/
class ServiceContext {
/**
* @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.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 || {};
let defaultVal = '';

if (params) {
for (var i = 0; i < Object.keys(params).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.

{style} Add a whitespace before '{'.

let key = Object.keys(params)[i];
let val = params[key];
this[key] = val;
}
}

this.client_id = this.config['client_id'] || defaultVal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought a bit more about the use of a variable for the default value, and I think in this case were you have two different default values it's slightly confusing. Therefore I would suggest to remove defaultVal and use '' or {} directly.

this.issuer = this.config['issuer'] || defaultVal;
this.client_secret = this.config['client_secret'] || 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 this line.

this.setclient_secret(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.

Replace this.client_secret with this.config['client_secret'] and remove the line above.

this.base_url = this.config['base_url'] || defaultVal;
this.request_dir = this.config['requests_dir'] || defaultVal;

defaultVal = {}
this.allow = this.config['allow'] || defaultVal;
this.client_prefs = this.config['client_preferences'] || defaultVal;
this.behavior = this.config['behaviour'] || defaultVal;
this.provider_info = this.config['provider_info'] || defaultVal;

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 = {};
}

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;
}

getclient_secret() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this change from getClientSecret to getclient_secret intentional?

return this.client_secret;
}

setclient_secret(val) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above. I think it wasn't your intention to also change this function name?

if (!val) {
this.client_secret = '';
} 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.
* @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.base_url);
if (!path.startsWith('/')){
return [this.base_url + '/' + path+ '/' + m.digest('hex')];
}else{
return [this.base_url + 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be of type 'string' rather than '*'?

*/
filenameFromWebName(webName) {
if (webName.startsWith(this.base_url) == 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.base_url.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