From e36ad9d98474a0e36a2d64111aa31e20df152d43 Mon Sep 17 00:00:00 2001 From: Dessalines Date: Wed, 26 Jan 2022 12:57:16 -0500 Subject: [PATCH] Removing the site creator, adding leave_admin. Fixes #1808 (#2052) * Removing the site creator, adding leave_admin. Fixes #1808 * Making sure there's at least one admin. Fixing unit tests --- crates/api/src/community.rs | 16 +------ crates/api/src/lib.rs | 4 +- crates/api/src/local_user.rs | 15 +------ crates/api/src/site.rs | 44 ++++++++----------- crates/api_common/src/site.rs | 3 +- crates/api_crud/src/site/create.rs | 1 - crates/api_crud/src/site/read.rs | 13 +----- crates/api_crud/src/site/update.rs | 1 - .../src/aggregates/site_aggregates.rs | 10 +++-- crates/db_schema/src/impls/person.rs | 6 +++ crates/db_schema/src/impls/site.rs | 9 +--- crates/db_schema/src/schema.rs | 2 - crates/db_schema/src/source/site.rs | 7 +-- crates/db_views/src/site_view.rs | 26 +++-------- crates/websocket/src/lib.rs | 2 +- .../down.sql | 14 ++++++ .../up.sql | 2 + src/api_routes.rs | 4 +- 18 files changed, 63 insertions(+), 116 deletions(-) create mode 100644 migrations/2022-01-20-160328_remove_site_creator/down.sql create mode 100644 migrations/2022-01-20-160328_remove_site_creator/up.sql diff --git a/crates/api/src/community.rs b/crates/api/src/community.rs index c8b44aceb..a1a44f094 100644 --- a/crates/api/src/community.rs +++ b/crates/api/src/community.rs @@ -44,7 +44,6 @@ use lemmy_db_schema::{ }, person::Person, post::Post, - site::Site, }, traits::{Bannable, Blockable, Crud, Followable, Joinable}, }; @@ -457,20 +456,7 @@ impl Perform for TransferCommunity { let local_user_view = get_local_user_view_from_jwt(&data.auth, context.pool(), context.secret()).await?; - let site_creator_id = blocking(context.pool(), move |conn| { - Site::read(conn, 1).map(|s| s.creator_id) - }) - .await??; - - let mut admins = blocking(context.pool(), PersonViewSafe::admins).await??; - - // Making sure the site creator, if an admin, is at the top - let creator_index = admins - .iter() - .position(|r| r.person.id == site_creator_id) - .context(location_info!())?; - let creator_person = admins.remove(creator_index); - admins.insert(0, creator_person); + let admins = blocking(context.pool(), PersonViewSafe::admins).await??; // Fetch the community mods let community_id = data.community_id; diff --git a/crates/api/src/lib.rs b/crates/api/src/lib.rs index 3cffe0166..c7c24062d 100644 --- a/crates/api/src/lib.rs +++ b/crates/api/src/lib.rs @@ -111,9 +111,7 @@ pub async fn match_websocket_operation( UserOperation::TransferCommunity => { do_websocket_operation::(context, id, op, data).await } - UserOperation::TransferSite => { - do_websocket_operation::(context, id, op, data).await - } + UserOperation::LeaveAdmin => do_websocket_operation::(context, id, op, data).await, // Community ops UserOperation::FollowCommunity => { diff --git a/crates/api/src/local_user.rs b/crates/api/src/local_user.rs index 9452e7540..974beb6ea 100644 --- a/crates/api/src/local_user.rs +++ b/crates/api/src/local_user.rs @@ -1,6 +1,5 @@ use crate::{captcha_as_wav_base64, Perform}; use actix_web::web::Data; -use anyhow::Context; use bcrypt::verify; use captcha::{gen, Difficulty}; use chrono::Duration; @@ -51,7 +50,6 @@ use lemmy_db_views_actor::{ }; use lemmy_utils::{ claims::Claims, - location_info, utils::{is_valid_display_name, is_valid_matrix_id, naive_from_unix}, ConnectionId, LemmyError, @@ -409,18 +407,7 @@ impl Perform for AddAdmin { blocking(context.pool(), move |conn| ModAdd::create(conn, &form)).await??; - let site_creator_id = blocking(context.pool(), move |conn| { - Site::read(conn, 1).map(|s| s.creator_id) - }) - .await??; - - let mut admins = blocking(context.pool(), PersonViewSafe::admins).await??; - let creator_index = admins - .iter() - .position(|r| r.person.id == site_creator_id) - .context(location_info!())?; - let creator_person = admins.remove(creator_index); - admins.insert(0, creator_person); + let admins = blocking(context.pool(), PersonViewSafe::admins).await??; let res = AddAdminResponse { admins }; diff --git a/crates/api/src/site.rs b/crates/api/src/site.rs index 04cd099ec..c77b177fa 100644 --- a/crates/api/src/site.rs +++ b/crates/api/src/site.rs @@ -1,6 +1,5 @@ use crate::Perform; use actix_web::web::Data; -use anyhow::Context; use diesel::NotFound; use lemmy_api_common::{ blocking, @@ -27,6 +26,7 @@ use lemmy_db_schema::{ source::{ local_user::{LocalUser, LocalUserForm}, moderator::*, + person::Person, registration_application::{RegistrationApplication, RegistrationApplicationForm}, site::Site, }, @@ -62,7 +62,7 @@ use lemmy_db_views_moderator::{ mod_sticky_post_view::ModStickyPostView, mod_transfer_community_view::ModTransferCommunityView, }; -use lemmy_utils::{location_info, settings::structs::Settings, version, ConnectionId, LemmyError}; +use lemmy_utils::{settings::structs::Settings, version, ConnectionId, LemmyError}; use lemmy_websocket::LemmyContext; #[async_trait::async_trait(?Send)] @@ -462,7 +462,7 @@ async fn convert_response( } #[async_trait::async_trait(?Send)] -impl Perform for TransferSite { +impl Perform for LeaveAdmin { type Response = GetSiteResponse; #[tracing::instrument(skip(context, _websocket_id))] @@ -471,44 +471,36 @@ impl Perform for TransferSite { context: &Data, _websocket_id: Option, ) -> Result { - let data: &TransferSite = self; + let data: &LeaveAdmin = self; let local_user_view = get_local_user_view_from_jwt(&data.auth, context.pool(), context.secret()).await?; is_admin(&local_user_view)?; - let read_site = blocking(context.pool(), Site::read_simple).await??; - - // Make sure user is the creator - if read_site.creator_id != local_user_view.person.id { - return Err(LemmyError::from_message("not_an_admin")); + // Make sure there isn't just one admin (so if one leaves, there will still be one left) + let admins = blocking(context.pool(), PersonViewSafe::admins).await??; + if admins.len() == 1 { + return Err(LemmyError::from_message("cannot_leave_admin")); } - let new_creator_id = data.person_id; - let transfer_site = move |conn: &'_ _| Site::transfer(conn, new_creator_id); - blocking(context.pool(), transfer_site) - .await? - .map_err(LemmyError::from) - .map_err(|e| e.with_message("couldnt_update_site"))?; + let person_id = local_user_view.person.id; + blocking(context.pool(), move |conn| { + Person::leave_admin(conn, person_id) + }) + .await??; // Mod tables let form = ModAddForm { - mod_person_id: local_user_view.person.id, - other_person_id: data.person_id, - removed: Some(false), + mod_person_id: person_id, + other_person_id: person_id, + removed: Some(true), }; blocking(context.pool(), move |conn| ModAdd::create(conn, &form)).await??; + // Reread site and admins let site_view = blocking(context.pool(), SiteView::read).await??; - - let mut admins = blocking(context.pool(), PersonViewSafe::admins).await??; - let creator_index = admins - .iter() - .position(|r| r.person.id == site_view.creator.id) - .context(location_info!())?; - let creator_person = admins.remove(creator_index); - admins.insert(0, creator_person); + let admins = blocking(context.pool(), PersonViewSafe::admins).await??; let federated_instances = build_federated_instances( context.pool(), diff --git a/crates/api_common/src/site.rs b/crates/api_common/src/site.rs index 07f7e8539..e3bc52a2d 100644 --- a/crates/api_common/src/site.rs +++ b/crates/api_common/src/site.rs @@ -155,8 +155,7 @@ pub struct MyUserInfo { } #[derive(Debug, Serialize, Deserialize)] -pub struct TransferSite { - pub person_id: PersonId, +pub struct LeaveAdmin { pub auth: Sensitive, } diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index 043a31c2b..3de2c3ab8 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -62,7 +62,6 @@ impl PerformCrud for CreateSite { description, icon, banner, - creator_id: local_user_view.person.id, enable_downvotes: data.enable_downvotes, open_registration: data.open_registration, enable_nsfw: data.enable_nsfw, diff --git a/crates/api_crud/src/site/read.rs b/crates/api_crud/src/site/read.rs index 229d5939c..ee777d2d7 100644 --- a/crates/api_crud/src/site/read.rs +++ b/crates/api_crud/src/site/read.rs @@ -79,18 +79,7 @@ impl PerformCrud for GetSite { } }; - let mut admins = blocking(context.pool(), PersonViewSafe::admins).await??; - - // Make sure the site creator is the top admin - if let Some(site_view) = site_view.to_owned() { - let site_creator_id = site_view.creator.id; - // TODO investigate why this is sometimes coming back null - // Maybe user_.admin isn't being set to true? - if let Some(creator_index) = admins.iter().position(|r| r.person.id == site_creator_id) { - let creator_person = admins.remove(creator_index); - admins.insert(0, creator_person); - } - } + let admins = blocking(context.pool(), PersonViewSafe::admins).await??; let online = context .chat_server() diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index bcd6a1a32..0a94062a7 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -54,7 +54,6 @@ impl PerformCrud for EditSite { } let site_form = SiteForm { - creator_id: found_site.creator_id, name: data.name.to_owned().unwrap_or(found_site.name), sidebar, description, diff --git a/crates/db_schema/src/aggregates/site_aggregates.rs b/crates/db_schema/src/aggregates/site_aggregates.rs index 08d4dd01e..b94899247 100644 --- a/crates/db_schema/src/aggregates/site_aggregates.rs +++ b/crates/db_schema/src/aggregates/site_aggregates.rs @@ -55,7 +55,6 @@ mod tests { let site_form = SiteForm { name: "test_site".into(), - creator_id: inserted_person.id, sidebar: None, description: None, icon: None, @@ -133,7 +132,12 @@ mod tests { let community_num_deleted = Community::delete(&conn, inserted_community.id).unwrap(); assert_eq!(1, community_num_deleted); - let after_delete = SiteAggregates::read(&conn); - assert!(after_delete.is_err()); + // Site should still exist, it can without a site creator. + let after_delete_creator = SiteAggregates::read(&conn); + assert!(after_delete_creator.is_ok()); + + Site::delete(&conn, 1).unwrap(); + let after_delete_site = SiteAggregates::read(&conn); + assert!(after_delete_site.is_err()); } } diff --git a/crates/db_schema/src/impls/person.rs b/crates/db_schema/src/impls/person.rs index 7e756543b..17833253b 100644 --- a/crates/db_schema/src/impls/person.rs +++ b/crates/db_schema/src/impls/person.rs @@ -274,6 +274,12 @@ impl Person { pub fn is_banned(&self) -> bool { is_banned(self.banned, self.ban_expires) } + + pub fn leave_admin(conn: &PgConnection, person_id: PersonId) -> Result { + diesel::update(person.find(person_id)) + .set(admin.eq(false)) + .get_result::(conn) + } } impl PersonSafe { diff --git a/crates/db_schema/src/impls/site.rs b/crates/db_schema/src/impls/site.rs index af0e8153d..8a84bdfbb 100644 --- a/crates/db_schema/src/impls/site.rs +++ b/crates/db_schema/src/impls/site.rs @@ -1,4 +1,4 @@ -use crate::{naive_now, newtypes::PersonId, source::site::*, traits::Crud}; +use crate::{source::site::*, traits::Crud}; use diesel::{dsl::*, result::Error, *}; impl Crud for Site { @@ -27,13 +27,6 @@ impl Crud for Site { } impl Site { - pub fn transfer(conn: &PgConnection, new_creator_id: PersonId) -> Result { - use crate::schema::site::dsl::*; - diesel::update(site.find(1)) - .set((creator_id.eq(new_creator_id), updated.eq(naive_now()))) - .get_result::(conn) - } - pub fn read_simple(conn: &PgConnection) -> Result { use crate::schema::site::dsl::*; site.first::(conn) diff --git a/crates/db_schema/src/schema.rs b/crates/db_schema/src/schema.rs index c01114d9e..681452d9f 100644 --- a/crates/db_schema/src/schema.rs +++ b/crates/db_schema/src/schema.rs @@ -441,7 +441,6 @@ table! { id -> Int4, name -> Varchar, sidebar -> Nullable, - creator_id -> Int4, published -> Timestamp, updated -> Nullable, enable_downvotes -> Bool, @@ -648,7 +647,6 @@ joinable!(post_read -> post (post_id)); joinable!(post_report -> post (post_id)); joinable!(post_saved -> person (person_id)); joinable!(post_saved -> post (post_id)); -joinable!(site -> person (creator_id)); joinable!(site_aggregates -> site (site_id)); joinable!(email_verification -> local_user (local_user_id)); joinable!(registration_application -> local_user (local_user_id)); diff --git a/crates/db_schema/src/source/site.rs b/crates/db_schema/src/source/site.rs index f99ffd887..01c5bc16e 100644 --- a/crates/db_schema/src/source/site.rs +++ b/crates/db_schema/src/source/site.rs @@ -1,7 +1,4 @@ -use crate::{ - newtypes::{DbUrl, PersonId}, - schema::site, -}; +use crate::{newtypes::DbUrl, schema::site}; use serde::{Deserialize, Serialize}; #[derive(Queryable, Identifiable, PartialEq, Debug, Clone, Serialize, Deserialize)] @@ -10,7 +7,6 @@ pub struct Site { pub id: i32, pub name: String, pub sidebar: Option, - pub creator_id: PersonId, pub published: chrono::NaiveDateTime, pub updated: Option, pub enable_downvotes: bool, @@ -30,7 +26,6 @@ pub struct Site { #[table_name = "site"] pub struct SiteForm { pub name: String, - pub creator_id: PersonId, pub sidebar: Option>, pub updated: Option, pub enable_downvotes: Option, diff --git a/crates/db_views/src/site_view.rs b/crates/db_views/src/site_view.rs index 3c94975aa..e77d7c6ce 100644 --- a/crates/db_views/src/site_view.rs +++ b/crates/db_views/src/site_view.rs @@ -1,38 +1,24 @@ use diesel::{result::Error, *}; use lemmy_db_schema::{ aggregates::site_aggregates::SiteAggregates, - schema::{person, site, site_aggregates}, - source::{ - person::{Person, PersonSafe}, - site::Site, - }, - traits::ToSafe, + schema::{site, site_aggregates}, + source::site::Site, }; use serde::{Deserialize, Serialize}; #[derive(Debug, Serialize, Deserialize, Clone)] pub struct SiteView { pub site: Site, - pub creator: PersonSafe, pub counts: SiteAggregates, } impl SiteView { pub fn read(conn: &PgConnection) -> Result { - let (site, creator, counts) = site::table - .inner_join(person::table) + let (site, counts) = site::table .inner_join(site_aggregates::table) - .select(( - site::all_columns, - Person::safe_columns_tuple(), - site_aggregates::all_columns, - )) - .first::<(Site, PersonSafe, SiteAggregates)>(conn)?; + .select((site::all_columns, site_aggregates::all_columns)) + .first::<(Site, SiteAggregates)>(conn)?; - Ok(SiteView { - site, - creator, - counts, - }) + Ok(SiteView { site, counts }) } } diff --git a/crates/websocket/src/lib.rs b/crates/websocket/src/lib.rs index e5c2730fa..324562180 100644 --- a/crates/websocket/src/lib.rs +++ b/crates/websocket/src/lib.rs @@ -136,7 +136,7 @@ pub enum UserOperation { MarkAllAsRead, SaveUserSettings, TransferCommunity, - TransferSite, + LeaveAdmin, PasswordReset, PasswordChange, MarkPrivateMessageAsRead, diff --git a/migrations/2022-01-20-160328_remove_site_creator/down.sql b/migrations/2022-01-20-160328_remove_site_creator/down.sql new file mode 100644 index 000000000..8195f719b --- /dev/null +++ b/migrations/2022-01-20-160328_remove_site_creator/down.sql @@ -0,0 +1,14 @@ +-- Add the column back +alter table site add column creator_id int references person on update cascade on delete cascade; + +-- Add the data, selecting the highest admin +update site +set creator_id = sub.id +from ( + select id from person + where admin = true + limit 1 +) as sub; + +-- Set to not null +alter table site alter column creator_id set not null; diff --git a/migrations/2022-01-20-160328_remove_site_creator/up.sql b/migrations/2022-01-20-160328_remove_site_creator/up.sql new file mode 100644 index 000000000..78774445b --- /dev/null +++ b/migrations/2022-01-20-160328_remove_site_creator/up.sql @@ -0,0 +1,2 @@ +-- Drop the column +alter table site drop column creator_id; diff --git a/src/api_routes.rs b/src/api_routes.rs index 5d5fe876c..69bfe50bf 100644 --- a/src/api_routes.rs +++ b/src/api_routes.rs @@ -19,7 +19,6 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimit) { // Admin Actions .route("", web::post().to(route_post_crud::)) .route("", web::put().to(route_post_crud::)) - .route("/transfer", web::post().to(route_post::)) .route("/config", web::get().to(route_get::)) .route("/config", web::put().to(route_post::)), ) @@ -212,7 +211,8 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimit) { ) .route("/report_count", web::get().to(route_get::)) .route("/unread_count", web::get().to(route_get::)) - .route("/verify_email", web::post().to(route_post::)), + .route("/verify_email", web::post().to(route_post::)) + .route("/leave_admin", web::post().to(route_post::)), ) // Admin Actions .service(