Differential D6929 Diff 23883 keyserver/src/updaters/thread-updaters.js

Changeset View

Standalone View

keyserver/src/updaters/thread-updaters.js

Show All 17 Lines	import {
type RoleChangeRequest,		type RoleChangeRequest,
type ChangeThreadSettingsResult,		type ChangeThreadSettingsResult,
type RemoveMembersRequest,		type RemoveMembersRequest,
type LeaveThreadRequest,		type LeaveThreadRequest,
type LeaveThreadResult,		type LeaveThreadResult,
type UpdateThreadRequest,		type UpdateThreadRequest,
type ServerThreadJoinRequest,		type ServerThreadJoinRequest,
type ThreadJoinResult,		type ThreadJoinResult,
		type ToggleMessagePinRequest,
threadPermissions,		threadPermissions,
threadTypes,		threadTypes,
} from 'lib/types/thread-types.js';		} from 'lib/types/thread-types.js';
import { updateTypes } from 'lib/types/update-types.js';		import { updateTypes } from 'lib/types/update-types.js';
import { ServerError } from 'lib/utils/errors.js';		import { ServerError } from 'lib/utils/errors.js';
import { promiseAll } from 'lib/utils/promises.js';		import { promiseAll } from 'lib/utils/promises.js';
import { firstLine } from 'lib/utils/string-utils.js';		import { firstLine } from 'lib/utils/string-utils.js';

▲ Show 20 Lines • Show All 802 Lines • ▼ Show 20 Lines	updateDatas.push({
threadID: threadID,		threadID: threadID,
targetSession: viewer.session,		targetSession: viewer.session,
});		});
}		}

await createUpdates(updateDatas);		await createUpdates(updateDatas);
}		}

		async function toggleMessagePinForThread(
		viewer: Viewer,
		request: ToggleMessagePinRequest,
		): Promise<void> {
		const { messageID, threadID, action } = request;

		const hasPermission = await checkThreadPermission(
		viewer,
		threadID,
		threadPermissions.MANAGE_PINS,
		);
		tomekUnsubmitted Not Done Inline Actions Is it a good idea to trust a client that that a message if from a thread with `threadID`? For example, a client could send a `threadID` of a thread where pinning messages is allowed, and `messageID` from a thread where it isn't. It should be a lot safer to only receive a `messageID` and fetch `threadID` from the db. tomek: Is it a good idea to trust a client that that a message if from a thread with `threadID`? For…
		rohanAuthorUnsubmitted Done Inline Actions From a security perspective, if we're worried about a client who has pin / unpin privileges in one thread making a request with a message from another thread, maybe an easier solution would be to update the query to UPDATE messages SET pinned = 1, pin_time = ${Date.now()} WHERE id = ${messageID} AND thread = ${threadID} would be a solution to prevent another query? From my thinking, this validates 1) permissions between thread and viewer, and 2) ensures that the requested message is part of the thread that the viewer has permission to pin in. If I'm missing something though, I can also fetch the `threadID` from the `messageID`. rohan: From a security perspective, if we're worried about a client who has pin / unpin privileges in…
		tomekUnsubmitted Not Done Inline Actions It also makes sense. But the downside is that we won't inform a client that setting pin failed. Still, adding a condition on thread is beneficial. tomek: It also makes sense. But the downside is that we won't inform a client that setting pin failed.
		ashoatUnsubmitted Not Done Inline Actions Arguably we could just fetch the message and use the `threadID` from that ashoat: Arguably we could just fetch the message and use the `threadID` from that

		if (!hasPermission) {
		throw new ServerError('invalid_credentials');
		}

		// Toggle the pin status: if the message is pinned, unpin it, and vice versa
		tomekUnsubmitted Not Done Inline Actions Is this comment still relevant? We're not checking in the query what is the pin status - we're only setting the status based on what we received in a request. Also, the whole function might be renamed, as we're not toggling. tomek: Is this comment still relevant? We're not checking in the query what is the pin status - we're…
		rohanAuthorUnsubmitted Done Inline Actions Yeah I have the same opinion in my comment here - if we change it there I'll change it here rohan: Yeah I have the same opinion in my [[ https://phab.comm.dev/D6930#inline-47014 \| comment ]]…
		let togglePinQuery;
		if (action === 'pin') {
		togglePinQuery = SQL`
		UPDATE messages
		SET pinned = 1, pin_time = ${Date.now()}
		WHERE id = ${messageID}
		`;
		} else {
		togglePinQuery = SQL`
		UPDATE messages
		SET pinned = 0, pin_time = NULL
		WHERE id = ${messageID}
		`;
		}
		tomekUnsubmitted Not Done Inline Actions Maybe it will be a little more maintainable if the query was defined in one place and only the values were computed conditionally? tomek: Maybe it will be a little more maintainable if the query was defined in one place and only the…
		rohanAuthorUnsubmitted Done Inline Actions Yeah that's fair I'll update to do this instead rohan: Yeah that's fair I'll update to do this instead

		await dbQuery(togglePinQuery);
		}

export {		export {
updateRole,		updateRole,
removeMembers,		removeMembers,
leaveThread,		leaveThread,
updateThread,		updateThread,
joinThread,		joinThread,
updateThreadMembers,		updateThreadMembers,
		toggleMessagePinForThread,
};		};