Coverage for graphite.whitelist

[cbowman0]

I had this mostly together, so in light of issue #1569 and pull request #1570, I submit this for test cases before the wording changes.

• Fix graphite.whitelist.load_whitelist()
• Add test cases for show, add and remove

Of note, the load_whitelist() function was not working before this change. If the preferred method is to read the blob in and use unpickle.loads(), then that can be done.

[codecov-io]

Current coverage is 56.73%

Merging #1572 into master will increase coverage by 0.47%

@@             master      #1572   diff @@
==========================================
  Files            52         52          
  Lines          5781       5780     -1   
  Methods           0          0          
  Messages          0          0          
  Branches       1111       1111          
==========================================
+ Hits           3252       3279    +27   
+ Misses         2328       2300    -28   
  Partials        201        201          

Powered by Codecov. Last updated by d0295a2...cddfe40

[gwaldo]

Thank you for this, @cbowman0

[cbowman0]

I adjusted the testing to be deterministic. I would like someone else from @graphite-project/committers to confirm the changes to load_whitelist() is ok.

Once that is merged, I will help @gwaldo with adjusting the test cases to work with his wording changes, if required.

[obfuscurity]

@cbowman0 why the change from unpickle to pickle? The latter is intended as a safe alternative to the insecure pickle.

graphite-web/webapp/graphite/util.py

Lines 159 to 212 in af7c0c1

	# This whole song & dance is due to pickle being insecure
	# The SafeUnpickler classes were largely derived from
	# http://nadiana.com/python-pickle-insecure
	# This code also lives in carbon.util
	if USING_CPICKLE:
	class SafeUnpickler(object):
	PICKLE_SAFE = {
	'copy_reg': set(['_reconstructor']),
	'__builtin__': set(['object', 'list']),
	'collections': set(['deque']),
	'graphite.render.datalib': set(['TimeSeries']),
	'graphite.intervals': set(['Interval', 'IntervalSet']),
	}
	@classmethod
	def find_class(cls, module, name):
	if not module in cls.PICKLE_SAFE:
	raise pickle.UnpicklingError('Attempting to unpickle unsafe module %s' % module)
	__import__(module)
	mod = sys.modules[module]
	if not name in cls.PICKLE_SAFE[module]:
	raise pickle.UnpicklingError('Attempting to unpickle unsafe class %s' % name)
	return getattr(mod, name)
	@classmethod
	def loads(cls, pickle_string):
	pickle_obj = pickle.Unpickler(StringIO(pickle_string))
	pickle_obj.find_global = cls.find_class
	return pickle_obj.load()
	else:
	class SafeUnpickler(pickle.Unpickler):
	PICKLE_SAFE = {
	'copy_reg': set(['_reconstructor']),
	'__builtin__': set(['object', 'list']),
	'collections': set(['deque']),
	'graphite.render.datalib': set(['TimeSeries']),
	'graphite.intervals': set(['Interval', 'IntervalSet']),
	}
	def find_class(self, module, name):
	if not module in self.PICKLE_SAFE:
	raise pickle.UnpicklingError('Attempting to unpickle unsafe module %s' % module)
	__import__(module)
	mod = sys.modules[module]
	if not name in self.PICKLE_SAFE[module]:
	raise pickle.UnpicklingError('Attempting to unpickle unsafe class %s' % name)
	return getattr(mod, name)
	@classmethod
	def loads(cls, pickle_string):
	return cls(StringIO(pickle_string)).load()
	unpickle = SafeUnpickler

[cbowman0]

@obfuscurity Because it currently doesn't work. Calls to load_whitelist() error with:
AttributeError: type object 'SafeUnpickler' has no attribute 'load'

[obfuscurity]

Ah right, we're loading from a file, not a string. Looks like a typo from c198e58. Yeah, this change looks good.

👍

[cbowman0]

Alternatively, we can do:

   buffer = open(settings.WHITELIST_FILE, 'rb').read()
   whitelist = unpickle.loads(buffer)

and add set to the PICKLE_SAFE set. I can do that quickly, if that's preferred.

[obfuscurity]

@cbowman0 Yeah, good idea.

[cbowman0]

Done. Once automated checks go green again, I'll merge it.