Repository navigation
fix(meetings): match restricted-meeting registrants by the user's verified emails (PCC-1570) - #3341
zhenghchen wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
PR SummaryMedium Risk Overview After username and direct email lookups miss, Email registrant queries now OR-match both Coverage adds unit tests, a 13-scenario HTTP integration spec for join-url, and service tests for verified-email resolution and lookup failure/impersonation behavior. Reviewed by Cursor Bugbot for commit d9eb69c. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@zhenghchen I'm starting review 1 of this pull request now (started 2026-10-07 06:52 UTC). This usually takes a few minutes — I'll update this comment with the summary when I'm done. Hi @zhenghchen, thanks for the PR! This fixes the PCC-1570 restricted-meeting join failure by retrying the registrant lookup against the user's other verified emails. It also fixes the case-sensitivity gap by matching both No issues found in any dimension (correctness, security, privacy, performance, tests, style, docs). ✅ Approved ➡️ This auto-review agent is a preview. Please send feedback to @dealako. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The identity handling, failure behavior, upstream query contract, and relevant edge cases are consistently implemented and thoroughly tested.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes restricted-meeting joins by matching registrants against the signed-in user’s verified email addresses.
Changes:
- Adds prioritized verified-email resolution and case-insensitive registrant lookup.
- Returns a retryable 503 when email verification is unavailable.
- Adds unit and route-level integration coverage.
| File | Description |
|---|---|
packages/shared/src/interfaces/user-profile.interface.ts |
Defines verified-email resolution results. |
packages/shared/src/interfaces/meeting.interface.ts |
Defines verified-email registrant lookup results. |
apps/lfx-one/src/server/services/user-verified-emails.service.ts |
Resolves prioritized verified addresses. |
apps/lfx-one/src/server/services/user-verified-emails.service.spec.ts |
Tests email resolution and failure handling. |
apps/lfx-one/src/server/services/meeting.service.ts |
Adds case-insensitive, multi-email registrant matching. |
apps/lfx-one/src/server/services/meeting.service.spec.ts |
Tests query construction, ranking, and failures. |
apps/lfx-one/src/server/controllers/public-meeting.controller.ts |
Adds verified-email fallback to restricted joins. |
apps/lfx-one/src/server/controllers/public-meeting.controller.spec.ts |
Tests controller fallback behavior. |
apps/lfx-one/src/server/controllers/public-meeting-join.integration.spec.ts |
Exercises the complete restricted-join flow. |
apps/lfx-one/src/server/controllers/badges.controller.ts |
Reuses shared verified-email collection logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-3341.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
MRashad26
left a comment
MRashad26
left a comment
There was a problem hiding this comment.
Ran a full review plus the code-standards-enforcer agent (~/LFX/code-enforcer-agent.md), scoped to this PR's diff. 2 findings, neither blocking.
UserVerifiedEmailsService.getUserVerifiedEmails has zero logging, including on the incomplete: true path (user-verified-emails.service.ts:1126). When auth-service's user_emails.read NATS call fails/times out, EmailVerificationService.getUserEmails returns null and this service silently returns { incomplete: true, ... } with no trace at all — this violates .claude/rules/logging-patterns.md's rule to use logger.warning() for recoverable errors when returning null/empty. A production incident where verified-email join matching silently misses registrants would be invisible in logs today, since this file never calls logger.debug/info/warning. Suggested fix: add a warning log on the incomplete-lookup path.
restrictedMeetingCheck now chains up to 3 sequential query-service/NATS round-trips on the latency-sensitive public join-url path (public-meeting.controller.ts:1011). Any caller whose registrant record isn't matched by username or login email — the exact case this PR targets — now pays for 2 sequential query-service calls plus 2 more NATS round-trips before getting a response on POST /public/api/meetings/:id/join-url. The codebase already folds email+username into one filters_or query (getMeetingRegistrantsForUser); the verified-email clauses could in principle be merged into a single combined query once the verified emails are known, instead of adding a third strictly-sequential leg.
Everything else checked out clean: no correctness bugs found across 8 finder angles. Upstream assumptions verified against the real repos — case_insensitive_email is a genuinely indexed GSI field on the registrant model in lfx-v2-meeting-service, and filters/filters_or semantics in lfx-v2-query-service's OpenSearch query template match what this PR assumes. Impersonation handling, email-priority ranking, and lookupFailed/incomplete propagation are all covered by accompanying unit + integration tests (timeout, auth-service down, impersonation, mixed casing, anonymous caller). code-standards-enforcer: PASS — license headers, shared-interface placement, member ordering, kebab-case naming, 160-char lines, no console.log, fixture-email conventions all correctly followed.
Posting as Comment — both findings worth a look, neither a hard blocker.
|
@zhenghchen I'm starting review 2 of this pull request now (started 2026-10-08 17:30 UTC). This usually takes a few minutes — I'll update this comment with the summary when I'm done. Hi @zhenghchen, thanks for the quick follow-up and for acting on the earlier feedback! The new commit is small and focused: it adds a warning log when the auth-service email lookup is unavailable, with a matching assertion in the spec. The logger call matches the 👏 Nice work: the log carries just
No new issues found in any dimension. ✅ Approved ➡️ This auto-review agent is a preview. Please send feedback to @dealako. |
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
Follow-up review: the new commit adds the requested warning log on the incomplete verified-email path. No blocking, security, or privacy issues. Approved.
Generated by Claude Code
…s (PCC-1570) A signed-in user without an LF username could not join a restricted meeting when their registrant row was stored under their meeting-invite email. The join check now retries once against the user's verified emails, and the email lookups match case-insensitively. Signed-off-by: zhenghchen <zhenghchen2024@gmail.com>
Signed-off-by: zhenghchen <zhenghchen2024@gmail.com>
d8cbf21 to
d9eb69c
Compare
|
@zhenghchen I'm starting review 3 of this pull request now (started 2026-10-12 19:20 UTC). This usually takes a few minutes — I'll update this comment with the summary when I'm done. Hi @zhenghchen, thanks for keeping this branch current! This push rebased the PR onto the latest
No new issues found in any dimension. ✅ Approved ➡️ This auto-review agent is a preview. Please send feedback to @dealako. |
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
Follow-up review: the branch was rebased onto current main; the content is unchanged from the previously approved revision (verified-email fallback plus the incomplete-lookup warning log). No blocking, security, or privacy issues. Approved.
Generated by Claude Code
Summary
Fixes the join failure in PCC-1570. A signed-in user without an LF username couldn't join a restricted meeting from their calendar invite. They got "You are not registered for this restricted meeting" even though they were registered, because their registrant row is stored under their meeting-invite email, not their login email.
restrictedMeetingCheck(POST /public/api/meetings/:id/join-url) looks up registrants by only two things:itx-service-zoom writes committee registrants with the member's Meeting email preference and rewrites them when that preference changes, so the stored address can be any of the user's verified emails. An account with no username whose rows hold another address matches neither lookup.
The email lookups also had a casing problem. They matched
emailwith a lowercased value, but query-servicetermmatching is case-sensitive andemailkeeps the stored casing, so a mixed-case stored address never matched.Fix
restrictedMeetingCheckretries once against the user's other verified emails, usingMeetingService.getMeetingRegistrantsByVerifiedEmails.filters_orquery.UserVerifiedEmailsServiceresolves those emails:SERVICE_ADVISORY("Unable to verify your meeting registration right now…") rather than telling a registered user they aren't registered.getMeetingRegistrantsByEmailandgetMeetingRegistrantsForUsernow matchemailorcase_insensitive_email. Both fields are indexed onv1_meeting_registrant.badges.controllerreuses the sharedcollectAuthServiceVerifiedEmailshelper. Its behaviour is unchanged.matched_source(meeting_invite_preference|auth_service).Users with a username, or whose login email matches, see no change and trigger no extra calls. Anonymous callers are unchanged.
Ticket
PCC-1570
Testing
public-meeting-join.integration.spec.ts), and the full suite, lint and type-check pass.Tested on the dev preview (ui-pr-3341, head d8cbf21).
Setup: restricted meeting, guest added by email alias (+pcc1570b) before that alias was verified on the account, so the registrant row has no username link. Same account, meeting, row and password on both hosts.