Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Users without preview permission subscribing to public channel stream #34922

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

fix: Users without preview permission subscribing to public channel stream #34922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c594af8
fix
tiagoevanp Jan 9, 2025
af11983
Merge branch 'develop' into fix/public-room-subscription-preview
tiagoevanp Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { roomCoordinator } from '../../../../client/lib/rooms/roomCoordinator';
import { fireGlobalEvent } from '../../../../client/lib/utils/fireGlobalEvent';
import { getConfig } from '../../../../client/lib/utils/getConfig';
import { callbacks } from '../../../../lib/callbacks';
import { hasPermission } from '../../../authorization/client';
import { Messages, Subscriptions, CachedChatSubscription } from '../../../models/client';
import { sdk } from '../../../utils/client/lib/SDKClient';

Expand Down Expand Up @@ -82,6 +83,7 @@ const computation = Tracker.autorun(() => {
if (!mainReady.get()) {
return;
}

Tracker.nonreactive(() =>
Object.entries(openedRooms).forEach(([typeName, record]) => {
if (record.active !== true || (record.ready === true && record.streamActive === true)) {
Expand All @@ -96,6 +98,11 @@ const computation = Tracker.autorun(() => {
void RoomHistoryManager.getMoreIfIsEmpty(record.rid);

if (room) {
const subscription = Subscriptions.findOne({ rid: record.rid }, { reactive: false });
if (!hasPermission('preview-c-room') && room.t === 'c' && !subscription) {
return;
}

if (record.streamActive !== true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't if (record.streamActive !== true) { check come first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because if stream not active we will have exactly what we are trying to avoid, load the stream.
In this case we have another check who comes first, and it's related to preview the public room with no subscriptions...

void sdk
.stream('room-messages', [record.rid], async (msg) => {
Expand All @@ -105,7 +112,6 @@ const computation = Tracker.autorun(() => {
// }
// Do not load command messages into channel
if (msg.t !== 'command') {
const subscription = Subscriptions.findOne({ rid: record.rid }, { reactive: false });
const isNew = !Messages.findOne({ _id: msg._id, temp: { $ne: true } });
await upsertMessage({ msg, subscription });

Expand Down
29 changes: 25 additions & 4 deletions apps/meteor/client/views/room/composer/ComposerMessage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { IMessage, ISubscription } from '@rocket.chat/core-typings';
import { useToastMessageDispatch } from '@rocket.chat/ui-contexts';
import { usePermission, useToastMessageDispatch } from '@rocket.chat/ui-contexts';
import type { ReactElement, ReactNode } from 'react';
import { memo, useCallback, useMemo } from 'react';

Expand Down Expand Up @@ -29,6 +29,7 @@ const ComposerMessage = ({ tmid, onSend, ...props }: ComposerMessageProps): Reac
const chat = useChat();
const room = useRoom();
const dispatchToastMessage = useToastMessageDispatch();
const hasPreviewPublicRoomPermission = usePermission('preview-c-room');

const composerProps = useMemo(
() => ({
Expand All @@ -53,7 +54,7 @@ const ComposerMessage = ({ tmid, onSend, ...props }: ComposerMessageProps): Reac
isSlashCommandAllowed?: boolean;
}): Promise<void> => {
try {
await chat?.action.stop('typing');
chat?.action.stop('typing');
const newMessageSent = await chat?.flows.sendMessage({
text,
tshow,
Expand Down Expand Up @@ -85,11 +86,31 @@ const ComposerMessage = ({ tmid, onSend, ...props }: ComposerMessageProps): Reac
useCallback(() => LegacyRoomManager.getOpenedRoomByRid(room._id)?.streamActive ?? false, [room._id]),
);

if (!publicationReady) {
if (!publicationReady && hasPreviewPublicRoomPermission) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I'm part of the room and I don't have this permission? Won't it break the loading composer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because publicationReady turns into true!
But if we proceed with the new design, the one without composer, we should be fine and remove this permission check!

return <ComposerSkeleton />;
}

return <MessageBox key={room._id} tmid={tmid} {...composerProps} showFormattingTips={true} {...props} />;
const subscribe = () => {
if (!hasPreviewPublicRoomPermission) {
// Get room subscription
LegacyRoomManager.open({ rid: room._id, typeName: room.t });
LegacyRoomManager.computation.invalidate();
}
};
Comment on lines +93 to +99
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useOpenRoom already does LegacyRoomManager.open. Also, why is this fn called subscribe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but a lot of other things too. And it does not do the computation.invalidate too...


return (
<MessageBox
key={room._id}
tmid={tmid}
{...composerProps}
showFormattingTips={true}
onJoin={async () => {
await composerProps.onJoin();
subscribe();
}}
{...props}
/>
);
};

export default memo(ComposerMessage);
Loading