fix: upload not working #225
Conversation
WalkthroughRemoves MatrixBridgedRoom repository and its DI wiring, shifts media authorization to read Matrix room ID directly from uploads, updates Upload repository API accordingly, and adds "federation-bundle" to Biome ignore list. No new public exports are added; one repository file is deleted and related imports/registrations are removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant AuthService as EventAuthorizationService
participant UploadRepo as UploadRepository
participant Matrix as Matrix Homeserver
Client->>AuthService: canAccessMedia(mediaId, userId)
AuthService->>UploadRepo: findByMediaId(mediaId)
UploadRepo-->>AuthService: Upload | null
alt Upload found
note over AuthService: Extract mrid from upload.federation.mrid
AuthService->>Matrix: Verify user access to mrid
Matrix-->>AuthService: allowed | denied
AuthService-->>Client: result
else No upload
AuthService-->>Client: denied (not found)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
=======================================
Coverage 81.07% 81.07%
=======================================
Files 63 63
Lines 4719 4719
=======================================
Hits 3826 3826
Misses 893 893 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/federation-sdk/src/repositories/upload.repository.ts (2)
8-11: New federation.mrid requires data backfill/validation.Runtime will throw if older Upload docs lack federation.mrid. Confirm a migration/backfill exists, or ensure callers defensively handle missing mrid (see service comment).
Suggestion: add a small migration/index:
- Backfill federation.mrid for existing uploads with federation.mediaId.
- Create an index on "federation.mediaId" (unique if appropriate) to keep lookups fast.
20-22: Align return type with MongoDB driver and add index on federation.mediaId.
- In packages/federation-sdk/src/repositories/upload.repository.ts change
async findByMediaId(mediaId: string): Promise<Upload | null>
to
async findByMediaId(mediaId: string): Promise<WithId<Upload> | null>(importWithIdfrommongodb).- Add an index on
federation.mediaId(e.g. in your DB initialization or repository constructor) to avoid full collection scans.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
biome.jsonc(1 hunks)packages/federation-sdk/src/container.ts(0 hunks)packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts(0 hunks)packages/federation-sdk/src/repositories/upload.repository.ts(2 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/federation-sdk/src/repositories/matrix-bridged-room.repository.ts
- packages/federation-sdk/src/container.ts
🔇 Additional comments (1)
biome.jsonc (1)
49-49: Ensure ignore glob matches actual path(s).If the bundle can appear outside the repo root, prefer a glob: "**/federation-bundle".
Apply this diff if needed:
- "federation-bundle", + "**/federation-bundle",
| const rcUpload = await this.uploadRepository.findByMediaId(mediaId); | ||
| if (!rcUpload) { | ||
| this.logger.debug(`Media ${mediaId} not found in any room`); | ||
| return false; | ||
| } | ||
|
|
||
| const matrixRoomId = | ||
| await this.matrixBridgedRoomRepository.findMatrixRoomId(rcRoomId); | ||
| if (!matrixRoomId) { | ||
| this.logger.debug(`Media ${mediaId} not found in any room`); | ||
| return false; | ||
| } | ||
| const matrixRoomId = rcUpload.federation.mrid; | ||
|
|
There was a problem hiding this comment.
Guard against missing federation.mrid to prevent runtime crash.
Older upload docs may lack federation or mrid. Accessing rcUpload.federation.mrid can throw.
Apply this diff:
- const rcUpload = await this.uploadRepository.findByMediaId(mediaId);
- if (!rcUpload) {
+ const rcUpload = await this.uploadRepository.findByMediaId(mediaId);
+ if (!rcUpload) {
this.logger.debug(`Media ${mediaId} not found in any room`);
return false;
}
-
- const matrixRoomId = rcUpload.federation.mrid;
+ const matrixRoomId = rcUpload.federation?.mrid;
+ if (!matrixRoomId) {
+ this.logger.debug(
+ `Media ${mediaId} found but missing federation.mrid; denying access`,
+ );
+ return false;
+ }📝 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.
| const rcUpload = await this.uploadRepository.findByMediaId(mediaId); | |
| if (!rcUpload) { | |
| this.logger.debug(`Media ${mediaId} not found in any room`); | |
| return false; | |
| } | |
| const matrixRoomId = | |
| await this.matrixBridgedRoomRepository.findMatrixRoomId(rcRoomId); | |
| if (!matrixRoomId) { | |
| this.logger.debug(`Media ${mediaId} not found in any room`); | |
| return false; | |
| } | |
| const matrixRoomId = rcUpload.federation.mrid; | |
| const rcUpload = await this.uploadRepository.findByMediaId(mediaId); | |
| if (!rcUpload) { | |
| this.logger.debug(`Media ${mediaId} not found in any room`); | |
| return false; | |
| } | |
| const matrixRoomId = rcUpload.federation?.mrid; | |
| if (!matrixRoomId) { | |
| this.logger.debug( | |
| `Media ${mediaId} found but missing federation.mrid; denying access`, | |
| ); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/event-authorization.service.ts around
lines 368 to 375, accessing rcUpload.federation.mrid may throw for older upload
docs missing federation or mrid; guard by checking rcUpload.federation and
rcUpload.federation.mrid before reading it, log a debug/warn with the mediaId if
missing, and return false (or otherwise bail out) instead of dereferencing;
ensure matrixRoomId is only assigned after the presence check and keep typesafe
access (optional: narrow types or use optional chaining with a clear fallback
and early return).
Linked to RocketChat/Rocket.Chat#37064
Summary by CodeRabbit