Skip to content
Open
Changes from 1 commit
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
265 changes: 265 additions & 0 deletions src/oicMsg/message.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,265 @@
'use strict';
const jwtDecoder = require('../oicMsg/jose/jwt/decode');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use uppercase since this is a class name: JWTDecoder

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

const jwtSigner = require('../oicMsg/jose/jwt/sign');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this defined? I don't see it in https://github.com/GJWT/nodeOIDCMsg/tree/master/src/oicMsg/jose/jwt
If this is a class name, it should be capitalized too: JWTSigner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has not been reviewed yet.. Most of the classes depend on each other.. I didn't post them as there is already three PRs open and thought it would better to close these ones before i open new ones.


/**
* @fileoverview
* Message is the top layer class that handles common functionality among the
* different serialization and deserialization types, such as claim
* verification. When sending request, it must be possible to serialize the
* information to a format that can be transmitted over-the-wire. Likewise, when
* receiving responses it must be possible to de-serialize these into an
* internal representation. Because of this a number of methods have been added
* to the token profile to support serialization to and deserialization from a
* number of representations that are used in the OAuth2 and OIDC protocol
* exchange.
*/

/**
* Message
* @class
* @constructor
*/
class Message {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This class doesn't have unit tests. Please provide tests.

constructor(claims) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add javadoc for what claims is.

if (claims){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix spacing
if (claims) {

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.claims = claims;
}else{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix spacing
} else {

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

this.initData();

/** Provided required claims */
this.requiredClaims = {};

/** Provided optional claims */
this.optionalClaims = {};

/** Expected required claim values */
this.verificationClaims = {};

/** Expected optional verification claims that are known */
this.optionalVerificationClaims = {};

/** None algorithm type */
this.noneAlgorithm = false;

/** Required claims */
this.optionsToPayload = [];

/** Other option values */
this.optionsForObjects = [];

/** Known required claims */
this.knownOptionalClaims = [];

/** Required verification claims */
this.claimsForVerification = []

/** Key value map of claims that make up the payload of a Message */
this.cParam = {};

/** Map of allowed values for each claim of Message */
this.cAllowedValues = {};

return this;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need to return this? Constructor does not need this.
const msg = new Message(claims);

}

initData() {
this.noneAlgorithm = false;
}

/**
* Add optional claims
* @param {?Object<string, string>} optionalClaims Claims that are not required
* */
addOptionalClaims(optionalClaims) {
this.optionalClaims = optionalClaims;
this.optionalVerificationClaims = {};
for (var i = 0; i < optionalClaims.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.

let i = 0;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also optionalClaims is defined as an object but you treat it like an array

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 key = optionalClaims[i];
if (key) {
this.optionalVerificationClaims[key] = key;
}
};
}

/** Check for missing required claims */
validateRequiredFields() {
for (var i = 0; i < this.optionsToPayload.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.

let i = 0;

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 key = this.optionsToPayload[i];
if (!this[key]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Be aware that if this[key] evaluates to empty string, false or null, !this[key] will be false.

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

throw new Error('You are missing a required parameter');
}
};
}

/** Fetch Required claims */
getRequiredClaims() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these getters defined by the API?
Because you can have them as properties: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get
For example here:

get requiredClaims() {
  return ...
}

this.requiredClaims = {};
for (var i = 0; i < this.optionsToPayload.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.

let i = 0;

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 key = this.optionsToPayload[i];
this.requiredClaims[key] = this[key];
}
return this.requiredClaims;
}


/**
* Fetch optional claims
*/
getOptionalClaims() {
return this.optionalClaims;
}

/** Fetch expected verification values for required claims */
getVerificationClaims() {
return this.verificationClaims;
}

/** Fetch expected verification values for optional claims */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can end your comments with a fullstop:
/** Fetch expected verification values for optional claims. */

Also define @return in your javadoc. eg:
@return {!Object<string, string>} The optional verification claims.

getOptionalVerificationClaims() {
return this.optionalVerificationClaims;
}

/**
* User explicitly wants to set None Algorithm attribute
* @param {?boolean} boolVal Bool value that determines none algorithm setting
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be null?

* */
setNoneAlgorithm(boolVal) {
this.noneAlgorithm = boolVal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is null, you need to convert to boolean:
this.noneAlgorithm = !!boolVal;

}

/** Fetch current none algorithm bool value */
getNoneAlgorithm() {
return this.noneAlgorithm;
}

/**
* Throws error if required verification claims are not present
* @param {?Object<string, string>} claimsToVerify Claims that need to be verified
* */
validateRequiredVerificationClaims(claimsToVerify) {
for (var i = 0; i < this.claimsForVerification.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.

let i = 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

For all array traversals, you should use forEach

let key = this.claimsForVerification[i];
if (!claimsToVerify[key]) {
throw new Error(`Missing required verification claim: ${key}`);
}
};
this.verificationClaims = claimsToVerify;
}

/**
* Throws error if required non Required verification claims are not present
* @param {?Object<string, string>} claimsToVerify Claims that need to be verified
*/
validateOptionalVerificationClaims(claimsToVerify) {
if (this.optionalVerificationClaims['nbf'] ||
this.optionalVerificationClaims['exp']) {
this.optionalVerificationClaimsCheck('clockTolerance', claimsToVerify);
}
if (this.optionalVerificationClaims['aud']) {
this.optionalVerificationClaimsCheck('aud', claimsToVerify);
}
}

optionalVerificationClaimsCheck(key, claimsToVerify) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add javadoc for this function

if (!claimsToVerify[key]) {
throw new Error(`Missing required verification claim: ${key}`);
} else {
this.verificationClaims[key] = claimsToVerify[key];
if (key == 'aud') {
this.claimsForVerification['aud'] = 'aud';
}
}
}

/**
* Serialization of JWT type
* Signs JWT and checks for valid input
* @param secretOrPublicKey is a string or buffer containing either the secret
Copy link
Collaborator

Choose a reason for hiding this comment

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

This javadoc definition is incorrect:
@param {string|!Buffer} secretOrPublicKey A string of buffer...

for HMAC algorithms, or the PEM encoded public key for RSA and ECDSA
* @param options consists of other inputs that are not part of the payload,
for ex : 'algorithm'
* @param callback is called with the decoded payload if the signature is
valid and optional expiration, audience, or issuer are valid. If not, it will
be called with the error. When supplied, the function acts asynchronously.
**/
toJWT(secretOrPrivateKey, options, callback) {
return jwtSigner.prototype.sign(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you returning jwtSigner.prototype ?

this, secretOrPrivateKey, options, callback);
}

/**
* Deserialization of JWT type
* Signs JWT and checks for valid input
* @param {string} signedJWT Signed JWT string
* @param {*} secretOrPublicKey String or buffer containing either the secret for HMAC algorithms, or the PEM encoded public key for RSA and ECDSA
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use more explicit types
{string|!Buffer}

Copy link
Collaborator

Choose a reason for hiding this comment

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

When comments are too long split to second line. Indent the second line by 4 spaces.

* @param {?Object<string, string>} claimsToVerify Dictionary contains claims that need to be verified
* @param {?Object<string, string>} options Consists of other inputs that are not part of the payload, for ex : 'algorithm'
* @param {*} callback Called with the decoded payload if the signature is valid and optional expiration, audience, or issuer are valid. If not, it
will be called with the error. When supplied, the function acts
asynchronously.
**/
fromJWT(signedJWT, secretOrPrivateKey, claimsToVerify, options, callback) {
this.validateRequiredVerificationClaims(claimsToVerify);
this.validateOptionalVerificationClaims(claimsToVerify);
return jwtDecoder.prototype.decode(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you returning jwtDecoder.prototype ?

signedJWT, secretOrPrivateKey, this, options, callback);
}

/**
* Serialization of JSON type
* @param {?Object<string, string>} obj Object that needs to be converted to JSON
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is the @return definition?

*/
static toJSON(obj) {
if (obj) {
this.claims = JSON.stringify(obj);
}else if (typeof this.claims !== String){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix spacing. Also typeof returns a string
} else if (typeof this.claims !== 'string'){

this.claims = JSON.stringify(this.claims);
}
return this.claims;
}

/**
* Deserialization of JSON type
* @param {string} jsonString Json object that needs to be deserialized
* */
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 * */

static fromJSON(jsonString) {
return JSON.parse(jsonString);
}

/**
* Serialization of URL Encoded type
* @param {?Object<string, string>} obj Object that needs to be URL encoded
Copy link
Collaborator

Choose a reason for hiding this comment

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

Define @return

*/
static toUrlEncoded(obj) {
if (!obj) {
obj =
Object.assign({}, this.getRequiredClaims(), this.getOptionalClaims());
}
const str = [];
for (const p in obj)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When traversing an object, you should check hasOwnProperty:
https://stackoverflow.com/questions/684672/how-do-i-loop-through-or-enumerate-a-javascript-object

for (const p in obj) {
  if (obj.hasOwnProperty(p)) {
  }
}

You can also use Object.keys(obj).forEach()

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

str.push(`${encodeURIComponent(p)}=${encodeURIComponent(obj[p])}`);
return str.join('&');
}

/**
* Deserialization of URL Encoded string
* @param {string} obj Url encoded string that needs to be deserialized
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is called obj but in the function below, the parameter name is urlEncodedString

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

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

static fromUrlEncoded(urlEncodedString) {
if (typeof urlEncodedString === 'string') {
const obj = {};
urlEncodedString.replace(/([^=&]+)=([^&]*)/g, (m, key, value) => {
obj[decodeURIComponent(key)] = decodeURIComponent(value);
});
return obj;
} else {
return urlEncodedString;
}
}
}

module.exports = Message;