fix: hide workspace registration status from users without manage-cloud permission #37951
Conversation
🦋 Changeset detectedLatest commit: e7c5048 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces an admin-role check with a permission-based check ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37951 +/- ##
===========================================
- Coverage 70.63% 70.63% -0.01%
===========================================
Files 3143 3143
Lines 108693 108695 +2
Branches 19577 19537 -40
===========================================
+ Hits 76779 76780 +1
- Misses 29904 29910 +6
+ Partials 2010 2005 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report📈 Changes
📊 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/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 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/26 20:12 (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, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 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, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 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, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 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.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
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, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 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.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 26 Dec 2025 20:12:14 GMT |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx">
<violation number="1" location="apps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx:51">
P1: The `isRegistered` value will be `undefined` during loading, which is falsy and will incorrectly show "Workspace not registered" until the data loads. Consider destructuring `isPending` from `useIsRegistered()` and including it in the loading condition, or defaulting `isRegistered` to handle the loading state.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
apps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/hooks/useIsRegistered.ts (1)
8-14: Consider simplifying the query function.The
queryFnwraps the result in a new object, but the endpoint already returns{ isRegistered: boolean }. You can return the result directly.🔎 Proposed simplification
return useQuery({ queryKey: ['cloud', 'isRegistered'], - queryFn: async () => { - const result = await getIsRegistered(); - return { isRegistered: result.isRegistered }; - }, + queryFn: getIsRegistered, staleTime: Infinity, });
📜 Review details
Configuration used: Organization 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 (6)
.changeset/polite-oranges-attack.mdapps/meteor/app/api/server/v1/cloud.tsapps/meteor/client/hooks/useIsRegistered.tsapps/meteor/client/hooks/useRegistrationStatus.tsapps/meteor/client/views/admin/subscription/SubscriptionPage.tsxapps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx
💤 Files with no reviewable changes (1)
- apps/meteor/client/hooks/useRegistrationStatus.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/client/views/admin/subscription/SubscriptionPage.tsxapps/meteor/app/api/server/v1/cloud.tsapps/meteor/client/hooks/useIsRegistered.tsapps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/client/views/admin/subscription/SubscriptionPage.tsx
📚 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:
.changeset/polite-oranges-attack.md
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/client/hooks/useIsRegistered.ts
🧬 Code graph analysis (3)
apps/meteor/client/views/admin/subscription/SubscriptionPage.tsx (1)
apps/meteor/client/hooks/useIsRegistered.ts (1)
useIsRegistered(5-16)
apps/meteor/app/api/server/v1/cloud.ts (4)
apps/meteor/app/api/server/index.ts (1)
API(51-51)packages/rest-typings/src/v1/Ajv.ts (1)
ajv(24-24)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)packages/rest-typings/src/index.ts (1)
Endpoints(48-93)
apps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx (1)
apps/meteor/client/hooks/useIsRegistered.ts (1)
useIsRegistered(5-16)
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (5)
.changeset/polite-oranges-attack.md (1)
1-5: LGTM!The changeset correctly documents the fix as a patch version bump with a clear description of the issue being resolved.
apps/meteor/client/views/admin/subscription/SubscriptionPage.tsx (1)
34-34: LGTM!The migration to
useIsRegisteredis correctly implemented. The optional chaining onregistrationData?.isRegisteredappropriately handles loading and error states, defaulting toundefinedwhich safely hides the sync button until the registration status is confirmed.Also applies to: 57-57, 68-68
apps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx (1)
50-51: LGTM!The migration to
useIsRegisteredis correctly implemented. Note that while the registration status is loading,isRegisteredwill beundefined, causing!isRegistered(line 86) to evaluate as truthy. This briefly shows the "Register Workspace" button during loading, which is acceptable UX since it resolves quickly.apps/meteor/app/api/server/v1/cloud.ts (2)
183-210: LGTM!The new
cloud.isRegisteredendpoint correctly implements the PR objective. Key observations:
- Security:
authRequired: truewithoutpermissionsRequiredis intentional, allowing all authenticated users to check registration status. This is appropriate since the endpoint only exposes a single boolean value, not sensitive workspace details.- Response schema: Properly validates the response with AJV.
- Type extraction: Uses
ExtractRoutesFromAPIto derive type-safe route definitions.
212-221: LGTM!The type augmentation correctly extends the
Endpointsinterface with both the auto-extractedCloudEndpointsand the manually declared/v1/cloud.checkoutUrlroute that uses the olderaddRoutepattern.
ggazzo
left a comment
ggazzo
left a comment
There was a problem hiding this comment.
what is wrong with the registrationStatus ? Is there no way to fix this by altering this endpoint? It seems to me that it was designed precisely for what we're trying to fix. otherwise will this endpoint no longer be used? Should it be deprecated? I think there should be a changeset about the new endpoint, one about deprecation, and then one about fixing it.
if we agree that exposing this information more broadly is acceptable, then we could consider reusing the existing endpoint. however, that would result in a breaking change either by removing sensitive fields from the response or by removing the role check, right? edit: i’ve added the code assuming we move forward with a new route and deprecate the existing one. also, waiting on team’s input on which approach makes more sense, since adjusting the current route would introduce a breaking change. |
ricardogarim
force-pushed
the
fix/workspace-registration-status
branch
from
a4ed873 to
284060d
Compare
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
ricardogarim
force-pushed
the
fix/workspace-registration-status
branch
from
284060d to
8bfa2d6
Compare
ricardogarim
changed the title
ricardogarim
force-pushed
the
fix/workspace-registration-status
branch
from
8bfa2d6 to
e7c5048
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/cloud.ts (1)
86-100: Consider usingpermissionsRequiredin route options for consistency.The manual permission check using
hasPermissionAsyncworks correctly, but other routes in this file usepermissionsRequiredin the route configuration (lines 42, 58, 68, 106, 126). For consistency and idiomatic code, consider refactoring to:🔎 Proposed refactor
API.v1.addRoute( 'cloud.registrationStatus', - { authRequired: true }, + { authRequired: true, permissionsRequired: ['manage-cloud'] }, { async get() { - if (!(await hasPermissionAsync(this.userId, 'manage-cloud'))) { - return API.v1.forbidden(); - } - const registrationStatus = await retrieveRegistrationStatus(); return API.v1.success({ registrationStatus });
📜 Review details
Configuration used: Organization 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)
.changeset/little-steaks-itch.mdapps/meteor/app/api/server/v1/cloud.tsapps/meteor/client/hooks/useRegistrationStatus.tsapps/meteor/client/views/admin/subscription/SubscriptionPage.tsxapps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/little-steaks-itch.md
🧰 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/cloud.tsapps/meteor/client/views/admin/subscription/SubscriptionPage.tsxapps/meteor/client/hooks/useRegistrationStatus.tsapps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx
🧠 Learnings (2)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/client/views/admin/subscription/SubscriptionPage.tsx
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx
🧬 Code graph analysis (3)
apps/meteor/client/views/admin/subscription/SubscriptionPage.tsx (1)
apps/meteor/client/hooks/useRegistrationStatus.ts (1)
useRegistrationStatus(11-31)
apps/meteor/client/hooks/useRegistrationStatus.ts (2)
packages/rest-typings/src/index.ts (1)
OperationResult(191-193)packages/ui-contexts/src/index.ts (1)
usePermission(57-57)
apps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx (1)
apps/meteor/client/hooks/useRegistrationStatus.ts (1)
useRegistrationStatus(11-31)
⏰ 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). (7)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/views/admin/subscription/SubscriptionPage.tsx (1)
57-57: LGTM! Proper permission-based gating.The change from
isRegisteredtocanViewRegistrationStatuscorrectly ensures that the "Sync license update" button is only visible to users with themanage-cloudpermission, which aligns with the PR objectives.Also applies to: 111-115
apps/meteor/client/hooks/useRegistrationStatus.ts (1)
6-31: LGTM! Important typo fix and proper API expansion.The changes correctly:
- Fix the typo (
canViewregistrationStatus→canViewRegistrationStatus)- Expose
canViewRegistrationStatusin the return type for consumers- Compute
isRegisteredas!queryResult.isPending && queryResult.data?.registrationStatus?.workspaceRegistered, which properly handles the loading state (returnsfalseduring loading rather thanundefined)apps/meteor/client/views/admin/workspace/VersionCard/VersionCard.tsx (1)
50-50: LGTM! Proper permission-based gating of registration status.The changes correctly ensure that registration-related UI elements (register button and status indicators) are only shown to users with the
manage-cloudpermission. The useMemo dependencies are properly updated to includecanViewRegistrationStatus.Note: The previous concern about
isRegisteredbeingundefinedduring loading has been addressed by theuseRegistrationStatushook, which now returnsfalseduring loading rather thanundefined.Also applies to: 85-96, 110-110, 157-167
As per SUP-871, this PR fixes the issue where "Workspace not registered" incorrectly shows for all users with access to the workspace administration page, even when the workspace is actually registered.
Proposed changes (including videos or screenshots)
/v1/cloud.registrationStatusauthorization fromadminrole tomanage-cloudpermission (aligns with frontend)canViewRegistrationStatusflag and fixed typo (canViewregistrationStatus->canViewRegistrationStatus)VersionCard.tsx: Conditionally display registration status only whencanViewRegistrationStatusis trueSubscriptionPage.tsx: Show "Sync license update" button based oncanViewRegistrationStatusinstead ofisRegisteredIssue(s)
Steps to test or reproduce
manage-cloudpermission/admin/infoBefore: "Workspace not registered" shows incorrectly even though workspace is registered
After: Registration status is hidden for users without
manage-cloudpermission (no false "not registered" message)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.