File manager exception fix #2229
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 (
|
KesterTan
requested review from
a team and
jlge
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (1)
spec/controllers/file_manager_controller_spec.rb (1)
164-164: Redundantsign_incalls within test examplesIn several test examples (lines 164, 221, 278, 284, 351, and 358),
sign_inis 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_incalls:- 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_accessshared 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
pathparametersAt lines 352 and 359, the
pathparameter inget :download_tarincludes"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.
🛠️ 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.
🛠️ 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.
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.
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.
🛠️ 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.
🛠️ 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.
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.
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.
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.
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.
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.rband make sure that everything passesTypes of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting