Fix: Security issue - Access control of a list submission

[syphax-bouazzouni]

Context and Issue

For some time, the demo portal has returned an error when going to the ontologies page (https://demo.ontoportal.org/ontologies) when not logged in, or logged in but not admin.

In the background, that page is called /submissions API endpoint. Seeing the logs of the API, we see an Access denied for this resource. Which is returning if a user doesn't have access to something.

This test is done by this function

ontologies_api/helpers/access_control_helper.rb

Lines 7 to 25 in c4baa22

	##
	# For a given object, check the access control settings. If they are restricted, handle appropriately.
	# For a list, this will filter out results. For single objects, if will throw an error if access is denied.
	def check_access(obj)
	return obj unless LinkedData.settings.enable_security
	if obj.is_a?(Enumerable)
	if obj.first.is_a?(LinkedData::Models::Base) && obj.first.access_based_on?
	check_access(obj.first)
	else
	filter_access(obj)
	end
	else
	if obj.respond_to?(:read_restricted?) && obj.read_restricted?
	readable = obj.readable?(env["REMOTE_USER"])
	error 403, "Access denied for this resource" unless readable
	end
	end
	obj
	end

It seems that it does the test by checking only the first element of the returned list (submissions in this case). So if the first element is accessible publicly we say that list is publicly accessible, and otherwise no.

The implementation has two issues

1. In the demo portal, randomly the first submission in the list was private, so it considered all the list of submissions as inaccessible (where this is not the case)
2. Anyone can access the private ontologies, e.g. I submitted a private ontology to Bioportal called TEST_PIZZA, but you can totally access its information with this link https://data.bioontology.org/submissions?display=description&include_status=ANY&apikey=8b5b7825-538d-40e0-9e9e-5ab9274a9aeb and search TEST_PIZZA you will see my description TEST PIZZA

Fix

This was already fixed in Agroportal (and originally at Ecoportal), by testing all the elements of the list to tell if a list is accessible or not.

For information, the first implementation of testing only the first element was done certainly for performance matters and does work for the other endpoints.

Changes

• Add a test to reproduce the issue (6d98714)
• Update the the check_access helper use filter_access if the object is a list (22eba94)

[codecov-commenter]

Codecov Report

Merging #126 (275b7ae) into master (194cac3) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   71.86%   71.99%   +0.13%     
==========================================
  Files          52       52              
  Lines        2847     2846       -1     
==========================================
+ Hits         2046     2049       +3     
+ Misses        801      797       -4     

Flag	Coverage Δ
unittests	71.99% <100.00%> (+0.13%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
helpers/access_control_helper.rb	83.33% <ø> (-1.29%)	⬇️
controllers/ontology_submissions_controller.rb	73.07% <100.00%> (+3.17%)	⬆️

... and 1 file with indirect coverage changes

[jvendetti]

For information, the first implementation of testing only the first element was done certainly for performance matters and does work for the other endpoints.

This pull request was already accepted, but I'll leave a comment here that it's still not clear to me if there will be any performance implications in production BioPortal.