nyeholt / silverstripe-restrictedobjects

A SilverStripe module to lock down object access, for the CMS as well as non-page objects
BSD 3-Clause "New" or "Revised" License
18 stars 10 forks source link

How to handle allowing the system to update a DataObject when the currently logged in Member does not have the correct permissions #16

Closed jordanmkoncz closed 8 years ago

jordanmkoncz commented 8 years ago

I have a website that's using silverstripe-restrictedobjects, and I've run into an issue while trying to add eCommerce functionality to this website.

The issue happens when a Member has just paid for a product with an external payment gateway and is redirected back to the website, at which point the website recognises the payment was successful and attempts to decrement the stock level of the purchased product.

When it does this, silverstripe-restrictedobjects catches the attempt to write to this Product DataObject in Restrictable->onBeforeWrite(), and proceeds to check whether the currently logged in Member has the Write permissions for this Product DataObject. Of course, the Member does not have the Write permissions so silverstripe-restrictedobjects throws a PermissionDeniedException, the user sees a Server Error page, and the attempt to decrement the purchased product's stock fails.

What is the best way to resolve this issue? It's essentially a case where it's really the website itself that is trying to update the product, which it needs to be able to do, but silverstripe-restrictedobjects does not recognise this distinction and just checks whether the currently logged in Member has permissions to perform the write action that has been triggered.

For testing purposes I confirmed that I could prevent the PermissionDeniedException by explicitly granting the Write permission for the Product to the Member who is making the purchase, but I can't imagine the best solution here is to just give all users Write permissions to all Products.

In case it helps, here's the full stack trace for the PermissionDeniedException:

[Error] Uncaught PermissionDeniedException: You must have write permission to Product #643 (Write)
/var/www/html/restrictedobjects/code/extensions/Restrictable.php:283 

Restrictable->onBeforeWrite(,,,,,,) 
Object.php:1002
Object->extend(onBeforeWrite,) 
DataObject.php:1021
DataObject->onBeforeWrite() 
SiteTree.php:1492
SiteTree->onBeforeWrite() 
Page.php:98
Page->onBeforeWrite() 
DataObject.php:1145
DataObject->write() 
TrackStockOnBuyable.php:216
Milkyway\SS\Shop\Inventory\Extensions\TrackStockOnBuyable->decrementStock(1,Product_OrderItem) 
call_user_func_array(Array,Array) 
Object.php:731
Object->__call(decrementStock,Array) 
Order.php:20
Product->decrementStock(1,Product_OrderItem) 
Order.php:20
Milkyway\SS\Shop\Inventory\Extensions\Order->Milkyway\SS\Shop\Inventory\Extensions\{closure}(Product_OrderItem) 
DataList.php:642
DataList->each(Closure) 
Order.php:22
Milkyway\SS\Shop\Inventory\Extensions\Order->onPlaceOrder(,,,,,,) 
Object.php:1002
Object->extend(onPlaceOrder) 
OrderProcessor.php:280
OrderProcessor->placeOrder() 
OrderProcessor.php:163
OrderProcessor->completePayment() 
ShopPayment.php:18
ShopPayment->onCaptured(GatewayResponse,,,,,,) 
Object.php:1002
Object->extend(onCaptured,GatewayResponse) 
PurchaseService.php:139
PurchaseService->completePurchase() 
PaymentGatewayController.php:58
PaymentGatewayController->index(SS_HTTPRequest) 
RequestHandler.php:288
RequestHandler->handleAction(SS_HTTPRequest,index) 
Controller.php:194
Controller->handleAction(SS_HTTPRequest,index) 
RequestHandler.php:200
RequestHandler->handleRequest(SS_HTTPRequest,DataModel) 
Controller.php:153
Controller->handleRequest(SS_HTTPRequest,DataModel) 
Director.php:370
Director::handleRequest(SS_HTTPRequest,Session,DataModel) 
Director.php:153
Director::direct(/paymentendpoint/c115020e7846b4adfa4db4c5e5efb9/complete,DataModel) 
main.php:177

And here's the decrementStock() function that is causing the write operation that is caught by silverstripe-restrictedobjects:

public function decrementStock($value = 1, $orderItem = null, $write = true)
{
    $result = ValidationResult::create();

    if ($this->owner->{$this->stockField . '_NoTracking'}) {
        return $result;
    }

    $currentStock = $this->owner->AvailableStock();

    if ($currentStock < $value) {
        if ($this->autoAdjust && $orderItem) {
            $orderItem->Quantity = strtolower(Config::env('ShopConfig.Inventory.AffectStockDuring')) == 'cart' ? $orderItem->PreviousQuantity + $currentStock : $currentStock;

            if ($orderItem->exists() && $write) {
                $orderItem->write();
                $result->error('Not enough stock, stock has been adjusted.', 'adjustment');
            }
        } else {
            $result->error('Not enough stock.', 'error');
        }

        if (!$this->autoAdjust || $this->validateStock) {
            throw new ValidationException($result);
        }
    }

    $this->owner->{$this->stockField} -= $value;

    if ($this->owner->{$this->stockField} < 0) {
        $this->owner->{$this->stockField} = 0;
    }

    $this->fireEvents($orderItem);

    if ($write) {
        $this->owner->write();

        if (
            $this->owner->hasExtension('Versioned') &&
            in_array("Stage", $this->owner->getVersionedStages())
        ) {
            $this->owner->writeToStage('Stage');
            $this->owner->publish('Stage', 'Live');
        }
    }

    $this->owner->extend('onDecrementStock', $value, $orderItem, $write);

    return $result;
}
nyeholt commented 8 years ago

Take a look at TransactionManager::runAsAdmin($closure), or TransactionManager::runAs($closure, $user)


$owner = $this->owner;

singleton('TransactionManager')->runAsAdmin(function () use ($owner) {
    $owner->decrementStock(a, b, c);
});

Is that useful?

jordanmkoncz commented 8 years ago

Thanks for the suggestion, I'll give that a try. I'm guessing that this method would essentially require me to fork the SilverStripe module that makes the call to the decrementStock() function just so that I can wrap the call to this function with silverstripe-restrictedobjects TransactionManager?

nyeholt commented 8 years ago

Hmm, yes most likely - unless there's an interim method you can hook into that will allow you to temporarily disable the restrictable checks via Restrictable::set_enabled(false);

jordanmkoncz commented 8 years ago

I ended up forking the eComerce inventory module (https://github.com/milkyway-multimedia/ss-shop-inventory) and adding an additional extension point to decrementStock() via $this->owner->extend() before the write operations happen, to go along with the existing one (onDecrementStock) that is called after the write operations happen. I then created a project-specific DataExtension that applies to the same classes (Product and ProductVariation) and calls Restrictable::set_enabled(false) in the first extension point and then Restrictable::set_enabled(true) in the second extension point. That way I was able to avoid putting any code specific to silverstripe-restricedobjects in my fork of the eCommerce inventory module.

Thanks for your suggestions!