feat(auth): implement Facebook data deletion callback and backfill command - #150
feat(auth): implement Facebook data deletion callback and backfill command#150smarcet wants to merge 7 commits into
Conversation
…mmand Adds the Data Deletion Request Callback Facebook requires apps to implement (https://developers.facebook.com/documentation/development/create-an-app/app-dashboard/data-deletion-callback): - POST /api/public/v1/facebook/data-deletion verifies the HMAC-SHA256 signed_request, unlinks the matching user's Facebook identity (external_id/external_provider/external_pic), and returns the required {url, confirmation_code} JSON. - GET /api/public/v1/facebook/data-deletion/status/{code} renders a human-readable status page, per Facebook's spec. - idp:facebook-data-deletion-backfill processes the CSV of pending app-scoped IDs already queued in the app dashboard. FacebookDataDeletionService::processDeletionRequest() is idempotent: a (provider, external_id) unique constraint plus a re-select-on-collision fallback ensure a duplicate submission (Facebook retry, or an ID already handled by the live callback later appearing in a CSV backfill) never re-processes the user or creates a duplicate audit row. All facebook_deletion_requests reads/writes route through the same Doctrine connection the entity flush uses, resolved fresh per call, so the audit row and the user unlink commit or roll back together.
📝 WalkthroughWalkthroughAdds Facebook data deletion support through signed callbacks, persistent request tracking, status pages, and an idempotent CSV backfill command. The change includes service wiring, nullable Facebook user fields, repository lookup, migration, routes, workflows, and automated tests. ChangesFacebook data deletion
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Facebook
participant FacebookDataDeletionController
participant FacebookSignedRequestParser
participant FacebookDataDeletionService
participant Doctrine
Facebook->>FacebookDataDeletionController: Send signed deletion callback
FacebookDataDeletionController->>FacebookSignedRequestParser: Validate signed_request
FacebookSignedRequestParser-->>FacebookDataDeletionController: Return user_id or null
FacebookDataDeletionController->>FacebookDataDeletionService: Process deletion request
FacebookDataDeletionService->>Doctrine: Unlink user and store request
Doctrine-->>FacebookDataDeletionService: Return confirmation status
FacebookDataDeletionService-->>FacebookDataDeletionController: Return confirmation data
FacebookDataDeletionController-->>Facebook: Return confirmation code and status URL
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-150/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Console/Commands/FacebookDataDeletionBackfill.php`:
- Line 96: Update FacebookDataDeletionBackfill::handle so the Log::debug message
no longer includes the sensitive $external_id; retain only the confirmation
result or a non-sensitive aggregate/status message.
In `@app/Http/Controllers/Api/FacebookDataDeletionController.php`:
- Around line 68-73: Trace processDeletionRequest in FacebookDataDeletionService
together with the facebook_deletion_requests schema and DoctrineUserRepository
update flow. Resolve the exception during the transaction, deletion-request
insert, or user unlink so a valid matched-user request commits successfully and
returns the existing url and confirmation_code fields from the controller.
Preserve rollback behavior for failures and do not alter the callback response
contract.
- Around line 49-59: Update the signed_request validation in
FacebookDataDeletionController::handle to reject every non-string input,
including non-empty arrays, before calling FacebookSignedRequestParser::parse.
Preserve the existing missing-value response and return HTTP 400 with the
invalid_request payload for all rejected types. Add a functional test covering
an array-valued signed_request and asserting a 400 response.
In `@database/migrations/Version20260806190000.php`:
- Around line 28-43: Update database/migrations/Version20260806190000.php at
lines 28-43 to alter users.external_id, users.external_provider, and
users.external_pic as nullable columns, and update app/libs/Auth/Models/User.php
at lines 2194-2228 to add nullable: true to the ORM mappings for those same
fields. Ensure both the database schema and User mappings permit Facebook
identity fields to be set to null.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1df6e9f9-1979-46cb-a3e9-6c314ff1838a
📒 Files selected for processing (17)
app/Console/Commands/FacebookDataDeletionBackfill.phpapp/Console/Kernel.phpapp/Http/Controllers/Api/FacebookDataDeletionController.phpapp/Repositories/DoctrineUserRepository.phpapp/Services/Auth/FacebookDataDeletionService.phpapp/Services/Auth/IFacebookDataDeletionService.phpapp/Services/ServicesProvider.phpapp/libs/Auth/FacebookSignedRequestParser.phpapp/libs/Auth/Models/User.phpapp/libs/Auth/Repositories/IUserRepository.phpdatabase/migrations/Version20260806190000.phpresources/views/auth/facebook_data_deletion_status.blade.phproutes/api_public.phptests/FacebookDataDeletionApiTest.phptests/FacebookDataDeletionBackfillCommandTest.phptests/unit/FacebookDataDeletionServiceRaceTest.phptests/unit/FacebookSignedRequestParserTest.php
| public function up(Schema $schema):void | ||
| { | ||
| $builder = new Builder($schema); | ||
|
|
||
| if (!$builder->hasTable("facebook_deletion_requests")) { | ||
| $builder->create("facebook_deletion_requests", function (Table $table) { | ||
| $table->increments('id'); | ||
| $table->timestamps(); | ||
| $table->string("provider")->setNotnull(true); | ||
| $table->string("external_id")->setNotnull(true); | ||
| $table->string("confirmation_code")->setNotnull(true); | ||
| $table->string("status")->setNotnull(true); | ||
| $table->integer("user_id")->setNotnull(false); | ||
| $table->unique(["provider", "external_id"]); | ||
| $table->unique("confirmation_code"); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Make the Facebook identity columns nullable before unlinking them.
FacebookDataDeletionService sets all three Facebook identity fields to null. The User ORM mappings remain non-nullable, and this migration does not alter the existing users columns. A matched deletion request will fail when Doctrine flushes the user update.
database/migrations/Version20260806190000.php#L28-L43: alterusers.external_id,users.external_provider, andusers.external_picto acceptNULL.app/libs/Auth/Models/User.php#L2194-L2228: addnullable: trueto the ORM mappings for the same three fields.
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 --glob '*.php' 'external_(id|provider|pic)' app/libs/Auth/Models/User.php database/migrations📍 Affects 2 files
database/migrations/Version20260806190000.php#L28-L43(this comment)app/libs/Auth/Models/User.php#L2194-L2228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/migrations/Version20260806190000.php` around lines 28 - 43, Update
database/migrations/Version20260806190000.php at lines 28-43 to alter
users.external_id, users.external_provider, and users.external_pic as nullable
columns, and update app/libs/Auth/Models/User.php at lines 2194-2228 to add
nullable: true to the ORM mappings for those same fields. Ensure both the
database schema and User mappings permit Facebook identity fields to be set to
null.
There was a problem hiding this comment.
Pull request overview
Adds first-class support for Facebook’s Data Deletion Request Callback workflow to OpenStackID, including a public API callback endpoint, a status page, persistent auditing, and a CLI backfill command for already-exported app-scoped IDs.
Changes:
- Implemented
POST /api/public/v1/facebook/data-deletion(signed_request verification + unlinking) andGET .../status/{confirmation_code}status page. - Added
facebook_deletion_requestsaudit table + a backfill command to process Facebook CSV exports idempotently. - Extended user repository APIs + user external-* setters to support unlinking, and added unit/functional test coverage.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/FacebookSignedRequestParserTest.php | Unit tests for signed_request parsing/verification. |
| tests/unit/FacebookDataDeletionServiceRaceTest.php | Unit test covering unique-constraint collision/idempotency behavior. |
| tests/FacebookDataDeletionBackfillCommandTest.php | Functional tests for the backfill command behavior and idempotency. |
| tests/FacebookDataDeletionApiTest.php | Functional tests for callback/status endpoints and unlink behavior. |
| routes/api_public.php | Registers the public Facebook callback + status routes (throttled). |
| resources/views/auth/facebook_data_deletion_status.blade.php | Human-readable status page for confirmation codes. |
| database/migrations/Version20260806190000.php | Adds facebook_deletion_requests table with uniqueness constraints. |
| app/Services/ServicesProvider.php | Registers IFacebookDataDeletionService binding in the container. |
| app/Services/Auth/IFacebookDataDeletionService.php | Defines service contract for processing deletions and fetching status. |
| app/Services/Auth/FacebookDataDeletionService.php | Core unlink + audit write logic with idempotency handling. |
| app/Repositories/DoctrineUserRepository.php | Adds repository lookup by (external_provider, external_id). |
| app/libs/Auth/Repositories/IUserRepository.php | Exposes getByExternalId in the repository interface. |
| app/libs/Auth/Models/User.php | Allows nulling external identity fields via nullable setters. |
| app/libs/Auth/FacebookSignedRequestParser.php | Implements signed_request parsing + HMAC verification. |
| app/Http/Controllers/Api/FacebookDataDeletionController.php | Implements callback handler + status page controller actions. |
| app/Console/Kernel.php | Registers the backfill Artisan command. |
| app/Console/Commands/FacebookDataDeletionBackfill.php | CLI command to process a CSV export and unlink app-scoped IDs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static function base64UrlDecode(string $input): string | ||
| { | ||
| return base64_decode(strtr($input, '-_', '+/')); | ||
| } |
| $secret = Config::get('services.facebook.client_secret'); | ||
| $data = FacebookSignedRequestParser::parse($signed_request, $secret); | ||
|
|
| $handle = fopen($path, 'r'); | ||
| while (($line = fgets($handle)) !== false) { |
| $registry = $this->createMock(ManagerRegistry::class); | ||
| $registry->method('getManager')->willReturn($em); | ||
| Registry::swap($registry); | ||
|
|
||
| $tx_service = $this->createMock(ITransactionService::class); | ||
| $tx_service->method('transaction')->willReturnCallback(fn($callback) => $callback()); | ||
|
|
||
| $service = new FacebookDataDeletionService($user_repository, $tx_service); | ||
|
|
||
| $result = $service->processDeletionRequest('racing-asid'); | ||
|
|
||
| $this->assertSame('winner-confirmation-code', $result['confirmation_code']); | ||
| $this->assertSame('not_found', $result['status']); | ||
| } |
| /** | ||
| * @param string $external_provider | ||
| * @param string|null $external_provider | ||
| */ | ||
| public function setExternalProvider(string $external_provider): void | ||
| public function setExternalProvider(?string $external_provider): void |
| /** | ||
| * @param string $external_pic | ||
| * @param string|null $external_pic | ||
| */ | ||
| public function setExternalPic(string $external_pic): void | ||
| public function setExternalPic(?string $external_pic): void |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-150/ This page is automatically updated on each push to this PR. |
Root cause of the CI failure on testMatchedUserIsUnlinkedAndReturnsConfirmation:
.env.testing (which supplies FACEBOOK_CLIENT_SECRET locally) is gitignored,
and none of the three GitHub Actions workflows ever set FACEBOOK_CLIENT_ID/
FACEBOOK_CLIENT_SECRET - a pre-existing gap this PR was the first to depend
on. Config::get('services.facebook.client_secret') resolved to null in CI,
and FacebookSignedRequestParser::parse()'s non-nullable string $secret
parameter turned that into a fatal TypeError (500) instead of a clean
rejection.
- Widen parse()'s $secret to ?string and reject immediately on null/'' -
a webhook endpoint should fail closed with 400 on missing server config,
never 500.
- Add FACEBOOK_CLIENT_ID/FACEBOOK_CLIENT_SECRET/FACEBOOK_REDIRECT_URI
(same dummy test values as .env.testing) to all three CI workflows.
- Two new parser tests lock in the defensive null/empty-secret behavior.
Reverts the temporary response-body-dump instrumentation from 477109d,
which was used to capture the real TypeError from CI's APP_DEBUG=true
error page.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-150/ This page is automatically updated on each push to this PR. |
…rror
Request::input('signed_request') can return an array (e.g. array-notation
POST body signed_request[]=x), which empty() treats as non-empty and passes
through to FacebookSignedRequestParser::parse(string $signed_request, ...).
That non-nullable string param throws an uncaught TypeError on an array
argument, turning a malformed request to this public unauthenticated
endpoint into a 500 instead of the existing clean 400 response.
…requests.user_id Every other user_id/performer_id column in this codebase (users_deleted, users_email_changed, banned_ips, oauth2_client, ...) is bigInteger() ->setUnsigned(true) with an explicit index() and foreign() constraint. This table's user_id was a plain, unindexed, un-constrained integer: no referential integrity, a full table scan on lookups by user, and a type mismatch against users.id (bigint unsigned). ON DELETE SET NULL (not CASCADE) preserves the audit row if a user is later hard-deleted, matching oauth2_client_user_id_foreign's precedent for the same case. Verified by rolling the migration down/up against idp-db-local's idp_test database and inspecting the resulting SHOW CREATE TABLE output.
Log::debug wrote the raw external_id being unlinked, which — if LOG_LEVEL is ever raised above the default 'error' — writes the very identifier the deletion request exists to purge into log files, undermining the point of the deletion. Log the outcome status instead; the confirmation_code/status are already the durable audit trail via facebook_deletion_requests.
is_readable() passing doesn't guarantee fopen() succeeds (fd exhaustion, file removed mid-check). An unchecked fopen() failure previously crashed the command (PHP escalates the fopen() warning to an ErrorException, or fgets() throws TypeError on a false stream) instead of using the command's own clean $this->error(...); return 1; pattern already used for the is_readable check two lines above. Test uses a minimal custom stream wrapper (url_stat succeeds, stream_open returns false) to deterministically reproduce the gap without depending on a real filesystem race.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-150/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/libs/Auth/FacebookSignedRequestParser.php (2)
41-43: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate decoded field types before using string APIs.
json_decode(..., true)can decodealgorithmas an array;strtoupper()throws aTypeErrorbefore HMAC signature validation. A non-empty arrayuser_idalso passesempty()and is cast byFacebookDataDeletionController::handlebefore deletion. Require string values for both fields before comparing or forwarding them.< details>
< summary>Proposed fix- if (strtoupper($data['algorithm'] ?? '') !== 'HMAC-SHA256') return null; - if (empty($data['user_id'])) return null; + $algorithm = $data['algorithm'] ?? null; + $user_id = $data['user_id'] ?? null; + if (!is_string($algorithm) || strtoupper($algorithm) !== 'HMAC-SHA256') return null; + if (!is_string($user_id) || $user_id === '') return null;</ details>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/libs/Auth/FacebookSignedRequestParser.php` around lines 41 - 43, In the signed-request parsing validation, require both data['algorithm'] and data['user_id'] to be strings before calling strtoupper or accepting the identifier. Update the checks following the is_array($data) guard, while preserving the existing HMAC-SHA256 comparison and empty-value rejection for valid strings.
56-59: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject malformed Base64Url components before decoding.
parse()is type-safe for malformed input, butbase64UrlDecode()can return valid binary bytes from inputs such as---orY_9=. Since Facebooksigned_requestcomponents are Base64URL alphabet, validate both decoded components before passing either tojson_decode()orhash_equals().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/libs/Auth/FacebookSignedRequestParser.php` around lines 56 - 59, Update FacebookSignedRequestParser::base64UrlDecode and its callers in parse() to validate each signed_request component against the Base64URL format before decoding, rejecting malformed or invalidly padded input rather than passing decoded bytes to json_decode() or hash_equals(). Preserve the existing type-safe malformed-input behavior and only decode components that pass validation.
🧹 Nitpick comments (4)
tests/FacebookDataDeletionApiTest.php (3)
126-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
$responseassignment.The test only needs to execute the POST request. Call
$this->post(...)without assigning its return value. This removes the PHPMDUnusedLocalVariablewarning.Proposed fix
- $response = $this->post(self::CallbackUri, ['signed_request' => ['a', 'b']]); + $this->post(self::CallbackUri, ['signed_request' => ['a', 'b']]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/FacebookDataDeletionApiTest.php` around lines 126 - 131, Remove the unused $response assignment in testArraySignedRequestReturns400InsteadOf500 and invoke $this->post(...) directly, preserving the existing request arguments and status assertion.Source: Linters/SAST tools
60-82: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that non-Facebook data remains unchanged.
The test verifies that the user remains available and that Facebook fields are cleared. It does not verify that non-Facebook data survives. Capture one seeded non-Facebook field before the callback and compare it after reloading the user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/FacebookDataDeletionApiTest.php` around lines 60 - 82, The test method testMatchedUserIsUnlinkedAndReturnsConfirmation should capture a seeded non-Facebook user field before posting the callback, then assert the same field remains unchanged on the reloaded user. Keep the existing Facebook-field clearing and deletion-request assertions intact.
140-157: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise idempotency for a matched user.
This test submits an unmatched identifier, so it covers only repeated
not_foundprocessing. Link the seeded user before the first submission and assert one completed request plus cleared Facebook fields after the second submission. This covers idempotency on the state-mutating path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/FacebookDataDeletionApiTest.php` around lines 140 - 157, Update testDuplicateSubmissionIsIdempotent to associate the seeded user with the Facebook identifier before the first post, then assert both submissions return the same confirmation code, exactly one completed deletion request exists, and the user’s Facebook fields are cleared after the second submission.tests/FacebookDataDeletionBackfillCommandTest.php (1)
64-65: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify all Facebook identity fields after backfill.
This test verifies only
external_id. The deletion service also clearsexternal_providerandexternal_pic. Seed a non-empty picture, then assert that the provider and picture are null after the command completes. This detects partial identity deletion regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/FacebookDataDeletionBackfillCommandTest.php` around lines 64 - 65, Update the test around the reloaded User assertion to seed a non-empty external picture and provider before running the backfill command, then assert that getExternalId(), getExternalProvider(), and getExternalPic() all return null afterward. Preserve the existing external ID assertion while covering every Facebook identity field cleared by the deletion service.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pull_request_unit_tests.yml:
- Line 41: Remove the hard-coded Facebook application secret and use the GitHub
Actions secret reference or a non-sensitive test fixture in all affected
workflows: .github/workflows/pull_request_unit_tests.yml lines 41-41,
.github/workflows/nightly_unit_tests.yml lines 36-36, and
.github/workflows/push.yml lines 37-37. Rotate the exposed credential before
merging.
In `@app/libs/Auth/FacebookSignedRequestParser.php`:
- Around line 26-31: Update FacebookSignedRequestParser::parse to accept a mixed
secret at the boundary, returning null unless the value is a non-empty string;
preserve the existing null/empty rejection and parsing behavior for valid
secrets, so FacebookDataDeletionController::handle can return its normal 400
response instead of encountering a TypeError.
In `@tests/FacebookDataDeletionBackfillCommandTest.php`:
- Around line 90-100: Update testDoesNotLogExternalId to store the Mockery spy
returned by Log::spy(), then invoke shouldNotHaveReceived on that spy instance
instead of calling the assertion statically through Log. Preserve the existing
debug-message and external-ID matching behavior.
---
Outside diff comments:
In `@app/libs/Auth/FacebookSignedRequestParser.php`:
- Around line 41-43: In the signed-request parsing validation, require both
data['algorithm'] and data['user_id'] to be strings before calling strtoupper or
accepting the identifier. Update the checks following the is_array($data) guard,
while preserving the existing HMAC-SHA256 comparison and empty-value rejection
for valid strings.
- Around line 56-59: Update FacebookSignedRequestParser::base64UrlDecode and its
callers in parse() to validate each signed_request component against the
Base64URL format before decoding, rejecting malformed or invalidly padded input
rather than passing decoded bytes to json_decode() or hash_equals(). Preserve
the existing type-safe malformed-input behavior and only decode components that
pass validation.
---
Nitpick comments:
In `@tests/FacebookDataDeletionApiTest.php`:
- Around line 126-131: Remove the unused $response assignment in
testArraySignedRequestReturns400InsteadOf500 and invoke $this->post(...)
directly, preserving the existing request arguments and status assertion.
- Around line 60-82: The test method
testMatchedUserIsUnlinkedAndReturnsConfirmation should capture a seeded
non-Facebook user field before posting the callback, then assert the same field
remains unchanged on the reloaded user. Keep the existing Facebook-field
clearing and deletion-request assertions intact.
- Around line 140-157: Update testDuplicateSubmissionIsIdempotent to associate
the seeded user with the Facebook identifier before the first post, then assert
both submissions return the same confirmation code, exactly one completed
deletion request exists, and the user’s Facebook fields are cleared after the
second submission.
In `@tests/FacebookDataDeletionBackfillCommandTest.php`:
- Around line 64-65: Update the test around the reloaded User assertion to seed
a non-empty external picture and provider before running the backfill command,
then assert that getExternalId(), getExternalProvider(), and getExternalPic()
all return null afterward. Preserve the existing external ID assertion while
covering every Facebook identity field cleared by the deletion service.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e88fc57-5c12-4fc2-9ad9-520315056516
📒 Files selected for processing (10)
.github/workflows/nightly_unit_tests.yml.github/workflows/pull_request_unit_tests.yml.github/workflows/push.ymlapp/Console/Commands/FacebookDataDeletionBackfill.phpapp/Http/Controllers/Api/FacebookDataDeletionController.phpapp/libs/Auth/FacebookSignedRequestParser.phpdatabase/migrations/Version20260806190000.phptests/FacebookDataDeletionApiTest.phptests/FacebookDataDeletionBackfillCommandTest.phptests/unit/FacebookSignedRequestParserTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- app/Http/Controllers/Api/FacebookDataDeletionController.php
- app/Console/Commands/FacebookDataDeletionBackfill.php
- database/migrations/Version20260806190000.php
| OTEL_SDK_DISABLED: true | ||
| OTEL_SERVICE_ENABLED: false | ||
| FACEBOOK_CLIENT_ID: 214242500242860 | ||
| FACEBOOK_CLIENT_SECRET: e62fa81aa898699d8cebf14bf5e586aa |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the hard-coded Facebook application secret from all CI workflows.
The same credential is committed in three files and is exposed to job steps. Replace each value with ${{ secrets.FACEBOOK_CLIENT_SECRET }} or a non-sensitive test fixture. Rotate the exposed credential before merge.
.github/workflows/pull_request_unit_tests.yml#L41-L41: replace the plaintext secret in the pull-request test job..github/workflows/nightly_unit_tests.yml#L36-L36: replace the plaintext secret in the nightly test job..github/workflows/push.yml#L37-L37: replace the plaintext secret in the push test job.
🧰 Tools
🪛 Betterleaks (1.7.3)
[high] 41-41: Discovered a Facebook Application secret, posing a risk of unauthorized access to Facebook accounts and personal data exposure.
(facebook-secret)
📍 Affects 3 files
.github/workflows/pull_request_unit_tests.yml#L41-L41(this comment).github/workflows/nightly_unit_tests.yml#L36-L36.github/workflows/push.yml#L37-L37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pull_request_unit_tests.yml at line 41, Remove the
hard-coded Facebook application secret and use the GitHub Actions secret
reference or a non-sensitive test fixture in all affected workflows:
.github/workflows/pull_request_unit_tests.yml lines 41-41,
.github/workflows/nightly_unit_tests.yml lines 36-36, and
.github/workflows/push.yml lines 37-37. Rotate the exposed credential before
merging.
Source: Linters/SAST tools
| * @param string|null $secret | ||
| * @return array|null | ||
| */ | ||
| public static function parse(string $signed_request, ?string $secret): ?array | ||
| { | ||
| if ($secret === null || $secret === '') return null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
fd -a 'FacebookSignedRequestParser.php|FacebookDataDeletionController.php' . || true
printf '\nGit status (short):\n'
git status --short
printf '\nParser file:\n'
if [ -f app/libs/Auth/FacebookSignedRequestParser.php ]; then
nl -ba app/libs/Auth/FacebookSignedRequestParser.php | sed -n '1,140p'
fi || true
printf '\nController relevant lines:\n'
if [ -f app/Http/Controllers/Api/FacebookDataDeletionController.php ]; then
nl -ba app/Http/Controllers/Api/FacebookDataDeletionController.php | sed -n '1,130p'
fi || true
printf '\nSearch usages of FacebookSignedRequestParser::parse:\n'
rg -n "FacebookSignedRequestParser::parse|FacebookSignedRequestParser" . || trueRepository: OpenStackweb/openstackid
Length of output: 2447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Parser file with line numbers:\n'
awk '{printf "%6d %s\n", NR, $0}' app/libs/Auth/FacebookSignedRequestParser.php | sed -n '1,180p'
printf '\nController relevant lines with line numbers:\n'
awk '{printf "%6d %s\n", NR, $0}' app/Http/Controllers/Api/FacebookDataDeletionController.php | sed -n '1,120p'
printf '\nTest relevant lines with line numbers:\n'
awk '{printf "%6d %s\n", NR, $0}' tests/unit/FacebookSignedRequestParserTest.php | sed -n '1,140p'Repository: OpenStackweb/openstackid
Length of output: 11492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'PHP version:\n'
php -v | head -n1 || true
printf '\nStrict scalar coercion probe:\n'
php -r 'try { FacebookSignedRequestParserMock.php; } catch (Error $e) { echo $e->getMessage(), "\n"; }' <<'PHPCODE'
<?php
declare(strict_types=1);
class Proxy {
public static function parse(?string $secret): string {
return $secret ?? '';
}
}
try {
Proxy::parse('numeric-ish');
} catch (Error $e) {
echo "string coerces: " . get_class($e) . ": " . $e->getMessage() . "\n";
}
try {
Proxy::parse(['array-secret']);
} catch (Error $e) {
echo "array does not coerce: " . get_class($e) . ": " . $e->getMessage() . "\n";
}
try {
Proxy::parse(123);
} catch (Error $e) {
echo "int does not coerce: " . get_class($e) . ": " . $e->getMessage() . "\n";
}
class FacebookSignedRequestParserMock {
public static function parse(string $signed_request, ?string $secret): ?array {
if ($secret === null || $secret === '') return null;
$expected_sig = hash_hmac('sha256', $signed_request, $secret, true);
return ['user_id' => 'test'];
}
private static function base64UrlDecode(string $input): string {
return base64_decode(strtr($input, '-_', '+/'));
}
}
PHPCODE
printf '\nNon-strict optional string coercion probe:\n'
PHP_OPTIONS='PHPDEBAGERRORREPRATION=0 /tmp/coerce_probe.php' php <<'PHPCODE'
<?php
class Proxy {
public static function parse(?string $secret): string {
return $secret ?? '';
}
}
foreach (['numeric-ish', ['array-secret'], 123, 0, false, null] as $secret) {
try {
echo gettype($secret) . " -> " . Proxy::parse($secret) . "\n";
} catch (Error $e) {
echo gettype($secret) . " throws: " . $e->getMessage() . "\n";
}
}
PHPCODERepository: OpenStackweb/openstackid
Length of output: 568
Reject non-string secrets at the parser boundary.
FacebookDataDeletionController::handle() passes Config::get('services.facebook.client_secret') directly to parse(). If that config value is an array, the ?string $secret parameter raises TypeError before the null check, so the callback cannot return 400. Accept mixed and reject non-empty-string values, or validate the config value before calling parse().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/libs/Auth/FacebookSignedRequestParser.php` around lines 26 - 31, Update
FacebookSignedRequestParser::parse to accept a mixed secret at the boundary,
returning null unless the value is a non-empty string; preserve the existing
null/empty rejection and parsing behavior for valid secrets, so
FacebookDataDeletionController::handle can return its normal 400 response
instead of encountering a TypeError.
| public function testDoesNotLogExternalId(): void | ||
| { | ||
| Log::spy(); | ||
| $asid = '555000444'; | ||
| $csv_path = $this->writeCsvFixture([$asid]); | ||
|
|
||
| Artisan::call(self::Command, ['path' => $csv_path]); | ||
|
|
||
| Log::shouldNotHaveReceived('debug', function (string $message) use ($asid) { | ||
| return str_contains($message, $asid); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether PHPStan analyzes this test and whether the error is ignored.
fd -a 'phpstan*.neon*' 'composer.json' 'composer.lock' . -E vendor
rg -n -C 3 \
'FacebookDataDeletionBackfillCommandTest|shouldNotHaveReceived|ignoreErrors|paths:' \
--glob 'phpstan*.neon*' --glob 'composer.json' --glob 'composer.lock' .Repository: OpenStackweb/openstackid
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files of interest:"
git ls-files | rg '(^|/)(phpstan[^/]*\.neon|composer\.(json|lock)|tests/FacebookDataDeletionBackfillCommandTest\.php)$' || true
echo
echo "Test file excerpt:"
if [ -f tests/FacebookDataDeletionBackfillCommandTest.php ]; then
sed -n '70,115p' tests/FacebookDataDeletionBackfillCommandTest.php | cat -n
fi
echo
echo "PHPStan/test related occurrences:"
rg -n -C 2 \
'FacebookDataDeletionBackfillCommandTest|shouldNotHaveReceived|ignoreErrors|paths:|phpstan' \
--glob '*.neon*' --glob 'composer.json' --glob 'composer.lock' . || trueRepository: OpenStackweb/openstackid
Length of output: 26879
🌐 Web query:
Laravel Log facade facades.php shouldNotHaveReceived spy return value shouldReceive
💡 Result:
When testing with the Laravel Log facade, it is important to distinguish between using shouldReceive (which sets up a mock expectation) and spy (which records interactions for later verification). 1. Spies (spy): When you call Log::spy(), Laravel replaces the Log facade with a spy object that records all calls made to it [1][2]. This allows you to perform assertions after the code execution using shouldHaveReceived() [1][3][2]. 2. Verifying non-execution (shouldNotHaveReceived): You can use Log::shouldNotHaveReceived('methodName') to assert that a specific logging method was not called [3]. Note that Mockery, which powers this, does not support argument matching for shouldNotHaveReceived() with the same fluency as shouldHaveReceived() [3]. 3. Return values: Spies do not automatically handle return values like mocks do. If your code requires the Log facade to return a specific object (such as when chaining methods like Log::channel('name')->info()), using a simple spy() can result in a "Call to a member function on null" error because the spy does not inherently know how to handle the channel() method call [4][5]. 4. Workarounds for chains: For chained calls like Log::channel('logistics')->info(), simply calling Log::spy() often fails [4][5]. Developers typically address this by using Log::shouldReceive('channel->info') to explicitly mock the chain, or by using specialized packages like timacdonald/log-fake designed to handle log channels correctly in tests [4][6][5]. 5. Spies vs. Mocks: If you need to define a specific return value for a method call during a test, you must use shouldReceive() (creating a mock) rather than spy() [3][7]. In summary, use spy() and shouldHaveReceived() for verifying that logging occurred, and use shouldReceive() when you need to control return values or mock complex method chains [3][4].
Citations:
- 1: https://laravel.com/docs/13.x/mocking
- 2: https://laravel.com/docs/12.x/mocking
- 3: https://docs.mockery.io/en/stable/reference/spies.html
- 4: Using Log::spy with channels laravel/framework#52925
- 5: https://stackoverflow.com/questions/79018107/using-laravels-logspy-with-channels
- 6: https://laracasts.com/discuss/channels/testing/testing-that-the-log-record-has-been-written
- 7: https://stackoverflow.com/questions/79590956/laravel-spies-not-calling-implementation
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "composer.json dev/phpstan/test scripts:"
node -e "const c=require('./composer.json'); console.log(JSON.stringify({scripts: c.scripts||{}, requireDev: c['require-dev']||{}, require: c.require||{}}, null, 2))"
echo
echo "Locate LaravelFacadeTestTrait / facade spy definition:"
rg -n "trait LaravelFacadeTestTrait|function spy|Log::spy|shouldReceive\('debug'\)|shouldNotHaveReceived" vendor/laravel/framework tests provider files 2>/dev/null || true
echo
echo "Search all files for shouldNotHaveReceived definitions/usages:"
git ls-files | xargs rg -n "function\s+shouldNotHaveReceived|shouldNotHaveReceived\(" --glob '*.php' || trueRepository: OpenStackweb/openstackid
Length of output: 3664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Facade spy implementation:"
sed -n '55,85p' vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php | cat -n
echo
echo "Facade shouldReceive/shouldReceive definitions:"
rg -n "function\s+(shouldReceive|shouldNotHaveReceived)\(" vendor/laravel/framework/src/Illuminate/Support/Facades vendor/laravel/framework/src/Illuminate/Foundation/Testing --glob '*.php' || true
echo
echo "Inspect test class for Mockery facade trait usage:"
sed -n '1,35p' tests/FacebookDataDeletionBackfillCommandTest.php | cat -nRepository: OpenStackweb/openstackid
Length of output: 2868
Call shouldNotHaveReceived on the returned spy.
Log::spy() swaps the facade with a Mockery spy and returns it. The test calls Log::shouldNotHaveReceived(...), which tests this facade, while the static call that works in PHP’s dynamic facade resolution does not satisfy static analysis. Keep the value from Log::spy() and call the assertion on it.
Proposed fix
- Log::spy();
+ $log = Log::spy();
...
- Log::shouldNotHaveReceived('debug', function (string $message) use ($asid) {
+ $log->shouldNotHaveReceived('debug', function (string $message) use ($asid) {
return str_contains($message, $asid);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testDoesNotLogExternalId(): void | |
| { | |
| Log::spy(); | |
| $asid = '555000444'; | |
| $csv_path = $this->writeCsvFixture([$asid]); | |
| Artisan::call(self::Command, ['path' => $csv_path]); | |
| Log::shouldNotHaveReceived('debug', function (string $message) use ($asid) { | |
| return str_contains($message, $asid); | |
| }); | |
| public function testDoesNotLogExternalId(): void | |
| { | |
| $log = Log::spy(); | |
| $asid = '555000444'; | |
| $csv_path = $this->writeCsvFixture([$asid]); | |
| Artisan::call(self::Command, ['path' => $csv_path]); | |
| $log->shouldNotHaveReceived('debug', function (string $message) use ($asid) { | |
| return str_contains($message, $asid); | |
| }); |
🧰 Tools
🪛 PHPStan (2.2.7)
[error] 98-98: Call to an undefined static method Illuminate\Support\Facades\Log::shouldNotHaveReceived().
(staticMethod.notFound)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/FacebookDataDeletionBackfillCommandTest.php` around lines 90 - 100,
Update testDoesNotLogExternalId to store the Mockery spy returned by Log::spy(),
then invoke shouldNotHaveReceived on that spy instance instead of calling the
assertion statically through Log. Preserve the existing debug-message and
external-ID matching behavior.
Source: Linters/SAST tools
ref: https://app.clickup.com/t/86bbab70g
Summary
Facebook requires apps to implement a Data Deletion Request Callback (or an instructions URL). Neither existed in this codebase, which is why periodic "user data deletion" emails were arriving with no automated way to handle them.
POST /api/public/v1/facebook/data-deletionverifies Facebook's HMAC-SHA256signed_request, unlinks the matching user's Facebook identity (external_id/external_provider/external_pic— the account itself, email, password, groups, and all non-Facebook data survive untouched), and returns the required{url, confirmation_code}JSON.GET /api/public/v1/facebook/data-deletion/status/{confirmation_code}renders a human-readable status page, per Facebook's spec.idp:facebook-data-deletion-backfill {path}processes the CSV of already-queued app-scoped IDs downloaded from the app dashboard's Advanced Settings, applying the same unlink logic to the backlog that accumulated before this endpoint existed.Design notes
FacebookDataDeletionService::processDeletionRequest()is idempotent: a(provider, external_id)unique constraint plus a re-select-on-collision fallback (catchingUniqueConstraintViolationException) ensure a duplicate submission — a Facebook retry, or an ID already handled by the live callback later appearing in a CSV backfill — never re-processes the user or creates a duplicate audit row.facebook_deletion_requestsreads/writes route through the same Doctrine DBAL connection the user-entity flush uses (resolved fresh per call viaRegistry::getManager(), matchingDoctrineRepository::getEntityManager()'s existing pattern, so it survivesDoctrineTransactionService's reconnect/retry logic). This keeps the audit-row write and the user unlink atomic — they commit or roll back together.status=not_found, giving an auditable trail even for IDs Facebook's own FAQ says can be disregarded.Testing
changes-review) — 1 must_fix (cross-connection atomicity) and 1 should_fix (untested race-condition path) found and fixed; final round: 0 issues, compliance/quality "high".idp-app+idp-db-local): POST callback → 200 with correct JSON; tampered signature → 400; status page → 200 with expected text; unknown code → 404; backfill command executed against a real CSV fixture with correct matched/not-found counts.Out of scope / follow-up
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests