Skip to content

feat: Check user ownership of groups parties cats #109

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

feat: Check user ownership of groups parties cats #109

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions app/Http/Controllers/API/v1/TransactionController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,32 @@ public function store(Request $request): JsonResponse
unset($data['categories']);
}
Copy link

@sourceant sourceant bot Oct 8, 2025

Choose a reason for hiding this comment

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

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));
}
}
}
}



//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));
}
}
Comment on lines +265 to +288
Copy link

@sourceant sourceant bot Oct 12, 2025

Choose a reason for hiding this comment

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

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);


try {
$transaction = DB::transaction(function () use ($request, $data, $categories, $recurring_transaction_data) {
/** @var Transaction $transaction */
Expand Down Expand Up @@ -311,6 +337,10 @@ public function store(Request $request): JsonResponse
'exception' => $e,
]);

// 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());
}
Copy link

@sourceant sourceant bot Oct 12, 2025

Choose a reason for hiding this comment

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

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()]);

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

Expand Down Expand Up @@ -546,6 +576,33 @@ public function update(Request $request, $id): JsonResponse
} catch (HttpException $e) {
return $this->failure($e->getMessage(), $e->getStatusCode());
}

//check ownership of fields passed in request
$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));
}
}
Comment on lines +581 to +604
Copy link

@sourceant sourceant bot Oct 12, 2025

Choose a reason for hiding this comment

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

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);


try {
$transaction = DB::transaction(function () use ($validatedData, $transaction, $recurring_transaction_data, $request) {

Expand Down
Loading