From 3a2003ba863252f305fb32098bcd3f095b10e2ff Mon Sep 17 00:00:00 2001 From: Jack Jennings Date: Mon, 29 May 2017 09:22:22 -0700 Subject: [PATCH] Extract authorization policy for viewing statuses (#3150) --- Gemfile | 1 + Gemfile.lock | 3 + .../api/activitypub/activities_controller.rb | 4 +- .../api/activitypub/notes_controller.rb | 4 +- app/controllers/api/v1/statuses_controller.rb | 7 +- app/controllers/concerns/authorization.rb | 22 ++++++ app/controllers/media_controller.rb | 7 +- app/controllers/statuses_controller.rb | 7 +- app/controllers/stream_entries_controller.rb | 8 ++- app/models/status.rb | 12 +--- app/policies/status_policy.rb | 20 ++++++ app/services/favourite_service.rb | 4 +- app/services/reblog_service.rb | 4 +- spec/controllers/media_controller_spec.rb | 2 +- spec/models/status_spec.rb | 60 ---------------- spec/policies/status_policy_spec.rb | 70 +++++++++++++++++++ 16 files changed, 155 insertions(+), 80 deletions(-) create mode 100644 app/controllers/concerns/authorization.rb create mode 100644 app/policies/status_policy.rb create mode 100644 spec/policies/status_policy_spec.rb diff --git a/Gemfile b/Gemfile index 811dd1f99f8..1246658374c 100644 --- a/Gemfile +++ b/Gemfile @@ -38,6 +38,7 @@ gem 'nokogiri', '~> 1.7' gem 'oj', '~> 3.0' gem 'ostatus2', '~> 2.0' gem 'ox', '~> 2.5' +gem 'pundit', '~> 1.1' gem 'rabl', '~> 0.13' gem 'rack-attack', '~> 5.0' gem 'rack-cors', '~> 0.4', require: 'rack/cors' diff --git a/Gemfile.lock b/Gemfile.lock index e8c3ba48700..69429c979bd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -283,6 +283,8 @@ GEM pry (>= 0.10.4) public_suffix (2.0.5) puma (3.8.2) + pundit (1.1.0) + activesupport (>= 3.0.0) rabl (0.13.1) activesupport (>= 2.3.14) rack (2.0.3) @@ -519,6 +521,7 @@ DEPENDENCIES pkg-config (~> 1.2) pry-rails (~> 0.3) puma (~> 3.8) + pundit (~> 1.1) rabl (~> 0.13) rack-attack (~> 5.0) rack-cors (~> 0.4) diff --git a/app/controllers/api/activitypub/activities_controller.rb b/app/controllers/api/activitypub/activities_controller.rb index ba03738fc92..025ab960e37 100644 --- a/app/controllers/api/activitypub/activities_controller.rb +++ b/app/controllers/api/activitypub/activities_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Api::Activitypub::ActivitiesController < ApiController + include Authorization + # before_action :set_follow, only: [:show_follow] before_action :set_status, only: [:show_status] @@ -8,7 +10,7 @@ class Api::Activitypub::ActivitiesController < ApiController # Show a status in AS2 format, as either an Announce (reblog) or a Create (post) activity. def show_status - return forbidden unless @status.permitted? + authorize @status, :show? if @status.reblog? render :show_status_announce diff --git a/app/controllers/api/activitypub/notes_controller.rb b/app/controllers/api/activitypub/notes_controller.rb index 6489243dc06..ff9383413b4 100644 --- a/app/controllers/api/activitypub/notes_controller.rb +++ b/app/controllers/api/activitypub/notes_controller.rb @@ -1,12 +1,14 @@ # frozen_string_literal: true class Api::Activitypub::NotesController < ApiController + include Authorization + before_action :set_status respond_to :activitystreams2 def show - forbidden unless @status.permitted? + authorize @status, :show? end private diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 852ffc3ab1e..592540f4577 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Api::V1::StatusesController < ApiController + include Authorization + before_action :authorize_if_got_token, except: [:create, :destroy, :reblog, :unreblog, :favourite, :unfavourite, :mute, :unmute] before_action -> { doorkeeper_authorize! :write }, only: [:create, :destroy, :reblog, :unreblog, :favourite, :unfavourite, :mute, :unmute] before_action :require_user!, except: [:show, :context, :card, :reblogged_by, :favourited_by] @@ -130,7 +132,10 @@ class Api::V1::StatusesController < ApiController def set_status @status = Status.find(params[:id]) - raise ActiveRecord::RecordNotFound unless @status.permitted?(current_account) + authorize @status, :show? + rescue Mastodon::NotPermittedError + # Reraise in order to get a 404 instead of a 403 error code + raise ActiveRecord::RecordNotFound end def set_conversation diff --git a/app/controllers/concerns/authorization.rb b/app/controllers/concerns/authorization.rb new file mode 100644 index 00000000000..7828fe48de0 --- /dev/null +++ b/app/controllers/concerns/authorization.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Authorization + extend ActiveSupport::Concern + include Pundit + + def pundit_user + current_account + end + + def authorize(*) + super + rescue Pundit::NotAuthorizedError + raise Mastodon::NotPermittedError + end + + def authorize_with(user, record, query) + Pundit.authorize(user, record, query) + rescue Pundit::NotAuthorizedError + raise Mastodon::NotPermittedError + end +end diff --git a/app/controllers/media_controller.rb b/app/controllers/media_controller.rb index fa1daf012a6..f652f5acef9 100644 --- a/app/controllers/media_controller.rb +++ b/app/controllers/media_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class MediaController < ApplicationController + include Authorization + before_action :verify_permitted_status def show @@ -14,6 +16,9 @@ class MediaController < ApplicationController end def verify_permitted_status - raise ActiveRecord::RecordNotFound unless media_attachment.status.permitted?(current_account) + authorize media_attachment.status, :show? + rescue Mastodon::NotPermittedError + # Reraise in order to get a 404 instead of a 403 error code + raise ActiveRecord::RecordNotFound end end diff --git a/app/controllers/statuses_controller.rb b/app/controllers/statuses_controller.rb index 696bb4f521c..59c9d0a87fa 100644 --- a/app/controllers/statuses_controller.rb +++ b/app/controllers/statuses_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class StatusesController < ApplicationController + include Authorization + layout 'public' before_action :set_account @@ -30,7 +32,10 @@ class StatusesController < ApplicationController @stream_entry = @status.stream_entry @type = @stream_entry.activity_type.downcase - raise ActiveRecord::RecordNotFound unless @status.permitted?(current_account) + authorize @status, :show? + rescue Mastodon::NotPermittedError + # Reraise in order to get a 404 + raise ActiveRecord::RecordNotFound end def check_account_suspension diff --git a/app/controllers/stream_entries_controller.rb b/app/controllers/stream_entries_controller.rb index baff4317a3c..314d5961909 100644 --- a/app/controllers/stream_entries_controller.rb +++ b/app/controllers/stream_entries_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class StreamEntriesController < ApplicationController + include Authorization + layout 'public' before_action :set_account @@ -42,7 +44,11 @@ class StreamEntriesController < ApplicationController @stream_entry = @account.stream_entries.where(activity_type: 'Status').find(params[:id]) @type = @stream_entry.activity_type.downcase - raise ActiveRecord::RecordNotFound if @stream_entry.activity.nil? || (@stream_entry.hidden? && !@stream_entry.activity.permitted?(current_account)) + raise ActiveRecord::RecordNotFound if @stream_entry.activity.nil? + authorize @stream_entry.activity, :show? if @stream_entry.hidden? + rescue Mastodon::NotPermittedError + # Reraise in order to get a 404 + raise ActiveRecord::RecordNotFound end def check_account_suspension diff --git a/app/models/status.rb b/app/models/status.rb index a3dbce9f13c..a371083d056 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -113,16 +113,6 @@ class Status < ApplicationRecord private_visibility? || direct_visibility? end - def permitted?(other_account = nil) - if direct_visibility? - account.id == other_account&.id || mentions.where(account: other_account).exists? - elsif private_visibility? - account.id == other_account&.id || other_account&.following?(account) || mentions.where(account: other_account).exists? - else - other_account.nil? || !account.blocking?(other_account) - end - end - def ancestors(account = nil) ids = Rails.cache.fetch("ancestors:#{id}") { (Status.find_by_sql(['WITH RECURSIVE search_tree(id, in_reply_to_id, path) AS (SELECT id, in_reply_to_id, ARRAY[id] FROM statuses WHERE id = ? UNION ALL SELECT statuses.id, statuses.in_reply_to_id, path || statuses.id FROM search_tree JOIN statuses ON statuses.id = search_tree.in_reply_to_id WHERE NOT statuses.id = ANY(path)) SELECT id FROM search_tree ORDER BY path DESC', id]) - [self]).pluck(:id) } find_statuses_from_tree_path(ids, account) @@ -301,7 +291,7 @@ class Status < ApplicationRecord should_filter ||= account&.domain_blocking?(status.account.domain) should_filter ||= account&.muting?(status.account_id) should_filter ||= (status.account.silenced? && !account&.following?(status.account_id)) - should_filter ||= !status.permitted?(account) + should_filter ||= !StatusPolicy.new(account, status).show? should_filter end end diff --git a/app/policies/status_policy.rb b/app/policies/status_policy.rb new file mode 100644 index 00000000000..658ba6d1233 --- /dev/null +++ b/app/policies/status_policy.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class StatusPolicy + attr_reader :account, :status + + def initialize(account, status) + @account = account + @status = status + end + + def show? + if status.direct_visibility? + status.account.id == account&.id || status.mentions.where(account: account).exists? + elsif status.private_visibility? + status.account.id == account&.id || account&.following?(status.account) || status.mentions.where(account: account).exists? + else + account.nil? || !status.account.blocking?(account) + end + end +end diff --git a/app/services/favourite_service.rb b/app/services/favourite_service.rb index e92aada64f1..f27145c9613 100644 --- a/app/services/favourite_service.rb +++ b/app/services/favourite_service.rb @@ -1,12 +1,14 @@ # frozen_string_literal: true class FavouriteService < BaseService + include Authorization + # Favourite a status and notify remote user # @param [Account] account # @param [Status] status # @return [Favourite] def call(account, status) - raise Mastodon::NotPermittedError unless status.permitted?(account) + authorize_with account, status, :show? favourite = Favourite.create!(account: account, status: status) diff --git a/app/services/reblog_service.rb b/app/services/reblog_service.rb index 11446ce28b6..9c44b1980ec 100644 --- a/app/services/reblog_service.rb +++ b/app/services/reblog_service.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class ReblogService < BaseService + include Authorization include StreamEntryRenderer # Reblog a status and notify its remote author @@ -10,7 +11,8 @@ class ReblogService < BaseService def call(account, reblogged_status) reblogged_status = reblogged_status.reblog if reblogged_status.reblog? - raise Mastodon::NotPermittedError if reblogged_status.direct_visibility? || reblogged_status.private_visibility? || !reblogged_status.permitted?(account) + authorize_with account, reblogged_status, :show? + raise Mastodon::NotPermittedError if reblogged_status.direct_visibility? || reblogged_status.private_visibility? reblog = account.statuses.create!(reblog: reblogged_status, text: '') diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb index 2541df7344a..5b03899e4cc 100644 --- a/spec/controllers/media_controller_spec.rb +++ b/spec/controllers/media_controller_spec.rb @@ -30,7 +30,7 @@ describe MediaController do it 'raises when not permitted to view' do status = Fabricate(:status) media_attachment = Fabricate(:media_attachment, status: status) - allow_any_instance_of(Status).to receive(:permitted?).and_return(false) + allow_any_instance_of(MediaController).to receive(:authorize).and_raise(ActiveRecord::RecordNotFound) get :show, params: { id: media_attachment.to_param } expect(response).to have_http_status(:missing) diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb index d3a66134b20..ec07e615680 100644 --- a/spec/models/status_spec.rb +++ b/spec/models/status_spec.rb @@ -119,66 +119,6 @@ RSpec.describe Status, type: :model do end end - describe '#permitted?' do - it 'returns true when direct and account is viewer' do - subject.visibility = :direct - expect(subject.permitted?(subject.account)).to be true - end - - it 'returns true when direct and viewer is mentioned' do - subject.visibility = :direct - subject.mentions = [Fabricate(:mention, account: alice)] - - expect(subject.permitted?(alice)).to be true - end - - it 'returns false when direct and viewer is not mentioned' do - viewer = Fabricate(:account) - subject.visibility = :direct - - expect(subject.permitted?(viewer)).to be false - end - - it 'returns true when private and account is viewer' do - subject.visibility = :direct - expect(subject.permitted?(subject.account)).to be true - end - - it 'returns true when private and account is following viewer' do - follow = Fabricate(:follow) - subject.visibility = :private - subject.account = follow.target_account - - expect(subject.permitted?(follow.account)).to be true - end - - it 'returns true when private and viewer is mentioned' do - subject.visibility = :private - subject.mentions = [Fabricate(:mention, account: alice)] - - expect(subject.permitted?(alice)).to be true - end - - it 'returns false when private and viewer is not mentioned or followed' do - viewer = Fabricate(:account) - subject.visibility = :private - - expect(subject.permitted?(viewer)).to be false - end - - it 'returns true when no viewer' do - expect(subject.permitted?).to be true - end - - it 'returns false when viewer is blocked' do - block = Fabricate(:block) - subject.visibility = :private - subject.account = block.target_account - - expect(subject.permitted?(block.account)).to be false - end - end - describe '#ancestors' do let!(:alice) { Fabricate(:account, username: 'alice') } let!(:bob) { Fabricate(:account, username: 'bob', domain: 'example.com') } diff --git a/spec/policies/status_policy_spec.rb b/spec/policies/status_policy_spec.rb new file mode 100644 index 00000000000..ee7060b989a --- /dev/null +++ b/spec/policies/status_policy_spec.rb @@ -0,0 +1,70 @@ +require 'rails_helper' +require 'pundit/rspec' + +RSpec.describe StatusPolicy, type: :model do + subject { described_class } + + let(:alice) { Fabricate(:account, username: 'alice') } + let(:status) { Fabricate(:status, account: alice) } + + permissions :show? do + it 'grants access when direct and account is viewer' do + status.visibility = :direct + expect(subject).to permit(status.account, status) + end + + it 'grants access when direct and viewer is mentioned' do + status.visibility = :direct + status.mentions = [Fabricate(:mention, account: alice)] + + expect(subject).to permit(alice, status) + end + + it 'denies access when direct and viewer is not mentioned' do + viewer = Fabricate(:account) + status.visibility = :direct + + expect(subject).to_not permit(viewer, status) + end + + it 'grants access when private and account is viewer' do + status.visibility = :direct + + expect(subject).to permit(status.account, status) + end + + it 'grants access when private and account is following viewer' do + follow = Fabricate(:follow) + status.visibility = :private + status.account = follow.target_account + + expect(subject).to permit(follow.account, status) + end + + it 'grants access when private and viewer is mentioned' do + status.visibility = :private + status.mentions = [Fabricate(:mention, account: alice)] + + expect(subject).to permit(alice, status) + end + + it 'denies access when private and viewer is not mentioned or followed' do + viewer = Fabricate(:account) + status.visibility = :private + + expect(subject).to_not permit(viewer, status) + end + + it 'grants access when no viewer' do + expect(subject).to permit(nil, status) + end + + it 'denies access when viewer is blocked' do + block = Fabricate(:block) + status.visibility = :private + status.account = block.target_account + + expect(subject).to_not permit(block.account, status) + end + end +end