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

Added support for setting the SameSite option on cookies. #541

Merged
merged 1 commit into from Jun 7, 2018

Conversation

Projects
None yet
5 participants
@Asamsig
Copy link
Contributor

Asamsig commented Jun 7, 2018

Pull Request Checklist

Purpose

Makes it possible to set the SameSite option on cookies.

Background Context

I chose to use a String representation, since using the Cookie.SameSite type, would break current solutions using Ficus. Alternatively we could create our own Enumeration and convert that to the Play Cookie.SameSite type. EDIT: We're now using the Cookie.SameSite type.

I chose the default to be Lax, since from looking at other settings, we want to recommend a sane default, where as Strict, might be too harsh, and None would simply do nothing.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 7, 2018

Coverage Status

Coverage increased (+0.01%) to 95.593% when pulling dcf6da4 on Asamsig:sameSite-support into d0b3618 on mohiva:master.

@akkie

This comment has been minimized.

Copy link
Member

akkie commented Jun 7, 2018

Thanks @Asamsig 👍

In my opinion the lib should use the Cookie.SameSite type, because the lib itself isn't bound to ficus. Ficus is used by the seed template but this is only one way of wiring config objects. There are plenty of configuration libs out there that can be used for that task and which maybe can use the type provided by Play. For the seed template we can use our own Ficus ValueReader to get the type working with Play.

@Asamsig

This comment has been minimized.

Copy link
Contributor Author

Asamsig commented Jun 7, 2018

Okay, I'll make that change 👍

@Asamsig Asamsig force-pushed the Asamsig:sameSite-support branch from 59e5499 to dcf6da4 Jun 7, 2018

@Asamsig

This comment has been minimized.

Copy link
Contributor Author

Asamsig commented Jun 7, 2018

I've changed to the Cookie.SameSite type. You can ping me, when the new version is released, then I can help update the seed template if you'd like.

@akkie akkie merged commit 10d5c2a into mohiva:master Jun 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akkie

This comment has been minimized.

Copy link
Member

akkie commented Jun 7, 2018

@Asamsig Awesome 👍 I'll do that.

@akkie

This comment has been minimized.

Copy link
Member

akkie commented Jun 9, 2018

@Asamsig I've just released version 5.0.5

@akkie akkie referenced this pull request Jun 9, 2018

Closed

5.0.5 SameSite upgrade issue #543

@Enalmada

This comment has been minimized.

Copy link

Enalmada commented Jun 9, 2018

@Asamsig Lax seems like a smart default and I am disappointed that play doesn't default to it.

@Enalmada

This comment has been minimized.

Copy link

Enalmada commented Jun 29, 2018

Look at the linked ticket in play-silhouette-seed for migration code. This is what you need put in your SilhouetteModule thanks to @Asamsig:

import com.typesafe.config.Config
import net.ceedubs.ficus.readers.ValueReader
import play.api.mvc.{Cookie}

implicit val sameSiteReader: ValueReader[Option[Option[Cookie.SameSite]]] =
  (config: Config, path: String) => {
    if (config.hasPathOrNull(path)) {
      if (config.getIsNull(path))
        Some(None)
      else {
        Some(Cookie.SameSite.parse(config.getString(path)))
      }
    } else {
      None
    }
  }
@nafg

This comment has been minimized.

Copy link

nafg commented Jun 29, 2018

Actually this suffices:

implicit val sameSiteReader: ValueReader[Option[Cookie.SameSite]] =
    ValueReader.relative(cfg => Cookie.SameSite.parse(cfg.as[String]))
@akkie

This comment has been minimized.

Copy link
Member

akkie commented Jul 2, 2018

@nafg Could you please create a PR against the seed template with the value reader?

@Asamsig

This comment has been minimized.

Copy link
Contributor Author

Asamsig commented Jul 2, 2018

I tested the provided code from @nafg on my local project, and it doesn't seem to work.

Also keep in mind, the reason the code is quite verbose in the seed project, is because it handles these cases:
Not set, set None, will use default ('Lax')
Set to null, set Some(None), will use 'No Restriction'
Set to a string value try to match, Some(Option(string))

On further consideration, it should probably either log a warning or throw, when given a String value that doesn't match a setting, since otherwise it simply downgrades to 'No Restriction'.

@nafg

This comment has been minimized.

Copy link

nafg commented Jul 3, 2018

I'm no expert in Ficus, but now it sounds like you may want something like

implicit val sameSiteReader: ValueReader[Option[Cookie.SameSite]] =
  ValueReader.relative { cfg =>
    if(cfg.getIsNull(".") None
    else Some(
      Cookie.SameSite.parse(cfg.as[String]).getOrElse(throw new RuntimeException("Invalid SameSite value"))
    )
  }

I think if the value is not present it will set the default value automatically. That's why it's looking for an Option[Option[SameSite]].

janniclas added a commit to delphi-hub/delphi-management that referenced this pull request Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.