From 49219508bc2e01fe724830ca31a7cfa7adba15cf Mon Sep 17 00:00:00 2001 From: Claire Date: Sat, 3 Jul 2021 21:13:47 +0200 Subject: Fix anonymous access to outbox not being cached by the reverse proxy (#16458) * Fix anonymous access to outbox not being cached by the reverse proxy Up until now, anonymous access to outbox was marked as public, but with a 0 duration for caching, which means remote proxies would only serve from cache when the server was completely overwhelmed. Changed that cache duration to one minute, so that repeated anonymous access to one account's outbox can be appropriately cached. Also added `Signature` to the `Vary` header in case a page is requested, so that authenticated fetches are never served from cache (which only contains public toots). * Remove Vary: Accept header from webfinger controller Indeed, we have stopped returning xrd, and only ever return jrd, so the Accept request header does not matter anymore. * Cache negative webfinger hits for 3 minutes --- app/controllers/activitypub/outboxes_controller.rb | 10 +++++++++- app/controllers/well_known/webfinger_controller.rb | 3 ++- 2 files changed, 11 insertions(+), 2 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/activitypub/outboxes_controller.rb b/app/controllers/activitypub/outboxes_controller.rb index 4a52560ac..b2aab56a5 100644 --- a/app/controllers/activitypub/outboxes_controller.rb +++ b/app/controllers/activitypub/outboxes_controller.rb @@ -11,7 +11,11 @@ class ActivityPub::OutboxesController < ActivityPub::BaseController before_action :set_cache_headers def show - expires_in(page_requested? ? 0 : 3.minutes, public: public_fetch_mode? && !(signed_request_account.present? && page_requested?)) + if page_requested? + expires_in(1.minute, public: public_fetch_mode? && signed_request_account.nil?) + else + expires_in(3.minutes, public: public_fetch_mode?) + end render json: outbox_presenter, serializer: ActivityPub::OutboxSerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json' end @@ -76,4 +80,8 @@ class ActivityPub::OutboxesController < ActivityPub::BaseController def set_account @account = params[:account_username].present? ? Account.find_local!(username_param) : Account.representative end + + def set_cache_headers + response.headers['Vary'] = 'Signature' if authorized_fetch_mode? || page_requested? + end end diff --git a/app/controllers/well_known/webfinger_controller.rb b/app/controllers/well_known/webfinger_controller.rb index 0227f722a..2b296ea3b 100644 --- a/app/controllers/well_known/webfinger_controller.rb +++ b/app/controllers/well_known/webfinger_controller.rb @@ -4,7 +4,6 @@ module WellKnown class WebfingerController < ActionController::Base include RoutingHelper - before_action { response.headers['Vary'] = 'Accept' } before_action :set_account before_action :check_account_suspension @@ -39,10 +38,12 @@ module WellKnown end def bad_request + expires_in(3.minutes, public: true) head 400 end def not_found + expires_in(3.minutes, public: true) head 404 end -- cgit From 771c9d4ba87a388dc306c58139d11bf510680c98 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 8 Jul 2021 05:31:28 +0200 Subject: Add ability to skip sign-in token authentication for specific users (#16427) Remove "active within last two weeks" exception for sign in token requirement Change admin reset password to lock access until the password is reset --- app/controllers/admin/resets_controller.rb | 4 +-- .../sign_in_token_authentications_controller.rb | 27 ++++++++++++++ .../admin/two_factor_authentications_controller.rb | 2 +- app/models/user.rb | 25 +++++++++++-- app/policies/user_policy.rb | 8 +++++ app/views/admin/accounts/show.html.haml | 24 +++++++++++-- config/locales/en.yml | 42 +++++++++++++--------- config/routes.rb | 1 + ...210621221010_add_skip_sign_in_token_to_users.rb | 5 +++ db/schema.rb | 1 + lib/mastodon/accounts_cli.rb | 15 ++++++-- spec/controllers/admin/resets_controller_spec.rb | 2 +- .../two_factor_authentications_controller_spec.rb | 8 ++--- spec/models/user_spec.rb | 28 +++++++++++++++ 14 files changed, 160 insertions(+), 32 deletions(-) create mode 100644 app/controllers/admin/sign_in_token_authentications_controller.rb create mode 100644 db/migrate/20210621221010_add_skip_sign_in_token_to_users.rb (limited to 'app/controllers') diff --git a/app/controllers/admin/resets_controller.rb b/app/controllers/admin/resets_controller.rb index db8f61d64..7962b7a58 100644 --- a/app/controllers/admin/resets_controller.rb +++ b/app/controllers/admin/resets_controller.rb @@ -6,9 +6,9 @@ module Admin def create authorize @user, :reset_password? - @user.send_reset_password_instructions + @user.reset_password! log_action :reset_password, @user - redirect_to admin_accounts_path + redirect_to admin_account_path(@user.account_id) end end end diff --git a/app/controllers/admin/sign_in_token_authentications_controller.rb b/app/controllers/admin/sign_in_token_authentications_controller.rb new file mode 100644 index 000000000..e620ab292 --- /dev/null +++ b/app/controllers/admin/sign_in_token_authentications_controller.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Admin + class SignInTokenAuthenticationsController < BaseController + before_action :set_target_user + + def create + authorize @user, :enable_sign_in_token_auth? + @user.update(skip_sign_in_token: false) + log_action :enable_sign_in_token_auth, @user + redirect_to admin_account_path(@user.account_id) + end + + def destroy + authorize @user, :disable_sign_in_token_auth? + @user.update(skip_sign_in_token: true) + log_action :disable_sign_in_token_auth, @user + redirect_to admin_account_path(@user.account_id) + end + + private + + def set_target_user + @user = User.find(params[:user_id]) + end + end +end diff --git a/app/controllers/admin/two_factor_authentications_controller.rb b/app/controllers/admin/two_factor_authentications_controller.rb index 0652c3a7a..f7fb7eb8f 100644 --- a/app/controllers/admin/two_factor_authentications_controller.rb +++ b/app/controllers/admin/two_factor_authentications_controller.rb @@ -9,7 +9,7 @@ module Admin @user.disable_two_factor! log_action :disable_2fa, @user UserMailer.two_factor_disabled(@user).deliver_later! - redirect_to admin_accounts_path + redirect_to admin_account_path(@user.account_id) end private diff --git a/app/models/user.rb b/app/models/user.rb index 4973c68b6..4059c96b5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -42,6 +42,7 @@ # sign_in_token_sent_at :datetime # webauthn_id :string # sign_up_ip :inet +# skip_sign_in_token :boolean # class User < ApplicationRecord @@ -200,7 +201,7 @@ class User < ApplicationRecord end def suspicious_sign_in?(ip) - !otp_required_for_login? && current_sign_in_at.present? && current_sign_in_at < 2.weeks.ago && !recent_ip?(ip) + !otp_required_for_login? && !skip_sign_in_token? && current_sign_in_at.present? && !recent_ip?(ip) end def functional? @@ -329,12 +330,32 @@ class User < ApplicationRecord super end - def reset_password!(new_password, new_password_confirmation) + def reset_password(new_password, new_password_confirmation) return false if encrypted_password.blank? super end + def reset_password! + # First, change password to something random, invalidate the remember-me token, + # and deactivate all sessions + transaction do + update(remember_token: nil, remember_created_at: nil, password: SecureRandom.hex) + session_activations.destroy_all + end + + # Then, remove all authorized applications and connected push subscriptions + Doorkeeper::AccessGrant.by_resource_owner(self).in_batches.update_all(revoked_at: Time.now.utc) + + Doorkeeper::AccessToken.by_resource_owner(self).in_batches do |batch| + batch.update_all(revoked_at: Time.now.utc) + Web::PushSubscription.where(access_token_id: batch).delete_all + end + + # Finally, send a reset password prompt to the user + send_reset_password_instructions + end + def show_all_media? setting_display_media == 'show_all' end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index d832bff75..6695a0ddf 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -13,6 +13,14 @@ class UserPolicy < ApplicationPolicy admin? && !record.staff? end + def disable_sign_in_token_auth? + staff? + end + + def enable_sign_in_token_auth? + staff? + end + def confirm? staff? && !record.confirmed? end diff --git a/app/views/admin/accounts/show.html.haml b/app/views/admin/accounts/show.html.haml index 27e1f80a7..66eb49342 100644 --- a/app/views/admin/accounts/show.html.haml +++ b/app/views/admin/accounts/show.html.haml @@ -129,6 +129,27 @@ - else = t('admin.accounts.confirming') %td= table_link_to 'refresh', t('admin.accounts.resend_confirmation.send'), resend_admin_account_confirmation_path(@account.id), method: :post if can?(:confirm, @account.user) + %tr + %th{ rowspan: can?(:reset_password, @account.user) ? 2 : 1 }= t('admin.accounts.security') + %td{ rowspan: can?(:reset_password, @account.user) ? 2 : 1 } + - if @account.user&.two_factor_enabled? + = t 'admin.accounts.security_measures.password_and_2fa' + - elsif @account.user&.skip_sign_in_token? + = t 'admin.accounts.security_measures.only_password' + - else + = t 'admin.accounts.security_measures.password_and_sign_in_token' + %td + - if @account.user&.two_factor_enabled? + = table_link_to 'unlock', t('admin.accounts.disable_two_factor_authentication'), admin_user_two_factor_authentication_path(@account.user.id), method: :delete if can?(:disable_2fa, @account.user) + - elsif @account.user&.skip_sign_in_token? + = table_link_to 'lock', t('admin.accounts.enable_sign_in_token_auth'), admin_user_sign_in_token_authentication_path(@account.user.id), method: :post if can?(:enable_sign_in_token_auth, @account.user) + - else + = table_link_to 'unlock', t('admin.accounts.disable_sign_in_token_auth'), admin_user_sign_in_token_authentication_path(@account.user.id), method: :delete if can?(:disable_sign_in_token_auth, @account.user) + + - if can?(:reset_password, @account.user) + %tr + %td + = table_link_to 'key', t('admin.accounts.reset_password'), admin_account_reset_path(@account.id), method: :create, data: { confirm: t('admin.accounts.are_you_sure') } %tr %th= t('simple_form.labels.defaults.locale') @@ -221,9 +242,6 @@ %div - if @account.local? - = link_to t('admin.accounts.reset_password'), admin_account_reset_path(@account.id), method: :create, class: 'button' if can?(:reset_password, @account.user) - - if @account.user&.otp_required_for_login? - = link_to t('admin.accounts.disable_two_factor_authentication'), admin_user_two_factor_authentication_path(@account.user.id), method: :delete, class: 'button' if can?(:disable_2fa, @account.user) - if !@account.memorial? && @account.user_approved? = link_to t('admin.accounts.memorialize'), memorialize_admin_account_path(@account.id), method: :post, data: { confirm: t('admin.accounts.are_you_sure') }, class: 'button button--destructive' if can?(:memorialize, @account) - else diff --git a/config/locales/en.yml b/config/locales/en.yml index cdb2e3df7..51764a0e1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -44,7 +44,7 @@ en: rejecting_media: 'Media files from these servers will not be processed or stored, and no thumbnails will be displayed, requiring manual click-through to the original file:' rejecting_media_title: Filtered media silenced: 'Posts from these servers will be hidden in public timelines and conversations, and no notifications will be generated from their users interactions, unless you are following them:' - silenced_title: Silenced servers + silenced_title: Limited servers suspended: 'No data from these servers will be processed, stored or exchanged, making any interaction or communication with users from these servers impossible:' suspended_title: Suspended servers unavailable_content_html: Mastodon generally allows you to view content from and interact with users from any other server in the fediverse. These are the exceptions that have been made on this particular server. @@ -119,6 +119,7 @@ en: demote: Demote destroyed_msg: "%{username}'s data is now queued to be deleted imminently" disable: Freeze + disable_sign_in_token_auth: Disable e-mail token authentication disable_two_factor_authentication: Disable 2FA disabled: Frozen display_name: Display name @@ -127,6 +128,7 @@ en: email: Email email_status: Email status enable: Unfreeze + enable_sign_in_token_auth: Enable e-mail token authentication enabled: Enabled enabled_msg: Successfully unfroze %{username}'s account followers: Followers @@ -151,7 +153,7 @@ en: active: Active all: All pending: Pending - silenced: Silenced + silenced: Limited suspended: Suspended title: Moderation moderation_notes: Moderation notes @@ -191,8 +193,12 @@ en: search: Search search_same_email_domain: Other users with the same e-mail domain search_same_ip: Other users with the same IP - sensitive: Sensitive - sensitized: marked as sensitive + security_measures: + only_password: Only password + password_and_2fa: Password and 2FA + password_and_sign_in_token: Password and e-mail token + sensitive: Force-sensitive + sensitized: Marked as sensitive shared_inbox_url: Shared inbox URL show: created_reports: Made reports @@ -207,10 +213,10 @@ en: time_in_queue: Waiting in queue %{time} title: Accounts unconfirmed_email: Unconfirmed email - undo_sensitized: Undo sensitive - undo_silenced: Undo silence + undo_sensitized: Undo force-sensitive + undo_silenced: Undo limit undo_suspension: Undo suspension - unsilenced_msg: Successfully unlimited %{username}'s account + unsilenced_msg: Successfully undid limit of %{username}'s account unsubscribe: Unsubscribe unsuspended_msg: Successfully unsuspended %{username}'s account username: Username @@ -236,14 +242,16 @@ en: destroy_custom_emoji: Delete Custom Emoji destroy_domain_allow: Delete Domain Allow destroy_domain_block: Delete Domain Block - destroy_email_domain_block: Delete e-mail domain block + destroy_email_domain_block: Delete E-mail Domain Block destroy_ip_block: Delete IP rule destroy_status: Delete Post destroy_unavailable_domain: Delete Unavailable Domain disable_2fa_user: Disable 2FA disable_custom_emoji: Disable Custom Emoji + disable_sign_in_token_auth_user: Disable E-mail Token Authentication for User disable_user: Disable User enable_custom_emoji: Enable Custom Emoji + enable_sign_in_token_auth_user: Enable E-mail Token Authentication for User enable_user: Enable User memorialize_account: Memorialize Account promote_user: Promote User @@ -251,12 +259,12 @@ en: reopen_report: Reopen Report reset_password_user: Reset Password resolve_report: Resolve Report - sensitive_account: Mark the media in your account as sensitive - silence_account: Silence Account + sensitive_account: Force-Sensitive Account + silence_account: Limit Account suspend_account: Suspend Account unassigned_report: Unassign Report - unsensitive_account: Unmark the media in your account as sensitive - unsilence_account: Unsilence Account + unsensitive_account: Undo Force-Sensitive Account + unsilence_account: Undo Limit Account unsuspend_account: Unsuspend Account update_announcement: Update Announcement update_custom_emoji: Update Custom Emoji @@ -285,8 +293,10 @@ en: destroy_unavailable_domain_html: "%{name} resumed delivery to domain %{target}" disable_2fa_user_html: "%{name} disabled two factor requirement for user %{target}" disable_custom_emoji_html: "%{name} disabled emoji %{target}" + disable_sign_in_token_auth_user_html: "%{name} disabled e-mail token authentication for %{target}" disable_user_html: "%{name} disabled login for user %{target}" enable_custom_emoji_html: "%{name} enabled emoji %{target}" + enable_sign_in_token_auth_user_html: "%{name} enabled e-mail token authentication for %{target}" enable_user_html: "%{name} enabled login for user %{target}" memorialize_account_html: "%{name} turned %{target}'s account into a memoriam page" promote_user_html: "%{name} promoted user %{target}" @@ -295,11 +305,11 @@ en: reset_password_user_html: "%{name} reset password of user %{target}" resolve_report_html: "%{name} resolved report %{target}" sensitive_account_html: "%{name} marked %{target}'s media as sensitive" - silence_account_html: "%{name} silenced %{target}'s account" + silence_account_html: "%{name} limited %{target}'s account" suspend_account_html: "%{name} suspended %{target}'s account" unassigned_report_html: "%{name} unassigned report %{target}" unsensitive_account_html: "%{name} unmarked %{target}'s media as sensitive" - unsilence_account_html: "%{name} unsilenced %{target}'s account" + unsilence_account_html: "%{name} undid limit of %{target}'s account" unsuspend_account_html: "%{name} unsuspended %{target}'s account" update_announcement_html: "%{name} updated announcement %{target}" update_custom_emoji_html: "%{name} updated emoji %{target}" @@ -421,14 +431,14 @@ en: rejecting_media: rejecting media files rejecting_reports: rejecting reports severity: - silence: silenced + silence: limited suspend: suspended show: affected_accounts: one: One account in the database affected other: "%{count} accounts in the database affected" retroactive: - silence: Unsilence existing affected accounts from this domain + silence: Undo limit of existing affected accounts from this domain suspend: Unsuspend existing affected accounts from this domain title: Undo domain block for %{domain} undo: Undo diff --git a/config/routes.rb b/config/routes.rb index eb618324a..0c4b29546 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -283,6 +283,7 @@ Rails.application.routes.draw do resources :users, only: [] do resource :two_factor_authentication, only: [:destroy] + resource :sign_in_token_authentication, only: [:create, :destroy] end resources :custom_emojis, only: [:index, :new, :create] do diff --git a/db/migrate/20210621221010_add_skip_sign_in_token_to_users.rb b/db/migrate/20210621221010_add_skip_sign_in_token_to_users.rb new file mode 100644 index 000000000..43ad9b954 --- /dev/null +++ b/db/migrate/20210621221010_add_skip_sign_in_token_to_users.rb @@ -0,0 +1,5 @@ +class AddSkipSignInTokenToUsers < ActiveRecord::Migration[6.1] + def change + add_column :users, :skip_sign_in_token, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 935e2a564..b2929f693 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -927,6 +927,7 @@ ActiveRecord::Schema.define(version: 2021_06_30_000137) do t.datetime "sign_in_token_sent_at" t.string "webauthn_id" t.inet "sign_up_ip" + t.boolean "skip_sign_in_token" t.index ["account_id"], name: "index_users_on_account_id" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["created_by_application_id"], name: "index_users_on_created_by_application_id" diff --git a/lib/mastodon/accounts_cli.rb b/lib/mastodon/accounts_cli.rb index 74162256f..050194801 100644 --- a/lib/mastodon/accounts_cli.rb +++ b/lib/mastodon/accounts_cli.rb @@ -54,7 +54,8 @@ module Mastodon option :email, required: true option :confirmed, type: :boolean - option :role, default: 'user' + option :role, default: 'user', enum: %w(user moderator admin) + option :skip_sign_in_token, type: :boolean option :reattach, type: :boolean option :force, type: :boolean desc 'create USERNAME', 'Create a new user' @@ -68,6 +69,9 @@ module Mastodon With the --role option one of "user", "admin" or "moderator" can be supplied. Defaults to "user" + With the --skip-sign-in-token option, you can ensure that + the user is never asked for an e-mailed security code. + With the --reattach option, the new user will be reattached to a given existing username of an old account. If the old account is still in use by someone else, you can supply @@ -77,7 +81,7 @@ module Mastodon def create(username) account = Account.new(username: username) password = SecureRandom.hex - user = User.new(email: options[:email], password: password, agreement: true, approved: true, admin: options[:role] == 'admin', moderator: options[:role] == 'moderator', confirmed_at: options[:confirmed] ? Time.now.utc : nil, bypass_invite_request_check: true) + user = User.new(email: options[:email], password: password, agreement: true, approved: true, admin: options[:role] == 'admin', moderator: options[:role] == 'moderator', confirmed_at: options[:confirmed] ? Time.now.utc : nil, bypass_invite_request_check: true, skip_sign_in_token: options[:skip_sign_in_token]) if options[:reattach] account = Account.find_local(username) || Account.new(username: username) @@ -113,7 +117,7 @@ module Mastodon end end - option :role + option :role, enum: %w(user moderator admin) option :email option :confirm, type: :boolean option :enable, type: :boolean @@ -121,6 +125,7 @@ module Mastodon option :disable_2fa, type: :boolean option :approve, type: :boolean option :reset_password, type: :boolean + option :skip_sign_in_token, type: :boolean desc 'modify USERNAME', 'Modify a user' long_desc <<-LONG_DESC Modify a user account. @@ -142,6 +147,9 @@ module Mastodon With the --reset-password option, the user's password is replaced by a randomly-generated one, printed in the output. + + With the --skip-sign-in-token option, you can ensure that + the user is never asked for an e-mailed security code. LONG_DESC def modify(username) user = Account.find_local(username)&.user @@ -163,6 +171,7 @@ module Mastodon user.disabled = true if options[:disable] user.approved = true if options[:approve] user.otp_required_for_login = false if options[:disable_2fa] + user.skip_sign_in_token = options[:skip_sign_in_token] unless options[:skip_sign_in_token].nil? user.confirm if options[:confirm] if user.save diff --git a/spec/controllers/admin/resets_controller_spec.rb b/spec/controllers/admin/resets_controller_spec.rb index a20a460bd..c1e34b7f9 100644 --- a/spec/controllers/admin/resets_controller_spec.rb +++ b/spec/controllers/admin/resets_controller_spec.rb @@ -16,7 +16,7 @@ describe Admin::ResetsController do post :create, params: { account_id: account.id } - expect(response).to redirect_to(admin_accounts_path) + expect(response).to redirect_to(admin_account_path(account.id)) end end end diff --git a/spec/controllers/admin/two_factor_authentications_controller_spec.rb b/spec/controllers/admin/two_factor_authentications_controller_spec.rb index b0e82d3d6..c65095729 100644 --- a/spec/controllers/admin/two_factor_authentications_controller_spec.rb +++ b/spec/controllers/admin/two_factor_authentications_controller_spec.rb @@ -15,12 +15,12 @@ describe Admin::TwoFactorAuthenticationsController do user.update(otp_required_for_login: true) end - it 'redirects to admin accounts page' do + it 'redirects to admin account page' do delete :destroy, params: { user_id: user.id } user.reload expect(user.otp_enabled?).to eq false - expect(response).to redirect_to(admin_accounts_path) + expect(response).to redirect_to(admin_account_path(user.account_id)) end end @@ -38,13 +38,13 @@ describe Admin::TwoFactorAuthenticationsController do nickname: 'Security Key') end - it 'redirects to admin accounts page' do + it 'redirects to admin account page' do delete :destroy, params: { user_id: user.id } user.reload expect(user.otp_enabled?).to eq false expect(user.webauthn_enabled?).to eq false - expect(response).to redirect_to(admin_accounts_path) + expect(response).to redirect_to(admin_account_path(user.account_id)) end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5db249be2..54bb6db7f 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -344,6 +344,34 @@ RSpec.describe User, type: :model do end end + describe '#reset_password!' do + subject(:user) { Fabricate(:user, password: 'foobar12345') } + + let!(:session_activation) { Fabricate(:session_activation, user: user) } + let!(:access_token) { Fabricate(:access_token, resource_owner_id: user.id) } + let!(:web_push_subscription) { Fabricate(:web_push_subscription, access_token: access_token) } + + before do + user.reset_password! + end + + it 'changes the password immediately' do + expect(user.external_or_valid_password?('foobar12345')).to be false + end + + it 'deactivates all sessions' do + expect(user.session_activations.count).to eq 0 + end + + it 'revokes all access tokens' do + expect(Doorkeeper::AccessToken.active_for(user).count).to eq 0 + end + + it 'removes push subscriptions' do + expect(Web::PushSubscription.where(user: user).or(Web::PushSubscription.where(access_token: access_token)).count).to eq 0 + end + end + describe '#confirm!' do subject(:user) { Fabricate(:user, confirmed_at: confirmed_at) } -- cgit From 30ce6e395c5094c3989288619a99f539bb61f05d Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 14 Jul 2021 05:35:49 +0200 Subject: Fix user email address being banned on self-deletion (#16503) * Add tests * Fix user email address being banned on self-deletion Fixes #16498 --- app/controllers/settings/deletes_controller.rb | 2 +- app/models/account.rb | 4 ++-- spec/controllers/settings/deletes_controller_spec.rb | 4 ++++ 3 files changed, 7 insertions(+), 3 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/settings/deletes_controller.rb b/app/controllers/settings/deletes_controller.rb index 7b8f8d207..e0dd5edcb 100644 --- a/app/controllers/settings/deletes_controller.rb +++ b/app/controllers/settings/deletes_controller.rb @@ -42,7 +42,7 @@ class Settings::DeletesController < Settings::BaseController end def destroy_account! - current_account.suspend!(origin: :local) + current_account.suspend!(origin: :local, block_email: false) AccountDeletionWorker.perform_async(current_user.account_id) sign_out end diff --git a/app/models/account.rb b/app/models/account.rb index 8be36bf5b..a6d8d1537 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -232,11 +232,11 @@ class Account < ApplicationRecord suspended? && deletion_request.present? end - def suspend!(date: Time.now.utc, origin: :local) + def suspend!(date: Time.now.utc, origin: :local, block_email: true) transaction do create_deletion_request! update!(suspended_at: date, suspension_origin: origin) - create_canonical_email_block! + create_canonical_email_block! if block_email end end diff --git a/spec/controllers/settings/deletes_controller_spec.rb b/spec/controllers/settings/deletes_controller_spec.rb index 8d5c4774f..92ab401c9 100644 --- a/spec/controllers/settings/deletes_controller_spec.rb +++ b/spec/controllers/settings/deletes_controller_spec.rb @@ -59,6 +59,10 @@ describe Settings::DeletesController do expect(user.account.reload).to be_suspended end + it 'does not create an email block' do + expect(CanonicalEmailBlock.block?(user.email)).to be false + end + context 'when suspended' do let(:user) { Fabricate(:user, account_attributes: { username: 'alice', suspended_at: Time.now.utc }) } -- cgit From d8629e7b86b6fcaedc8b31de57dd95e85fe4fb04 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 21 Jul 2021 18:34:39 +0200 Subject: Add logging of S3-related errors (#16381) --- app/controllers/api/base_controller.rb | 7 ++++++- app/controllers/application_controller.rb | 7 ++++++- app/lib/activitypub/activity/create.rb | 8 ++++---- app/services/backup_service.rb | 4 ++-- 4 files changed, 18 insertions(+), 8 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index 85f4cc768..b863d8643 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -40,7 +40,12 @@ class Api::BaseController < ApplicationController render json: { error: 'This action is not allowed' }, status: 403 end - rescue_from Mastodon::RaceConditionError, Seahorse::Client::NetworkingError, Stoplight::Error::RedLight do + rescue_from Seahorse::Client::NetworkingError do |e| + Rails.logger.warn "Storage server error: #{e}" + render json: { error: 'There was a temporary problem serving your request, please try again' }, status: 503 + end + + rescue_from Mastodon::RaceConditionError, Stoplight::Error::RedLight do render json: { error: 'There was a temporary problem serving your request, please try again' }, status: 503 end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b4fb83661..3d2f8280b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -26,7 +26,12 @@ class ApplicationController < ActionController::Base rescue_from Mastodon::RateLimitExceededError, with: :too_many_requests rescue_from HTTP::Error, OpenSSL::SSL::SSLError, with: :internal_server_error - rescue_from Mastodon::RaceConditionError, Seahorse::Client::NetworkingError, Stoplight::Error::RedLight, ActiveRecord::SerializationFailure, with: :service_unavailable + rescue_from Mastodon::RaceConditionError, Stoplight::Error::RedLight, ActiveRecord::SerializationFailure, with: :service_unavailable + + rescue_from Seahorse::Client::NetworkingError do |e| + Rails.logger.warn "Storage server error: #{e}" + service_unavailable + end before_action :store_current_location, except: :raise_not_found, unless: :devise_controller? before_action :require_functional!, if: :user_signed_in? diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index 9a2960507..504f10a67 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -223,8 +223,8 @@ class ActivityPub::Activity::Create < ActivityPub::Activity emoji ||= CustomEmoji.new(domain: @account.domain, shortcode: shortcode, uri: uri) emoji.image_remote_url = image_url emoji.save - rescue Seahorse::Client::NetworkingError - nil + rescue Seahorse::Client::NetworkingError => e + Rails.logger.warn "Error storing emoji: #{e}" end def process_attachments @@ -247,8 +247,8 @@ class ActivityPub::Activity::Create < ActivityPub::Activity media_attachment.save rescue Mastodon::UnexpectedResponseError, HTTP::TimeoutError, HTTP::ConnectionError, OpenSSL::SSL::SSLError RedownloadMediaWorker.perform_in(rand(30..600).seconds, media_attachment.id) - rescue Seahorse::Client::NetworkingError - nil + rescue Seahorse::Client::NetworkingError => e + Rails.logger.warn "Error storing media attachment: #{e}" end end diff --git a/app/services/backup_service.rb b/app/services/backup_service.rb index 6a1575616..037f519d3 100644 --- a/app/services/backup_service.rb +++ b/app/services/backup_service.rb @@ -167,7 +167,7 @@ class BackupService < BaseService io.write(buffer) end end - rescue Errno::ENOENT, Seahorse::Client::NetworkingError - Rails.logger.warn "Could not backup file #{filename}: file not found" + rescue Errno::ENOENT, Seahorse::Client::NetworkingError => e + Rails.logger.warn "Could not backup file #{filename}: #{e}" end end -- cgit From 4ac78e2a066508a54de82f1d910ef2fd36c3d106 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 9 Aug 2021 23:11:50 +0200 Subject: Add feature to automatically delete old toots (#16529) * Add account statuses cleanup policy model * Record last inspected toot to delete to speed up successive calls to statuses_to_delete * Add service to cleanup a given account's statuses within a budget * Add worker to go through account policies and delete old toots * Fix last inspected status id logic All existing statuses older or equal to last inspected status id must be kept by the current policy. This is an invariant that must be kept so that resuming deletion from the last inspected status remains sound. * Add tests * Refactor scheduler and add tests * Add user interface * Add support for discriminating based on boosts/favs * Add UI support for min_reblogs and min_favs, rework UI * Address first round of review comments * Replace Snowflake#id_at_start with with_random parameter * Add tests * Add tests for StatusesCleanupController * Rework settings page * Adjust load-avoiding mechanisms * Please CodeClimate --- app/controllers/statuses_cleanup_controller.rb | 35 ++ app/models/account_statuses_cleanup_policy.rb | 171 +++++++ app/models/bookmark.rb | 8 + app/models/concerns/account_associations.rb | 3 + app/models/favourite.rb | 7 + app/models/status_pin.rb | 8 + app/services/account_statuses_cleanup_service.rb | 27 + app/views/statuses_cleanup/show.html.haml | 45 ++ .../accounts_statuses_cleanup_scheduler.rb | 96 ++++ config/locales/en.yml | 35 ++ config/navigation.rb | 1 + config/routes.rb | 1 + config/sidekiq.yml | 4 + ...340_create_account_statuses_cleanup_policies.rb | 20 + db/schema.rb | 18 + lib/mastodon/snowflake.rb | 7 +- .../statuses_cleanup_controller_spec.rb | 27 + .../account_statuses_cleanup_policy_fabricator.rb | 3 + .../models/account_statuses_cleanup_policy_spec.rb | 546 +++++++++++++++++++++ .../account_statuses_cleanup_service_spec.rb | 101 ++++ .../accounts_statuses_cleanup_scheduler_spec.rb | 127 +++++ 21 files changed, 1287 insertions(+), 3 deletions(-) create mode 100644 app/controllers/statuses_cleanup_controller.rb create mode 100644 app/models/account_statuses_cleanup_policy.rb create mode 100644 app/services/account_statuses_cleanup_service.rb create mode 100644 app/views/statuses_cleanup/show.html.haml create mode 100644 app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb create mode 100644 db/migrate/20210722120340_create_account_statuses_cleanup_policies.rb create mode 100644 spec/controllers/statuses_cleanup_controller_spec.rb create mode 100644 spec/fabricators/account_statuses_cleanup_policy_fabricator.rb create mode 100644 spec/models/account_statuses_cleanup_policy_spec.rb create mode 100644 spec/services/account_statuses_cleanup_service_spec.rb create mode 100644 spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb (limited to 'app/controllers') diff --git a/app/controllers/statuses_cleanup_controller.rb b/app/controllers/statuses_cleanup_controller.rb new file mode 100644 index 000000000..be234cdcb --- /dev/null +++ b/app/controllers/statuses_cleanup_controller.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class StatusesCleanupController < ApplicationController + layout 'admin' + + before_action :authenticate_user! + before_action :set_policy + before_action :set_body_classes + + def show; end + + def update + if @policy.update(resource_params) + redirect_to statuses_cleanup_path, notice: I18n.t('generic.changes_saved_msg') + else + render action: :show + end + rescue ActionController::ParameterMissing + # Do nothing + end + + private + + def set_policy + @policy = current_account.statuses_cleanup_policy || current_account.build_statuses_cleanup_policy(enabled: false) + end + + def resource_params + params.require(:account_statuses_cleanup_policy).permit(:enabled, :min_status_age, :keep_direct, :keep_pinned, :keep_polls, :keep_media, :keep_self_fav, :keep_self_bookmark, :min_favs, :min_reblogs) + end + + def set_body_classes + @body_classes = 'admin' + end +end diff --git a/app/models/account_statuses_cleanup_policy.rb b/app/models/account_statuses_cleanup_policy.rb new file mode 100644 index 000000000..705ccff54 --- /dev/null +++ b/app/models/account_statuses_cleanup_policy.rb @@ -0,0 +1,171 @@ +# frozen_string_literal: true + +# == Schema Information +# +# Table name: account_statuses_cleanup_policies +# +# id :bigint not null, primary key +# account_id :bigint not null +# enabled :boolean default(TRUE), not null +# min_status_age :integer default(1209600), not null +# keep_direct :boolean default(TRUE), not null +# keep_pinned :boolean default(TRUE), not null +# keep_polls :boolean default(FALSE), not null +# keep_media :boolean default(FALSE), not null +# keep_self_fav :boolean default(TRUE), not null +# keep_self_bookmark :boolean default(TRUE), not null +# min_favs :integer +# min_reblogs :integer +# created_at :datetime not null +# updated_at :datetime not null +# +class AccountStatusesCleanupPolicy < ApplicationRecord + include Redisable + + ALLOWED_MIN_STATUS_AGE = [ + 2.weeks.seconds, + 1.month.seconds, + 2.months.seconds, + 3.months.seconds, + 6.months.seconds, + 1.year.seconds, + 2.years.seconds, + ].freeze + + EXCEPTION_BOOLS = %w(keep_direct keep_pinned keep_polls keep_media keep_self_fav keep_self_bookmark).freeze + EXCEPTION_THRESHOLDS = %w(min_favs min_reblogs).freeze + + # Depending on the cleanup policy, the query to discover the next + # statuses to delete my get expensive if the account has a lot of old + # statuses otherwise excluded from deletion by the other exceptions. + # + # Therefore, `EARLY_SEARCH_CUTOFF` is meant to be the maximum number of + # old statuses to be considered for deletion prior to checking exceptions. + # + # This is used in `compute_cutoff_id` to provide a `max_id` to + # `statuses_to_delete`. + EARLY_SEARCH_CUTOFF = 5_000 + + belongs_to :account + + validates :min_status_age, inclusion: { in: ALLOWED_MIN_STATUS_AGE } + validates :min_favs, numericality: { greater_than_or_equal_to: 1, allow_nil: true } + validates :min_reblogs, numericality: { greater_than_or_equal_to: 1, allow_nil: true } + validate :validate_local_account + + before_save :update_last_inspected + + def statuses_to_delete(limit = 50, max_id = nil, min_id = nil) + scope = account.statuses + scope.merge!(old_enough_scope(max_id)) + scope = scope.where(Status.arel_table[:id].gteq(min_id)) if min_id.present? + scope.merge!(without_popular_scope) unless min_favs.nil? && min_reblogs.nil? + scope.merge!(without_direct_scope) if keep_direct? + scope.merge!(without_pinned_scope) if keep_pinned? + scope.merge!(without_poll_scope) if keep_polls? + scope.merge!(without_media_scope) if keep_media? + scope.merge!(without_self_fav_scope) if keep_self_fav? + scope.merge!(without_self_bookmark_scope) if keep_self_bookmark? + + scope.reorder(id: :asc).limit(limit) + end + + # This computes a toot id such that: + # - the toot would be old enough to be candidate for deletion + # - there are at most EARLY_SEARCH_CUTOFF toots between the last inspected toot and this one + # + # The idea is to limit expensive SQL queries when an account has lots of toots excluded from + # deletion, while not starting anew on each run. + def compute_cutoff_id + min_id = last_inspected || 0 + max_id = Mastodon::Snowflake.id_at(min_status_age.seconds.ago, with_random: false) + subquery = account.statuses.where(Status.arel_table[:id].gteq(min_id)).where(Status.arel_table[:id].lteq(max_id)) + subquery = subquery.select(:id).reorder(id: :asc).limit(EARLY_SEARCH_CUTOFF) + + # We're textually interpolating a subquery here as ActiveRecord seem to not provide + # a way to apply the limit to the subquery + Status.connection.execute("SELECT MAX(id) FROM (#{subquery.to_sql}) t").values.first.first + end + + # The most important thing about `last_inspected` is that any toot older than it is guaranteed + # not to be kept by the policy regardless of its age. + def record_last_inspected(last_id) + redis.set("account_cleanup:#{account.id}", last_id, ex: 1.week.seconds) + end + + def last_inspected + redis.get("account_cleanup:#{account.id}")&.to_i + end + + def invalidate_last_inspected(status, action) + last_value = last_inspected + return if last_value.nil? || status.id > last_value || status.account_id != account_id + + case action + when :unbookmark + return unless keep_self_bookmark? + when :unfav + return unless keep_self_fav? + when :unpin + return unless keep_pinned? + end + + record_last_inspected(status.id) + end + + private + + def update_last_inspected + if EXCEPTION_BOOLS.map { |name| attribute_change_to_be_saved(name) }.compact.include?([true, false]) + # Policy has been widened in such a way that any previously-inspected status + # may need to be deleted, so we'll have to start again. + redis.del("account_cleanup:#{account.id}") + end + if EXCEPTION_THRESHOLDS.map { |name| attribute_change_to_be_saved(name) }.compact.any? { |old, new| old.present? && (new.nil? || new > old) } + redis.del("account_cleanup:#{account.id}") + end + end + + def validate_local_account + errors.add(:account, :invalid) unless account&.local? + end + + def without_direct_scope + Status.where.not(visibility: :direct) + end + + def old_enough_scope(max_id = nil) + # Filtering on `id` rather than `min_status_age` ago will treat + # non-snowflake statuses as older than they really are, but Mastodon + # has switched to snowflake IDs significantly over 2 years ago anyway. + max_id = [max_id, Mastodon::Snowflake.id_at(min_status_age.seconds.ago, with_random: false)].compact.min + Status.where(Status.arel_table[:id].lteq(max_id)) + end + + def without_self_fav_scope + Status.where('NOT EXISTS (SELECT * FROM favourites fav WHERE fav.account_id = statuses.account_id AND fav.status_id = statuses.id)') + end + + def without_self_bookmark_scope + Status.where('NOT EXISTS (SELECT * FROM bookmarks bookmark WHERE bookmark.account_id = statuses.account_id AND bookmark.status_id = statuses.id)') + end + + def without_pinned_scope + Status.where('NOT EXISTS (SELECT * FROM status_pins pin WHERE pin.account_id = statuses.account_id AND pin.status_id = statuses.id)') + end + + def without_media_scope + Status.where('NOT EXISTS (SELECT * FROM media_attachments media WHERE media.status_id = statuses.id)') + end + + def without_poll_scope + Status.where(poll_id: nil) + end + + def without_popular_scope + scope = Status.left_joins(:status_stat) + scope = scope.where('COALESCE(status_stats.reblogs_count, 0) <= ?', min_reblogs) unless min_reblogs.nil? + scope = scope.where('COALESCE(status_stats.favourites_count, 0) <= ?', min_favs) unless min_favs.nil? + scope + end +end diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 916261a17..f21ea714c 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -23,4 +23,12 @@ class Bookmark < ApplicationRecord before_validation do self.status = status.reblog if status&.reblog? end + + after_destroy :invalidate_cleanup_info + + def invalidate_cleanup_info + return unless status&.account_id == account_id && account.local? + + account.statuses_cleanup_policy&.invalidate_last_inspected(status, :unbookmark) + end end diff --git a/app/models/concerns/account_associations.rb b/app/models/concerns/account_associations.rb index aaf371ebd..f2a4eae77 100644 --- a/app/models/concerns/account_associations.rb +++ b/app/models/concerns/account_associations.rb @@ -66,5 +66,8 @@ module AccountAssociations # Follow recommendations has_one :follow_recommendation_suppression, inverse_of: :account, dependent: :destroy + + # Account statuses cleanup policy + has_one :statuses_cleanup_policy, class_name: 'AccountStatusesCleanupPolicy', inverse_of: :account, dependent: :destroy end end diff --git a/app/models/favourite.rb b/app/models/favourite.rb index 35028b7dd..ca8bce146 100644 --- a/app/models/favourite.rb +++ b/app/models/favourite.rb @@ -28,6 +28,7 @@ class Favourite < ApplicationRecord after_create :increment_cache_counters after_destroy :decrement_cache_counters + after_destroy :invalidate_cleanup_info private @@ -39,4 +40,10 @@ class Favourite < ApplicationRecord return if association(:status).loaded? && status.marked_for_destruction? status&.decrement_count!(:favourites_count) end + + def invalidate_cleanup_info + return unless status&.account_id == account_id && account.local? + + account.statuses_cleanup_policy&.invalidate_last_inspected(status, :unfav) + end end diff --git a/app/models/status_pin.rb b/app/models/status_pin.rb index afc76bded..93a0ea1c0 100644 --- a/app/models/status_pin.rb +++ b/app/models/status_pin.rb @@ -15,4 +15,12 @@ class StatusPin < ApplicationRecord belongs_to :status validates_with StatusPinValidator + + after_destroy :invalidate_cleanup_info + + def invalidate_cleanup_info + return unless status&.account_id == account_id && account.local? + + account.statuses_cleanup_policy&.invalidate_last_inspected(status, :unpin) + end end diff --git a/app/services/account_statuses_cleanup_service.rb b/app/services/account_statuses_cleanup_service.rb new file mode 100644 index 000000000..cbadecc63 --- /dev/null +++ b/app/services/account_statuses_cleanup_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class AccountStatusesCleanupService < BaseService + # @param [AccountStatusesCleanupPolicy] account_policy + # @param [Integer] budget + # @return [Integer] + def call(account_policy, budget = 50) + return 0 unless account_policy.enabled? + + cutoff_id = account_policy.compute_cutoff_id + return 0 if cutoff_id.blank? + + num_deleted = 0 + last_deleted = nil + + account_policy.statuses_to_delete(budget, cutoff_id, account_policy.last_inspected).reorder(nil).find_each(order: :asc) do |status| + status.discard + RemovalWorker.perform_async(status.id, redraft: false) + num_deleted += 1 + last_deleted = status.id + end + + account_policy.record_last_inspected(last_deleted.presence || cutoff_id) + + num_deleted + end +end diff --git a/app/views/statuses_cleanup/show.html.haml b/app/views/statuses_cleanup/show.html.haml new file mode 100644 index 000000000..59de4b5aa --- /dev/null +++ b/app/views/statuses_cleanup/show.html.haml @@ -0,0 +1,45 @@ +- content_for :page_title do + = t('settings.statuses_cleanup') + +- content_for :heading_actions do + = button_tag t('generic.save_changes'), class: 'button', form: 'edit_policy' + += simple_form_for @policy, url: statuses_cleanup_path, method: :put, html: { id: 'edit_policy' } do |f| + + .fields-row + .fields-row__column.fields-row__column-6.fields-group + = f.input :enabled, as: :boolean, wrapper: :with_label, label: t('statuses_cleanup.enabled'), hint: t('statuses_cleanup.enabled_hint') + .fields-row__column.fields-row__column-6.fields-group + = f.input :min_status_age, wrapper: :with_label, label: t('statuses_cleanup.min_age_label'), collection: AccountStatusesCleanupPolicy::ALLOWED_MIN_STATUS_AGE.map(&:to_i), label_method: lambda { |i| t("statuses_cleanup.min_age.#{i}") }, include_blank: false, hint: false + + .flash-message= t('statuses_cleanup.explanation') + + %h4= t('statuses_cleanup.exceptions') + + .fields-row + .fields-row__column.fields-row__column-6.fields-group + = f.input :keep_pinned, wrapper: :with_label, label: t('statuses_cleanup.keep_pinned'), hint: t('statuses_cleanup.keep_pinned_hint') + .fields-row__column.fields-row__column-6.fields-group + = f.input :keep_direct, wrapper: :with_label, label: t('statuses_cleanup.keep_direct'), hint: t('statuses_cleanup.keep_direct_hint') + + .fields-row + .fields-row__column.fields-row__column-6.fields-group + = f.input :keep_self_fav, wrapper: :with_label, label: t('statuses_cleanup.keep_self_fav'), hint: t('statuses_cleanup.keep_self_fav_hint') + .fields-row__column.fields-row__column-6.fields-group + = f.input :keep_self_bookmark, wrapper: :with_label, label: t('statuses_cleanup.keep_self_bookmark'), hint: t('statuses_cleanup.keep_self_bookmark_hint') + + .fields-row + .fields-row__column.fields-row__column-6.fields-group + = f.input :keep_polls, wrapper: :with_label, label: t('statuses_cleanup.keep_polls'), hint: t('statuses_cleanup.keep_polls_hint') + .fields-row__column.fields-row__column-6.fields-group + = f.input :keep_media, wrapper: :with_label, label: t('statuses_cleanup.keep_media'), hint: t('statuses_cleanup.keep_media_hint') + + %h4= t('statuses_cleanup.interaction_exceptions') + + .fields-row + .fields-row__column.fields-row__column-6.fields-group + = f.input :min_favs, wrapper: :with_label, label: t('statuses_cleanup.min_favs'), hint: t('statuses_cleanup.min_favs_hint'), input_html: { min: 1, placeholder: t('statuses_cleanup.ignore_favs') } + .fields-row__column.fields-row__column-6.fields-group + = f.input :min_reblogs, wrapper: :with_label, label: t('statuses_cleanup.min_reblogs'), hint: t('statuses_cleanup.min_reblogs_hint'), input_html: { min: 1, placeholder: t('statuses_cleanup.ignore_reblogs') } + + .flash-message= t('statuses_cleanup.interaction_exceptions_explanation') diff --git a/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb b/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb new file mode 100644 index 000000000..f42d4bca6 --- /dev/null +++ b/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +class Scheduler::AccountsStatusesCleanupScheduler + include Sidekiq::Worker + + # This limit is mostly to be nice to the fediverse at large and not + # generate too much traffic. + # This also helps limiting the running time of the scheduler itself. + MAX_BUDGET = 50 + + # This is an attempt to spread the load across instances, as various + # accounts are likely to have various followers. + PER_ACCOUNT_BUDGET = 5 + + # This is an attempt to limit the workload generated by status removal + # jobs to something the particular instance can handle. + PER_THREAD_BUDGET = 5 + + # Those avoid loading an instance that is already under load + MAX_DEFAULT_SIZE = 2 + MAX_DEFAULT_LATENCY = 5 + MAX_PUSH_SIZE = 5 + MAX_PUSH_LATENCY = 10 + # 'pull' queue has lower priority jobs, and it's unlikely that pushing + # deletes would cause much issues with this queue if it didn't cause issues + # with default and push. Yet, do not enqueue deletes if the instance is + # lagging behind too much. + MAX_PULL_SIZE = 500 + MAX_PULL_LATENCY = 300 + + # This is less of an issue in general, but deleting old statuses is likely + # to cause delivery errors, and thus increase the number of jobs to be retried. + # This doesn't directly translate to load, but connection errors and a high + # number of dead instances may lead to this spiraling out of control if + # unchecked. + MAX_RETRY_SIZE = 50_000 + + sidekiq_options retry: 0, lock: :until_executed + + def perform + return if under_load? + + budget = compute_budget + first_policy_id = last_processed_id + + loop do + num_processed_accounts = 0 + + scope = AccountStatusesCleanupPolicy.where(enabled: true) + scope.where(Account.arel_table[:id].gt(first_policy_id)) if first_policy_id.present? + scope.find_each(order: :asc) do |policy| + num_deleted = AccountStatusesCleanupService.new.call(policy, [budget, PER_ACCOUNT_BUDGET].min) + num_processed_accounts += 1 unless num_deleted.zero? + budget -= num_deleted + if budget.zero? + save_last_processed_id(policy.id) + break + end + end + + # The idea here is to loop through all policies at least once until the budget is exhausted + # and start back after the last processed account otherwise + break if budget.zero? || (num_processed_accounts.zero? && first_policy_id.nil?) + first_policy_id = nil + end + end + + def compute_budget + threads = Sidekiq::ProcessSet.new.filter { |x| x['queues'].include?('push') }.map { |x| x['concurrency'] }.sum + [PER_THREAD_BUDGET * threads, MAX_BUDGET].min + end + + def under_load? + return true if Sidekiq::Stats.new.retry_size > MAX_RETRY_SIZE + queue_under_load?('default', MAX_DEFAULT_SIZE, MAX_DEFAULT_LATENCY) || queue_under_load?('push', MAX_PUSH_SIZE, MAX_PUSH_LATENCY) || queue_under_load?('pull', MAX_PULL_SIZE, MAX_PULL_LATENCY) + end + + private + + def queue_under_load?(name, max_size, max_latency) + queue = Sidekiq::Queue.new(name) + queue.size > max_size || queue.latency > max_latency + end + + def last_processed_id + Redis.current.get('account_statuses_cleanup_scheduler:last_account_id') + end + + def save_last_processed_id(id) + if id.nil? + Redis.current.del('account_statuses_cleanup_scheduler:last_account_id') + else + Redis.current.set('account_statuses_cleanup_scheduler:last_account_id', id, ex: 1.hour.seconds) + end + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index af7266d86..be6052948 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1254,6 +1254,7 @@ en: preferences: Preferences profile: Profile relationships: Follows and followers + statuses_cleanup: Automated post deletion two_factor_authentication: Two-factor Auth webauthn_authentication: Security keys statuses: @@ -1305,6 +1306,40 @@ en: public_long: Everyone can see unlisted: Unlisted unlisted_long: Everyone can see, but not listed on public timelines + statuses_cleanup: + enabled: Automatically delete old posts + enabled_hint: Automatically deletes your posts once they reach a specified age threshold, unless they match one of the exceptions below + exceptions: Exceptions + explanation: Because deleting posts is an expensive operation, this is done slowly over time when the server is not otherwise busy. For this reason, your posts may be deleted a while after they reach the age threshold. + ignore_favs: Ignore favourites + ignore_reblogs: Ignore boosts + interaction_exceptions: Exceptions based on interactions + interaction_exceptions_explanation: Note that there is no guarantee for posts to be deleted if they go below the favourite or boost threshold after having once gone over them. + keep_direct: Keep direct messages + keep_direct_hint: Doesn't delete any of your direct messages + keep_media: Keep posts with media attachments + keep_media_hint: Doesn't delete any of your posts that have media attachments + keep_pinned: Keep pinned posts + keep_pinned_hint: Doesn't delete any of your pinned posts + keep_polls: Keep polls + keep_polls_hint: Doesn't delete any of your polls + keep_self_bookmark: Keep posts you bookmarked + keep_self_bookmark_hint: Doesn't delete your own posts if you have bookmarked them + keep_self_fav: Keep posts you favourited + keep_self_fav_hint: Doesn't delete your own posts if you have favourited them + min_age: + '1209600': 2 weeks + '15778476': 6 months + '2629746': 1 month + '31556952': 1 year + '5259492': 2 months + '63113904': 2 years + '7889238': 3 months + min_age_label: Age threshold + min_favs: Keep posts favourited more than + min_favs_hint: Doesn't delete any of your posts that has received more than this amount of favourites. Leave blank to delete posts regardless of their number of favourites + min_reblogs: Keep posts boosted more than + min_reblogs_hint: Doesn't delete any of your posts that has been boosted more than this number of times. Leave blank to delete posts regardless of their number of boosts stream_entries: pinned: Pinned post reblogged: boosted diff --git a/config/navigation.rb b/config/navigation.rb index 5d1f55d74..37bfd7549 100644 --- a/config/navigation.rb +++ b/config/navigation.rb @@ -18,6 +18,7 @@ SimpleNavigation::Configuration.run do |navigation| n.item :relationships, safe_join([fa_icon('users fw'), t('settings.relationships')]), relationships_url, if: -> { current_user.functional? } n.item :filters, safe_join([fa_icon('filter fw'), t('filters.index.title')]), filters_path, highlights_on: %r{/filters}, if: -> { current_user.functional? } + n.item :statuses_cleanup, safe_join([fa_icon('history fw'), t('settings.statuses_cleanup')]), statuses_cleanup_url, if: -> { current_user.functional? } n.item :security, safe_join([fa_icon('lock fw'), t('settings.account')]), edit_user_registration_url do |s| s.item :password, safe_join([fa_icon('lock fw'), t('settings.account_settings')]), edit_user_registration_url, highlights_on: %r{/auth/edit|/settings/delete|/settings/migration|/settings/aliases|/settings/login_activities} diff --git a/config/routes.rb b/config/routes.rb index 0c4b29546..8abc438fe 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -176,6 +176,7 @@ Rails.application.routes.draw do resources :invites, only: [:index, :create, :destroy] resources :filters, except: [:show] resource :relationships, only: [:show, :update] + resource :statuses_cleanup, controller: :statuses_cleanup, only: [:show, :update] get '/public', to: 'public_timelines#show', as: :public_timeline get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy diff --git a/config/sidekiq.yml b/config/sidekiq.yml index a8e4c7feb..eab74338e 100644 --- a/config/sidekiq.yml +++ b/config/sidekiq.yml @@ -57,3 +57,7 @@ cron: '0 * * * *' class: Scheduler::InstanceRefreshScheduler queue: scheduler + accounts_statuses_cleanup_scheduler: + interval: 1 minute + class: Scheduler::AccountsStatusesCleanupScheduler + queue: scheduler diff --git a/db/migrate/20210722120340_create_account_statuses_cleanup_policies.rb b/db/migrate/20210722120340_create_account_statuses_cleanup_policies.rb new file mode 100644 index 000000000..28cfb6ef5 --- /dev/null +++ b/db/migrate/20210722120340_create_account_statuses_cleanup_policies.rb @@ -0,0 +1,20 @@ +class CreateAccountStatusesCleanupPolicies < ActiveRecord::Migration[6.1] + def change + create_table :account_statuses_cleanup_policies do |t| + t.belongs_to :account, null: false, foreign_key: { on_delete: :cascade } + t.boolean :enabled, null: false, default: true + t.integer :min_status_age, null: false, default: 2.weeks.seconds + t.boolean :keep_direct, null: false, default: true + t.boolean :keep_pinned, null: false, default: true + t.boolean :keep_polls, null: false, default: false + t.boolean :keep_media, null: false, default: false + t.boolean :keep_self_fav, null: false, default: true + t.boolean :keep_self_bookmark, null: false, default: true + t.integer :min_favs, null: true + t.integer :min_reblogs, null: true + + t.timestamps + end + end +end + diff --git a/db/schema.rb b/db/schema.rb index a0a98eb03..2376afff7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -114,6 +114,23 @@ ActiveRecord::Schema.define(version: 2021_08_08_071221) do t.index ["account_id"], name: "index_account_stats_on_account_id", unique: true end + create_table "account_statuses_cleanup_policies", force: :cascade do |t| + t.bigint "account_id", null: false + t.boolean "enabled", default: true, null: false + t.integer "min_status_age", default: 1209600, null: false + t.boolean "keep_direct", default: true, null: false + t.boolean "keep_pinned", default: true, null: false + t.boolean "keep_polls", default: false, null: false + t.boolean "keep_media", default: false, null: false + t.boolean "keep_self_fav", default: true, null: false + t.boolean "keep_self_bookmark", default: true, null: false + t.integer "min_favs" + t.integer "min_reblogs" + t.datetime "created_at", precision: 6, null: false + t.datetime "updated_at", precision: 6, null: false + t.index ["account_id"], name: "index_account_statuses_cleanup_policies_on_account_id" + end + create_table "account_warning_presets", force: :cascade do |t| t.text "text", default: "", null: false t.datetime "created_at", null: false @@ -984,6 +1001,7 @@ ActiveRecord::Schema.define(version: 2021_08_08_071221) do add_foreign_key "account_pins", "accounts", column: "target_account_id", on_delete: :cascade add_foreign_key "account_pins", "accounts", on_delete: :cascade add_foreign_key "account_stats", "accounts", on_delete: :cascade + add_foreign_key "account_statuses_cleanup_policies", "accounts", on_delete: :cascade add_foreign_key "account_warnings", "accounts", column: "target_account_id", on_delete: :cascade add_foreign_key "account_warnings", "accounts", on_delete: :nullify add_foreign_key "accounts", "accounts", column: "moved_to_account_id", on_delete: :nullify diff --git a/lib/mastodon/snowflake.rb b/lib/mastodon/snowflake.rb index 9e5bc7383..8e2d82a97 100644 --- a/lib/mastodon/snowflake.rb +++ b/lib/mastodon/snowflake.rb @@ -138,10 +138,11 @@ module Mastodon::Snowflake end end - def id_at(timestamp) - id = timestamp.to_i * 1000 + rand(1000) + def id_at(timestamp, with_random: true) + id = timestamp.to_i * 1000 + id += rand(1000) if with_random id = id << 16 - id += rand(2**16) + id += rand(2**16) if with_random id end diff --git a/spec/controllers/statuses_cleanup_controller_spec.rb b/spec/controllers/statuses_cleanup_controller_spec.rb new file mode 100644 index 000000000..924709260 --- /dev/null +++ b/spec/controllers/statuses_cleanup_controller_spec.rb @@ -0,0 +1,27 @@ +require 'rails_helper' + +RSpec.describe StatusesCleanupController, type: :controller do + render_views + + before do + @user = Fabricate(:user) + sign_in @user, scope: :user + end + + describe "GET #show" do + it "returns http success" do + get :show + expect(response).to have_http_status(200) + end + end + + describe 'PUT #update' do + it 'updates the account status cleanup policy' do + put :update, params: { account_statuses_cleanup_policy: { enabled: true, min_status_age: 2.weeks.seconds, keep_direct: false, keep_polls: true } } + expect(response).to redirect_to(statuses_cleanup_path) + expect(@user.account.statuses_cleanup_policy.enabled).to eq true + expect(@user.account.statuses_cleanup_policy.keep_direct).to eq false + expect(@user.account.statuses_cleanup_policy.keep_polls).to eq true + end + end +end diff --git a/spec/fabricators/account_statuses_cleanup_policy_fabricator.rb b/spec/fabricators/account_statuses_cleanup_policy_fabricator.rb new file mode 100644 index 000000000..29cf1d133 --- /dev/null +++ b/spec/fabricators/account_statuses_cleanup_policy_fabricator.rb @@ -0,0 +1,3 @@ +Fabricator(:account_statuses_cleanup_policy) do + account +end diff --git a/spec/models/account_statuses_cleanup_policy_spec.rb b/spec/models/account_statuses_cleanup_policy_spec.rb new file mode 100644 index 000000000..63e9c5d20 --- /dev/null +++ b/spec/models/account_statuses_cleanup_policy_spec.rb @@ -0,0 +1,546 @@ +require 'rails_helper' + +RSpec.describe AccountStatusesCleanupPolicy, type: :model do + let(:account) { Fabricate(:account, username: 'alice', domain: nil) } + + describe 'validation' do + it 'disallow remote accounts' do + account.update(domain: 'example.com') + account_statuses_cleanup_policy = Fabricate.build(:account_statuses_cleanup_policy, account: account) + account_statuses_cleanup_policy.valid? + expect(account_statuses_cleanup_policy).to model_have_error_on_field(:account) + end + end + + describe 'save hooks' do + context 'when widening a policy' do + let!(:account_statuses_cleanup_policy) do + Fabricate(:account_statuses_cleanup_policy, + account: account, + keep_direct: true, + keep_pinned: true, + keep_polls: true, + keep_media: true, + keep_self_fav: true, + keep_self_bookmark: true, + min_favs: 1, + min_reblogs: 1 + ) + end + + before do + account_statuses_cleanup_policy.record_last_inspected(42) + end + + it 'invalidates last_inspected when widened because of keep_direct' do + account_statuses_cleanup_policy.keep_direct = false + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + + it 'invalidates last_inspected when widened because of keep_pinned' do + account_statuses_cleanup_policy.keep_pinned = false + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + + it 'invalidates last_inspected when widened because of keep_polls' do + account_statuses_cleanup_policy.keep_polls = false + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + + it 'invalidates last_inspected when widened because of keep_media' do + account_statuses_cleanup_policy.keep_media = false + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + + it 'invalidates last_inspected when widened because of keep_self_fav' do + account_statuses_cleanup_policy.keep_self_fav = false + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + + it 'invalidates last_inspected when widened because of keep_self_bookmark' do + account_statuses_cleanup_policy.keep_self_bookmark = false + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + + it 'invalidates last_inspected when widened because of higher min_favs' do + account_statuses_cleanup_policy.min_favs = 5 + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + + it 'invalidates last_inspected when widened because of disabled min_favs' do + account_statuses_cleanup_policy.min_favs = nil + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + + it 'invalidates last_inspected when widened because of higher min_reblogs' do + account_statuses_cleanup_policy.min_reblogs = 5 + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + + it 'invalidates last_inspected when widened because of disable min_reblogs' do + account_statuses_cleanup_policy.min_reblogs = nil + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to be nil + end + end + + context 'when narrowing a policy' do + let!(:account_statuses_cleanup_policy) do + Fabricate(:account_statuses_cleanup_policy, + account: account, + keep_direct: false, + keep_pinned: false, + keep_polls: false, + keep_media: false, + keep_self_fav: false, + keep_self_bookmark: false, + min_favs: nil, + min_reblogs: nil + ) + end + + it 'does not unnecessarily invalidate last_inspected' do + account_statuses_cleanup_policy.record_last_inspected(42) + account_statuses_cleanup_policy.keep_direct = true + account_statuses_cleanup_policy.keep_pinned = true + account_statuses_cleanup_policy.keep_polls = true + account_statuses_cleanup_policy.keep_media = true + account_statuses_cleanup_policy.keep_self_fav = true + account_statuses_cleanup_policy.keep_self_bookmark = true + account_statuses_cleanup_policy.min_favs = 5 + account_statuses_cleanup_policy.min_reblogs = 5 + account_statuses_cleanup_policy.save + expect(account_statuses_cleanup_policy.last_inspected).to eq 42 + end + end + end + + describe '#record_last_inspected' do + let(:account_statuses_cleanup_policy) { Fabricate(:account_statuses_cleanup_policy, account: account) } + + it 'records the given id' do + account_statuses_cleanup_policy.record_last_inspected(42) + expect(account_statuses_cleanup_policy.last_inspected).to eq 42 + end + end + + describe '#invalidate_last_inspected' do + let(:account_statuses_cleanup_policy) { Fabricate(:account_statuses_cleanup_policy, account: account) } + let(:status) { Fabricate(:status, id: 10, account: account) } + subject { account_statuses_cleanup_policy.invalidate_last_inspected(status, action) } + + before do + account_statuses_cleanup_policy.record_last_inspected(42) + end + + context 'when the action is :unbookmark' do + let(:action) { :unbookmark } + + context 'when the policy is not to keep self-bookmarked toots' do + before do + account_statuses_cleanup_policy.keep_self_bookmark = false + end + + it 'does not change the recorded id' do + subject + expect(account_statuses_cleanup_policy.last_inspected).to eq 42 + end + end + + context 'when the policy is to keep self-bookmarked toots' do + before do + account_statuses_cleanup_policy.keep_self_bookmark = true + end + + it 'records the older id' do + subject + expect(account_statuses_cleanup_policy.last_inspected).to eq 10 + end + end + end + + context 'when the action is :unfav' do + let(:action) { :unfav } + + context 'when the policy is not to keep self-favourited toots' do + before do + account_statuses_cleanup_policy.keep_self_fav = false + end + + it 'does not change the recorded id' do + subject + expect(account_statuses_cleanup_policy.last_inspected).to eq 42 + end + end + + context 'when the policy is to keep self-favourited toots' do + before do + account_statuses_cleanup_policy.keep_self_fav = true + end + + it 'records the older id' do + subject + expect(account_statuses_cleanup_policy.last_inspected).to eq 10 + end + end + end + + context 'when the action is :unpin' do + let(:action) { :unpin } + + context 'when the policy is not to keep pinned toots' do + before do + account_statuses_cleanup_policy.keep_pinned = false + end + + it 'does not change the recorded id' do + subject + expect(account_statuses_cleanup_policy.last_inspected).to eq 42 + end + end + + context 'when the policy is to keep pinned toots' do + before do + account_statuses_cleanup_policy.keep_pinned = true + end + + it 'records the older id' do + subject + expect(account_statuses_cleanup_policy.last_inspected).to eq 10 + end + end + end + + context 'when the status is more recent than the recorded inspected id' do + let(:action) { :unfav } + let(:status) { Fabricate(:status, account: account) } + + it 'does not change the recorded id' do + subject + expect(account_statuses_cleanup_policy.last_inspected).to eq 42 + end + end + end + + describe '#compute_cutoff_id' do + let!(:unrelated_status) { Fabricate(:status, created_at: 3.years.ago) } + let(:account_statuses_cleanup_policy) { Fabricate(:account_statuses_cleanup_policy, account: account) } + + subject { account_statuses_cleanup_policy.compute_cutoff_id } + + context 'when the account has posted multiple toots' do + let!(:very_old_status) { Fabricate(:status, created_at: 3.years.ago, account: account) } + let!(:old_status) { Fabricate(:status, created_at: 3.weeks.ago, account: account) } + let!(:recent_status) { Fabricate(:status, created_at: 2.days.ago, account: account) } + + it 'returns the most recent id that is still below policy age' do + expect(subject).to eq old_status.id + end + end + + context 'when the account has not posted anything' do + it 'returns nil' do + expect(subject).to be_nil + end + end + end + + describe '#statuses_to_delete' do + let!(:unrelated_status) { Fabricate(:status, created_at: 3.years.ago) } + let!(:very_old_status) { Fabricate(:status, created_at: 3.years.ago, account: account) } + let!(:pinned_status) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:direct_message) { Fabricate(:status, created_at: 1.year.ago, account: account, visibility: :direct) } + let!(:self_faved) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:self_bookmarked) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:status_with_poll) { Fabricate(:status, created_at: 1.year.ago, account: account, poll_attributes: { account: account, voters_count: 0, options: ['a', 'b'], expires_in: 2.days }) } + let!(:status_with_media) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:faved4) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:faved5) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:reblogged4) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:reblogged5) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:recent_status) { Fabricate(:status, created_at: 2.days.ago, account: account) } + + let!(:media_attachment) { Fabricate(:media_attachment, account: account, status: status_with_media) } + let!(:status_pin) { Fabricate(:status_pin, account: account, status: pinned_status) } + let!(:favourite) { Fabricate(:favourite, account: account, status: self_faved) } + let!(:bookmark) { Fabricate(:bookmark, account: account, status: self_bookmarked) } + + let(:account_statuses_cleanup_policy) { Fabricate(:account_statuses_cleanup_policy, account: account) } + + subject { account_statuses_cleanup_policy.statuses_to_delete } + + before do + 4.times { faved4.increment_count!(:favourites_count) } + 5.times { faved5.increment_count!(:favourites_count) } + 4.times { reblogged4.increment_count!(:reblogs_count) } + 5.times { reblogged5.increment_count!(:reblogs_count) } + end + + context 'when passed a max_id' do + let!(:old_status) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:slightly_less_old_status) { Fabricate(:status, created_at: 6.months.ago, account: account) } + + subject { account_statuses_cleanup_policy.statuses_to_delete(50, old_status.id).pluck(:id) } + + it 'returns statuses including max_id' do + expect(subject).to include(old_status.id) + end + + it 'returns statuses including older than max_id' do + expect(subject).to include(very_old_status.id) + end + + it 'does not return statuses newer than max_id' do + expect(subject).to_not include(slightly_less_old_status.id) + end + end + + context 'when passed a min_id' do + let!(:old_status) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:slightly_less_old_status) { Fabricate(:status, created_at: 6.months.ago, account: account) } + + subject { account_statuses_cleanup_policy.statuses_to_delete(50, recent_status.id, old_status.id).pluck(:id) } + + it 'returns statuses including min_id' do + expect(subject).to include(old_status.id) + end + + it 'returns statuses including newer than max_id' do + expect(subject).to include(slightly_less_old_status.id) + end + + it 'does not return statuses older than min_id' do + expect(subject).to_not include(very_old_status.id) + end + end + + context 'when passed a low limit' do + it 'only returns the limited number of items' do + expect(account_statuses_cleanup_policy.statuses_to_delete(1).count).to eq 1 + end + end + + context 'when policy is set to keep statuses more recent than 2 years' do + before do + account_statuses_cleanup_policy.min_status_age = 2.years.seconds + end + + it 'does not return unrelated old status' do + expect(subject.pluck(:id)).to_not include(unrelated_status.id) + end + + it 'returns only oldest status for deletion' do + expect(subject.pluck(:id)).to eq [very_old_status.id] + end + end + + context 'when policy is set to keep DMs and reject everything else' do + before do + account_statuses_cleanup_policy.keep_direct = true + account_statuses_cleanup_policy.keep_pinned = false + account_statuses_cleanup_policy.keep_polls = false + account_statuses_cleanup_policy.keep_media = false + account_statuses_cleanup_policy.keep_self_fav = false + account_statuses_cleanup_policy.keep_self_bookmark = false + end + + it 'does not return the old direct message for deletion' do + expect(subject.pluck(:id)).to_not include(direct_message.id) + end + + it 'returns every other old status for deletion' do + expect(subject.pluck(:id)).to include(very_old_status.id, pinned_status.id, self_faved.id, self_bookmarked.id, status_with_poll.id, status_with_media.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id) + end + end + + context 'when policy is set to keep self-bookmarked toots and reject everything else' do + before do + account_statuses_cleanup_policy.keep_direct = false + account_statuses_cleanup_policy.keep_pinned = false + account_statuses_cleanup_policy.keep_polls = false + account_statuses_cleanup_policy.keep_media = false + account_statuses_cleanup_policy.keep_self_fav = false + account_statuses_cleanup_policy.keep_self_bookmark = true + end + + it 'does not return the old self-bookmarked message for deletion' do + expect(subject.pluck(:id)).to_not include(self_bookmarked.id) + end + + it 'returns every other old status for deletion' do + expect(subject.pluck(:id)).to include(direct_message.id, very_old_status.id, pinned_status.id, self_faved.id, status_with_poll.id, status_with_media.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id) + end + end + + context 'when policy is set to keep self-faved toots and reject everything else' do + before do + account_statuses_cleanup_policy.keep_direct = false + account_statuses_cleanup_policy.keep_pinned = false + account_statuses_cleanup_policy.keep_polls = false + account_statuses_cleanup_policy.keep_media = false + account_statuses_cleanup_policy.keep_self_fav = true + account_statuses_cleanup_policy.keep_self_bookmark = false + end + + it 'does not return the old self-bookmarked message for deletion' do + expect(subject.pluck(:id)).to_not include(self_faved.id) + end + + it 'returns every other old status for deletion' do + expect(subject.pluck(:id)).to include(direct_message.id, very_old_status.id, pinned_status.id, self_bookmarked.id, status_with_poll.id, status_with_media.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id) + end + end + + context 'when policy is set to keep toots with media and reject everything else' do + before do + account_statuses_cleanup_policy.keep_direct = false + account_statuses_cleanup_policy.keep_pinned = false + account_statuses_cleanup_policy.keep_polls = false + account_statuses_cleanup_policy.keep_media = true + account_statuses_cleanup_policy.keep_self_fav = false + account_statuses_cleanup_policy.keep_self_bookmark = false + end + + it 'does not return the old message with media for deletion' do + expect(subject.pluck(:id)).to_not include(status_with_media.id) + end + + it 'returns every other old status for deletion' do + expect(subject.pluck(:id)).to include(direct_message.id, very_old_status.id, pinned_status.id, self_faved.id, self_bookmarked.id, status_with_poll.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id) + end + end + + context 'when policy is set to keep toots with polls and reject everything else' do + before do + account_statuses_cleanup_policy.keep_direct = false + account_statuses_cleanup_policy.keep_pinned = false + account_statuses_cleanup_policy.keep_polls = true + account_statuses_cleanup_policy.keep_media = false + account_statuses_cleanup_policy.keep_self_fav = false + account_statuses_cleanup_policy.keep_self_bookmark = false + end + + it 'does not return the old poll message for deletion' do + expect(subject.pluck(:id)).to_not include(status_with_poll.id) + end + + it 'returns every other old status for deletion' do + expect(subject.pluck(:id)).to include(direct_message.id, very_old_status.id, pinned_status.id, self_faved.id, self_bookmarked.id, status_with_media.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id) + end + end + + context 'when policy is set to keep pinned toots and reject everything else' do + before do + account_statuses_cleanup_policy.keep_direct = false + account_statuses_cleanup_policy.keep_pinned = true + account_statuses_cleanup_policy.keep_polls = false + account_statuses_cleanup_policy.keep_media = false + account_statuses_cleanup_policy.keep_self_fav = false + account_statuses_cleanup_policy.keep_self_bookmark = false + end + + it 'does not return the old pinned message for deletion' do + expect(subject.pluck(:id)).to_not include(pinned_status.id) + end + + it 'returns every other old status for deletion' do + expect(subject.pluck(:id)).to include(direct_message.id, very_old_status.id, self_faved.id, self_bookmarked.id, status_with_poll.id, status_with_media.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id) + end + end + + context 'when policy is to not keep any special messages' do + before do + account_statuses_cleanup_policy.keep_direct = false + account_statuses_cleanup_policy.keep_pinned = false + account_statuses_cleanup_policy.keep_polls = false + account_statuses_cleanup_policy.keep_media = false + account_statuses_cleanup_policy.keep_self_fav = false + account_statuses_cleanup_policy.keep_self_bookmark = false + end + + it 'does not return the recent toot' do + expect(subject.pluck(:id)).to_not include(recent_status.id) + end + + it 'does not return the unrelated toot' do + expect(subject.pluck(:id)).to_not include(unrelated_status.id) + end + + it 'returns every other old status for deletion' do + expect(subject.pluck(:id)).to include(direct_message.id, very_old_status.id, pinned_status.id, self_faved.id, self_bookmarked.id, status_with_poll.id, status_with_media.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id) + end + end + + context 'when policy is set to keep every category of toots' do + before do + account_statuses_cleanup_policy.keep_direct = true + account_statuses_cleanup_policy.keep_pinned = true + account_statuses_cleanup_policy.keep_polls = true + account_statuses_cleanup_policy.keep_media = true + account_statuses_cleanup_policy.keep_self_fav = true + account_statuses_cleanup_policy.keep_self_bookmark = true + end + + it 'does not return unrelated old status' do + expect(subject.pluck(:id)).to_not include(unrelated_status.id) + end + + it 'returns only normal statuses for deletion' do + expect(subject.pluck(:id).sort).to eq [very_old_status.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id].sort + end + end + + context 'when policy is to keep statuses with more than 4 boosts' do + before do + account_statuses_cleanup_policy.min_reblogs = 4 + end + + it 'does not return the recent toot' do + expect(subject.pluck(:id)).to_not include(recent_status.id) + end + + it 'does not return the toot reblogged 5 times' do + expect(subject.pluck(:id)).to_not include(reblogged5.id) + end + + it 'does not return the unrelated toot' do + expect(subject.pluck(:id)).to_not include(unrelated_status.id) + end + + it 'returns old statuses not reblogged as much' do + expect(subject.pluck(:id)).to include(very_old_status.id, faved4.id, faved5.id, reblogged4.id) + end + end + + context 'when policy is to keep statuses with more than 4 favs' do + before do + account_statuses_cleanup_policy.min_favs = 4 + end + + it 'does not return the recent toot' do + expect(subject.pluck(:id)).to_not include(recent_status.id) + end + + it 'does not return the toot faved 5 times' do + expect(subject.pluck(:id)).to_not include(faved5.id) + end + + it 'does not return the unrelated toot' do + expect(subject.pluck(:id)).to_not include(unrelated_status.id) + end + + it 'returns old statuses not faved as much' do + expect(subject.pluck(:id)).to include(very_old_status.id, faved4.id, reblogged4.id, reblogged5.id) + end + end + end +end diff --git a/spec/services/account_statuses_cleanup_service_spec.rb b/spec/services/account_statuses_cleanup_service_spec.rb new file mode 100644 index 000000000..257655c41 --- /dev/null +++ b/spec/services/account_statuses_cleanup_service_spec.rb @@ -0,0 +1,101 @@ +require 'rails_helper' + +describe AccountStatusesCleanupService, type: :service do + let(:account) { Fabricate(:account, username: 'alice', domain: nil) } + let(:account_policy) { Fabricate(:account_statuses_cleanup_policy, account: account) } + let!(:unrelated_status) { Fabricate(:status, created_at: 3.years.ago) } + + describe '#call' do + context 'when the account has not posted anything' do + it 'returns 0 deleted toots' do + expect(subject.call(account_policy)).to eq 0 + end + end + + context 'when the account has posted several old statuses' do + let!(:very_old_status) { Fabricate(:status, created_at: 3.years.ago, account: account) } + let!(:old_status) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:another_old_status) { Fabricate(:status, created_at: 1.year.ago, account: account) } + let!(:recent_status) { Fabricate(:status, created_at: 1.day.ago, account: account) } + + context 'given a budget of 1' do + it 'reports 1 deleted toot' do + expect(subject.call(account_policy, 1)).to eq 1 + end + end + + context 'given a normal budget of 10' do + it 'reports 3 deleted statuses' do + expect(subject.call(account_policy, 10)).to eq 3 + end + + it 'records the last deleted id' do + subject.call(account_policy, 10) + expect(account_policy.last_inspected).to eq [old_status.id, another_old_status.id].max + end + + it 'actually deletes the statuses' do + subject.call(account_policy, 10) + expect(Status.find_by(id: [very_old_status.id, old_status.id, another_old_status.id])).to be_nil + end + end + + context 'when called repeatedly with a budget of 2' do + it 'reports 2 then 1 deleted statuses' do + expect(subject.call(account_policy, 2)).to eq 2 + expect(subject.call(account_policy, 2)).to eq 1 + end + + it 'actually deletes the statuses in the expected order' do + subject.call(account_policy, 2) + expect(Status.find_by(id: very_old_status.id)).to be_nil + subject.call(account_policy, 2) + expect(Status.find_by(id: [very_old_status.id, old_status.id, another_old_status.id])).to be_nil + end + end + + context 'when a self-faved toot is unfaved' do + let!(:self_faved) { Fabricate(:status, created_at: 6.months.ago, account: account) } + let!(:favourite) { Fabricate(:favourite, account: account, status: self_faved) } + + it 'deletes it once unfaved' do + expect(subject.call(account_policy, 20)).to eq 3 + expect(Status.find_by(id: self_faved.id)).to_not be_nil + expect(subject.call(account_policy, 20)).to eq 0 + favourite.destroy! + expect(subject.call(account_policy, 20)).to eq 1 + expect(Status.find_by(id: self_faved.id)).to be_nil + end + end + + context 'when there are more un-deletable old toots than the early search cutoff' do + before do + stub_const 'AccountStatusesCleanupPolicy::EARLY_SEARCH_CUTOFF', 5 + # Old statuses that should be cut-off + 10.times do + Fabricate(:status, created_at: 4.years.ago, visibility: :direct, account: account) + end + # New statuses that prevent cut-off id to reach the last status + 10.times do + Fabricate(:status, created_at: 4.seconds.ago, visibility: :direct, account: account) + end + end + + it 'reports 0 deleted statuses then 0 then 3 then 0 again' do + expect(subject.call(account_policy, 10)).to eq 0 + expect(subject.call(account_policy, 10)).to eq 0 + expect(subject.call(account_policy, 10)).to eq 3 + expect(subject.call(account_policy, 10)).to eq 0 + end + + it 'never causes the recorded id to get higher than oldest deletable toot' do + subject.call(account_policy, 10) + subject.call(account_policy, 10) + subject.call(account_policy, 10) + subject.call(account_policy, 10) + expect(account_policy.last_inspected).to be < Mastodon::Snowflake.id_at(account_policy.min_status_age.seconds.ago, with_random: false) + end + end + end + end +end diff --git a/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb b/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb new file mode 100644 index 000000000..8f20725c8 --- /dev/null +++ b/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb @@ -0,0 +1,127 @@ +require 'rails_helper' + +describe Scheduler::AccountsStatusesCleanupScheduler do + subject { described_class.new } + + let!(:account1) { Fabricate(:account, domain: nil) } + let!(:account2) { Fabricate(:account, domain: nil) } + let!(:account3) { Fabricate(:account, domain: nil) } + let!(:account4) { Fabricate(:account, domain: nil) } + let!(:remote) { Fabricate(:account) } + + let!(:policy1) { Fabricate(:account_statuses_cleanup_policy, account: account1) } + let!(:policy2) { Fabricate(:account_statuses_cleanup_policy, account: account3) } + let!(:policy3) { Fabricate(:account_statuses_cleanup_policy, account: account4, enabled: false) } + + let(:queue_size) { 0 } + let(:queue_latency) { 0 } + let(:process_set_stub) do + [ + { + 'concurrency' => 2, + 'queues' => ['push', 'default'], + }, + ] + end + let(:retry_size) { 0 } + + before do + queue_stub = double + allow(queue_stub).to receive(:size).and_return(queue_size) + allow(queue_stub).to receive(:latency).and_return(queue_latency) + allow(Sidekiq::Queue).to receive(:new).and_return(queue_stub) + allow(Sidekiq::ProcessSet).to receive(:new).and_return(process_set_stub) + + sidekiq_stats_stub = double + allow(sidekiq_stats_stub).to receive(:retry_size).and_return(retry_size) + allow(Sidekiq::Stats).to receive(:new).and_return(sidekiq_stats_stub) + + # Create a bunch of old statuses + 10.times do + Fabricate(:status, account: account1, created_at: 3.years.ago) + Fabricate(:status, account: account2, created_at: 3.years.ago) + Fabricate(:status, account: account3, created_at: 3.years.ago) + Fabricate(:status, account: account4, created_at: 3.years.ago) + Fabricate(:status, account: remote, created_at: 3.years.ago) + end + + # Create a bunch of newer statuses + 5.times do + Fabricate(:status, account: account1, created_at: 3.minutes.ago) + Fabricate(:status, account: account2, created_at: 3.minutes.ago) + Fabricate(:status, account: account3, created_at: 3.minutes.ago) + Fabricate(:status, account: account4, created_at: 3.minutes.ago) + Fabricate(:status, account: remote, created_at: 3.minutes.ago) + end + end + + describe '#under_load?' do + context 'when nothing is queued' do + it 'returns false' do + expect(subject.under_load?).to be false + end + end + + context 'when numerous jobs are queued' do + let(:queue_size) { 5 } + let(:queue_latency) { 120 } + + it 'returns true' do + expect(subject.under_load?).to be true + end + end + + context 'when there is a huge amount of jobs to retry' do + let(:retry_size) { 1_000_000 } + + it 'returns true' do + expect(subject.under_load?).to be true + end + end + end + + describe '#get_budget' do + context 'on a single thread' do + let(:process_set_stub) { [ { 'concurrency' => 1, 'queues' => ['push', 'default'] } ] } + + it 'returns a low value' do + expect(subject.compute_budget).to be < 10 + end + end + + context 'on a lot of threads' do + let(:process_set_stub) do + [ + { 'concurrency' => 2, 'queues' => ['push', 'default'] }, + { 'concurrency' => 2, 'queues' => ['push'] }, + { 'concurrency' => 2, 'queues' => ['push'] }, + { 'concurrency' => 2, 'queues' => ['push'] }, + ] + end + + it 'returns a larger value' do + expect(subject.compute_budget).to be > 10 + end + end + end + + describe '#perform' do + context 'when the budget is lower than the number of toots to delete' do + it 'deletes as many statuses as the given budget' do + expect { subject.perform }.to change { Status.count }.by(-subject.compute_budget) + end + + it 'does not delete from accounts with no cleanup policy' do + expect { subject.perform }.to_not change { account2.statuses.count } + end + + it 'does not delete from accounts with disabled cleanup policies' do + expect { subject.perform }.to_not change { account4.statuses.count } + end + + it 'eventually deletes every deletable toot' do + expect { subject.perform; subject.perform; subject.perform; subject.perform }.to change { Status.count }.by(-20) + end + end + end +end -- cgit From 76c7226eb0084ac3340d9935f1dbcbfaa132f436 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 9 Aug 2021 23:28:06 +0200 Subject: Fix account statuses cleanup settings controller for glitch-soc's theming system --- app/controllers/statuses_cleanup_controller.rb | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'app/controllers') diff --git a/app/controllers/statuses_cleanup_controller.rb b/app/controllers/statuses_cleanup_controller.rb index be234cdcb..3d4f4af02 100644 --- a/app/controllers/statuses_cleanup_controller.rb +++ b/app/controllers/statuses_cleanup_controller.rb @@ -6,6 +6,7 @@ class StatusesCleanupController < ApplicationController before_action :authenticate_user! before_action :set_policy before_action :set_body_classes + before_action :set_pack def show; end @@ -21,6 +22,10 @@ class StatusesCleanupController < ApplicationController private + def set_pack + use_pack 'settings' + end + def set_policy @policy = current_account.statuses_cleanup_policy || current_account.build_statuses_cleanup_policy(enabled: false) end -- cgit From 5c21021176813313e656bf1c92177a930a1ca9cc Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 25 Aug 2021 15:40:56 +0000 Subject: Fix undefined variable for Auth::OmniauthCallbacksController (#16654) The addition of authentication history broke the omniauth login with the following error: method=GET path=/auth/auth/cas/callback format=html controller=Auth::OmniauthCallbacksController action=cas status=500 error='NameError: undefined local variable or method `user' for # Did you mean? @user' duration=435.93 view=0.00 db=36.19 * app/controllers/auth/omniauth_callbacks_controller.rb: fix variable name to `@user` --- app/controllers/auth/omniauth_callbacks_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/controllers') diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb index 7925e23cb..991a50b03 100644 --- a/app/controllers/auth/omniauth_callbacks_controller.rb +++ b/app/controllers/auth/omniauth_callbacks_controller.rb @@ -11,7 +11,7 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController if @user.persisted? LoginActivity.create( - user: user, + user: @user, success: true, authentication_method: :omniauth, provider: provider, -- cgit From 94bcf453219da73015cc977835717516b9dc0a67 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 25 Aug 2021 22:52:41 +0200 Subject: Fix authentication failures after going halfway through a sign-in attempt (#16607) * Add tests * Add security-related tests My first (unpublished) attempt at fixing the issues introduced (extremely hard-to-exploit) security vulnerabilities, addressing them in a test. * Fix authentication failures after going halfway through a sign-in attempt * Refactor `authenticate_with_sign_in_token` and `authenticate_with_two_factor` to make the two authentication steps more obvious --- app/controllers/auth/sessions_controller.rb | 16 +-- .../sign_in_token_authentication_concern.rb | 20 ++-- .../concerns/two_factor_authentication_concern.rb | 22 +++-- spec/controllers/auth/sessions_controller_spec.rb | 109 +++++++++++++++++++++ 4 files changed, 144 insertions(+), 23 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index 9c73b39e2..7afd09e10 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -58,16 +58,20 @@ class Auth::SessionsController < Devise::SessionsController protected def find_user - if session[:attempt_user_id] + if user_params[:email].present? + find_user_from_params + elsif session[:attempt_user_id] User.find_by(id: session[:attempt_user_id]) - else - user = User.authenticate_with_ldap(user_params) if Devise.ldap_authentication - user ||= User.authenticate_with_pam(user_params) if Devise.pam_authentication - user ||= User.find_for_authentication(email: user_params[:email]) - user end end + def find_user_from_params + user = User.authenticate_with_ldap(user_params) if Devise.ldap_authentication + user ||= User.authenticate_with_pam(user_params) if Devise.pam_authentication + user ||= User.find_for_authentication(email: user_params[:email]) + user + end + def user_params params.require(:user).permit(:email, :password, :otp_attempt, :sign_in_token_attempt, credential: {}) end diff --git a/app/controllers/concerns/sign_in_token_authentication_concern.rb b/app/controllers/concerns/sign_in_token_authentication_concern.rb index cbee84569..384c5c50c 100644 --- a/app/controllers/concerns/sign_in_token_authentication_concern.rb +++ b/app/controllers/concerns/sign_in_token_authentication_concern.rb @@ -16,14 +16,18 @@ module SignInTokenAuthenticationConcern end def authenticate_with_sign_in_token - user = self.resource = find_user - - if user.present? && session[:attempt_user_id].present? && session[:attempt_user_updated_at] != user.updated_at.to_s - restart_session - elsif user_params.key?(:sign_in_token_attempt) && session[:attempt_user_id] - authenticate_with_sign_in_token_attempt(user) - elsif user.present? && user.external_or_valid_password?(user_params[:password]) - prompt_for_sign_in_token(user) + if user_params[:email].present? + user = self.resource = find_user_from_params + prompt_for_sign_in_token(user) if user&.external_or_valid_password?(user_params[:password]) + elsif session[:attempt_user_id] + user = self.resource = User.find_by(id: session[:attempt_user_id]) + return if user.nil? + + if session[:attempt_user_updated_at] != user.updated_at.to_s + restart_session + elsif user_params.key?(:sign_in_token_attempt) + authenticate_with_sign_in_token_attempt(user) + end end end diff --git a/app/controllers/concerns/two_factor_authentication_concern.rb b/app/controllers/concerns/two_factor_authentication_concern.rb index 909ab7717..2583d324b 100644 --- a/app/controllers/concerns/two_factor_authentication_concern.rb +++ b/app/controllers/concerns/two_factor_authentication_concern.rb @@ -35,16 +35,20 @@ module TwoFactorAuthenticationConcern end def authenticate_with_two_factor - user = self.resource = find_user + if user_params[:email].present? + user = self.resource = find_user_from_params + prompt_for_two_factor(user) if user&.external_or_valid_password?(user_params[:password]) + elsif session[:attempt_user_id] + user = self.resource = User.find_by(id: session[:attempt_user_id]) + return if user.nil? - if user.present? && session[:attempt_user_id].present? && session[:attempt_user_updated_at] != user.updated_at.to_s - restart_session - elsif user.webauthn_enabled? && user_params.key?(:credential) && session[:attempt_user_id] - authenticate_with_two_factor_via_webauthn(user) - elsif user_params.key?(:otp_attempt) && session[:attempt_user_id] - authenticate_with_two_factor_via_otp(user) - elsif user.present? && user.external_or_valid_password?(user_params[:password]) - prompt_for_two_factor(user) + if session[:attempt_user_updated_at] != user.updated_at.to_s + restart_session + elsif user.webauthn_enabled? && user_params.key?(:credential) + authenticate_with_two_factor_via_webauthn(user) + elsif user_params.key?(:otp_attempt) + authenticate_with_two_factor_via_otp(user) + end end end diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index d03ae51e8..051a0807d 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -206,6 +206,38 @@ RSpec.describe Auth::SessionsController, type: :controller do end end + context 'using email and password after an unfinished log-in attempt to a 2FA-protected account' do + let!(:other_user) do + Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: true, otp_secret: User.generate_otp_secret(32)) + end + + before do + post :create, params: { user: { email: other_user.email, password: other_user.password } } + post :create, params: { user: { email: user.email, password: user.password } } + end + + it 'renders two factor authentication page' do + expect(controller).to render_template("two_factor") + expect(controller).to render_template(partial: "_otp_authentication_form") + end + end + + context 'using email and password after an unfinished log-in attempt with a sign-in token challenge' do + let!(:other_user) do + Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago) + end + + before do + post :create, params: { user: { email: other_user.email, password: other_user.password } } + post :create, params: { user: { email: user.email, password: user.password } } + end + + it 'renders two factor authentication page' do + expect(controller).to render_template("two_factor") + expect(controller).to render_template(partial: "_otp_authentication_form") + end + end + context 'using upcase email and password' do before do post :create, params: { user: { email: user.email.upcase, password: user.password } } @@ -231,6 +263,21 @@ RSpec.describe Auth::SessionsController, type: :controller do end end + context 'using a valid OTP, attempting to leverage previous half-login to bypass password auth' do + let!(:other_user) do + Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago) + end + + before do + post :create, params: { user: { email: other_user.email, password: other_user.password } } + post :create, params: { user: { email: user.email, otp_attempt: user.current_otp } }, session: { attempt_user_updated_at: user.updated_at.to_s } + end + + it "doesn't log the user in" do + expect(controller.current_user).to be_nil + end + end + context 'when the server has an decryption error' do before do allow_any_instance_of(User).to receive(:validate_and_consume_otp!).and_raise(OpenSSL::Cipher::CipherError) @@ -380,6 +427,52 @@ RSpec.describe Auth::SessionsController, type: :controller do end end + context 'using email and password after an unfinished log-in attempt to a 2FA-protected account' do + let!(:other_user) do + Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: true, otp_secret: User.generate_otp_secret(32)) + end + + before do + post :create, params: { user: { email: other_user.email, password: other_user.password } } + post :create, params: { user: { email: user.email, password: user.password } } + end + + it 'renders sign in token authentication page' do + expect(controller).to render_template("sign_in_token") + end + + it 'generates sign in token' do + expect(user.reload.sign_in_token).to_not be_nil + end + + it 'sends sign in token e-mail' do + expect(UserMailer).to have_received(:sign_in_token) + end + end + + context 'using email and password after an unfinished log-in attempt with a sign-in token challenge' do + let!(:other_user) do + Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago) + end + + before do + post :create, params: { user: { email: other_user.email, password: other_user.password } } + post :create, params: { user: { email: user.email, password: user.password } } + end + + it 'renders sign in token authentication page' do + expect(controller).to render_template("sign_in_token") + end + + it 'generates sign in token' do + expect(user.reload.sign_in_token).to_not be_nil + end + + it 'sends sign in token e-mail' do + expect(UserMailer).to have_received(:sign_in_token).with(user, any_args) + end + end + context 'using a valid sign in token' do before do user.generate_sign_in_token && user.save @@ -395,6 +488,22 @@ RSpec.describe Auth::SessionsController, type: :controller do end end + context 'using a valid sign in token, attempting to leverage previous half-login to bypass password auth' do + let!(:other_user) do + Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago) + end + + before do + user.generate_sign_in_token && user.save + post :create, params: { user: { email: other_user.email, password: other_user.password } } + post :create, params: { user: { email: user.email, sign_in_token_attempt: user.sign_in_token } }, session: { attempt_user_updated_at: user.updated_at.to_s } + end + + it "doesn't log the user in" do + expect(controller.current_user).to be_nil + end + end + context 'using an invalid sign in token' do before do post :create, params: { user: { sign_in_token_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } -- cgit From 7283a5d3b94b655172744996ffa43ec80aff0e08 Mon Sep 17 00:00:00 2001 From: Truong Nguyen Date: Thu, 26 Aug 2021 23:51:22 +0900 Subject: Explicitly set userVerification to discoraged (#16545) --- app/controllers/auth/sessions_controller.rb | 5 ++++- .../two_factor_authentication/webauthn_credentials_controller.rb | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index 7afd09e10..2c3d510cb 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -45,7 +45,10 @@ class Auth::SessionsController < Devise::SessionsController user = find_user if user&.webauthn_enabled? - options_for_get = WebAuthn::Credential.options_for_get(allow: user.webauthn_credentials.pluck(:external_id)) + options_for_get = WebAuthn::Credential.options_for_get( + allow: user.webauthn_credentials.pluck(:external_id), + user_verification: 'discouraged' + ) session[:webauthn_challenge] = options_for_get.challenge diff --git a/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb b/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb index 1c557092b..a50d30f06 100644 --- a/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb +++ b/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb @@ -21,7 +21,8 @@ module Settings display_name: current_user.account.username, id: current_user.webauthn_id, }, - exclude: current_user.webauthn_credentials.pluck(:external_id) + exclude: current_user.webauthn_credentials.pluck(:external_id), + authenticator_selection: { user_verification: 'discouraged' } ) session[:webauthn_challenge] = options_for_create.challenge -- cgit From e0af97164af6440f439a2aa7ec0b4d11c1121097 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 15 Sep 2021 18:51:16 +0200 Subject: Fix followers synchronization mechanism not working when URI has empty path (#16744) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #16510, forgot the controller exposing the actual followers… --- app/controllers/activitypub/followers_synchronizations_controller.rb | 4 ++-- .../activitypub/followers_synchronizations_controller_spec.rb | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/activitypub/followers_synchronizations_controller.rb b/app/controllers/activitypub/followers_synchronizations_controller.rb index 525031105..940b77cf0 100644 --- a/app/controllers/activitypub/followers_synchronizations_controller.rb +++ b/app/controllers/activitypub/followers_synchronizations_controller.rb @@ -19,11 +19,11 @@ class ActivityPub::FollowersSynchronizationsController < ActivityPub::BaseContro private def uri_prefix - signed_request_account.uri[/http(s?):\/\/[^\/]+\//] + signed_request_account.uri[Account::URL_PREFIX_RE] end def set_items - @items = @account.followers.where(Account.arel_table[:uri].matches(uri_prefix + '%', false, true)).pluck(:uri) + @items = @account.followers.where(Account.arel_table[:uri].matches("#{Account.sanitize_sql_like(uri_prefix)}/%", false, true)).or(@account.followers.where(uri: uri_prefix)).pluck(:uri) end def collection_presenter diff --git a/spec/controllers/activitypub/followers_synchronizations_controller_spec.rb b/spec/controllers/activitypub/followers_synchronizations_controller_spec.rb index d373f56bd..3a382ff27 100644 --- a/spec/controllers/activitypub/followers_synchronizations_controller_spec.rb +++ b/spec/controllers/activitypub/followers_synchronizations_controller_spec.rb @@ -5,11 +5,13 @@ RSpec.describe ActivityPub::FollowersSynchronizationsController, type: :controll let!(:follower_1) { Fabricate(:account, domain: 'example.com', uri: 'https://example.com/users/a') } let!(:follower_2) { Fabricate(:account, domain: 'example.com', uri: 'https://example.com/users/b') } let!(:follower_3) { Fabricate(:account, domain: 'foo.com', uri: 'https://foo.com/users/a') } + let!(:follower_4) { Fabricate(:account, username: 'instance-actor', domain: 'example.com', uri: 'https://example.com') } before do follower_1.follow!(account) follower_2.follow!(account) follower_3.follow!(account) + follower_4.follow!(account) end before do @@ -45,7 +47,7 @@ RSpec.describe ActivityPub::FollowersSynchronizationsController, type: :controll it 'returns orderedItems with followers from example.com' do expect(body[:orderedItems]).to be_an Array - expect(body[:orderedItems].sort).to eq [follower_1.uri, follower_2.uri] + expect(body[:orderedItems].sort).to eq [follower_4.uri, follower_1.uri, follower_2.uri] end it 'returns private Cache-Control header' do -- cgit From 52e5c07948c4c91b73062846e1f19ea278ec0e24 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 26 Sep 2021 05:46:13 +0200 Subject: Change routing paths to use usernames in web UI (#16171) --- app/controllers/home_controller.rb | 25 +------- app/javascript/mastodon/actions/accounts.js | 32 ++++++++++ app/javascript/mastodon/actions/compose.js | 4 +- app/javascript/mastodon/api.js | 26 +++++++-- app/javascript/mastodon/components/account.js | 2 +- app/javascript/mastodon/components/hashtag.js | 2 +- app/javascript/mastodon/components/status.js | 64 +++++++++++--------- .../mastodon/components/status_action_bar.js | 2 +- .../mastodon/components/status_content.js | 6 +- app/javascript/mastodon/containers/mastodon.js | 26 ++++++++- .../mastodon/features/account/components/header.js | 6 +- .../mastodon/features/account_gallery/index.js | 64 ++++++++++++++------ .../features/account_timeline/components/header.js | 6 +- .../account_timeline/components/moved_note.js | 2 +- .../mastodon/features/account_timeline/index.js | 55 ++++++++++------- .../features/compose/components/navigation_bar.js | 4 +- .../features/compose/components/reply_indicator.js | 2 +- app/javascript/mastodon/features/compose/index.js | 6 +- .../direct_timeline/components/conversation.js | 2 +- .../features/directory/components/account_card.js | 2 +- .../components/account_authorize.js | 2 +- .../mastodon/features/followers/index.js | 68 +++++++++++++++------- .../mastodon/features/following/index.js | 68 +++++++++++++++------- .../getting_started/components/announcements.js | 6 +- .../mastodon/features/getting_started/index.js | 10 ++-- app/javascript/mastodon/features/lists/index.js | 2 +- .../notifications/components/follow_request.js | 2 +- .../notifications/components/notification.js | 6 +- .../picture_in_picture/components/footer.js | 2 +- .../picture_in_picture/components/header.js | 2 +- .../features/status/components/detailed_status.js | 6 +- app/javascript/mastodon/features/status/index.js | 2 +- .../mastodon/features/ui/components/boost_modal.js | 2 +- .../features/ui/components/columns_area.js | 2 +- .../mastodon/features/ui/components/list_panel.js | 2 +- .../mastodon/features/ui/components/media_modal.js | 7 --- .../features/ui/components/navigation_panel.js | 8 +-- .../mastodon/features/ui/components/tabs_bar.js | 6 +- app/javascript/mastodon/features/ui/index.js | 46 ++++++++------- app/javascript/mastodon/reducers/accounts_map.js | 15 +++++ app/javascript/mastodon/reducers/index.js | 2 + .../service_worker/web_push_notifications.js | 2 +- app/lib/permalink_redirector.rb | 63 ++++++++++++++++++++ spec/controllers/home_controller_spec.rb | 6 +- spec/lib/permalink_redirector_spec.rb | 42 +++++++++++++ 45 files changed, 492 insertions(+), 225 deletions(-) create mode 100644 app/javascript/mastodon/reducers/accounts_map.js create mode 100644 app/lib/permalink_redirector.rb create mode 100644 spec/lib/permalink_redirector_spec.rb (limited to 'app/controllers') diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb index 702889cd0..7e443eb9e 100644 --- a/app/controllers/home_controller.rb +++ b/app/controllers/home_controller.rb @@ -14,30 +14,7 @@ class HomeController < ApplicationController def redirect_unauthenticated_to_permalinks! return if user_signed_in? - matches = request.path.match(/\A\/web\/(statuses|accounts)\/([\d]+)\z/) - - if matches - case matches[1] - when 'statuses' - status = Status.find_by(id: matches[2]) - - if status&.distributable? - redirect_to(ActivityPub::TagManager.instance.url_for(status)) - return - end - when 'accounts' - account = Account.find_by(id: matches[2]) - - if account - redirect_to(ActivityPub::TagManager.instance.url_for(account)) - return - end - end - end - - matches = request.path.match(%r{\A/web/timelines/tag/(?.+)\z}) - - redirect_to(matches ? tag_path(CGI.unescape(matches[:tag])) : default_redirect_path) + redirect_to(PermalinkRedirector.new(request.path).redirect_path || default_redirect_path) end def default_redirect_path diff --git a/app/javascript/mastodon/actions/accounts.js b/app/javascript/mastodon/actions/accounts.js index 58b636602..ce7bb6d5f 100644 --- a/app/javascript/mastodon/actions/accounts.js +++ b/app/javascript/mastodon/actions/accounts.js @@ -5,6 +5,10 @@ export const ACCOUNT_FETCH_REQUEST = 'ACCOUNT_FETCH_REQUEST'; export const ACCOUNT_FETCH_SUCCESS = 'ACCOUNT_FETCH_SUCCESS'; export const ACCOUNT_FETCH_FAIL = 'ACCOUNT_FETCH_FAIL'; +export const ACCOUNT_LOOKUP_REQUEST = 'ACCOUNT_LOOKUP_REQUEST'; +export const ACCOUNT_LOOKUP_SUCCESS = 'ACCOUNT_LOOKUP_SUCCESS'; +export const ACCOUNT_LOOKUP_FAIL = 'ACCOUNT_LOOKUP_FAIL'; + export const ACCOUNT_FOLLOW_REQUEST = 'ACCOUNT_FOLLOW_REQUEST'; export const ACCOUNT_FOLLOW_SUCCESS = 'ACCOUNT_FOLLOW_SUCCESS'; export const ACCOUNT_FOLLOW_FAIL = 'ACCOUNT_FOLLOW_FAIL'; @@ -87,6 +91,34 @@ export function fetchAccount(id) { }; }; +export const lookupAccount = acct => (dispatch, getState) => { + dispatch(lookupAccountRequest(acct)); + + api(getState).get('/api/v1/accounts/lookup', { params: { acct } }).then(response => { + dispatch(fetchRelationships([response.data.id])); + dispatch(importFetchedAccount(response.data)); + dispatch(lookupAccountSuccess()); + }).catch(error => { + dispatch(lookupAccountFail(acct, error)); + }); +}; + +export const lookupAccountRequest = (acct) => ({ + type: ACCOUNT_LOOKUP_REQUEST, + acct, +}); + +export const lookupAccountSuccess = () => ({ + type: ACCOUNT_LOOKUP_SUCCESS, +}); + +export const lookupAccountFail = (acct, error) => ({ + type: ACCOUNT_LOOKUP_FAIL, + acct, + error, + skipAlert: true, +}); + export function fetchAccountRequest(id) { return { type: ACCOUNT_FETCH_REQUEST, diff --git a/app/javascript/mastodon/actions/compose.js b/app/javascript/mastodon/actions/compose.js index 39d31a88f..a000a036f 100644 --- a/app/javascript/mastodon/actions/compose.js +++ b/app/javascript/mastodon/actions/compose.js @@ -78,7 +78,7 @@ const COMPOSE_PANEL_BREAKPOINT = 600 + (285 * 1) + (10 * 1); export const ensureComposeIsVisible = (getState, routerHistory) => { if (!getState().getIn(['compose', 'mounted']) && window.innerWidth < COMPOSE_PANEL_BREAKPOINT) { - routerHistory.push('/statuses/new'); + routerHistory.push('/publish'); } }; @@ -158,7 +158,7 @@ export function submitCompose(routerHistory) { 'Idempotency-Key': getState().getIn(['compose', 'idempotencyKey']), }, }).then(function (response) { - if (routerHistory && routerHistory.location.pathname === '/statuses/new' && window.history.state) { + if (routerHistory && routerHistory.location.pathname === '/publish' && window.history.state) { routerHistory.goBack(); } diff --git a/app/javascript/mastodon/api.js b/app/javascript/mastodon/api.js index 98d59de43..645ef6500 100644 --- a/app/javascript/mastodon/api.js +++ b/app/javascript/mastodon/api.js @@ -12,21 +12,35 @@ export const getLinks = response => { return LinkHeader.parse(value); }; -let csrfHeader = {}; +const csrfHeader = {}; -function setCSRFHeader() { +const setCSRFHeader = () => { const csrfToken = document.querySelector('meta[name=csrf-token]'); + if (csrfToken) { csrfHeader['X-CSRF-Token'] = csrfToken.content; } -} +}; ready(setCSRFHeader); +const authorizationHeaderFromState = getState => { + const accessToken = getState && getState().getIn(['meta', 'access_token'], ''); + + if (!accessToken) { + return {}; + } + + return { + 'Authorization': `Bearer ${accessToken}`, + }; +}; + export default getState => axios.create({ - headers: Object.assign(csrfHeader, getState ? { - 'Authorization': `Bearer ${getState().getIn(['meta', 'access_token'], '')}`, - } : {}), + headers: { + ...csrfHeader, + ...authorizationHeaderFromState(getState), + }, transformResponse: [function (data) { try { diff --git a/app/javascript/mastodon/components/account.js b/app/javascript/mastodon/components/account.js index a85d683a7..62b5843a9 100644 --- a/app/javascript/mastodon/components/account.js +++ b/app/javascript/mastodon/components/account.js @@ -118,7 +118,7 @@ class Account extends ImmutablePureComponent { return (
- +
{mute_expires_at} diff --git a/app/javascript/mastodon/components/hashtag.js b/app/javascript/mastodon/components/hashtag.js index 75fcf20e3..1a7edf6e0 100644 --- a/app/javascript/mastodon/components/hashtag.js +++ b/app/javascript/mastodon/components/hashtag.js @@ -52,7 +52,7 @@ const Hashtag = ({ hashtag }) => (
#{hashtag.get('name')} diff --git a/app/javascript/mastodon/components/status.js b/app/javascript/mastodon/components/status.js index ccc9067d1..96e6a6c8d 100644 --- a/app/javascript/mastodon/components/status.js +++ b/app/javascript/mastodon/components/status.js @@ -134,42 +134,28 @@ class Status extends ImmutablePureComponent { this.setState({ showMedia: !this.state.showMedia }); } - handleClick = () => { - if (this.props.onClick) { - this.props.onClick(); + handleClick = e => { + if (e && (e.button !== 0 || e.ctrlKey || e.metaKey)) { return; } - if (!this.context.router) { - return; + if (e) { + e.preventDefault(); } - const { status } = this.props; - this.context.router.history.push(`/statuses/${status.getIn(['reblog', 'id'], status.get('id'))}`); + this.handleHotkeyOpen(); } - handleExpandClick = (e) => { - if (this.props.onClick) { - this.props.onClick(); + handleAccountClick = e => { + if (e && (e.button !== 0 || e.ctrlKey || e.metaKey)) { return; } - if (e.button === 0) { - if (!this.context.router) { - return; - } - - const { status } = this.props; - this.context.router.history.push(`/statuses/${status.getIn(['reblog', 'id'], status.get('id'))}`); - } - } - - handleAccountClick = (e) => { - if (this.context.router && e.button === 0 && !(e.ctrlKey || e.metaKey)) { - const id = e.currentTarget.getAttribute('data-id'); + if (e) { e.preventDefault(); - this.context.router.history.push(`/accounts/${id}`); } + + this.handleHotkeyOpenProfile(); } handleExpandedToggle = () => { @@ -242,11 +228,30 @@ class Status extends ImmutablePureComponent { } handleHotkeyOpen = () => { - this.context.router.history.push(`/statuses/${this._properStatus().get('id')}`); + if (this.props.onClick) { + this.props.onClick(); + return; + } + + const { router } = this.context; + const status = this._properStatus(); + + if (!router) { + return; + } + + router.history.push(`/@${status.getIn(['account', 'acct'])}/${status.get('id')}`); } handleHotkeyOpenProfile = () => { - this.context.router.history.push(`/accounts/${this._properStatus().getIn(['account', 'id'])}`); + const { router } = this.context; + const status = this._properStatus(); + + if (!router) { + return; + } + + router.history.push(`/@${status.getIn(['account', 'acct'])}`); } handleHotkeyMoveUp = e => { @@ -465,14 +470,15 @@ class Status extends ImmutablePureComponent { {prepend}
-
+
+
- + - +
{statusAvatar}
diff --git a/app/javascript/mastodon/components/status_action_bar.js b/app/javascript/mastodon/components/status_action_bar.js index 9981f2449..85c76edee 100644 --- a/app/javascript/mastodon/components/status_action_bar.js +++ b/app/javascript/mastodon/components/status_action_bar.js @@ -186,7 +186,7 @@ class StatusActionBar extends ImmutablePureComponent { } handleOpen = () => { - this.context.router.history.push(`/statuses/${this.props.status.get('id')}`); + this.context.router.history.push(`/@${this.props.status.getIn(['account', 'acct'])}/${this.props.status.get('id')}`); } handleEmbed = () => { diff --git a/app/javascript/mastodon/components/status_content.js b/app/javascript/mastodon/components/status_content.js index bf21a9fd6..d01365afb 100644 --- a/app/javascript/mastodon/components/status_content.js +++ b/app/javascript/mastodon/components/status_content.js @@ -112,7 +112,7 @@ export default class StatusContent extends React.PureComponent { onMentionClick = (mention, e) => { if (this.context.router && e.button === 0 && !(e.ctrlKey || e.metaKey)) { e.preventDefault(); - this.context.router.history.push(`/accounts/${mention.get('id')}`); + this.context.router.history.push(`/@${mention.get('acct')}`); } } @@ -121,7 +121,7 @@ export default class StatusContent extends React.PureComponent { if (this.context.router && e.button === 0 && !(e.ctrlKey || e.metaKey)) { e.preventDefault(); - this.context.router.history.push(`/timelines/tag/${hashtag}`); + this.context.router.history.push(`/tags/${hashtag}`); } } @@ -198,7 +198,7 @@ export default class StatusContent extends React.PureComponent { let mentionsPlaceholder = ''; const mentionLinks = status.get('mentions').map(item => ( - + @{item.get('username')} )).reduce((aggregate, item) => [...aggregate, item, ' '], []); diff --git a/app/javascript/mastodon/containers/mastodon.js b/app/javascript/mastodon/containers/mastodon.js index 892ff1ca9..0c3f6afa8 100644 --- a/app/javascript/mastodon/containers/mastodon.js +++ b/app/javascript/mastodon/containers/mastodon.js @@ -22,14 +22,38 @@ const hydrateAction = hydrateStore(initialState); store.dispatch(hydrateAction); store.dispatch(fetchCustomEmojis()); +const createIdentityContext = state => ({ + signedIn: !!state.meta.me, + accountId: state.meta.me, + accessToken: state.meta.access_token, +}); + export default class Mastodon extends React.PureComponent { static propTypes = { locale: PropTypes.string.isRequired, }; + static childContextTypes = { + identity: PropTypes.shape({ + signedIn: PropTypes.bool.isRequired, + accountId: PropTypes.string, + accessToken: PropTypes.string, + }).isRequired, + }; + + identity = createIdentityContext(initialState); + + getChildContext() { + return { + identity: this.identity, + }; + } + componentDidMount() { - this.disconnect = store.dispatch(connectUserStream()); + if (this.identity.signedIn) { + this.disconnect = store.dispatch(connectUserStream()); + } } componentWillUnmount () { diff --git a/app/javascript/mastodon/features/account/components/header.js b/app/javascript/mastodon/features/account/components/header.js index 20641121f..4d0a828c7 100644 --- a/app/javascript/mastodon/features/account/components/header.js +++ b/app/javascript/mastodon/features/account/components/header.js @@ -332,21 +332,21 @@ class Header extends ImmutablePureComponent { {!suspended && (
- + - + - + ({ - isAccount: !!state.getIn(['accounts', props.params.accountId]), - attachments: getAccountGallery(state, props.params.accountId), - isLoading: state.getIn(['timelines', `account:${props.params.accountId}:media`, 'isLoading']), - hasMore: state.getIn(['timelines', `account:${props.params.accountId}:media`, 'hasMore']), - suspended: state.getIn(['accounts', props.params.accountId, 'suspended'], false), - blockedBy: state.getIn(['relationships', props.params.accountId, 'blocked_by'], false), -}); +const mapStateToProps = (state, { params: { acct } }) => { + const accountId = state.getIn(['accounts_map', acct]); + + if (!accountId) { + return { + isLoading: true, + }; + } + + return { + accountId, + isAccount: !!state.getIn(['accounts', accountId]), + attachments: getAccountGallery(state, accountId), + isLoading: state.getIn(['timelines', `account:${accountId}:media`, 'isLoading']), + hasMore: state.getIn(['timelines', `account:${accountId}:media`, 'hasMore']), + suspended: state.getIn(['accounts', accountId, 'suspended'], false), + blockedBy: state.getIn(['relationships', accountId, 'blocked_by'], false), + }; +}; class LoadMoreMedia extends ImmutablePureComponent { @@ -52,7 +63,10 @@ export default @connect(mapStateToProps) class AccountGallery extends ImmutablePureComponent { static propTypes = { - params: PropTypes.object.isRequired, + params: PropTypes.shape({ + acct: PropTypes.string.isRequired, + }).isRequired, + accountId: PropTypes.string, dispatch: PropTypes.func.isRequired, attachments: ImmutablePropTypes.list.isRequired, isLoading: PropTypes.bool, @@ -67,15 +81,29 @@ class AccountGallery extends ImmutablePureComponent { width: 323, }; + _load () { + const { accountId, dispatch } = this.props; + + dispatch(expandAccountMediaTimeline(accountId)); + } + componentDidMount () { - this.props.dispatch(fetchAccount(this.props.params.accountId)); - this.props.dispatch(expandAccountMediaTimeline(this.props.params.accountId)); + const { params: { acct }, accountId, dispatch } = this.props; + + if (accountId) { + this._load(); + } else { + dispatch(lookupAccount(acct)); + } } - componentWillReceiveProps (nextProps) { - if (nextProps.params.accountId !== this.props.params.accountId && nextProps.params.accountId) { - this.props.dispatch(fetchAccount(nextProps.params.accountId)); - this.props.dispatch(expandAccountMediaTimeline(this.props.params.accountId)); + componentDidUpdate (prevProps) { + const { params: { acct }, accountId, dispatch } = this.props; + + if (prevProps.accountId !== accountId && accountId) { + this._load(); + } else if (prevProps.params.acct !== acct) { + dispatch(lookupAccount(acct)); } } @@ -95,7 +123,7 @@ class AccountGallery extends ImmutablePureComponent { } handleLoadMore = maxId => { - this.props.dispatch(expandAccountMediaTimeline(this.props.params.accountId, { maxId })); + this.props.dispatch(expandAccountMediaTimeline(this.props.accountId, { maxId })); }; handleLoadOlder = e => { @@ -165,7 +193,7 @@ class AccountGallery extends ImmutablePureComponent {
- + {(suspended || blockedBy) ? (
diff --git a/app/javascript/mastodon/features/account_timeline/components/header.js b/app/javascript/mastodon/features/account_timeline/components/header.js index 6b52defe4..17b693600 100644 --- a/app/javascript/mastodon/features/account_timeline/components/header.js +++ b/app/javascript/mastodon/features/account_timeline/components/header.js @@ -123,9 +123,9 @@ export default class Header extends ImmutablePureComponent { {!hideTabs && (
- - - + + +
)}
diff --git a/app/javascript/mastodon/features/account_timeline/components/moved_note.js b/app/javascript/mastodon/features/account_timeline/components/moved_note.js index 3e090bb5f..2e32d660f 100644 --- a/app/javascript/mastodon/features/account_timeline/components/moved_note.js +++ b/app/javascript/mastodon/features/account_timeline/components/moved_note.js @@ -21,7 +21,7 @@ export default class MovedNote extends ImmutablePureComponent { handleAccountClick = e => { if (e.button === 0) { e.preventDefault(); - this.context.router.history.push(`/accounts/${this.props.to.get('id')}`); + this.context.router.history.push(`/@${this.props.to.get('acct')}`); } e.stopPropagation(); diff --git a/app/javascript/mastodon/features/account_timeline/index.js b/app/javascript/mastodon/features/account_timeline/index.js index 821c852b6..8ca768373 100644 --- a/app/javascript/mastodon/features/account_timeline/index.js +++ b/app/javascript/mastodon/features/account_timeline/index.js @@ -2,7 +2,7 @@ import React from 'react'; import { connect } from 'react-redux'; import ImmutablePropTypes from 'react-immutable-proptypes'; import PropTypes from 'prop-types'; -import { fetchAccount } from '../../actions/accounts'; +import { lookupAccount, fetchAccount } from '../../actions/accounts'; import { expandAccountFeaturedTimeline, expandAccountTimeline } from '../../actions/timelines'; import StatusList from '../../components/status_list'; import LoadingIndicator from '../../components/loading_indicator'; @@ -20,10 +20,19 @@ import { connectTimeline, disconnectTimeline } from 'mastodon/actions/timelines' const emptyList = ImmutableList(); -const mapStateToProps = (state, { params: { accountId }, withReplies = false }) => { +const mapStateToProps = (state, { params: { acct }, withReplies = false }) => { + const accountId = state.getIn(['accounts_map', acct]); + + if (!accountId) { + return { + isLoading: true, + }; + } + const path = withReplies ? `${accountId}:with_replies` : accountId; return { + accountId, remote: !!(state.getIn(['accounts', accountId, 'acct']) !== state.getIn(['accounts', accountId, 'username'])), remoteUrl: state.getIn(['accounts', accountId, 'url']), isAccount: !!state.getIn(['accounts', accountId]), @@ -48,7 +57,10 @@ export default @connect(mapStateToProps) class AccountTimeline extends ImmutablePureComponent { static propTypes = { - params: PropTypes.object.isRequired, + params: PropTypes.shape({ + acct: PropTypes.string.isRequired, + }).isRequired, + accountId: PropTypes.string, dispatch: PropTypes.func.isRequired, statusIds: ImmutablePropTypes.list, featuredStatusIds: ImmutablePropTypes.list, @@ -63,8 +75,8 @@ class AccountTimeline extends ImmutablePureComponent { multiColumn: PropTypes.bool, }; - componentWillMount () { - const { params: { accountId }, withReplies, dispatch } = this.props; + _load () { + const { accountId, withReplies, dispatch } = this.props; dispatch(fetchAccount(accountId)); dispatch(fetchAccountIdentityProofs(accountId)); @@ -80,29 +92,32 @@ class AccountTimeline extends ImmutablePureComponent { } } - componentWillReceiveProps (nextProps) { - const { dispatch } = this.props; + componentDidMount () { + const { params: { acct }, accountId, dispatch } = this.props; - if ((nextProps.params.accountId !== this.props.params.accountId && nextProps.params.accountId) || nextProps.withReplies !== this.props.withReplies) { - dispatch(fetchAccount(nextProps.params.accountId)); - dispatch(fetchAccountIdentityProofs(nextProps.params.accountId)); + if (accountId) { + this._load(); + } else { + dispatch(lookupAccount(acct)); + } + } - if (!nextProps.withReplies) { - dispatch(expandAccountFeaturedTimeline(nextProps.params.accountId)); - } + componentDidUpdate (prevProps) { + const { params: { acct }, accountId, dispatch } = this.props; - dispatch(expandAccountTimeline(nextProps.params.accountId, { withReplies: nextProps.params.withReplies })); + if (prevProps.accountId !== accountId && accountId) { + this._load(); + } else if (prevProps.params.acct !== acct) { + dispatch(lookupAccount(acct)); } - if (nextProps.params.accountId === me && this.props.params.accountId !== me) { - dispatch(connectTimeline(`account:${me}`)); - } else if (this.props.params.accountId === me && nextProps.params.accountId !== me) { + if (prevProps.accountId === me && accountId !== me) { dispatch(disconnectTimeline(`account:${me}`)); } } componentWillUnmount () { - const { dispatch, params: { accountId } } = this.props; + const { dispatch, accountId } = this.props; if (accountId === me) { dispatch(disconnectTimeline(`account:${me}`)); @@ -110,7 +125,7 @@ class AccountTimeline extends ImmutablePureComponent { } handleLoadMore = maxId => { - this.props.dispatch(expandAccountTimeline(this.props.params.accountId, { maxId, withReplies: this.props.withReplies })); + this.props.dispatch(expandAccountTimeline(this.props.accountId, { maxId, withReplies: this.props.withReplies })); } render () { @@ -152,7 +167,7 @@ class AccountTimeline extends ImmutablePureComponent { } + prepend={} alwaysPrepend append={remoteMessage} scrollKey='account_timeline' diff --git a/app/javascript/mastodon/features/compose/components/navigation_bar.js b/app/javascript/mastodon/features/compose/components/navigation_bar.js index 840d0a3da..e6ba7d8b7 100644 --- a/app/javascript/mastodon/features/compose/components/navigation_bar.js +++ b/app/javascript/mastodon/features/compose/components/navigation_bar.js @@ -19,13 +19,13 @@ export default class NavigationBar extends ImmutablePureComponent { render () { return (
- + {this.props.account.get('acct')}
- + @{this.props.account.get('acct')} diff --git a/app/javascript/mastodon/features/compose/components/reply_indicator.js b/app/javascript/mastodon/features/compose/components/reply_indicator.js index a1d5c420c..863defb76 100644 --- a/app/javascript/mastodon/features/compose/components/reply_indicator.js +++ b/app/javascript/mastodon/features/compose/components/reply_indicator.js @@ -32,7 +32,7 @@ class ReplyIndicator extends ImmutablePureComponent { handleAccountClick = (e) => { if (e.button === 0 && !(e.ctrlKey || e.metaKey)) { e.preventDefault(); - this.context.router.history.push(`/accounts/${this.props.status.getIn(['account', 'id'])}`); + this.context.router.history.push(`/@${this.props.status.getIn(['account', 'acct'])}`); } } diff --git a/app/javascript/mastodon/features/compose/index.js b/app/javascript/mastodon/features/compose/index.js index c1bce0a3f..3d2796cd3 100644 --- a/app/javascript/mastodon/features/compose/index.js +++ b/app/javascript/mastodon/features/compose/index.js @@ -100,16 +100,16 @@ class Compose extends React.PureComponent {