Allow environment variables to configure cookie scheme#62
Allow environment variables to configure cookie scheme#62jaw187 merged 6 commits intohapijs:masterfrom
Conversation
Updated hapi-auth-cookie to use Joi for input validation. It will also use the Joi transformed result object for options so that booleans and integers can be passed in as strings but interpreted as the appropriate type. Also added unit tests to ensure original functionality of the removed asserts.
100% coverage achieved
lib/index.js
Outdated
There was a problem hiding this comment.
Domain should remain optional
There was a problem hiding this comment.
It is already optional, settings.domain will be set to undefined if the user did not provide a domain. This undefined value will be passed on to server.state() and it will behave as if it were not provided at all. I dug through the server.state() code and found the Joi schema :
hapi/node_modules/statehood/lib/index.js
domain: Joi.string().allow(null),
ttl: Joi.number().allow(null),If you really want to, I can specify null to be the default value for ttl and domain, but it doesn't really change anything functionally as far as I can tell. Please advise.
There was a problem hiding this comment.
Without doing any through testing, I figured it'd be more straight forward to maintain the original concepts used to create cookieOptions. You may be right, but I figured your enhancement could be accomplished with less changes.
There was a problem hiding this comment.
Agreed, this could definitely be accomplished without introducing Joi and just adding a few more if-statements. I thought adding Joi input validation would make this plugin more similar/conventional to other hapi core plugins that I have examined.
There was a problem hiding this comment.
You can leave the Joi validation. Keep the setting of cookieOptions.ttl as well as cookieOptions.domain the same and remove their respective default values from the schema.
There was a problem hiding this comment.
I'd still like to maintain the same process for adding ttl and domain to the cookieOptions object.
There was a problem hiding this comment.
Sure thing, I have just submitted a new commit.
Allow environment variables to configure cookie scheme
|
Issue and PR both missing milestone info. Also need to assign to someone. |
|
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions. |
Closes #61