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

Removing a few more Result<bool> . #4977

Merged
merged 14 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
5 changes: 1 addition & 4 deletions crates/api/src/community/add_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,12 @@ pub async fn add_mod_to_community(
// moderator. This is necessary because otherwise the action would be rejected
// by the community's home instance.
if local_user_view.local_user.admin && !community.local {
let is_mod = CommunityModeratorView::is_community_moderator(
CommunityModeratorView::is_community_moderator(
&mut context.pool(),
community.id,
local_user_view.person.id,
)
.await?;
if !is_mod {
Err(LemmyErrorType::NotAModerator)?
}
}

// Update in local database
Expand Down
4 changes: 1 addition & 3 deletions crates/api/src/local_user/save_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ pub async fn save_user_settings(
let previous_email = local_user_view.local_user.email.clone().unwrap_or_default();
// if email was changed, check that it is not taken and send verification mail
if previous_email.deref() != email {
if LocalUser::is_email_taken(&mut context.pool(), email).await? {
return Err(LemmyErrorType::EmailAlreadyExists)?;
}
LocalUser::is_email_taken(&mut context.pool(), email).await?;
send_verification_email(
&local_user_view,
email,
Expand Down
8 changes: 2 additions & 6 deletions crates/api_common/src/claims.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,8 @@ impl Claims {
let claims =
decode::<Claims>(jwt, &key, &validation).with_lemmy_type(LemmyErrorType::NotLoggedIn)?;
let user_id = LocalUserId(claims.claims.sub.parse()?);
let is_valid = LoginToken::validate(&mut context.pool(), user_id, jwt).await?;
if !is_valid {
Err(LemmyErrorType::NotLoggedIn)?
} else {
Ok(user_id)
}
LoginToken::validate(&mut context.pool(), user_id, jwt).await?;
Ok(user_id)
}

pub async fn generate(
Expand Down
84 changes: 7 additions & 77 deletions crates/api_common/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,7 @@ pub async fn is_mod_or_admin(
community_id: CommunityId,
) -> LemmyResult<()> {
check_user_valid(person)?;

let is_mod_or_admin = CommunityView::is_mod_or_admin(pool, person.id, community_id).await?;
if !is_mod_or_admin {
Err(LemmyErrorType::NotAModOrAdmin)?
} else {
Ok(())
}
CommunityView::is_mod_or_admin(pool, person.id, community_id).await
}

#[tracing::instrument(skip_all)]
Expand Down Expand Up @@ -106,13 +100,7 @@ pub async fn check_community_mod_of_any_or_admin_action(
let person = &local_user_view.person;

check_user_valid(person)?;

let is_mod_of_any_or_admin = CommunityView::is_mod_of_any_or_admin(pool, person.id).await?;
if !is_mod_of_any_or_admin {
Err(LemmyErrorType::NotAModOrAdmin)?
} else {
Ok(())
}
CommunityView::is_mod_of_any_or_admin(pool, person.id).await
}

pub fn is_admin(local_user_view: &LocalUserView) -> LemmyResult<()> {
Expand Down Expand Up @@ -202,7 +190,7 @@ pub async fn check_community_user_action(
) -> LemmyResult<()> {
check_user_valid(person)?;
check_community_deleted_removed(community_id, pool).await?;
check_community_ban(person, community_id, pool).await?;
CommunityPersonBanView::get(pool, person.id, community_id).await?;
Ok(())
}

Expand All @@ -219,19 +207,6 @@ async fn check_community_deleted_removed(
Ok(())
}

async fn check_community_ban(
person: &Person,
community_id: CommunityId,
pool: &mut DbPool<'_>,
) -> LemmyResult<()> {
// check if user was banned from site or community
let is_banned = CommunityPersonBanView::get(pool, person.id, community_id).await?;
if is_banned {
Err(LemmyErrorType::BannedFromCommunity)?
}
Ok(())
}

/// Check that the given user can perform a mod action in the community.
///
/// In particular it checks that he is an admin or mod, wasn't banned and the community isn't
Expand All @@ -243,7 +218,7 @@ pub async fn check_community_mod_action(
pool: &mut DbPool<'_>,
) -> LemmyResult<()> {
is_mod_or_admin(pool, person, community_id).await?;
check_community_ban(person, community_id, pool).await?;
CommunityPersonBanView::get(pool, person.id, community_id).await?;

// it must be possible to restore deleted community
if !allow_deleted {
Expand All @@ -269,51 +244,6 @@ pub fn check_comment_deleted_or_removed(comment: &Comment) -> LemmyResult<()> {
}
}

/// Throws an error if a recipient has blocked a person.
#[tracing::instrument(skip_all)]
pub async fn check_person_block(
my_id: PersonId,
potential_blocker_id: PersonId,
pool: &mut DbPool<'_>,
) -> LemmyResult<()> {
let is_blocked = PersonBlock::read(pool, potential_blocker_id, my_id).await?;
if is_blocked {
Err(LemmyErrorType::PersonIsBlocked)?
} else {
Ok(())
}
}

/// Throws an error if a recipient has blocked a community.
#[tracing::instrument(skip_all)]
async fn check_community_block(
community_id: CommunityId,
person_id: PersonId,
pool: &mut DbPool<'_>,
) -> LemmyResult<()> {
let is_blocked = CommunityBlock::read(pool, person_id, community_id).await?;
if is_blocked {
Err(LemmyErrorType::CommunityIsBlocked)?
} else {
Ok(())
}
}

/// Throws an error if a recipient has blocked an instance.
#[tracing::instrument(skip_all)]
async fn check_instance_block(
instance_id: InstanceId,
person_id: PersonId,
pool: &mut DbPool<'_>,
) -> LemmyResult<()> {
let is_blocked = InstanceBlock::read(pool, person_id, instance_id).await?;
if is_blocked {
Err(LemmyErrorType::InstanceIsBlocked)?
} else {
Ok(())
}
}

#[tracing::instrument(skip_all)]
pub async fn check_person_instance_community_block(
my_id: PersonId,
Expand All @@ -322,9 +252,9 @@ pub async fn check_person_instance_community_block(
community_id: CommunityId,
pool: &mut DbPool<'_>,
) -> LemmyResult<()> {
check_person_block(my_id, potential_blocker_id, pool).await?;
check_instance_block(community_instance_id, potential_blocker_id, pool).await?;
check_community_block(community_id, potential_blocker_id, pool).await?;
PersonBlock::read(pool, potential_blocker_id, my_id).await?;
InstanceBlock::read(pool, potential_blocker_id, community_instance_id).await?;
CommunityBlock::read(pool, potential_blocker_id, community_id).await?;
Ok(())
}

Expand Down
5 changes: 1 addition & 4 deletions crates/api_crud/src/post/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,15 +97,12 @@ pub async fn create_post(
.ok_or(LemmyErrorType::CouldntFindCommunity)?;
if community.posting_restricted_to_mods {
let community_id = data.community_id;
let is_mod = CommunityModeratorView::is_community_moderator(
CommunityModeratorView::is_community_moderator(
&mut context.pool(),
community_id,
local_user_view.local_user.person_id,
)
.await?;
if !is_mod {
Err(LemmyErrorType::OnlyModsCanPostInCommunity)?
}
}

// Only need to check if language is allowed in case user set it explicitly. When using default
Expand Down
8 changes: 4 additions & 4 deletions crates/api_crud/src/private_message/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use lemmy_api_common::{
private_message::{CreatePrivateMessage, PrivateMessageResponse},
send_activity::{ActivityChannel, SendActivityData},
utils::{
check_person_block,
get_interface_language,
get_url_blocklist,
local_site_to_slur_regex,
Expand All @@ -16,6 +15,7 @@ use lemmy_api_common::{
use lemmy_db_schema::{
source::{
local_site::LocalSite,
person_block::PersonBlock,
private_message::{PrivateMessage, PrivateMessageInsertForm},
},
traits::Crud,
Expand All @@ -39,10 +39,10 @@ pub async fn create_private_message(
let content = process_markdown(&data.content, &slur_regex, &url_blocklist, &context).await?;
is_valid_body_field(&content, false)?;

check_person_block(
local_user_view.person.id,
data.recipient_id,
PersonBlock::read(
&mut context.pool(),
data.recipient_id,
local_user_view.person.id,
)
.await?;

Expand Down
29 changes: 10 additions & 19 deletions crates/api_crud/src/user/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,22 +73,15 @@ pub async fn register(
}

if local_site.site_setup && local_site.captcha_enabled {
if let Some(captcha_uuid) = &data.captcha_uuid {
let uuid = uuid::Uuid::parse_str(captcha_uuid)?;
let check = CaptchaAnswer::check_captcha(
&mut context.pool(),
CheckCaptchaAnswer {
uuid,
answer: data.captcha_answer.clone().unwrap_or_default(),
},
)
.await?;
if !check {
Err(LemmyErrorType::CaptchaIncorrect)?
}
} else {
Err(LemmyErrorType::CaptchaIncorrect)?
}
let uuid = uuid::Uuid::parse_str(&data.captcha_uuid.clone().unwrap_or_default())?;
CaptchaAnswer::check_captcha(
&mut context.pool(),
CheckCaptchaAnswer {
uuid,
answer: data.captcha_answer.clone().unwrap_or_default(),
},
)
.await?;
}

let slur_regex = local_site_to_slur_regex(&local_site);
Expand All @@ -104,9 +97,7 @@ pub async fn register(
)?;

if let Some(email) = &data.email {
if LocalUser::is_email_taken(&mut context.pool(), email).await? {
Err(LemmyErrorType::EmailAlreadyExists)?
}
LocalUser::is_email_taken(&mut context.pool(), email).await?;
}

// We have to create both a person, and local_user
Expand Down
6 changes: 2 additions & 4 deletions crates/apub/src/activities/community/announce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,8 @@ async fn can_accept_activity_in_community(
context: &Data<LemmyContext>,
) -> LemmyResult<()> {
if let Some(community) = community {
if !community.local
&& !CommunityFollower::has_local_followers(&mut context.pool(), community.id).await?
{
Err(LemmyErrorType::CommunityHasNoFollowers)?
if !community.local {
CommunityFollower::has_local_followers(&mut context.pool(), community.id).await?
}
// Local only community can't federate
if community.visibility != CommunityVisibility::Public {
Expand Down
11 changes: 3 additions & 8 deletions crates/apub/src/activities/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,7 @@ pub(crate) async fn verify_person_in_community(
}
let person_id = person.id;
let community_id = community.id;
let is_banned = CommunityPersonBanView::get(&mut context.pool(), person_id, community_id).await?;
if is_banned {
Err(LemmyErrorType::PersonIsBannedFromCommunity)?
} else {
Ok(())
}
CommunityPersonBanView::get(&mut context.pool(), person_id, community_id).await
}

/// Verify that mod action in community was performed by a moderator.
Expand All @@ -109,8 +104,8 @@ pub(crate) async fn verify_mod_action(
let mod_ = mod_id.dereference(context).await?;

let is_mod_or_admin =
CommunityView::is_mod_or_admin(&mut context.pool(), mod_.id, community.id).await?;
if is_mod_or_admin {
CommunityView::is_mod_or_admin(&mut context.pool(), mod_.id, community.id).await;
if is_mod_or_admin.is_ok() {
return Ok(());
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some of these changes need to be fixed to correctly handle errors other than reading false in the query. For example, change this function to:

  // mod action comes from the same instance as the community, so it was presumably done
  // by an instance admin.
  // TODO: federate instance admin status and check it here
  if mod_id.inner().domain() == community.actor_id.domain() {
    return Ok(());
  }

  let mod_ = mod_id.dereference(context).await?;

  CommunityView::is_mod_or_admin(&mut context.pool(), mod_.id, community.id).await

If you don't want to change NotAModerator to NotAModOrAdmin, do this:

.map_err(|mut err| {
  if err.error_type == LemmyErrorType::NotAModOrAdmin {
     err.error_type = LemmyErrorType::NotAModerator;
  }
  err
})

An alternative is to use LemmyResult<LemmyResult<()>> for the return types, which would mean the only big change is that forgetting to use the inner value will cause a warning. This result should be checked with something like result?? or result? == Ok(()), not with is_ok because accidental use of result.is_ok() instead of result?.is_ok() would be hard to notice.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix this one. But also the ?.then_some(()).ok_or(LEMMY_ERROR). will convert any Ok(false) or diesel error into the lemmyerror, so we should be okay.

I'll make sure I get all the tests passing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm finding some (like the blocks), should actually be testing the not(exists( case, since a missing row should pass the check.

Expand Down
11 changes: 2 additions & 9 deletions crates/apub/src/objects/post.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,15 +184,8 @@ impl Object for ApubPost {
let creator = page.creator()?.dereference(context).await?;
let community = page.community(context).await?;
if community.posting_restricted_to_mods {
let is_mod = CommunityModeratorView::is_community_moderator(
&mut context.pool(),
community.id,
creator.id,
)
.await?;
if !is_mod {
Err(LemmyErrorType::OnlyModsCanPostInCommunity)?
}
CommunityModeratorView::is_community_moderator(&mut context.pool(), community.id, creator.id)
.await?;
}
let mut name = page
.name
Expand Down
5 changes: 3 additions & 2 deletions crates/apub/src/objects/private_message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,13 @@ use activitypub_federation::{
use chrono::{DateTime, Utc};
use lemmy_api_common::{
context::LemmyContext,
utils::{check_person_block, get_url_blocklist, local_site_opt_to_slur_regex, process_markdown},
utils::{get_url_blocklist, local_site_opt_to_slur_regex, process_markdown},
};
use lemmy_db_schema::{
source::{
local_site::LocalSite,
person::Person,
person_block::PersonBlock,
private_message::{PrivateMessage, PrivateMessageInsertForm},
},
traits::Crud,
Expand Down Expand Up @@ -130,7 +131,7 @@ impl Object for ApubPrivateMessage {
) -> LemmyResult<ApubPrivateMessage> {
let creator = note.attributed_to.dereference(context).await?;
let recipient = note.to[0].dereference(context).await?;
check_person_block(creator.id, recipient.id, &mut context.pool()).await?;
PersonBlock::read(&mut context.pool(), recipient.id, creator.id).await?;

let local_site = LocalSite::read(&mut context.pool()).await.ok();
let slur_regex = &local_site_opt_to_slur_regex(&local_site);
Expand Down
11 changes: 6 additions & 5 deletions crates/db_schema/src/impls/captcha_answer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use diesel::{
QueryDsl,
};
use diesel_async::RunQueryDsl;
use lemmy_utils::{error::LemmyResult, LemmyErrorType};

impl CaptchaAnswer {
pub async fn insert(pool: &mut DbPool<'_>, captcha: &CaptchaAnswerForm) -> Result<Self, Error> {
Expand All @@ -27,7 +28,7 @@ impl CaptchaAnswer {
pub async fn check_captcha(
pool: &mut DbPool<'_>,
to_check: CheckCaptchaAnswer,
) -> Result<bool, Error> {
) -> LemmyResult<()> {
let conn = &mut get_conn(pool).await?;

// fetch requested captcha
Expand All @@ -43,7 +44,9 @@ impl CaptchaAnswer {
.execute(conn)
.await?;

Ok(captcha_exists)
captcha_exists
.then_some(())
.ok_or(LemmyErrorType::CaptchaIncorrect.into())
}
}

Expand Down Expand Up @@ -83,7 +86,6 @@ mod tests {
.await;

assert!(result.is_ok());
assert!(result.unwrap());
}

#[tokio::test]
Expand Down Expand Up @@ -119,7 +121,6 @@ mod tests {
)
.await;

assert!(result_repeat.is_ok());
assert!(!result_repeat.unwrap());
assert!(result_repeat.is_err());
}
}
Loading