Skip to content

Conversation

@pierre-lehnen-rc
Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc commented Dec 2, 2025

Proposed changes (including videos or screenshots)

Issue(s)

CORE-1521

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Changes
    • VoIP extension fields are now always included in user results (no per-user permission gating).
    • Several VoIP-related API routes no longer enforce route-level permissions.
    • UI now shows VoIP extension based on feature flag rather than user permissions.
    • Removed related VoIP permission labels/translations and added a migration to clean up legacy permissions.

✏️ Tip: You can customize this high-level summary in your review settings.

ggazzo and others added 30 commits November 18, 2025 15:46
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Dec 2, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR has conflicts, please resolve them before merging
  • This PR is not mergeable

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Dec 2, 2025

⚠️ No Changeset found

Latest commit: 3e746ec

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pierre-lehnen-rc pierre-lehnen-rc added this to the 8.0.0 milestone Dec 2, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025

Walkthrough

This PR removes three VoIP-related permissions, always includes freeSwitchExtension in user projections, replaces permission-based UI visibility with a feature-flag hook, removes per-route permissionsRequired from several VoIP endpoints, and adds a migration to delete the old permission records.

Changes

Cohort / File(s) Summary
Permission Definition Removal
apps/meteor/app/authorization/server/constant/permissions.ts
Deleted three VoIP permission entries (manage-voip-extensions, view-user-voip-extension, view-voip-extension-details).
API & Projection Updates
apps/meteor/app/api/server/lib/users.ts, apps/meteor/app/api/server/v1/im.ts, apps/meteor/app/api/server/v1/users.ts
Removed runtime permission checks that gated inclusion of freeSwitchExtension; freeSwitchExtension is now always projected; adjusted canEditExtension call args.
Enterprise Routes
apps/meteor/ee/app/api-enterprise/server/voip-freeswitch.ts
Removed permissionsRequired entries from four voip-freeswitch routes (authentication remains).
Server Logic / Validation
apps/meteor/app/lib/server/functions/getFullUserData.ts, apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts
Removed permission checks that gated exposure/editing of freeSwitchExtension; simplified canEditExtension signature and flow.
Client Hooks & Components
apps/meteor/client/views/admin/users/useShowVoipExtension.tsx, apps/meteor/client/views/admin/users/useVoipExtensionPermission.tsx*, apps/meteor/client/views/admin/users/AdminUserForm.tsx, apps/meteor/client/views/admin/users/UsersTable/UsersTable.tsx
Added useShowVoipExtension (feature-flag based); removed useVoipExtensionPermission; components now use showVoipExtension to control VoIP extension visibility.
Database Migration
apps/meteor/server/startup/migrations/index.ts, apps/meteor/server/startup/migrations/v325.ts
Added migration v325 that deletes the three deprecated VoIP permission documents from Permissions.
Tests
apps/meteor/tests/end-to-end/api/users.ts
Removed test setup/teardown and assertions related to manage-voip-extensions; VoIP permission scenarios removed.
i18n Removals
packages/i18n/src/locales/en.i18n.json, packages/i18n/src/locales/nb.i18n.json, packages/i18n/src/locales/nn.i18n.json, packages/i18n/src/locales/pt-BR.i18n.json, packages/i18n/src/locales/sv.i18n.json
Removed localization keys and descriptions for the three VoIP permissions across locales.

* useVoipExtensionPermission.tsx file deleted and replaced by useShowVoipExtension.tsx.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review migration v325 for correct deletion filter and idempotence.
  • Confirm removed permissionsRequired does not widen access unintentionally (auth still required).
  • Verify all UI spots switched from permission to feature-flag checks and no permission-based checks remain where needed.
  • Check server-side projections and validation signature change for regressions.

Possibly related PRs

Suggested reviewers

  • gabriellsh
  • tassoevan

Poem

🐰 I hopped through code at break of day,

old VoIP rules I nudged away,
A feature flag now shows the line,
Extensions visible by design,
Hooray — a tidier burrowed play! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main objective of the PR: removing old VoIP permissions as specified in CORE-1521.
Linked Issues check ✅ Passed The PR successfully removes all three deprecated Voice Call permissions (manage-voip-extensions, view-user-voip-extension, view-voip-extension-details) from permissions definitions, API routes, UI, migrations, and i18n files as required by CORE-1521.
Out of Scope Changes check ✅ Passed All changes are directly related to removing VoIP permissions. The refactoring of permission checks and hook updates (useVoipExtensionPermission to useShowVoipExtension) are necessary to handle the permission removals.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/remove-old-voip-permissions

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 72ae8c1 and 3e746ec.

📒 Files selected for processing (3)
  • apps/meteor/app/api/server/v1/im.ts (2 hunks)
  • apps/meteor/app/api/server/v1/users.ts (1 hunks)
  • apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/meteor/app/api/server/v1/im.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation

Files:

  • apps/meteor/app/api/server/v1/users.ts
  • apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR #36718, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.

Applied to files:

  • apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).

Applied to files:

  • apps/meteor/app/api/server/v1/users.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/users.ts (1)
apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts (1)
  • canEditExtension (15-25)
⏰ 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
🔇 Additional comments (3)
apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts (2)

116-116: LGTM!

The call site correctly reflects the updated canEditExtension signature.


15-25: Authorization change is intentional and properly implemented across all call sites.

The removal of the userId parameter and permission check has been completed consistently throughout the codebase. The manage-voip-extensions permission was deliberately deleted as part of the v325 migration (matching the 8.0.0 VoIP permission removal strategy). All two call sites of canEditExtension use the new signature correctly: one in validateUserEditing.ts (line 116) for user editing validation, and one in the users API endpoint (line 329). The authorization model has shifted from role-based (manage-voip-extensions) to feature-flag-based (VoIP_TeamCollab_Enabled), which is the intended behavior.

apps/meteor/app/api/server/v1/users.ts (1)

329-331: LGTM with note on authorization change.

The call to canEditExtension correctly matches the updated signature. This change is consistent with the authorization model change noted in validateUserEditing.ts, where permission-based checks have been replaced with feature-flag-only gating.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pierre-lehnen-rc pierre-lehnen-rc marked this pull request as ready for review December 2, 2025 17:55
@pierre-lehnen-rc pierre-lehnen-rc requested review from a team as code owners December 2, 2025 17:55
@pierre-lehnen-rc pierre-lehnen-rc added the stat: QA assured Means it has been tested and approved by a company insider label Dec 2, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Dec 2, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

📦 Docker Image Size Report

📈 Changes

Service Current Baseline Change Percent
sum of all images 1.1GiB 1.1GiB +38MiB
rocketchat 366MiB 347MiB +19MiB
omnichannel-transcript-service 141MiB 132MiB +9.0MiB
queue-worker-service 141MiB 132MiB +9.0MiB
ddp-streamer-service 127MiB 126MiB +385KiB
account-service 114MiB 113MiB +406KiB
authorization-service 111MiB 111MiB +399KiB
presence-service 111MiB 111MiB +397KiB

📊 Historical Trend

---
config:
  theme: "dark"
  xyChart:
    width: 900
    height: 400
---
xychart
  title "Image Size Evolution by Service (Last 30 Days + This PR)"
  x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 18:29", "12/02 18:58 (PR)"]
  y-axis "Size (GB)" 0 --> 0.5
  line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
  line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.14]
  line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.14]
  line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.36]
Loading

Statistics (last 11 days):

  • 📊 Average: 1.5GiB
  • ⬇️ Minimum: 1.2GiB
  • ⬆️ Maximum: 1.6GiB
  • 🎯 Current PR: 1.1GiB
ℹ️ About this report

This report compares Docker image sizes from this build against the develop baseline.

  • Tag: pr-37672
  • Baseline: develop
  • Timestamp: 2025-12-02 18:58:42 UTC
  • Historical data points: 11

Updated: Tue, 02 Dec 2025 18:58:42 GMT

@ggazzo ggazzo requested review from a team as code owners December 2, 2025 21:26
@kodiakhq kodiakhq bot removed the stat: ready to merge PR tested and approved waiting for merge label Dec 2, 2025
@kodiakhq
Copy link
Contributor

kodiakhq bot commented Dec 2, 2025

This PR currently has a merge conflict. Please resolve this and then re-add the ['stat: ready to merge', 'automerge'] label.

@ggazzo ggazzo force-pushed the release-8.0.0 branch 4 times, most recently from 169b783 to 73d9200 Compare December 2, 2025 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: conflict stat: QA assured Means it has been tested and approved by a company insider

Projects

None yet

Development

Successfully merging this pull request may close these issues.