Skip to content

Commit e745f74

Browse files
author
John Corrigan
committed
add Attachments::ScopedToUser
fixes CNVS-21854 Moves permission-based scope concatenation from FilesController#api_index to Attachments::ScopedToUser#scope. Updates Folder permission policy to delegate permissions for :read_as_admin & :manage_files to its context. Add Folder.from_context_or_id to encapsulate folder finding logic from FilesController#api_index. test plan: - Regression testing for API files index action. Change-Id: I1dc855cc16a98969482a72d251848c47e300b05c Reviewed-on: https://gerrit.instructure.com/58415 Tested-by: Jenkins Reviewed-by: Matt Berns <mberns@instructure.com> QA-Review: Deepeeca Soundarrajan <dsoundarrajan@instructure.com> Product-Review: John Corrigan <jcorrigan@instructure.com>
1 parent 41d1536 commit e745f74

4 files changed

Lines changed: 100 additions & 36 deletions

File tree

app/controllers/files_controller.rb

Lines changed: 21 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,14 @@ def index
184184
@current_attachments = @current_folder.visible_file_attachments.by_position_then_display_name
185185
end
186186
@current_attachments = @current_attachments.includes(:thumbnail, :media_object)
187-
render :json => @current_attachments.map{ |a| a.as_json(methods: [:readable_size, :currently_locked, :thumbnail_url], permissions: {user: @current_user, session: session}) }
187+
render :json => @current_attachments.map do |a|
188+
a.as_json({
189+
methods: [ :readable_size, :currently_locked, :thumbnail_url ],
190+
permissions: {
191+
user: @current_user, session: session
192+
}
193+
})
194+
end
188195
else
189196
file_structure = {
190197
:contexts => [@context.as_json(permissions: {user: @current_user})],
@@ -249,38 +256,17 @@ def index
249256
# @returns [File]
250257
def api_index
251258
get_context
252-
if @context
253-
folder = Folder.root_folders(@context).first
254-
raise ActiveRecord::RecordNotFound unless folder
255-
context_index = true
256-
else
257-
verify_api_id
258-
folder = Folder.find(params[:id])
259-
end
259+
verify_api_id unless @context.present?
260+
@folder = Folder.from_context_or_id(@context, params[:id])
260261

261-
if authorized_action(folder, @current_user, :read_contents)
262-
@context = folder.context unless context_index
263-
can_view_hidden_files = can_view_hidden_files?(@context, @current_user, session)
262+
if authorized_action(@folder, @current_user, :read_contents)
264263
params[:sort] ||= params[:sort_by] # :sort_by was undocumented; :sort is more consistent with other APIs such as wikis
265264
params[:include] = Array(params[:include])
266265
params[:include] << 'user' if params[:sort] == 'user'
267266

268-
if context_index
269-
if can_view_hidden_files
270-
scope = @context.attachments.not_deleted
271-
else
272-
scope = @context.attachments.visible.not_hidden.not_locked.where(
273-
:folder_id => Folder.all_visible_folder_ids(@context))
274-
end
275-
else
276-
if can_view_hidden_files
277-
scope = folder.active_file_attachments
278-
else
279-
scope = folder.visible_file_attachments.not_hidden.not_locked
280-
end
281-
end
282-
scope = scope.includes(:user) if params[:include].include? 'user' && params[:sort] != 'user'
283-
scope = scope.includes(:usage_rights) if params[:include].include? 'usage_rights'
267+
scope = Attachments::ScopedToUser.new(@context || @folder, @current_user).scope
268+
scope = scope.includes(:user) if params[:include].include?('user') && params[:sort] != 'user'
269+
scope = scope.includes(:usage_rights) if params[:include].include?('usage_rights')
284270
scope = Attachment.search_by_attribute(scope, :display_name, params[:search_term])
285271

286272
order_clause = case params[:sort]
@@ -307,12 +293,14 @@ def api_index
307293
scope = scope.by_content_types(Array(params[:content_types]))
308294
end
309295

310-
url = context_index ? context_files_url : api_v1_list_files_url(folder)
296+
url = @context ? context_files_url : api_v1_list_files_url(@folder)
311297
@files = Api.paginate(scope, self, url)
312-
render :json => attachments_json(@files, @current_user, {},
313-
:can_view_hidden_files => can_view_hidden_files, :include => params[:include],
314-
:only => params[:only],
315-
:omit_verifier_in_app => !value_to_boolean(params[:use_verifiers]))
298+
render json: attachments_json(@files, @current_user, {}, {
299+
can_view_hidden_files: can_view_hidden_files?(@context || @folder, @current_user, session),
300+
include: params[:include],
301+
only: params[:only],
302+
omit_verifier_in_app: !value_to_boolean(params[:use_verifiers])
303+
})
316304
end
317305
end
318306

@@ -1104,7 +1092,6 @@ def show_thumbnail
11041092
end
11051093

11061094
private
1107-
11081095
def render_attachment_json(attachment, deleted_attachments, folder = attachment.folder)
11091096
json = {
11101097
:attachment => attachment.as_json(
@@ -1121,5 +1108,4 @@ def render_attachment_json(attachment, deleted_attachments, folder = attachment.
11211108

11221109
render :json => json, :as_text => true
11231110
end
1124-
11251111
end
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
#
2+
# Copyright (C) 2011 - 2015 Instructure, Inc.
3+
#
4+
# This file is part of Canvas.
5+
#
6+
# Canvas is free software: you can redistribute it and/or modify it under
7+
# the terms of the GNU Affero General Public License as published by the Free
8+
# Software Foundation, version 3 of the License.
9+
#
10+
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
11+
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
12+
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
13+
# details.
14+
#
15+
# You should have received a copy of the GNU Affero General Public License along
16+
# with this program. If not, see <http://www.gnu.org/licenses/>.
17+
#
18+
19+
module Attachments
20+
class ScopedToUser < ScopeFilter
21+
include Api::V1::Attachment
22+
23+
def scope
24+
context.is_a_context? ? scope_from_context : scope_from_folder
25+
end
26+
27+
private
28+
def scope_from_context
29+
if can_view_hidden_files?(context, user)
30+
context.attachments.not_deleted
31+
else
32+
context.attachments.visible.not_hidden.not_locked.where({
33+
folder_id: Folder.all_visible_folder_ids(context)
34+
})
35+
end
36+
end
37+
38+
def scope_from_folder
39+
if can_view_hidden_files?(context, user)
40+
context.active_file_attachments
41+
else
42+
context.visible_file_attachments.not_hidden.not_locked
43+
end
44+
end
45+
end
46+
end

app/models/folder.rb

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,12 @@ def currently_locked
464464

465465
given {|user, session| self.protected? && !self.locked? && self.context.grants_right?(user, session, :read) && self.context.users.include?(user) }
466466
can :read and can :read_contents
467+
468+
given { |user, session| self.context.grants_right?(user, session, :manage_files) }
469+
can :manage_files
470+
471+
given { |user, session| self.context.grants_right?(user, session, :read_as_admin) }
472+
can :read_as_admin
467473
end
468474

469475
# find all unlocked/visible folders that can be reached by following unlocked/visible folders from the root
@@ -479,7 +485,9 @@ def self.all_visible_folder_ids(context)
479485
visible_ids
480486
end
481487

482-
private
488+
def self.from_context_or_id(context, id)
489+
root_folders(context).first || where(id: id).first || (raise ActiveRecord::RecordNotFound)
490+
end
483491

484492
def self.find_visible_folders(visible_ids, folder_tree, dir_contents)
485493
visible_ids.concat dir_contents
@@ -489,4 +497,5 @@ def self.find_visible_folders(visible_ids, folder_tree, dir_contents)
489497
end
490498
nil
491499
end
500+
private_class_method :find_visible_folders
492501
end

spec/models/folder_spec.rb

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,4 +307,27 @@
307307
end
308308
end
309309
end
310+
311+
describe ".from_context_or_id" do
312+
it "delegate to root_folders when context is provided" do
313+
folder = Folder.root_folders(@course).first
314+
expect(Folder.from_context_or_id(@course, nil)).to eq(folder)
315+
end
316+
317+
it "should find by id when context is not provided and id is" do
318+
Folder.root_folders(@course).first
319+
account_model
320+
folder_model(context: @account)
321+
expect(Folder.root_folders(@course).first).not_to eq(@folder),
322+
'precondition'
323+
expect(Folder.from_context_or_id(nil, @folder.id)).to eq(@folder)
324+
end
325+
326+
it "should raise ActiveRecord::RecordNotFound when no record is found" do
327+
expect(Folder.where(id: 1)).to be_empty, 'precondition'
328+
expect {
329+
Folder.from_context_or_id(nil, 1)
330+
}.to raise_error(ActiveRecord::RecordNotFound)
331+
end
332+
end
310333
end

0 commit comments

Comments
 (0)