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

Logout as POST request #185

Merged
merged 8 commits into from
Dec 7, 2020
Merged

Logout as POST request #185

merged 8 commits into from
Dec 7, 2020

Conversation

Mister-42
Copy link
Contributor

@Mister-42 Mister-42 commented Dec 2, 2020

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

Currently leads to Unprocessable entity, need to find source and fix it.

@samdark
Copy link
Member

samdark commented Dec 2, 2020

Unprocessable entity is caused by CSRF protection. You should add CSRF token to the form.

@Mister-42
Copy link
Contributor Author

Unprocessable entity is caused by CSRF protection. You should add CSRF token to the form.

That would mean I have to pass CSRF to every page. That sounds like a bad idea somehow.

@samdark
Copy link
Member

samdark commented Dec 2, 2020

There are two solutions to this:

  1. Always apply CsrfViewInjection so CSRF token value is always in the view/layout context.
  2. Remove CSRF middleware from the application stack and add it to every route/group individually.

Both aren't ideal but first solution looks a bit better to me.

@BoShurik
Copy link

BoShurik commented Dec 2, 2020

Is it possible to inject token inside Form::widget()?

@samdark
Copy link
Member

samdark commented Dec 2, 2020

Yes.

@Mister-42
Copy link
Contributor Author

@samdark Is this what you meant? I am somewhat coding blind, because yii serve gives me errors.

@samdark
Copy link
Member

samdark commented Dec 3, 2020

Yes, something like that but likely that exact code won't work as is. You're passing token as an option but it should be a hidden form field.

@BoShurik idea may be interesting as well but looks a bit more complicated. Something like the following:

<?php


namespace App\Widget;

use Yiisoft\Csrf\CsrfToken;
use Yiisoft\Html\Html;
use Yiisoft\Http\Method;
use Yiisoft\Widget\Widget;
use Yiisoft\Form\Widget\Form as BasicForm;

final class Form extends Widget
{
    private string $action = '';
    private string $method = Method::POST;
    private array $options = [];
    private CsrfToken $csrfToken;

    public function __construct(CsrfToken $csrfToken)
    {
        $this->csrfToken = $csrfToken;
    }

    public function begin(): ?string
    {
        $config = [
            'action()' => [$this->action],
            'method()' => [$this->method],
            'options()' => [$this->options],
        ];
        return BasicForm::widget($config)->begin() . $this->renderCsrfToken();
    }

    private function renderCsrfToken(): string
    {
        return Html::hiddenInput('_csrf', $this->csrfToken->getValue());
    }

    protected function run(): string
    {
        return BasicForm::end();
    }

    public function action(string $value): self
    {
        $new = clone $this;
        $new->action = $value;
        return $new;
    }

    public function method(string $value): self
    {
        $new = clone $this;
        $new->method = $value;
        return $new;
    }

    public function options(array $value = []): self
    {
        $new = clone $this;
        $new->options = $value;
        return $new;
    }
}

@BoShurik am I correct?

@Mister-42
Copy link
Contributor Author

@samdark See https://github.com/yiisoft/form/blob/master/src/Widget/Form.php#L45, csrf passed as option creates a hidden input ;)

@BoShurik
Copy link

BoShurik commented Dec 3, 2020

It would be great to hide csrf protection under the Yiisoft\Form\Widget\Form. E.g.

$enableCsrfToken = ArrayHelper::remove($this->options, 'csrf', true); // boolean flag, not a token itself

if ($enableCsrfToken && strcasecmp($this->method, Method::POST) === 0) {
    $hiddenInputs[] = Html::hiddenInput('_csrf', $this->createCsrfToken());
}

@terabytesoftw
Copy link
Member

It would be great to hide csrf protection under the Yiisoft\Form\Widget\Form. E.g.

$enableCsrfToken = ArrayHelper::remove($this->options, 'csrf', true); // boolean flag, not a token itself

if ($enableCsrfToken && strcasecmp($this->method, Method::POST) === 0) {
    $hiddenInputs[] = Html::hiddenInput('_csrf', $this->createCsrfToken());
}

But then form will depend on csrf and I think that is not good.

@BoShurik
Copy link

BoShurik commented Dec 3, 2020

It may be optional. On the other hand 90% of all csrf tokens will be used in forms so it improve DX

@Mister-42
Copy link
Contributor Author

Mister-42 commented Dec 3, 2020

I guess the conclusion is we leave Form the way it is, at least for now.

That said, can anyone actualy test my code please? I am unable to run yii serve and don't have a testing webserver available right now.

I'm an idiot. All I had to do was install php-sqlite 🤦‍♂️

Code works!

@Mister-42 Mister-42 marked this pull request as ready for review December 3, 2020 16:35
@samdark samdark added type:enhancement Enhancement status:code review The pull request needs review. labels Dec 3, 2020
@samdark samdark requested a review from a team December 3, 2020 22:05
views/layout/main.php Outdated Show resolved Hide resolved
@samdark
Copy link
Member

samdark commented Dec 6, 2020

Verified that it works well. Checking code.

Copy link
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and works well but I think we can do it simpler by reusing existing CsrfViewInjection. Then withCsrf() could be removed from ViewRenderer and controllers.

src/ViewInjection/LayoutViewInjection.php Outdated Show resolved Hide resolved
@samdark samdark added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Dec 6, 2020
@Mister-42
Copy link
Contributor Author

Mister-42 commented Dec 6, 2020

I'm not quite sure how to do that. Also this is currently implemented for layout, not view (not sure if this is of any importance when done from Controllers).

@samdark
Copy link
Member

samdark commented Dec 6, 2020

CsrfViewInjection registers token for both view and layout. It could be activated globally via params.php, yiisoft/yii-view -> injections.

@Mister-42
Copy link
Contributor Author

Gotcha 👍🏻

@samdark
Copy link
Member

samdark commented Dec 6, 2020

@vjik what do you think about the need for withCsrf() considering this case? I think that logout via POST in layout would present in majority of cases.

@Mister-42
Copy link
Contributor Author

If I understand withCsrf() correctly it adds the meta tag. I was reading about this just now and found this post

Putting them in a meta tag only makes sense if you are using JavaScript. JavaScript could read the tokens from the meta tag and post them to an action.

I'll leave it up to you to decide to keep or remove those.

@samdark samdark added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Dec 7, 2020
@samdark samdark merged commit b4d6747 into yiisoft:master Dec 7, 2020
@samdark
Copy link
Member

samdark commented Dec 7, 2020

Thank you!

@Mister-42
Copy link
Contributor Author

If I understand withCsrf() correctly it adds the meta tag. I was reading about this just now and found this post

Putting them in a meta tag only makes sense if you are using JavaScript. JavaScript could read the tokens from the meta tag and post them to an action.

I'll leave it up to you to decide to keep or remove those.

Useful code in case anyone want to use the jQuery data-method link method, but in Vanilla JS. Makes use of the meta csrf tag.

(function() {
    var DataMethod = {
        init: function() {
            this.links = document.querySelectorAll('a[data-method]');
            this.registerEvents();
        },

        registerEvents: function() {
            Array.from(this.links).forEach(function(l) {
                l.addEventListener('click', DataMethod.render);
            });
        },

        render: function(e) {
            var el = this, httpMethod, form;

            httpMethod = el.getAttribute('data-method').toUpperCase();

            if (['POST', 'PUT', 'PATCH', 'DELETE'].indexOf(httpMethod) === -1) {
                return;
            }

            if (el.hasAttribute('data-confirm') && !DataMethod.verifyConfirm(el)) {
                e.preventDefault();
                return false;
            }

            form = DataMethod.createForm(el);
            form.submit();
            e.preventDefault();
        },

        verifyConfirm: function(link) {
            return confirm(link.getAttribute('data-confirm'));
        },

        createForm: function(link) {
            var form = document.createElement('form');
            DataMethod.setAttributes(form, {
                method: 'POST',
                action: link.getAttribute('href')
            });

            var csrfToken = document.querySelector("meta[name=csrf]").getAttribute('content');

            if (!csrfToken) {
                console.error('CSRF token not found!');
            }

            var inputToken = document.createElement('input');
            DataMethod.setAttributes(inputToken, {
                type: 'hidden',
                name: '_csrf',
                value: csrfToken
            });

            form.appendChild(inputToken);
            document.body.appendChild(form);

            return form;
        },

        setAttributes: function(el, attrs) {
            for (var key in attrs) {
                el.setAttribute(key, attrs[key]);
            }
        }
    };

    DataMethod.init();
})();

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.

5 participants