Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
202 changes: 202 additions & 0 deletions src/serviceContext.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
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 ATTRMAP = {
'userinfo': {
'sign': 'userinfo_signed_response_alg',
'alg': 'userinfo_encrypted_response_alg',
'enc': 'userinfo_encrypted_response_enc'
},
'id_token': {
'sign': 'id_token_signed_response_alg',
'alg': 'id_token_encrypted_response_alg',
'enc': 'id_token_encrypted_response_enc'
},
'request': {
'sign': 'request_object_signing_alg',
'alg': 'request_object_encryption_alg',
'enc': 'request_object_encryption_enc'
}
};

const DEFAULT_SIGN_ALG = {
'userinfo': 'RS256',
'request': 'RS384',
'id_token': 'ES384',
};

/**
* 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 = {};
this.registrationResponse = {};
this.kid = {'sig': {}, 'enc': {}};
this.config = config || {};
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;
}
}

this.client_id = this.config['client_id'] || defaultVal;
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.setClientSecret(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.

Change to

  this.setClientSecret(this.config['client_secret']);

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

this.callback = this.config['callback'] || {};

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

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 = '';
} 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.json.
* Relative to the directory from which the RP instance is run.
*
* @param {string} webName
Copy link
Collaborator

Choose a reason for hiding this comment

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

The description for the parameter is missing.

*/
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.

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

/**
* Reformat the crypto algorithm information gathered from a
* client registration response into something more palatable.
*
* @param {string} typ: 'id_token', 'userinfo' or 'request_object'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really 'request_object'? The ATTRMAP above currently just uses 'request'. Please update to match.

*/
signEncAlgs(typ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename typ into type. It's more intuitive.

let serviceContext = this;
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 and replace all occurrences of serviceContext. in this function with this..

let resp = {};
for (let i = 0; i < Object.keys(ATTRMAP[typ]).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.

How about using

for (const key of Object.keys(ATTRMAP[typ])) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might wanna check that the provided parameter really is one of the supported strings before using it, otherwise it could lead to unexpected behavior.

let key = Object.keys(ATTRMAP[typ])[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} Incorrect indentation. Below as well.

let val = ATTRMAP[typ][key];
if (serviceContext.registrationResponse && serviceContext.registrationResponse[val]){
Copy link
Collaborator

Choose a reason for hiding this comment

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

{style} Missing whitespace between ) and {.

resp[key] = serviceContext.registrationResponse[val];
}else if (key === 'sign') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

{style} Missing whitespace between } and else.

try {
resp[key] = DEFAULT_SIGN_ALG[typ];
} catch (err) {
return;
}
}
}
return resp;
}

/**
* Verifies that the algorithm to be used are supported by the other side.
* This will look at provider information either statically configured or
* obtained through dynamic provider info discovery.
*
* @param {string} alg The algorithm specification
* @param {string} usage In which context the 'alg' will be used.
* The following contexts are supported:
* - userinfo
* - id_token
* - request_object
* - token_endpoint_auth
* @param {string} typ Type of alg
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename this parameter into type. It's more intuitive.

* - signing_alg
* - encryption_alg
* - encryption_enc
*/
verifyAlgSupport(alg, usage, typ) {
let serviceContext = this;
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 and replace all occurrences of serviceContext. in this function with this..

let supported = serviceContext.providerInfo[usage + '_' + typ + '_values_supported'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if providerInfo is not set or the constructed key is not present in providerInfo?

if (supported.indexOf(alg) !== -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of this if statement you could simply write:

return (supported.indexOf(alg) !== -1);

return true;
} else {
return false;
}
}
}


module.exports.ServiceContext = ServiceContext;
Loading