Fixes for the CSRF filter

[jroper]

• Fixes CSRF filter in Play doesn't allow custom token generator #1734, custom token generator feature reinstatement
• Fixes Play 2.2.x doc related to CSRF is incorrect #1728, ensured CSRFFilter can be instantiated without a running application
• Made token signing optional, so that a custom token generator does not need to generate signed tokens.
• Abstracted tests so they can be run on many different permutations of configuration
• Added documentation about all the different configuration options

This commit breaks binary compatibility, the CSRFFilter constructor parameters are now not lazy, and CSRFFilter is no longer a case class, so many of the methods it used to provide are no longer there. This was deemed necessary because the intended use of CSRFFilter, ie:

object Global extends WithFilters(CSRFFilter()) with GlobalSettings

was not possible with the old constructor. The constructor is however still source compatible for most use cases.

Since that constructor is intentionally breaking binary compatibility, new parameters that were added for custom token generation and configuration signing were added without consideration for binary compatibility, only source compatibility.

[brikis98]

@jroper, a huge thanks for putting this together so quickly.

One question: is it possible to make the token comparison pluggable? If the custom token generator produces a signed token value (ie, a signature is happening, but outside of the Play filter code), it may need a way to check the signature instead of a literal string comparison. Instead of taking in signToken: Boolean and tokenGenerator: => String parameters, what about taking in a tokenManager: TokenManager parameter, where TokenManager is a trait like this:

trait TokenManager {
  def generateToken: String
  def compareTokens(tokenA: String, tokenB: String): Boolean
}

The default Play implementations of this class could be something like:

class SignedTokenManager extends TokenManager {
  def generateToken: String = Crypto.generateSignedToken()
  def compareTokens(tokenA: String, tokenB: String): Boolean = Crypto.compareSignedTokens(tokenA, tokenB)
}

class UnsignedTokenManager extends TokenManager {
  def generateToken: String = Crypto.generateToken()
  def compareTokens(tokenA: String, tokenB: String): Boolean = Crypto.constantTimeEquals(tokenA, tokenB)
}

class CSRFAction(next: EssentialAction,
     tokenName: String = CSRFConf.TokenName,
     cookieName: Option[String] = CSRFConf.CookieName,
     secureCookie: Boolean = CSRFConf.SecureCookie,
     createIfNotFound: RequestHeader => Boolean = CSRFConf.defaultCreateIfNotFound,
     tokenManager: TokenManager = new SignedTokenManager()) extends EssentialAction {

    // ...

}

Apologies for not thinking of this earlier, but it seems like a more robust and consistent API. What do you think?

[cloudbees-pull-request-builder]

play2-PRs #772 SUCCESS
This pull request looks good

[jroper]

That would probably make the code neater. I'll have a go.

[jroper]

The biggest problem with this is that it wouldn't work with CSRF.getToken, since that doesn't know what the TokenGenerator should be, and in the case of the BREACH vulnerability needs to be able to resign the tokens. Of course, anyone using a custom TokenGenerator could simply not use that method to get the current token, but implement their own.

[brikis98]

If we wanted end-to-end consistency with the CSRF implementation, we'd probably need to either:

1. Create a CSRFPlugin that extends play.api.Plugin and TokenManager. The default implementation would basically be the SignedTokenManager above. If you needed to override it, just create your own subclass of CSRFPlugin, override methods as necessary, and put it into conf/play.plugins at a higher priority.
2. Add a getCsrfTokenManager method to GlobalSettings. The default implementation would return the SignedTokenManager, but of course, it's easy to override that.

Either option would be accessible to CSRF.getToken, but I'm not sure if it's worth the effort. It's a small enough class and a rare enough use case where it's just as easy to provide a custom version of that helper.

[jroper]

Agreed. Anyway, PR is updated, I've called it TokenProvider, because certain people that might review this code have violent reactions to any class with Manager in the name.

[brikis98]

Should these few lines be using the defaultTokenProvider?

[jroper]

We'd have to add a resign() method to the TokenProvider for these lines to make sense in that context. Since there's no way of supplying a TokenProvider to the method yet, it didn't make sense to abstract that logic out yet.

[cloudbees-pull-request-builder]

play2-PRs #773 SUCCESS
This pull request looks good

[brikis98]

Gotcha. Thanks again for putting this together. Looks good to me 👍

[huntc]

LGTM Final review with @richdougherty

[richdougherty]

Maybe add a little more detail to this comment. :)

[richdougherty]

Other than a bit more commenting, lgtm. 👍

[KrzysztofKowalski]

please fix that

[cloudbees-pull-request-builder]

play2-PRs #794 SUCCESS
This pull request looks good

[huntc]

Master branch also updated.