From 9010c7feeca8f4e7e501ad474911deaaf7a1a367 Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 29 Oct 2024 20:44:03 -0400 Subject: Restrict channel names, too. Thankfully, channel creation only happens in one place, so we don't need a state machine for this. --- docs/api/channels-messages.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'docs/api/channels-messages.md') diff --git a/docs/api/channels-messages.md b/docs/api/channels-messages.md index 9854d22..6391b5a 100644 --- a/docs/api/channels-messages.md +++ b/docs/api/channels-messages.md @@ -64,6 +64,14 @@ The request must have the following fields: |:-------|:-------|:--| | `name` | string | The channel's name. | +The proposed `name` must be valid. The precise definition of valid is still up in the air, but, at minimum: + +* It must be non-empty. +* It must not be "too long." (Currently, 64 characters is too long.) +* It must begin with a printing character. +* It must end with a printing character. +* It must not contain runs of multiple whitespace characters. + ### Success This endpoint will respond with a status of `202 Accepted` when successful. The body of the response will be a JSON object describing the new channel: @@ -86,7 +94,11 @@ The returned name may not be identical to the name requested, as the name will b When completed, the service will emit a [channel created](events.md#channel-created) event with the channel's ID. -### Duplicate channel name +### Name not valid + +This endpoint will respond with a status of `400 Bad Request` if the proposed `name` is not valid. + +### Channel name in use This endpoint will respond with a status of `409 Conflict` if a channel with the requested name already exists. -- cgit v1.2.3 From e328d33fc7d6a0f2e3d260d8bddee3ef633318eb Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 29 Oct 2024 22:11:36 -0400 Subject: Restrict deletion to deleting your own messages. --- docs/api/channels-messages.md | 4 ++++ src/boot/routes/test.rs | 2 +- src/event/routes/test/message.rs | 6 +++--- src/message/app.rs | 14 ++++++++++++-- src/message/routes/message/mod.rs | 9 +++++++-- src/message/routes/message/test.rs | 37 ++++++++++++++++++++++++++++++++----- 6 files changed, 59 insertions(+), 13 deletions(-) (limited to 'docs/api/channels-messages.md') diff --git a/docs/api/channels-messages.md b/docs/api/channels-messages.md index 6391b5a..d87a01c 100644 --- a/docs/api/channels-messages.md +++ b/docs/api/channels-messages.md @@ -227,3 +227,7 @@ When completed, the service will emit a [message deleted](events.md#message-dele ### Invalid message ID This endpoint will respond with a status of `404 Not Found` if the message ID is not valid. + +### Not the sender + +This endpoint will respond with a status of `403 Forbidden` if the message was sent by a different login. diff --git a/src/boot/routes/test.rs b/src/boot/routes/test.rs index 8808b70..202dcb9 100644 --- a/src/boot/routes/test.rs +++ b/src/boot/routes/test.rs @@ -85,7 +85,7 @@ async fn excludes_deleted_messages() { let deleted_message = fixtures::message::send(&app, &channel, &sender, &fixtures::now()).await; app.messages() - .delete(&deleted_message.id, &fixtures::now()) + .delete(&sender, &deleted_message.id, &fixtures::now()) .await .expect("deleting valid message succeeds"); diff --git a/src/event/routes/test/message.rs b/src/event/routes/test/message.rs index 63a3f43..a7b25fb 100644 --- a/src/event/routes/test/message.rs +++ b/src/event/routes/test/message.rs @@ -260,7 +260,7 @@ async fn deleting() { // Delete the message app.messages() - .delete(&message.id, &fixtures::now()) + .delete(&sender, &message.id, &fixtures::now()) .await .expect("deleting a valid message succeeds"); @@ -286,7 +286,7 @@ async fn previously_deleted() { // Delete the message app.messages() - .delete(&message.id, &fixtures::now()) + .delete(&sender, &message.id, &fixtures::now()) .await .expect("deleting a valid message succeeds"); @@ -320,7 +320,7 @@ async fn previously_purged() { // Purge the message app.messages() - .delete(&message.id, &fixtures::ancient()) + .delete(&sender, &message.id, &fixtures::ancient()) .await .expect("deleting a valid message succeeds"); diff --git a/src/message/app.rs b/src/message/app.rs index eed6ba4..137a27d 100644 --- a/src/message/app.rs +++ b/src/message/app.rs @@ -45,16 +45,24 @@ impl<'a> Messages<'a> { Ok(message.as_sent()) } - pub async fn delete(&self, message: &Id, deleted_at: &DateTime) -> Result<(), DeleteError> { + pub async fn delete( + &self, + deleted_by: &Login, + message: &Id, + deleted_at: &DateTime, + ) -> Result<(), DeleteError> { let mut tx = self.db.begin().await?; let message = tx .messages() .by_id(message) .await .not_found(|| DeleteError::NotFound(message.clone()))?; - message + let snapshot = message .as_snapshot() .ok_or_else(|| DeleteError::Deleted(message.id().clone()))?; + if snapshot.sender != deleted_by.id { + return Err(DeleteError::NotSender(deleted_by.clone())); + } let deleted = tx.sequence().next(deleted_at).await?; let message = tx.messages().delete(&message, &deleted).await?; @@ -138,6 +146,8 @@ impl From for SendError { pub enum DeleteError { #[error("message {0} not found")] NotFound(Id), + #[error("login {} not the message's sender", .0.id)] + NotSender(Login), #[error("message {0} deleted")] Deleted(Id), #[error(transparent)] diff --git a/src/message/routes/message/mod.rs b/src/message/routes/message/mod.rs index 45a7e9d..e92f556 100644 --- a/src/message/routes/message/mod.rs +++ b/src/message/routes/message/mod.rs @@ -20,9 +20,11 @@ pub mod delete { State(app): State, Path(message): Path, RequestedAt(deleted_at): RequestedAt, - _: Identity, + identity: Identity, ) -> Result { - app.messages().delete(&message, &deleted_at).await?; + app.messages() + .delete(&identity.login, &message, &deleted_at) + .await?; Ok(Response { id: message }) } @@ -47,6 +49,9 @@ pub mod delete { let Self(error) = self; #[allow(clippy::match_wildcard_for_single_variants)] match error { + DeleteError::NotSender(_) => { + (StatusCode::FORBIDDEN, error.to_string()).into_response() + } DeleteError::NotFound(_) | DeleteError::Deleted(_) => { NotFound(error).into_response() } diff --git a/src/message/routes/message/test.rs b/src/message/routes/message/test.rs index ae89506..5178ab5 100644 --- a/src/message/routes/message/test.rs +++ b/src/message/routes/message/test.rs @@ -8,18 +8,17 @@ pub async fn delete_message() { // Set up the environment let app = fixtures::scratch_app().await; - let sender = fixtures::login::create(&app, &fixtures::now()).await; + let sender = fixtures::identity::create(&app, &fixtures::now()).await; let channel = fixtures::channel::create(&app, &fixtures::now()).await; - let message = fixtures::message::send(&app, &channel, &sender, &fixtures::now()).await; + let message = fixtures::message::send(&app, &channel, &sender.login, &fixtures::now()).await; // Send the request - let deleter = fixtures::identity::create(&app, &fixtures::now()).await; let response = delete::handler( State(app.clone()), Path(message.id.clone()), fixtures::now(), - deleter, + sender, ) .await .expect("deleting a valid message succeeds"); @@ -68,7 +67,7 @@ pub async fn delete_deleted() { let message = fixtures::message::send(&app, &channel, &sender, &fixtures::now()).await; app.messages() - .delete(&message.id, &fixtures::now()) + .delete(&sender, &message.id, &fixtures::now()) .await .expect("deleting a recently-sent message succeeds"); @@ -155,3 +154,31 @@ pub async fn delete_purged() { assert!(matches!(error, app::DeleteError::NotFound(id) if id == message.id)); } + +#[tokio::test] +pub async fn delete_not_sender() { + // Set up the environment + + let app = fixtures::scratch_app().await; + let sender = fixtures::login::create(&app, &fixtures::now()).await; + let channel = fixtures::channel::create(&app, &fixtures::now()).await; + let message = fixtures::message::send(&app, &channel, &sender, &fixtures::now()).await; + + // Send the request + + let deleter = fixtures::identity::create(&app, &fixtures::now()).await; + let delete::Error(error) = delete::handler( + State(app.clone()), + Path(message.id.clone()), + fixtures::now(), + deleter.clone(), + ) + .await + .expect_err("deleting a message someone else sent fails"); + + // Verify the response + + assert!( + matches!(error, app::DeleteError::NotSender(error_sender) if deleter.login == error_sender) + ); +} -- cgit v1.2.3 From 36e659e971d091cfcbe370f5e45a0d01102d2e83 Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Wed, 30 Oct 2024 01:07:12 -0400 Subject: Prevent deletion of non-empty channels. --- docs/api/channels-messages.md | 5 +++- src/channel/app.rs | 49 ++++++++++++++++++++++--------- src/channel/routes/channel/delete.rs | 9 ++++-- src/channel/routes/channel/test/delete.rs | 34 ++++++++++++++++++--- 4 files changed, 76 insertions(+), 21 deletions(-) (limited to 'docs/api/channels-messages.md') diff --git a/docs/api/channels-messages.md b/docs/api/channels-messages.md index d87a01c..2aa8ac5 100644 --- a/docs/api/channels-messages.md +++ b/docs/api/channels-messages.md @@ -164,7 +164,7 @@ This endpoint will respond with a status of `404 Not Found` if the channel ID is Deletes a channel. -Deleting a channel prevents it from receiving any further messages, and deletes the messages it contains at that point. +Deleting a channel prevents it from receiving any further messages. The channel must be empty; to delete a channel with messages in it, delete the messages first (or wait for them to expire). This endpoint requires the following path parameter: @@ -190,6 +190,9 @@ The response will have the following fields: When completed, the service will emit a [message deleted](events.md#message-deleted) event for each message in the channel, followed by a [channel deleted](events.md#channel-deleted) event with the channel's ID. +### Channel not empty + +This endpoint will respond with a status of `409 Conflict` if the channel contains messages. ### Invalid channel ID diff --git a/src/channel/app.rs b/src/channel/app.rs index 9a19b16..e32eb6c 100644 --- a/src/channel/app.rs +++ b/src/channel/app.rs @@ -10,7 +10,7 @@ use crate::{ clock::DateTime, db::{Duplicate as _, NotFound as _}, event::{repo::Provider as _, Broadcaster, Event, Sequence}, - message::repo::Provider as _, + message::{self, repo::Provider as _}, name::{self, Name}, }; @@ -48,38 +48,36 @@ impl<'a> Channels<'a> { // it exists in the specific moment when you call it. pub async fn get(&self, channel: &Id) -> Result { let not_found = || Error::NotFound(channel.clone()); + let deleted = || Error::Deleted(channel.clone()); let mut tx = self.db.begin().await?; let channel = tx.channels().by_id(channel).await.not_found(not_found)?; tx.commit().await?; - channel.as_snapshot().ok_or_else(not_found) + channel.as_snapshot().ok_or_else(deleted) } - pub async fn delete(&self, channel: &Id, deleted_at: &DateTime) -> Result<(), Error> { + pub async fn delete(&self, channel: &Id, deleted_at: &DateTime) -> Result<(), DeleteError> { let mut tx = self.db.begin().await?; let channel = tx .channels() .by_id(channel) .await - .not_found(|| Error::NotFound(channel.clone()))?; + .not_found(|| DeleteError::NotFound(channel.clone()))?; channel .as_snapshot() - .ok_or_else(|| Error::Deleted(channel.id().clone()))?; + .ok_or_else(|| DeleteError::Deleted(channel.id().clone()))?; let mut events = Vec::new(); let messages = tx.messages().live(&channel).await?; - for message in messages { - let deleted = tx.sequence().next(deleted_at).await?; - let message = tx.messages().delete(&message, &deleted).await?; - events.extend( - message - .events() - .filter(Sequence::start_from(deleted.sequence)) - .map(Event::from), - ); + let has_messages = messages + .iter() + .map(message::History::as_snapshot) + .any(|message| message.is_some()); + if has_messages { + return Err(DeleteError::NotEmpty(channel.id().clone())); } let deleted = tx.sequence().next(deleted_at).await?; @@ -191,6 +189,29 @@ impl From for Error { } } +#[derive(Debug, thiserror::Error)] +pub enum DeleteError { + #[error("channel {0} not found")] + NotFound(Id), + #[error("channel {0} deleted")] + Deleted(Id), + #[error("channel {0} not empty")] + NotEmpty(Id), + #[error(transparent)] + Database(#[from] sqlx::Error), + #[error(transparent)] + Name(#[from] name::Error), +} + +impl From for DeleteError { + fn from(error: LoadError) -> Self { + match error { + LoadError::Database(error) => error.into(), + LoadError::Name(error) => error.into(), + } + } +} + #[derive(Debug, thiserror::Error)] pub enum ExpireError { #[error(transparent)] diff --git a/src/channel/routes/channel/delete.rs b/src/channel/routes/channel/delete.rs index 2d2b5f1..9c093c1 100644 --- a/src/channel/routes/channel/delete.rs +++ b/src/channel/routes/channel/delete.rs @@ -36,14 +36,19 @@ impl IntoResponse for Response { #[derive(Debug, thiserror::Error)] #[error(transparent)] -pub struct Error(#[from] pub app::Error); +pub struct Error(#[from] pub app::DeleteError); impl IntoResponse for Error { fn into_response(self) -> response::Response { let Self(error) = self; #[allow(clippy::match_wildcard_for_single_variants)] match error { - app::Error::NotFound(_) | app::Error::Deleted(_) => NotFound(error).into_response(), + app::DeleteError::NotFound(_) | app::DeleteError::Deleted(_) => { + NotFound(error).into_response() + } + app::DeleteError::NotEmpty(_) => { + (StatusCode::CONFLICT, error.to_string()).into_response() + } other => Internal::from(other).into_response(), } } diff --git a/src/channel/routes/channel/test/delete.rs b/src/channel/routes/channel/test/delete.rs index 0371b0a..77a0b03 100644 --- a/src/channel/routes/channel/test/delete.rs +++ b/src/channel/routes/channel/test/delete.rs @@ -55,7 +55,7 @@ pub async fn invalid_channel_id() { // Verify the response - assert!(matches!(error, app::Error::NotFound(id) if id == channel)); + assert!(matches!(error, app::DeleteError::NotFound(id) if id == channel)); } #[tokio::test] @@ -84,7 +84,7 @@ pub async fn channel_deleted() { // Verify the response - assert!(matches!(error, app::Error::Deleted(id) if id == channel.id)); + assert!(matches!(error, app::DeleteError::Deleted(id) if id == channel.id)); } #[tokio::test] @@ -113,7 +113,7 @@ pub async fn channel_expired() { // Verify the response - assert!(matches!(error, app::Error::Deleted(id) if id == channel.id)); + assert!(matches!(error, app::DeleteError::Deleted(id) if id == channel.id)); } #[tokio::test] @@ -147,5 +147,31 @@ pub async fn channel_purged() { // Verify the response - assert!(matches!(error, app::Error::NotFound(id) if id == channel.id)); + assert!(matches!(error, app::DeleteError::NotFound(id) if id == channel.id)); +} + +#[tokio::test] +pub async fn channel_not_empty() { + // Set up the environment + + let app = fixtures::scratch_app().await; + let channel = fixtures::channel::create(&app, &fixtures::now()).await; + let sender = fixtures::login::create(&app, &fixtures::now()).await; + fixtures::message::send(&app, &channel, &sender, &fixtures::now()).await; + + // Send the request + + let deleter = fixtures::identity::create(&app, &fixtures::now()).await; + let delete::Error(error) = delete::handler( + State(app.clone()), + Path(channel.id.clone()), + fixtures::now(), + deleter, + ) + .await + .expect_err("deleting a channel with messages fails"); + + // Verify the response + + assert!(matches!(error, app::DeleteError::NotEmpty(id) if id == channel.id)); } -- cgit v1.2.3