Message #20
Message #20
Conversation
src/oicMsg/message.js
Outdated
| */ | ||
| class Message { | ||
| constructor(claims) { | ||
| if (claims){ |
There was a problem hiding this comment.
Fix spacing
if (claims) {
src/oicMsg/message.js
Outdated
| constructor(claims) { | ||
| if (claims){ | ||
| this.claims = claims; | ||
| }else{ |
There was a problem hiding this comment.
Fix spacing
} else {
src/oicMsg/message.js
Outdated
| addOptionalClaims(optionalClaims) { | ||
| this.optionalClaims = optionalClaims; | ||
| this.optionalVerificationClaims = {}; | ||
| for (var i = 0; i < optionalClaims.length; i++){ |
There was a problem hiding this comment.
Also optionalClaims is defined as an object but you treat it like an array
src/oicMsg/message.js
Outdated
|
|
||
| /** Check for missing required claims */ | ||
| validateRequiredFields() { | ||
| for (var i = 0; i < this.optionsToPayload.length; i++){ |
src/oicMsg/message.js
Outdated
| /** Fetch Required claims */ | ||
| getRequiredClaims() { | ||
| this.requiredClaims = {}; | ||
| for (var i = 0; i < this.optionsToPayload.length; i++){ |
src/oicMsg/message.js
Outdated
| @@ -0,0 +1,265 @@ | |||
| 'use strict'; | |||
| const jwtDecoder = require('../oicMsg/jose/jwt/decode'); | |||
There was a problem hiding this comment.
Use uppercase since this is a class name: JWTDecoder
src/oicMsg/message.js
Outdated
| @@ -0,0 +1,265 @@ | |||
| 'use strict'; | |||
| const jwtDecoder = require('../oicMsg/jose/jwt/decode'); | |||
| const jwtSigner = require('../oicMsg/jose/jwt/sign'); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| Object.assign({}, this.getRequiredClaims(), this.getOptionalClaims()); | ||
| } | ||
| const str = []; | ||
| for (const p in obj) |
There was a problem hiding this comment.
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()
src/oicMsg/message.js
Outdated
| validateRequiredFields() { | ||
| for (var i = 0; i < this.optionsToPayload.length; i++){ | ||
| let key = this.optionsToPayload[i]; | ||
| if (!this[key]) { |
There was a problem hiding this comment.
Be aware that if this[key] evaluates to empty string, false or null, !this[key] will be false.
src/oicMsg/message.js
Outdated
|
|
||
| /** | ||
| * Deserialization of URL Encoded string | ||
| * @param {string} obj Url encoded string that needs to be deserialized |
There was a problem hiding this comment.
This is called obj but in the function below, the parameter name is urlEncodedString
TODO: convert require statements to import. Need additional setting up to enable this.