Silverstripe-framework: Unable to publish files via cli due to permission check

[zanderwar]

Affected Version

v4.0.3

Description

Unable to publish files via cli due to the canPublish() lookup as no Member exists via cli

Steps to Reproduce

use SilverStripe\Assets\Image;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Dev\BuildTask;
use SilverStripe\ORM\DataList;

class PublishImages extends BuildTask
{
    /**
     * Implement this method in the task subclass to
     * execute via the TaskRunner
     *
     * @param HTTPRequest $request
     *
     * @return void
     */
    public function run($request)
    {
        /** @var DataList|Image[] $images */
        $images = Image::get();

        foreach ($images as $image) {
            if (!$image->isPublished()) {
                $image->publishFile();
                $image->publishSingle();
                echo 1;
            } else {
                echo 'P';
            }
        }
    }
}

> php ./vendor/silverstripe/framework/cli-script.php dev/build
> php ./vendor/silverstripe/framework/cli-script.php dev/tasks/PublishImages
PPP1P1P1111111P1P1111111111111P11111111P1111111111111P1P1PPP11111111P111P11P1P111111111PPP1P1P1PP1111111P111P11P11P111111111P11P1PP11111111P11111111111111111111111111111P11PP1111P11111P1111P111
> php ./vendor/silverstripe/framework/cli-script.php dev/tasks/PublishImages
PPP1P1P1111111P1P1111111111111P11111111P1111111111111P1P1PPP11111111P111P11P1P111111111PPP1P1P1PP1111111P111P11P11P111111111P11P1PP11111111P11111111111111111111111111111P11PP1111P11111P1111P111

All should be published on the second run of dev/tasks/PublishImages

Notes

Works when running in browser - painfully slow though

[dhensby]

is this not a duplicate of https://github.com/silverstripe/silverstripe-versioned/issues/89 ?

[zanderwar]

Yes, except for publishSingle() having nothing to do with ChangeSet afaik

[tractorcow]

I think this is what Member::actAs() was created for; If your CLI task requires elevated permissions, you can have a separate user the task can run as.

You can even create a temporary user if you wish.

In the future we might add actWithPermissions($code), but that's a way off.

This isn't a bug but it is a weakness of the CLI architecture.

[zanderwar]

It's a cli task though, shouldn't it immediately be elevated to practically sudo?

Thanks for the actAs workaround of this strange feature.

I still wholeheartedly think it's up to the developer to do these permission checks much like yourself in your ChangeSet issue

[tractorcow]

@zanderwar how about this as an idea:

Set a "permission mode" for any task, or sub-process. This mode determines how permissions for specific actions should behave, rather than leaving it up to specific implementations to decide:

• Error (default): Fail if action requested that cannot be done. E.g. $file->doPublish() throws exception.
• Ignore: Do nothing if action requested that cannot be done. E.g. $file->doPulblish() will publish if permitted, but silently does nothing if not.
• Always: Do all requested actions, even if not allowed. Used by CLI tasks in particular. No permissions are checked for this mode (all checks return true without checking DB).

Also related https://github.com/silverstripe/silverstripe-versioned/issues/112

[tractorcow]

Permission::withMode(Permission::MODE_ALWAYS, function () {
    $this->doEverything();
});

[tractorcow]

Marking this as both bug and enhancement because I can't make up my mind.

[robbieaverill]

I've always expected CLI scripts to be executed with admin permissions and have been tripped up in the past when they aren't =)

[tractorcow]

There's a slight distinction between act-as-admin and skip permission checks though. One doesn't even do the checks, one does the checks with elevated privileges.

[zanderwar]

I don't mind actAs, but I literally just throw Member::get()->first() in there. That permission mode is actually appealing to me if this canPublish() _feature_ decides to stay

[kinglozzer]

Setting aside CLI vs web, I think we need to decide one way or another whether/how to implement these checks and do so consistently. We currently have a disclaimer in our docs about model permissions:

These checks are not enforced on low-level ORM operations such as write() or delete(), but rather rely on being checked in the invoking code. The CMS default sections as well as custom interfaces like ModelAdmin or GridField already enforce these permissions.

The two blockers I can see to implementing these checks everywhere are:

• BC headaches. Some of this could be alleviated by making CLI tasks “admin by default”, but non-CLI tasks would probably need manual intervention
• If we guard all ORM actions with permission checks, where does that leave DataList and canView()? Would be a major performance sucker to check every object

[tractorcow]

I think you are right, we should follow the permission model philosophy correctly. If users must guard their model via external permission checks, it should be consistent through the code base.

[tractorcow]

Partly fixed with https://github.com/silverstripe/silverstripe-versioned/pull/113 for versioned actions only.

[tractorcow]

Actually, it may be completely fixed.