Download and upload limit doesn't work on files app

[pescobar]

Hi,

We are using Open Ondemand 3.0.1 on centos7 installed with RPMs (ondemand-3.0.1-1.el7.x86_64) and we cannot limit the download size following the docs in

https://osc.github.io/ood-documentation/latest/customizations.html?highlight=download#set-download-limits
https://osc.github.io/ood-documentation/latest/customizations.html?highlight=download#set-upload-limits

This is how we have configured the download limit to 1GB:

$> cat /etc/ood/config/apps/files/env
OOD_DOWNLOAD_DIR_MAX=1000000000

But we can download files of any size. We have tried with different values for OOD_DOWNLOAD_DIR_MAX like 1, 0, 10, -1 but none works.

While checking the docs we realized of two details which are inconsistent which we don't know if could be related:

1. why the variable name for upload is FILE_UPLOAD_MAX but for download is OOD_DOWNLOAD_DIR_MAX? Why the DIR in the var name?

2. Why upload limit is defined in /etc/ood/config/apps/shell/env (shell app) and the download limit is defined in /etc/ood/config/apps/files/env (files app) ?

[johrstrom]

Hi thanks for the ticket.

1. why the variable name for upload is FILE_UPLOAD_MAX but for download is OOD_DOWNLOAD_DIR_MAX? Why the DIR in the var name?

Dir is there because we zip the dir before we download. Though it would seem now that this should apply to files as well.

2. Why upload limit is defined in /etc/ood/config/apps/shell/env (shell app) and the download limit is defined in /etc/ood/config/apps/files/env (files app) ?

The docs seem to be wrong - shouldn't be shell. Shouldn't even be files either. Should be /etc/ood/config/apps/dashboard/env.

[i-mtz]

Hi @johrstrom

Dir is there because we zip the dir before we download. Though it would seem now that this should apply to files as well.

This seems to be the issue. I tried downloading a folder and size limits are enforced in that case, but not for single files. The parameter set by the environment variable OOD_DOWNLOAD_DIR_MAX is only used if we try to download a directory but is not used for a single file:

ondemand/apps/dashboard/app/controllers/files_controller.rb

Lines 11 to 90 in 883f0c8

	if @path.directory?
	@path.raise_if_cant_access_directory_contents
	request.format = 'zip' if params[:download]
	respond_to do |format|
	format.html do
	render :index
	end
	format.json do
	response.headers['Cache-Control'] = 'no-store'
	if params[:can_download]
	# check to see if this directory can be downloaded as a zip
	can_download, error_message = @path.can_download_as_zip?
	render json: { can_download: can_download, error_message: error_message }
	else
	@files = @path.ls
	render :index
	end
	end
	# FIXME: below is a large block that should be moved to a model
	# if moved to a model the exceptions can be handled there and
	# then this code will be simpler to read
	# and we can avoid rescuing in a block so we can reintroduce
	# the block braces which is the Rails convention with the respond_to formats.
	format.zip do
	can_download, error_message = @path.can_download_as_zip?
	if can_download
	zipname = @path.basename.to_s.gsub('"', '\"') + '.zip'
	response.set_header 'Content-Disposition', "attachment; filename=\"#{zipname}\""
	response.set_header 'Content-Type', 'application/zip'
	response.set_header 'Last-Modified', Time.now.httpdate
	response.sending_file = true
	response.cache_control[:public] ||= false
	# FIXME: strategy 1: is below, use zip_tricks
	# strategy 2: use actual zip command (likely much faster) and ActionController::Live
	zip_tricks_stream do |zip|
	@path.files_to_zip.each do |file|
	begin
	next unless File.readable?(file.path)
	if File.file?(file.path)
	zip.write_deflated_file(file.relative_path.to_s) do |zip_file|
	IO.copy_stream(file.path, zip_file)
	end
	else
	zip.add_empty_directory(dirname: file.relative_path.to_s)
	end
	rescue => e
	logger.warn("error writing file #{file.path} to zip: #{e.message}")
	end
	end
	end
	else
	logger.warn "unable to download directory #{@path.to_s}: #{error_message}"
	response.set_header 'X-OOD-Failure-Reason', error_message
	head :internal_server_error
	end
	rescue => e
	# Third party API requests (from outside of OnDemand) will see this error
	# message if there's an error while downloading a directory.
	#
	# The client side code in the Files App performs checks before downloading
	# a directory with the ?can_download query parameter but other implementations
	# that don't perform this check will see HTTP 500 returned and the error
	# error message will be in the "X-OOD-Failure-Reason" header.
	#
	Rails.logger.warn "exception raised when attempting to download directory #{@path.to_s}: #{e.message}"
	response.set_header 'X-OOD-Failure-Reason', e.message
	head :internal_server_error
	end
	end
	else
	show_file
	end

ondemand/apps/dashboard/app/models/posix_file.rb

Line 124 in 883f0c8

def can_download_as_zip?(timeout: Configuration.file_download_dir_timeout, download_directory_size_limit: Configuration.file_download_dir_max)

which is defined also on the remote_file.rb too, but in both cases only called if trying to download a directory.

I'm not sure if adding file size checking code would be enough as in this case it would require also to disable the inline display of files of certain size, or at least display a warning on why a file is not displayed? All of this comes for instances where the file size download limit is set for restricting data extraction but I wonder if in those cases, disabling the files app entirely does not make more sense...

[pescobar]

@johrstrom I see you have updated the docs to use the right path /etc/ood/config/apps/dashboard/env but there is still an error in the docs using /etc/ood/config/apps/files/env

For example, to set the limit to 5 GB, you can add the following line to the /etc/ood/config/apps/files/env file:

OOD_DOWNLOAD_DIR_MAX=5368709120

[johrstrom]

Thanks! I'll get that fixed shortly.