Phase 2: fix Civil ID upload and removal flow#82
Open
its-amann wants to merge 3 commits into
Open
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds S3 URL/key resolution and headObject checks; Candidate model stages and verifies civil-ID file copies, defers deletions, centralizes S3 error logging, and exposes deletePendingCivilIdFiles; controller tightens civil-id/expiry validation, propagates model errors, and marks verification state on photo removal; unit and functional tests added. ChangesCivil ID and Photo Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
Related Knowledge 1 document with suggested updates is ready for review. BAWES Universe Civil ID Upload FlowView Suggested Changes@@ -56,9 +56,10 @@
BE->>BE: urldecode param, guard against undefined
BE->>BE: S3FileExistValidator — fileExists(key) in temp bucket
BE->>BE: Candidate::updateCivilId('front')
- BE->>BE: deleteFile() — remove old civil ID if present
BE->>PermS3: copyObject(CopySource: tempBucket/key, Key: photos/key, ACL: public-read)
PermS3-->>BE: copy result
+ BE->>BE: fileExists(photos/key) — verify copy succeeded
+ BE->>BE: deleteFile($bestEffort: true) — remove old civil ID if verified
BE->>BE: Reset candidate_civil_expiry_date + candidate_civil_id = null
BE->>BE: model.save()
BE-->>App: { operation: "success", candidate_civil_photo_front, civilExpired, ... }
@@ -369,7 +370,12 @@
return ['operation' => 'error', 'message' => '...'];
}
- $model->updateCivilId('front');
+ if (!$model->updateCivilId('front')) {
+ return [
+ 'operation' => 'error',
+ 'message' => $model->getErrors()
+ ];
+ }
// Reset old OCR-extracted data — forces re-extraction on next check
$model->candidate_civil_expiry_date = null;
@@ -411,25 +417,51 @@
? 'candidate_civil_photo_front'
: 'candidate_civil_photo_back';
- // Delete previous file from permanent bucket if one exists
- if (!empty($this->oldAttributes[$idSide])) {
- $this->deleteFile('civil-id', $side);
- }
-
- $fileName = $this->$idSide;
+ $fileName = $this->$idSide;
+
+ if (!$fileName) {
+ $this->addError($idSide, Yii::t('app', 'file not available to save.'));
+ return false;
+ }
+
$sourceBucket = Yii::$app->temporaryBucketResourceManager->bucket;
$targetPath = "photos/" . $fileName;
+ $oldFilePath = $this->resolveStoredFileKey('civil-id', $side, true);
try {
- return Yii::$app->resourceManager->copy($fileName, $targetPath, $sourceBucket);
+ Yii::$app->resourceManager->copy($fileName, $targetPath, $sourceBucket);
+
+ if (!Yii::$app->resourceManager->fileExists($targetPath)) {
+ throw new \RuntimeException('Copied file could not be verified in the permanent bucket.');
+ }
+
+ if ($oldFilePath && $oldFilePath !== $targetPath) {
+ $this->deleteFile('civil-id', $side, true);
+ }
+
+ return true;
} catch (\Aws\S3\Exception\S3Exception $e) {
- Yii::error($e->getMessage(), 'candidate');
+ Yii::error([
+ 'message' => 'Failed to copy candidate file',
+ 'operation' => 'copy',
+ 'route' => Yii::$app->requestedRoute,
+ 'candidate_id' => $this->candidate_id,
+ 'file_key' => $targetPath,
+ 'error' => $e->getMessage(),
+ ], 'candidate');
$this->addError($idSide, Yii::t('app', 'file not available to save.'));
return false;
} catch (\Exception $e) {
- Yii::error($e->getMessage(), 'candidate');
+ Yii::error([
+ 'message' => 'Failed to copy candidate file',
+ 'operation' => 'copy',
+ 'route' => Yii::$app->requestedRoute,
+ 'candidate_id' => $this->candidate_id,
+ 'file_key' => $targetPath,
+ 'error' => $e->getMessage(),
+ ], 'candidate');
$this->addError($idSide, Yii::t('app', 'file not available to save.'));
return false;
}
@@ -438,9 +470,10 @@
Key behaviours:
-- **Old file cleanup:** If the candidate already has a civil photo stored (`oldAttributes[$idSide]` is set), the old file is deleted from the permanent bucket before the new one is copied. This prevents orphaned files accumulating.
-- **Target path:** Files are stored as `photos/{filename}` in the permanent bucket.
-- **Error handling:** Both `S3Exception` and generic `\Exception` are caught. Errors are logged to the `candidate` category (routed to Slack/Sentry) and surfaced as a model validation error.
+- **Copy first, verify, then delete:** The method copies the new file to the permanent bucket, verifies it exists using `fileExists()`, and only then deletes the old file (with `$bestEffort = true`). This prevents data loss if the copy fails.
+- **Target path:** Files are stored as `photos/{filename}` in the permanent bucket (not `candidate-civil-id/`).
+- **Error handling:** Both `S3Exception` and generic `\Exception` are caught. Errors are logged to the `candidate` category with structured context (route, candidate ID, file key) and surfaced as a model validation error.
+- **Best-effort deletion:** The old file is deleted with `$bestEffort = true`, which tolerates missing S3 objects instead of failing the upload.
***
@@ -479,6 +512,91 @@
***
+### Candidate Model — deleteFile()
+
+**File:** `common/models/Candidate.php`
+
+The `deleteFile()` method removes a file from the permanent S3 bucket. It supports a `$bestEffort` parameter (third parameter, defaults to `false`):
+
+```php
+public function deleteFile($type = 'resume', $side = 'front', $bestEffort = false) {
+ $attribute = $this->resolveStoredFileAttribute($type, $side);
+ $fileKey = $this->resolveStoredFileKey($type, $side, true);
+
+ if (!$fileKey || !isset($this->oldPrimaryKey)) {
+ return true;
+ }
+
+ try {
+ Yii::$app->resourceManager->delete($fileKey);
+ return true;
+
+ } catch (\Aws\S3\Exception\S3Exception $e) {
+ Yii::error([
+ 'message' => 'Failed to delete candidate file',
+ 'operation' => 'delete',
+ 'route' => Yii::$app->requestedRoute,
+ 'candidate_id' => $this->candidate_id,
+ 'file_key' => $fileKey,
+ 'error' => $e->getMessage(),
+ ], 'candidate');
+
+ if ($bestEffort) {
+ return true;
+ }
+
+ $this->addError($attribute, Yii::t('app', 'file not available to delete.'));
+ return false;
+
+ } catch (\Exception $e) {
+ // Same handling
+ }
+}
+```
+
+Key behaviours:
+
+- **Best-effort mode:** When `$bestEffort = true`, the method returns `true` even if the S3 object doesn't exist or deletion fails. This is used during Civil ID removal to tolerate missing files instead of returning a 500 error.
+- **Correct S3 path prefix:** For `$type = 'civil-id'`, files are resolved to `photos/{filename}` (not `candidate-civil-id/`).
+- **Field-specific errors:** Errors are attached to the correct attribute (`candidate_civil_photo_front` or `candidate_civil_photo_back` for civil-id type, not `candidate_resume`).
+- **Structured logging:** Failures are logged with context including route, candidate ID, and file key.
+
+***
+
+### S3ResourceManager — fileExists() and headObject()
+
+**File:** `common/components/S3ResourceManager.php`
+
+The `fileExists()` method checks whether a file exists in S3 using the AWS SDK's `headObject()` method:
+
+```php
+public function fileExists($filenameOrUrl)
+{
+ try {
+ $this->headObject($filenameOrUrl);
+ return true;
+ } catch (AwsException $e) {
+ return false;
+ } catch (\Exception $e) {
+ return false;
+ }
+}
+
+public function headObject($filenameOrUrl)
+{
+ [$bucket, $key] = $this->resolveObjectLocation($filenameOrUrl);
+
+ return $this->getClient()->headObject([
+ 'Bucket' => $bucket,
+ 'Key' => $key,
+ ]);
+}
+```
+
+The method now uses the S3 client's `headObject()` call instead of GuzzleHttp with HTTP HEAD requests. This means it works for all S3 objects, not just public resources.
+
+***
+
### S3FileExistValidator
**File:** `common/components/S3FileExistValidator.php`
@@ -504,7 +622,7 @@
}
```
-The validator uses an HTTP `HEAD` request against the public S3 URL to check existence, and optionally validates file extension and size. [[32]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3FileExistValidator.php#L46-L88)
+The validator calls `S3ResourceManager::fileExists()`, which now uses SDK-based `headObject()` checks instead of HTTP HEAD requests. [[32]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3FileExistValidator.php#L46-L88)
***
@@ -545,8 +663,11 @@
| Method | Endpoint | Auth | Description |
|---|---|---|---|
| `GET` | `/aws/config` | None (CORS only) | Returns temp S3 credentials for direct upload |
-| `POST` | `/v1/account/update-civil-photo-front` | Candidate JWT | Validates S3 key, copies front photo to permanent bucket |
-| `POST` | `/v1/account/update-civil-photo-back` | Candidate JWT | Validates S3 key, copies back photo to permanent bucket |
+| `POST` | `/v1/account/update-civil-photo-front` | Candidate JWT | Validates S3 key, copies front photo to permanent bucket, returns error on copy failure |
+| `POST` | `/v1/account/update-civil-photo-back` | Candidate JWT | Validates S3 key, copies back photo to permanent bucket, returns error on copy failure |
+| `POST` | `/v1/account/remove-civil-photo-front` | Candidate JWT | Removes front Civil ID photo, tolerates missing S3 objects, sets `candidate_civil_need_verification = true` |
+| `POST` | `/v1/account/remove-civil-photo-back` | Candidate JWT | Removes back Civil ID photo, tolerates missing S3 objects, sets `candidate_civil_need_verification = true` |
+| `POST` | `/v1/account/update-civil-id-expiry-date` | Candidate JWT | Validates and saves Civil ID number (12 digits) and expiry date, returns structured errors on invalid input |
**Request bodies:**
@@ -666,11 +787,11 @@
#### S3FileExistValidator Guards the Copy Operation
-Before the backend copies a file from the temp bucket, it validates that the key actually exists in S3 (`HEAD` request via GuzzleHttp). This prevents an attacker from specifying an arbitrary S3 key and tricking the backend into copying a file they did not upload. [[54]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3FileExistValidator.php#L46-L58)
-
-#### Old Files Deleted Before Copy
-
-`updateCivilId()` deletes the candidate's previously stored civil photo before copying the new one, preventing unbounded growth of files in the permanent bucket. [[55]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/models/Candidate.php#L2743-L2745)
+Before the backend copies a file from the temp bucket, it validates that the key actually exists in S3 using `S3ResourceManager::fileExists()`, which calls the AWS SDK's `headObject()` method. This prevents an attacker from specifying an arbitrary S3 key and tricking the backend into copying a file they did not upload. [[54]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3FileExistValidator.php#L46-L58)
+
+#### Old Files Deleted After Copy and Verification
+
+`updateCivilId()` copies the new file first, verifies the copy succeeded using `fileExists()`, and only then deletes the candidate's previously stored civil photo (with `$bestEffort = true`). This prevents data loss if the copy or verification fails, and tolerates missing old files instead of failing the upload. [[55]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/models/Candidate.php#L2743-L2745)
***
@@ -705,7 +826,3 @@
Uploaded files are not scanned for malware before being promoted to the permanent bucket. Civil ID uploads are expected to be images, but the `S3FileExistValidator` only checks extension and size — it does not validate MIME content or scan for malicious payloads. [[59]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3FileExistValidator.php#L61-L88)
**Recommendation:** Consider integrating an AWS Lambda function triggered on temp-bucket `PutObject` events to run ClamAV or a commercial AV solution before the backend is permitted to copy the file.
-
-#### 🟢 Low Priority: `fileExists()` Uses HTTP HEAD (Public Access Required)
-
-The `S3FileExistValidator` checks file existence using a GuzzleHttp `HEAD` request to the public URL of the file [[60]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3ResourceManager.php#L174-L188). This means the file must be publicly accessible in the temp bucket for validation to work. If the ACL recommendation above is ever applied to the temp bucket, this validation approach would need to be replaced with an SDK `headObject` call using credentials.How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
candidate/tests/functional/AccountCest.php (1)
187-213: ⚡ Quick winAdd a regression for the non-empty invalid Civil ID branch.
These tests cover “missing ID” and “bad date”, but the new
preg_match('/^\d{12}$/', ...)rejection is still unasserted. A case likecivil_id => '70'should verify the"Civil ID must be 12 digit number"response too.🤖 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 `@candidate/tests/functional/AccountCest.php` around lines 187 - 213, Add a new functional test to cover the non-empty invalid civil ID branch: create a method (e.g., tryUpdateCivilIdExpiryDateRejectsNonEmptyInvalidCivilId) alongside tryUpdateCivilIdExpiryDateRequiresCivilId and tryUpdateCivilIdExpiryDateRejectsInvalidDate that sends a POST to 'v1/account/update-civil-id-expiry-date' with 'civil_id' => '70' and a valid 'civil_expiry_date' (e.g., date('+1 month')), then assert HttpCode::OK and that the JSON response contains 'operation' => 'error' and 'message' => 'Civil ID must be 12 digit number' to exercise the preg_match('/^\d{12}$/', ...) validation path.
🤖 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 `@candidate/modules/v1/controllers/AccountController.php`:
- Around line 1492-1499: The log call in the catch block (Yii::error) currently
includes the raw government civil ID (variable candidate_civil_id) which is
sensitive; remove that field (and any other sensitive PII like
candidate->candidate_civil_expiry_date if you deem it sensitive) from the array
passed to Yii::error and only include non-PII for debugging (e.g.,
Yii::$app->requestedRoute, candidate->candidate_id, and $e->getMessage());
alternatively, if you must keep a civil-id indicator, replace candidate_civil_id
with a masked version (e.g., last 2 chars only) before passing it to Yii::error.
In `@common/components/S3ResourceManager.php`:
- Around line 179-188: The URL parsing only extracts virtual-hosted bucket names
and leaves path-style URLs broken and percent-encoded; update the logic in
S3ResourceManager (the code handling $filenameOrUrl and returning [$bucket,
$key]) to detect path-style hosts like "s3[.-]..." where the bucket is the first
segment of the parsed path, set $bucket = array_shift($pathSegments) and $key =
implode('/', $pathSegments), and ensure you rawurldecode() the path (or each
path segment) so percent-encoded characters are decoded before returning the key
for headObject() / fileExists().
In `@common/models/Candidate.php`:
- Around line 2818-2820: The deletion of the old Civil ID file is happening
before the new filename is persisted (see variables $oldFilePath, $targetPath
and the call to $this->deleteFile('civil-id', $side, true)), which can lead to
data loss if the subsequent $model->save() fails; change the flow so the old
file is removed only after the DB update successfully commits — either move the
deleteFile call to run after the controller successfully calls $model->save()
(or inside the model's afterSave()/saved() hook) or perform the create/update
inside a DB transaction and register the file deletion in a commit callback so
deletion occurs only on successful commit.
---
Nitpick comments:
In `@candidate/tests/functional/AccountCest.php`:
- Around line 187-213: Add a new functional test to cover the non-empty invalid
civil ID branch: create a method (e.g.,
tryUpdateCivilIdExpiryDateRejectsNonEmptyInvalidCivilId) alongside
tryUpdateCivilIdExpiryDateRequiresCivilId and
tryUpdateCivilIdExpiryDateRejectsInvalidDate that sends a POST to
'v1/account/update-civil-id-expiry-date' with 'civil_id' => '70' and a valid
'civil_expiry_date' (e.g., date('+1 month')), then assert HttpCode::OK and that
the JSON response contains 'operation' => 'error' and 'message' => 'Civil ID
must be 12 digit number' to exercise the preg_match('/^\d{12}$/', ...)
validation path.
🪄 Autofix (Beta)
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
Run ID: 177b727d-c767-44e5-a530-7fcb5f305df8
📒 Files selected for processing (5)
candidate/modules/v1/controllers/AccountController.phpcandidate/tests/functional/AccountCest.phpcommon/components/S3ResourceManager.phpcommon/models/Candidate.phpcommon/tests/unit/models/CandidateS3BehaviorTest.php
68 tasks
Author
|
/claim #55 Claiming the Phase 2 backend Civil ID remediation slice covered by this PR. Scope is candidate backend handling for missing-object-safe Civil ID removal, Civil ID expiry validation, corrected Follow-up pushed in
Verification run locally:
The full Companion staff-side Phase 3 UI handling is in BAWES-Universe/studenthub-staff#57. I kept it separate because it touches the staff repo. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@common/models/Candidate.php`:
- Around line 2719-2730: The resolveStoredFileKey method assumes $fileName is
always a bare filename and blindly prepends $directory, causing duplicates like
"photos/photos/..."; update resolveStoredFileKey to normalize existing keys by
checking the resolved $fileName (from $this->oldAttributes or $this->$attribute)
and if it already begins with the intended $directory (or already contains a
path separator indicating a bucket-relative key) return it as-is instead of
prepending, otherwise prepend $directory; keep the existing useOldAttributes
behavior and use resolveStoredFileAttribute($type, $side) to locate the
attribute.
- Around line 2809-2821: The catch blocks in Candidate.php that handle S3
deletion currently unset $this->pendingCivilIdDeletionKeys[$pendingSide] and
allow the method to still return $deleted=true; change the error path so that on
any exception thrown by Yii::$app->resourceManager->delete(...) you do NOT unset
$this->pendingCivilIdDeletionKeys[$pendingSide], keep the entry staged for
retry, call $this->logS3FileFailure('delete', $entry['attribute'],
$entry['file_key'], $e) as already done, and ensure the method returns false
(e.g. set $deleted = false or return false immediately) so callers know the
deletion failed and can retry/alert; preserve the existing successful-path
behavior in the try block where the key is unset.
🪄 Autofix (Beta)
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
Run ID: 1c95aae3-c26a-4082-993c-886d5534f1fa
📒 Files selected for processing (5)
candidate/modules/v1/controllers/AccountController.phpcandidate/tests/functional/AccountCest.phpcommon/components/S3ResourceManager.phpcommon/models/Candidate.phpcommon/tests/unit/models/CandidateS3BehaviorTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- candidate/tests/functional/AccountCest.php
- candidate/modules/v1/controllers/AccountController.php
- common/components/S3ResourceManager.php
Author
|
Follow-up pushed in
Verification run locally:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR covers the Phase 2 backend remediation from #55.
What changed
civil_idandcivil_expiry_dateinupdate-civil-id-expiry-dateand returnoperation:erroron bad input or save failuresCandidate::deleteFile("civil-id")to use the correctphotos/prefix and attach errors to the Civil ID field instead ofcandidate_resumeCandidate::updateCivilId()to copy the new file first, verify the destination, and delete the old file only after the new object is confirmedroute/candidate_id/filenamecontext to Civil ID copy and delete failure logsS3ResourceManager::headObject()and movefileExists()to S3headObjectchecks instead of HTTP HEADVerification
php -l common/components/S3ResourceManager.phpphp -l common/models/Candidate.phpphp -l candidate/modules/v1/controllers/AccountController.phpphp -d extension=gd -d extension=zip vendor\\bin\\codecept run common\\tests\\unit\\models\\CandidateS3BehaviorTest.php --stepsOK (2 tests, 10 assertions)Notes
php -d extension=gd -d extension=zip vendor\\bin\\codecept run candidate\\tests\\functional\\AccountCest.php --stepslocally. That suite is currently blocked here by a missing MySQL test database and fails during bootstrap withSQLSTATE[HY000] [2002]againstpayroll_test, so I am not claiming that functional suite as locally passing.Summary by CodeRabbit
New Features
Bug Fixes
Tests
/claim #55