-
Notifications
You must be signed in to change notification settings - Fork 219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
File manager exception fix #2229
base: master
Are you sure you want to change the base?
Conversation
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in the pull request involve modifications to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (1)
spec/controllers/file_manager_controller_spec.rb (1)
164-164
: Redundantsign_in
calls within test examplesIn several test examples (lines 164, 221, 278, 284, 351, and 358),
sign_in
is called inside the test block even though the user is already signed in within thebefore(:each)
block. This is unnecessary and can be removed to clean up the tests.Consider removing the redundant
sign_in
calls:- sign_in(instructor)
Also applies to: 221-221, 278-278, 284-284, 351-351, 358-358
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/controllers/file_manager_controller.rb (5 hunks)
- spec/controllers/file_manager_controller_spec.rb (2 hunks)
🔇 Additional comments (2)
spec/controllers/file_manager_controller_spec.rb (2)
26-33
: Well-defined shared example for unauthorized accessThe
unauthorized_access
shared example is correctly implemented. It enhances test coverage by ensuring unauthorized access scenarios are consistently tested across different contexts.
352-352
:⚠️ Potential issueVerify correctness of constructed
path
parametersAt lines 352 and 359, the
path
parameter inget :download_tar
includes"autopopulated/test/#{dir_path}"
and"autopopulated/test/#{file_path}"
. Ensure that these paths are correctly constructed and correspond to the actual file system paths used in the application.Run the following script to check the constructed paths:
Also applies to: 359-359
if check_instructor(absolute_path) | ||
parent = absolute_path.parent | ||
raise "Unable to delete courses in the root directory." if parent == BASE_DIRECTORY | ||
|
||
FileUtils.rm_rf(absolute_path) | ||
FileUtils.rm_rf(absolute_path) | ||
else | ||
flash[:error] = "You are not authorized to delete this" | ||
redirect_to file_manager_index_path | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor Authorization Checks to Avoid Duplication
The authorization check if check_instructor(absolute_path)
is repeated across multiple actions. To enhance maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider using a before_action
filter.
Implement a before_action
callback:
before_action :authorize_instructor!, only: [:delete]
private
def authorize_instructor!
unless check_instructor(absolute_path)
flash[:error] = "You are not authorized to perform this action"
redirect_to file_manager_index_path and return
end
end
if check_instructor(absolute_path) | ||
if File.directory?(absolute_path) | ||
tar_stream = StringIO.new("") | ||
Gem::Package::TarWriter.new(tar_stream) do |tar| | ||
Dir[File.join(absolute_path.to_s, '**', '**')].each do |file| | ||
mode = File.stat(file).mode | ||
relative_path = file.sub(%r{^#{Regexp.escape(absolute_path.to_s)}/?}, '') | ||
if File.directory?(file) | ||
tar.mkdir relative_path, mode | ||
else | ||
tar.add_file relative_path, mode do |tar_file| | ||
File.open(file, "rb") { |f| tar_file.write f.read } | ||
end | ||
end | ||
end | ||
end | ||
tar_stream.rewind | ||
tar_stream.close | ||
send_data tar_stream.string.force_encoding("binary"), | ||
filename: "file_manager.tar", | ||
type: "application/x-tar", | ||
disposition: "attachment" | ||
else | ||
send_file(absolute_path, | ||
filename: File.basename(absolute_path), | ||
disposition: 'attachment') | ||
end | ||
tar_stream.rewind | ||
tar_stream.close | ||
send_data tar_stream.string.force_encoding("binary"), | ||
filename: "file_manager.tar", | ||
type: "application/x-tar", | ||
disposition: "attachment" | ||
else | ||
send_file(absolute_path, | ||
filename: File.basename(absolute_path), | ||
disposition: 'attachment') | ||
flash[:error] = "You are not authorized to download atttachments at this path" | ||
redirect_to file_manager_index_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Extract File Download Logic to Improve Readability
The code for downloading tar files contains nested logic that can be difficult to read and maintain. Consider extracting the file download and tar creation logic into separate private methods.
Refactor the code as follows:
def download_tar
path = params[:path]&.split("/")&.drop(2)&.join("/")
path = CGI.unescape(path)
absolute_path = check_path_exist(path)
if check_instructor(absolute_path)
if File.directory?(absolute_path)
send_tarball(absolute_path)
else
send_file_download(absolute_path)
end
else
flash[:error] = "You are not authorized to download attachments at this path"
redirect_to file_manager_index_path, status: :forbidden
end
end
private
def send_tarball(absolute_path)
tar_stream = StringIO.new("")
Gem::Package::TarWriter.new(tar_stream) do |tar|
Dir[File.join(absolute_path.to_s, '**', '**')].each do |file|
mode = File.stat(file).mode
relative_path = file.sub(%r{^#{Regexp.escape(absolute_path.to_s)}/?}, '')
if File.directory?(file)
tar.mkdir relative_path, mode
else
tar.add_file relative_path, mode do |tar_file|
File.open(file, "rb") { |f| tar_file.write f.read }
end
end
end
end
tar_stream.rewind
tar_stream.close
send_data tar_stream.string.force_encoding("binary"),
filename: "file_manager.tar",
type: "application/x-tar",
disposition: "attachment"
end
def send_file_download(absolute_path)
send_file(absolute_path,
filename: File.basename(absolute_path),
disposition: 'attachment')
end
flash[:error] = "You are not authorized to download atttachments at this path" | ||
redirect_to file_manager_index_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct Typographical Error in Error Message
There's a typo in the error message: "You are not authorized to download atttachments at this path."
Correct the misspelled word:
flash[:error] = "You are not authorized to download atttachments at this path"
+flash[:error] = "You are not authorized to download attachments at this path"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
flash[:error] = "You are not authorized to download atttachments at this path" | |
redirect_to file_manager_index_path | |
flash[:error] = "You are not authorized to download attachments at this path" | |
redirect_to file_manager_index_path |
else | ||
flash[:error] = "You are not authorized to upload files at this path" | ||
redirect_to file_manager_index_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure Method Execution Halts After Redirect
After calling redirect_to
, the method continues execution unless explicitly halted. This might lead to unexpected behavior or errors.
Add and return
to halt execution:
else
flash[:error] = "You are not authorized to upload files at this path"
- redirect_to file_manager_index_path
+ redirect_to file_manager_index_path and return
end
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
else | |
flash[:error] = "You are not authorized to upload files at this path" | |
redirect_to file_manager_index_path | |
else | |
flash[:error] = "You are not authorized to upload files at this path" | |
redirect_to file_manager_index_path and return |
else | ||
flash[:error] = "You are not authorized to view this path" | ||
redirect_to file_manager_index_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider Using Proper HTTP Status Code for Unauthorized Access
When a user is not authorized to view a path, it's advisable to return a 403 Forbidden
HTTP status code to clearly communicate the nature of the error.
Update the redirect to include the status:
else
flash[:error] = "You are not authorized to view this path"
- redirect_to file_manager_index_path
+ redirect_to file_manager_index_path, status: :forbidden
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
else | |
flash[:error] = "You are not authorized to view this path" | |
redirect_to file_manager_index_path | |
else | |
flash[:error] = "You are not authorized to view this path" | |
redirect_to file_manager_index_path, status: :forbidden |
else | ||
flash[:error] = "You are not authorized to rename this path" | ||
redirect_to file_manager_index_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consistent Use of HTTP Status Codes for Unauthorized Actions
For consistency and better client-side error handling, consider returning a 403 Forbidden
status code when a user is not authorized to rename a path.
Modify the redirect to include the status:
else
flash[:error] = "You are not authorized to rename this path"
- redirect_to file_manager_index_path
+ redirect_to file_manager_index_path, status: :forbidden
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
else | |
flash[:error] = "You are not authorized to rename this path" | |
redirect_to file_manager_index_path | |
else | |
flash[:error] = "You are not authorized to rename this path" | |
redirect_to file_manager_index_path, status: :forbidden |
get :download_tar, params: { path: '' } | ||
expect(response).to redirect_to(file_manager_index_path) | ||
expect(flash[:error]).to eq("You are not authorized to download atttachments at this path") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error in flash error message
The error message in lines 369, 395, and 401 contains a typo: "atttachments" should be "attachments".
Correct the spelling in the error messages:
- expect(flash[:error]).to eq("You are not authorized to download atttachments at this path")
+ expect(flash[:error]).to eq("You are not authorized to download attachments at this path")
Also applies to: 395-395, 401-401
end | ||
|
||
it "renames the file successfully" do | ||
sign_in(:instructor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect argument in sign_in
method
At line 221, sign_in(:instructor)
uses a symbol :instructor
instead of the variable instructor
. This could lead to authentication issues in the test.
Update the sign_in
method to use the instructor
variable:
- sign_in(:instructor)
+ sign_in(instructor)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
sign_in(:instructor) | |
sign_in(instructor) |
sign_in(:instructor) | ||
put :rename, params: { relative_path: file_path, new_name:, path: file_path } | ||
expect(flash[:success]).to eq("Successfully renamed file to #{new_name}") | ||
expect(File.exist?(File.join(@base_dir, course.name, new_name))).to be_truthy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undefined variable course
used in file path
In line 224, course.name
is used, but course
is not defined in this context. The course variable is referenced as @course
elsewhere in the code.
Modify the code to use @course.name
:
- expect(File.exist?(File.join(@base_dir, course.name, new_name))).to be_truthy
+ expect(File.exist?(File.join(@base_dir, @course.name, new_name))).to be_truthy
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
expect(File.exist?(File.join(@base_dir, course.name, new_name))).to be_truthy | |
expect(File.exist?(File.join(@base_dir, @course.name, new_name))).to be_truthy |
context "when user is not an instructor of the course" do | ||
let(:unauthorized_path) { "#{@course.name}/secret_folder" } | ||
let!(:instructor) { instructor_user } | ||
|
||
before(:each) do | ||
sign_in(instructor) | ||
# Set up the BASE_DIRECTORY and create the course directory | ||
@base_dir = Dir.mktmpdir | ||
stub_const('FileManagerController::BASE_DIRECTORY', Pathname.new(@base_dir)) | ||
FileUtils.mkdir_p(File.join(@base_dir, unauthorized_path)) | ||
# Ensure that the controller uses the modified BASE_DIRECTORY | ||
allow(FileManagerController).to receive(:const_get) | ||
.with(:BASE_DIRECTORY) | ||
.and_return(Pathname.new(@base_dir)) | ||
end | ||
|
||
let!(:u) { student_user } | ||
it_behaves_like "unauthorized_access" | ||
|
||
after(:each) do | ||
delete_course_files(course_hash[:course]) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential confusion with user sign-in in the context
In the context "when user is not an instructor of the course", the before(:each)
block at line 121 signs in the instructor
, but the test is meant to simulate a student attempting unauthorized access. At line 132, u
is set to student_user
, and the shared example likely uses u
for the tests.
Adjust the before
block to sign in u
instead of instructor
to reflect the student user's perspective:
- before(:each) do
- sign_in(instructor)
+ before(:each) do
+ sign_in(u)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
context "when user is not an instructor of the course" do | |
let(:unauthorized_path) { "#{@course.name}/secret_folder" } | |
let!(:instructor) { instructor_user } | |
before(:each) do | |
sign_in(instructor) | |
# Set up the BASE_DIRECTORY and create the course directory | |
@base_dir = Dir.mktmpdir | |
stub_const('FileManagerController::BASE_DIRECTORY', Pathname.new(@base_dir)) | |
FileUtils.mkdir_p(File.join(@base_dir, unauthorized_path)) | |
# Ensure that the controller uses the modified BASE_DIRECTORY | |
allow(FileManagerController).to receive(:const_get) | |
.with(:BASE_DIRECTORY) | |
.and_return(Pathname.new(@base_dir)) | |
end | |
let!(:u) { student_user } | |
it_behaves_like "unauthorized_access" | |
after(:each) do | |
delete_course_files(course_hash[:course]) | |
end | |
end | |
end | |
context "when user is not an instructor of the course" do | |
let(:unauthorized_path) { "#{@course.name}/secret_folder" } | |
let!(:instructor) { instructor_user } | |
before(:each) do | |
sign_in(u) | |
# Set up the BASE_DIRECTORY and create the course directory | |
@base_dir = Dir.mktmpdir | |
stub_const('FileManagerController::BASE_DIRECTORY', Pathname.new(@base_dir)) | |
FileUtils.mkdir_p(File.join(@base_dir, unauthorized_path)) | |
# Ensure that the controller uses the modified BASE_DIRECTORY | |
allow(FileManagerController).to receive(:const_get) | |
.with(:BASE_DIRECTORY) | |
.and_return(Pathname.new(@base_dir)) | |
end | |
let!(:u) { student_user } | |
it_behaves_like "unauthorized_access" | |
after(:each) do | |
delete_course_files(course_hash[:course]) | |
end | |
end | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I login as a student user and access the file_manager
path, it shows the base screen, but no error flash is displayed.
I think a more ideal behavior would be to redirect back to the Autolab homepage and flash the "You are not authorized to view this page." message there. Also when an unauthorized user views the file manager, they can still press the "create folder" and "upload files" buttons which I know don't work but I think it's unideal, so this can be prevented by redirecting out to a different page.
Description
This PR fixes the routing when a user is not allowed to access a path. This also adds unit tests which tests this functionality for this fix.
Resolves #2228
How Has This Been Tested?
rake spec SPEC=./spec/controllers/file_manager_controller_spec.rb
and make sure that everything passesTypes of changes
Checklist:
overcommit --install && overcommit --sign
to use pre-commit hook for linting