feat: Check user ownership of groups parties cats #109

iMercyvlogs · 2025-10-08T23:38:09Z

Checks the ownership of each category,party and group before saving.

sourceant · 2025-10-08T23:39:13Z

Code Review Summary

✨ The Pull Request significantly improves the security posture of the TransactionController by implementing crucial ownership checks for related resources (groups, parties, and categories) during both transaction creation and updates. This ensures that users can only associate transactions with resources they legitimately own. Additionally, the error handling within transaction blocks has been enhanced to correctly propagate HttpExceptions, providing more accurate feedback for validation failures.

🚀 Key Improvements

Enhanced Resource Ownership Validation: Crucial security checks are now in place to prevent unauthorized association of groups, parties, and categories with transactions.
Improved Error Handling: HttpExceptions thrown during validation within database transactions are now properly caught and re-thrown with their original status codes.

💡 Minor Suggestions

The ownership validation logic is duplicated across the store and update methods. Refactoring this into a private helper method would significantly improve code maintainability and reduce redundancy.
Consider using Laravel Form Requests to encapsulate validation logic, further decoupling it from the controller methods.

🚨 Critical Issues

Bug in Update Method's Category Check: The category ownership validation in the update method incorrectly attempts to use an uninitialized $categories variable, which would bypass this critical security check. The proposed refactoring addresses this by ensuring correct variable initialization.
Inconsistent Exception Handling: The update method's catch block does not include the necessary logic to re-throw HttpExceptions, unlike the store method. This means client-side 4xx errors related to ownership validation would be reported as generic 500 errors.

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2025-10-08T23:39:14Z

app/Http/Controllers/API/v1/TransactionController.php

        if (isset($data['categories'])) {
            $categories = $data['categories'];
            unset($data['categories']);
        }


Introduce a new private helper method authorizeTransactionOwnership to centralize and reuse the ownership validation logic for groups, parties, and categories. This method will take the validated data and categories array as arguments, improving modularity and preventing code duplication across controller actions.

Suggested change

}

private function authorizeTransactionOwnership(array $data, array $categories = []): void

{

/** @var \App\Models\User $user */

$user = auth()->user();

// check for group ownership

if (isset($data['group_id']) && $data['group_id'] && !$user->groups()->where('id', $data['group_id'])->exists()) {

throw new HttpException(400, 'The selected group does not belong to user');

}

// check for party ownership

if (isset($data['party_id']) && $data['party_id'] && !$user->parties()->where('id', $data['party_id'])->exists()) {

throw new HttpException(400, 'The selected party does not belong to user');

}

// check for categories ownership

if (!empty($categories)) {

$user_category_ids = $user->categories()->pluck('id')->toArray();

$invalid_categories = array_diff($categories, $user_category_ids);

if (!empty($invalid_categories)) {

throw new HttpException(400, 'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));

}

}

}

}

sourceant · 2025-10-08T23:39:14Z

app/Http/Controllers/API/v1/TransactionController.php

+       $user = auth()->user();
+
+       //check for group
+       if (isset($data['group_id']) && $data['group_id']){
+            if (!$user->groups()->where('id', $data['group_id'])->exists()){
+                throw new HttpException(400,'The selected group does not belong to user');
+            }
+       }
+       //check for party
+       if (isset($data['party_id']) && $data['party_id']) {
+            if (!$user->parties()->where('id', $data['party_id'])->exists()){
+                throw new HttpException(400,'The selected party does not belong to user');
+            }
+       }
+
+       //check for categories
+       if (!empty($categories)){
+            $user_category_ids = $user->categories()->pluck('id')->toArray();  //get user category ids
+            $invalid_categories = array_diff($categories, $user_category_ids);  //find any ids not owned by user
+            if (!empty($invalid_categories)){
+                throw new HttpException(400,'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));
+            }
+       }


These ownership checks are duplicated in both the store and update methods. Extracting this logic into a private helper method will significantly improve maintainability, reduce redundancy, and make your controller actions cleaner. Consider creating a private function authorizeTransactionOwnership(array $data, array $categories = []): void.

Suggested change

$user = auth()->user();

//check for group

if (isset($data['group_id']) && $data['group_id']){

if (!$user->groups()->where('id', $data['group_id'])->exists()){

throw new HttpException(400,'The selected group does not belong to user');

}

}

//check for party

if (isset($data['party_id']) && $data['party_id']) {

if (!$user->parties()->where('id', $data['party_id'])->exists()){

throw new HttpException(400,'The selected party does not belong to user');

}

}

//check for categories

if (!empty($categories)){

$user_category_ids = $user->categories()->pluck('id')->toArray(); //get user category ids

$invalid_categories = array_diff($categories, $user_category_ids); //find any ids not owned by user

if (!empty($invalid_categories)){

throw new HttpException(400,'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));

}

}

$this->authorizeTransactionOwnership($data, $categories);

sourceant · 2025-10-08T23:39:14Z

app/Http/Controllers/API/v1/TransactionController.php

+        $user = auth()->user();
+
+        //check for group
+        if (isset($validatedData['group_id']) && $validatedData['group_id']){
+            if (!$user->groups()->where('id', $validatedData['group_id'])->exists()){
+                throw new HttpException(400,'The selected group does not belong to user');
+
+            }
+        }
+        //check for party
+        if (isset($validatedData['party_id']) && $validatedData['party_id']) {
+            if (!$user->parties()->where('id', $validatedData['party_id'])->exists()){
+                throw new HttpException(400,'The selected party does not belong to user');
+            }
+        }
+
+        //check for categories
+        if (!empty($categories)){
+            $user_category_ids = $user->categories()->pluck('id')->toArray();  //get user category ids
+            $invalid_categories = array_diff($categories, $user_category_ids);  //find any ids not owned by user
+            if (!empty($invalid_categories)){
+                throw new HttpException(400,'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));
+            }
+        }


This block of ownership checks is duplicated from the store method, indicating a need for refactoring into a shared private method. Additionally, the $categories variable is not defined within the scope of the update method's ownership check, leading to a PHP error if categories are present in the request. The refactored helper method should explicitly accept the categories from $validatedData to correct this bug.

Suggested change

$user = auth()->user();

//check for group

if (isset($validatedData['group_id']) && $validatedData['group_id']){

if (!$user->groups()->where('id', $validatedData['group_id'])->exists()){

throw new HttpException(400,'The selected group does not belong to user');

}

}

//check for party

if (isset($validatedData['party_id']) && $validatedData['party_id']) {

if (!$user->parties()->where('id', $validatedData['party_id'])->exists()){

throw new HttpException(400,'The selected party does not belong to user');

}

}

//check for categories

if (!empty($categories)){

$user_category_ids = $user->categories()->pluck('id')->toArray(); //get user category ids

$invalid_categories = array_diff($categories, $user_category_ids); //find any ids not owned by user

if (!empty($invalid_categories)){

throw new HttpException(400,'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));

}

}

$this->authorizeTransactionOwnership($validatedData, $validatedData['categories'] ?? []);

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2025-10-09T11:09:14Z

app/Http/Controllers/API/v1/TransactionController.php

+        if (! empty($categories)) {
+            $user_category_ids = $user->categories()->pluck('id')->toArray();  // get user category ids
+            $invalid_categories = array_diff($categories, $user_category_ids);  // find any ids not owned by user
+            if (! empty($invalid_categories)) {
+                throw new HttpException(400, 'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));
+            }
+        }


The update method's ownership check for categories uses an undefined $categories variable. This will lead to a PHP Undefined variable error if categories are provided in the request. It should directly use $validatedData['categories'] as store method uses $data['categories'] (after assigning it to a local $categories variable). Making this change simplifies the logic and directly uses the validated input.

Suggested change

if (! empty($categories)) {

$user_category_ids = $user->categories()->pluck('id')->toArray(); // get user category ids

$invalid_categories = array_diff($categories, $user_category_ids); // find any ids not owned by user

if (! empty($invalid_categories)) {

throw new HttpException(400, 'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));

}

}

if (isset($validatedData['categories']) && ! empty($validatedData['categories'])) {

$user_category_ids = $user->categories()->pluck('id')->toArray();

$invalid_categories = array_diff($validatedData['categories'], $user_category_ids);

if (! empty($invalid_categories)) {

throw new HttpException(400, 'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));

}

}

kofimokome

We use $this->success() and $this->failure() for success and error messages respectively. Do not throw and HttpException except you want to write a try-catch to catch the exception and use the functions above to return the appriate message to the user.

nfebe · 2025-10-09T16:59:59Z

@kofimokome Someone was already working on this ?

kofimokome · 2025-10-10T09:19:19Z

Yes, I told her to work on it so that she gets familiar with the code. Also the guy hasn't responded to our feedback. If Mercy's work solves the issue and the other guy takes forever to reply, I would merge hers. For now I will leave both open.

@iMercyvlogs split this into two pull requests. One for #99 and the other for #100.

iMercyvlogs · 2025-10-10T10:47:55Z

If I split them, the pull request with the checks will always fail the run tests 🙏🏾.

…

On Fri, 10 Oct 2025 at 10:19 Kofi Mokome ***@***.***> wrote: *kofimokome* left a comment (trakli/webservice#109) <#109 (comment)> Yes, I told her to work on it so that she gets familiar with the code. Also the guy hasn't responded to our feedback. @iMercyvlogs <https://github.com/iMercyvlogs> split this into two pull requests. One for #99 <#99> and the other for #100 <#100>. — Reply to this email directly, view it on GitHub <#109 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2PBDJBQ2LW26EH4EJCBJZD3W5225AVCNFSM6AAAAACIVT4PJ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGOBZGAZTCMZUGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

kofimokome · 2025-10-10T10:50:40Z

Split it, I will merge the other pull request first if it's okay

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2025-10-12T19:15:05Z

app/Http/Controllers/API/v1/TransactionController.php

+            // If an HttpException was thrown earlier (e.g., from ownership check), re-throw it with the correct status code.
+            if ($e instanceof HttpException) {
+                return $this->failure($e->getMessage(), $e->getStatusCode());
+            }


The update method's catch block does not correctly handle HttpExceptions thrown by the newly added ownership checks. This means that a user-facing 400 error would be converted into a generic 500 error, obscuring the actual problem. This change aligns the error handling with the store method, ensuring appropriate HTTP status codes are returned.

Suggested change

}

// If an HttpException was thrown earlier (e.g., from ownership check), re-throw it with the correct status code.

if ($e instanceof HttpException) {

return $this->failure($e->getMessage(), $e->getStatusCode());

}

return $this->failure('Failed to update transaction', 500, [$e->getMessage()]);

sourceant · 2025-10-12T19:15:05Z

app/Http/Controllers/API/v1/TransactionController.php

+       //ownership checks for authenticated user
+       $user = auth()->user();
+
+       //check for group
+       if (isset($data['group_id']) && $data['group_id']){
+            if (!$user->groups()->where('id', $data['group_id'])->exists()){
+                throw new HttpException(400,'The selected group does not belong to user');
+            }
+       }
+       //check for party
+       if (isset($data['party_id']) && $data['party_id']) {
+            if (!$user->parties()->where('id', $data['party_id'])->exists()){
+                throw new HttpException(400,'The selected party does not belong to user');
+            }
+       }
+
+       //check for categories
+       if (!empty($categories)){
+            $user_category_ids = $user->categories()->pluck('id')->toArray();  //get user category ids
+            $invalid_categories = array_diff($categories, $user_category_ids);  //find any ids not owned by user
+            if (!empty($invalid_categories)){
+                throw new HttpException(400,'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));
+            }
+       }


The ownership checks for groups, parties, and categories are duplicated across the store and update methods. This is a prime candidate for refactoring. Extracting this logic into a private helper method will improve maintainability, reduce redundancy, and make the code cleaner. This suggestion replaces the duplicated block with a call to the new validateResourceOwnership method.

Suggested change

//ownership checks for authenticated user

$user = auth()->user();

//check for group

if (isset($data['group_id']) && $data['group_id']){

if (!$user->groups()->where('id', $data['group_id'])->exists()){

throw new HttpException(400,'The selected group does not belong to user');

}

}

//check for party

if (isset($data['party_id']) && $data['party_id']) {

if (!$user->parties()->where('id', $data['party_id'])->exists()){

throw new HttpException(400,'The selected party does not belong to user');

}

}

//check for categories

if (!empty($categories)){

$user_category_ids = $user->categories()->pluck('id')->toArray(); //get user category ids

$invalid_categories = array_diff($categories, $user_category_ids); //find any ids not owned by user

if (!empty($invalid_categories)){

throw new HttpException(400,'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));

}

}

$user = auth()->user();

$this->validateResourceOwnership($user, $data, $categories);

sourceant · 2025-10-12T19:15:05Z

app/Http/Controllers/API/v1/TransactionController.php

+        $user = auth()->user();
+
+        //check for group
+        if (isset($validatedData['group_id']) && $validatedData['group_id']){
+            if (!$user->groups()->where('id', $validatedData['group_id'])->exists()){
+                throw new HttpException(400,'The selected group does not belong to user');
+
+            }
+        }
+        //check for party
+        if (isset($validatedData['party_id']) && $validatedData['party_id']) {
+            if (!$user->parties()->where('id', $validatedData['party_id'])->exists()){
+                throw new HttpException(400,'The selected party does not belong to user');
+            }
+        }
+
+        //check for categories
+        if (!empty($categories)){
+            $user_category_ids = $user->categories()->pluck('id')->toArray();  //get user category ids
+            $invalid_categories = array_diff($categories, $user_category_ids);  //find any ids not owned by user
+            if (!empty($invalid_categories)){
+                throw new HttpException(400,'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));
+            }
+        }


Similar to the store method, the ownership checks here are duplicated logic. Refactor this into a private helper method for better maintainability. Additionally, the original implementation of this block would have led to a bug where category ownership checks are skipped due to the $categories variable being uninitialized in this scope. The suggested code properly initializes $inputCategories for the check.

Suggested change

$user = auth()->user();

//check for group

if (isset($validatedData['group_id']) && $validatedData['group_id']){

if (!$user->groups()->where('id', $validatedData['group_id'])->exists()){

throw new HttpException(400,'The selected group does not belong to user');

}

}

//check for party

if (isset($validatedData['party_id']) && $validatedData['party_id']) {

if (!$user->parties()->where('id', $validatedData['party_id'])->exists()){

throw new HttpException(400,'The selected party does not belong to user');

}

}

//check for categories

if (!empty($categories)){

$user_category_ids = $user->categories()->pluck('id')->toArray(); //get user category ids

$invalid_categories = array_diff($categories, $user_category_ids); //find any ids not owned by user

if (!empty($invalid_categories)){

throw new HttpException(400,'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));

}

}

$user = auth()->user();

$inputCategories = $validatedData['categories'] ?? [];

$this->validateResourceOwnership($user, $validatedData, $inputCategories);

nfebe · 2025-10-15T09:24:31Z

Handled in fix: Verify resource ownership & adjust tests

feat: Check user ownership of groups parties cats

d3f93c3

iMercyvlogs requested a review from kofimokome October 8, 2025 23:38

sourceant bot approved these changes Oct 8, 2025

View reviewed changes

sourceant bot approved these changes Oct 9, 2025

View reviewed changes

iMercyvlogs mentioned this pull request Oct 9, 2025

Ensure groups, categories and parties belong to the user. #99

Closed

kofimokome requested changes Oct 9, 2025

View reviewed changes

iMercyvlogs force-pushed the feat/userCheckPerRequest branch from 2659af5 to d3f93c3 Compare October 12, 2025 19:13

sourceant bot approved these changes Oct 12, 2025

View reviewed changes

nfebe closed this Oct 15, 2025

-        }
+    private function authorizeTransactionOwnership(array $data, array $categories = []): void
+    {
+        /** @var \App\Models\User $user */
+        $user = auth()->user();
+        // check for group ownership
+        if (isset($data['group_id']) && $data['group_id'] && !$user->groups()->where('id', $data['group_id'])->exists()) {
+            throw new HttpException(400, 'The selected group does not belong to user');
+        }
+        // check for party ownership
+        if (isset($data['party_id']) && $data['party_id'] && !$user->parties()->where('id', $data['party_id'])->exists()) {
+            throw new HttpException(400, 'The selected party does not belong to user');
+        }
+        // check for categories ownership
+        if (!empty($categories)) {
+            $user_category_ids = $user->categories()->pluck('id')->toArray();
+            $invalid_categories = array_diff($categories, $user_category_ids);
+            if (!empty($invalid_categories)) {
+                throw new HttpException(400, 'Some of the selected categories do not belong to user. Invalid category IDs: '.implode(',', $invalid_categories));
+            }
+        }
+    }
+}

feat: Check user ownership of groups parties cats #109

feat: Check user ownership of groups parties cats #109

Uh oh!

Conversation

iMercyvlogs commented Oct 8, 2025

Uh oh!

sourceant bot commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Summary

🚀 Key Improvements

💡 Minor Suggestions

🚨 Critical Issues

Uh oh!

sourceant bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourceant bot Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

sourceant bot Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

sourceant bot Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

sourceant bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourceant bot Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

kofimokome left a comment

Choose a reason for hiding this comment

Uh oh!

nfebe commented Oct 9, 2025

Uh oh!

kofimokome commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iMercyvlogs commented Oct 10, 2025 via email

Uh oh!

kofimokome commented Oct 10, 2025

Uh oh!

sourceant bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourceant bot Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

sourceant bot Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

sourceant bot Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

nfebe commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sourceant bot commented Oct 8, 2025 •

edited

Loading

kofimokome commented Oct 10, 2025 •

edited

Loading