redirectTo is now required, should it be? about cookie HOT 16 CLOSED

Just to add a use case to this, I have endpoints that accept basic auth for API access, and cookie auth for user sessions. If hapi-auth-cookie fails and attempts a redirect, it would be inappropriate response for an api request, which should simply return some sort of error.

I realize I can pass in a setting via the route config itself, but this is a large amount of change / extra code for a piece of functionality that seems like it should be optional.

Changing this line to joi.string().allow(false) feels like a nice solution.

from cookie.

This was definitely an oversight caused by my last pull request which introduced the joi schema. Joi.string() does not allow null by default, and therefore, all optional schema elements that use Joi.string() should also allow null/empty string.

from cookie.

redirectTo is not required. It is only required to be a string when present. The changes will only present a problem when redirectTo is explicitly set to null or false.

The docs state that redirectTo is an "optional login URI to redirect unauthenticated requests to". Since false and null aren't URIs, I'm not sure if this needs to be fixed.

from cookie.

The way the code is currently, redirectTo is required to be a string and to be present, leaving no option for disabling it aside from route-level plugin config, which doesn't get validated against the joi schema.

from cookie.

Definitely a bug that should be fixed immediately. Both redirectTo and domain should be addressed.

In Joi Documentation for Joi.string():
Note that empty strings are not allowed by default and must be enabled with allow('').

from cookie.

redirectTo is not required to be present.

> options = { password: 'asdf' }
{ password: 'asdf' }
> var results = Joi.validate(options, schema);
undefined
> results
{ error: null,
  value: 
   { password: 'asdf',
     cookie: 'sid',
     path: '/',
     clearInvalid: false,
     keepAlive: false,
     isSecure: true,
     isHttpOnly: true,
     appendNext: false,
     redirectOnTry: true } }

from cookie.

As for empty strings, they're falsey values which weren't ever being passed along to cookieOptions in the past, so I'm not sure if they need to be allowed.

from cookie.

I see what you're saying. Given that the tests are passing, and many have no redirectTo set it must have been a problem with my config setting an empty string. Validation will fail on an empty string, presently.

I guess it falls more into the realm of enhancement then, but, I always like to be able to set a null / falsey value explicitly in the event of a higher-level defaults / validation scenario where properties get defined, but values aren't necessarily set.

from cookie.

It is unfortunate that the recent changes have broken code that used to work with this module.

from cookie.

Haven't heard from anyone else, so I think this will be closed. @mshick Have you been able to modify your config or have you just kept using version 2.0.0?

from cookie.

I'm still on v2. I can modify my config and solve the problem, but the issue remains that the current code is making the presence of a property have an unintended / unexpected meaning in my opinion.

If I'm building a plugin/service that depends on hapi-auth-cookie and has some options to set up an auth strategy. I'd have to give special treatment to incoming domain and redirectTo options, in a way I don't for any other property. That is to say, if there is a default settings object there wouldn't be any way for the user to null / falsify those properties unless I explicitly check the incoming options obj for those null vals and delete the properties from the object that gets sent to the auth strategy.

Is there any rationale for the new behavior of redirectTo and domain? @hofan41 even seems to think it was an oversight on his part. At a minimum it makes this update more of a breaking change than seems necessary -- again, unless there's a good reason.

from cookie.

This issue definitely falls in the gray area where there is justification on both sides. The reason I see this as an oversight is because it breaks configurations that used to work. Something innocent like:

{
    domain: process.env.DOMAIN
}

Now need to have defaults clearly specified such that they do not break in the event the environment var is undefined.

{
    domain: process.env.DOMAIN || 'example.com'
}

The counter argument would be that you could just as easily enforce that environment var to be defined.

from cookie.

Right, and the default you just gave an example of would break many setups — they would only be able to set cookies on example.com.

There’s no way to provide a “wildcard” default domain except by removing the domain property, and no way to make redirectTo falsey except by deleting that property entirely as well.

On Apr 29, 2015, at 6:02 PM, Ho-Fan Kang [email protected] wrote:

This issue definitely falls in the gray area where there is justification on both sides. The reason I see this as an oversight is because it breaks configurations that used to work. Something innocent like:

{
domain: process.env.DOMAIN
}
Now need to have defaults clearly specified such that they do not break in the event the environment var is undefined.

{
domain: process.env.DOMAIN || 'example.com'
}
—
Reply to this email directly or view it on GitHub #66 (comment).

from cookie.

I think this issue should be addressed in the code simply because it is a greater inconvenience to work around than not. It might not be correct or proper to the letter of the api, but it sure is convenient.

from cookie.

Lets move forward with #65

from cookie.

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.

from cookie.