chore: Restrict certain messages actions on ABAC rooms #37128
Conversation
|
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 |
|
WalkthroughAdds room-aware ABAC checks and tooltips to message actions (forward, permalink, reply-in-DM), passes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant MT as Message Toolbar
participant A as Action Hook / Component
participant R as Room Context
U->>MT: Open message actions
MT->>A: computeAction(message, meta, { room })
A->>R: read room.abacAttributes
A->>A: determine isEncrypted || isABACEnabled
alt Disabled (encrypted or ABAC)
A-->>MT: { disabled: true, tooltip }
U->>MT: Hover / press
MT-->>U: Show tooltip
else Enabled
A-->>MT: { disabled: false }
U->>MT: Click action
MT->>A: execute action
A-->>U: Open modal / copy link
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (4)
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 #37128 +/- ##
===========================================
+ Coverage 67.40% 67.42% +0.01%
===========================================
Files 3288 3288
Lines 111688 111814 +126
Branches 20405 20419 +14
===========================================
+ Hits 75285 75388 +103
- Misses 33716 33738 +22
- Partials 2687 2688 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 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 (5)
apps/meteor/client/components/message/toolbar/useReplyInDMAction.ts (2)
19-20: Document timeline for IRoom type extension.The
ts-expect-errorindicatesabacAttributesis not yet in theIRoomtype definition. Please ensure there's a tracking issue or timeline for adding this field toIRoomin@rocket.chat/core-typingsto avoid long-term type suppression.
42-50: Consistent tooltip logic across actions.The tooltip computation prioritizes encryption over ABAC. Verify this precedence is intentional and consistent with
ForwardMessageActionandusePermalinkAction. If ABAC should take precedence or both conditions should display a combined message, update the logic accordingly.apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.spec.tsx (1)
86-105: Verify tooltip precedence is intentional.Line 103 expects the encryption tooltip when both conditions are true. Ensure this precedence (encryption over ABAC) is intentional and documented. If both conditions should display a combined message or ABAC should take precedence, update both the implementation and test.
apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.tsx (2)
21-22: Document timeline for IRoom type extension.The
ts-expect-errorindicatesabacAttributesis not yet in theIRoomtype definition. Ensure there's a tracking issue or timeline for adding this field toIRoomin@rocket.chat/core-typingsto avoid long-term type suppression. This pattern is repeated across multiple files in this PR (useReplyInDMAction.ts, test files).
24-32: Verify tooltip precedence is consistent.The title computation prioritizes encryption over ABAC (lines 25-30). Ensure this precedence is:
- Intentional and documented
- Consistent across all ABAC-aware actions (Reply in DM, Permalink)
If the precedence differs or should be unified, update accordingly.
📜 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 (13)
.changeset/wicked-ligers-hide.md(1 hunks)apps/meteor/app/ui-utils/client/lib/MessageAction.ts(1 hunks)apps/meteor/client/components/message/toolbar/MessageToolbarActionMenu.tsx(2 hunks)apps/meteor/client/components/message/toolbar/MessageToolbarStarsActionMenu.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/DefaultItems.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/MobileItems.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/ThreadsItems.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.spec.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.tsx(2 hunks)apps/meteor/client/components/message/toolbar/usePermalinkAction.spec.ts(1 hunks)apps/meteor/client/components/message/toolbar/usePermalinkAction.ts(3 hunks)apps/meteor/client/components/message/toolbar/useReplyInDMAction.ts(4 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.tsx (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IMessage/IMessage.ts (1)
isE2EEMessage(426-426)
apps/meteor/client/components/message/toolbar/usePermalinkAction.ts (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/ui-utils/client/lib/MessageAction.ts (1)
MessageActionConfig(21-33)packages/core-typings/src/IMessage/IMessage.ts (1)
isE2EEMessage(426-426)
apps/meteor/client/components/message/toolbar/MessageToolbarActionMenu.tsx (1)
apps/meteor/client/components/message/toolbar/usePermalinkAction.ts (1)
usePermalinkAction(10-52)
apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/tests/mocks/data.ts (1)
createFakeRoom(46-61)
apps/meteor/client/components/message/toolbar/useReplyInDMAction.ts (1)
packages/ui-contexts/src/index.ts (1)
usePermission(55-55)
apps/meteor/client/components/message/toolbar/usePermalinkAction.spec.ts (5)
apps/meteor/tests/mocks/data.ts (1)
createFakeUser(32-44)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/client/components/message/toolbar/usePermalinkAction.ts (1)
usePermalinkAction(10-52)
🔇 Additional comments (20)
apps/meteor/client/components/message/toolbar/items/MobileItems.tsx (1)
21-21: LGTM!Correctly passes the
roomprop toForwardMessageActionto enable ABAC-aware forwarding logic, consistent with updates inDefaultItems.tsxandThreadsItems.tsx.apps/meteor/app/ui-utils/client/lib/MessageAction.ts (1)
26-26: LGTM!Clean extension of the
MessageActionConfigtype to support explicit tooltip strings. The optional field is non-breaking and aligns with the broader tooltip handling refactor across message actions.apps/meteor/client/components/message/toolbar/items/DefaultItems.tsx (1)
20-20: LGTM!Correctly passes the
roomprop toForwardMessageAction, consistent with the component's updated signature.apps/meteor/client/components/message/toolbar/useReplyInDMAction.ts (1)
86-87: LGTM!Correctly disables the action when encrypted or ABAC-enabled, and conditionally spreads the tooltip when present.
apps/meteor/client/components/message/toolbar/items/ThreadsItems.tsx (1)
19-19: LGTM!Correctly passes the
roomprop toForwardMessageAction, consistent with the component's updated signature.apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.spec.tsx (3)
35-48: LGTM!Comprehensive test coverage for the normal message case with accessibility validation.
Also applies to: 107-119
50-66: LGTM!Correctly tests the encrypted message scenario with proper disabled state and accessibility checks.
Also applies to: 121-136
68-84: LGTM!Correctly tests the ABAC-enabled room scenario with proper disabled state and accessibility checks.
Also applies to: 138-153
apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.tsx (1)
1-2: LGTM!Clean integration of room context and ABAC-aware disabled state. The component correctly computes the title based on encryption and ABAC state, and disables the action when either condition is true.
Also applies to: 4-4, 13-13, 16-16, 38-38, 40-40
apps/meteor/client/components/message/toolbar/MessageToolbarStarsActionMenu.tsx (1)
40-40: Disabled actions already provide explicit tooltips. Verified that the disabled case in MessageToolbarStarsActionMenu.tsx includes an explicittooltip.apps/meteor/client/components/message/toolbar/usePermalinkAction.ts (3)
1-4: LGTM!The additional imports (
IRoomanduseMemo) are correctly used for the new room-based ABAC logic and tooltip memoization.
22-30: LGTM!The tooltip logic correctly prioritizes encryption over ABAC when both conditions are true, and the memoization is appropriate for the dependencies.
49-50: LGTM!The
disabledlogic correctly combines both conditions, and the conditional tooltip spreading is a clean pattern that avoids adding undefined fields to the return object.apps/meteor/client/components/message/toolbar/usePermalinkAction.spec.ts (5)
29-68: LGTM!The mock factories are well-structured with sensible defaults and support for overrides, enabling flexible test scenarios.
71-94: LGTM!The test correctly verifies that the permalink action is enabled for normal messages without encryption or ABAC restrictions.
95-122: Verify the expected tooltip value.Line 120 expects
tooltip: 'Action_not_available_encrypted_content', but the implementation (usePermalinkAction.ts line 24) callst('Action_not_available_encrypted_content', { action: t('Copy_link') }), which should return a translated string, not the translation key. Confirm whether the test should expect the translated value or if themockAppRootis configured to return translation keys as-is.
152-181: LGTM with verification needed.The test correctly validates that when both encryption and ABAC are present, the encrypted tooltip takes priority (line 179), which aligns with the implementation logic in usePermalinkAction.ts. However, the same translation key vs. translated value concern applies here as noted in the previous comment.
21-27: Ensure all tooltip translations are mocked in tests.The
usePermalinkAction.spec.tssetup mocks onlyCopy_linkandCopiedbut asserts tooltipsAction_not_available_encrypted_contentandNot_available_for_ABAC_enabled_rooms; mock those keys or verify fallback behavior.apps/meteor/client/components/message/toolbar/MessageToolbarActionMenu.tsx (2)
53-64: LGTM!All three
usePermalinkActioncalls correctly pass theroomparameter, enabling ABAC-aware behavior for permalink actions in different contexts (starred, pinned, and general messages).
106-106: LGTM!Delegating tooltip handling to the action configuration is cleaner and more flexible, allowing each action hook to provide its own contextual tooltip.
MartinSchoeler
changed the title
MartinSchoeler
force-pushed
the
feat/ABAC-message-restrictions
branch
from
16764f8 to
3700d4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (10)
apps/meteor/client/components/message/toolbar/MessageToolbarActionMenu.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/DefaultItems.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/MobileItems.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/ThreadsItems.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.spec.tsx(1 hunks)apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.tsx(2 hunks)apps/meteor/client/components/message/toolbar/usePermalinkAction.spec.ts(1 hunks)apps/meteor/client/components/message/toolbar/usePermalinkAction.ts(3 hunks)apps/meteor/client/components/message/toolbar/useReplyInDMAction.ts(3 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/meteor/client/components/message/toolbar/items/DefaultItems.tsx
- apps/meteor/client/components/message/toolbar/items/ThreadsItems.tsx
- apps/meteor/client/components/message/toolbar/items/MobileItems.tsx
- apps/meteor/client/components/message/toolbar/usePermalinkAction.spec.ts
- apps/meteor/client/components/message/toolbar/usePermalinkAction.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.tsx (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IMessage/IMessage.ts (1)
isE2EEMessage(426-426)
apps/meteor/client/components/message/toolbar/MessageToolbarActionMenu.tsx (1)
apps/meteor/client/components/message/toolbar/usePermalinkAction.ts (1)
usePermalinkAction(10-52)
apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/tests/mocks/data.ts (1)
createFakeRoom(46-61)
⏰ 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 (5)
apps/meteor/client/components/message/toolbar/useReplyInDMAction.ts (2)
42-50: LGTM!The tooltip calculation correctly prioritizes encrypted content messaging over ABAC restrictions and properly uses the new i18n key. The dependency array is complete and the logic is sound.
86-87: LGTM!The disabled flag correctly combines both encrypted and ABAC states, and the conditional tooltip spread is implemented idiomatically. The changes align well with the tooltip calculation logic.
apps/meteor/client/components/message/toolbar/MessageToolbarActionMenu.tsx (2)
53-54: LGTM!The addition of the
{ room }parameter to both permalink action calls is correct and aligns with the updatedusePermalinkActionsignature. The room context is properly passed from the component scope.
55-64: LGTM!The multi-line formatting improves readability for this complex permalink action configuration, and the
{ room }parameter is correctly added. All contexts, type, and order properties are properly specified.packages/i18n/src/locales/en.i18n.json (1)
7039-7042: ABAC tooltip string looks goodKey naming and copy align with the existing locale patterns. No issues spotted.
apps/meteor/client/components/message/toolbar/items/actions/ForwardMessageAction.spec.tsx
Show resolved
Hide resolved
apps/meteor/client/components/message/toolbar/usePermalinkAction.spec.ts
Show resolved
Hide resolved
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Restricts the following message actions on ABAC managed rooms:
Contains #37127
Issue(s)
ABAC-42
Steps to test or reproduce
Further comments
I've set the PR as chore since these changes are not available without mocking data
Summary by CodeRabbit
New Features
Tests