mirror of
https://github.com/dsec-hub/dsec-discord-bot.git
synced 2026-09-22 07:44:26 +00:00
COL-BOT-01: /unlink edits its deferred reply + disambiguates delete failures
#7b — one coherent reply: /unlink now takes ApplicationContext, defers via defer_response(true), and EDITs that deferred ephemeral (create/edit at the serenity layer). Previously ctx.send after a defer posted a followup in this poise version, leaving the "thinking…" placeholder dangling. Defer failure now aborts before any DB op (mirrors the verify fix). new #A — no false "nothing changed": a DELETE whose HTTP call errors may still have committed server-side. On a delete error /unlink now reads the row back: row present -> genuinely failed, nothing changed; row gone -> the delete took effect, proceed to role removal; read-back also fails -> report status UNKNOWN and do NOT touch the role, telling the moderator to verify by hand. It never asserts "no change" on a bare error. new #B — runbook SQL corrected: the bot stores student ids as digits only (strips the leading s and punctuation), so the find-link query now searches '123456789', not 's123456789', with a note on the stored format. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE
This commit is contained in:
parent
dcf0866fef
commit
327a4962d1
2 changed files with 149 additions and 81 deletions
|
|
@ -29,10 +29,12 @@ You need the Supabase project credentials for this. **Who holds them:** the club
|
||||||
committee — ask in the committee Discord. (Historically the bot maintainer; TODO: name
|
committee — ask in the committee Discord. (Historically the bot maintainer; TODO: name
|
||||||
the current holder here.) Do **not** paste real credentials into a chat or a ticket.
|
the current holder here.) Do **not** paste real credentials into a chat or a ticket.
|
||||||
|
|
||||||
Find the link row(s) for a student id:
|
Find the link row(s) for a student id. The bot stores the id as **digits only** (it
|
||||||
|
strips the leading `s` and any punctuation before saving), so search the bare digits —
|
||||||
|
`'123456789'`, not `'s123456789'`:
|
||||||
|
|
||||||
```sql
|
```sql
|
||||||
select * from dsec_discord_members where student_id = 's123456789';
|
select * from dsec_discord_members where student_id = '123456789';
|
||||||
```
|
```
|
||||||
|
|
||||||
Delete a specific link by hand:
|
Delete a specific link by hand:
|
||||||
|
|
|
||||||
|
|
@ -1,4 +1,7 @@
|
||||||
use crate::{Context, Error, events::interaction_create::DiscordMemberRow, redact_digits};
|
use crate::{
|
||||||
|
AppState, ApplicationContext, Context, Error, events::interaction_create::DiscordMemberRow,
|
||||||
|
redact_digits,
|
||||||
|
};
|
||||||
use poise::{CreateReply, serenity_prelude as serenity};
|
use poise::{CreateReply, serenity_prelude as serenity};
|
||||||
|
|
||||||
/// Send message to logs channel
|
/// Send message to logs channel
|
||||||
|
|
@ -139,6 +142,29 @@ pub async fn embed(
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Whether a `dsec_discord_members` row exists for `discord_id`. Used as a read-back
|
||||||
|
/// to disambiguate a delete whose HTTP call errored (SEC-19 new #A).
|
||||||
|
async fn link_row_exists(state: &AppState, discord_id: &str) -> Result<bool, Error> {
|
||||||
|
let rows: Vec<serde_json::Value> = state
|
||||||
|
.supabase
|
||||||
|
.database()
|
||||||
|
.from("dsec_discord_members")
|
||||||
|
.select("discord_id")
|
||||||
|
.eq("discord_id", discord_id)
|
||||||
|
.execute()
|
||||||
|
.await?;
|
||||||
|
Ok(!rows.is_empty())
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Build an ephemeral edit for the deferred `/unlink` response.
|
||||||
|
fn unlink_reply(title: &str, description: impl Into<String>) -> serenity::EditInteractionResponse {
|
||||||
|
serenity::EditInteractionResponse::new().embed(
|
||||||
|
serenity::CreateEmbed::new()
|
||||||
|
.title(title)
|
||||||
|
.description(description),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
/// Remove a member's verification link (COL-BOT-01).
|
/// Remove a member's verification link (COL-BOT-01).
|
||||||
///
|
///
|
||||||
/// This is the moderator-gated undo for a hijacked verification. It deletes the
|
/// This is the moderator-gated undo for a hijacked verification. It deletes the
|
||||||
|
|
@ -149,6 +175,10 @@ pub async fn embed(
|
||||||
/// the role by hand. A member-facing `/unverify` is deliberately NOT provided: a
|
/// the role by hand. A member-facing `/unverify` is deliberately NOT provided: a
|
||||||
/// self-service unlink would let a hijacker cover their tracks.
|
/// self-service unlink would let a hijacker cover their tracks.
|
||||||
///
|
///
|
||||||
|
/// Uses `ApplicationContext` so it can `defer_response` and then EDIT that one deferred
|
||||||
|
/// ephemeral (poise's `ctx.send` after a defer posts a *followup* in this version,
|
||||||
|
/// leaving the "thinking…" placeholder dangling — SEC-19 #7b).
|
||||||
|
///
|
||||||
/// AuthZ note: the `MANAGE_ROLES` gate is for command visibility only and is NOT
|
/// AuthZ note: the `MANAGE_ROLES` gate is for command visibility only and is NOT
|
||||||
/// trusted for authorization — poise 0.6 treats a DM invoker as `Permissions::all()`,
|
/// trusted for authorization — poise 0.6 treats a DM invoker as `Permissions::all()`,
|
||||||
/// so the gate passes in a DM. The command is `guild_only` and, before any database
|
/// so the gate passes in a DM. The command is `guild_only` and, before any database
|
||||||
|
|
@ -156,41 +186,48 @@ pub async fn embed(
|
||||||
/// moderator of some *other* guild the bot is in from deleting DSEC rows (SEC-19 #1).
|
/// moderator of some *other* guild the bot is in from deleting DSEC rows (SEC-19 #1).
|
||||||
#[poise::command(slash_command, guild_only, required_permissions = "MANAGE_ROLES")]
|
#[poise::command(slash_command, guild_only, required_permissions = "MANAGE_ROLES")]
|
||||||
pub async fn unlink(
|
pub async fn unlink(
|
||||||
ctx: Context<'_>,
|
ctx: ApplicationContext<'_>,
|
||||||
#[description = "The member whose verification link should be removed"] user: serenity::User,
|
#[description = "The member whose verification link should be removed"] user: serenity::User,
|
||||||
) -> Result<(), Error> {
|
) -> Result<(), Error> {
|
||||||
let state = &ctx.data().state;
|
let state = &ctx.data.state;
|
||||||
|
let serenity_ctx = ctx.serenity_context;
|
||||||
|
let interaction = ctx.interaction;
|
||||||
|
|
||||||
// AuthZ, before anything else and before any DB op: must be the DSEC guild.
|
// AuthZ, before anything else and before any DB op: must be the DSEC guild.
|
||||||
if ctx.guild_id() != Some(state.guild_id) {
|
if interaction.guild_id != Some(state.guild_id) {
|
||||||
ctx.send(
|
interaction
|
||||||
CreateReply::default()
|
.create_response(
|
||||||
.embed(
|
serenity_ctx,
|
||||||
serenity::CreateEmbed::new()
|
serenity::CreateInteractionResponse::Message(
|
||||||
.title("Unavailable here")
|
serenity::CreateInteractionResponseMessage::new()
|
||||||
.description("This command can only be used in the DSEC server."),
|
.embed(
|
||||||
)
|
serenity::CreateEmbed::new()
|
||||||
.ephemeral(true),
|
.title("Unavailable here")
|
||||||
)
|
.description("This command can only be used in the DSEC server."),
|
||||||
.await?;
|
)
|
||||||
|
.ephemeral(true),
|
||||||
|
),
|
||||||
|
)
|
||||||
|
.await?;
|
||||||
return Ok(());
|
return Ok(());
|
||||||
}
|
}
|
||||||
|
|
||||||
// Acknowledge within Discord's ~3s window before any DB/HTTP work, so a mutation
|
// Acknowledge within Discord's ~3s window before any DB/HTTP work. If the defer
|
||||||
// can never happen without an ack and every reply below edits this deferred
|
// itself fails the interaction is dead — ABORT before any mutation (SEC-19 #7a).
|
||||||
// response (SEC-19 #7).
|
if let Err(err) = ctx.defer_response(true).await {
|
||||||
ctx.defer_ephemeral().await?;
|
eprintln!("[unlink] defer failed: {}", redact_digits(&err.to_string()));
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
|
||||||
let user_id = user.id.to_string();
|
let user_id = user.id.to_string();
|
||||||
let moderator = ctx.author();
|
let moderator = &interaction.user;
|
||||||
|
|
||||||
// 1. Delete the link row FIRST. `.returning(...)` is required: without it
|
// 1. Delete the link row FIRST. `.returning(...)` is required (COR-03 empty-204).
|
||||||
// PostgREST answers a DELETE with an empty 204 body that fails to deserialise
|
// On a delete error we must NOT assert "nothing changed": the DELETE may have
|
||||||
// (the COR-03 gotcha), and the returned rows also tell us whether a link
|
// committed before a response/body failure. Read the row back to disambiguate,
|
||||||
// actually existed. Handle a delete error HERE (do not let `?` skip the
|
// and report UNKNOWN only if the read-back also fails (SEC-19 #A). Errors are
|
||||||
// moderator reply and the audit): a raw supabase error also leaks the target
|
// redacted before printing and never shown to the user (SEC-19 #9).
|
||||||
// id, so it is redacted before printing and never shown to the user (SEC-19 #9).
|
let link_status: &str = match state
|
||||||
let deleted = match state
|
|
||||||
.supabase
|
.supabase
|
||||||
.database()
|
.database()
|
||||||
.delete("dsec_discord_members")
|
.delete("dsec_discord_members")
|
||||||
|
|
@ -199,65 +236,101 @@ pub async fn unlink(
|
||||||
.execute::<DiscordMemberRow>()
|
.execute::<DiscordMemberRow>()
|
||||||
.await
|
.await
|
||||||
{
|
{
|
||||||
Ok(rows) => rows,
|
Ok(rows) if rows.is_empty() => "no link row existed",
|
||||||
|
Ok(_) => "deleted",
|
||||||
Err(err) => {
|
Err(err) => {
|
||||||
eprintln!(
|
eprintln!(
|
||||||
"[unlink] delete failed for target {}: {}",
|
"[unlink] delete failed for target {}: {}",
|
||||||
user.id,
|
user.id,
|
||||||
redact_digits(&err.to_string())
|
redact_digits(&err.to_string())
|
||||||
);
|
);
|
||||||
let _ = ctx
|
match link_row_exists(state, &user_id).await {
|
||||||
.send(
|
// Row still present: the delete genuinely did not happen.
|
||||||
CreateReply::default()
|
Ok(true) => {
|
||||||
.embed(serenity::CreateEmbed::new().title("Unlink failed").description(
|
let _ = interaction
|
||||||
"Could not remove the link row (database error). Nothing was changed. Try again, or remove it by hand — see SECURITY.md.",
|
.edit_response(
|
||||||
))
|
serenity_ctx,
|
||||||
.ephemeral(true),
|
unlink_reply(
|
||||||
)
|
"Unlink failed",
|
||||||
.await;
|
"The database returned an error and the link row is still present (confirmed by read-back). Nothing changed. Try again, or remove it by hand — see SECURITY.md.",
|
||||||
let _ = log_embed(
|
),
|
||||||
ctx.serenity_context(),
|
)
|
||||||
state.logs_channel_id,
|
.await;
|
||||||
Some("Unlink FAILED (database error)".to_string()),
|
let _ = log_embed(
|
||||||
None,
|
serenity_ctx,
|
||||||
Some(format!(
|
state.logs_channel_id,
|
||||||
"Moderator <@{}> (id `{}`) ran /unlink on <@{}> (id `{}`), but deleting the link row failed. No changes made.",
|
Some("Unlink FAILED (no change)".to_string()),
|
||||||
moderator.id, moderator.id, user.id, user.id
|
None,
|
||||||
)),
|
Some(format!(
|
||||||
None,
|
"Moderator <@{}> (id `{}`) ran /unlink on <@{}> (id `{}`): the delete errored and a read-back confirms the row is still present. No changes made.",
|
||||||
None,
|
moderator.id, moderator.id, user.id, user.id
|
||||||
None,
|
)),
|
||||||
None,
|
None,
|
||||||
Some(true),
|
None,
|
||||||
)
|
None,
|
||||||
.await;
|
None,
|
||||||
return Ok(());
|
Some(true),
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
// Row gone: the delete actually committed; fall through to role removal.
|
||||||
|
Ok(false) => "deleted (confirmed by read-back after a write error)",
|
||||||
|
// Read-back also failed: genuinely UNKNOWN — do not touch the role.
|
||||||
|
Err(err2) => {
|
||||||
|
eprintln!(
|
||||||
|
"[unlink] read-back after delete error failed for target {}: {}",
|
||||||
|
user.id,
|
||||||
|
redact_digits(&err2.to_string())
|
||||||
|
);
|
||||||
|
let _ = interaction
|
||||||
|
.edit_response(
|
||||||
|
serenity_ctx,
|
||||||
|
unlink_reply(
|
||||||
|
"Unlink status UNKNOWN",
|
||||||
|
"A database error occurred and a follow-up read could not confirm whether the link row was removed. It MAY already be gone. Verify by hand before relying on this — see SECURITY.md. The role was not touched.",
|
||||||
|
),
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
let _ = log_embed(
|
||||||
|
serenity_ctx,
|
||||||
|
state.logs_channel_id,
|
||||||
|
Some("Unlink UNKNOWN (database error)".to_string()),
|
||||||
|
None,
|
||||||
|
Some(format!(
|
||||||
|
"Moderator <@{}> (id `{}`) ran /unlink on <@{}> (id `{}`): the delete errored and a read-back also failed. The link row may or may not be removed; manual verification required. Role not touched.",
|
||||||
|
moderator.id, moderator.id, user.id, user.id
|
||||||
|
)),
|
||||||
|
None,
|
||||||
|
None,
|
||||||
|
None,
|
||||||
|
None,
|
||||||
|
Some(true),
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
let had_link = !deleted.is_empty();
|
|
||||||
|
|
||||||
// 2. Then remove the verified role. Report clearly if THIS half fails so a human
|
// 2. Then remove the verified role. We reach here only when the row is gone or
|
||||||
// can finish it — the row is already gone, so nothing else is inconsistent.
|
// never existed. Report clearly if THIS half fails so a human can finish it.
|
||||||
|
// Role errors are Discord API errors (no student id), safe to show the mod.
|
||||||
let role_id = state.verified_role_id;
|
let role_id = state.verified_role_id;
|
||||||
let mut role_removed = false;
|
let mut role_removed = false;
|
||||||
let mut role_error: Option<String> = None;
|
let mut role_error: Option<String> = None;
|
||||||
// We already asserted this is the DSEC guild, so operate on it directly. Role
|
match state.guild_id.member(serenity_ctx, user.id).await {
|
||||||
// errors are Discord API errors (no student id), so they are safe to show the mod.
|
Ok(member) => match member.remove_role(serenity_ctx, role_id).await {
|
||||||
match state.guild_id.member(ctx.serenity_context(), user.id).await {
|
|
||||||
Ok(member) => match member.remove_role(ctx.serenity_context(), role_id).await {
|
|
||||||
Ok(()) => role_removed = true,
|
Ok(()) => role_removed = true,
|
||||||
Err(err) => role_error = Some(err.to_string()),
|
Err(err) => role_error = Some(err.to_string()),
|
||||||
},
|
},
|
||||||
Err(err) => role_error = Some(err.to_string()),
|
Err(err) => role_error = Some(err.to_string()),
|
||||||
}
|
}
|
||||||
|
|
||||||
// Ephemeral report to the moderator (edits the deferred response). Best-effort so a
|
// Report to the moderator by EDITING the deferred response (exactly one ephemeral,
|
||||||
// failed reply cannot skip the audit log below (SEC-19 #7).
|
// not a followup — SEC-19 #7b). Best-effort so a failed reply cannot skip the audit.
|
||||||
let mut summary = if had_link {
|
let mut summary = format!("Link row: {link_status}.\n");
|
||||||
String::from("Deleted the verification link row.\n")
|
|
||||||
} else {
|
|
||||||
String::from("No verification link row existed (nothing to delete).\n")
|
|
||||||
};
|
|
||||||
if role_removed {
|
if role_removed {
|
||||||
summary.push_str("Removed the verified role.");
|
summary.push_str("Removed the verified role.");
|
||||||
} else {
|
} else {
|
||||||
|
|
@ -266,29 +339,22 @@ pub async fn unlink(
|
||||||
role_error.as_deref().unwrap_or("unknown error")
|
role_error.as_deref().unwrap_or("unknown error")
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
let _ = interaction
|
||||||
let _ = ctx
|
.edit_response(serenity_ctx, unlink_reply("Unlink", summary))
|
||||||
.send(
|
|
||||||
CreateReply::default()
|
|
||||||
.embed(serenity::CreateEmbed::new().title("Unlink").description(summary))
|
|
||||||
.ephemeral(true),
|
|
||||||
)
|
|
||||||
.await;
|
.await;
|
||||||
|
|
||||||
// Audit to the logs channel, naming the moderator — runs regardless of whether the
|
// Audit to the logs channel, naming the moderator — runs regardless of the reply.
|
||||||
// reply above succeeded.
|
|
||||||
let _ = log_embed(
|
let _ = log_embed(
|
||||||
ctx.serenity_context(),
|
serenity_ctx,
|
||||||
state.logs_channel_id,
|
state.logs_channel_id,
|
||||||
Some("Verification link removed (/unlink)".to_string()),
|
Some("Verification link removed (/unlink)".to_string()),
|
||||||
None,
|
None,
|
||||||
Some(format!(
|
Some(format!(
|
||||||
"Moderator <@{}> (id `{}`) ran /unlink on <@{}> (id `{}`). Link row: {}. Verified role: {}.",
|
"Moderator <@{}> (id `{}`) ran /unlink on <@{}> (id `{}`). Link row: {link_status}. Verified role: {}.",
|
||||||
moderator.id,
|
moderator.id,
|
||||||
moderator.id,
|
moderator.id,
|
||||||
user.id,
|
user.id,
|
||||||
user.id,
|
user.id,
|
||||||
if had_link { "deleted" } else { "none found" },
|
|
||||||
if role_removed {
|
if role_removed {
|
||||||
"removed"
|
"removed"
|
||||||
} else {
|
} else {
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue