Skip to content

Allow null to be passed for the ttl#124

Merged
jaw187 merged 1 commit intohapijs:masterfrom
briandela:patch-1
Aug 9, 2016
Merged

Allow null to be passed for the ttl#124
jaw187 merged 1 commit intohapijs:masterfrom
briandela:patch-1

Conversation

@briandela
Copy link
Copy Markdown
Contributor

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
@briandela briandela deleted the patch-1 branch August 9, 2016 15:37
@nlf nlf added the feature New functionality or improvement label Mar 28, 2017
@nlf nlf added this to the 7.0.0 milestone Mar 28, 2017
@lock
Copy link
Copy Markdown

lock bot commented Jan 9, 2020

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.

@lock lock bot unassigned jaw187 Jan 9, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature New functionality or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants