From 5f387995d9ae6f89c93518518233c6d9874f6621 Mon Sep 17 00:00:00 2001 From: ThibG Date: Mon, 24 Dec 2018 19:06:14 +0100 Subject: Limit maximum visibility of local silenced users to unlisted (#9583) Fixes #9580 --- spec/services/post_status_service_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'spec') diff --git a/spec/services/post_status_service_spec.rb b/spec/services/post_status_service_spec.rb index 349ad861b..8f3552224 100644 --- a/spec/services/post_status_service_spec.rb +++ b/spec/services/post_status_service_spec.rb @@ -68,6 +68,13 @@ RSpec.describe PostStatusService, type: :service do expect(status.visibility).to eq "private" end + it 'creates a status with limited visibility for silenced users' do + status = subject.call(Fabricate(:account, silenced: true), 'test', nil, visibility: :public) + + expect(status).to be_persisted + expect(status.visibility).to eq "unlisted" + end + it 'creates a status for the given application' do application = Fabricate(:application) -- cgit From 5d2fc6de321ac556ded72e5a8fa9fcf47d172e16 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 24 Dec 2018 19:12:38 +0100 Subject: Add REST API for creating an account (#9572) * Add REST API for creating an account The method is available to apps with a token obtained via the client credentials grant. It creates a user and account records, as well as an access token for the app that initiated the request. The user is unconfirmed, and an e-mail is sent as usual. The method returns the access token, which the app should save for later. The REST API is not available to users with unconfirmed accounts, so the app must be smart to wait for the user to click a link in their e-mail inbox. The method is rate-limited by IP to 5 requests per 30 minutes. * Redirect users back to app from confirmation if they were created with an app * Add tests * Return 403 on the method if registrations are not open * Require agreement param to be true in the API when creating an account --- app/controllers/api/base_controller.rb | 2 +- app/controllers/api/v1/accounts_controller.rb | 26 ++++++++++++-- app/controllers/auth/confirmations_controller.rb | 10 +++++- app/controllers/auth/registrations_controller.rb | 1 + app/models/user.rb | 7 ++-- app/services/app_sign_up_service.rb | 23 ++++++++++++ .../confirmation_instructions.html.haml | 8 +++-- .../user_mailer/confirmation_instructions.text.erb | 2 +- config/initializers/rack_attack.rb | 4 +++ config/locales/devise.en.yml | 1 + config/routes.rb | 2 +- ...35220_add_created_by_application_id_to_users.rb | 8 +++++ db/schema.rb | 5 ++- lib/mastodon/accounts_cli.rb | 2 +- .../controllers/api/v1/accounts_controller_spec.rb | 34 ++++++++++++++++++ spec/fabricators/user_fabricator.rb | 1 + spec/models/user_spec.rb | 14 ++++---- spec/services/app_sign_up_service_spec.rb | 41 ++++++++++++++++++++++ 18 files changed, 171 insertions(+), 20 deletions(-) create mode 100644 app/services/app_sign_up_service.rb create mode 100644 db/migrate/20181219235220_add_created_by_application_id_to_users.rb create mode 100644 spec/services/app_sign_up_service_spec.rb (limited to 'spec') diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index ac8de5fc0..2bf8e82db 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -68,7 +68,7 @@ class Api::BaseController < ApplicationController end def require_user! - if current_user && !current_user.disabled? + if current_user && !current_user.disabled? && current_user.confirmed? set_user_activity elsif current_user render json: { error: 'Your login is currently disabled' }, status: 403 diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index f711c4676..6e4084c4e 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -1,14 +1,16 @@ # frozen_string_literal: true class Api::V1::AccountsController < Api::BaseController - before_action -> { authorize_if_got_token! :read, :'read:accounts' }, except: [:follow, :unfollow, :block, :unblock, :mute, :unmute] + before_action -> { authorize_if_got_token! :read, :'read:accounts' }, except: [:create, :follow, :unfollow, :block, :unblock, :mute, :unmute] before_action -> { doorkeeper_authorize! :follow, :'write:follows' }, only: [:follow, :unfollow] before_action -> { doorkeeper_authorize! :follow, :'write:mutes' }, only: [:mute, :unmute] before_action -> { doorkeeper_authorize! :follow, :'write:blocks' }, only: [:block, :unblock] + before_action -> { doorkeeper_authorize! :write, :'write:accounts' }, only: [:create] - before_action :require_user!, except: [:show] - before_action :set_account + before_action :require_user!, except: [:show, :create] + before_action :set_account, except: [:create] before_action :check_account_suspension, only: [:show] + before_action :check_enabled_registrations, only: [:create] respond_to :json @@ -16,6 +18,16 @@ class Api::V1::AccountsController < Api::BaseController render json: @account, serializer: REST::AccountSerializer end + def create + token = AppSignUpService.new.call(doorkeeper_token.application, account_params) + response = Doorkeeper::OAuth::TokenResponse.new(token) + + headers.merge!(response.headers) + + self.response_body = Oj.dump(response.body) + self.status = response.status + end + def follow FollowService.new.call(current_user.account, @account, reblogs: truthy_param?(:reblogs)) @@ -62,4 +74,12 @@ class Api::V1::AccountsController < Api::BaseController def check_account_suspension gone if @account.suspended? end + + def account_params + params.permit(:username, :email, :password, :agreement) + end + + def check_enabled_registrations + forbidden if single_user_mode? || !Setting.open_registrations + end end diff --git a/app/controllers/auth/confirmations_controller.rb b/app/controllers/auth/confirmations_controller.rb index 7af9cbe81..c28c7471c 100644 --- a/app/controllers/auth/confirmations_controller.rb +++ b/app/controllers/auth/confirmations_controller.rb @@ -6,9 +6,9 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController before_action :set_body_classes before_action :set_user, only: [:finish_signup] - # GET/PATCH /users/:id/finish_signup def finish_signup return unless request.patch? && params[:user] + if @user.update(user_params) @user.skip_reconfirmation! bypass_sign_in(@user) @@ -31,4 +31,12 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController def user_params params.require(:user).permit(:email) end + + def after_confirmation_path_for(_resource_name, user) + if user.created_by_application && truthy_param?(:redirect_to_app) + user.created_by_application.redirect_uri + else + super + end + end end diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 088832be3..f2a832542 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -26,6 +26,7 @@ class Auth::RegistrationsController < Devise::RegistrationsController resource.locale = I18n.locale resource.invite_code = params[:invite_code] if resource.invite_code.blank? + resource.agreement = true resource.build_account if resource.account.nil? end diff --git a/app/models/user.rb b/app/models/user.rb index 44e0d1113..77e48ed4b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -36,6 +36,7 @@ # invite_id :bigint(8) # remember_token :string # chosen_languages :string is an Array +# created_by_application_id :bigint(8) # class User < ApplicationRecord @@ -66,6 +67,7 @@ class User < ApplicationRecord belongs_to :account, inverse_of: :user belongs_to :invite, counter_cache: :uses, optional: true + belongs_to :created_by_application, class_name: 'Doorkeeper::Application', optional: true accepts_nested_attributes_for :account has_many :applications, class_name: 'Doorkeeper::Application', as: :owner @@ -74,6 +76,7 @@ class User < ApplicationRecord validates :locale, inclusion: I18n.available_locales.map(&:to_s), if: :locale? validates_with BlacklistedEmailValidator, if: :email_changed? validates_with EmailMxValidator, if: :validate_email_dns? + validates :agreement, acceptance: { allow_nil: false, accept: [true, 'true', '1'] }, on: :create scope :recent, -> { order(id: :desc) } scope :admins, -> { where(admin: true) } @@ -294,7 +297,7 @@ class User < ApplicationRecord end if resource.blank? - resource = new(email: attributes[:email]) + resource = new(email: attributes[:email], agreement: true) if Devise.check_at_sign && !resource[:email].index('@') resource[:email] = Rpam2.getenv(resource.find_pam_service, attributes[:email], attributes[:password], 'email', false) resource[:email] = "#{attributes[:email]}@#{resource.find_pam_suffix}" unless resource[:email] @@ -307,7 +310,7 @@ class User < ApplicationRecord resource = joins(:account).find_by(accounts: { username: attributes[Devise.ldap_uid.to_sym].first }) if resource.blank? - resource = new(email: attributes[:mail].first, account_attributes: { username: attributes[Devise.ldap_uid.to_sym].first }) + resource = new(email: attributes[:mail].first, agreement: true, account_attributes: { username: attributes[Devise.ldap_uid.to_sym].first }) resource.ldap_setup(attributes) end diff --git a/app/services/app_sign_up_service.rb b/app/services/app_sign_up_service.rb new file mode 100644 index 000000000..1878587e8 --- /dev/null +++ b/app/services/app_sign_up_service.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class AppSignUpService < BaseService + def call(app, params) + return unless allowed_registrations? + + user_params = params.slice(:email, :password, :agreement) + account_params = params.slice(:username) + user = User.create!(user_params.merge(created_by_application: app, password_confirmation: user_params[:password], account_attributes: account_params)) + + Doorkeeper::AccessToken.create!(application: app, + resource_owner_id: user.id, + scopes: app.scopes, + expires_in: Doorkeeper.configuration.access_token_expires_in, + use_refresh_token: Doorkeeper.configuration.refresh_token_enabled?) + end + + private + + def allowed_registrations? + Setting.open_registrations && !Rails.configuration.x.single_user_mode + end +end diff --git a/app/views/user_mailer/confirmation_instructions.html.haml b/app/views/user_mailer/confirmation_instructions.html.haml index 1f088a16f..f75f7529a 100644 --- a/app/views/user_mailer/confirmation_instructions.html.haml +++ b/app/views/user_mailer/confirmation_instructions.html.haml @@ -55,8 +55,12 @@ %tbody %tr %td.button-primary - = link_to confirmation_url(@resource, confirmation_token: @token) do - %span= t 'devise.mailer.confirmation_instructions.action' + - if @resource.created_by_application + = link_to confirmation_url(@resource, confirmation_token: @token, redirect_to_app: 'true') do + %span= t 'devise.mailer.confirmation_instructions.action_with_app', app: @resource.created_by_application.name + - else + = link_to confirmation_url(@resource, confirmation_token: @token) do + %span= t 'devise.mailer.confirmation_instructions.action' %table.email-table{ cellspacing: 0, cellpadding: 0 } %tbody diff --git a/app/views/user_mailer/confirmation_instructions.text.erb b/app/views/user_mailer/confirmation_instructions.text.erb index e01eecb27..65b4626c6 100644 --- a/app/views/user_mailer/confirmation_instructions.text.erb +++ b/app/views/user_mailer/confirmation_instructions.text.erb @@ -4,7 +4,7 @@ <%= t 'devise.mailer.confirmation_instructions.explanation', host: site_hostname %> -=> <%= confirmation_url(@resource, confirmation_token: @token) %> +=> <%= confirmation_url(@resource, confirmation_token: @token, redirect_to_app: @resource.created_by_application ? 'true' : nil) %> <%= strip_tags(t('devise.mailer.confirmation_instructions.extra_html', terms_path: about_more_url, policy_path: terms_url)) %> diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 8756b8fbf..35302e37b 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -57,6 +57,10 @@ class Rack::Attack req.authenticated_user_id if req.post? && req.path.start_with?('/api/v1/media') end + throttle('throttle_api_sign_up', limit: 5, period: 30.minutes) do |req| + req.ip if req.post? && req.path == '/api/v1/accounts' + end + throttle('protected_paths', limit: 25, period: 5.minutes) do |req| req.ip if req.post? && req.path =~ PROTECTED_PATHS_REGEX end diff --git a/config/locales/devise.en.yml b/config/locales/devise.en.yml index 20938e47b..bd0642b25 100644 --- a/config/locales/devise.en.yml +++ b/config/locales/devise.en.yml @@ -18,6 +18,7 @@ en: mailer: confirmation_instructions: action: Verify email address + action_with_app: Confirm and return to %{app} explanation: You have created an account on %{host} with this email address. You are one click away from activating it. If this wasn't you, please ignore this email. extra_html: Please also check out the rules of the instance and our terms of service. subject: 'Mastodon: Confirmation instructions for %{instance}' diff --git a/config/routes.rb b/config/routes.rb index 7723a08af..808bb5acd 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -336,7 +336,7 @@ Rails.application.routes.draw do resources :relationships, only: :index end - resources :accounts, only: [:show] do + resources :accounts, only: [:create, :show] do resources :statuses, only: :index, controller: 'accounts/statuses' resources :followers, only: :index, controller: 'accounts/follower_accounts' resources :following, only: :index, controller: 'accounts/following_accounts' diff --git a/db/migrate/20181219235220_add_created_by_application_id_to_users.rb b/db/migrate/20181219235220_add_created_by_application_id_to_users.rb new file mode 100644 index 000000000..17ce900af --- /dev/null +++ b/db/migrate/20181219235220_add_created_by_application_id_to_users.rb @@ -0,0 +1,8 @@ +class AddCreatedByApplicationIdToUsers < ActiveRecord::Migration[5.2] + disable_ddl_transaction! + + def change + add_reference :users, :created_by_application, foreign_key: { to_table: 'oauth_applications', on_delete: :nullify }, index: false + add_index :users, :created_by_application_id, algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index 51a7b5e74..e47960b16 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2018_12_13_185533) do +ActiveRecord::Schema.define(version: 2018_12_19_235220) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -637,8 +637,10 @@ ActiveRecord::Schema.define(version: 2018_12_13_185533) do t.bigint "invite_id" t.string "remember_token" t.string "chosen_languages", array: true + t.bigint "created_by_application_id" 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" t.index ["email"], name: "index_users_on_email", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true end @@ -730,6 +732,7 @@ ActiveRecord::Schema.define(version: 2018_12_13_185533) do add_foreign_key "subscriptions", "accounts", name: "fk_9847d1cbb5", on_delete: :cascade add_foreign_key "users", "accounts", name: "fk_50500f500d", on_delete: :cascade add_foreign_key "users", "invites", on_delete: :nullify + add_foreign_key "users", "oauth_applications", column: "created_by_application_id", on_delete: :nullify add_foreign_key "web_push_subscriptions", "oauth_access_tokens", column: "access_token_id", on_delete: :cascade add_foreign_key "web_push_subscriptions", "users", on_delete: :cascade add_foreign_key "web_settings", "users", name: "fk_11910667b2", on_delete: :cascade diff --git a/lib/mastodon/accounts_cli.rb b/lib/mastodon/accounts_cli.rb index b21968223..bbda244ea 100644 --- a/lib/mastodon/accounts_cli.rb +++ b/lib/mastodon/accounts_cli.rb @@ -73,7 +73,7 @@ module Mastodon def create(username) account = Account.new(username: username) password = SecureRandom.hex - user = User.new(email: options[:email], password: password, admin: options[:role] == 'admin', moderator: options[:role] == 'moderator', confirmed_at: Time.now.utc) + user = User.new(email: options[:email], password: password, agreement: true, admin: options[:role] == 'admin', moderator: options[:role] == 'moderator', confirmed_at: options[:confirmed] ? Time.now.utc : nil) if options[:reattach] account = Account.find_local(username) || Account.new(username: username) diff --git a/spec/controllers/api/v1/accounts_controller_spec.rb b/spec/controllers/api/v1/accounts_controller_spec.rb index c506fb5f0..f5f65c000 100644 --- a/spec/controllers/api/v1/accounts_controller_spec.rb +++ b/spec/controllers/api/v1/accounts_controller_spec.rb @@ -19,6 +19,40 @@ RSpec.describe Api::V1::AccountsController, type: :controller do end end + describe 'POST #create' do + let(:app) { Fabricate(:application) } + let(:token) { Doorkeeper::AccessToken.find_or_create_for(app, nil, 'read write', nil, false) } + let(:agreement) { nil } + + before do + post :create, params: { username: 'test', password: '12345678', email: 'hello@world.tld', agreement: agreement } + end + + context 'given truthy agreement' do + let(:agreement) { 'true' } + + it 'returns http success' do + expect(response).to have_http_status(200) + end + + it 'returns a new access token as JSON' do + expect(body_as_json[:access_token]).to_not be_blank + end + + it 'creates a user' do + user = User.find_by(email: 'hello@world.tld') + expect(user).to_not be_nil + expect(user.created_by_application_id).to eq app.id + end + end + + context 'given no agreement' do + it 'returns http unprocessable entity' do + expect(response).to have_http_status(422) + end + end + end + describe 'GET #show' do let(:scopes) { 'read:accounts' } diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb index 7dfbdb52d..8f5956501 100644 --- a/spec/fabricators/user_fabricator.rb +++ b/spec/fabricators/user_fabricator.rb @@ -3,4 +3,5 @@ Fabricator(:user) do email { sequence(:email) { |i| "#{i}#{Faker::Internet.email}" } } password "123456789" confirmed_at { Time.zone.now } + agreement true end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c82919597..856254ce4 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -106,19 +106,19 @@ RSpec.describe User, type: :model do end it 'should allow a non-blacklisted user to be created' do - user = User.new(email: 'foo@example.com', account: account, password: password) + user = User.new(email: 'foo@example.com', account: account, password: password, agreement: true) expect(user.valid?).to be_truthy end it 'should not allow a blacklisted user to be created' do - user = User.new(email: 'foo@mvrht.com', account: account, password: password) + user = User.new(email: 'foo@mvrht.com', account: account, password: password, agreement: true) expect(user.valid?).to be_falsey end it 'should not allow a subdomain blacklisted user to be created' do - user = User.new(email: 'foo@mvrht.com.topdomain.tld', account: account, password: password) + user = User.new(email: 'foo@mvrht.com.topdomain.tld', account: account, password: password, agreement: true) expect(user.valid?).to be_falsey end @@ -210,17 +210,17 @@ RSpec.describe User, type: :model do end it 'should not allow a user to be created unless they are whitelisted' do - user = User.new(email: 'foo@example.com', account: account, password: password) + user = User.new(email: 'foo@example.com', account: account, password: password, agreement: true) expect(user.valid?).to be_falsey end it 'should allow a user to be created if they are whitelisted' do - user = User.new(email: 'foo@mastodon.space', account: account, password: password) + user = User.new(email: 'foo@mastodon.space', account: account, password: password, agreement: true) expect(user.valid?).to be_truthy end it 'should not allow a user with a whitelisted top domain as subdomain in their email address to be created' do - user = User.new(email: 'foo@mastodon.space.userdomain.com', account: account, password: password) + user = User.new(email: 'foo@mastodon.space.userdomain.com', account: account, password: password, agreement: true) expect(user.valid?).to be_falsey end @@ -242,7 +242,7 @@ RSpec.describe User, type: :model do it_behaves_like 'Settings-extended' do def create! - User.create!(account: Fabricate(:account), email: 'foo@mastodon.space', password: 'abcd1234') + User.create!(account: Fabricate(:account), email: 'foo@mastodon.space', password: 'abcd1234', agreement: true) end def fabricate diff --git a/spec/services/app_sign_up_service_spec.rb b/spec/services/app_sign_up_service_spec.rb new file mode 100644 index 000000000..d480df348 --- /dev/null +++ b/spec/services/app_sign_up_service_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +RSpec.describe AppSignUpService, type: :service do + let(:app) { Fabricate(:application, scopes: 'read write') } + let(:good_params) { { username: 'alice', password: '12345678', email: 'good@email.com', agreement: true } } + + subject { described_class.new } + + describe '#call' do + it 'returns nil when registrations are closed' do + Setting.open_registrations = false + expect(subject.call(app, good_params)).to be_nil + end + + it 'raises an error when params are missing' do + expect { subject.call(app, {}) }.to raise_error ActiveRecord::RecordInvalid + end + + it 'creates an unconfirmed user with access token' do + access_token = subject.call(app, good_params) + expect(access_token).to_not be_nil + user = User.find_by(id: access_token.resource_owner_id) + expect(user).to_not be_nil + expect(user.confirmed?).to be false + end + + it 'creates access token with the app\'s scopes' do + access_token = subject.call(app, good_params) + expect(access_token).to_not be_nil + expect(access_token.scopes.to_s).to eq 'read write' + end + + it 'creates an account' do + access_token = subject.call(app, good_params) + expect(access_token).to_not be_nil + user = User.find_by(id: access_token.resource_owner_id) + expect(user).to_not be_nil + expect(user.account).to_not be_nil + end + end +end -- cgit From ccb9c1b952e87dd954bfb651db59dc02a4d9341f Mon Sep 17 00:00:00 2001 From: ysksn Date: Fri, 28 Dec 2018 16:18:47 +0900 Subject: Add pending specs for StatusLengthValidator (#9647) * Add pending specs of StatusLengthValidator * Use instance variable --- app/validators/status_length_validator.rb | 22 ++++++++++++---------- spec/validators/status_length_validator_spec.rb | 13 +++++++++++-- 2 files changed, 23 insertions(+), 12 deletions(-) (limited to 'spec') diff --git a/app/validators/status_length_validator.rb b/app/validators/status_length_validator.rb index ed5563f64..93bae2fa8 100644 --- a/app/validators/status_length_validator.rb +++ b/app/validators/status_length_validator.rb @@ -5,27 +5,29 @@ class StatusLengthValidator < ActiveModel::Validator def validate(status) return unless status.local? && !status.reblog? - status.errors.add(:text, I18n.t('statuses.over_character_limit', max: MAX_CHARS)) if too_long?(status) + + @status = status + status.errors.add(:text, I18n.t('statuses.over_character_limit', max: MAX_CHARS)) if too_long? end private - def too_long?(status) - countable_length(status) > MAX_CHARS + def too_long? + countable_length > MAX_CHARS end - def countable_length(status) - total_text(status).mb_chars.grapheme_length + def countable_length + total_text.mb_chars.grapheme_length end - def total_text(status) - [status.spoiler_text, countable_text(status)].join + def total_text + [@status.spoiler_text, countable_text].join end - def countable_text(status) - return '' if status.text.nil? + def countable_text + return '' if @status.text.nil? - status.text.dup.tap do |new_text| + @status.text.dup.tap do |new_text| new_text.gsub!(FetchLinkCardService::URL_PATTERN, 'x' * 23) new_text.gsub!(Account::MENTION_RE, '@\2') end diff --git a/spec/validators/status_length_validator_spec.rb b/spec/validators/status_length_validator_spec.rb index e2d1a15ec..11e55f933 100644 --- a/spec/validators/status_length_validator_spec.rb +++ b/spec/validators/status_length_validator_spec.rb @@ -4,8 +4,17 @@ require 'rails_helper' describe StatusLengthValidator do describe '#validate' do - it 'does not add errors onto remote statuses' - it 'does not add errors onto local reblogs' + it 'does not add errors onto remote statuses' do + status = double(local?: false) + subject.validate(status) + expect(status).not_to receive(:errors) + end + + it 'does not add errors onto local reblogs' do + status = double(local?: false, reblog?: true) + subject.validate(status) + expect(status).not_to receive(:errors) + end it 'adds an error when content warning is over 500 characters' do status = double(spoiler_text: 'a' * 520, text: '', errors: double(add: nil), local?: true, reblog?: false) -- cgit From d01c840e14d544e933d90f6e474ff0078bc4a899 Mon Sep 17 00:00:00 2001 From: ysksn Date: Fri, 28 Dec 2018 18:09:32 +0900 Subject: Add specs for StatusPinValidator (#9648) --- spec/validators/status_pin_validator_spec.rb | 57 ++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 spec/validators/status_pin_validator_spec.rb (limited to 'spec') diff --git a/spec/validators/status_pin_validator_spec.rb b/spec/validators/status_pin_validator_spec.rb new file mode 100644 index 000000000..06532e5b3 --- /dev/null +++ b/spec/validators/status_pin_validator_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe StatusPinValidator, type: :validator do + describe '#validate' do + before do + subject.validate(pin) + end + + let(:pin) { double(account: account, errors: errors, status: status, account_id: pin_account_id) } + let(:status) { double(reblog?: reblog, account_id: status_account_id, visibility: visibility) } + let(:account) { double(status_pins: status_pins, local?: local) } + let(:status_pins) { double(count: count) } + let(:errors) { double(add: nil) } + let(:pin_account_id) { 1 } + let(:status_account_id) { 1 } + let(:visibility) { 'public' } + let(:local) { false } + let(:reblog) { false } + let(:count) { 0 } + + context 'pin.status.reblog?' do + let(:reblog) { true } + + it 'calls errors.add' do + expect(errors).to have_received(:add).with(:base, I18n.t('statuses.pin_errors.reblog')) + end + end + + context 'pin.account_id != pin.status.account_id' do + let(:pin_account_id) { 1 } + let(:status_account_id) { 2 } + + it 'calls errors.add' do + expect(errors).to have_received(:add).with(:base, I18n.t('statuses.pin_errors.ownership')) + end + end + + context 'unless %w(public unlisted).include?(pin.status.visibility)' do + let(:visibility) { '' } + + it 'calls errors.add' do + expect(errors).to have_received(:add).with(:base, I18n.t('statuses.pin_errors.private')) + end + end + + context 'pin.account.status_pins.count > 4 && pin.account.local?' do + let(:count) { 5 } + let(:local) { true } + + it 'calls errors.add' do + expect(errors).to have_received(:add).with(:base, I18n.t('statuses.pin_errors.limit')) + end + end + end +end -- cgit From 0f938ff29c2e9bf92e3eb9c23be8d4ba3a1b97f7 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 29 Dec 2018 02:24:36 +0100 Subject: Add handler for Move activity (#9629) --- app/lib/activitypub/activity.rb | 2 + app/lib/activitypub/activity/follow.rb | 2 +- app/lib/activitypub/activity/move.rb | 43 ++++++++++++++++++ app/lib/activitypub/adapter.rb | 1 + app/models/account.rb | 5 +++ app/serializers/activitypub/actor_serializer.rb | 5 +++ .../activitypub/process_account_service.rb | 1 + app/services/follow_service.rb | 2 +- app/workers/unfollow_follow_worker.rb | 18 ++++++++ ...20181226021420_add_also_known_as_to_accounts.rb | 5 +++ db/schema.rb | 3 +- spec/lib/activitypub/activity/move_spec.rb | 52 ++++++++++++++++++++++ 12 files changed, 136 insertions(+), 3 deletions(-) create mode 100644 app/lib/activitypub/activity/move.rb create mode 100644 app/workers/unfollow_follow_worker.rb create mode 100644 db/migrate/20181226021420_add_also_known_as_to_accounts.rb create mode 100644 spec/lib/activitypub/activity/move_spec.rb (limited to 'spec') diff --git a/app/lib/activitypub/activity.rb b/app/lib/activitypub/activity.rb index 0a729011f..87318fb1c 100644 --- a/app/lib/activitypub/activity.rb +++ b/app/lib/activitypub/activity.rb @@ -50,6 +50,8 @@ class ActivityPub::Activity ActivityPub::Activity::Add when 'Remove' ActivityPub::Activity::Remove + when 'Move' + ActivityPub::Activity::Move end end end diff --git a/app/lib/activitypub/activity/follow.rb b/app/lib/activitypub/activity/follow.rb index c45832648..7ca650fdf 100644 --- a/app/lib/activitypub/activity/follow.rb +++ b/app/lib/activitypub/activity/follow.rb @@ -6,7 +6,7 @@ class ActivityPub::Activity::Follow < ActivityPub::Activity return if target_account.nil? || !target_account.local? || delete_arrived_first?(@json['id']) || @account.requested?(target_account) - if target_account.blocking?(@account) || target_account.domain_blocking?(@account.domain) + if target_account.blocking?(@account) || target_account.domain_blocking?(@account.domain) || target_account.moved? reject_follow_request!(target_account) return end diff --git a/app/lib/activitypub/activity/move.rb b/app/lib/activitypub/activity/move.rb new file mode 100644 index 000000000..d7a5f595c --- /dev/null +++ b/app/lib/activitypub/activity/move.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +class ActivityPub::Activity::Move < ActivityPub::Activity + PROCESSING_COOLDOWN = 7.days.seconds + + def perform + return if origin_account.uri != object_uri || processed? + + mark_as_processing! + + target_account = ActivityPub::FetchRemoteAccountService.new.call(target_uri) + + return if target_account.nil? || !target_account.also_known_as.include?(origin_account.uri) + + # In case for some reason we didn't have a redirect for the profile already, set it + origin_account.update(moved_to_account: target_account) if origin_account.moved_to_account_id.nil? + + # Initiate a re-follow for each follower + origin_account.followers.local.select(:id).find_in_batches do |follower_accounts| + UnfollowFollowWorker.push_bulk(follower_accounts.map(&:id)) do |follower_account_id| + [follower_account_id, origin_account.id, target_account.id] + end + end + end + + private + + def origin_account + @account + end + + def target_uri + value_or_id(@json['target']) + end + + def processed? + redis.exists("move_in_progress:#{@account.id}") + end + + def mark_as_processing! + redis.setex("move_in_progress:#{@account.id}", PROCESSING_COOLDOWN, true) + end +end diff --git a/app/lib/activitypub/adapter.rb b/app/lib/activitypub/adapter.rb index d35cae889..99f4d9305 100644 --- a/app/lib/activitypub/adapter.rb +++ b/app/lib/activitypub/adapter.rb @@ -10,6 +10,7 @@ class ActivityPub::Adapter < ActiveModelSerializers::Adapter::Base 'manuallyApprovesFollowers' => 'as:manuallyApprovesFollowers', 'sensitive' => 'as:sensitive', 'movedTo' => { '@id' => 'as:movedTo', '@type' => '@id' }, + 'alsoKnownAs' => { '@id' => 'as:alsoKnownAs', '@type' => '@id' }, 'Hashtag' => 'as:Hashtag', 'ostatus' => 'http://ostatus.org#', 'atomUri' => 'ostatus:atomUri', diff --git a/app/models/account.rb b/app/models/account.rb index 66f02b27d..f354bc29b 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -44,6 +44,7 @@ # fields :jsonb # actor_type :string # discoverable :boolean +# also_known_as :string is an Array # class Account < ApplicationRecord @@ -227,6 +228,10 @@ class Account < ApplicationRecord end end + def also_known_as + self[:also_known_as] || [] + end + def fields (self[:fields] || []).map { |f| Field.new(self, f) } end diff --git a/app/serializers/activitypub/actor_serializer.rb b/app/serializers/activitypub/actor_serializer.rb index 72c30dc73..6746c1782 100644 --- a/app/serializers/activitypub/actor_serializer.rb +++ b/app/serializers/activitypub/actor_serializer.rb @@ -14,6 +14,7 @@ class ActivityPub::ActorSerializer < ActiveModel::Serializer has_many :virtual_attachments, key: :attachment attribute :moved_to, if: :moved? + attribute :also_known_as, if: :also_known_as? class EndpointsSerializer < ActiveModel::Serializer include RoutingHelper @@ -116,6 +117,10 @@ class ActivityPub::ActorSerializer < ActiveModel::Serializer ActivityPub::TagManager.instance.uri_for(object.moved_to_account) end + def also_known_as? + !object.also_known_as.empty? + end + class CustomEmojiSerializer < ActivityPub::EmojiSerializer end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 5c865dae2..d6480028f 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -75,6 +75,7 @@ class ActivityPub::ProcessAccountService < BaseService @account.note = @json['summary'] || '' @account.locked = @json['manuallyApprovesFollowers'] || false @account.fields = property_values || {} + @account.also_known_as = as_array(@json['alsoKnownAs'] || []).map { |item| value_or_id(item) } @account.actor_type = actor_type end diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb index 0020bc9fe..862926260 100644 --- a/app/services/follow_service.rb +++ b/app/services/follow_service.rb @@ -12,7 +12,7 @@ class FollowService < BaseService target_account = ResolveAccountService.new.call(target_account, skip_webfinger: true) raise ActiveRecord::RecordNotFound if target_account.nil? || target_account.id == source_account.id || target_account.suspended? - raise Mastodon::NotPermittedError if target_account.blocking?(source_account) || source_account.blocking?(target_account) + raise Mastodon::NotPermittedError if target_account.blocking?(source_account) || source_account.blocking?(target_account) || target_account.moved? if source_account.following?(target_account) # We're already following this account, but we'll call follow! again to diff --git a/app/workers/unfollow_follow_worker.rb b/app/workers/unfollow_follow_worker.rb new file mode 100644 index 000000000..a2133bb8c --- /dev/null +++ b/app/workers/unfollow_follow_worker.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class UnfollowFollowWorker + include Sidekiq::Worker + + sidekiq_options queue: 'pull' + + def perform(follower_account_id, old_target_account_id, new_target_account_id) + follower_account = Account.find(follower_account_id) + old_target_account = Account.find(old_target_account_id) + new_target_account = Account.find(new_target_account_id) + + UnfollowService.new.call(follower_account, old_target_account) + FollowService.new.call(follower_account, new_target_account) + rescue ActiveRecord::RecordNotFound + true + end +end diff --git a/db/migrate/20181226021420_add_also_known_as_to_accounts.rb b/db/migrate/20181226021420_add_also_known_as_to_accounts.rb new file mode 100644 index 000000000..1fd956680 --- /dev/null +++ b/db/migrate/20181226021420_add_also_known_as_to_accounts.rb @@ -0,0 +1,5 @@ +class AddAlsoKnownAsToAccounts < ActiveRecord::Migration[5.2] + def change + add_column :accounts, :also_known_as, :string, array: true + end +end diff --git a/db/schema.rb b/db/schema.rb index e47960b16..066a90b52 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2018_12_19_235220) do +ActiveRecord::Schema.define(version: 2018_12_26_021420) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -134,6 +134,7 @@ ActiveRecord::Schema.define(version: 2018_12_19_235220) do t.jsonb "fields" t.string "actor_type" t.boolean "discoverable" + t.string "also_known_as", array: true t.index "(((setweight(to_tsvector('simple'::regconfig, (display_name)::text), 'A'::\"char\") || setweight(to_tsvector('simple'::regconfig, (username)::text), 'B'::\"char\")) || setweight(to_tsvector('simple'::regconfig, (COALESCE(domain, ''::character varying))::text), 'C'::\"char\")))", name: "search_index", using: :gin t.index "lower((username)::text), lower((domain)::text)", name: "index_accounts_on_username_and_domain_lower", unique: true t.index ["moved_to_account_id"], name: "index_accounts_on_moved_to_account_id" diff --git a/spec/lib/activitypub/activity/move_spec.rb b/spec/lib/activitypub/activity/move_spec.rb new file mode 100644 index 000000000..3574f273a --- /dev/null +++ b/spec/lib/activitypub/activity/move_spec.rb @@ -0,0 +1,52 @@ +require 'rails_helper' + +RSpec.describe ActivityPub::Activity::Move do + let(:follower) { Fabricate(:account) } + let(:old_account) { Fabricate(:account) } + let(:new_account) { Fabricate(:account) } + + before do + follower.follow!(old_account) + + old_account.update!(uri: 'https://example.org/alice', domain: 'example.org', protocol: :activitypub, inbox_url: 'https://example.org/inbox') + new_account.update!(uri: 'https://example.com/alice', domain: 'example.com', protocol: :activitypub, inbox_url: 'https://example.com/inbox', also_known_as: [old_account.uri]) + + stub_request(:post, 'https://example.org/inbox').to_return(status: 200) + stub_request(:post, 'https://example.com/inbox').to_return(status: 200) + + service_stub = double + allow(ActivityPub::FetchRemoteAccountService).to receive(:new).and_return(service_stub) + allow(service_stub).to receive(:call).and_return(new_account) + end + + let(:json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'foo', + type: 'Move', + actor: old_account.uri, + object: old_account.uri, + target: new_account.uri, + }.with_indifferent_access + end + + describe '#perform' do + subject { described_class.new(json, old_account) } + + before do + subject.perform + end + + it 'sets moved account on old account' do + expect(old_account.reload.moved_to_account_id).to eq new_account.id + end + + it 'makes followers unfollow old account' do + expect(follower.following?(old_account)).to be false + end + + it 'makes followers follow-request the new account' do + expect(follower.requested?(new_account)).to be true + end + end +end -- cgit From 4725aeec9f6d91d099e93ac13b3be8df5200e5e6 Mon Sep 17 00:00:00 2001 From: ysksn Date: Sat, 29 Dec 2018 15:22:51 +0900 Subject: Add specs for DisallowedHashtagsValidator (#9653) In order to implement tests easier, `#select_tags` created. --- app/validators/disallowed_hashtags_validator.rb | 9 ++++- .../disallowed_hashtags_validator_spec.rb | 46 ++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 spec/validators/disallowed_hashtags_validator_spec.rb (limited to 'spec') diff --git a/app/validators/disallowed_hashtags_validator.rb b/app/validators/disallowed_hashtags_validator.rb index 22c027b0f..ee06b20f6 100644 --- a/app/validators/disallowed_hashtags_validator.rb +++ b/app/validators/disallowed_hashtags_validator.rb @@ -4,14 +4,19 @@ class DisallowedHashtagsValidator < ActiveModel::Validator def validate(status) return unless status.local? && !status.reblog? - tags = Extractor.extract_hashtags(status.text) - tags.keep_if { |tag| disallowed_hashtags.include? tag.downcase } + @status = status + tags = select_tags status.errors.add(:text, I18n.t('statuses.disallowed_hashtags', tags: tags.join(', '), count: tags.size)) unless tags.empty? end private + def select_tags + tags = Extractor.extract_hashtags(@status.text) + tags.keep_if { |tag| disallowed_hashtags.include? tag.downcase } + end + def disallowed_hashtags return @disallowed_hashtags if @disallowed_hashtags diff --git a/spec/validators/disallowed_hashtags_validator_spec.rb b/spec/validators/disallowed_hashtags_validator_spec.rb new file mode 100644 index 000000000..8ec1302ab --- /dev/null +++ b/spec/validators/disallowed_hashtags_validator_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe DisallowedHashtagsValidator, type: :validator do + describe '#validate' do + before do + allow_any_instance_of(described_class).to receive(:select_tags) { tags } + described_class.new.validate(status) + end + + let(:status) { double(errors: errors, local?: local, reblog?: reblog, text: '') } + let(:errors) { double(add: nil) } + + context 'unless status.local? && !status.reblog?' do + let(:local) { false } + let(:reblog) { true } + + it 'not calls errors.add' do + expect(errors).not_to have_received(:add).with(:text, any_args) + end + end + + context 'status.local? && !status.reblog?' do + let(:local) { true } + let(:reblog) { false } + + context 'tags.empty?' do + let(:tags) { [] } + + it 'not calls errors.add' do + expect(errors).not_to have_received(:add).with(:text, any_args) + end + end + + context '!tags.empty?' do + let(:tags) { %w(a b c) } + + it 'calls errors.add' do + expect(errors).to have_received(:add) + .with(:text, I18n.t('statuses.disallowed_hashtags', tags: tags.join(', '), count: tags.size)) + end + end + end + end +end -- cgit From 05edec69172f553397164b4e0a71c798124b8f19 Mon Sep 17 00:00:00 2001 From: ysksn Date: Sat, 29 Dec 2018 15:23:44 +0900 Subject: Add specs for BlackListedEmailValidator (#9651) * Add specs for BlackListedEmailValidator * Use instance variable --- app/validators/blacklisted_email_validator.rb | 17 ++++++------ .../validators/blacklisted_email_validator_spec.rb | 31 ++++++++++++++++++++++ 2 files changed, 40 insertions(+), 8 deletions(-) create mode 100644 spec/validators/blacklisted_email_validator_spec.rb (limited to 'spec') diff --git a/app/validators/blacklisted_email_validator.rb b/app/validators/blacklisted_email_validator.rb index 3f203f49a..a2061fdd3 100644 --- a/app/validators/blacklisted_email_validator.rb +++ b/app/validators/blacklisted_email_validator.rb @@ -2,31 +2,32 @@ class BlacklistedEmailValidator < ActiveModel::Validator def validate(user) - user.errors.add(:email, I18n.t('users.invalid_email')) if blocked_email?(user.email) + @email = user.email + user.errors.add(:email, I18n.t('users.invalid_email')) if blocked_email? end private - def blocked_email?(value) - on_blacklist?(value) || not_on_whitelist?(value) + def blocked_email? + on_blacklist? || not_on_whitelist? end - def on_blacklist?(value) - return true if EmailDomainBlock.block?(value) + def on_blacklist? + return true if EmailDomainBlock.block?(@email) return false if Rails.configuration.x.email_domains_blacklist.blank? domains = Rails.configuration.x.email_domains_blacklist.gsub('.', '\.') regexp = Regexp.new("@(.+\\.)?(#{domains})", true) - value =~ regexp + @email =~ regexp end - def not_on_whitelist?(value) + def not_on_whitelist? return false if Rails.configuration.x.email_domains_whitelist.blank? domains = Rails.configuration.x.email_domains_whitelist.gsub('.', '\.') regexp = Regexp.new("@(.+\\.)?(#{domains})$", true) - value !~ regexp + @email !~ regexp end end diff --git a/spec/validators/blacklisted_email_validator_spec.rb b/spec/validators/blacklisted_email_validator_spec.rb new file mode 100644 index 000000000..d2e442f4a --- /dev/null +++ b/spec/validators/blacklisted_email_validator_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe BlacklistedEmailValidator, type: :validator do + describe '#validate' do + let(:user) { double(email: 'info@mail.com', errors: errors) } + let(:errors) { double(add: nil) } + + before do + allow_any_instance_of(described_class).to receive(:blocked_email?) { blocked_email } + described_class.new.validate(user) + end + + context 'blocked_email?' do + let(:blocked_email) { true } + + it 'calls errors.add' do + expect(errors).to have_received(:add).with(:email, I18n.t('users.invalid_email')) + end + end + + context '!blocked_email?' do + let(:blocked_email) { false } + + it 'not calls errors.add' do + expect(errors).not_to have_received(:add).with(:email, I18n.t('users.invalid_email')) + end + end + end +end -- cgit From fb08039de58bee131e66c1a41db9d9f1f831d6e9 Mon Sep 17 00:00:00 2001 From: ysksn Date: Sat, 29 Dec 2018 16:24:52 +0900 Subject: Add specs for FollowLimitValidator (#9655) --- spec/validators/follow_limit_validator_spec.rb | 51 ++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 spec/validators/follow_limit_validator_spec.rb (limited to 'spec') diff --git a/spec/validators/follow_limit_validator_spec.rb b/spec/validators/follow_limit_validator_spec.rb new file mode 100644 index 000000000..cc8fbb631 --- /dev/null +++ b/spec/validators/follow_limit_validator_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe FollowLimitValidator, type: :validator do + describe '#validate' do + before do + allow_any_instance_of(described_class).to receive(:limit_reached?).with(account) do + limit_reached + end + + described_class.new.validate(follow) + end + + let(:follow) { double(account: account, errors: errors) } + let(:errors) { double(add: nil) } + let(:account) { double(nil?: _nil, local?: local, following_count: 0, followers_count: 0) } + let(:_nil) { true } + let(:local) { false } + + context 'follow.account.nil? || !follow.account.local?' do + let(:_nil) { true } + + it 'not calls errors.add' do + expect(errors).not_to have_received(:add).with(:base, any_args) + end + end + + context '!(follow.account.nil? || !follow.account.local?)' do + let(:_nil) { false } + let(:local) { true } + + context 'limit_reached?' do + let(:limit_reached) { true } + + it 'calls errors.add' do + expect(errors).to have_received(:add) + .with(:base, I18n.t('users.follow_limit_reached', limit: FollowLimitValidator::LIMIT)) + end + end + + context '!limit_reached?' do + let(:limit_reached) { false } + + it 'not calls errors.add' do + expect(errors).not_to have_received(:add).with(:base, any_args) + end + end + end + end +end -- cgit From 290932602b32e3bdc2a4c5cb278fb73755a2cd52 Mon Sep 17 00:00:00 2001 From: ThibG Date: Sun, 30 Dec 2018 09:48:59 +0100 Subject: Reduce usage of LD signatures (#9659) * Do not LDS-sign Follow, Accept, Reject, Undo, Block * Do not use LDS for Create activities of private toots * Minor cleanup * Ignore unsigned activities instead of misattributing them * Use status.distributable? instead of querying visibility directly --- app/lib/activitypub/activity/follow.rb | 2 +- app/services/activitypub/process_collection_service.rb | 2 +- app/services/after_block_domain_from_account_service.rb | 4 ++-- app/services/authorize_follow_service.rb | 4 ++-- app/services/block_service.rb | 6 ++---- app/services/follow_service.rb | 6 ++---- app/services/process_mentions_service.rb | 6 ++++-- app/services/reject_follow_service.rb | 4 ++-- app/services/unblock_service.rb | 4 ++-- app/services/unfollow_service.rb | 4 ++-- app/workers/activitypub/distribution_worker.rb | 14 +++++++++----- app/workers/activitypub/reply_distribution_worker.rb | 12 ++++++++---- .../activitypub/process_collection_service_spec.rb | 6 +++--- 13 files changed, 40 insertions(+), 34 deletions(-) (limited to 'spec') diff --git a/app/lib/activitypub/activity/follow.rb b/app/lib/activitypub/activity/follow.rb index 7ca650fdf..1e805c0d1 100644 --- a/app/lib/activitypub/activity/follow.rb +++ b/app/lib/activitypub/activity/follow.rb @@ -28,7 +28,7 @@ class ActivityPub::Activity::Follow < ActivityPub::Activity end def reject_follow_request!(target_account) - json = Oj.dump(ActivityPub::LinkedDataSignature.new(ActiveModelSerializers::SerializableResource.new(FollowRequest.new(account: @account, target_account: target_account, uri: @json['id']), serializer: ActivityPub::RejectFollowSerializer, adapter: ActivityPub::Adapter).as_json).sign!(target_account)) + json = ActiveModelSerializers::SerializableResource.new(FollowRequest.new(account: @account, target_account: target_account, uri: @json['id']), serializer: ActivityPub::RejectFollowSerializer, adapter: ActivityPub::Adapter).to_json ActivityPub::DeliveryWorker.perform_async(json, target_account.id, @account.inbox_url) end end diff --git a/app/services/activitypub/process_collection_service.rb b/app/services/activitypub/process_collection_service.rb index 79cdca297..5c54aad89 100644 --- a/app/services/activitypub/process_collection_service.rb +++ b/app/services/activitypub/process_collection_service.rb @@ -27,7 +27,7 @@ class ActivityPub::ProcessCollectionService < BaseService private def different_actor? - @json['actor'].present? && value_or_id(@json['actor']) != @account.uri && @json['signature'].present? + @json['actor'].present? && value_or_id(@json['actor']) != @account.uri end def process_items(items) diff --git a/app/services/after_block_domain_from_account_service.rb b/app/services/after_block_domain_from_account_service.rb index 56cc819fb..180f13403 100644 --- a/app/services/after_block_domain_from_account_service.rb +++ b/app/services/after_block_domain_from_account_service.rb @@ -31,11 +31,11 @@ class AfterBlockDomainFromAccountService < BaseService return unless follow.account.activitypub? - json = Oj.dump(ActivityPub::LinkedDataSignature.new(ActiveModelSerializers::SerializableResource.new( + json = ActiveModelSerializers::SerializableResource.new( follow, serializer: ActivityPub::RejectFollowSerializer, adapter: ActivityPub::Adapter - ).as_json).sign!(@account)) + ).to_json ActivityPub::DeliveryWorker.perform_async(json, @account.id, follow.account.inbox_url) end diff --git a/app/services/authorize_follow_service.rb b/app/services/authorize_follow_service.rb index 1674239df..f2e3ebe7d 100644 --- a/app/services/authorize_follow_service.rb +++ b/app/services/authorize_follow_service.rb @@ -24,11 +24,11 @@ class AuthorizeFollowService < BaseService end def build_json(follow_request) - Oj.dump(ActivityPub::LinkedDataSignature.new(ActiveModelSerializers::SerializableResource.new( + ActiveModelSerializers::SerializableResource.new( follow_request, serializer: ActivityPub::AcceptFollowSerializer, adapter: ActivityPub::Adapter - ).as_json).sign!(follow_request.target_account)) + ).to_json end def build_xml(follow_request) diff --git a/app/services/block_service.rb b/app/services/block_service.rb index b39c3eef2..140b238df 100644 --- a/app/services/block_service.rb +++ b/app/services/block_service.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class BlockService < BaseService - include StreamEntryRenderer - def call(account, target_account) return if account.id == target_account.id @@ -27,11 +25,11 @@ class BlockService < BaseService end def build_json(block) - Oj.dump(ActivityPub::LinkedDataSignature.new(ActiveModelSerializers::SerializableResource.new( + ActiveModelSerializers::SerializableResource.new( block, serializer: ActivityPub::BlockSerializer, adapter: ActivityPub::Adapter - ).as_json).sign!(block.account)) + ).to_json end def build_xml(block) diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb index 862926260..9d36a1449 100644 --- a/app/services/follow_service.rb +++ b/app/services/follow_service.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class FollowService < BaseService - include StreamEntryRenderer - # Follow a remote user, notify remote user about the follow # @param [Account] source_account From which to follow # @param [String, Account] uri User URI to follow in the form of username@domain (or account record) @@ -82,10 +80,10 @@ class FollowService < BaseService end def build_json(follow_request) - Oj.dump(ActivityPub::LinkedDataSignature.new(ActiveModelSerializers::SerializableResource.new( + ActiveModelSerializers::SerializableResource.new( follow_request, serializer: ActivityPub::FollowSerializer, adapter: ActivityPub::Adapter - ).as_json).sign!(follow_request.account)) + ).to_json end end diff --git a/app/services/process_mentions_service.rb b/app/services/process_mentions_service.rb index ec7d33b1d..2595c5fd3 100644 --- a/app/services/process_mentions_service.rb +++ b/app/services/process_mentions_service.rb @@ -60,11 +60,13 @@ class ProcessMentionsService < BaseService end def activitypub_json - @activitypub_json ||= Oj.dump(ActivityPub::LinkedDataSignature.new(ActiveModelSerializers::SerializableResource.new( + return @activitypub_json if defined?(@activitypub_json) + payload = ActiveModelSerializers::SerializableResource.new( @status, serializer: ActivityPub::ActivitySerializer, adapter: ActivityPub::Adapter - ).as_json).sign!(@status.account)) + ).as_json + @activitypub_json = Oj.dump(@status.distributable? ? ActivityPub::LinkedDataSignature.new(payload).sign!(@status.account) : payload) end def resolve_account_service diff --git a/app/services/reject_follow_service.rb b/app/services/reject_follow_service.rb index c1f7bcb60..a91266aa4 100644 --- a/app/services/reject_follow_service.rb +++ b/app/services/reject_follow_service.rb @@ -19,11 +19,11 @@ class RejectFollowService < BaseService end def build_json(follow_request) - Oj.dump(ActivityPub::LinkedDataSignature.new(ActiveModelSerializers::SerializableResource.new( + ActiveModelSerializers::SerializableResource.new( follow_request, serializer: ActivityPub::RejectFollowSerializer, adapter: ActivityPub::Adapter - ).as_json).sign!(follow_request.target_account)) + ).to_json end def build_xml(follow_request) diff --git a/app/services/unblock_service.rb b/app/services/unblock_service.rb index 869f62d1c..72fc5ab15 100644 --- a/app/services/unblock_service.rb +++ b/app/services/unblock_service.rb @@ -20,11 +20,11 @@ class UnblockService < BaseService end def build_json(unblock) - Oj.dump(ActivityPub::LinkedDataSignature.new(ActiveModelSerializers::SerializableResource.new( + ActiveModelSerializers::SerializableResource.new( unblock, serializer: ActivityPub::UndoBlockSerializer, adapter: ActivityPub::Adapter - ).as_json).sign!(unblock.account)) + ).to_json end def build_xml(block) diff --git a/app/services/unfollow_service.rb b/app/services/unfollow_service.rb index 73a64929f..03e45912d 100644 --- a/app/services/unfollow_service.rb +++ b/app/services/unfollow_service.rb @@ -43,11 +43,11 @@ class UnfollowService < BaseService end def build_json(follow) - Oj.dump(ActivityPub::LinkedDataSignature.new(ActiveModelSerializers::SerializableResource.new( + ActiveModelSerializers::SerializableResource.new( follow, serializer: ActivityPub::UndoFollowSerializer, adapter: ActivityPub::Adapter - ).as_json).sign!(follow.account)) + ).to_json end def build_xml(follow) diff --git a/app/workers/activitypub/distribution_worker.rb b/app/workers/activitypub/distribution_worker.rb index 17c1ef7ff..59aacabfd 100644 --- a/app/workers/activitypub/distribution_worker.rb +++ b/app/workers/activitypub/distribution_worker.rb @@ -12,7 +12,7 @@ class ActivityPub::DistributionWorker return if skip_distribution? ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url| - [signed_payload, @account.id, inbox_url] + [payload, @account.id, inbox_url] end relay! if relayable? @@ -35,20 +35,24 @@ class ActivityPub::DistributionWorker end def signed_payload - @signed_payload ||= Oj.dump(ActivityPub::LinkedDataSignature.new(payload).sign!(@account)) + Oj.dump(ActivityPub::LinkedDataSignature.new(unsigned_payload).sign!(@account)) end - def payload - @payload ||= ActiveModelSerializers::SerializableResource.new( + def unsigned_payload + ActiveModelSerializers::SerializableResource.new( @status, serializer: ActivityPub::ActivitySerializer, adapter: ActivityPub::Adapter ).as_json end + def payload + @payload ||= @status.distributable? ? signed_payload : Oj.dump(unsigned_payload) + end + def relay! ActivityPub::DeliveryWorker.push_bulk(Relay.enabled.pluck(:inbox_url)) do |inbox_url| - [signed_payload, @account.id, inbox_url] + [payload, @account.id, inbox_url] end end end diff --git a/app/workers/activitypub/reply_distribution_worker.rb b/app/workers/activitypub/reply_distribution_worker.rb index c0ed3a1f4..892cc1474 100644 --- a/app/workers/activitypub/reply_distribution_worker.rb +++ b/app/workers/activitypub/reply_distribution_worker.rb @@ -12,7 +12,7 @@ class ActivityPub::ReplyDistributionWorker return unless @account.present? && @status.distributable? ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url| - [signed_payload, @status.account_id, inbox_url] + [payload, @status.account_id, inbox_url] end rescue ActiveRecord::RecordNotFound true @@ -25,14 +25,18 @@ class ActivityPub::ReplyDistributionWorker end def signed_payload - @signed_payload ||= Oj.dump(ActivityPub::LinkedDataSignature.new(payload).sign!(@status.account)) + Oj.dump(ActivityPub::LinkedDataSignature.new(unsigned_payload).sign!(@status.account)) end - def payload - @payload ||= ActiveModelSerializers::SerializableResource.new( + def unsigned_payload + ActiveModelSerializers::SerializableResource.new( @status, serializer: ActivityPub::ActivitySerializer, adapter: ActivityPub::Adapter ).as_json end + + def payload + @payload ||= @status.distributable? ? signed_payload : Oj.dump(unsigned_payload) + end end diff --git a/spec/services/activitypub/process_collection_service_spec.rb b/spec/services/activitypub/process_collection_service_spec.rb index bbe97d211..b3baf6b6b 100644 --- a/spec/services/activitypub/process_collection_service_spec.rb +++ b/spec/services/activitypub/process_collection_service_spec.rb @@ -26,9 +26,9 @@ RSpec.describe ActivityPub::ProcessCollectionService, type: :service do context 'when actor differs from sender' do let(:forwarder) { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/other_account') } - it 'processes payload with sender if no signature exists' do - expect_any_instance_of(ActivityPub::LinkedDataSignature).not_to receive(:verify_account!) - expect(ActivityPub::Activity).to receive(:factory).with(instance_of(Hash), forwarder, instance_of(Hash)) + it 'does not process payload if no signature exists' do + expect_any_instance_of(ActivityPub::LinkedDataSignature).to receive(:verify_account!).and_return(nil) + expect(ActivityPub::Activity).not_to receive(:factory) subject.call(json, forwarder) end -- cgit From 70be301d6988c229b18e165b6501f988359181ff Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 2 Jan 2019 01:12:02 +0100 Subject: Ensure blocked user unfollows blocker if Block/Undo Block are processed out of order (#9687) * Ensure blocked user unfollows blocker if Block/Undo Block are processed out of order * Add specs for Block causing unfollow and for out-of-order Block + Undo --- app/lib/activitypub/activity/block.rb | 5 ++- spec/lib/activitypub/activity/block_spec.rb | 67 ++++++++++++++++++++++++++--- 2 files changed, 65 insertions(+), 7 deletions(-) (limited to 'spec') diff --git a/app/lib/activitypub/activity/block.rb b/app/lib/activitypub/activity/block.rb index 26da8bdf5..a17a2d50a 100644 --- a/app/lib/activitypub/activity/block.rb +++ b/app/lib/activitypub/activity/block.rb @@ -4,9 +4,10 @@ class ActivityPub::Activity::Block < ActivityPub::Activity def perform target_account = account_from_uri(object_uri) - return if target_account.nil? || !target_account.local? || delete_arrived_first?(@json['id']) || @account.blocking?(target_account) + return if target_account.nil? || !target_account.local? || @account.blocking?(target_account) UnfollowService.new.call(target_account, @account) if target_account.following?(@account) - @account.block!(target_account, uri: @json['id']) + + @account.block!(target_account, uri: @json['id']) unless delete_arrived_first?(@json['id']) end end diff --git a/spec/lib/activitypub/activity/block_spec.rb b/spec/lib/activitypub/activity/block_spec.rb index 23c8cc31c..94d37356d 100644 --- a/spec/lib/activitypub/activity/block_spec.rb +++ b/spec/lib/activitypub/activity/block_spec.rb @@ -14,15 +14,72 @@ RSpec.describe ActivityPub::Activity::Block do }.with_indifferent_access end - describe '#perform' do - subject { described_class.new(json, sender) } + context 'when the recipient does not follow the sender' do + describe '#perform' do + subject { described_class.new(json, sender) } + + before do + subject.perform + end + + it 'creates a block from sender to recipient' do + expect(sender.blocking?(recipient)).to be true + end + end + end + + context 'when the recipient follows the sender' do + before do + recipient.follow!(sender) + end + + describe '#perform' do + subject { described_class.new(json, sender) } + + before do + subject.perform + end + + it 'creates a block from sender to recipient' do + expect(sender.blocking?(recipient)).to be true + end + + it 'ensures recipient is not following sender' do + expect(recipient.following?(sender)).to be false + end + end + end + + context 'when a matching undo has been received first' do + let(:undo_json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'bar', + type: 'Undo', + actor: ActivityPub::TagManager.instance.uri_for(sender), + object: json, + }.with_indifferent_access + end before do - subject.perform + recipient.follow!(sender) + ActivityPub::Activity::Undo.new(undo_json, sender).perform end - it 'creates a block from sender to recipient' do - expect(sender.blocking?(recipient)).to be true + describe '#perform' do + subject { described_class.new(json, sender) } + + before do + subject.perform + end + + it 'does not create a block from sender to recipient' do + expect(sender.blocking?(recipient)).to be false + end + + it 'ensures recipient is not following sender' do + expect(recipient.following?(sender)).to be false + end end end end -- cgit From 66436d08959998be20c6c6bf631177d8c1f3e0d1 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 2 Jan 2019 10:47:32 +0100 Subject: Improve e-mail digest (#9689) - Reduce time-to-digest from 20 to 7 days - Fetch mentions starting from +1 day since last login - Fix case when last login is more recent than last e-mail - Do not render all mentions, only 40, but show number in subject - Do not send digest to moved accounts - Do send digest to silenced accounts --- app/mailers/notification_mailer.rb | 16 ++++++++++------ app/models/account.rb | 5 +++-- app/models/user.rb | 4 +++- app/views/notification_mailer/digest.html.haml | 2 +- app/views/notification_mailer/digest.text.erb | 2 +- app/workers/scheduler/email_scheduler.rb | 12 ++++++------ spec/mailers/notification_mailer_spec.rb | 14 +------------- 7 files changed, 25 insertions(+), 30 deletions(-) (limited to 'spec') diff --git a/app/mailers/notification_mailer.rb b/app/mailers/notification_mailer.rb index b45844296..66fa337c1 100644 --- a/app/mailers/notification_mailer.rb +++ b/app/mailers/notification_mailer.rb @@ -66,16 +66,20 @@ class NotificationMailer < ApplicationMailer end def digest(recipient, **opts) - @me = recipient - @since = opts[:since] || @me.user.last_emailed_at || @me.user.current_sign_in_at - @notifications = Notification.where(account: @me, activity_type: 'Mention').where('created_at > ?', @since) - @follows_since = Notification.where(account: @me, activity_type: 'Follow').where('created_at > ?', @since).count + return if recipient.user.disabled? + + @me = recipient + @since = opts[:since] || [@me.user.last_emailed_at, (@me.user.current_sign_in_at + 1.day)].compact.max + @notifications_count = Notification.where(account: @me, activity_type: 'Mention').where('created_at > ?', @since).count - return if @me.user.disabled? || @notifications.empty? + return if @notifications_count.zero? + + @notifications = Notification.where(account: @me, activity_type: 'Mention').where('created_at > ?', @since).limit(40) + @follows_since = Notification.where(account: @me, activity_type: 'Follow').where('created_at > ?', @since).count locale_for_account(@me) do mail to: @me.user.email, - subject: I18n.t(:subject, scope: [:notification_mailer, :digest], count: @notifications.size) + subject: I18n.t(:subject, scope: [:notification_mailer, :digest], count: @notifications_count) end end diff --git a/app/models/account.rb b/app/models/account.rb index f354bc29b..97beb416a 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -85,6 +85,7 @@ class Account < ApplicationRecord scope :silenced, -> { where(silenced: true) } scope :suspended, -> { where(suspended: true) } scope :without_suspended, -> { where(suspended: false) } + scope :without_silenced, -> { where(silenced: false) } scope :recent, -> { reorder(id: :desc) } scope :bots, -> { where(actor_type: %w(Application Service)) } scope :alphabetic, -> { order(domain: :asc, username: :asc) } @@ -92,8 +93,8 @@ class Account < ApplicationRecord scope :matches_username, ->(value) { where(arel_table[:username].matches("#{value}%")) } scope :matches_display_name, ->(value) { where(arel_table[:display_name].matches("#{value}%")) } scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) } - scope :searchable, -> { where(suspended: false).where(moved_to_account_id: nil) } - scope :discoverable, -> { searchable.where(silenced: false).where(discoverable: true).joins(:account_stat).where(AccountStat.arel_table[:followers_count].gteq(MIN_FOLLOWERS_DISCOVERY)).by_recent_status } + scope :searchable, -> { without_suspended.where(moved_to_account_id: nil) } + scope :discoverable, -> { searchable.without_silenced.where(discoverable: true).joins(:account_stat).where(AccountStat.arel_table[:followers_count].gteq(MIN_FOLLOWERS_DISCOVERY)).by_recent_status } scope :tagged_with, ->(tag) { joins(:accounts_tags).where(accounts_tags: { tag_id: tag }) } scope :by_recent_status, -> { order(Arel.sql('(case when account_stats.last_status_at is null then 1 else 0 end) asc, account_stats.last_status_at desc')) } scope :popular, -> { order('account_stats.followers_count desc') } diff --git a/app/models/user.rb b/app/models/user.rb index 1684b9bea..8b374c182 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -50,7 +50,7 @@ class User < ApplicationRecord # every day. Raising the duration reduces the amount of expensive # RegenerationWorker jobs that need to be run when those people come # to check their feed - ACTIVE_DURATION = ENV.fetch('USER_ACTIVE_DAYS', 7).to_i.days + ACTIVE_DURATION = ENV.fetch('USER_ACTIVE_DAYS', 7).to_i.days.freeze devise :two_factor_authenticatable, otp_secret_encryption_key: Rails.configuration.x.otp_secret @@ -83,9 +83,11 @@ class User < ApplicationRecord scope :moderators, -> { where(moderator: true) } scope :staff, -> { admins.or(moderators) } scope :confirmed, -> { where.not(confirmed_at: nil) } + scope :enabled, -> { where(disabled: false) } scope :inactive, -> { where(arel_table[:current_sign_in_at].lt(ACTIVE_DURATION.ago)) } scope :active, -> { confirmed.where(arel_table[:current_sign_in_at].gteq(ACTIVE_DURATION.ago)).joins(:account).where(accounts: { suspended: false }) } scope :matches_email, ->(value) { where(arel_table[:email].matches("#{value}%")) } + scope :emailable, -> { confirmed.enabled.joins(:account).merge(Account.searchable) } before_validation :sanitize_languages diff --git a/app/views/notification_mailer/digest.html.haml b/app/views/notification_mailer/digest.html.haml index 10e44f8dd..a94ace228 100644 --- a/app/views/notification_mailer/digest.html.haml +++ b/app/views/notification_mailer/digest.html.haml @@ -14,7 +14,7 @@ %tr %td.column-cell.text-center.padded %h1= t 'notification_mailer.digest.title' - %p.lead= t('notification_mailer.digest.body', since: l(@since.to_date, format: :short), instance: site_hostname) + %p.lead= t('notification_mailer.digest.body', since: l((@me.user_current_sign_in_at || @since).to_date, format: :short), instance: site_hostname) %table.button{ align: 'center', cellspacing: 0, cellpadding: 0 } %tbody %tr diff --git a/app/views/notification_mailer/digest.text.erb b/app/views/notification_mailer/digest.text.erb index e0d1f9b8b..b2c85a9e3 100644 --- a/app/views/notification_mailer/digest.text.erb +++ b/app/views/notification_mailer/digest.text.erb @@ -1,6 +1,6 @@ <%= raw t('application_mailer.salutation', name: display_name(@me)) %> -<%= raw t('notification_mailer.digest.body', since: l(@since), instance: root_url) %> +<%= raw t('notification_mailer.digest.body', since: l(@me.user_current_sign_in_at || @since), instance: root_url) %> <% @notifications.each do |notification| %> * <%= raw t('notification_mailer.digest.mention', name: notification.from_account.acct) %> diff --git a/app/workers/scheduler/email_scheduler.rb b/app/workers/scheduler/email_scheduler.rb index 24ec89b29..1eeeee412 100644 --- a/app/workers/scheduler/email_scheduler.rb +++ b/app/workers/scheduler/email_scheduler.rb @@ -5,6 +5,9 @@ class Scheduler::EmailScheduler sidekiq_options unique: :until_executed, retry: 0 + FREQUENCY = 7.days.freeze + SIGN_IN_OFFSET = 1.day.freeze + def perform eligible_users.reorder(nil).find_each do |user| next unless user.allows_digest_emails? @@ -15,11 +18,8 @@ class Scheduler::EmailScheduler private def eligible_users - User.confirmed - .joins(:account) - .where(accounts: { silenced: false, suspended: false }) - .where(disabled: false) - .where('current_sign_in_at < ?', 20.days.ago) - .where('last_emailed_at IS NULL OR last_emailed_at < ?', 20.days.ago) + User.emailable + .where('current_sign_in_at < ?', (FREQUENCY + SIGN_IN_OFFSET).ago) + .where('last_emailed_at IS NULL OR last_emailed_at < ?', FREQUENCY.ago) end end diff --git a/spec/mailers/notification_mailer_spec.rb b/spec/mailers/notification_mailer_spec.rb index 1be01e8ba..38916b54f 100644 --- a/spec/mailers/notification_mailer_spec.rb +++ b/spec/mailers/notification_mailer_spec.rb @@ -126,19 +126,7 @@ RSpec.describe NotificationMailer, type: :mailer do end end - it 'includes activities since the date specified by :since option' do - receiver.update!(last_emailed_at: '2000-02-01T00:00:00Z', current_sign_in_at: '2000-03-01T00:00:00Z') - mail = NotificationMailer.digest(receiver.account, since: Time.parse('2000-01-01T00:00:00Z')) - expect(mail.body.encoded).to include 'Jan 01, 2000, 00:00' - end - - it 'includes activities since the receiver was last emailed if :since option is unavailable' do - receiver.update!(last_emailed_at: '2000-02-01T00:00:00Z', current_sign_in_at: '2000-03-01T00:00:00Z') - mail = NotificationMailer.digest(receiver.account) - expect(mail.body.encoded).to include 'Feb 01, 2000, 00:00' - end - - it 'includes activities since the receiver last signed in if :since option and the last emailed date are unavailable' do + it 'includes activities since the receiver last signed in' do receiver.update!(last_emailed_at: nil, current_sign_in_at: '2000-03-01T00:00:00Z') mail = NotificationMailer.digest(receiver.account) expect(mail.body.encoded).to include 'Mar 01, 2000, 00:00' -- cgit