Yii 1.1中Auth扩展结合CDbTransaction的技术咨询

Hey there, let's dive into the potential permission-related pitfalls in your code and share actionable optimization tips to make it more secure and robust in a permission-controlled environment!

First, let's recap your code for context:

public function actionUnpost($id) {
    if (Yii::app()->request->isPostRequest) {
        $model = $this->loadModel($id);
        $qtyCheck = array();
        $transaction = Yii::app()->db->beginTransaction(); // Transaction begin
        try {
            // If unpost this will resulting in negative remaining quantity, we should cancel this unpost!!!
            foreach ($model->formDetails as $detail) {
                if (isset($qtyCheck[$detail->id])) {
                    $qtyCheck[$detail->id] -= ($detail->quantity * $detail->type);
                } else {
                    $qtyCheck[$detail->id] = Journal::model()->getRemReceivingQty($detail->id, $detail->locationFk) - ($detail->quantity * $detail->type);
                }
                // remaining has normal balance positif, so check the remaining should not be less than 0
                if (0 > $qtyCheck[$detail->id]) {
                    throw new CDbException('Stok barang ' . $detail->item->name . ' menjadi negatif, FB tidak bisa diunposting!');
                }
                // Delete journal entries
                foreach ($detail->journalEntries as $entry) {
                    $entry->delete();
                }
            }
            $model->postingStatus = FormHeader::UNPOSTED;
            $model->save(false);
            $transaction->commit();
            Yii::app()->user->setFlash('success', "FB $model->headerNo telah berhasil diunposting.");
        } catch (Exception $e) {
            $transaction->rollBack();
            Yii::app()->user->setFlash('error', $e->getMessage());
        }
        $this->redirect(array('view', 'id' => $model->id));
    } else {
        throw new CHttpException(400, 'Invalid request. Please do not repeat this request again.');
    }
}

• Missing Early Permission Checks: Right now, you start a transaction and run database operations before verifying if the current user has permission to unpost the form. Even if the transaction rolls back later, this wastes database resources and could expose sensitive data (like item names in error messages) to unauthorized users.
• Unchecked Access to Related Records: You're modifying formDetails and deleting journalEntries without verifying if the user has permission to interact with those specific records. A user might have permission to unpost a form but not access certain line items or journal entries linked to it.
• Sensitive Error Message Exposure: You're directly displaying the raw exception message to users. If an unauthorized user triggers an error, they might see item names, internal database details, or other sensitive information they shouldn't have access to.
• Skipped Validation with save(false): Using save(false) bypasses all model validations, including any permission-based validation rules you might have set up for modifying the postingStatus field. This opens the door for unauthorized users to modify the status even if they shouldn't have access.

Let's fix these issues step by step with concrete changes:

1. Add Permission Checks Before Starting Transactions
   Validate the user's right to unpost the form immediately after loading the model. If they don't have permission, throw a 403 error right away—no need to proceed with any database work:

   $model = $this->loadModel($id);
   // Check if user can unpost this specific form
   if (!Yii::app()->user->checkAccess('unpostForm', ['model' => $model])) {
       throw new CHttpException(403, 'You are not authorized to unpost this form.');
   }
   

2. Validate Access to Related Records
   Add checks for each related record (form details, journal entries) to ensure the user has permission to modify them. You can create helper methods in your models for this, like canBeManagedByCurrentUser():

   foreach ($model->formDetails as $detail) {
       if (!$detail->canBeManagedByCurrentUser()) {
           throw new CHttpException(403, 'You cannot modify this line item.');
       }
       // ... rest of your code
       foreach ($detail->journalEntries as $entry) {
           if (!$entry->canBeDeletedByCurrentUser()) {
               throw new CHttpException(403, 'You cannot delete this journal entry.');
           }
           $entry->delete();
       }
   }
   

3. Sanitize Error Messages
   Don't expose raw exception details to users. Instead, show user-friendly messages, and only include sensitive data (like item names) if the user has permission to see them:

   if (0 > $qtyCheck[$detail->id]) {
       // Only show item name if user has permission to view it
       $itemLabel = Yii::app()->user->checkAccess('viewItem', ['item' => $detail->item]) 
           ? $detail->item->name 
           : 'this item';
       throw new CDbException("Stock for $itemLabel would be negative—cannot unpost this form.");
   }
   

4. Avoid save(false)—Use Targeted Validation
   Instead of skipping all validations, specify only the fields you need to save, so your model's permission rules still run:

   $model->postingStatus = FormHeader::UNPOSTED;
   // Validate only the postingStatus field (or let all validations run if needed)
   if (!$model->save(['postingStatus'])) {
       throw new CDbException('Failed to update form status.');
   }
   

5. Separate Exception Handling
   Differentiate between permission errors (CHttpException) and database errors to handle them appropriately. Log internal errors for debugging instead of showing them to users:

   catch (CHttpException $e) {
       Yii::app()->user->setFlash('error', $e->getMessage());
       if ($transaction->active) {
           $transaction->rollBack();
       }
   } catch (Exception $e) {
       $transaction->rollBack();
       // Log the error for your team to debug
       Yii::log($e->getMessage(), CLogger::LEVEL_ERROR, __METHOD__);
       Yii::app()->user->setFlash('error', 'An error occurred. Please try again later.');
   }
   

These changes will make your code more secure, efficient, and compliant with permission rules in non-superadmin scenarios.

内容的提问来源于stack exchange，提问作者Daniel Adinugroho