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.

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

User correct Matrix accounts when redacting events #780

Merged

Conversation

tadzik
Copy link
Collaborator

@tadzik tadzik commented Mar 29, 2024

Fixes #766

For events sent from Slack and later deleted from Slack, we use the same Slack Ghost that sent the message – this alleviates the need for an elevated Power Level.

For events sent from Matrix and deleted from Slack, we try to use our bot account. This does require us to have a sufficient power level, so we try to be helpful in the logs if we don't.

Note: Slack doesn't tell us who deleted the message, so we can't bridge that precisely – the sender of a redact event is just our best effort.

This makes us the original sender's intent to redact messages,
so that we do not require an elevated PL to handle message deletion.
@tadzik tadzik requested a review from a team as a code owner March 29, 2024 10:44
@@ -50,6 +50,7 @@ export interface ISlackEventMessageAttachment {
}

export interface ISlackMessageEvent extends ISlackEvent {
team?: string;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have not actually seen team_id in incoming Slack messages, and the only place I see it used is when it's artificially assigned to in SlackRTMHandler – perhaps it's an error in our typing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really remember when this was introduced, it's possible it's invalid and team_id is the right one.

src/SlackEventHandler.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Some thoughts.

}
else {
const ghost = await this.main.ghostStore.get(previousMessage.user, previousMessage.team_domain, previousMessage.team);
await ghost.redactEvent(originalEvent.roomId, originalEvent.eventId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we try with the botClient if this fails, as a fallback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in b4f2911

}
}
else if (!previousMessage.user) {
log.error("Cannot redact Slack message if we don't know the original sender:", previousMessage);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just use the botClient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in b4f2911; this has a side effect of us trying to delete messages sent by originally-Matrix users with the slack bot – which probably makes sense.

@@ -50,6 +50,7 @@ export interface ISlackEventMessageAttachment {
}

export interface ISlackMessageEvent extends ISlackEvent {
team?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really remember when this was introduced, it's possible it's invalid and team_id is the right one.

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Good to go after changelog.

@tadzik
Copy link
Collaborator Author

tadzik commented Apr 17, 2024

Good to go after changelog.

Added in e2df892

@Half-Shot Half-Shot merged commit e46e46a into matrix-org:develop Apr 29, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loss of messages after a redacted events
2 participants