Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow environment variables to configure cookie scheme #62

Merged
merged 6 commits into from Apr 20, 2015

Conversation

@hofan41
Copy link
Contributor

hofan41 commented Apr 14, 2015

Closes #61

hofan41 added 3 commits Apr 14, 2015
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
@hofan41 hofan41 changed the title Issue #61 - Allow environment variables to configure cookie scheme Allow environment variables to configure cookie scheme Apr 14, 2015
clearInvalid: settings.clearInvalid,
ignoreErrors: true
ignoreErrors: true,
domain: settings.domain,

This comment has been minimized.

Copy link
@jaw187

jaw187 Apr 15, 2015

Contributor

Domain should remain optional

This comment has been minimized.

Copy link
@hofan41

hofan41 Apr 15, 2015

Author Contributor

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.

This comment has been minimized.

Copy link
@jaw187

jaw187 Apr 16, 2015

Contributor

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.

This comment has been minimized.

Copy link
@hofan41

hofan41 Apr 16, 2015

Author Contributor

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.

This comment has been minimized.

Copy link
@jaw187

jaw187 Apr 16, 2015

Contributor

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.

This comment has been minimized.

Copy link
@hofan41

hofan41 Apr 16, 2015

Author Contributor

Ok, done :)

This comment has been minimized.

Copy link
@jaw187

jaw187 Apr 17, 2015

Contributor

I'd still like to maintain the same process for adding ttl and domain to the cookieOptions object.

This comment has been minimized.

Copy link
@hofan41

hofan41 Apr 17, 2015

Author Contributor

Sure thing, I have just submitted a new commit.

ignoreErrors: true
ignoreErrors: true,
domain: settings.domain,
ttl: settings.ttl

This comment has been minimized.

Copy link
@jaw187

jaw187 Apr 15, 2015

Contributor

TTL should remain optional

@jaw187 jaw187 added the feature label Apr 16, 2015
hofan41 added 2 commits Apr 16, 2015
jaw187 added a commit that referenced this pull request Apr 20, 2015
Allow environment variables to configure cookie scheme
@jaw187 jaw187 merged commit 299ba05 into hapijs:master Apr 20, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Apr 20, 2015

Issue and PR both missing milestone info. Also need to assign to someone.

@jaw187 jaw187 added this to the 2.1.0 milestone Apr 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.