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 null to be passed for the ttl #124

Merged
merged 1 commit into from Aug 9, 2016
Merged

Allow null to be passed for the ttl #124

merged 1 commit into from Aug 9, 2016

Conversation

@briandela
Copy link
Contributor

briandela commented Apr 15, 2016

null is a valid default option for the ttl of the cookie but the schema isn't currently allowing it.

As an example, in our case the value for ttl is coming from an external place; from a configuration system which sets it on a per-env basis.
In test/development the ttl might be set to an explicit value whereas in production it might be null

        server.auth.strategy('session', 'cookie', 'try', {
            password: options.cookieOptions.password,
            cookie: 'ssosid',
            ttl: options.cookieOptions.ttl,
            domain: options.cookieOptions.domain,
            redirectOnTry: false,
            appendNext: true,
            clearInvalid: true,
            isSecure: true,
            isHttpOnly: true,
            validateFunc: validate
        });

We can work around this easily by doing something like:

        const strategyOptions = {
            password: options.cookieOptions.password,
            cookie: 'ssosid',
            domain: options.cookieOptions.domain,
            redirectOnTry: false,
            appendNext: true,
            clearInvalid: true,
            isSecure: true,
            isHttpOnly: true,
            validateFunc: validate
        };

        if(options.cookieOptions.ttl) {
            strategyOptions.ttl = options.cookieOptions.ttl;
        }

        server.auth.strategy('session', 'cookie', 'try', strategyOptions);

However, it just felt a little cleaner to have the ttl schema accept a valid cookie ttl value.

Thoughts?

`null` is a valid default option for the `ttl` of the cookie but the schema isn't currently allowing it.

As an example, in our case the value for `ttl` is coming from an external place; from a configuration system which sets it on a per-env basis.
In test/development the `ttl` might be set to an explicit value whereas in production it might be `null` 

```js
        server.auth.strategy('session', 'cookie', 'try', {
            password: options.cookieOptions.password,
            cookie: 'ssosid',
            ttl: options.cookieOptions.ttl,
            domain: options.cookieOptions.domain,
            redirectOnTry: false,
            appendNext: true,
            clearInvalid: true,
            isSecure: true,
            isHttpOnly: true,
            validateFunc: validate
        });
```

We can work around this easily by doing something like:
```js
        const strategyOptions = {
            password: options.cookieOptions.password,
            cookie: 'ssosid',
            domain: options.cookieOptions.domain,
            redirectOnTry: false,
            appendNext: true,
            clearInvalid: true,
            isSecure: true,
            isHttpOnly: true,
            validateFunc: validate
        };

        if(options.cookieOptions.ttl) {
            strategyOptions.ttl = options.cookieOptions.ttl;
        }

        server.auth.strategy('session', 'cookie', 'try', strategyOptions);
```

However, it just felt a little cleaner to have the `ttl` schema accept a valid cookie ttl value.

Thoughts?
@jaw187 jaw187 merged commit acc3bbe into hapijs:master Aug 9, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@briandela briandela deleted the briandela:patch-1 branch Aug 9, 2016
@nlf nlf added the feature label Mar 28, 2017
@nlf nlf added this to the 7.0.0 milestone Mar 28, 2017
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.