silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
719 stars 820 forks source link

SecurityToken classes cannot be replaced in Forms class #10128

Open micschk opened 2 years ago

micschk commented 2 years ago

I wanted to replace the SecurityToken class on a specific form to use a 'StatelessSecurityToken' class (eg not coupled to Session). So I was expecting to be able to ->setSecurityToken(...) on the form. However, this method does not exist.

There's also a few new SecurityToken()s in the Form class (else I'd be able to use the Injector). Form disableSecurityToken() is hard-coded to NullSecurityToken instead of setting some kind of securitytoken-disabled flag on the form. Not sure if having some kind of securitytoken class is technically necessary but a 'null' class (instead of just a flag/unsetting the securitytoken) seems unexpected behaviour to me. Calling enableSecurityToken() also effectively resets the securityToken to new SecurityToken() instead of enabling it.

Being able to set your own token-validator on a form would be nice (eg adding a setSecurityToken(...) method to the Form class). (Probably that would involve some rewriting of the various new calls mentioned above as well).

micschk commented 2 years ago

To elaborate a bit on this, there's a comment in SecurityToken mentioning that an existing instance cannot be disabled and needs to be overwritten (just doesn't make clear why not).
Oh, an both the SecurityToken and NullSecurityToken are also tightly coupled.

     /**
     * You can't disable an existing instance, it will need to be overwritten like this:
     * <code>
     * $old = SecurityToken::inst(); // isEnabled() returns true
     * SecurityToken::disable();
     * $new = SecurityToken::inst(); // isEnabled() returns false
     * </code>
     *
     * @return boolean
     */
    public function isEnabled()
    {
        return !($this instanceof NullSecurityToken);
    }
micschk commented 2 years ago

Not sure if one of these options would be possible (within the constraints of semantic versioning), but maybe we could either add a setSecurityToken method to the Form class:

    public function setSecurityToken(SecurityToken $securityToken)
    {
        $this->securityToken = $securityToken;

        return $this;
    }

OR just make enableSecurityToken accept an optional argument (default null should keep it backwards compatible?):

    public function enableSecurityToken(SecurityToken $securityToken = null)
    {
        $this->securityToken = $securityToken ?: new SecurityToken();

        return $this;
    }

Both options seem to be working OK for my purpose but would also allow more advanced token setups like JWT or PASETO (PHP-PASETO)

xini commented 2 years ago

I have noticed the same issues. I would like to create a global search form without security token, so that the page can still be cached in varnish. Have you found a solution to this?

The new SecurityToken(); in https://github.com/silverstripe/silverstripe-framework/blob/4/src/Forms/Form.php#L316 always opens a new session, even if Form::disableSecurityToken() is called and the security token is replaced with a NullSecurityToken later on. For that reason I believe it is not possible to create a form without opening a session as well.

Maybe we could add a 6th parameter to the form contructor $enableSecurity = true that could be used in the checks in https://github.com/silverstripe/silverstripe-framework/blob/4/src/Forms/Form.php#L310-L314? Not sure about backwards compatibility though?

kinglozzer commented 2 years ago

@xini could you use the functionality from the lines above and add a method to your controller (via an extension if you can’t edit it directly)?

public function securityTokenEnabled()
{
    return false;
}

It’s not a great solution, because if you have other forms that do need a token then you’re faced with the reverse problem - though you should just be able to call ->enableSecurityToken() for those, and it’d solve the session issue.

Perhaps the way to fix the issue you’ve described would be to remove the token creation from __construct() and do it in ->getSecurityToken() instead? That way it wouldn’t be called until render-time, by which point any calls to disableSecurityToken() would’ve already run

xini commented 2 years ago

@kinglozzer the securityTokenEnabled() controller method is the workaround that I have used so far, but as you mention, I have other forms on a page that need the tokens. Also, when dealing with extensions adding forms etc it gets really messy.

Removing the token creation from the constructor would be great. @micschk that would solve your issue as well, right?