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

Add a custom failure handler feature #30

Merged
merged 9 commits into from
Nov 22, 2021
Merged

Add a custom failure handler feature #30

merged 9 commits into from
Nov 22, 2021

Conversation

solventt
Copy link
Contributor

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues -

I think it would be nice to let users define their custom failure handler. When a CSRF check fails one may want to do something else besides forming a simple Response, for example, destroy a session, log a CSRF "accident".

But this feature is good if this component is used separately outside of the yii framework. If you take the framework, you have to somehow inject a session and logger instances into the middleware.

@samdark samdark added status:code review The pull request needs review. type:enhancement Enhancement labels Oct 29, 2021
src/CsrfMiddleware.php Outdated Show resolved Hide resolved
@samdark samdark requested a review from a team October 29, 2021 07:04
src/CsrfMiddleware.php Outdated Show resolved Hide resolved
@solventt
Copy link
Contributor Author

solventt commented Nov 3, 2021

@samdark I specified Closure because there is a problem with the callable type. Quote:
"The callable type is not consistent. It is possible for a callable to be valid in one context but not in others, and so people need to consider how it is used carefully" (https://wiki.php.net/rfc/consistent_callables)

The example from https://wiki.php.net/rfc/typed_properties_v2#callable_type

class Test {
    public callable $cb;
 
    public function __construct() {
        // $this->cb is callable here
        $this->cb = [$this, 'method'];
    }
 
    private function method() {}
}
 
$obj = new Test;
// $obj->cb is NOT callable here
($obj->cb)();

Also see https://stackoverflow.com/questions/57935734/is-type-callable-supported-with-typed-properties

@devanych yes, Its convenient that you suggested. But to operate two handlers of RequestHandlerInterface in the proccess method - isn't this incorrect with respect to PSR-15?

@devanych
Copy link
Member

devanych commented Nov 3, 2021

@devanych yes, Its convenient that you suggested. But to operate two handlers of RequestHandlerInterface in the proccess method - isn't this incorrect with respect to PSR-15?

I don't see anything incorrect here, this is an absolutely standard case. This does not contradict the PSR-15 in any way.

@solventt
Copy link
Contributor Author

solventt commented Nov 3, 2021

@devanych

Ok. Your version is also quite good when dependencies (a session, logger) are needed. I like it. But it is strictly obligatory to create your own class to handle the fail. What if someone doesnt want to create an extra class to handle a fail?

Don't think that I'm imagining possible cases, for example I use the Slim microframework and reason based on real needs.

@devanych
Copy link
Member

devanych commented Nov 3, 2021

@devanych

Ok. Your version is also quite good when dependencies (a session, logger) are needed. I like it. But it is strictly obligatory to create your own class to handle the fail. What if someone doesnt want to create an extra class to handle a fail?

Don't think that I'm imagining possible cases, for example I use the Slim microframework and reason based on real needs.

This is a matter of preference of course. But if I use PSR-15, then it is logical for me to create a separate class that will be responsible for this. And using any microframework would not be a hindrance to me.

@samdark
Copy link
Member

samdark commented Nov 11, 2021

I'd go with RequestHandlerInterface as @devanych suggested. Here's how to use it w/o creating a separate class:

$failureHandler = new class() implements \Psr\Http\Server\RequestHandlerInterface {
  public function handle(ServerRequestInterface $request): ResponseInterface
  {
     // return response
  }
};
$middleware = new CsrfMiddleware(
       $responseFactory,
       $token
       $token,
       $failureHandler
);

@samdark
Copy link
Member

samdark commented Nov 11, 2021

@solventt does that look alright to you?

@solventt
Copy link
Contributor Author

@samdark I have no argument to discuss with @devanych's presented variant - its good. But as @devanych said: "This is a matter of preference of course" - and so I decided to write my own simpler solution (for the Slim microframework). Also see issue

@samdark samdark merged commit e7a091c into yiisoft:master Nov 22, 2021
@samdark
Copy link
Member

samdark commented Nov 22, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review. type:enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants