From 4ec1771165ab8dd40e52804fd087eacfab25290b Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 28 Sep 2017 15:31:31 +0200 Subject: Add ability to specify alternative text for media attachments (#5123) * Fix #117 - Add ability to specify alternative text for media attachments - POST /api/v1/media accepts `description` straight away - PUT /api/v1/media/:id to update `description` (only for unattached ones) - Serialized as `name` of Document object in ActivityPub - Uploads form adjusted for better performance and description input * Add tests * Change undo button blend mode to difference --- app/controllers/api/v1/media_controller.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/api/v1/media_controller.rb b/app/controllers/api/v1/media_controller.rb index 8a1992fca..9f330f0df 100644 --- a/app/controllers/api/v1/media_controller.rb +++ b/app/controllers/api/v1/media_controller.rb @@ -10,7 +10,7 @@ class Api::V1::MediaController < Api::BaseController respond_to :json def create - @media = current_account.media_attachments.create!(file: media_params[:file]) + @media = current_account.media_attachments.create!(media_params) render json: @media, serializer: REST::MediaAttachmentSerializer rescue Paperclip::Errors::NotIdentifiedByImageMagickError render json: file_type_error, status: 422 @@ -18,10 +18,16 @@ class Api::V1::MediaController < Api::BaseController render json: processing_error, status: 500 end + def update + @media = current_account.media_attachments.where(status_id: nil).find(params[:id]) + @media.update!(media_params) + render json: @media, serializer: REST::MediaAttachmentSerializer + end + private def media_params - params.permit(:file) + params.permit(:file, :description) end def file_type_error -- cgit From 76f360c625d6f7e1200a35430cced872fc6098ff Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 28 Sep 2017 17:50:14 +0200 Subject: If HTTP signature is wrong and webfinger cache is stale, retry with resolve (#5129) If the signature could not be verified and the webfinger of the account was last retrieved longer than the cache period, try re-resolving the account and then attempting to verify the signature again --- app/controllers/concerns/signature_verification.rb | 9 +++++++++ app/models/account.rb | 9 +++++++++ app/services/resolve_remote_account_service.rb | 2 +- 3 files changed, 19 insertions(+), 1 deletion(-) (limited to 'app/controllers') diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 4211283ed..52a9cf290 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -44,6 +44,15 @@ module SignatureVerification if account.keypair.public_key.verify(OpenSSL::Digest::SHA256.new, signature, compare_signed_string) @signed_request_account = account @signed_request_account + elsif account.possibly_stale? + account = account.refresh! + + if account.keypair.public_key.verify(OpenSSL::Digest::SHA256.new, signature, compare_signed_string) + @signed_request_account = account + @signed_request_account + else + @signed_request_account = nil + end else @signed_request_account = nil end diff --git a/app/models/account.rb b/app/models/account.rb index 0b025d1be..ce7773b4b 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -137,6 +137,15 @@ class Account < ApplicationRecord subscription_expires_at.present? end + def possibly_stale? + last_webfingered_at.nil? || last_webfingered_at <= 1.day.ago + end + + def refresh! + return if local? + ResolveRemoteAccountService.new.call(acct) + end + def keypair @keypair ||= OpenSSL::PKey::RSA.new(private_key || public_key) end diff --git a/app/services/resolve_remote_account_service.rb b/app/services/resolve_remote_account_service.rb index 57c80fc82..93ba07702 100644 --- a/app/services/resolve_remote_account_service.rb +++ b/app/services/resolve_remote_account_service.rb @@ -74,7 +74,7 @@ class ResolveRemoteAccountService < BaseService end def webfinger_update_due? - @account.nil? || @account.last_webfingered_at.nil? || @account.last_webfingered_at <= 1.day.ago + @account.nil? || @account.possibly_stale? end def activitypub_ready? -- cgit From f4ca116ea8f86057e91c99a1cd8e64e116c86746 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 29 Sep 2017 03:16:20 +0200 Subject: After 7 days of repeated delivery failures, give up on inbox (#5131) - A successful delivery cancels it out - An incoming delivery from account of the inbox cancels it out --- app/controllers/activitypub/inboxes_controller.rb | 1 + app/lib/delivery_failure_tracker.rb | 56 ++++++++++++++++++ app/models/account.rb | 3 +- app/workers/activitypub/delivery_worker.rb | 7 +++ spec/lib/delivery_failure_tracker_spec.rb | 71 +++++++++++++++++++++++ 5 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 app/lib/delivery_failure_tracker.rb create mode 100644 spec/lib/delivery_failure_tracker_spec.rb (limited to 'app/controllers') diff --git a/app/controllers/activitypub/inboxes_controller.rb b/app/controllers/activitypub/inboxes_controller.rb index b37910b36..d0f8073ed 100644 --- a/app/controllers/activitypub/inboxes_controller.rb +++ b/app/controllers/activitypub/inboxes_controller.rb @@ -32,6 +32,7 @@ class ActivityPub::InboxesController < Api::BaseController end Pubsubhubbub::UnsubscribeWorker.perform_async(signed_request_account.id) if signed_request_account.subscribed? + DeliveryFailureTracker.track_inverse_success!(signed_request_account) end def process_payload diff --git a/app/lib/delivery_failure_tracker.rb b/app/lib/delivery_failure_tracker.rb new file mode 100644 index 000000000..8d3be35de --- /dev/null +++ b/app/lib/delivery_failure_tracker.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +class DeliveryFailureTracker + FAILURE_DAYS_THRESHOLD = 7 + + def initialize(inbox_url) + @inbox_url = inbox_url + end + + def track_failure! + Redis.current.sadd(exhausted_deliveries_key, today) + Redis.current.sadd('unavailable_inboxes', @inbox_url) if reached_failure_threshold? + end + + def track_success! + Redis.current.del(exhausted_deliveries_key) + Redis.current.srem('unavailable_inboxes', @inbox_url) + end + + def days + Redis.current.scard(exhausted_deliveries_key) || 0 + end + + class << self + def filter(arr) + arr.reject(&method(:unavailable?)) + end + + def unavailable?(url) + Redis.current.sismember('unavailable_inboxes', url) + end + + def available?(url) + !unavailable?(url) + end + + def track_inverse_success!(from_account) + new(from_account.inbox_url).track_success! if from_account.inbox_url.present? + new(from_account.shared_inbox_url).track_success! if from_account.shared_inbox_url.present? + end + end + + private + + def exhausted_deliveries_key + "exhausted_deliveries:#{@inbox_url}" + end + + def today + Time.now.utc.strftime('%Y%m%d') + end + + def reached_failure_threshold? + days >= FAILURE_DAYS_THRESHOLD + end +end diff --git a/app/models/account.rb b/app/models/account.rb index ce7773b4b..54035d94a 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -190,7 +190,8 @@ class Account < ApplicationRecord end def inboxes - reorder(nil).where(protocol: :activitypub).pluck("distinct coalesce(nullif(accounts.shared_inbox_url, ''), accounts.inbox_url)") + urls = reorder(nil).where(protocol: :activitypub).pluck("distinct coalesce(nullif(accounts.shared_inbox_url, ''), accounts.inbox_url)") + DeliveryFailureTracker.filter(urls) end def triadic_closures(account, limit: 5, offset: 0) diff --git a/app/workers/activitypub/delivery_worker.rb b/app/workers/activitypub/delivery_worker.rb index 059c32813..7510b1739 100644 --- a/app/workers/activitypub/delivery_worker.rb +++ b/app/workers/activitypub/delivery_worker.rb @@ -15,7 +15,10 @@ class ActivityPub::DeliveryWorker perform_request raise Mastodon::UnexpectedResponseError, @response unless response_successful? + + failure_tracker.track_success! rescue => e + failure_tracker.track_failure! raise e.class, "Delivery failed for #{inbox_url}: #{e.message}", e.backtrace[0] end @@ -34,4 +37,8 @@ class ActivityPub::DeliveryWorker def response_successful? @response.code > 199 && @response.code < 300 end + + def failure_tracker + @failure_tracker ||= DeliveryFailureTracker.new(@inbox_url) + end end diff --git a/spec/lib/delivery_failure_tracker_spec.rb b/spec/lib/delivery_failure_tracker_spec.rb new file mode 100644 index 000000000..39c8c7aaf --- /dev/null +++ b/spec/lib/delivery_failure_tracker_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe DeliveryFailureTracker do + subject { described_class.new('http://example.com/inbox') } + + describe '#track_success!' do + before do + subject.track_failure! + subject.track_success! + end + + it 'marks URL as available again' do + expect(described_class.available?('http://example.com/inbox')).to be true + end + + it 'resets days to 0' do + expect(subject.days).to be_zero + end + end + + describe '#track_failure!' do + it 'marks URL as unavailable after 7 days of being called' do + 6.times { |i| Redis.current.sadd('exhausted_deliveries:http://example.com/inbox', i) } + subject.track_failure! + + expect(subject.days).to eq 7 + expect(described_class.unavailable?('http://example.com/inbox')).to be true + end + + it 'repeated calls on the same day do not count' do + subject.track_failure! + subject.track_failure! + + expect(subject.days).to eq 1 + end + end + + describe '.filter' do + before do + Redis.current.sadd('unavailable_inboxes', 'http://example.com/unavailable/inbox') + end + + it 'removes URLs that are unavailable' do + result = described_class.filter(['http://example.com/good/inbox', 'http://example.com/unavailable/inbox']) + + expect(result).to include('http://example.com/good/inbox') + expect(result).to_not include('http://example.com/unavailable/inbox') + end + end + + describe '.track_inverse_success!' do + let(:from_account) { Fabricate(:account, inbox_url: 'http://example.com/inbox', shared_inbox_url: 'http://example.com/shared/inbox') } + + before do + Redis.current.sadd('unavailable_inboxes', 'http://example.com/inbox') + Redis.current.sadd('unavailable_inboxes', 'http://example.com/shared/inbox') + + described_class.track_inverse_success!(from_account) + end + + it 'marks inbox URL as available again' do + expect(described_class.available?('http://example.com/inbox')).to be true + end + + it 'marks shared inbox URL as available again' do + expect(described_class.available?('http://example.com/shared/inbox')).to be true + end + end +end -- cgit From eb605141ffb95290c5a537802ea418e6e45bf95f Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 30 Sep 2017 22:05:42 +0200 Subject: Fix #5104 - GET /api/v1/apps/verify_credentials to confirm app works (#5112) --- .../api/v1/apps/credentials_controller.rb | 11 ++++++ app/controllers/api/v1/apps_controller.rb | 2 - config/routes.rb | 7 +++- .../api/v1/apps/credentials_controller_spec.rb | 43 ++++++++++++++++++++++ 4 files changed, 60 insertions(+), 3 deletions(-) create mode 100644 app/controllers/api/v1/apps/credentials_controller.rb create mode 100644 spec/controllers/api/v1/apps/credentials_controller_spec.rb (limited to 'app/controllers') diff --git a/app/controllers/api/v1/apps/credentials_controller.rb b/app/controllers/api/v1/apps/credentials_controller.rb new file mode 100644 index 000000000..e469c7d21 --- /dev/null +++ b/app/controllers/api/v1/apps/credentials_controller.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class Api::V1::Apps::CredentialsController < Api::BaseController + before_action -> { doorkeeper_authorize! :read } + + respond_to :json + + def show + render json: doorkeeper_token.application, serializer: REST::StatusSerializer::ApplicationSerializer + end +end diff --git a/app/controllers/api/v1/apps_controller.rb b/app/controllers/api/v1/apps_controller.rb index 44a27b20a..e9f7a7291 100644 --- a/app/controllers/api/v1/apps_controller.rb +++ b/app/controllers/api/v1/apps_controller.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class Api::V1::AppsController < Api::BaseController - respond_to :json - def create @app = Doorkeeper::Application.create!(application_options) render json: @app, serializer: REST::ApplicationSerializer diff --git a/config/routes.rb b/config/routes.rb index ad2d8fca2..de3c1e0f9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -194,12 +194,17 @@ Rails.application.routes.draw do resources :follows, only: [:create] resources :media, only: [:create, :update] - resources :apps, only: [:create] resources :blocks, only: [:index] resources :mutes, only: [:index] resources :favourites, only: [:index] resources :reports, only: [:index, :create] + namespace :apps do + get :verify_credentials, to: 'credentials#show' + end + + resources :apps, only: [:create] + resource :instance, only: [:show] resource :domain_blocks, only: [:show, :create, :destroy] diff --git a/spec/controllers/api/v1/apps/credentials_controller_spec.rb b/spec/controllers/api/v1/apps/credentials_controller_spec.rb new file mode 100644 index 000000000..38f2a4e10 --- /dev/null +++ b/spec/controllers/api/v1/apps/credentials_controller_spec.rb @@ -0,0 +1,43 @@ +require 'rails_helper' + +describe Api::V1::Apps::CredentialsController do + render_views + + let(:token) { Fabricate(:accessible_access_token, scopes: 'read', application: Fabricate(:application)) } + + context 'with an oauth token' do + before do + allow(controller).to receive(:doorkeeper_token) { token } + end + + describe 'GET #show' do + before do + get :show + end + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + + it 'does not contain client credentials' do + json = body_as_json + + expect(json).to_not have_key(:client_secret) + expect(json).to_not have_key(:client_id) + end + end + end + + context 'without an oauth token' do + before do + allow(controller).to receive(:doorkeeper_token) { nil } + end + + describe 'GET #show' do + it 'returns http unauthorized' do + get :show + expect(response).to have_http_status(:unauthorized) + end + end + end +end -- cgit From cdacac8c6cbd85ed6e8a1cac8ce6fa5994094c7c Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sun, 1 Oct 2017 06:06:09 +0900 Subject: Fix order of paginated accounts in FollowerDomainsController and spec (#3357) * Fix order of paginated accounts in FollowerDomainsController Unordered pagination could result in unexpected behavior. * Cover Settings::FollowerDomainsController more --- .../settings/follower_domains_controller.rb | 2 +- .../settings/follower_domains_controller_spec.rb | 67 +++++++++++++++++++--- 2 files changed, 59 insertions(+), 10 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/settings/follower_domains_controller.rb b/app/controllers/settings/follower_domains_controller.rb index 90b48887f..9968504e5 100644 --- a/app/controllers/settings/follower_domains_controller.rb +++ b/app/controllers/settings/follower_domains_controller.rb @@ -9,7 +9,7 @@ class Settings::FollowerDomainsController < ApplicationController def show @account = current_account - @domains = current_account.followers.reorder(nil).group('accounts.domain').select('accounts.domain, count(accounts.id) as accounts_from_domain').page(params[:page]).per(10) + @domains = current_account.followers.reorder('MIN(follows.id) DESC').group('accounts.domain').select('accounts.domain, count(accounts.id) as accounts_from_domain').page(params[:page]).per(10) end def update diff --git a/spec/controllers/settings/follower_domains_controller_spec.rb b/spec/controllers/settings/follower_domains_controller_spec.rb index d48c3e68c..333223c61 100644 --- a/spec/controllers/settings/follower_domains_controller_spec.rb +++ b/spec/controllers/settings/follower_domains_controller_spec.rb @@ -5,15 +5,41 @@ describe Settings::FollowerDomainsController do let(:user) { Fabricate(:user) } - before do - sign_in user, scope: :user + shared_examples 'authenticate user' do + it 'redirects when not signed in' do + is_expected.to redirect_to '/auth/sign_in' + end end describe 'GET #show' do + subject { get :show, params: { page: 2 } } + + it 'assigns @account' do + sign_in user, scope: :user + subject + expect(assigns(:account)).to eq user.account + end + + it 'assigns @domains' do + Fabricate(:account, domain: 'old').follow!(user.account) + Fabricate(:account, domain: 'recent').follow!(user.account) + + sign_in user, scope: :user + subject + + assigned = assigns(:domains).per(1).to_a + expect(assigned.size).to eq 1 + expect(assigned[0].accounts_from_domain).to eq 1 + expect(assigned[0].domain).to eq 'old' + end + it 'returns http success' do - get :show + sign_in user, scope: :user + subject expect(response).to have_http_status(:success) end + + include_examples 'authenticate user' end describe 'PATCH #update' do @@ -21,16 +47,39 @@ describe Settings::FollowerDomainsController do before do stub_request(:post, 'http://example.com/salmon').to_return(status: 200) - poopfeast.follow!(user.account) - patch :update, params: { select: ['example.com'] } end - it 'redirects back to followers page' do - expect(response).to redirect_to(settings_follower_domains_path) + shared_examples 'redirects back to followers page' do |notice| + it 'redirects back to followers page' do + poopfeast.follow!(user.account) + + sign_in user, scope: :user + subject + + expect(flash[:notice]).to eq notice + expect(response).to redirect_to(settings_follower_domains_path) + end + end + + context 'when select parameter is not provided' do + subject { patch :update } + include_examples 'redirects back to followers page', 'In the process of soft-blocking followers from 0 domains...' end - it 'soft-blocks followers from selected domains' do - expect(poopfeast.following?(user.account)).to be false + context 'when select parameter is provided' do + subject { patch :update, params: { select: ['example.com'] } } + + it 'soft-blocks followers from selected domains' do + poopfeast.follow!(user.account) + + sign_in user, scope: :user + subject + + expect(poopfeast.following?(user.account)).to be false + end + + include_examples 'authenticate user' + include_examples 'redirects back to followers page', 'In the process of soft-blocking followers from one domain...' end end end -- cgit From 47ecd652d3f8256a191401f005d42760e858e6de Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 2 Oct 2017 01:23:32 +0200 Subject: Make Chrome splash screen same color as web UI's background color (#5169) --- app/controllers/manifests_controller.rb | 8 ++--- app/serializers/manifest_serializer.rb | 52 +++++++++++++++++++++++++++ app/views/manifests/show.json.rabl | 11 ------ spec/controllers/manifests_controller_spec.rb | 4 --- 4 files changed, 54 insertions(+), 21 deletions(-) create mode 100644 app/serializers/manifest_serializer.rb delete mode 100644 app/views/manifests/show.json.rabl (limited to 'app/controllers') diff --git a/app/controllers/manifests_controller.rb b/app/controllers/manifests_controller.rb index 832e1eb6f..ac267c229 100644 --- a/app/controllers/manifests_controller.rb +++ b/app/controllers/manifests_controller.rb @@ -1,11 +1,7 @@ # frozen_string_literal: true class ManifestsController < ApplicationController - before_action :set_instance_presenter - - def show; end - - def set_instance_presenter - @instance_presenter = InstancePresenter.new + def show + render json: InstancePresenter.new, serializer: ManifestSerializer end end diff --git a/app/serializers/manifest_serializer.rb b/app/serializers/manifest_serializer.rb new file mode 100644 index 000000000..95bcc21bb --- /dev/null +++ b/app/serializers/manifest_serializer.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +class ManifestSerializer < ActiveModel::Serializer + include RoutingHelper + include ActionView::Helpers::TextHelper + + attributes :name, :short_name, :description, + :icons, :theme_color, :background_color, + :display, :start_url, :scope + + def name + object.site_title + end + + def short_name + object.site_title + end + + def description + strip_tags(object.site_description.presence || I18n.t('about.about_mastodon_html')) + end + + def icons + [ + { + src: '/android-chrome-192x192.png', + sizes: '192x192', + type: 'image/png', + }, + ] + end + + def theme_color + '#282c37' + end + + def background_color + '#191b22' + end + + def display + 'standalone' + end + + def start_url + '/web/timelines/home' + end + + def scope + root_url + end +end diff --git a/app/views/manifests/show.json.rabl b/app/views/manifests/show.json.rabl deleted file mode 100644 index ee0a70324..000000000 --- a/app/views/manifests/show.json.rabl +++ /dev/null @@ -1,11 +0,0 @@ -object false - -node(:name) { Setting.site_title } -node(:short_name) { Setting.site_title } -node(:description) { strip_tags(Setting.site_description.presence || I18n.t('about.about_mastodon_html')) } -node(:icons) { [{ src: '/android-chrome-192x192.png', sizes: '192x192', type: 'image/png' }] } -node(:theme_color) { '#282c37' } -node(:background_color) { '#d9e1e8' } -node(:display) { 'standalone' } -node(:start_url) { '/web/timelines/home' } -node(:scope) { root_url } diff --git a/spec/controllers/manifests_controller_spec.rb b/spec/controllers/manifests_controller_spec.rb index 6f188fa35..71967e4f0 100644 --- a/spec/controllers/manifests_controller_spec.rb +++ b/spec/controllers/manifests_controller_spec.rb @@ -8,10 +8,6 @@ describe ManifestsController do get :show, format: :json end - it 'assigns @instance_presenter' do - expect(assigns(:instance_presenter)).to be_kind_of InstancePresenter - end - it 'returns http success' do expect(response).to have_http_status(:success) end -- cgit From dfaa219f8820224d37cd060d253a507111c63460 Mon Sep 17 00:00:00 2001 From: ThibG Date: Tue, 3 Oct 2017 23:21:19 +0200 Subject: Fix HTTP responses for salmon and ActivityPub inbox processing (#5200) * Return sensible HTTP status for ActivityPub inbox processing * Return sensible HTTP status for salmon slap processing * Return additional information to debug signature verification failures --- app/controllers/activitypub/inboxes_controller.rb | 4 ++-- app/controllers/api/salmon_controller.rb | 6 ++++-- app/controllers/concerns/signature_verification.rb | 9 +++++++++ spec/controllers/api/salmon_controller_spec.rb | 4 ++-- 4 files changed, 17 insertions(+), 6 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/activitypub/inboxes_controller.rb b/app/controllers/activitypub/inboxes_controller.rb index d0f8073ed..76553a162 100644 --- a/app/controllers/activitypub/inboxes_controller.rb +++ b/app/controllers/activitypub/inboxes_controller.rb @@ -9,9 +9,9 @@ class ActivityPub::InboxesController < Api::BaseController if signed_request_account upgrade_account process_payload - head 201 - else head 202 + else + [signature_verification_failure_reason, 401] end end diff --git a/app/controllers/api/salmon_controller.rb b/app/controllers/api/salmon_controller.rb index e9e700b18..143e9d3cd 100644 --- a/app/controllers/api/salmon_controller.rb +++ b/app/controllers/api/salmon_controller.rb @@ -7,9 +7,11 @@ class Api::SalmonController < Api::BaseController def update if verify_payload? process_salmon - head 201 - else head 202 + elsif payload.present? + [signature_verification_failure_reason, 401] + else + head 400 end end diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 52a9cf290..dc2d9a678 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -9,10 +9,15 @@ module SignatureVerification request.headers['Signature'].present? end + def signature_verification_failure_reason + return @signature_verification_failure_reason if defined?(@signature_verification_failure_reason) + end + def signed_request_account return @signed_request_account if defined?(@signed_request_account) unless signed_request? + @signature_verification_failure_reason = 'Request not signed' @signed_request_account = nil return end @@ -27,6 +32,7 @@ module SignatureVerification end if incompatible_signature?(signature_params) + @signature_verification_failure_reason = 'Incompatible request signature' @signed_request_account = nil return end @@ -34,6 +40,7 @@ module SignatureVerification account = account_from_key_id(signature_params['keyId']) if account.nil? + @signature_verification_failure_reason = "Public key not found for key #{signature_params['keyId']}" @signed_request_account = nil return end @@ -51,9 +58,11 @@ module SignatureVerification @signed_request_account = account @signed_request_account else + @signed_verification_failure_reason = "Verification failed for #{account.username}@#{account.domain} #{account.uri}" @signed_request_account = nil end else + @signed_verification_failure_reason = "Verification failed for #{account.username}@#{account.domain} #{account.uri}" @signed_request_account = nil end end diff --git a/spec/controllers/api/salmon_controller_spec.rb b/spec/controllers/api/salmon_controller_spec.rb index 3e4686200..323d85b61 100644 --- a/spec/controllers/api/salmon_controller_spec.rb +++ b/spec/controllers/api/salmon_controller_spec.rb @@ -46,8 +46,8 @@ RSpec.describe Api::SalmonController, type: :controller do post :update, params: { id: account.id } end - it 'returns http success' do - expect(response).to have_http_status(202) + it 'returns http client error' do + expect(response).to have_http_status(400) end end end -- cgit From 63f097979990bf5ba9db848b8a253056bad781af Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 4 Oct 2017 08:13:48 +0900 Subject: Validate id of ActivityPub representations (#5114) Additionally, ActivityPub::FetchRemoteStatusService no longer parses activities. OStatus::Activity::Creation no longer delegates to ActivityPub because the provided ActivityPub representations are not signed while OStatus representations are. --- app/controllers/concerns/signature_verification.rb | 2 +- app/helpers/jsonld_helper.rb | 13 ++++++- app/lib/activitypub/activity/announce.rb | 2 +- app/lib/activitypub/activity/create.rb | 2 +- app/lib/activitypub/linked_data_signature.rb | 2 +- app/lib/ostatus/activity/creation.rb | 9 ----- .../activitypub/fetch_remote_account_service.rb | 10 ++++-- .../activitypub/fetch_remote_key_service.rb | 25 +++++++++---- .../activitypub/fetch_remote_status_service.rb | 37 +++++++++---------- .../activitypub/process_account_service.rb | 6 ++-- app/services/fetch_atom_service.rb | 13 +++---- app/services/fetch_remote_account_service.rb | 14 ++++---- app/services/fetch_remote_status_service.rb | 16 ++++----- app/services/resolve_remote_account_service.rb | 2 +- spec/helpers/jsonld_helper_spec.rb | 35 +++++++++++++++++- .../fetch_remote_account_service_spec.rb | 2 +- .../fetch_remote_status_service_spec.rb | 41 +--------------------- 17 files changed, 118 insertions(+), 113 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index dc2d9a678..2baafb5bf 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -117,7 +117,7 @@ module SignatureVerification ResolveRemoteAccountService.new.call(key_id.gsub(/\Aacct:/, '')) elsif !ActivityPub::TagManager.instance.local_uri?(key_id) account = ActivityPub::TagManager.instance.uri_to_resource(key_id, Account) - account ||= ActivityPub::FetchRemoteKeyService.new.call(key_id) + account ||= ActivityPub::FetchRemoteKeyService.new.call(key_id, id: false) account end end diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index d82a07332..c23a2e095 100644 --- a/app/helpers/jsonld_helper.rb +++ b/app/helpers/jsonld_helper.rb @@ -22,7 +22,18 @@ module JsonLdHelper graph.dump(:normalize) end - def fetch_resource(uri) + def fetch_resource(uri, id) + unless id + json = fetch_resource_without_id_validation(uri) + return unless json + uri = json['id'] + end + + json = fetch_resource_without_id_validation(uri) + json.present? && json['id'] == uri ? json : nil + end + + def fetch_resource_without_id_validation(uri) response = build_request(uri).perform return if response.code != 200 body_to_json(response.to_s) diff --git a/app/lib/activitypub/activity/announce.rb b/app/lib/activitypub/activity/announce.rb index 4516454e1..1cf844281 100644 --- a/app/lib/activitypub/activity/announce.rb +++ b/app/lib/activitypub/activity/announce.rb @@ -27,7 +27,7 @@ class ActivityPub::Activity::Announce < ActivityPub::Activity if object_uri.start_with?('http') return if ActivityPub::TagManager.instance.local_uri?(object_uri) - ActivityPub::FetchRemoteStatusService.new.call(object_uri) + ActivityPub::FetchRemoteStatusService.new.call(object_uri, id: true) elsif @object['url'].present? ::FetchRemoteStatusService.new.call(@object['url']) end diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index 55addd66e..be656de48 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -80,7 +80,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity return if tag['href'].blank? account = account_from_uri(tag['href']) - account = FetchRemoteAccountService.new.call(tag['href']) if account.nil? + account = FetchRemoteAccountService.new.call(tag['href'], id: false) if account.nil? return if account.nil? account.mentions.create(status: status) end diff --git a/app/lib/activitypub/linked_data_signature.rb b/app/lib/activitypub/linked_data_signature.rb index adb8b6cdf..16142a6ff 100644 --- a/app/lib/activitypub/linked_data_signature.rb +++ b/app/lib/activitypub/linked_data_signature.rb @@ -19,7 +19,7 @@ class ActivityPub::LinkedDataSignature return unless type == 'RsaSignature2017' creator = ActivityPub::TagManager.instance.uri_to_resource(creator_uri, Account) - creator ||= ActivityPub::FetchRemoteKeyService.new.call(creator_uri) + creator ||= ActivityPub::FetchRemoteKeyService.new.call(creator_uri, id: false) return if creator.nil? diff --git a/app/lib/ostatus/activity/creation.rb b/app/lib/ostatus/activity/creation.rb index 2687776f9..511c462d4 100644 --- a/app/lib/ostatus/activity/creation.rb +++ b/app/lib/ostatus/activity/creation.rb @@ -9,11 +9,6 @@ class OStatus::Activity::Creation < OStatus::Activity::Base return [nil, false] if @account.suspended? - if activitypub_uri? && [:public, :unlisted].include?(visibility_scope) - result = perform_via_activitypub - return result if result.first.present? - end - RedisLock.acquire(lock_options) do |lock| if lock.acquired? # Return early if status already exists in db @@ -66,10 +61,6 @@ class OStatus::Activity::Creation < OStatus::Activity::Base status end - def perform_via_activitypub - [find_status(activitypub_uri) || ActivityPub::FetchRemoteStatusService.new.call(activitypub_uri), false] - end - def content @xml.at_xpath('./xmlns:content', xmlns: OStatus::TagManager::XMLNS).content end diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb index cb6e40748..e6c6338be 100644 --- a/app/services/activitypub/fetch_remote_account_service.rb +++ b/app/services/activitypub/fetch_remote_account_service.rb @@ -5,14 +5,18 @@ class ActivityPub::FetchRemoteAccountService < BaseService # Should be called when uri has already been checked for locality # Does a WebFinger roundtrip on each call - def call(uri, prefetched_json = nil) - @json = body_to_json(prefetched_json) || fetch_resource(uri) + def call(uri, id: true, prefetched_body: nil) + @json = if prefetched_body.nil? + fetch_resource(uri, id) + else + body_to_json(prefetched_body) + end return unless supported_context? && expected_type? @uri = @json['id'] @username = @json['preferredUsername'] - @domain = Addressable::URI.parse(uri).normalized_host + @domain = Addressable::URI.parse(@uri).normalized_host return unless verified_webfinger? diff --git a/app/services/activitypub/fetch_remote_key_service.rb b/app/services/activitypub/fetch_remote_key_service.rb index ebd64071e..ce1048fee 100644 --- a/app/services/activitypub/fetch_remote_key_service.rb +++ b/app/services/activitypub/fetch_remote_key_service.rb @@ -4,13 +4,26 @@ class ActivityPub::FetchRemoteKeyService < BaseService include JsonLdHelper # Returns account that owns the key - def call(uri, prefetched_json = nil) - @json = body_to_json(prefetched_json) || fetch_resource(uri) + def call(uri, id: true, prefetched_body: nil) + if prefetched_body.nil? + if id + @json = fetch_resource_without_id_validation(uri) + if person? + @json = fetch_resource(@json['id'], true) + elsif uri != @json['id'] + return + end + else + @json = fetch_resource(uri, id) + end + else + @json = body_to_json(prefetched_body) + end return unless supported_context?(@json) && expected_type? - return find_account(uri, @json) if person? + return find_account(@json['id'], @json) if person? - @owner = fetch_resource(owner_uri) + @owner = fetch_resource(owner_uri, true) return unless supported_context?(@owner) && confirmed_owner? @@ -19,9 +32,9 @@ class ActivityPub::FetchRemoteKeyService < BaseService private - def find_account(uri, prefetched_json) + def find_account(uri, prefetched_body) account = ActivityPub::TagManager.instance.uri_to_resource(uri, Account) - account ||= ActivityPub::FetchRemoteAccountService.new.call(uri, prefetched_json) + account ||= ActivityPub::FetchRemoteAccountService.new.call(uri, prefetched_body: prefetched_body) account end diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb index a95931afe..c7414f161 100644 --- a/app/services/activitypub/fetch_remote_status_service.rb +++ b/app/services/activitypub/fetch_remote_status_service.rb @@ -4,36 +4,33 @@ class ActivityPub::FetchRemoteStatusService < BaseService include JsonLdHelper # Should be called when uri has already been checked for locality - def call(uri, prefetched_json = nil) - @json = body_to_json(prefetched_json) || fetch_resource(uri) + def call(uri, id: true, prefetched_body: nil) + @json = if prefetched_body.nil? + fetch_resource(uri, id) + else + body_to_json(prefetched_body) + end - return unless supported_context? + return unless expected_type? && supported_context? - activity = activity_json - actor_id = value_or_id(activity['actor']) - - return unless expected_type?(activity) && trustworthy_attribution?(uri, actor_id) + return if actor_id.nil? || !trustworthy_attribution?(@json['id'], actor_id) actor = ActivityPub::TagManager.instance.uri_to_resource(actor_id, Account) - actor = ActivityPub::FetchRemoteAccountService.new.call(actor_id) if actor.nil? + actor = ActivityPub::FetchRemoteAccountService.new.call(actor_id, id: true) if actor.nil? return if actor.suspended? - ActivityPub::Activity.factory(activity, actor).perform + ActivityPub::Activity.factory(activity_json, actor).perform end private def activity_json - if %w(Note Article).include? @json['type'] - { - 'type' => 'Create', - 'actor' => first_of_value(@json['attributedTo']), - 'object' => @json, - } - else - @json - end + { 'type' => 'Create', 'actor' => actor_id, 'object' => @json } + end + + def actor_id + first_of_value(@json['attributedTo']) end def trustworthy_attribution?(uri, attributed_to) @@ -44,7 +41,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService super(@json) end - def expected_type?(json) - %w(Create Announce).include? json['type'] + def expected_type? + %w(Note Article).include? @json['type'] end end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 811209537..f93baf4b5 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -90,7 +90,7 @@ class ActivityPub::ProcessAccountService < BaseService return if value.nil? return value['url'] if value.is_a?(Hash) - image = fetch_resource(value) + image = fetch_resource_without_id_validation(value) image['url'] if image end @@ -100,7 +100,7 @@ class ActivityPub::ProcessAccountService < BaseService return if value.nil? return value['publicKeyPem'] if value.is_a?(Hash) - key = fetch_resource(value) + key = fetch_resource_without_id_validation(value) key['publicKeyPem'] if key end @@ -130,7 +130,7 @@ class ActivityPub::ProcessAccountService < BaseService return if @json[type].blank? return @collections[type] if @collections.key?(type) - collection = fetch_resource(@json[type]) + collection = fetch_resource_without_id_validation(@json[type]) @collections[type] = collection.is_a?(Hash) && collection['totalItems'].present? && collection['totalItems'].is_a?(Numeric) ? collection['totalItems'] : nil rescue HTTP::Error, OpenSSL::SSL::SSLError diff --git a/app/services/fetch_atom_service.rb b/app/services/fetch_atom_service.rb index 9c5777b5d..bcf516bc3 100644 --- a/app/services/fetch_atom_service.rb +++ b/app/services/fetch_atom_service.rb @@ -41,10 +41,11 @@ class FetchAtomService < BaseService return nil if @response.code != 200 if @response.mime_type == 'application/atom+xml' - [@url, @response.to_s, :ostatus] + [@url, { prefetched_body: @response.to_s }, :ostatus] elsif ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].include?(@response.mime_type) - if supported_activity?(@response.to_s) - [@url, @response.to_s, :activitypub] + json = body_to_json(body) + if supported_context?(json) && json['type'] == 'Person' && json['inbox'].present? + [json['id'], { id: true }, :activitypub] else @unsupported_activity = true nil @@ -79,10 +80,4 @@ class FetchAtomService < BaseService result end - - def supported_activity?(body) - json = body_to_json(body) - return false unless supported_context?(json) - json['type'] == 'Person' ? json['inbox'].present? : true - end end diff --git a/app/services/fetch_remote_account_service.rb b/app/services/fetch_remote_account_service.rb index bd98e70d1..a0f031a44 100644 --- a/app/services/fetch_remote_account_service.rb +++ b/app/services/fetch_remote_account_service.rb @@ -5,24 +5,24 @@ class FetchRemoteAccountService < BaseService def call(url, prefetched_body = nil, protocol = :ostatus) if prefetched_body.nil? - resource_url, body, protocol = FetchAtomService.new.call(url) + resource_url, resource_options, protocol = FetchAtomService.new.call(url) else - resource_url = url - body = prefetched_body + resource_url = url + resource_options = { prefetched_body: prefetched_body } end case protocol when :ostatus - process_atom(resource_url, body) + process_atom(resource_url, **resource_options) when :activitypub - ActivityPub::FetchRemoteAccountService.new.call(resource_url, body) + ActivityPub::FetchRemoteAccountService.new.call(resource_url, **resource_options) end end private - def process_atom(url, body) - xml = Nokogiri::XML(body) + def process_atom(url, prefetched_body:) + xml = Nokogiri::XML(prefetched_body) xml.encoding = 'utf-8' account = author_from_xml(xml.at_xpath('/xmlns:feed', xmlns: OStatus::TagManager::XMLNS), false) diff --git a/app/services/fetch_remote_status_service.rb b/app/services/fetch_remote_status_service.rb index 1b90854c4..cacf6ba51 100644 --- a/app/services/fetch_remote_status_service.rb +++ b/app/services/fetch_remote_status_service.rb @@ -5,26 +5,26 @@ class FetchRemoteStatusService < BaseService def call(url, prefetched_body = nil, protocol = :ostatus) if prefetched_body.nil? - resource_url, body, protocol = FetchAtomService.new.call(url) + resource_url, resource_options, protocol = FetchAtomService.new.call(url) else - resource_url = url - body = prefetched_body + resource_url = url + resource_options = { prefetched_body: prefetched_body } end case protocol when :ostatus - process_atom(resource_url, body) + process_atom(resource_url, **resource_options) when :activitypub - ActivityPub::FetchRemoteStatusService.new.call(resource_url, body) + ActivityPub::FetchRemoteStatusService.new.call(resource_url, **resource_options) end end private - def process_atom(url, body) + def process_atom(url, prefetched_body:) Rails.logger.debug "Processing Atom for remote status at #{url}" - xml = Nokogiri::XML(body) + xml = Nokogiri::XML(prefetched_body) xml.encoding = 'utf-8' account = author_from_xml(xml.at_xpath('/xmlns:entry', xmlns: OStatus::TagManager::XMLNS)) @@ -32,7 +32,7 @@ class FetchRemoteStatusService < BaseService return nil unless !account.nil? && confirmed_domain?(domain, account) - statuses = ProcessFeedService.new.call(body, account) + statuses = ProcessFeedService.new.call(prefetched_body, account) statuses.first rescue Nokogiri::XML::XPath::SyntaxError Rails.logger.debug 'Invalid XML or missing namespace' diff --git a/app/services/resolve_remote_account_service.rb b/app/services/resolve_remote_account_service.rb index 93ba07702..3d0a36f6c 100644 --- a/app/services/resolve_remote_account_service.rb +++ b/app/services/resolve_remote_account_service.rb @@ -189,7 +189,7 @@ class ResolveRemoteAccountService < BaseService def actor_json return @actor_json if defined?(@actor_json) - json = fetch_resource(actor_url) + json = fetch_resource(actor_url, false) @actor_json = supported_context?(json) && json['type'] == 'Person' ? json : nil end diff --git a/spec/helpers/jsonld_helper_spec.rb b/spec/helpers/jsonld_helper_spec.rb index 7d3912e6c..48bfdc306 100644 --- a/spec/helpers/jsonld_helper_spec.rb +++ b/spec/helpers/jsonld_helper_spec.rb @@ -30,6 +30,39 @@ describe JsonLdHelper do end describe '#fetch_resource' do - pending + context 'when the second argument is false' do + it 'returns resource even if the retrieved ID and the given URI does not match' do + stub_request(:get, 'https://bob/').to_return body: '{"id": "https://alice/"}' + stub_request(:get, 'https://alice/').to_return body: '{"id": "https://alice/"}' + + expect(fetch_resource('https://bob/', false)).to eq({ 'id' => 'https://alice/' }) + end + + it 'returns nil if the object identified by the given URI and the object identified by the retrieved ID does not match' do + stub_request(:get, 'https://mallory/').to_return body: '{"id": "https://marvin/"}' + stub_request(:get, 'https://marvin/').to_return body: '{"id": "https://alice/"}' + + expect(fetch_resource('https://mallory/', false)).to eq nil + end + end + + context 'when the second argument is true' do + it 'returns nil if the retrieved ID and the given URI does not match' do + stub_request(:get, 'https://mallory/').to_return body: '{"id": "https://alice/"}' + expect(fetch_resource('https://mallory/', true)).to eq nil + end + end + end + + describe '#fetch_resource_without_id_validation' do + it 'returns nil if the status code is not 200' do + stub_request(:get, 'https://host/').to_return status: 400, body: '{}' + expect(fetch_resource_without_id_validation('https://host/')).to eq nil + end + + it 'returns hash' do + stub_request(:get, 'https://host/').to_return status: 200, body: '{}' + expect(fetch_resource_without_id_validation('https://host/')).to eq({}) + end end end diff --git a/spec/services/activitypub/fetch_remote_account_service_spec.rb b/spec/services/activitypub/fetch_remote_account_service_spec.rb index ed7e9bba8..c50d3fb97 100644 --- a/spec/services/activitypub/fetch_remote_account_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_account_service_spec.rb @@ -16,7 +16,7 @@ RSpec.describe ActivityPub::FetchRemoteAccountService do end describe '#call' do - let(:account) { subject.call('https://example.com/alice') } + let(:account) { subject.call('https://example.com/alice', id: true) } shared_examples 'sets profile data' do it 'returns an account' do diff --git a/spec/services/activitypub/fetch_remote_status_service_spec.rb b/spec/services/activitypub/fetch_remote_status_service_spec.rb index 3b22257ed..ebf422392 100644 --- a/spec/services/activitypub/fetch_remote_status_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_status_service_spec.rb @@ -15,21 +15,11 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do } end - let(:create) do - { - '@context': 'https://www.w3.org/ns/activitystreams', - id: "https://#{valid_domain}/@foo/1234/activity", - type: 'Create', - actor: ActivityPub::TagManager.instance.uri_for(sender), - object: note, - } - end - subject { described_class.new } describe '#call' do before do - subject.call(object[:id], Oj.dump(object)) + subject.call(object[:id], prefetched_body: Oj.dump(object)) end context 'with Note object' do @@ -42,34 +32,5 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do expect(status.text).to eq 'Lorem ipsum' end end - - context 'with Create activity' do - let(:object) { create } - - it 'creates status' do - status = sender.statuses.first - - expect(status).to_not be_nil - expect(status.text).to eq 'Lorem ipsum' - end - end - - context 'with Announce activity' do - let(:status) { Fabricate(:status, account: recipient) } - - let(:object) do - { - '@context': 'https://www.w3.org/ns/activitystreams', - id: "https://#{valid_domain}/@foo/1234/activity", - type: 'Announce', - actor: ActivityPub::TagManager.instance.uri_for(sender), - object: ActivityPub::TagManager.instance.uri_for(status), - } - end - - it 'creates a reblog by sender of status' do - expect(sender.reblogged?(status)).to be true - end - end end end -- cgit From 468523f4ad85f99d78fd341ca4f5fc96f561a533 Mon Sep 17 00:00:00 2001 From: aschmitz Date: Wed, 4 Oct 2017 02:56:37 -0500 Subject: Non-Serial ("Snowflake") IDs (#4801) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Use non-serial IDs This change makes a number of nontrivial tweaks to the data model in Mastodon: * All IDs are now 8 byte integers (rather than mixed 4- and 8-byte) * IDs are now assigned as: * Top 6 bytes: millisecond-resolution time from epoch * Bottom 2 bytes: serial (within the millisecond) sequence number * See /lib/tasks/db.rake's `define_timestamp_id` for details, but note that the purpose of these changes is to make it difficult to determine the number of objects in a table from the ID of any object. * The Redis sorted set used for the feed will have values used to look up toots, rather than scores. This is almost always the same as the existing behavior, except in the case of boosted toots. This change was made because Redis stores scores as double-precision floats, which cannot store the new ID format exactly. Note that this doesn't cause problems with sorting/pagination, because ZREVRANGEBYSCORE sorts lexicographically when scores are tied. (This will still cause sorting issues when the ID gains a new significant digit, but that's extraordinarily uncommon.) Note a couple of tradeoffs have been made in this commit: * lib/tasks/db.rake is used to enforce many/most column constraints, because this commit seems likely to take a while to bring upstream. Enforcing a post-migrate hook is an easier way to maintain the code in the interim. * Boosted toots will appear in the timeline as many times as they have been boosted. This is a tradeoff due to the way the feed is saved in Redis at the moment, but will be handled by a future commit. This would effectively close Mastodon's #1059, as it is a snowflake-like system of generating IDs. However, given how involved the changes were simply within Mastodon, it may have unexpected interactions with some clients, if they store IDs as doubles (or as 4-byte integers). This was a problem that Twitter ran into with their "snowflake" transition, particularly in JavaScript clients that treated IDs as JS integers, rather than strings. It therefore would be useful to test these changes at least in the web interface and popular clients before pushing them to all users. * Fix JavaScript interface with long IDs Somewhat predictably, the JS interface handled IDs as numbers, which in JS are IEEE double-precision floats. This loses some precision when working with numbers as large as those generated by the new ID scheme, so we instead handle them here as strings. This is relatively simple, and doesn't appear to have caused any problems, but should definitely be tested more thoroughly than the built-in tests. Several days of use appear to support this working properly. BREAKING CHANGE: The major(!) change here is that IDs are now returned as strings by the REST endpoints, rather than as integers. In practice, relatively few changes were required to make the existing JS UI work with this change, but it will likely hit API clients pretty hard: it's an entirely different type to consume. (The one API client I tested, Tusky, handles this with no problems, however.) Twitter ran into this issue when introducing Snowflake IDs, and decided to instead introduce an `id_str` field in JSON responses. I have opted to *not* do that, and instead force all IDs to 64-bit integers represented by strings in one go. (I believe Twitter exacerbated their problem by rolling out the changes three times: once for statuses, once for DMs, and once for user IDs, as well as by leaving an integer ID value in JSON. As they said, "If you’re using the `id` field with JSON in a Javascript-related language, there is a very high likelihood that the integers will be silently munged by Javascript interpreters. In most cases, this will result in behavior such as being unable to load or delete a specific direct message, because the ID you're sending to the API is different than the actual identifier associated with the message." [1]) However, given that this is a significant change for API users, alternatives or a transition time may be appropriate. 1: https://blog.twitter.com/developer/en_us/a/2011/direct-messages-going-snowflake-on-sep-30-2011.html * Restructure feed pushes/unpushes This was necessary because the previous behavior used Redis zset scores to identify statuses, but those are IEEE double-precision floats, so we can't actually use them to identify all 64-bit IDs. However, it leaves the code in a much better state for refactoring reblog handling / coalescing. Feed-management code has been consolidated in FeedManager, including: * BatchedRemoveStatusService no longer directly manipulates feed zsets * RemoveStatusService no longer directly manipulates feed zsets * PrecomputeFeedService has moved its logic to FeedManager#populate_feed (PrecomputeFeedService largely made lots of calls to FeedManager, but didn't follow the normal adding-to-feed process.) This has the effect of unifying all of the feed push/unpush logic in FeedManager, making it much more tractable to update it in the future. Due to some additional checks that must be made during, for example, batch status removals, some Redis pipelining has been removed. It does not appear that this should cause significantly increased load, but if necessary, some optimizations are possible in batch cases. These were omitted in the pursuit of simplicity, but a batch_push and batch_unpush would be possible in the future. Tests were added to verify that pushes happen under expected conditions, and to verify reblog behavior (both on pushing and unpushing). In the case of unpushing, this includes testing behavior that currently leads to confusion such as Mastodon's #2817, but this codifies that the behavior is currently expected. * Rubocop fixes I could swear I made these changes already, but I must have lost them somewhere along the line. * Address review comments This addresses the first two comments from review of this feature: https://github.com/tootsuite/mastodon/pull/4801#discussion_r139336735 https://github.com/tootsuite/mastodon/pull/4801#discussion_r139336931 This adds an optional argument to FeedManager#key, the subtype of feed key to generate. It also tests to ensure that FeedManager's settings are such that reblogs won't be tracked forever. * Hardcode IdToBigints migration columns This addresses a comment during review: https://github.com/tootsuite/mastodon/pull/4801#discussion_r139337452 This means we'll need to make sure that all _id columns going forward are bigints, but that should happen automatically in most cases. * Additional fixes for stringified IDs in JSON These should be the last two. These were identified using eslint to try to identify any plain casts to JavaScript numbers. (Some such casts are legitimate, but these were not.) Adding the following to .eslintrc.yml will identify casts to numbers: ~~~ no-restricted-syntax: - warn - selector: UnaryExpression[operator='+'] > :not(Literal) message: Avoid the use of unary + - selector: CallExpression[callee.name='Number'] message: Casting with Number() may coerce string IDs to numbers ~~~ The remaining three casts appear legitimate: two casts to array indices, one in a server to turn an environment variable into a number. * Only implement timestamp IDs for Status IDs Per discussion in #4801, this is only being merged in for Status IDs at this point. We do this in a migration, as there is no longer use for a post-migration hook. We keep the initialization of the timestamp_id function as a Rake task, as it is also needed after db:schema:load (as db/schema.rb doesn't store Postgres functions). * Change internal streaming payloads to stringified IDs as well This is equivalent to 591a9af356faf2d5c7e66e3ec715502796c875cd from #5019, with an extra change for the addition to FeedManager#unpush. * Ensure we have a status_id_seq sequence Apparently this is not a given when specifying a custom ID function, so now we ensure it gets created. This uses the generic version of this function to more easily support adding additional tables with timestamp IDs in the future, although it would be possible to cut this down to a less generic version if necessary. It is only run during db:schema:load or the relevant migration, so the overhead is extraordinarily minimal. * Transition reblogs to new Redis format This provides a one-way migration to transition old Redis reblog entries into the new format, with a separate tracking entry for reblogs. It is not invertible because doing so could (if timestamp IDs are used) require a database query for each status in each users' feed, which is likely to be a significant toll on major instances. * Address review comments from @akihikodaki No functional changes. * Additional review changes * Heredoc cleanup * Run db:schema:load hooks for test in development This matches the behavior in Rails' ActiveRecord::Tasks::DatabaseTasks.each_current_configuration, which would otherwise break `rake db:setup` in development. It also moves some functionality out to a library, which will be a good place to put additional related functionality in the near future. --- .../api/v1/accounts/relationships_controller.rb | 5 +- app/lib/feed_manager.rb | 128 +++++++++++++++++---- app/models/feed.rb | 2 +- app/services/batched_remove_status_service.rb | 37 ++---- app/services/precompute_feed_service.rb | 38 +----- app/services/remove_status_service.rb | 8 +- .../20170920024819_status_ids_to_timestamp_ids.rb | 32 ++++++ db/migrate/20170920032311_fix_reblogs_in_feeds.rb | 63 ++++++++++ db/schema.rb | 2 +- lib/mastodon/timestamp_ids.rb | 126 ++++++++++++++++++++ lib/tasks/db.rake | 56 +++++++++ spec/lib/feed_manager_spec.rb | 109 ++++++++++++++++++ spec/models/feed_spec.rb | 2 +- .../services/batched_remove_status_service_spec.rb | 3 +- spec/services/precompute_feed_service_spec.rb | 2 +- 15 files changed, 509 insertions(+), 104 deletions(-) create mode 100644 db/migrate/20170920024819_status_ids_to_timestamp_ids.rb create mode 100644 db/migrate/20170920032311_fix_reblogs_in_feeds.rb create mode 100644 lib/mastodon/timestamp_ids.rb (limited to 'app/controllers') diff --git a/app/controllers/api/v1/accounts/relationships_controller.rb b/app/controllers/api/v1/accounts/relationships_controller.rb index a88cf2021..91a942d75 100644 --- a/app/controllers/api/v1/accounts/relationships_controller.rb +++ b/app/controllers/api/v1/accounts/relationships_controller.rb @@ -7,7 +7,10 @@ class Api::V1::Accounts::RelationshipsController < Api::BaseController respond_to :json def index - @accounts = Account.where(id: account_ids).select('id') + accounts = Account.where(id: account_ids).select('id') + # .where doesn't guarantee that our results are in the same order + # we requested them, so return the "right" order to the requestor. + @accounts = accounts.index_by(&:id).values_at(*account_ids) render json: @accounts, each_serializer: REST::RelationshipSerializer, relationships: relationships end diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index b1ae11084..c509c5702 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -7,8 +7,13 @@ class FeedManager MAX_ITEMS = 400 - def key(type, id) - "feed:#{type}:#{id}" + # Must be <= MAX_ITEMS or the tracking sets will grow forever + REBLOG_FALLOFF = 40 + + def key(type, id, subtype = nil) + return "feed:#{type}:#{id}" unless subtype + + "feed:#{type}:#{id}:#{subtype}" end def filter?(timeline_type, status, receiver_id) @@ -22,23 +27,36 @@ class FeedManager end def push(timeline_type, account, status) - timeline_key = key(timeline_type, account.id) + return false unless add_to_feed(timeline_type, account, status) - if status.reblog? - # If the original status is within 40 statuses from top, do not re-insert it into the feed - rank = redis.zrevrank(timeline_key, status.reblog_of_id) - return if !rank.nil? && rank < 40 - redis.zadd(timeline_key, status.id, status.reblog_of_id) - else - redis.zadd(timeline_key, status.id, status.id) - trim(timeline_type, account.id) - end + trim(timeline_type, account.id) PushUpdateWorker.perform_async(account.id, status.id) if push_update_required?(timeline_type, account.id) + + true + end + + def unpush(timeline_type, account, status) + return false unless remove_from_feed(timeline_type, account, status) + + payload = Oj.dump(event: :delete, payload: status.id.to_s) + Redis.current.publish("timeline:#{account.id}", payload) + + true end def trim(type, account_id) - redis.zremrangebyrank(key(type, account_id), '0', (-(FeedManager::MAX_ITEMS + 1)).to_s) + timeline_key = key(type, account_id) + reblog_key = key(type, account_id, 'reblogs') + # Remove any items past the MAX_ITEMS'th entry in our feed + redis.zremrangebyrank(timeline_key, '0', (-(FeedManager::MAX_ITEMS + 1)).to_s) + + # Get the score of the REBLOG_FALLOFF'th item in our feed, and stop + # tracking anything after it for deduplication purposes. + falloff_rank = FeedManager::REBLOG_FALLOFF - 1 + falloff_range = redis.zrevrange(timeline_key, falloff_rank, falloff_rank, with_scores: true) + falloff_score = falloff_range&.first&.last&.to_i || 0 + redis.zremrangebyscore(reblog_key, 0, falloff_score) end def push_update_required?(timeline_type, account_id) @@ -54,11 +72,9 @@ class FeedManager query = query.where('id > ?', oldest_home_score) end - redis.pipelined do - query.each do |status| - next if status.direct_visibility? || filter?(:home, status, into_account) - redis.zadd(timeline_key, status.id, status.id) - end + query.each do |status| + next if status.direct_visibility? || filter?(:home, status, into_account) + add_to_feed(:home, into_account, status) end trim(:home, into_account.id) @@ -69,11 +85,8 @@ class FeedManager oldest_home_score = redis.zrange(timeline_key, 0, 0, with_scores: true)&.first&.last&.to_i || 0 from_account.statuses.select('id').where('id > ?', oldest_home_score).reorder(nil).find_in_batches do |statuses| - redis.pipelined do - statuses.each do |status| - redis.zrem(timeline_key, status.id) - redis.zremrangebyscore(timeline_key, status.id, status.id) - end + statuses.each do |status| + unpush(:home, into_account, status) end end end @@ -81,9 +94,20 @@ class FeedManager def clear_from_timeline(account, target_account) timeline_key = key(:home, account.id) timeline_status_ids = redis.zrange(timeline_key, 0, -1) - target_status_ids = Status.where(id: timeline_status_ids, account: target_account).ids + target_statuses = Status.where(id: timeline_status_ids, account: target_account) - redis.zrem(timeline_key, target_status_ids) if target_status_ids.present? + target_statuses.each do |status| + unpush(:home, account, status) + end + end + + def populate_feed(account) + prepopulate_limit = FeedManager::MAX_ITEMS / 4 + statuses = Status.as_home_timeline(account).order(account_id: :desc).limit(prepopulate_limit) + statuses.reverse_each do |status| + next if filter_from_home?(status, account) + add_to_feed(:home, account, status) + end end private @@ -131,4 +155,58 @@ class FeedManager should_filter end + + # Adds a status to an account's feed, returning true if a status was + # added, and false if it was not added to the feed. Note that this is + # an internal helper: callers must call trim or push updates if + # either action is appropriate. + def add_to_feed(timeline_type, account, status) + timeline_key = key(timeline_type, account.id) + reblog_key = key(timeline_type, account.id, 'reblogs') + + if status.reblog? + # If the original status or a reblog of it is within + # REBLOG_FALLOFF statuses from the top, do not re-insert it into + # the feed + rank = redis.zrevrank(timeline_key, status.reblog_of_id) + return false if !rank.nil? && rank < FeedManager::REBLOG_FALLOFF + + reblog_rank = redis.zrevrank(reblog_key, status.reblog_of_id) + return false unless reblog_rank.nil? + + redis.zadd(timeline_key, status.id, status.id) + redis.zadd(reblog_key, status.id, status.reblog_of_id) + else + redis.zadd(timeline_key, status.id, status.id) + end + + true + end + + # Removes an individual status from a feed, correctly handling cases + # with reblogs, and returning true if a status was removed. As with + # `add_to_feed`, this does not trigger push updates, so callers must + # do so if appropriate. + def remove_from_feed(timeline_type, account, status) + timeline_key = key(timeline_type, account.id) + reblog_key = key(timeline_type, account.id, 'reblogs') + + if status.reblog? + # 1. If the reblogging status is not in the feed, stop. + status_rank = redis.zrevrank(timeline_key, status.id) + return false if status_rank.nil? + + # 2. Remove the reblogged status from the `:reblogs` zset. + redis.zrem(reblog_key, status.reblog_of_id) + + # 3. Add the reblogged status to the feed using the reblogging + # status' ID as its score, and the reblogged status' ID as its + # value. + redis.zadd(timeline_key, status.id, status.reblog_of_id) + + # 4. Remove the reblogging status from the feed (as normal) + end + + redis.zrem(timeline_key, status.id) + end end diff --git a/app/models/feed.rb b/app/models/feed.rb index beb4a8de3..5f7b7877a 100644 --- a/app/models/feed.rb +++ b/app/models/feed.rb @@ -19,7 +19,7 @@ class Feed def from_redis(limit, max_id, since_id) max_id = '+inf' if max_id.blank? since_id = '-inf' if since_id.blank? - unhydrated = redis.zrevrangebyscore(key, "(#{max_id}", "(#{since_id}", limit: [0, limit], with_scores: true).map(&:last).map(&:to_i) + unhydrated = redis.zrevrangebyscore(key, "(#{max_id}", "(#{since_id}", limit: [0, limit], with_scores: true).map(&:first).map(&:to_i) Status.where(id: unhydrated).cache_ids end diff --git a/app/services/batched_remove_status_service.rb b/app/services/batched_remove_status_service.rb index 2fd623922..5d83771c9 100644 --- a/app/services/batched_remove_status_service.rb +++ b/app/services/batched_remove_status_service.rb @@ -29,7 +29,7 @@ class BatchedRemoveStatusService < BaseService statuses.group_by(&:account_id).each do |_, account_statuses| account = account_statuses.first.account - unpush_from_home_timelines(account_statuses) + unpush_from_home_timelines(account, account_statuses) if account.local? batch_stream_entries(account, account_statuses) @@ -72,14 +72,15 @@ class BatchedRemoveStatusService < BaseService end end - def unpush_from_home_timelines(statuses) - account = statuses.first.account - recipients = account.followers.local.pluck(:id) + def unpush_from_home_timelines(account, statuses) + recipients = account.followers.local.to_a - recipients << account.id if account.local? + recipients << account if account.local? - recipients.each do |follower_id| - unpush(follower_id, statuses) + recipients.each do |follower| + statuses.each do |status| + FeedManager.instance.unpush(:home, follower, status) + end end end @@ -109,28 +110,6 @@ class BatchedRemoveStatusService < BaseService end end - def unpush(follower_id, statuses) - key = FeedManager.instance.key(:home, follower_id) - - originals = statuses.reject(&:reblog?) - reblogs = statuses.select(&:reblog?) - - # Quickly remove all originals - redis.pipelined do - originals.each do |status| - redis.zremrangebyscore(key, status.id, status.id) - redis.publish("timeline:#{follower_id}", @json_payloads[status.id]) - end - end - - # For reblogs, re-add original status to feed, unless the reblog - # was not in the feed in the first place - reblogs.each do |status| - redis.zadd(key, status.reblog_of_id, status.reblog_of_id) unless redis.zscore(key, status.reblog_of_id).nil? - redis.publish("timeline:#{follower_id}", @json_payloads[status.id]) - end - end - def redis Redis.current end diff --git a/app/services/precompute_feed_service.rb b/app/services/precompute_feed_service.rb index 85635a008..36aabaa00 100644 --- a/app/services/precompute_feed_service.rb +++ b/app/services/precompute_feed_service.rb @@ -1,43 +1,7 @@ # frozen_string_literal: true class PrecomputeFeedService < BaseService - LIMIT = FeedManager::MAX_ITEMS / 4 - def call(account) - @account = account - populate_feed - end - - private - - attr_reader :account - - def populate_feed - pairs = statuses.reverse_each.lazy.reject(&method(:status_filtered?)).map(&method(:process_status)).to_a - - redis.pipelined do - redis.zadd(account_home_key, pairs) if pairs.any? - redis.del("account:#{@account.id}:regeneration") - end - end - - def process_status(status) - [status.id, status.reblog? ? status.reblog_of_id : status.id] - end - - def status_filtered?(status) - FeedManager.instance.filter?(:home, status, account.id) - end - - def account_home_key - FeedManager.instance.key(:home, account.id) - end - - def statuses - Status.as_home_timeline(account).order(account_id: :desc).limit(LIMIT) - end - - def redis - Redis.current + FeedManager.instance.populate_feed(account) end end diff --git a/app/services/remove_status_service.rb b/app/services/remove_status_service.rb index 14f24908c..96d9208cc 100644 --- a/app/services/remove_status_service.rb +++ b/app/services/remove_status_service.rb @@ -102,13 +102,7 @@ class RemoveStatusService < BaseService end def unpush(type, receiver, status) - if status.reblog? && !redis.zscore(FeedManager.instance.key(type, receiver.id), status.reblog_of_id).nil? - redis.zadd(FeedManager.instance.key(type, receiver.id), status.reblog_of_id, status.reblog_of_id) - else - redis.zremrangebyscore(FeedManager.instance.key(type, receiver.id), status.id, status.id) - end - - Redis.current.publish("timeline:#{receiver.id}", @payload) + FeedManager.instance.unpush(type, receiver, status) end def remove_from_hashtags diff --git a/db/migrate/20170920024819_status_ids_to_timestamp_ids.rb b/db/migrate/20170920024819_status_ids_to_timestamp_ids.rb new file mode 100644 index 000000000..5d15817bd --- /dev/null +++ b/db/migrate/20170920024819_status_ids_to_timestamp_ids.rb @@ -0,0 +1,32 @@ +class StatusIdsToTimestampIds < ActiveRecord::Migration[5.1] + def up + # Prepare the function we will use to generate IDs. + Rake::Task['db:define_timestamp_id'].execute + + # Set up the statuses.id column to use our timestamp-based IDs. + ActiveRecord::Base.connection.execute(<<~SQL) + ALTER TABLE statuses + ALTER COLUMN id + SET DEFAULT timestamp_id('statuses') + SQL + + # Make sure we have a sequence to use. + Rake::Task['db:ensure_id_sequences_exist'].execute + end + + def down + # Revert the column to the old method of just using the sequence + # value for new IDs. Set the current ID sequence to the maximum + # existing ID, such that the next sequence will be one higher. + + # We lock the table during this so that the ID won't get clobbered, + # but ID is indexed, so this should be a fast operation. + ActiveRecord::Base.connection.execute(<<~SQL) + LOCK statuses; + SELECT setval('statuses_id_seq', (SELECT MAX(id) FROM statuses)); + ALTER TABLE statuses + ALTER COLUMN id + SET DEFAULT nextval('statuses_id_seq');" + SQL + end +end diff --git a/db/migrate/20170920032311_fix_reblogs_in_feeds.rb b/db/migrate/20170920032311_fix_reblogs_in_feeds.rb new file mode 100644 index 000000000..c813ecd46 --- /dev/null +++ b/db/migrate/20170920032311_fix_reblogs_in_feeds.rb @@ -0,0 +1,63 @@ +class FixReblogsInFeeds < ActiveRecord::Migration[5.1] + def up + redis = Redis.current + fm = FeedManager.instance + + # find_each is batched on the database side. + User.includes(:account).find_each do |user| + account = user.account + + # Old scheme: + # Each user's feed zset had a series of score:value entries, + # where "regular" statuses had the same score and value (their + # ID). Reblogs had a score of the reblogging status' ID, and a + # value of the reblogged status' ID. + + # New scheme: + # The feed contains only entries with the same score and value. + # Reblogs result in the reblogging status being added to the + # feed, with an entry in a reblog tracking zset (where the score + # is once again set to the reblogging status' ID, and the value + # is set to the reblogged status' ID). This is safe for Redis' + # float coersion because in this reblog tracking zset, we only + # need the rebloggging status' ID to be able to stop tracking + # entries after they have gotten too far down the feed, which + # does not require an exact value. + + # So, first, we iterate over the user's feed to find any reblogs. + timeline_key = fm.key(:home, account.id) + reblog_key = fm.key(:home, account.id, 'reblogs') + redis.zrange(timeline_key, 0, -1, with_scores: true).each do |entry| + next if entry[0] == entry[1] + + # The score and value don't match, so this is a reblog. + # (note that we're transitioning from IDs < 53 bits so we + # don't have to worry about the loss of precision) + + reblogged_id, reblogging_id = entry + + # Remove the old entry + redis.zrem(timeline_key, reblogged_id) + + # Add a new one for the reblogging status + redis.zadd(timeline_key, reblogging_id, reblogging_id) + + # Track the fact that this was a reblog + redis.zadd(reblog_key, reblogging_id, reblogged_id) + end + end + end + + def down + # We *deliberately* do nothing here. This means that reverting + # this and the associated changes to the FeedManager code could + # allow one superfluous reblog of any given status, but in the case + # where things have gone wrong and a revert is necessary, this + # appears preferable to requiring a database hit for every status + # in every users' feed simply to revert. + + # Note that this is operating under the assumption that entries + # with >53-bit IDs have already been entered. Otherwise, we could + # just use the data in Redis to reverse this transition. + end +end diff --git a/db/schema.rb b/db/schema.rb index 2cb105553..00cc24bae 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -321,7 +321,7 @@ ActiveRecord::Schema.define(version: 20170927215609) do t.index ["account_id", "status_id"], name: "index_status_pins_on_account_id_and_status_id", unique: true end - create_table "statuses", force: :cascade do |t| + create_table "statuses", id: :bigint, default: -> { "timestamp_id('statuses'::text)" }, force: :cascade do |t| t.string "uri" t.text "text", default: "", null: false t.datetime "created_at", null: false diff --git a/lib/mastodon/timestamp_ids.rb b/lib/mastodon/timestamp_ids.rb new file mode 100644 index 000000000..d49b5c1b5 --- /dev/null +++ b/lib/mastodon/timestamp_ids.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +module Mastodon + module TimestampIds + def self.define_timestamp_id + conn = ActiveRecord::Base.connection + + # Make sure we don't already have a `timestamp_id` function. + unless conn.execute(<<~SQL).values.first.first + SELECT EXISTS( + SELECT * FROM pg_proc WHERE proname = 'timestamp_id' + ); + SQL + # The function doesn't exist, so we'll define it. + conn.execute(<<~SQL) + CREATE OR REPLACE FUNCTION timestamp_id(table_name text) + RETURNS bigint AS + $$ + DECLARE + time_part bigint; + sequence_base bigint; + tail bigint; + BEGIN + -- Our ID will be composed of the following: + -- 6 bytes (48 bits) of millisecond-level timestamp + -- 2 bytes (16 bits) of sequence data + + -- The 'sequence data' is intended to be unique within a + -- given millisecond, yet obscure the 'serial number' of + -- this row. + + -- To do this, we hash the following data: + -- * Table name (if provided, skipped if not) + -- * Secret salt (should not be guessable) + -- * Timestamp (again, millisecond-level granularity) + + -- We then take the first two bytes of that value, and add + -- the lowest two bytes of the table ID sequence number + -- (`table_name`_id_seq). This means that even if we insert + -- two rows at the same millisecond, they will have + -- distinct 'sequence data' portions. + + -- If this happens, and an attacker can see both such IDs, + -- they can determine which of the two entries was inserted + -- first, but not the total number of entries in the table + -- (even mod 2**16). + + -- The table name is included in the hash to ensure that + -- different tables derive separate sequence bases so rows + -- inserted in the same millisecond in different tables do + -- not reveal the table ID sequence number for one another. + + -- The secret salt is included in the hash to ensure that + -- external users cannot derive the sequence base given the + -- timestamp and table name, which would allow them to + -- compute the table ID sequence number. + + time_part := ( + -- Get the time in milliseconds + ((date_part('epoch', now()) * 1000))::bigint + -- And shift it over two bytes + << 16); + + sequence_base := ( + 'x' || + -- Take the first two bytes (four hex characters) + substr( + -- Of the MD5 hash of the data we documented + md5(table_name || + '#{SecureRandom.hex(16)}' || + time_part::text + ), + 1, 4 + ) + -- And turn it into a bigint + )::bit(16)::bigint; + + -- Finally, add our sequence number to our base, and chop + -- it to the last two bytes + tail := ( + (sequence_base + nextval(table_name || '_id_seq')) + & 65535); + + -- Return the time part and the sequence part. OR appears + -- faster here than addition, but they're equivalent: + -- time_part has no trailing two bytes, and tail is only + -- the last two bytes. + RETURN time_part | tail; + END + $$ LANGUAGE plpgsql VOLATILE; + SQL + end + end + + def self.ensure_id_sequences_exist + conn = ActiveRecord::Base.connection + + # Find tables using timestamp IDs. + default_regex = /timestamp_id\('(?\w+)'/ + conn.tables.each do |table| + # We're only concerned with "id" columns. + next unless (id_col = conn.columns(table).find { |col| col.name == 'id' }) + + # And only those that are using timestamp_id. + next unless (data = default_regex.match(id_col.default_function)) + + seq_name = data[:seq_prefix] + '_id_seq' + # If we were on Postgres 9.5+, we could do CREATE SEQUENCE IF + # NOT EXISTS, but we can't depend on that. Instead, catch the + # possible exception and ignore it. + # Note that seq_name isn't a column name, but it's a + # relation, like a column, and follows the same quoting rules + # in Postgres. + conn.execute(<<~SQL) + DO $$ + BEGIN + CREATE SEQUENCE #{conn.quote_column_name(seq_name)}; + EXCEPTION WHEN duplicate_table THEN + -- Do nothing, we have the sequence already. + END + $$ LANGUAGE plpgsql; + SQL + end + end + end +end diff --git a/lib/tasks/db.rake b/lib/tasks/db.rake index 7a055bf25..66468d999 100644 --- a/lib/tasks/db.rake +++ b/lib/tasks/db.rake @@ -1,5 +1,36 @@ # frozen_string_literal: true +require Rails.root.join('lib', 'mastodon', 'timestamp_ids') + +def each_schema_load_environment + # If we're in development, also run this for the test environment. + # This is a somewhat hacky way to do this, so here's why: + # 1. We have to define this before we load the schema, or we won't + # have a timestamp_id function when we get to it in the schema. + # 2. db:setup calls db:schema:load_if_ruby, which calls + # db:schema:load, which we define above as having a prerequisite + # of this task. + # 3. db:schema:load ends up running + # ActiveRecord::Tasks::DatabaseTasks.load_schema_current, which + # calls a private method `each_current_configuration`, which + # explicitly also does the loading for the `test` environment + # if the current environment is `development`, so we end up + # needing to do the same, and we can't even use the same method + # to do it. + + if Rails.env == 'development' + test_conf = ActiveRecord::Base.configurations['test'] + if test_conf['database']&.present? + ActiveRecord::Base.establish_connection(:test) + yield + + ActiveRecord::Base.establish_connection(Rails.env.to_sym) + end + end + + yield +end + namespace :db do namespace :migrate do desc 'Setup the db or migrate depending on state of db' @@ -16,4 +47,29 @@ namespace :db do end end end + + # Before we load the schema, define the timestamp_id function. + # Idiomatically, we might do this in a migration, but then it + # wouldn't end up in schema.rb, so we'd need to figure out a way to + # get it in before doing db:setup as well. This is simpler, and + # ensures it's always in place. + Rake::Task['db:schema:load'].enhance ['db:define_timestamp_id'] + + # After we load the schema, make sure we have sequences for each + # table using timestamp IDs. + Rake::Task['db:schema:load'].enhance do + Rake::Task['db:ensure_id_sequences_exist'].invoke + end + + task :define_timestamp_id do + each_schema_load_environment do + Mastodon::TimestampIds.define_timestamp_id + end + end + + task :ensure_id_sequences_exist do + each_schema_load_environment do + Mastodon::TimestampIds.ensure_id_sequences_exist + end + end end diff --git a/spec/lib/feed_manager_spec.rb b/spec/lib/feed_manager_spec.rb index 22439cf35..923894ccb 100644 --- a/spec/lib/feed_manager_spec.rb +++ b/spec/lib/feed_manager_spec.rb @@ -1,6 +1,10 @@ require 'rails_helper' RSpec.describe FeedManager do + it 'tracks at least as many statuses as reblogs' do + expect(FeedManager::REBLOG_FALLOFF).to be <= FeedManager::MAX_ITEMS + end + describe '#key' do subject { FeedManager.instance.key(:home, 1) } @@ -150,5 +154,110 @@ RSpec.describe FeedManager do expect(Redis.current.zcard("feed:type:#{account.id}")).to eq FeedManager::MAX_ITEMS end + + it 'sends push updates for non-home timelines' do + account = Fabricate(:account) + status = Fabricate(:status) + allow(Redis.current).to receive_messages(publish: nil) + + FeedManager.instance.push('type', account, status) + + expect(Redis.current).to have_received(:publish).with("timeline:#{account.id}", any_args).at_least(:once) + end + + context 'reblogs' do + it 'saves reblogs of unseen statuses' do + account = Fabricate(:account) + reblogged = Fabricate(:status) + reblog = Fabricate(:status, reblog: reblogged) + + expect(FeedManager.instance.push('type', account, reblog)).to be true + end + + it 'does not save a new reblog of a recent status' do + account = Fabricate(:account) + reblogged = Fabricate(:status) + reblog = Fabricate(:status, reblog: reblogged) + + FeedManager.instance.push('type', account, reblogged) + + expect(FeedManager.instance.push('type', account, reblog)).to be false + end + + it 'saves a new reblog of an old status' do + account = Fabricate(:account) + reblogged = Fabricate(:status) + reblog = Fabricate(:status, reblog: reblogged) + + FeedManager.instance.push('type', account, reblogged) + + # Fill the feed with intervening statuses + FeedManager::REBLOG_FALLOFF.times do + FeedManager.instance.push('type', account, Fabricate(:status)) + end + + expect(FeedManager.instance.push('type', account, reblog)).to be true + end + + it 'does not save a new reblog of a recently-reblogged status' do + account = Fabricate(:account) + reblogged = Fabricate(:status) + reblogs = 2.times.map { Fabricate(:status, reblog: reblogged) } + + # The first reblog will be accepted + FeedManager.instance.push('type', account, reblogs.first) + + # The second reblog should be ignored + expect(FeedManager.instance.push('type', account, reblogs.last)).to be false + end + + it 'saves a new reblog of a long-ago-reblogged status' do + account = Fabricate(:account) + reblogged = Fabricate(:status) + reblogs = 2.times.map { Fabricate(:status, reblog: reblogged) } + + # The first reblog will be accepted + FeedManager.instance.push('type', account, reblogs.first) + + # Fill the feed with intervening statuses + FeedManager::REBLOG_FALLOFF.times do + FeedManager.instance.push('type', account, Fabricate(:status)) + end + + # The second reblog should also be accepted + expect(FeedManager.instance.push('type', account, reblogs.last)).to be true + end + end + end + + describe '#unpush' do + it 'leaves a reblogged status when deleting the reblog' do + account = Fabricate(:account) + reblogged = Fabricate(:status) + status = Fabricate(:status, reblog: reblogged) + + FeedManager.instance.push('type', account, status) + + # The reblogging status should show up under normal conditions. + expect(Redis.current.zrange("feed:type:#{account.id}", 0, -1)).to eq [status.id.to_s] + + FeedManager.instance.unpush('type', account, status) + + # Because we couldn't tell if the status showed up any other way, + # we had to stick the reblogged status in by itself. + expect(Redis.current.zrange("feed:type:#{account.id}", 0, -1)).to eq [reblogged.id.to_s] + end + + it 'sends push updates' do + account = Fabricate(:account) + status = Fabricate(:status) + FeedManager.instance.push('type', account, status) + + allow(Redis.current).to receive_messages(publish: nil) + FeedManager.instance.unpush('type', account, status) + + deletion = Oj.dump(event: :delete, payload: status.id.to_s) + expect(Redis.current).to have_received(:publish).with("timeline:#{account.id}", deletion) + end end end diff --git a/spec/models/feed_spec.rb b/spec/models/feed_spec.rb index 1c377c17f..5433f44bd 100644 --- a/spec/models/feed_spec.rb +++ b/spec/models/feed_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Feed, type: :model do Fabricate(:status, account: account, id: 3) Fabricate(:status, account: account, id: 10) Redis.current.zadd(FeedManager.instance.key(:home, account.id), - [[4, 'deleted'], [3, 'val3'], [2, 'val2'], [1, 'val1']]) + [[4, 4], [3, 3], [2, 2], [1, 1]]) feed = Feed.new(:home, account) results = feed.get(3) diff --git a/spec/services/batched_remove_status_service_spec.rb b/spec/services/batched_remove_status_service_spec.rb index f5c9adfb5..c82c45e09 100644 --- a/spec/services/batched_remove_status_service_spec.rb +++ b/spec/services/batched_remove_status_service_spec.rb @@ -5,7 +5,7 @@ RSpec.describe BatchedRemoveStatusService do let!(:alice) { Fabricate(:account) } let!(:bob) { Fabricate(:account, username: 'bob', domain: 'example.com', salmon_url: 'http://example.com/salmon') } - let!(:jeff) { Fabricate(:account) } + let!(:jeff) { Fabricate(:user).account } let!(:hank) { Fabricate(:account, username: 'hank', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } let(:status1) { PostStatusService.new.call(alice, 'Hello @bob@example.com') } @@ -19,6 +19,7 @@ RSpec.describe BatchedRemoveStatusService do stub_request(:post, 'http://example.com/inbox').to_return(status: 200) Fabricate(:subscription, account: alice, callback_url: 'http://example.com/push', confirmed: true, expires_at: 30.days.from_now) + jeff.user.update(current_sign_in_at: Time.now) jeff.follow!(alice) hank.follow!(alice) diff --git a/spec/services/precompute_feed_service_spec.rb b/spec/services/precompute_feed_service_spec.rb index dbd08ac1b..d1ef6c184 100644 --- a/spec/services/precompute_feed_service_spec.rb +++ b/spec/services/precompute_feed_service_spec.rb @@ -16,7 +16,7 @@ RSpec.describe PrecomputeFeedService do subject.call(account) - expect(Redis.current.zscore(FeedManager.instance.key(:home, account.id), reblog.id)).to eq status.id + expect(Redis.current.zscore(FeedManager.instance.key(:home, account.id), reblog.id)).to eq status.id.to_f end it 'does not raise an error even if it could not find any status' do -- cgit From 178f718a9b1cab57fbd9df511abe56533f12e129 Mon Sep 17 00:00:00 2001 From: Yamagishi Kazutoshi Date: Wed, 4 Oct 2017 17:22:52 +0900 Subject: Separate notifications preferences from general preferences (#4447) * Separate notifications preferences from general preferences * Refine settings/notifications/show * remove preferences.notifications --- .../settings/notifications_controller.rb | 32 +++++++++++++++++++ app/lib/user_settings_decorator.rb | 26 ++++++++------- app/views/settings/notifications/show.html.haml | 25 +++++++++++++++ app/views/settings/preferences/show.html.haml | 19 ----------- config/locales/de.yml | 2 +- config/locales/en.yml | 2 +- config/locales/ja.yml | 2 +- config/locales/ko.yml | 2 +- config/locales/oc.yml | 2 +- config/locales/pl.yml | 2 +- config/navigation.rb | 1 + config/routes.rb | 1 + .../settings/notifications_controller_spec.rb | 37 ++++++++++++++++++++++ .../settings/preferences_controller_spec.rb | 6 ---- 14 files changed, 117 insertions(+), 42 deletions(-) create mode 100644 app/controllers/settings/notifications_controller.rb create mode 100644 app/views/settings/notifications/show.html.haml create mode 100644 spec/controllers/settings/notifications_controller_spec.rb (limited to 'app/controllers') diff --git a/app/controllers/settings/notifications_controller.rb b/app/controllers/settings/notifications_controller.rb new file mode 100644 index 000000000..09839f16e --- /dev/null +++ b/app/controllers/settings/notifications_controller.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class Settings::NotificationsController < ApplicationController + layout 'admin' + + before_action :authenticate_user! + + def show; end + + def update + user_settings.update(user_settings_params.to_h) + + if current_user.save + redirect_to settings_notifications_path, notice: I18n.t('generic.changes_saved_msg') + else + render :show + end + end + + private + + def user_settings + UserSettingsDecorator.new(current_user) + end + + def user_settings_params + params.require(:user).permit( + notification_emails: %i(follow follow_request reblog favourite mention digest), + interactions: %i(must_be_follower must_be_following) + ) + end +end diff --git a/app/lib/user_settings_decorator.rb b/app/lib/user_settings_decorator.rb index cb1b3c4a9..1053ec488 100644 --- a/app/lib/user_settings_decorator.rb +++ b/app/lib/user_settings_decorator.rb @@ -15,17 +15,17 @@ class UserSettingsDecorator private def process_update - user.settings['notification_emails'] = merged_notification_emails - user.settings['interactions'] = merged_interactions - user.settings['default_privacy'] = default_privacy_preference - user.settings['default_sensitive'] = default_sensitive_preference - user.settings['unfollow_modal'] = unfollow_modal_preference - user.settings['boost_modal'] = boost_modal_preference - user.settings['delete_modal'] = delete_modal_preference - user.settings['auto_play_gif'] = auto_play_gif_preference - user.settings['system_font_ui'] = system_font_ui_preference - user.settings['noindex'] = noindex_preference - user.settings['theme'] = theme_preference + user.settings['notification_emails'] = merged_notification_emails if change?('notification_emails') + user.settings['interactions'] = merged_interactions if change?('interactions') + user.settings['default_privacy'] = default_privacy_preference if change?('setting_default_privacy') + user.settings['default_sensitive'] = default_sensitive_preference if change?('setting_default_sensitive') + user.settings['unfollow_modal'] = unfollow_modal_preference if change?('setting_unfollow_modal') + user.settings['boost_modal'] = boost_modal_preference if change?('setting_boost_modal') + user.settings['delete_modal'] = delete_modal_preference if change?('setting_delete_modal') + user.settings['auto_play_gif'] = auto_play_gif_preference if change?('setting_auto_play_gif') + user.settings['system_font_ui'] = system_font_ui_preference if change?('setting_system_font_ui') + user.settings['noindex'] = noindex_preference if change?('setting_noindex') + user.settings['theme'] = theme_preference if change?('theme') end def merged_notification_emails @@ -83,4 +83,8 @@ class UserSettingsDecorator def coerce_values(params_hash) params_hash.transform_values { |x| x == '1' } end + + def change?(key) + !settings[key].nil? + end end diff --git a/app/views/settings/notifications/show.html.haml b/app/views/settings/notifications/show.html.haml new file mode 100644 index 000000000..80cd615c7 --- /dev/null +++ b/app/views/settings/notifications/show.html.haml @@ -0,0 +1,25 @@ +- content_for :page_title do + = t('settings.notifications') + += simple_form_for current_user, url: settings_notifications_path, html: { method: :put } do |f| + = render 'shared/error_messages', object: current_user + + .fields-group + = f.simple_fields_for :notification_emails, hash_to_object(current_user.settings.notification_emails) do |ff| + = ff.input :follow, as: :boolean, wrapper: :with_label + = ff.input :follow_request, as: :boolean, wrapper: :with_label + = ff.input :reblog, as: :boolean, wrapper: :with_label + = ff.input :favourite, as: :boolean, wrapper: :with_label + = ff.input :mention, as: :boolean, wrapper: :with_label + + .fields-group + = f.simple_fields_for :notification_emails, hash_to_object(current_user.settings.notification_emails) do |ff| + = ff.input :digest, as: :boolean, wrapper: :with_label + + .fields-group + = f.simple_fields_for :interactions, hash_to_object(current_user.settings.interactions) do |ff| + = ff.input :must_be_follower, as: :boolean, wrapper: :with_label + = ff.input :must_be_following, as: :boolean, wrapper: :with_label + + .actions + = f.button :button, t('generic.save_changes'), type: :submit diff --git a/app/views/settings/preferences/show.html.haml b/app/views/settings/preferences/show.html.haml index ffb1bbf6a..7475e3fd2 100644 --- a/app/views/settings/preferences/show.html.haml +++ b/app/views/settings/preferences/show.html.haml @@ -18,25 +18,6 @@ = f.input :setting_default_sensitive, as: :boolean, wrapper: :with_label - %h4= t 'preferences.notifications' - - .fields-group - = f.simple_fields_for :notification_emails, hash_to_object(current_user.settings.notification_emails) do |ff| - = ff.input :follow, as: :boolean, wrapper: :with_label - = ff.input :follow_request, as: :boolean, wrapper: :with_label - = ff.input :reblog, as: :boolean, wrapper: :with_label - = ff.input :favourite, as: :boolean, wrapper: :with_label - = ff.input :mention, as: :boolean, wrapper: :with_label - - .fields-group - = f.simple_fields_for :notification_emails, hash_to_object(current_user.settings.notification_emails) do |ff| - = ff.input :digest, as: :boolean, wrapper: :with_label - - .fields-group - = f.simple_fields_for :interactions, hash_to_object(current_user.settings.interactions) do |ff| - = ff.input :must_be_follower, as: :boolean, wrapper: :with_label - = ff.input :must_be_following, as: :boolean, wrapper: :with_label - %h4= t 'preferences.other' .fields-group diff --git a/config/locales/de.yml b/config/locales/de.yml index dce86409b..d4a925d23 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -319,7 +319,6 @@ de: truncate: "…" preferences: languages: Sprachen - notifications: Benachrichtigungen other: Weiteres publishing: Beiträge web: Web @@ -390,6 +389,7 @@ de: export: Datenexport followers: Autorisierte Folgende import: Datenimport + notifications: Benachrichtigungen preferences: Einstellungen settings: Einstellungen two_factor_authentication: Zwei-Faktor-Authentisierung diff --git a/config/locales/en.yml b/config/locales/en.yml index 3049e0365..4a6df8cb2 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -395,7 +395,6 @@ en: truncate: "…" preferences: languages: Languages - notifications: Notifications other: Other publishing: Publishing web: Web @@ -466,6 +465,7 @@ en: export: Data export followers: Authorized followers import: Import + notifications: Notifications preferences: Preferences settings: Settings two_factor_authentication: Two-factor Authentication diff --git a/config/locales/ja.yml b/config/locales/ja.yml index 78465e121..d637a99ea 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -395,7 +395,6 @@ ja: truncate: "…" preferences: languages: 言語 - notifications: 通知 other: その他 publishing: 投稿 web: ウェブ @@ -466,6 +465,7 @@ ja: export: データのエクスポート followers: 信頼済みのインスタンス import: データのインポート + notifications: 通知 preferences: ユーザー設定 settings: 設定 two_factor_authentication: 二段階認証 diff --git a/config/locales/ko.yml b/config/locales/ko.yml index 3a7636dbb..73f3f3a37 100644 --- a/config/locales/ko.yml +++ b/config/locales/ko.yml @@ -393,7 +393,6 @@ ko: truncate: "…" preferences: languages: 언어 - notifications: 알림 other: 기타 publishing: 퍼블리싱 web: 웹 @@ -464,6 +463,7 @@ ko: export: 데이터 내보내기 followers: 신뢰 중인 인스턴스 import: 데이터 가져오기 + notifications: 알림 preferences: 사용자 설정 settings: 설정 two_factor_authentication: 2단계 인증 diff --git a/config/locales/oc.yml b/config/locales/oc.yml index 0b53b6b2d..1f25525a0 100644 --- a/config/locales/oc.yml +++ b/config/locales/oc.yml @@ -473,7 +473,6 @@ oc: truncate: "…" preferences: languages: Lengas - notifications: Notificacions other: Autre publishing: Publicar web: Interfàcia Web @@ -544,6 +543,7 @@ oc: export: Export donadas followers: Seguidors autorizats import: Importar + notifications: Notificacions preferences: Preferéncias settings: Paramètres two_factor_authentication: Autentificacion en dos temps diff --git a/config/locales/pl.yml b/config/locales/pl.yml index d49ecfbe6..26a8a9c69 100644 --- a/config/locales/pl.yml +++ b/config/locales/pl.yml @@ -396,7 +396,6 @@ pl: truncate: "…" preferences: languages: Języki - notifications: Powiadomienia other: Pozostałe publishing: Publikowanie web: Sieć @@ -467,6 +466,7 @@ pl: export: Eksportowanie danych followers: Autoryzowani śledzący import: Importowanie danych + notifications: Powiadomienia preferences: Preferencje settings: Ustawienia two_factor_authentication: Uwierzytelnianie dwuetapowe diff --git a/config/navigation.rb b/config/navigation.rb index 0a6ab6d3d..215d843b9 100644 --- a/config/navigation.rb +++ b/config/navigation.rb @@ -7,6 +7,7 @@ SimpleNavigation::Configuration.run do |navigation| primary.item :settings, safe_join([fa_icon('cog fw'), t('settings.settings')]), settings_profile_url do |settings| settings.item :profile, safe_join([fa_icon('user fw'), t('settings.edit_profile')]), settings_profile_url settings.item :preferences, safe_join([fa_icon('sliders fw'), t('settings.preferences')]), settings_preferences_url + settings.item :notifications, safe_join([fa_icon('bell fw'), t('settings.notifications')]), settings_notifications_url settings.item :password, safe_join([fa_icon('lock fw'), t('auth.change_password')]), edit_user_registration_url, highlights_on: %r{/auth/edit|/settings/delete} settings.item :two_factor_authentication, safe_join([fa_icon('mobile fw'), t('settings.two_factor_authentication')]), settings_two_factor_authentication_url, highlights_on: %r{/settings/two_factor_authentication} settings.item :import, safe_join([fa_icon('cloud-upload fw'), t('settings.import')]), settings_import_url diff --git a/config/routes.rb b/config/routes.rb index de3c1e0f9..8e80e1510 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -67,6 +67,7 @@ Rails.application.routes.draw do namespace :settings do resource :profile, only: [:show, :update] resource :preferences, only: [:show, :update] + resource :notifications, only: [:show, :update] resource :import, only: [:show, :create] resource :export, only: [:show] diff --git a/spec/controllers/settings/notifications_controller_spec.rb b/spec/controllers/settings/notifications_controller_spec.rb new file mode 100644 index 000000000..0bd993448 --- /dev/null +++ b/spec/controllers/settings/notifications_controller_spec.rb @@ -0,0 +1,37 @@ +require 'rails_helper' + +describe Settings::NotificationsController do + render_views + + let(:user) { Fabricate(:user) } + + before do + sign_in user, scope: :user + end + + describe 'GET #show' do + it 'returns http success' do + get :show + expect(response).to have_http_status(:success) + end + end + + describe 'PUT #update' do + it 'updates notifications settings' do + user.settings['notification_emails'] = user.settings['notification_emails'].merge('follow' => false) + user.settings['interactions'] = user.settings['interactions'].merge('must_be_follower' => true) + + put :update, params: { + user: { + notification_emails: { follow: '1' }, + interactions: { must_be_follower: '0' }, + } + } + + expect(response).to redirect_to(settings_notifications_path) + user.reload + expect(user.settings['notification_emails']['follow']).to be true + expect(user.settings['interactions']['must_be_follower']).to be false + end + end +end diff --git a/spec/controllers/settings/preferences_controller_spec.rb b/spec/controllers/settings/preferences_controller_spec.rb index 60fa42302..0f9431673 100644 --- a/spec/controllers/settings/preferences_controller_spec.rb +++ b/spec/controllers/settings/preferences_controller_spec.rb @@ -29,15 +29,11 @@ describe Settings::PreferencesController do it 'updates user settings' do user.settings['boost_modal'] = false user.settings['delete_modal'] = true - user.settings['notification_emails'] = user.settings['notification_emails'].merge('follow' => false) - user.settings['interactions'] = user.settings['interactions'].merge('must_be_follower' => true) put :update, params: { user: { setting_boost_modal: '1', setting_delete_modal: '0', - notification_emails: { follow: '1' }, - interactions: { must_be_follower: '0' }, } } @@ -45,8 +41,6 @@ describe Settings::PreferencesController do user.reload expect(user.settings['boost_modal']).to be true expect(user.settings['delete_modal']).to be false - expect(user.settings['notification_emails']['follow']).to be true - expect(user.settings['interactions']['must_be_follower']).to be false end end end -- cgit From b3af3f9f8cd5ed9c7ee06452e981b1b7734e1d89 Mon Sep 17 00:00:00 2001 From: utam0k Date: Wed, 4 Oct 2017 22:16:10 +0900 Subject: Implement EmailBlackList (#5109) * Implement BlacklistedEmailDomain * Use Faker::Internet.domain_name * Remove note column * Add frozen_string_literal comment * Delete unnecessary codes * Sort alphabetically * Change of wording * Rename BlacklistedEmailDomain to EmailDomainBlock --- .../admin/email_domain_blocks_controller.rb | 40 +++++++++++++++ app/models/email_domain_block.rb | 17 +++++++ app/validators/blacklisted_email_validator.rb | 1 + .../_email_domain_block.html.haml | 5 ++ .../admin/email_domain_blocks/index.html.haml | 13 +++++ app/views/admin/email_domain_blocks/new.html.haml | 10 ++++ config/locales/en.yml | 10 ++++ config/locales/ja.yml | 10 ++++ config/navigation.rb | 1 + config/routes.rb | 1 + .../20170928082043_create_email_domain_blocks.rb | 9 ++++ db/schema.rb | 8 ++- .../admin/email_domain_blocks_controller_spec.rb | 59 ++++++++++++++++++++++ spec/fabricators/email_domain_block_fabricator.rb | 3 ++ spec/models/email_domain_block_spec.rb | 21 ++++++++ 15 files changed, 207 insertions(+), 1 deletion(-) create mode 100644 app/controllers/admin/email_domain_blocks_controller.rb create mode 100644 app/models/email_domain_block.rb create mode 100644 app/views/admin/email_domain_blocks/_email_domain_block.html.haml create mode 100644 app/views/admin/email_domain_blocks/index.html.haml create mode 100644 app/views/admin/email_domain_blocks/new.html.haml create mode 100644 db/migrate/20170928082043_create_email_domain_blocks.rb create mode 100644 spec/controllers/admin/email_domain_blocks_controller_spec.rb create mode 100644 spec/fabricators/email_domain_block_fabricator.rb create mode 100644 spec/models/email_domain_block_spec.rb (limited to 'app/controllers') diff --git a/app/controllers/admin/email_domain_blocks_controller.rb b/app/controllers/admin/email_domain_blocks_controller.rb new file mode 100644 index 000000000..09275d5dc --- /dev/null +++ b/app/controllers/admin/email_domain_blocks_controller.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module Admin + class EmailDomainBlocksController < BaseController + before_action :set_email_domain_block, only: [:show, :destroy] + + def index + @email_domain_blocks = EmailDomainBlock.page(params[:page]) + end + + def new + @email_domain_block = EmailDomainBlock.new + end + + def create + @email_domain_block = EmailDomainBlock.new(resource_params) + + if @email_domain_block.save + redirect_to admin_email_domain_blocks_path, notice: I18n.t('admin.email_domain_blocks.created_msg') + else + render :new + end + end + + def destroy + @email_domain_block.destroy + redirect_to admin_email_domain_blocks_path, notice: I18n.t('admin.email_domain_blocks.destroyed_msg') + end + + private + + def set_email_domain_block + @email_domain_block = EmailDomainBlock.find(params[:id]) + end + + def resource_params + params.require(:email_domain_block).permit(:domain) + end + end +end diff --git a/app/models/email_domain_block.rb b/app/models/email_domain_block.rb new file mode 100644 index 000000000..839038bea --- /dev/null +++ b/app/models/email_domain_block.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true +# == Schema Information +# +# Table name: email_domain_blocks +# +# id :integer not null, primary key +# domain :string not null +# created_at :datetime not null +# updated_at :datetime not null +# + +class EmailDomainBlock < ApplicationRecord + def self.block?(email) + domain = email.gsub(/.+@([^.]+)/, '\1') + where(domain: domain).exists? + end +end diff --git a/app/validators/blacklisted_email_validator.rb b/app/validators/blacklisted_email_validator.rb index 0ba79694b..3f203f49a 100644 --- a/app/validators/blacklisted_email_validator.rb +++ b/app/validators/blacklisted_email_validator.rb @@ -12,6 +12,7 @@ class BlacklistedEmailValidator < ActiveModel::Validator end def on_blacklist?(value) + return true if EmailDomainBlock.block?(value) return false if Rails.configuration.x.email_domains_blacklist.blank? domains = Rails.configuration.x.email_domains_blacklist.gsub('.', '\.') diff --git a/app/views/admin/email_domain_blocks/_email_domain_block.html.haml b/app/views/admin/email_domain_blocks/_email_domain_block.html.haml new file mode 100644 index 000000000..61cff9395 --- /dev/null +++ b/app/views/admin/email_domain_blocks/_email_domain_block.html.haml @@ -0,0 +1,5 @@ +%tr + %td.domain + %samp= email_domain_block.domain + %td + = table_link_to 'trash', t('admin.email_domain_blocks.delete'), admin_email_domain_block_path(email_domain_block), method: :delete diff --git a/app/views/admin/email_domain_blocks/index.html.haml b/app/views/admin/email_domain_blocks/index.html.haml new file mode 100644 index 000000000..fbdb3b80b --- /dev/null +++ b/app/views/admin/email_domain_blocks/index.html.haml @@ -0,0 +1,13 @@ +- content_for :page_title do + = t('admin.email_domain_blocks.title') + +%table.table + %thead + %tr + %th= t('admin.email_domain_blocks.domain') + %th + %tbody + = render @email_domain_blocks + += paginate @email_domain_blocks += link_to t('admin.email_domain_blocks.add_new'), new_admin_email_domain_block_path, class: 'button' diff --git a/app/views/admin/email_domain_blocks/new.html.haml b/app/views/admin/email_domain_blocks/new.html.haml new file mode 100644 index 000000000..bcae867d9 --- /dev/null +++ b/app/views/admin/email_domain_blocks/new.html.haml @@ -0,0 +1,10 @@ +- content_for :page_title do + = t('.title') + += simple_form_for @email_domain_block, url: admin_email_domain_blocks_path do |f| + = render 'shared/error_messages', object: @email_domain_block + + = f.input :domain, placeholder: t('admin.email_domain_blocks.domain') + + .actions + = f.button :button, t('.create'), type: :submit diff --git a/config/locales/en.yml b/config/locales/en.yml index 4a6df8cb2..5d9557535 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -152,6 +152,16 @@ en: undo: Undo title: Domain Blocks undo: Undo + email_domain_blocks: + add_new: Add new + created_msg: Email domain block successfully created + delete: Delete + destroyed_msg: Email domain block successfully deleted + domain: Domain + new: + create: Create block + title: New email domain block + title: Email Domain Block instances: account_count: Known accounts domain_name: Domain diff --git a/config/locales/ja.yml b/config/locales/ja.yml index d637a99ea..3d6f2fd0b 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -152,6 +152,16 @@ ja: undo: 元に戻す title: ドメインブロック undo: 元に戻す + email_domain_blocks: + add_new: 新規追加 + created_msg: 処理を完了しました + delete: 消去 + destroyed_msg: 消去しました + domain: ドメイン + new: + create: ブロックを作成 + title: 新規メールドメインブロック + title: メールドメインブロック instances: account_count: 既知のアカウント数 domain_name: ドメイン名 diff --git a/config/navigation.rb b/config/navigation.rb index 215d843b9..50bfbd480 100644 --- a/config/navigation.rb +++ b/config/navigation.rb @@ -26,6 +26,7 @@ SimpleNavigation::Configuration.run do |navigation| admin.item :instances, safe_join([fa_icon('cloud fw'), t('admin.instances.title')]), admin_instances_url, highlights_on: %r{/admin/instances} admin.item :subscriptions, safe_join([fa_icon('paper-plane-o fw'), t('admin.subscriptions.title')]), admin_subscriptions_url admin.item :domain_blocks, safe_join([fa_icon('lock fw'), t('admin.domain_blocks.title')]), admin_domain_blocks_url, highlights_on: %r{/admin/domain_blocks} + admin.item :email_domain_blocks, safe_join([fa_icon('envelope fw'), t('admin.email_domain_blocks.title')]), admin_email_domain_blocks_url, highlights_on: %r{/admin/email_domain_blocks} admin.item :sidekiq, safe_join([fa_icon('diamond fw'), 'Sidekiq']), sidekiq_url, link_html: { target: 'sidekiq' } admin.item :pghero, safe_join([fa_icon('database fw'), 'PgHero']), pghero_url, link_html: { target: 'pghero' } admin.item :settings, safe_join([fa_icon('cogs fw'), t('admin.settings.title')]), edit_admin_settings_url diff --git a/config/routes.rb b/config/routes.rb index 8e80e1510..959afc23f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -108,6 +108,7 @@ Rails.application.routes.draw do namespace :admin do resources :subscriptions, only: [:index] resources :domain_blocks, only: [:index, :new, :create, :show, :destroy] + resources :email_domain_blocks, only: [:index, :new, :create, :destroy] resource :settings, only: [:edit, :update] resources :instances, only: [:index] do diff --git a/db/migrate/20170928082043_create_email_domain_blocks.rb b/db/migrate/20170928082043_create_email_domain_blocks.rb new file mode 100644 index 000000000..1f0fb7587 --- /dev/null +++ b/db/migrate/20170928082043_create_email_domain_blocks.rb @@ -0,0 +1,9 @@ +class CreateEmailDomainBlocks < ActiveRecord::Migration[5.1] + def change + create_table :email_domain_blocks do |t| + t.string :domain, null: false + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 00cc24bae..337678c67 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: 20170927215609) do +ActiveRecord::Schema.define(version: 20170928082043) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -110,6 +110,12 @@ ActiveRecord::Schema.define(version: 20170927215609) do t.index ["domain"], name: "index_domain_blocks_on_domain", unique: true end + create_table "email_domain_blocks", force: :cascade do |t| + t.string "domain", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "favourites", force: :cascade do |t| t.datetime "created_at", null: false t.datetime "updated_at", null: false diff --git a/spec/controllers/admin/email_domain_blocks_controller_spec.rb b/spec/controllers/admin/email_domain_blocks_controller_spec.rb new file mode 100644 index 000000000..295de9073 --- /dev/null +++ b/spec/controllers/admin/email_domain_blocks_controller_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Admin::EmailDomainBlocksController, type: :controller do + render_views + + before do + sign_in Fabricate(:user, admin: true), scope: :user + end + + describe 'GET #index' do + around do |example| + default_per_page = EmailDomainBlock.default_per_page + EmailDomainBlock.paginates_per 1 + example.run + EmailDomainBlock.paginates_per default_per_page + end + + it 'renders email blacks' do + 2.times { Fabricate(:email_domain_block) } + + get :index, params: { page: 2 } + + assigned = assigns(:email_domain_blocks) + expect(assigned.count).to eq 1 + expect(assigned.klass).to be EmailDomainBlock + expect(response).to have_http_status(:success) + end + end + + describe 'GET #new' do + it 'assigns a new email black' do + get :new + + expect(assigns(:email_domain_block)).to be_instance_of(EmailDomainBlock) + expect(response).to have_http_status(:success) + end + end + + describe 'POST #create' do + it 'blocks the domain when succeeded to save' do + post :create, params: { email_domain_block: { domain: 'example.com'} } + + expect(flash[:notice]).to eq I18n.t('admin.email_domain_blocks.created_msg') + expect(response).to redirect_to(admin_email_domain_blocks_path) + end + end + + describe 'DELETE #destroy' do + it 'unblocks the domain' do + email_domain_block = Fabricate(:email_domain_block) + delete :destroy, params: { id: email_domain_block.id } + + expect(flash[:notice]).to eq I18n.t('admin.email_domain_blocks.destroyed_msg') + expect(response).to redirect_to(admin_email_domain_blocks_path) + end + end +end diff --git a/spec/fabricators/email_domain_block_fabricator.rb b/spec/fabricators/email_domain_block_fabricator.rb new file mode 100644 index 000000000..d18af6433 --- /dev/null +++ b/spec/fabricators/email_domain_block_fabricator.rb @@ -0,0 +1,3 @@ +Fabricator(:email_domain_block) do + domain { sequence(:domain) { |i| "#{i}#{Faker::Internet.domain_name}" } } +end diff --git a/spec/models/email_domain_block_spec.rb b/spec/models/email_domain_block_spec.rb new file mode 100644 index 000000000..5f5d189d9 --- /dev/null +++ b/spec/models/email_domain_block_spec.rb @@ -0,0 +1,21 @@ +require 'rails_helper' + +RSpec.describe EmailDomainBlock, type: :model do + describe 'validations' do + it 'has a valid fabricator' do + email_domain_block = Fabricate.build(:email_domain_block) + expect(email_domain_block).to be_valid + end + end + + describe 'block?' do + it 'returns true if the domain is registed' do + Fabricate(:email_domain_block, domain: 'example.com') + expect(EmailDomainBlock.block?('nyarn@example.com')).to eq true + end + it 'returns true if the domain is not registed' do + Fabricate(:email_domain_block, domain: 'domain') + expect(EmailDomainBlock.block?('example')).to eq false + end + end +end -- cgit From 49cc0eb3e7d1521079e33a60216df46679082547 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 5 Oct 2017 23:42:05 +0200 Subject: Improve admin UI for custom emojis, add copy/disable/enable (#5231) --- app/controllers/admin/custom_emojis_controller.rb | 42 ++++- app/controllers/api/v1/custom_emojis_controller.rb | 2 +- app/models/account_filter.rb | 2 + app/models/custom_emoji.rb | 11 +- app/models/custom_emoji_filter.rb | 34 ++++ .../admin/custom_emojis/_custom_emoji.html.haml | 13 ++ app/views/admin/custom_emojis/index.html.haml | 20 +++ config/brakeman.ignore | 182 ++++++++++++++++++++- config/locales/de.yml | 6 +- config/locales/en.yml | 7 + config/routes.rb | 8 +- ...20171005171936_add_disabled_to_custom_emojis.rb | 15 ++ db/schema.rb | 3 +- 13 files changed, 330 insertions(+), 15 deletions(-) create mode 100644 app/models/custom_emoji_filter.rb create mode 100644 db/migrate/20171005171936_add_disabled_to_custom_emojis.rb (limited to 'app/controllers') diff --git a/app/controllers/admin/custom_emojis_controller.rb b/app/controllers/admin/custom_emojis_controller.rb index d70514d9a..dba9f1012 100644 --- a/app/controllers/admin/custom_emojis_controller.rb +++ b/app/controllers/admin/custom_emojis_controller.rb @@ -2,8 +2,10 @@ module Admin class CustomEmojisController < BaseController + before_action :set_custom_emoji, except: [:index, :new, :create] + def index - @custom_emojis = CustomEmoji.local + @custom_emojis = filtered_custom_emojis.page(params[:page]) end def new @@ -21,14 +23,50 @@ module Admin end def destroy - CustomEmoji.find(params[:id]).destroy + @custom_emoji.destroy redirect_to admin_custom_emojis_path, notice: I18n.t('admin.custom_emojis.destroyed_msg') end + def copy + emoji = @custom_emoji.dup + emoji.domain = nil + + if emoji.save + redirect_to admin_custom_emojis_path, notice: I18n.t('admin.custom_emojis.copied_msg') + else + redirect_to admin_custom_emojis_path, alert: I18n.t('admin.custom_emojis.copy_failed_msg') + end + end + + def enable + @custom_emoji.update!(disabled: false) + redirect_to admin_custom_emojis_path, notice: I18n.t('admin.custom_emojis.enabled_msg') + end + + def disable + @custom_emoji.update!(disabled: true) + redirect_to admin_custom_emojis_path, notice: I18n.t('admin.custom_emojis.disabled_msg') + end + private + def set_custom_emoji + @custom_emoji = CustomEmoji.find(params[:id]) + end + def resource_params params.require(:custom_emoji).permit(:shortcode, :image) end + + def filtered_custom_emojis + CustomEmojiFilter.new(filter_params).results + end + + def filter_params + params.permit( + :local, + :remote + ) + end end end diff --git a/app/controllers/api/v1/custom_emojis_controller.rb b/app/controllers/api/v1/custom_emojis_controller.rb index 4dd77fb55..f8cd64455 100644 --- a/app/controllers/api/v1/custom_emojis_controller.rb +++ b/app/controllers/api/v1/custom_emojis_controller.rb @@ -4,6 +4,6 @@ class Api::V1::CustomEmojisController < Api::BaseController respond_to :json def index - render json: CustomEmoji.local, each_serializer: REST::CustomEmojiSerializer + render json: CustomEmoji.local.where(disabled: false), each_serializer: REST::CustomEmojiSerializer end end diff --git a/app/models/account_filter.rb b/app/models/account_filter.rb index 1a8cc5192..189872368 100644 --- a/app/models/account_filter.rb +++ b/app/models/account_filter.rb @@ -9,9 +9,11 @@ class AccountFilter def results scope = Account.alphabetic + params.each do |key, value| scope.merge!(scope_for(key, value)) if value.present? end + scope end diff --git a/app/models/custom_emoji.rb b/app/models/custom_emoji.rb index 9e9be5e12..258b50c82 100644 --- a/app/models/custom_emoji.rb +++ b/app/models/custom_emoji.rb @@ -12,6 +12,7 @@ # image_updated_at :datetime # created_at :datetime not null # updated_at :datetime not null +# disabled :boolean default(FALSE), not null # class CustomEmoji < ApplicationRecord @@ -26,10 +27,16 @@ class CustomEmoji < ApplicationRecord validates_attachment :image, content_type: { content_type: 'image/png' }, presence: true, size: { in: 0..50.kilobytes } validates :shortcode, uniqueness: { scope: :domain }, format: { with: /\A#{SHORTCODE_RE_FRAGMENT}\z/ }, length: { minimum: 2 } - scope :local, -> { where(domain: nil) } + scope :local, -> { where(domain: nil) } + scope :remote, -> { where.not(domain: nil) } + scope :alphabetic, -> { order(domain: :asc, shortcode: :asc) } include Remotable + def local? + domain.nil? + end + class << self def from_text(text, domain) return [] if text.blank? @@ -38,7 +45,7 @@ class CustomEmoji < ApplicationRecord return [] if shortcodes.empty? - where(shortcode: shortcodes, domain: domain) + where(shortcode: shortcodes, domain: domain, disabled: false) end end end diff --git a/app/models/custom_emoji_filter.rb b/app/models/custom_emoji_filter.rb new file mode 100644 index 000000000..2d1394a59 --- /dev/null +++ b/app/models/custom_emoji_filter.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class CustomEmojiFilter + attr_reader :params + + def initialize(params) + @params = params + end + + def results + scope = CustomEmoji.alphabetic + + params.each do |key, value| + scope.merge!(scope_for(key, value)) if value.present? + end + + scope + end + + private + + def scope_for(key, value) + case key.to_s + when 'local' + CustomEmoji.local + when 'remote' + CustomEmoji.remote + when 'by_domain' + CustomEmoji.where(domain: value) + else + raise "Unknown filter: #{key}" + end + end +end diff --git a/app/views/admin/custom_emojis/_custom_emoji.html.haml b/app/views/admin/custom_emojis/_custom_emoji.html.haml index ff1aa9925..53263c43f 100644 --- a/app/views/admin/custom_emojis/_custom_emoji.html.haml +++ b/app/views/admin/custom_emojis/_custom_emoji.html.haml @@ -3,5 +3,18 @@ = image_tag custom_emoji.image.url, class: 'emojione', alt: ":#{custom_emoji.shortcode}:" %td %samp= ":#{custom_emoji.shortcode}:" + %td + - if custom_emoji.local? + = t('admin.accounts.location.local') + - else + = custom_emoji.domain + %td + - unless custom_emoji.local? + = table_link_to 'copy', t('admin.custom_emojis.copy'), copy_admin_custom_emoji_path(custom_emoji), method: :post + %td + - if custom_emoji.disabled? + = table_link_to 'power-off', t('admin.custom_emojis.enable'), enable_admin_custom_emoji_path(custom_emoji), method: :post, data: { confirm: t('admin.accounts.are_you_sure') } + - else + = table_link_to 'power-off', t('admin.custom_emojis.disable'), disable_admin_custom_emoji_path(custom_emoji), method: :post, data: { confirm: t('admin.accounts.are_you_sure') } %td = table_link_to 'times', t('admin.custom_emojis.delete'), admin_custom_emoji_path(custom_emoji), method: :delete, data: { confirm: t('admin.accounts.are_you_sure') } diff --git a/app/views/admin/custom_emojis/index.html.haml b/app/views/admin/custom_emojis/index.html.haml index d5f32e84b..20ffb8529 100644 --- a/app/views/admin/custom_emojis/index.html.haml +++ b/app/views/admin/custom_emojis/index.html.haml @@ -1,14 +1,34 @@ - content_for :page_title do = t('admin.custom_emojis.title') +.filters + .filter-subset + %strong= t('admin.accounts.location.title') + %ul + %li= filter_link_to t('admin.accounts.location.all'), local: nil, remote: nil + %li + - if selected? local: '1', remote: nil + = filter_link_to t('admin.accounts.location.local'), {local: nil, remote: nil}, {local: '1', remote: nil} + - else + = filter_link_to t('admin.accounts.location.local'), local: '1', remote: nil + %li + - if selected? remote: '1', local: nil + = filter_link_to t('admin.accounts.location.remote'), {remote: nil, local: nil}, {remote: '1', local: nil} + - else + = filter_link_to t('admin.accounts.location.remote'), remote: '1', local: nil + .table-wrapper %table.table %thead %tr %th= t('admin.custom_emojis.emoji') %th= t('admin.custom_emojis.shortcode') + %th= t('admin.accounts.domain') + %th + %th %th %tbody = render @custom_emojis += paginate @custom_emojis = link_to t('admin.custom_emojis.upload'), new_admin_custom_emoji_path, class: 'button' diff --git a/config/brakeman.ignore b/config/brakeman.ignore index dbb59dd07..ed6e121d2 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -1,5 +1,81 @@ { "ignored_warnings": [ + { + "warning_type": "Cross-Site Scripting", + "warning_code": 4, + "fingerprint": "0adbe361b91afff22ba51e5fc2275ec703cc13255a0cb3eecd8dab223ab9f61e", + "check_name": "LinkToHref", + "message": "Potentially unsafe model attribute in link_to href", + "file": "app/views/admin/accounts/show.html.haml", + "line": 122, + "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", + "code": "link_to(Account.find(params[:id]).inbox_url, Account.find(params[:id]).inbox_url)", + "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":13,"file":"app/controllers/admin/accounts_controller.rb"}], + "location": { + "type": "template", + "template": "admin/accounts/show" + }, + "user_input": "Account.find(params[:id]).inbox_url", + "confidence": "Weak", + "note": "" + }, + { + "warning_type": "Cross-Site Scripting", + "warning_code": 4, + "fingerprint": "1fc29c578d0c89bf13bd5476829d272d54cd06b92ccf6df18568fa1f2674926e", + "check_name": "LinkToHref", + "message": "Potentially unsafe model attribute in link_to href", + "file": "app/views/admin/accounts/show.html.haml", + "line": 128, + "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", + "code": "link_to(Account.find(params[:id]).shared_inbox_url, Account.find(params[:id]).shared_inbox_url)", + "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":13,"file":"app/controllers/admin/accounts_controller.rb"}], + "location": { + "type": "template", + "template": "admin/accounts/show" + }, + "user_input": "Account.find(params[:id]).shared_inbox_url", + "confidence": "Weak", + "note": "" + }, + { + "warning_type": "Cross-Site Scripting", + "warning_code": 4, + "fingerprint": "2129d4c1e63a351d28d8d2937ff0b50237809c3df6725c0c5ef82b881dbb2086", + "check_name": "LinkToHref", + "message": "Potentially unsafe model attribute in link_to href", + "file": "app/views/admin/accounts/show.html.haml", + "line": 35, + "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", + "code": "link_to(Account.find(params[:id]).url, Account.find(params[:id]).url)", + "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":13,"file":"app/controllers/admin/accounts_controller.rb"}], + "location": { + "type": "template", + "template": "admin/accounts/show" + }, + "user_input": "Account.find(params[:id]).url", + "confidence": "Weak", + "note": "" + }, + { + "warning_type": "Dynamic Render Path", + "warning_code": 15, + "fingerprint": "3b0a20b08aef13cf8cf865384fae0cfd3324d8200a83262bf4abbc8091b5fec5", + "check_name": "Render", + "message": "Render path contains parameter value", + "file": "app/views/admin/custom_emojis/index.html.haml", + "line": 31, + "link": "http://brakemanscanner.org/docs/warning_types/dynamic_render_path/", + "code": "render(action => filtered_custom_emojis.page(params[:page]), {})", + "render_path": [{"type":"controller","class":"Admin::CustomEmojisController","method":"index","line":9,"file":"app/controllers/admin/custom_emojis_controller.rb"}], + "location": { + "type": "template", + "template": "admin/custom_emojis/index" + }, + "user_input": "params[:page]", + "confidence": "Weak", + "note": "" + }, { "warning_type": "Dynamic Render Path", "warning_code": 15, @@ -19,6 +95,44 @@ "confidence": "Weak", "note": "" }, + { + "warning_type": "Cross-Site Scripting", + "warning_code": 4, + "fingerprint": "64b5b2a02ede9c2b3598881eb5a466d63f7d27fe0946aa00d570111ec7338d2e", + "check_name": "LinkToHref", + "message": "Potentially unsafe model attribute in link_to href", + "file": "app/views/admin/accounts/show.html.haml", + "line": 131, + "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", + "code": "link_to(Account.find(params[:id]).followers_url, Account.find(params[:id]).followers_url)", + "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":13,"file":"app/controllers/admin/accounts_controller.rb"}], + "location": { + "type": "template", + "template": "admin/accounts/show" + }, + "user_input": "Account.find(params[:id]).followers_url", + "confidence": "Weak", + "note": "" + }, + { + "warning_type": "Cross-Site Scripting", + "warning_code": 4, + "fingerprint": "82f7b0d09beb3ab68e0fa16be63cedf4e820f2490326e9a1cec05761d92446cd", + "check_name": "LinkToHref", + "message": "Potentially unsafe model attribute in link_to href", + "file": "app/views/admin/accounts/show.html.haml", + "line": 106, + "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", + "code": "link_to(Account.find(params[:id]).salmon_url, Account.find(params[:id]).salmon_url)", + "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":13,"file":"app/controllers/admin/accounts_controller.rb"}], + "location": { + "type": "template", + "template": "admin/accounts/show" + }, + "user_input": "Account.find(params[:id]).salmon_url", + "confidence": "Weak", + "note": "" + }, { "warning_type": "Dynamic Render Path", "warning_code": 15, @@ -26,7 +140,7 @@ "check_name": "Render", "message": "Render path contains parameter value", "file": "app/views/admin/accounts/index.html.haml", - "line": 63, + "line": 64, "link": "http://brakemanscanner.org/docs/warning_types/dynamic_render_path/", "code": "render(action => filtered_accounts.page(params[:page]), {})", "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"index","line":10,"file":"app/controllers/admin/accounts_controller.rb"}], @@ -38,6 +152,25 @@ "confidence": "Weak", "note": "" }, + { + "warning_type": "Cross-Site Scripting", + "warning_code": 4, + "fingerprint": "bb0ad5c4a42e06e3846c2089ff5269c17f65483a69414f6ce65eecf2bb11fab7", + "check_name": "LinkToHref", + "message": "Potentially unsafe model attribute in link_to href", + "file": "app/views/admin/accounts/show.html.haml", + "line": 95, + "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", + "code": "link_to(Account.find(params[:id]).remote_url, Account.find(params[:id]).remote_url)", + "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":13,"file":"app/controllers/admin/accounts_controller.rb"}], + "location": { + "type": "template", + "template": "admin/accounts/show" + }, + "user_input": "Account.find(params[:id]).remote_url", + "confidence": "Weak", + "note": "" + }, { "warning_type": "Redirect", "warning_code": 18, @@ -65,7 +198,7 @@ "check_name": "Render", "message": "Render path contains parameter value", "file": "app/views/admin/reports/index.html.haml", - "line": 24, + "line": 25, "link": "http://brakemanscanner.org/docs/warning_types/dynamic_render_path/", "code": "render(action => filtered_reports.page(params[:page]), {})", "render_path": [{"type":"controller","class":"Admin::ReportsController","method":"index","line":9,"file":"app/controllers/admin/reports_controller.rb"}], @@ -77,6 +210,45 @@ "confidence": "Weak", "note": "" }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "cd440d9d0bcb76225f4142030cec0bdec6ad119c537c108c9d514bf87bc34d29", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "lib/mastodon/timestamp_ids.rb", + "line": 69, + "link": "http://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "ActiveRecord::Base.connection.execute(\" CREATE OR REPLACE FUNCTION timestamp_id(table_name text)\\n RETURNS bigint AS\\n $$\\n DECLARE\\n time_part bigint;\\n sequence_base bigint;\\n tail bigint;\\n BEGIN\\n -- Our ID will be composed of the following:\\n -- 6 bytes (48 bits) of millisecond-level timestamp\\n -- 2 bytes (16 bits) of sequence data\\n\\n -- The 'sequence data' is intended to be unique within a\\n -- given millisecond, yet obscure the 'serial number' of\\n -- this row.\\n\\n -- To do this, we hash the following data:\\n -- * Table name (if provided, skipped if not)\\n -- * Secret salt (should not be guessable)\\n -- * Timestamp (again, millisecond-level granularity)\\n\\n -- We then take the first two bytes of that value, and add\\n -- the lowest two bytes of the table ID sequence number\\n -- (`table_name`_id_seq). This means that even if we insert\\n -- two rows at the same millisecond, they will have\\n -- distinct 'sequence data' portions.\\n\\n -- If this happens, and an attacker can see both such IDs,\\n -- they can determine which of the two entries was inserted\\n -- first, but not the total number of entries in the table\\n -- (even mod 2**16).\\n\\n -- The table name is included in the hash to ensure that\\n -- different tables derive separate sequence bases so rows\\n -- inserted in the same millisecond in different tables do\\n -- not reveal the table ID sequence number for one another.\\n\\n -- The secret salt is included in the hash to ensure that\\n -- external users cannot derive the sequence base given the\\n -- timestamp and table name, which would allow them to\\n -- compute the table ID sequence number.\\n\\n time_part := (\\n -- Get the time in milliseconds\\n ((date_part('epoch', now()) * 1000))::bigint\\n -- And shift it over two bytes\\n << 16);\\n\\n sequence_base := (\\n 'x' ||\\n -- Take the first two bytes (four hex characters)\\n substr(\\n -- Of the MD5 hash of the data we documented\\n md5(table_name ||\\n '#{SecureRandom.hex(16)}' ||\\n time_part::text\\n ),\\n 1, 4\\n )\\n -- And turn it into a bigint\\n )::bit(16)::bigint;\\n\\n -- Finally, add our sequence number to our base, and chop\\n -- it to the last two bytes\\n tail := (\\n (sequence_base + nextval(table_name || '_id_seq'))\\n & 65535);\\n\\n -- Return the time part and the sequence part. OR appears\\n -- faster here than addition, but they're equivalent:\\n -- time_part has no trailing two bytes, and tail is only\\n -- the last two bytes.\\n RETURN time_part | tail;\\n END\\n $$ LANGUAGE plpgsql VOLATILE;\\n\")", + "render_path": null, + "location": { + "type": "method", + "class": "Mastodon::TimestampIds", + "method": "s(:self).define_timestamp_id" + }, + "user_input": "SecureRandom.hex(16)", + "confidence": "Medium", + "note": "" + }, + { + "warning_type": "Cross-Site Scripting", + "warning_code": 4, + "fingerprint": "e04aafe1e06cf8317fb6ac0a7f35783e45aa1274272ee6eaf28d39adfdad489b", + "check_name": "LinkToHref", + "message": "Potentially unsafe model attribute in link_to href", + "file": "app/views/admin/accounts/show.html.haml", + "line": 125, + "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", + "code": "link_to(Account.find(params[:id]).outbox_url, Account.find(params[:id]).outbox_url)", + "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":13,"file":"app/controllers/admin/accounts_controller.rb"}], + "location": { + "type": "template", + "template": "admin/accounts/show" + }, + "user_input": "Account.find(params[:id]).outbox_url", + "confidence": "Weak", + "note": "" + }, { "warning_type": "Dynamic Render Path", "warning_code": 15, @@ -84,7 +256,7 @@ "check_name": "Render", "message": "Render path contains parameter value", "file": "app/views/stream_entries/show.html.haml", - "line": 23, + "line": 21, "link": "http://brakemanscanner.org/docs/warning_types/dynamic_render_path/", "code": "render(partial => \"stream_entries/#{Account.find_local!(params[:account_username]).statuses.find(params[:id]).stream_entry.activity_type.downcase}\", { :locals => ({ Account.find_local!(params[:account_username]).statuses.find(params[:id]).stream_entry.activity_type.downcase.to_sym => Account.find_local!(params[:account_username]).statuses.find(params[:id]).stream_entry.activity, :include_threads => true }) })", "render_path": [{"type":"controller","class":"StatusesController","method":"show","line":20,"file":"app/controllers/statuses_controller.rb"}], @@ -97,6 +269,6 @@ "note": "" } ], - "updated": "2017-08-30 05:14:04 +0200", - "brakeman_version": "3.7.2" + "updated": "2017-10-05 20:06:40 +0200", + "brakeman_version": "4.0.1" } diff --git a/config/locales/de.yml b/config/locales/de.yml index ec48bd5ff..7c0edff94 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -137,7 +137,7 @@ de: reject_media: Mediendateien ablehnen reject_media_hint: Entfernt lokal gespeicherte Mediendateien und verhindert deren künftiges Herunterladen. Für Sperren irrelevant severities: - none: Kein + noop: Kein silence: Stummschaltung suspend: Sperren severity: Schweregrad @@ -180,7 +180,7 @@ de: nsfw: 'false': Medienanhänge wieder anzeigen 'true': Medienanhänge verbergen - report: "Meldung #%{id}" + report: 'Meldung #%{id}' report_contents: Inhalt reported_account: Gemeldetes Konto reported_by: Gemeldet von @@ -386,7 +386,7 @@ de: body: "%{name} hat dich erwähnt:" subject: "%{name} hat dich erwähnt" reblog: - body: '%{name} hat deinen Beitrag geteilt:' + body: "%{name} hat deinen Beitrag geteilt:" subject: "%{name} hat deinen Beitrag geteilt" number: human: diff --git a/config/locales/en.yml b/config/locales/en.yml index 5d9557535..2059c5e2b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -109,10 +109,17 @@ en: username: Username web: Web custom_emojis: + copied_msg: Successfully created local copy of the emoji + copy: Copy + copy_failed_msg: Could not make a local copy of that emoji created_msg: Emoji successfully created! delete: Delete destroyed_msg: Emojo successfully destroyed! + disable: Disable + disabled_msg: Successfully disabled that emoji emoji: Emoji + enable: Enable + enabled_msg: Successfully enabled that emoji image_hint: PNG up to 50KB new: title: Add new custom emoji diff --git a/config/routes.rb b/config/routes.rb index 959afc23f..cc1f66e52 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -139,7 +139,13 @@ Rails.application.routes.draw do resource :two_factor_authentication, only: [:destroy] end - resources :custom_emojis, only: [:index, :new, :create, :destroy] + resources :custom_emojis, only: [:index, :new, :create, :destroy] do + member do + post :copy + post :enable + post :disable + end + end end get '/admin', to: redirect('/admin/settings/edit', status: 302) diff --git a/db/migrate/20171005171936_add_disabled_to_custom_emojis.rb b/db/migrate/20171005171936_add_disabled_to_custom_emojis.rb new file mode 100644 index 000000000..067a7bee0 --- /dev/null +++ b/db/migrate/20171005171936_add_disabled_to_custom_emojis.rb @@ -0,0 +1,15 @@ +require Rails.root.join('lib', 'mastodon', 'migration_helpers') + +class AddDisabledToCustomEmojis < ActiveRecord::Migration[5.1] + include Mastodon::MigrationHelpers + + disable_ddl_transaction! + + def up + safety_assured { add_column_with_default :custom_emojis, :disabled, :bool, default: false } + end + + def down + remove_column :custom_emojis, :disabled + end +end diff --git a/db/schema.rb b/db/schema.rb index 337678c67..3358e2997 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: 20170928082043) do +ActiveRecord::Schema.define(version: 20171005171936) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -98,6 +98,7 @@ ActiveRecord::Schema.define(version: 20170928082043) do t.datetime "image_updated_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.boolean "disabled", default: false, null: false t.index ["shortcode", "domain"], name: "index_custom_emojis_on_shortcode_and_domain", unique: true end -- cgit From 3a3475450e46f670e8beaf4bf804b820ad39a5f9 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 7 Oct 2017 17:43:42 +0200 Subject: Encode custom emojis as resolveable objects in ActivityPub (#5243) * Encode custom emojis as resolveable objects in ActivityPub * Improve code style --- app/controllers/accounts_controller.rb | 5 +++- app/controllers/emojis_controller.rb | 22 ++++++++++++++++ app/controllers/follower_accounts_controller.rb | 5 +++- app/controllers/following_accounts_controller.rb | 5 +++- app/controllers/statuses_controller.rb | 10 ++++++-- app/controllers/tags_controller.rb | 5 +++- app/lib/activitypub/activity/create.rb | 12 ++++++--- app/lib/activitypub/tag_manager.rb | 2 ++ app/models/custom_emoji.rb | 6 +++++ app/serializers/activitypub/actor_serializer.rb | 18 ++------------ app/serializers/activitypub/emoji_serializer.rb | 29 ++++++++++++++++++++++ app/serializers/activitypub/image_serializer.rb | 19 ++++++++++++++ app/serializers/activitypub/note_serializer.rb | 17 +------------ config/routes.rb | 5 ++-- .../20171006142024_add_uri_to_custom_emojis.rb | 6 +++++ db/schema.rb | 4 ++- spec/lib/activitypub/activity/create_spec.rb | 10 +++++--- 17 files changed, 132 insertions(+), 48 deletions(-) create mode 100644 app/controllers/emojis_controller.rb create mode 100644 app/serializers/activitypub/emoji_serializer.rb create mode 100644 app/serializers/activitypub/image_serializer.rb create mode 100644 db/migrate/20171006142024_add_uri_to_custom_emojis.rb (limited to 'app/controllers') diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 26ab6636b..75915b337 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -26,7 +26,10 @@ class AccountsController < ApplicationController end format.json do - render json: @account, serializer: ActivityPub::ActorSerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json' + render json: @account, + serializer: ActivityPub::ActorSerializer, + adapter: ActivityPub::Adapter, + content_type: 'application/activity+json' end end end diff --git a/app/controllers/emojis_controller.rb b/app/controllers/emojis_controller.rb new file mode 100644 index 000000000..a82b9340b --- /dev/null +++ b/app/controllers/emojis_controller.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class EmojisController < ApplicationController + before_action :set_emoji + + def show + respond_to do |format| + format.json do + render json: @emoji, + serializer: ActivityPub::EmojiSerializer, + adapter: ActivityPub::Adapter, + content_type: 'application/activity+json' + end + end + end + + private + + def set_emoji + @emoji = CustomEmoji.local.find(params[:id]) + end +end diff --git a/app/controllers/follower_accounts_controller.rb b/app/controllers/follower_accounts_controller.rb index 8eb4d2822..399e79665 100644 --- a/app/controllers/follower_accounts_controller.rb +++ b/app/controllers/follower_accounts_controller.rb @@ -10,7 +10,10 @@ class FollowerAccountsController < ApplicationController format.html format.json do - render json: collection_presenter, serializer: ActivityPub::CollectionSerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json' + render json: collection_presenter, + serializer: ActivityPub::CollectionSerializer, + adapter: ActivityPub::Adapter, + content_type: 'application/activity+json' end end end diff --git a/app/controllers/following_accounts_controller.rb b/app/controllers/following_accounts_controller.rb index 1ca6f0fe7..1e73d4bd4 100644 --- a/app/controllers/following_accounts_controller.rb +++ b/app/controllers/following_accounts_controller.rb @@ -10,7 +10,10 @@ class FollowingAccountsController < ApplicationController format.html format.json do - render json: collection_presenter, serializer: ActivityPub::CollectionSerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json' + render json: collection_presenter, + serializer: ActivityPub::CollectionSerializer, + adapter: ActivityPub::Adapter, + content_type: 'application/activity+json' end end end diff --git a/app/controllers/statuses_controller.rb b/app/controllers/statuses_controller.rb index 65206ea96..e8a360fb5 100644 --- a/app/controllers/statuses_controller.rb +++ b/app/controllers/statuses_controller.rb @@ -21,13 +21,19 @@ class StatusesController < ApplicationController end format.json do - render json: @status, serializer: ActivityPub::NoteSerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json' + render json: @status, + serializer: ActivityPub::NoteSerializer, + adapter: ActivityPub::Adapter, + content_type: 'application/activity+json' end end end def activity - render json: @status, serializer: ActivityPub::ActivitySerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json' + render json: @status, + serializer: ActivityPub::ActivitySerializer, + adapter: ActivityPub::Adapter, + content_type: 'application/activity+json' end def embed diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 3001b2ee3..240ef058a 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -12,7 +12,10 @@ class TagsController < ApplicationController format.html format.json do - render json: collection_presenter, serializer: ActivityPub::CollectionSerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json' + render json: collection_presenter, + serializer: ActivityPub::CollectionSerializer, + adapter: ActivityPub::Adapter, + content_type: 'application/activity+json' end end end diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index be656de48..9421a0aa7 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -86,15 +86,19 @@ class ActivityPub::Activity::Create < ActivityPub::Activity end def process_emoji(tag, _status) - return if tag['name'].blank? || tag['href'].blank? + return if skip_download? + return if tag['name'].blank? || tag['icon'].blank? || tag['icon']['url'].blank? shortcode = tag['name'].delete(':') + image_url = tag['icon']['url'] + uri = tag['id'] + updated = tag['updated'] emoji = CustomEmoji.find_by(shortcode: shortcode, domain: @account.domain) - return if !emoji.nil? || skip_download? + return unless emoji.nil? || emoji.updated_at >= updated - emoji = CustomEmoji.new(domain: @account.domain, shortcode: shortcode) - emoji.image_remote_url = tag['href'] + emoji ||= CustomEmoji.new(domain: @account.domain, shortcode: shortcode, uri: uri) + emoji.image_remote_url = image_url emoji.save end diff --git a/app/lib/activitypub/tag_manager.rb b/app/lib/activitypub/tag_manager.rb index 4ec3b8c56..0708713e6 100644 --- a/app/lib/activitypub/tag_manager.rb +++ b/app/lib/activitypub/tag_manager.rb @@ -33,6 +33,8 @@ class ActivityPub::TagManager when :note, :comment, :activity return activity_account_status_url(target.account, target) if target.reblog? account_status_url(target.account, target) + when :emoji + emoji_url(target) end end diff --git a/app/models/custom_emoji.rb b/app/models/custom_emoji.rb index 258b50c82..65d9840d5 100644 --- a/app/models/custom_emoji.rb +++ b/app/models/custom_emoji.rb @@ -13,6 +13,8 @@ # created_at :datetime not null # updated_at :datetime not null # disabled :boolean default(FALSE), not null +# uri :string +# image_remote_url :string # class CustomEmoji < ApplicationRecord @@ -37,6 +39,10 @@ class CustomEmoji < ApplicationRecord domain.nil? end + def object_type + :emoji + end + class << self def from_text(text, domain) return [] if text.blank? diff --git a/app/serializers/activitypub/actor_serializer.rb b/app/serializers/activitypub/actor_serializer.rb index a11178f5b..896d67115 100644 --- a/app/serializers/activitypub/actor_serializer.rb +++ b/app/serializers/activitypub/actor_serializer.rb @@ -10,20 +10,6 @@ class ActivityPub::ActorSerializer < ActiveModel::Serializer has_one :public_key, serializer: ActivityPub::PublicKeySerializer - class ImageSerializer < ActiveModel::Serializer - include RoutingHelper - - attributes :type, :url - - def type - 'Image' - end - - def url - full_asset_url(object.url(:original)) - end - end - class EndpointsSerializer < ActiveModel::Serializer include RoutingHelper @@ -36,8 +22,8 @@ class ActivityPub::ActorSerializer < ActiveModel::Serializer has_one :endpoints, serializer: EndpointsSerializer - has_one :icon, serializer: ImageSerializer, if: :avatar_exists? - has_one :image, serializer: ImageSerializer, if: :header_exists? + has_one :icon, serializer: ActivityPub::ImageSerializer, if: :avatar_exists? + has_one :image, serializer: ActivityPub::ImageSerializer, if: :header_exists? def id account_url(object) diff --git a/app/serializers/activitypub/emoji_serializer.rb b/app/serializers/activitypub/emoji_serializer.rb new file mode 100644 index 000000000..7b06b1e5d --- /dev/null +++ b/app/serializers/activitypub/emoji_serializer.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class ActivityPub::EmojiSerializer < ActiveModel::Serializer + include RoutingHelper + + attributes :id, :type, :name, :updated + + has_one :icon, serializer: ActivityPub::ImageSerializer + + def id + ActivityPub::TagManager.instance.uri_for(object) + end + + def type + 'Emoji' + end + + def icon + object.image + end + + def updated + object.updated_at.iso8601 + end + + def name + ":#{object.shortcode}:" + end +end diff --git a/app/serializers/activitypub/image_serializer.rb b/app/serializers/activitypub/image_serializer.rb new file mode 100644 index 000000000..a015c6b1b --- /dev/null +++ b/app/serializers/activitypub/image_serializer.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class ActivityPub::ImageSerializer < ActiveModel::Serializer + include RoutingHelper + + attributes :type, :media_type, :url + + def type + 'Image' + end + + def url + full_asset_url(object.url(:original)) + end + + def media_type + object.content_type + end +end diff --git a/app/serializers/activitypub/note_serializer.rb b/app/serializers/activitypub/note_serializer.rb index 4dbf6a444..24c39f3c9 100644 --- a/app/serializers/activitypub/note_serializer.rb +++ b/app/serializers/activitypub/note_serializer.rb @@ -142,21 +142,6 @@ class ActivityPub::NoteSerializer < ActiveModel::Serializer end end - class CustomEmojiSerializer < ActiveModel::Serializer - include RoutingHelper - - attributes :type, :href, :name - - def type - 'Emoji' - end - - def href - full_asset_url(object.image.url) - end - - def name - ":#{object.shortcode}:" - end + class CustomEmojiSerializer < ActivityPub::EmojiSerializer end end diff --git a/config/routes.rb b/config/routes.rb index cc1f66e52..bd7068b5c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -96,8 +96,9 @@ Rails.application.routes.draw do resources :sessions, only: [:destroy] end - resources :media, only: [:show] - resources :tags, only: [:show] + resources :media, only: [:show] + resources :tags, only: [:show] + resources :emojis, only: [:show] get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy diff --git a/db/migrate/20171006142024_add_uri_to_custom_emojis.rb b/db/migrate/20171006142024_add_uri_to_custom_emojis.rb new file mode 100644 index 000000000..04dfcf397 --- /dev/null +++ b/db/migrate/20171006142024_add_uri_to_custom_emojis.rb @@ -0,0 +1,6 @@ +class AddUriToCustomEmojis < ActiveRecord::Migration[5.1] + def change + add_column :custom_emojis, :uri, :string + add_column :custom_emojis, :image_remote_url, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 3358e2997..7180d3515 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: 20171005171936) do +ActiveRecord::Schema.define(version: 20171006142024) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -99,6 +99,8 @@ ActiveRecord::Schema.define(version: 20171005171936) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.boolean "disabled", default: false, null: false + t.string "uri" + t.string "image_remote_url" t.index ["shortcode", "domain"], name: "index_custom_emojis_on_shortcode_and_domain", unique: true end diff --git a/spec/lib/activitypub/activity/create_spec.rb b/spec/lib/activitypub/activity/create_spec.rb index cdd499150..3c3991c13 100644 --- a/spec/lib/activitypub/activity/create_spec.rb +++ b/spec/lib/activitypub/activity/create_spec.rb @@ -290,7 +290,9 @@ RSpec.describe ActivityPub::Activity::Create do tag: [ { type: 'Emoji', - href: 'http://example.com/emoji.png', + icon: { + url: 'http://example.com/emoji.png', + }, name: 'tinking', }, ], @@ -314,7 +316,9 @@ RSpec.describe ActivityPub::Activity::Create do tag: [ { type: 'Emoji', - href: 'http://example.com/emoji.png', + icon: { + url: 'http://example.com/emoji.png', + }, }, ], } @@ -326,7 +330,7 @@ RSpec.describe ActivityPub::Activity::Create do end end - context 'with emojis missing href' do + context 'with emojis missing icon' do let(:object_json) do { id: 'bar', -- cgit From f486ef2666dacbcb6fcd26e371bb5e945369dcfe Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 7 Oct 2017 20:00:35 +0200 Subject: Redesign public hashtag pages (#5237) --- app/controllers/tags_controller.rb | 30 +++++-- .../mastodon/containers/timeline_container.js | 14 +++- .../features/standalone/hashtag_timeline/index.js | 70 +++++++++++++++++ app/javascript/packs/about.js | 6 +- app/javascript/styles/about.scss | 91 ++++++++++++++++++++++ app/javascript/styles/basics.scss | 5 ++ app/javascript/styles/components.scss | 1 + app/views/about/show.html.haml | 2 +- app/views/tags/_og.html.haml | 6 ++ app/views/tags/show.html.haml | 47 +++++++---- config/locales/en.yml | 1 + spec/controllers/tags_controller_spec.rb | 42 ++-------- 12 files changed, 253 insertions(+), 62 deletions(-) create mode 100644 app/javascript/mastodon/features/standalone/hashtag_timeline/index.js create mode 100644 app/views/tags/_og.html.haml (limited to 'app/controllers') diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 240ef058a..9f3090e37 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -1,17 +1,22 @@ # frozen_string_literal: true class TagsController < ApplicationController - layout 'public' + before_action :set_body_classes + before_action :set_instance_presenter def show - @tag = Tag.find_by!(name: params[:id].downcase) - @statuses = Status.as_tag_timeline(@tag, current_account, params[:local]).paginate_by_max_id(20, params[:max_id]) - @statuses = cache_collection(@statuses, Status) + @tag = Tag.find_by!(name: params[:id].downcase) respond_to do |format| - format.html + format.html do + serializable_resource = ActiveModelSerializers::SerializableResource.new(InitialStatePresenter.new(initial_state_params), serializer: InitialStateSerializer) + @initial_state_json = serializable_resource.to_json + end format.json do + @statuses = Status.as_tag_timeline(@tag, current_account, params[:local]).paginate_by_max_id(20, params[:max_id]) + @statuses = cache_collection(@statuses, Status) + render json: collection_presenter, serializer: ActivityPub::CollectionSerializer, adapter: ActivityPub::Adapter, @@ -22,6 +27,14 @@ class TagsController < ApplicationController private + def set_body_classes + @body_classes = 'tag-body' + end + + def set_instance_presenter + @instance_presenter = InstancePresenter.new + end + def collection_presenter ActivityPub::CollectionPresenter.new( id: tag_url(@tag), @@ -30,4 +43,11 @@ class TagsController < ApplicationController items: @statuses.map { |s| ActivityPub::TagManager.instance.uri_for(s) } ) end + + def initial_state_params + { + settings: {}, + token: current_session&.token, + } + end end diff --git a/app/javascript/mastodon/containers/timeline_container.js b/app/javascript/mastodon/containers/timeline_container.js index 6b545ef09..4be037955 100644 --- a/app/javascript/mastodon/containers/timeline_container.js +++ b/app/javascript/mastodon/containers/timeline_container.js @@ -6,6 +6,7 @@ import { hydrateStore } from '../actions/store'; import { IntlProvider, addLocaleData } from 'react-intl'; import { getLocale } from '../locales'; import PublicTimeline from '../features/standalone/public_timeline'; +import HashtagTimeline from '../features/standalone/hashtag_timeline'; const { localeData, messages } = getLocale(); addLocaleData(localeData); @@ -22,15 +23,24 @@ export default class TimelineContainer extends React.PureComponent { static propTypes = { locale: PropTypes.string.isRequired, + hashtag: PropTypes.string, }; render () { - const { locale } = this.props; + const { locale, hashtag } = this.props; + + let timeline; + + if (hashtag) { + timeline = ; + } else { + timeline = ; + } return ( - + {timeline} ); diff --git a/app/javascript/mastodon/features/standalone/hashtag_timeline/index.js b/app/javascript/mastodon/features/standalone/hashtag_timeline/index.js new file mode 100644 index 000000000..f15fbb2f4 --- /dev/null +++ b/app/javascript/mastodon/features/standalone/hashtag_timeline/index.js @@ -0,0 +1,70 @@ +import React from 'react'; +import { connect } from 'react-redux'; +import PropTypes from 'prop-types'; +import StatusListContainer from '../../ui/containers/status_list_container'; +import { + refreshHashtagTimeline, + expandHashtagTimeline, +} from '../../../actions/timelines'; +import Column from '../../../components/column'; +import ColumnHeader from '../../../components/column_header'; + +@connect() +export default class HashtagTimeline extends React.PureComponent { + + static propTypes = { + dispatch: PropTypes.func.isRequired, + hashtag: PropTypes.string.isRequired, + }; + + handleHeaderClick = () => { + this.column.scrollTop(); + } + + setRef = c => { + this.column = c; + } + + componentDidMount () { + const { dispatch, hashtag } = this.props; + + dispatch(refreshHashtagTimeline(hashtag)); + + this.polling = setInterval(() => { + dispatch(refreshHashtagTimeline(hashtag)); + }, 10000); + } + + componentWillUnmount () { + if (typeof this.polling !== 'undefined') { + clearInterval(this.polling); + this.polling = null; + } + } + + handleLoadMore = () => { + this.props.dispatch(expandHashtagTimeline(this.props.hashtag)); + } + + render () { + const { hashtag } = this.props; + + return ( + + + + + + ); + } + +} diff --git a/app/javascript/packs/about.js b/app/javascript/packs/about.js index 6705377c1..50c81198e 100644 --- a/app/javascript/packs/about.js +++ b/app/javascript/packs/about.js @@ -4,9 +4,9 @@ require.context('../images/', true); function loaded() { const TimelineContainer = require('../mastodon/containers/timeline_container').default; - const React = require('react'); - const ReactDOM = require('react-dom'); - const mountNode = document.getElementById('mastodon-timeline'); + const React = require('react'); + const ReactDOM = require('react-dom'); + const mountNode = document.getElementById('mastodon-timeline'); if (mountNode !== null) { const props = JSON.parse(mountNode.getAttribute('data-props')); diff --git a/app/javascript/styles/about.scss b/app/javascript/styles/about.scss index 2adcb5ba2..a15afc32c 100644 --- a/app/javascript/styles/about.scss +++ b/app/javascript/styles/about.scss @@ -481,6 +481,7 @@ flex: 0 0 auto; background: $ui-base-color; overflow: hidden; + border-radius: 4px; box-shadow: 0 0 6px rgba($black, 0.1); .column-header { @@ -703,8 +704,98 @@ .features #mastodon-timeline { height: 70vh; width: 100%; + min-width: 330px; margin-bottom: 50px; + + .column { + width: 100%; + } + } + } + + .cta { + margin: 20px; + } + + &.tag-page { + .brand { + padding-top: 20px; + margin-bottom: 20px; + + img { + height: 48px; + width: auto; + } + } + + .container { + max-width: 690px; + } + + .cta { + margin: 40px 0; + margin-bottom: 80px; + + .button { + margin-right: 4px; + } + } + + .about-mastodon { + max-width: 330px; + + p { + strong { + color: $ui-secondary-color; + font-weight: 700; + } + } } + + @media screen and (max-width: 675px) { + .container { + display: flex; + flex-direction: column; + } + + .features { + padding: 20px 0; + } + + .about-mastodon { + order: 1; + flex: 0 0 auto; + max-width: 100%; + } + + #mastodon-timeline { + order: 2; + flex: 0 0 auto; + height: 60vh; + } + + .cta { + margin: 20px 0; + margin-bottom: 30px; + } + + .features-list { + display: none; + } + + .stripe { + display: none; + } + } + } + + .stripe { + width: 100%; + height: 360px; + overflow: hidden; + background: darken($ui-base-color, 4%); + position: absolute; + z-index: -1; } } diff --git a/app/javascript/styles/basics.scss b/app/javascript/styles/basics.scss index 0018c9a5d..500e506f6 100644 --- a/app/javascript/styles/basics.scss +++ b/app/javascript/styles/basics.scss @@ -42,6 +42,11 @@ body { padding-bottom: 0; } + &.tag-body { + background: darken($ui-base-color, 8%); + padding-bottom: 0; + } + &.embed { background: transparent; margin: 0; diff --git a/app/javascript/styles/components.scss b/app/javascript/styles/components.scss index 6c64528d6..0e7022e9b 100644 --- a/app/javascript/styles/components.scss +++ b/app/javascript/styles/components.scss @@ -66,6 +66,7 @@ text-transform: none; background: transparent; padding: 3px 15px; + border-radius: 4px; border: 1px solid $ui-primary-color; &:active, diff --git a/app/views/about/show.html.haml b/app/views/about/show.html.haml index 0d311b895..ef27d07a1 100644 --- a/app/views/about/show.html.haml +++ b/app/views/about/show.html.haml @@ -62,7 +62,7 @@ .about-mastodon %h3= t 'about.what_is_mastodon' %p= t 'about.about_mastodon_html' - %a.button.button-secondary{ href: 'https://joinmastodon.org/' }= t 'about.learn_more' + = link_to t('about.learn_more'), 'https://joinmastodon.org/', class: 'button button-secondary' = render 'features' .footer-links .container diff --git a/app/views/tags/_og.html.haml b/app/views/tags/_og.html.haml new file mode 100644 index 000000000..853a499ae --- /dev/null +++ b/app/views/tags/_og.html.haml @@ -0,0 +1,6 @@ += opengraph 'og:site_name', t('about.hosted_on', domain: site_hostname) += opengraph 'og:url', tag_url(@tag) += opengraph 'og:type', 'website' += opengraph 'og:title', "##{@tag.name}" += opengraph 'og:description', t('about.about_hashtag_html', hashtag: @tag.name) += opengraph 'twitter:card', 'summary' diff --git a/app/views/tags/show.html.haml b/app/views/tags/show.html.haml index 8cd2f1825..6266d3c0c 100644 --- a/app/views/tags/show.html.haml +++ b/app/views/tags/show.html.haml @@ -1,19 +1,38 @@ - content_for :page_title do = "##{@tag.name}" -.compact-header - %h1< - = link_to site_title, root_path - %br - %small ##{@tag.name} +- content_for :header_tags do + %script#initial-state{ type: 'application/json' }!= json_escape(@initial_state_json) + = javascript_pack_tag 'about', integrity: true, crossorigin: 'anonymous' + = render 'og' -- if @statuses.empty? - .accounts-grid - = render partial: 'accounts/nothing_here' -- else - .activity-stream.h-feed - = render partial: 'stream_entries/status', collection: @statuses, as: :status +.landing-page.tag-page + .stripe + .features + .container + #mastodon-timeline{ data: { props: Oj.dump(default_props.merge(hashtag: @tag.name)) } } -- if @statuses.size == 20 - .pagination - = link_to safe_join([t('pagination.next'), fa_icon('chevron-right')], ' '), tag_url(@tag, max_id: @statuses.last.id), class: 'next', rel: 'next' + .about-mastodon + .brand + = link_to root_url do + = image_tag asset_pack_path('logo_full.svg'), alt: 'Mastodon' + + %p= t 'about.about_hashtag_html', hashtag: @tag.name + + .cta + = link_to t('auth.login'), new_user_session_path, class: 'button button-secondary' + = link_to t('about.learn_more'), root_url, class: 'button button-alternative' + + .features-list + .features-list__row + .text + %h6= t 'about.features.not_a_product_title' + = t 'about.features.not_a_product_body' + .visual + = fa_icon 'fw users' + .features-list__row + .text + %h6= t 'about.features.humane_approach_title' + = t 'about.features.humane_approach_body' + .visual + = fa_icon 'fw leaf' diff --git a/config/locales/en.yml b/config/locales/en.yml index 2059c5e2b..82041be24 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2,6 +2,7 @@ en: about: about_mastodon_html: Mastodon is a social network based on open web protocols and free, open-source software. It is decentralized like e-mail. + about_hashtag_html: These are public toots tagged with #%{hashtag}. You can interact with them if you have an account anywhere in the fediverse. about_this: About closed_registrations: Registrations are currently closed on this instance. However! You can find a different instance to make an account on and get access to the very same network from there. contact: Contact diff --git a/spec/controllers/tags_controller_spec.rb b/spec/controllers/tags_controller_spec.rb index 3f46c14c0..b04666c0f 100644 --- a/spec/controllers/tags_controller_spec.rb +++ b/spec/controllers/tags_controller_spec.rb @@ -5,9 +5,9 @@ RSpec.describe TagsController, type: :controller do describe 'GET #show' do let!(:tag) { Fabricate(:tag, name: 'test') } - let!(:local) { Fabricate(:status, tags: [ tag ], text: 'local #test') } - let!(:remote) { Fabricate(:status, tags: [ tag ], text: 'remote #test', account: Fabricate(:account, domain: 'remote')) } - let!(:late) { Fabricate(:status, tags: [ tag ], text: 'late #test') } + let!(:local) { Fabricate(:status, tags: [tag], text: 'local #test') } + let!(:remote) { Fabricate(:status, tags: [tag], text: 'remote #test', account: Fabricate(:account, domain: 'remote')) } + let!(:late) { Fabricate(:status, tags: [tag], text: 'late #test') } context 'when tag exists' do it 'returns http success' do @@ -15,41 +15,9 @@ RSpec.describe TagsController, type: :controller do expect(response).to have_http_status(:success) end - it 'renders public layout' do + it 'renders application layout' do get :show, params: { id: 'test', max_id: late.id } - expect(response).to render_template layout: 'public' - end - - it 'renders only local statuses if local parameter is specified' do - get :show, params: { id: 'test', local: true, max_id: late.id } - - expect(assigns(:tag)).to eq tag - statuses = assigns(:statuses).to_a - expect(statuses.size).to eq 1 - expect(statuses[0]).to eq local - end - - it 'renders local and remote statuses if local parameter is not specified' do - get :show, params: { id: 'test', max_id: late.id } - - expect(assigns(:tag)).to eq tag - statuses = assigns(:statuses).to_a - expect(statuses.size).to eq 2 - expect(statuses[0]).to eq remote - expect(statuses[1]).to eq local - end - - it 'filters statuses by the current account' do - user = Fabricate(:user) - user.account.block!(remote.account) - - sign_in(user) - get :show, params: { id: 'test', max_id: late.id } - - expect(assigns(:tag)).to eq tag - statuses = assigns(:statuses).to_a - expect(statuses.size).to eq 1 - expect(statuses[0]).to eq local + expect(response).to render_template layout: 'application' end end -- cgit From 633426b2616e8559acfa76f4294a51afcf434fc2 Mon Sep 17 00:00:00 2001 From: nullkal Date: Sun, 8 Oct 2017 03:26:43 +0900 Subject: Add moderation note (#5240) * Add moderation note * Add frozen_string_literal * Make rspec pass --- .../admin/account_moderation_notes_controller.rb | 31 ++++++++++++++++++++++ app/controllers/admin/accounts_controller.rb | 5 +++- .../admin/account_moderation_notes_helper.rb | 4 +++ app/models/account.rb | 4 +++ app/models/account_moderation_note.rb | 22 +++++++++++++++ .../_account_moderation_note.html.haml | 10 +++++++ app/views/admin/accounts/show.html.haml | 22 +++++++++++++++ config/locales/en.yml | 10 +++++++ config/routes.rb | 2 ++ ...171005102658_create_account_moderation_notes.rb | 12 +++++++++ db/schema.rb | 11 ++++++++ .../account_moderation_notes_controller_spec.rb | 4 +++ .../account_moderation_note_fabricator.rb | 4 +++ .../admin/account_moderation_notes_helper_spec.rb | 15 +++++++++++ spec/models/account_moderation_note_spec.rb | 5 ++++ 15 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 app/controllers/admin/account_moderation_notes_controller.rb create mode 100644 app/helpers/admin/account_moderation_notes_helper.rb create mode 100644 app/models/account_moderation_note.rb create mode 100644 app/views/admin/account_moderation_notes/_account_moderation_note.html.haml create mode 100644 db/migrate/20171005102658_create_account_moderation_notes.rb create mode 100644 spec/controllers/admin/account_moderation_notes_controller_spec.rb create mode 100644 spec/fabricators/account_moderation_note_fabricator.rb create mode 100644 spec/helpers/admin/account_moderation_notes_helper_spec.rb create mode 100644 spec/models/account_moderation_note_spec.rb (limited to 'app/controllers') diff --git a/app/controllers/admin/account_moderation_notes_controller.rb b/app/controllers/admin/account_moderation_notes_controller.rb new file mode 100644 index 000000000..414a875d0 --- /dev/null +++ b/app/controllers/admin/account_moderation_notes_controller.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class Admin::AccountModerationNotesController < Admin::BaseController + def create + @account_moderation_note = current_account.account_moderation_notes.new(resource_params) + if @account_moderation_note.save + @target_account = @account_moderation_note.target_account + redirect_to admin_account_path(@target_account.id), notice: I18n.t('admin.account_moderation_notes.created_msg') + else + @account = @account_moderation_note.target_account + @moderation_notes = @account.targeted_moderation_notes.latest + render template: 'admin/accounts/show' + end + end + + def destroy + @account_moderation_note = AccountModerationNote.find(params[:id]) + @target_account = @account_moderation_note.target_account + @account_moderation_note.destroy + redirect_to admin_account_path(@target_account.id), notice: I18n.t('admin.account_moderation_notes.destroyed_msg') + end + + private + + def resource_params + params.require(:account_moderation_note).permit( + :content, + :target_account_id + ) + end +end diff --git a/app/controllers/admin/accounts_controller.rb b/app/controllers/admin/accounts_controller.rb index 54c659e1b..ffa4dc850 100644 --- a/app/controllers/admin/accounts_controller.rb +++ b/app/controllers/admin/accounts_controller.rb @@ -9,7 +9,10 @@ module Admin @accounts = filtered_accounts.page(params[:page]) end - def show; end + def show + @account_moderation_note = current_account.account_moderation_notes.new(target_account: @account) + @moderation_notes = @account.targeted_moderation_notes.latest + end def subscribe Pubsubhubbub::SubscribeWorker.perform_async(@account.id) diff --git a/app/helpers/admin/account_moderation_notes_helper.rb b/app/helpers/admin/account_moderation_notes_helper.rb new file mode 100644 index 000000000..b17c52264 --- /dev/null +++ b/app/helpers/admin/account_moderation_notes_helper.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +module Admin::AccountModerationNotesHelper +end diff --git a/app/models/account.rb b/app/models/account.rb index 54035d94a..88f16026d 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -90,6 +90,10 @@ class Account < ApplicationRecord has_many :reports has_many :targeted_reports, class_name: 'Report', foreign_key: :target_account_id + # Moderation notes + has_many :account_moderation_notes + has_many :targeted_moderation_notes, class_name: 'AccountModerationNote', foreign_key: :target_account_id + scope :remote, -> { where.not(domain: nil) } scope :local, -> { where(domain: nil) } scope :without_followers, -> { where(followers_count: 0) } diff --git a/app/models/account_moderation_note.rb b/app/models/account_moderation_note.rb new file mode 100644 index 000000000..be52d10b6 --- /dev/null +++ b/app/models/account_moderation_note.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# == Schema Information +# +# Table name: account_moderation_notes +# +# id :integer not null, primary key +# content :text not null +# account_id :integer +# target_account_id :integer +# created_at :datetime not null +# updated_at :datetime not null +# + +class AccountModerationNote < ApplicationRecord + belongs_to :account + belongs_to :target_account, class_name: 'Account' + + scope :latest, -> { reorder('created_at DESC') } + + validates :content, presence: true, length: { maximum: 500 } +end diff --git a/app/views/admin/account_moderation_notes/_account_moderation_note.html.haml b/app/views/admin/account_moderation_notes/_account_moderation_note.html.haml new file mode 100644 index 000000000..4651630e9 --- /dev/null +++ b/app/views/admin/account_moderation_notes/_account_moderation_note.html.haml @@ -0,0 +1,10 @@ +%tr + %td + = simple_format(h(account_moderation_note.content)) + %td + = account_moderation_note.account.acct + %td + %time.formatted{ datetime: account_moderation_note.created_at.iso8601, title: l(account_moderation_note.created_at) } + = l account_moderation_note.created_at + %td + = link_to t('admin.account_moderation_notes.delete'), admin_account_moderation_note_path(account_moderation_note), method: :delete diff --git a/app/views/admin/accounts/show.html.haml b/app/views/admin/accounts/show.html.haml index 3775b6721..1f5c8fcf5 100644 --- a/app/views/admin/accounts/show.html.haml +++ b/app/views/admin/accounts/show.html.haml @@ -129,3 +129,25 @@ %tr %th= t('admin.accounts.followers_url') %td= link_to @account.followers_url, @account.followers_url + +%hr +%h3= t('admin.accounts.moderation_notes') + += simple_form_for @account_moderation_note, url: admin_account_moderation_notes_path do |f| + = render 'shared/error_messages', object: @account_moderation_note + + = f.input :content + = f.hidden_field :target_account_id + + .actions + = f.button :button, t('admin.account_moderation_notes.create'), type: :submit + +.table-wrapper + %table.table + %thead + %tr + %th + %th= t('admin.account_moderation_notes.account') + %th= t('admin.account_moderation_notes.created_at') + %tbody + = render @moderation_notes diff --git a/config/locales/en.yml b/config/locales/en.yml index 82041be24..7d2596fc6 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -76,6 +76,7 @@ en: silenced: Silenced suspended: Suspended title: Moderation + moderation_notes: Moderation notes most_recent_activity: Most recent activity most_recent_ip: Most recent IP not_subscribed: Not subscribed @@ -109,6 +110,15 @@ en: unsubscribe: Unsubscribe username: Username web: Web + + account_moderation_notes: + account: Moderator + created_at: Date + create: Create + created_msg: Moderation note successfully created! + delete: Delete + destroyed_msg: Moderation note successfully destroyed! + custom_emojis: copied_msg: Successfully created local copy of the emoji copy: Copy diff --git a/config/routes.rb b/config/routes.rb index bd7068b5c..5a6351f77 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -147,6 +147,8 @@ Rails.application.routes.draw do post :disable end end + + resources :account_moderation_notes, only: [:create, :destroy] end get '/admin', to: redirect('/admin/settings/edit', status: 302) diff --git a/db/migrate/20171005102658_create_account_moderation_notes.rb b/db/migrate/20171005102658_create_account_moderation_notes.rb new file mode 100644 index 000000000..d1802b5b3 --- /dev/null +++ b/db/migrate/20171005102658_create_account_moderation_notes.rb @@ -0,0 +1,12 @@ +class CreateAccountModerationNotes < ActiveRecord::Migration[5.1] + def change + create_table :account_moderation_notes do |t| + t.text :content, null: false + t.references :account + t.references :target_account + + t.timestamps + end + add_foreign_key :account_moderation_notes, :accounts, column: :target_account_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 7180d3515..91f1b1acb 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -23,6 +23,16 @@ ActiveRecord::Schema.define(version: 20171006142024) do t.index ["account_id", "domain"], name: "index_account_domain_blocks_on_account_id_and_domain", unique: true end + create_table "account_moderation_notes", force: :cascade do |t| + t.text "content", null: false + t.bigint "account_id" + t.bigint "target_account_id" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["account_id"], name: "index_account_moderation_notes_on_account_id" + t.index ["target_account_id"], name: "index_account_moderation_notes_on_target_account_id" + end + create_table "accounts", force: :cascade do |t| t.string "username", default: "", null: false t.string "domain" @@ -449,6 +459,7 @@ ActiveRecord::Schema.define(version: 20171006142024) do end add_foreign_key "account_domain_blocks", "accounts", name: "fk_206c6029bd", on_delete: :cascade + add_foreign_key "account_moderation_notes", "accounts", column: "target_account_id" add_foreign_key "blocks", "accounts", column: "target_account_id", name: "fk_9571bfabc1", on_delete: :cascade add_foreign_key "blocks", "accounts", name: "fk_4269e03e65", on_delete: :cascade add_foreign_key "conversation_mutes", "accounts", name: "fk_225b4212bb", on_delete: :cascade diff --git a/spec/controllers/admin/account_moderation_notes_controller_spec.rb b/spec/controllers/admin/account_moderation_notes_controller_spec.rb new file mode 100644 index 000000000..ca4e55c4d --- /dev/null +++ b/spec/controllers/admin/account_moderation_notes_controller_spec.rb @@ -0,0 +1,4 @@ +require 'rails_helper' + +RSpec.describe Admin::AccountModerationNotesController, type: :controller do +end diff --git a/spec/fabricators/account_moderation_note_fabricator.rb b/spec/fabricators/account_moderation_note_fabricator.rb new file mode 100644 index 000000000..9277af165 --- /dev/null +++ b/spec/fabricators/account_moderation_note_fabricator.rb @@ -0,0 +1,4 @@ +Fabricator(:account_moderation_note) do + content "MyText" + account nil +end diff --git a/spec/helpers/admin/account_moderation_notes_helper_spec.rb b/spec/helpers/admin/account_moderation_notes_helper_spec.rb new file mode 100644 index 000000000..01b60c851 --- /dev/null +++ b/spec/helpers/admin/account_moderation_notes_helper_spec.rb @@ -0,0 +1,15 @@ +require 'rails_helper' + +# Specs in this file have access to a helper object that includes +# the Admin::AccountModerationNotesHelper. For example: +# +# describe Admin::AccountModerationNotesHelper do +# describe "string concat" do +# it "concats two strings with spaces" do +# expect(helper.concat_strings("this","that")).to eq("this that") +# end +# end +# end +RSpec.describe Admin::AccountModerationNotesHelper, type: :helper do + pending "add some examples to (or delete) #{__FILE__}" +end diff --git a/spec/models/account_moderation_note_spec.rb b/spec/models/account_moderation_note_spec.rb new file mode 100644 index 000000000..c4be8c4af --- /dev/null +++ b/spec/models/account_moderation_note_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe AccountModerationNote, type: :model do + pending "add some examples to (or delete) #{__FILE__}" +end -- cgit From cc796298c9b1c1d2e8b6d36311eb9acc95ab8dc0 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Tue, 10 Oct 2017 00:30:31 +0900 Subject: Fix pagination in Api::V1::BlocksController (#5285) --- app/controllers/api/v1/blocks_controller.rb | 26 +++++++------- spec/controllers/api/v1/blocks_controller_spec.rb | 42 ++++++++++++++++++++--- 2 files changed, 49 insertions(+), 19 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/api/v1/blocks_controller.rb b/app/controllers/api/v1/blocks_controller.rb index a412e4341..3a6690766 100644 --- a/app/controllers/api/v1/blocks_controller.rb +++ b/app/controllers/api/v1/blocks_controller.rb @@ -15,19 +15,17 @@ class Api::V1::BlocksController < Api::BaseController private def load_accounts - default_accounts.merge(paginated_blocks).to_a - end - - def default_accounts - Account.includes(:blocked_by).references(:blocked_by) + paginated_blocks.map(&:target_account) end def paginated_blocks - Block.where(account: current_account).paginate_by_max_id( - limit_param(DEFAULT_ACCOUNTS_LIMIT), - params[:max_id], - params[:since_id] - ) + @paginated_blocks ||= Block.eager_load(:target_account) + .where(account: current_account) + .paginate_by_max_id( + limit_param(DEFAULT_ACCOUNTS_LIMIT), + params[:max_id], + params[:since_id] + ) end def insert_pagination_headers @@ -41,21 +39,21 @@ class Api::V1::BlocksController < Api::BaseController end def prev_path - unless @accounts.empty? + unless paginated_blocks.empty? api_v1_blocks_url pagination_params(since_id: pagination_since_id) end end def pagination_max_id - @accounts.last.blocked_by_ids.last + paginated_blocks.last.id end def pagination_since_id - @accounts.first.blocked_by_ids.first + paginated_blocks.first.id end def records_continue? - @accounts.size == limit_param(DEFAULT_ACCOUNTS_LIMIT) + paginated_blocks.size == limit_param(DEFAULT_ACCOUNTS_LIMIT) end def pagination_params(core_params) diff --git a/spec/controllers/api/v1/blocks_controller_spec.rb b/spec/controllers/api/v1/blocks_controller_spec.rb index f25a7e878..9b2bbdf0e 100644 --- a/spec/controllers/api/v1/blocks_controller_spec.rb +++ b/spec/controllers/api/v1/blocks_controller_spec.rb @@ -6,15 +6,47 @@ RSpec.describe Api::V1::BlocksController, type: :controller do let(:user) { Fabricate(:user, account: Fabricate(:account, username: 'alice')) } let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'follow') } - before do - Fabricate(:block, account: user.account) - allow(controller).to receive(:doorkeeper_token) { token } - end + before { allow(controller).to receive(:doorkeeper_token) { token } } describe 'GET #index' do - it 'returns http success' do + it 'limits according to limit parameter' do + 2.times.map { Fabricate(:block, account: user.account) } get :index, params: { limit: 1 } + expect(body_as_json.size).to eq 1 + end + + it 'queries blocks in range according to max_id' do + blocks = 2.times.map { Fabricate(:block, account: user.account) } + + get :index, params: { max_id: blocks[1] } + + expect(body_as_json.size).to eq 1 + expect(body_as_json[0][:id]).to eq blocks[0].target_account_id.to_s + end + + it 'queries blocks in range according to since_id' do + blocks = 2.times.map { Fabricate(:block, account: user.account) } + get :index, params: { since_id: blocks[0] } + + expect(body_as_json.size).to eq 1 + expect(body_as_json[0][:id]).to eq blocks[1].target_account_id.to_s + end + + it 'sets pagination header for next path' do + blocks = 2.times.map { Fabricate(:block, account: user.account) } + get :index, params: { limit: 1, since_id: blocks[0] } + expect(response.headers['Link'].find_link(['rel', 'next']).href).to eq api_v1_blocks_url(limit: 1, max_id: blocks[1]) + end + + it 'sets pagination header for previous path' do + block = Fabricate(:block, account: user.account) + get :index + expect(response.headers['Link'].find_link(['rel', 'prev']).href).to eq api_v1_blocks_url(since_id: block) + end + + it 'returns http success' do + get :index expect(response).to have_http_status(:success) end end -- cgit From 61d3ecc8055cc9e72826e92638caa5f667023683 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 10 Oct 2017 15:18:27 +0200 Subject: Fix custom emoji copy not copying file (#5298) --- app/controllers/admin/custom_emojis_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'app/controllers') diff --git a/app/controllers/admin/custom_emojis_controller.rb b/app/controllers/admin/custom_emojis_controller.rb index dba9f1012..ca81f3255 100644 --- a/app/controllers/admin/custom_emojis_controller.rb +++ b/app/controllers/admin/custom_emojis_controller.rb @@ -28,8 +28,7 @@ module Admin end def copy - emoji = @custom_emoji.dup - emoji.domain = nil + emoji = CustomEmoji.new(domain: nil, shortcode: @custom_emoji.shortcode, image: @custom_emoji.image) if emoji.save redirect_to admin_custom_emojis_path, notice: I18n.t('admin.custom_emojis.copied_msg') -- cgit From 552d22bec9602a64616538f7df0bdac13140c7f8 Mon Sep 17 00:00:00 2001 From: takayamaki Date: Wed, 11 Oct 2017 07:52:25 +0900 Subject: sign_in and sign_up views present og meta infos (#5308) --- app/controllers/auth/registrations_controller.rb | 5 +++++ app/controllers/auth/sessions_controller.rb | 5 +++++ app/views/about/_og.html.haml | 10 ---------- app/views/about/more.html.haml | 2 +- app/views/about/show.html.haml | 2 +- app/views/auth/registrations/new.html.haml | 3 +++ app/views/auth/sessions/new.html.haml | 3 +++ app/views/shared/_og.html.haml | 10 ++++++++++ 8 files changed, 28 insertions(+), 12 deletions(-) delete mode 100644 app/views/about/_og.html.haml create mode 100644 app/views/shared/_og.html.haml (limited to 'app/controllers') diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 60ace04d7..aac3c31ff 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -6,6 +6,7 @@ class Auth::RegistrationsController < Devise::RegistrationsController before_action :check_enabled_registrations, only: [:new, :create] before_action :configure_sign_up_params, only: [:create] before_action :set_sessions, only: [:edit, :update] + before_action :set_instance_presenter, only: [:new, :update] def destroy not_found @@ -39,6 +40,10 @@ class Auth::RegistrationsController < Devise::RegistrationsController private + def set_instance_presenter + @instance_presenter = InstancePresenter.new + end + def determine_layout %w(edit update).include?(action_name) ? 'admin' : 'auth' end diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index bc3bd2f4b..463a183e4 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -8,6 +8,7 @@ class Auth::SessionsController < Devise::SessionsController skip_before_action :require_no_authentication, only: [:create] skip_before_action :check_suspension, only: [:destroy] prepend_before_action :authenticate_with_two_factor, if: :two_factor_enabled?, only: [:create] + before_action :set_instance_presenter, only: [:new] def create super do |resource| @@ -84,6 +85,10 @@ class Auth::SessionsController < Devise::SessionsController private + def set_instance_presenter + @instance_presenter = InstancePresenter.new + end + def home_paths(resource) paths = [about_path] if single_user_mode? && resource.is_a?(User) diff --git a/app/views/about/_og.html.haml b/app/views/about/_og.html.haml deleted file mode 100644 index dbd476915..000000000 --- a/app/views/about/_og.html.haml +++ /dev/null @@ -1,10 +0,0 @@ -- thumbnail = @instance_presenter.thumbnail -= opengraph 'og:site_name', t('about.hosted_on', domain: site_hostname) -= opengraph 'og:url', about_url -= opengraph 'og:type', 'website' -= opengraph 'og:title', @instance_presenter.site_title -= opengraph 'og:description', strip_tags(@instance_presenter.site_description.presence || t('about.about_mastodon_html')) -= opengraph 'og:image', full_asset_url(thumbnail&.file&.url || asset_pack_path('preview.jpg', protocol: :request)) -= opengraph 'og:image:width', thumbnail ? thumbnail.meta['width'] : '1200' -= opengraph 'og:image:height', thumbnail ? thumbnail.meta['height'] : '630' -= opengraph 'twitter:card', 'summary_large_image' diff --git a/app/views/about/more.html.haml b/app/views/about/more.html.haml index 1a4e74643..b012606ce 100644 --- a/app/views/about/more.html.haml +++ b/app/views/about/more.html.haml @@ -3,7 +3,7 @@ - content_for :header_tags do = javascript_pack_tag 'public', integrity: true, crossorigin: 'anonymous' - = render partial: 'og' + = render partial: 'shared/og' .landing-page .header-wrapper.compact diff --git a/app/views/about/show.html.haml b/app/views/about/show.html.haml index ef27d07a1..f8f90ce24 100644 --- a/app/views/about/show.html.haml +++ b/app/views/about/show.html.haml @@ -4,7 +4,7 @@ - content_for :header_tags do %script#initial-state{ type: 'application/json' }!= json_escape(@initial_state_json) = javascript_pack_tag 'about', integrity: true, crossorigin: 'anonymous' - = render partial: 'og' + = render partial: 'shared/og' .landing-page .header-wrapper diff --git a/app/views/auth/registrations/new.html.haml b/app/views/auth/registrations/new.html.haml index 807020310..f71675df0 100644 --- a/app/views/auth/registrations/new.html.haml +++ b/app/views/auth/registrations/new.html.haml @@ -1,6 +1,9 @@ - content_for :page_title do = t('auth.register') +- content_for :header_tags do + = render partial: 'shared/og' + = simple_form_for(resource, as: resource_name, url: registration_path(resource_name)) do |f| = render 'shared/error_messages', object: resource diff --git a/app/views/auth/sessions/new.html.haml b/app/views/auth/sessions/new.html.haml index e589377bf..a52b0053b 100644 --- a/app/views/auth/sessions/new.html.haml +++ b/app/views/auth/sessions/new.html.haml @@ -1,6 +1,9 @@ - content_for :page_title do = t('auth.login') +- content_for :header_tags do + = render partial: 'shared/og' + = simple_form_for(resource, as: resource_name, url: session_path(resource_name)) do |f| = f.input :email, autofocus: true, placeholder: t('simple_form.labels.defaults.email'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.email') } = f.input :password, placeholder: t('simple_form.labels.defaults.password'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.password'), :autocomplete => 'off' } diff --git a/app/views/shared/_og.html.haml b/app/views/shared/_og.html.haml new file mode 100644 index 000000000..dbd476915 --- /dev/null +++ b/app/views/shared/_og.html.haml @@ -0,0 +1,10 @@ +- thumbnail = @instance_presenter.thumbnail += opengraph 'og:site_name', t('about.hosted_on', domain: site_hostname) += opengraph 'og:url', about_url += opengraph 'og:type', 'website' += opengraph 'og:title', @instance_presenter.site_title += opengraph 'og:description', strip_tags(@instance_presenter.site_description.presence || t('about.about_mastodon_html')) += opengraph 'og:image', full_asset_url(thumbnail&.file&.url || asset_pack_path('preview.jpg', protocol: :request)) += opengraph 'og:image:width', thumbnail ? thumbnail.meta['width'] : '1200' += opengraph 'og:image:height', thumbnail ? thumbnail.meta['height'] : '630' += opengraph 'twitter:card', 'summary_large_image' -- cgit