feat(federation): add verifyMatrixIds method and API #37080
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds an authenticated REST endpoint Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as REST API (/federation/matrixIds.verify)
participant FS as IFederationMatrixService (verifyMatrixIds)
participant U as Local User Store
participant HS as Remote Homeserver
Note over API: Auth + AJV validation
C->>API: GET /federation/matrixIds.verify?matrixId=...
API->>FS: verifyMatrixIds([ids])
alt local home server
FS->>U: lookup username
U-->>FS: found / not found
else remote home server
loop per remote id
FS->>HS: GET /_matrix/federation/v1/query/profile?user_id=...
HS-->>FS: profile / { errcode: M_NOT_FOUND } / error
end
end
FS-->>API: { matrixId: status, ... }
API-->>C: 200 { matrixId -> status }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 @@
## develop #37080 +/- ##
===========================================
- Coverage 67.40% 67.39% -0.01%
===========================================
Files 3328 3328
Lines 113338 113342 +4
Branches 20561 20557 -4
===========================================
- Hits 76391 76385 -6
- Misses 34346 34356 +10
Partials 2601 2601
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…rix ID verification
ggazzo
force-pushed
the
chore/matrixIds.verify
branch
from
5b0700a to
d983a85
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core-services/src/types/IFederationMatrixService.ts (1)
30-30: Prefer a strongly typed verification result overRecord<string, string>Returning a plain string per matrixId leaves the contract open-ended—callers have no compile-time clue about the possible statuses or payload shape we intend to surface. Please introduce a dedicated type (e.g., a union of the allowed status literals or a structured
{ status: ..., reason?: ... }object) and usePromise<Record<string, MatrixIdVerificationResult>>(or similar) here. That keeps the interface self-documenting and prevents the REST layer and downstream clients from silently diverging later.
📜 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 (3)
apps/meteor/ee/server/api/federation.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/server/api/federation.ts
🧬 Code graph analysis (1)
apps/meteor/ee/server/api/federation.ts (2)
packages/core-services/src/types/IFederationMatrixService.ts (1)
IFederationMatrixService(4-31)packages/rest-typings/src/v1/Ajv.ts (1)
ajv(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🚢 Build Docker Images for Testing (alpine)
| query: ajv.compile<{ | ||
| matrixIds: string[]; | ||
| }>({ | ||
| type: 'object', | ||
| properties: { | ||
| matrixIds: { type: 'array', items: { type: 'string' } }, | ||
| }, | ||
| }), | ||
| response: { | ||
| 200: ajv.compile<{ | ||
| results: { [key: string]: string }; | ||
| }>({ | ||
| type: 'object', | ||
| properties: { | ||
| results: { type: 'object', additionalProperties: { type: 'string' } }, | ||
| }, | ||
| }), | ||
| }, | ||
| }, | ||
| async function () { | ||
| const { matrixIds } = this.queryParams; | ||
| if (!federationService) { | ||
| throw new Error('Federation service not registered'); | ||
| } | ||
| return API.v1.success({ | ||
| results: await federationService.verifyMatrixIds(matrixIds), | ||
| }); |
There was a problem hiding this comment.
Make matrixIds mandatory in the query schema
Right now the AJV schema doesn’t mark matrixIds as required, so a request without that parameter is accepted and matrixIds is undefined. The handler then calls federationService.verifyMatrixIds(matrixIds), which blows up at runtime because it tries to iterate over undefined. Please make the field required (and consider rejecting unexpected extras) so we respond with a 400 instead of a 500:
query: ajv.compile<{
matrixIds: string[];
}>({
type: 'object',
properties: {
matrixIds: { type: 'array', items: { type: 'string' } },
},
+ required: ['matrixIds'],
+ additionalProperties: 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.
| query: ajv.compile<{ | |
| matrixIds: string[]; | |
| }>({ | |
| type: 'object', | |
| properties: { | |
| matrixIds: { type: 'array', items: { type: 'string' } }, | |
| }, | |
| }), | |
| response: { | |
| 200: ajv.compile<{ | |
| results: { [key: string]: string }; | |
| }>({ | |
| type: 'object', | |
| properties: { | |
| results: { type: 'object', additionalProperties: { type: 'string' } }, | |
| }, | |
| }), | |
| }, | |
| }, | |
| async function () { | |
| const { matrixIds } = this.queryParams; | |
| if (!federationService) { | |
| throw new Error('Federation service not registered'); | |
| } | |
| return API.v1.success({ | |
| results: await federationService.verifyMatrixIds(matrixIds), | |
| }); | |
| query: ajv.compile<{ | |
| matrixIds: string[]; | |
| }>({ | |
| type: 'object', | |
| properties: { | |
| matrixIds: { type: 'array', items: { type: 'string' } }, | |
| }, | |
| required: ['matrixIds'], | |
| additionalProperties: false, | |
| }), |
🤖 Prompt for AI Agents
In apps/meteor/ee/server/api/federation.ts around lines 17 to 43, the AJV query
schema does not mark matrixIds as required so matrixIds can be undefined at
runtime; update the query schema to include required: ['matrixIds'], tighten the
matrixIds property to be an array of strings (optionally with minItems: 1 if
empty should be rejected), and set additionalProperties: false on the query
object to reject unexpected extras; this ensures AJV returns a 400 for
missing/extra params and prevents passing undefined into
federationService.verifyMatrixIds.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
ee/packages/federation-matrix/src/FederationMatrix.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
| if ('errcode' in result && result.errcode === 'M_NOT_FOUND') { | ||
| return [matrixId, 'UNVERIFIED']; | ||
| } | ||
|
|
||
| return [matrixId, 'VERIFIED']; | ||
| } catch (e) { |
There was a problem hiding this comment.
Do not mark errored Matrix profile queries as VERIFIED
Any response that still contains an errcode (e.g., M_FORBIDDEN, M_UNAUTHORIZED, M_LIMIT_EXCEEDED) currently falls through this branch and is treated as VERIFIED. That gives a false positive: a homeserver telling us it could not supply the profile is strong evidence that we failed to verify the ID. Please treat every non-M_NOT_FOUND errcode as UNABLE_TO_VERIFY instead of VERIFIED.
- if ('errcode' in result && result.errcode === 'M_NOT_FOUND') {
- return [matrixId, 'UNVERIFIED'];
- }
-
- return [matrixId, 'VERIFIED'];
+ if ('errcode' in result) {
+ if (result.errcode === 'M_NOT_FOUND') {
+ return [matrixId, 'UNVERIFIED'];
+ }
+
+ return [matrixId, 'UNABLE_TO_VERIFY'];
+ }Committable suggestion skipped: line range outside the PR's diff.
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
https://rocketchat.atlassian.net/browse/FDR-172
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements