-
-
Notifications
You must be signed in to change notification settings - Fork 0
Add authorization checks to WebhookSecretController #41
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.
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
base: dev
Are you sure you want to change the base?
Changes from all commits
49be857
6f6b616
4392570
aafb1e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| namespace Core\Api\Controllers\Api; | ||
|
|
||
| use Illuminate\Foundation\Auth\Access\AuthorizesRequests; | ||
| use Illuminate\Http\JsonResponse; | ||
| use Illuminate\Http\Request; | ||
| use Illuminate\Routing\Controller; | ||
|
|
@@ -16,6 +17,8 @@ | |
| */ | ||
| class WebhookSecretController extends Controller | ||
| { | ||
| use AuthorizesRequests; | ||
|
|
||
| public function __construct( | ||
| protected WebhookSecretRotationService $rotationService | ||
| ) {} | ||
|
|
@@ -39,6 +42,8 @@ public function rotateSocialSecret(Request $request, string $uuid): JsonResponse | |
| return response()->json(['error' => 'Webhook not found'], 404); | ||
| } | ||
|
|
||
| $this->authorize('update', $webhook); | ||
|
|
||
| $validated = $request->validate([ | ||
| 'grace_period_seconds' => 'nullable|integer|min:300|max:604800', // 5 min to 7 days | ||
| ]); | ||
|
|
@@ -77,6 +82,8 @@ public function rotateContentSecret(Request $request, string $uuid): JsonRespons | |
| return response()->json(['error' => 'Webhook endpoint not found'], 404); | ||
| } | ||
|
|
||
| $this->authorize('update', $endpoint); | ||
|
|
||
| $validated = $request->validate([ | ||
| 'grace_period_seconds' => 'nullable|integer|min:300|max:604800', | ||
| ]); | ||
|
|
@@ -115,6 +122,8 @@ public function socialSecretStatus(Request $request, string $uuid): JsonResponse | |
| return response()->json(['error' => 'Webhook not found'], 404); | ||
| } | ||
|
|
||
| $this->authorize('update', $webhook); | ||
|
|
||
| return response()->json([ | ||
| 'data' => $this->rotationService->getSecretStatus($webhook), | ||
| ]); | ||
|
|
@@ -139,6 +148,8 @@ public function contentSecretStatus(Request $request, string $uuid): JsonRespons | |
| return response()->json(['error' => 'Webhook endpoint not found'], 404); | ||
| } | ||
|
|
||
| $this->authorize('update', $endpoint); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to Consider adding a $this->authorize('view', $endpoint); |
||
|
|
||
| return response()->json([ | ||
| 'data' => $this->rotationService->getSecretStatus($endpoint), | ||
| ]); | ||
|
|
@@ -163,6 +174,8 @@ public function invalidateSocialPreviousSecret(Request $request, string $uuid): | |
| return response()->json(['error' => 'Webhook not found'], 404); | ||
| } | ||
|
|
||
| $this->authorize('update', $webhook); | ||
|
|
||
| $this->rotationService->invalidatePreviousSecret($webhook); | ||
|
|
||
| return response()->json([ | ||
|
|
@@ -190,6 +203,8 @@ public function invalidateContentPreviousSecret(Request $request, string $uuid): | |
| return response()->json(['error' => 'Webhook endpoint not found'], 404); | ||
| } | ||
|
|
||
| $this->authorize('update', $endpoint); | ||
|
|
||
| $this->rotationService->invalidatePreviousSecret($endpoint); | ||
|
|
||
| return response()->json([ | ||
|
|
@@ -217,6 +232,8 @@ public function updateSocialGracePeriod(Request $request, string $uuid): JsonRes | |
| return response()->json(['error' => 'Webhook not found'], 404); | ||
| } | ||
|
|
||
| $this->authorize('update', $webhook); | ||
|
|
||
| $validated = $request->validate([ | ||
| 'grace_period_seconds' => 'required|integer|min:300|max:604800', | ||
| ]); | ||
|
|
@@ -251,6 +268,8 @@ public function updateContentGracePeriod(Request $request, string $uuid): JsonRe | |
| return response()->json(['error' => 'Webhook endpoint not found'], 404); | ||
| } | ||
|
|
||
| $this->authorize('update', $endpoint); | ||
|
|
||
| $validated = $request->validate([ | ||
| 'grace_period_seconds' => 'required|integer|min:300|max:604800', | ||
| ]); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Core\Api\Policies; | ||
|
|
||
| use Core\Tenant\Models\User; | ||
| use Illuminate\Auth\Access\HandlesAuthorization; | ||
|
|
||
| class WebhookPolicy | ||
| { | ||
| use HandlesAuthorization; | ||
|
|
||
| /** | ||
| * Determine if the user can update the webhook. | ||
| * | ||
| * Only workspace owners and admins can manage webhook secrets | ||
| * and rotation settings. | ||
| * | ||
| * @param User $user | ||
| * @param mixed $webhook Social Webhook or Content Webhook Endpoint | ||
| * @return bool | ||
| */ | ||
| public function update(User $user, mixed $webhook): bool | ||
|
Comment on lines
+21
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For improved type safety and code clarity, consider using a union type for the Using fully qualified names here makes the code a bit verbose. You could also add * @param \Core\Social\Models\Webhook|\Core\Content\Models\ContentWebhookEndpoint $webhook Social Webhook or Content Webhook Endpoint
* @return bool
*/
public function update(User $user, \Core\Social\Models\Webhook|\Core\Content\Models\ContentWebhookEndpoint $webhook): bool |
||
| { | ||
| return $user->workspaces() | ||
| ->where('workspaces.id', $webhook->workspace_id) | ||
| ->wherePivotIn('role', ['owner', 'admin']) | ||
| ->exists(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Core\Api\Tests\Feature; | ||
|
|
||
| use Core\Social\Models\Webhook; | ||
| use Core\Tenant\Models\User; | ||
| use Core\Tenant\Models\Workspace; | ||
| use Illuminate\Support\Str; | ||
|
|
||
| uses(\Illuminate\Foundation\Testing\RefreshDatabase::class); | ||
|
|
||
| // Skip if models don't exist in local sandbox environment | ||
| if (class_exists('Core\Tenant\Models\Workspace')) { | ||
| beforeEach(function () { | ||
| $this->workspace = Workspace::factory()->create(); | ||
| $this->webhook = Webhook::factory()->create([ | ||
| 'workspace_id' => $this->workspace->id, | ||
| 'uuid' => Str::uuid()->toString(), | ||
| ]); | ||
|
|
||
| // Create users with different roles | ||
| $this->owner = User::factory()->create(); | ||
| $this->workspace->users()->attach($this->owner->id, ['role' => 'owner']); | ||
|
|
||
| $this->admin = User::factory()->create(); | ||
| $this->workspace->users()->attach($this->admin->id, ['role' => 'admin']); | ||
|
|
||
| $this->member = User::factory()->create(); | ||
| $this->workspace->users()->attach($this->member->id, ['role' => 'member']); | ||
| }); | ||
|
|
||
| describe('Webhook Authorization', function () { | ||
| it('allows owner to rotate social secret', function () { | ||
| $this->actingAs($this->owner) | ||
| ->postJson("/api/webhooks/social/{$this->webhook->uuid}/rotate") | ||
| ->assertOk(); | ||
| }); | ||
|
|
||
| it('denies member from rotating social secret', function () { | ||
| $this->actingAs($this->member) | ||
| ->postJson("/api/webhooks/social/{$this->webhook->uuid}/rotate") | ||
| ->assertStatus(403); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better semantic clarity and maintainability, it's recommended to use a
viewauthorization policy for read-only actions likesocialSecretStatus. While using theupdatepolicy works, it can be confusing as this method doesn't modify any data.Consider adding a
viewmethod toWebhookPolicyand calling$this->authorize('view', $webhook);here.Example in
WebhookPolicy.php:This would make the authorization intent clearer. The same applies to
contentSecretStatus.