openstack/deb-python-oauth2client
Code Issues Proposed changes

Handle missing storage files (#576)

Browse Source 
* Move `validate_file` to `oauth2client.util`
* Warn user if storage file is missing
* Raise an `IOError` exception if the given filename is a directory.
* Raise an `IOError` exception if the given filename is a symbolic link.
      (Previously raised `CredentialsFileSymbolicLinkError`)
* (test) Expanding single-letter variables
* (test) `assertEqual(None, <obj>)` -> `assertIsNone(<obj>)`
* (test) `assertNotEqual(None, <obj>)` -> `assertIsNotNone(<obj>)`
This commit is contained in:
Pat Ferate
2016-08-01 09:49:02 -07:00
committed by Jon Wayne Parrott
parent ae73312942
commit eb019c2dad
7 changed files with 96 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
oauth2client/contrib/_fcntl_opener.py
View File

@@ -16,6 +16,7 @@ import errno
import fcntl
import time
from oauth2client import util
from oauth2client.contrib import locked_file
@@ -32,15 +33,14 @@ class _FcntlOpener(locked_file._Opener):
Raises:
AlreadyLockedException: if the lock is already acquired.
IOError: if the open fails.
CredentialsFileSymbolicLinkError: if the file is a symbolic
link.
IOError: if the file is a symbolic link.
"""
if self._locked:
raise locked_file.AlreadyLockedException(
'File {0} is already locked'.format(self._filename))
start_time = time.time()
locked_file.validate_file(self._filename)
util.validate_file(self._filename)
try:
self._fh = open(self._filename, self._mode)
except IOError as e:

6
oauth2client/contrib/_win32_opener.py
View File

@@ -19,6 +19,7 @@ import pywintypes
import win32con
import win32file
from oauth2client import util
from oauth2client.contrib import locked_file
@@ -43,15 +44,14 @@ class _Win32Opener(locked_file._Opener):
Raises:
AlreadyLockedException: if the lock is already acquired.
IOError: if the open fails.
CredentialsFileSymbolicLinkError: if the file is a symbolic
link.
IOError: if the file is a symbolic link.
"""
if self._locked:
raise locked_file.AlreadyLockedException(
'File {0} is already locked'.format(self._filename))
start_time = time.time()
locked_file.validate_file(self._filename)
util.validate_file(self._filename)
try:
self._fh = open(self._filename, self._mode)
except IOError as e:

14
oauth2client/contrib/locked_file.py
View File

@@ -45,21 +45,11 @@ __author__ = 'cache@google.com (David T McWherter)'
logger = logging.getLogger(__name__)
class CredentialsFileSymbolicLinkError(Exception):
"""Credentials files must not be symbolic links."""
class AlreadyLockedException(Exception):
"""Trying to lock a file that has already been locked by the LockedFile."""
pass
def validate_file(filename):
if os.path.islink(filename):
raise CredentialsFileSymbolicLinkError(
'File: {0} is a symbolic link.'.format(filename))
class _Opener(object):
"""Base class for different locking primitives."""
@@ -119,14 +109,14 @@ class _PosixOpener(_Opener):
Raises:
AlreadyLockedException: if the lock is already acquired.
IOError: if the open fails.
CredentialsFileSymbolicLinkError if the file is a symbolic link.
IOError: if the file is a symbolic link.
"""
if self._locked:
raise AlreadyLockedException(
'File {0} is already locked'.format(self._filename))
self._locked = False
validate_file(self._filename)
util.validate_file(self._filename)
try:
self._fh = open(self._filename, self._mode)
except IOError as e:

18
oauth2client/file.py
View File

@@ -22,15 +22,12 @@ import os
import threading
from oauth2client import client
from oauth2client import util
__author__ = 'jcgregorio@google.com (Joe Gregorio)'
class CredentialsFileSymbolicLinkError(Exception):
"""Credentials files must not be symbolic links."""
class Storage(client.Storage):
"""Store and retrieve a single credential to and from a file."""
@@ -38,11 +35,6 @@ class Storage(client.Storage):
super(Storage, self).__init__(lock=threading.Lock())
self._filename = filename
def _validate_file(self):
if os.path.islink(self._filename):
raise CredentialsFileSymbolicLinkError(
'File: {0} is a symbolic link.'.format(self._filename))
def locked_get(self):
"""Retrieve Credential from file.
@@ -50,10 +42,10 @@ class Storage(client.Storage):
oauth2client.client.Credentials
Raises:
CredentialsFileSymbolicLinkError if the file is a symbolic link.
IOError if the file is a symbolic link.
"""
credentials = None
self._validate_file()
util.validate_file(self._filename)
try:
f = open(self._filename, 'rb')
content = f.read()
@@ -89,10 +81,10 @@ class Storage(client.Storage):
credentials: Credentials, the credentials to store.
Raises:
CredentialsFileSymbolicLinkError if the file is a symbolic link.
IOError if the file is a symbolic link.
"""
self._create_file_if_needed()
self._validate_file()
util.validate_file(self._filename)
f = open(self._filename, 'w')
f.write(credentials.to_json())
f.close()

15
oauth2client/util.py
View File

@@ -17,6 +17,8 @@
import functools
import inspect
import logging
import os
import warnings
import six
from six.moves import urllib
@@ -44,6 +46,10 @@ POSITIONAL_SET = frozenset([POSITIONAL_WARNING, POSITIONAL_EXCEPTION,
positional_parameters_enforcement = POSITIONAL_WARNING
_SYM_LINK_MESSAGE = 'File: {0}: Is a symbolic link.'
_IS_DIR_MESSAGE = '{0}: Is a directory'
_MISSING_FILE_MESSAGE = 'Cannot access {0}: No such file or directory'
def positional(max_positional_args):
"""A decorator to declare that only the first N arguments my be positional.
@@ -204,3 +210,12 @@ def _add_query_parameter(url, name, value):
q[name] = value
parsed[4] = urllib.parse.urlencode(q)
return urllib.parse.urlunparse(parsed)
def validate_file(filename):
if os.path.islink(filename):
raise IOError(_SYM_LINK_MESSAGE.format(filename))
elif os.path.isdir(filename):
raise IOError(_IS_DIR_MESSAGE.format(filename))
elif not os.path.isfile(filename):
warnings.warn(_MISSING_FILE_MESSAGE.format(filename))

20
tests/contrib/test_multistore_file.py
View File

@@ -25,7 +25,6 @@ import unittest2
from oauth2client import client
from oauth2client import util
from oauth2client.contrib import locked_file
from oauth2client.contrib import multistore_file
_filehandle, FILENAME = tempfile.mkstemp('oauth2client_test.data')
@@ -187,8 +186,7 @@ class MultistoreFileTests(unittest2.TestCase):
'user-agent/1.0',
['some-scope', 'some-other-scope'])
try:
with self.assertRaises(
locked_file.CredentialsFileSymbolicLinkError):
with self.assertRaises(IOError):
store.get()
finally:
os.unlink(SYMFILENAME)
@@ -201,7 +199,7 @@ class MultistoreFileTests(unittest2.TestCase):
['some-scope', 'some-other-scope'])
credentials = store.get()
self.assertEquals(None, credentials)
self.assertIsNone(credentials)
def test_multistore_file(self):
credentials = self._create_test_credentials()
@@ -216,14 +214,14 @@ class MultistoreFileTests(unittest2.TestCase):
store.put(credentials)
credentials = store.get()
self.assertNotEquals(None, credentials)
self.assertIsNotNone(credentials)
self.assertEquals('foo', credentials.access_token)
# Delete credentials
store.delete()
credentials = store.get()
self.assertEquals(None, credentials)
self.assertIsNone(credentials)
if os.name == 'posix': # pragma: NO COVER
self.assertEquals(
@@ -239,14 +237,14 @@ class MultistoreFileTests(unittest2.TestCase):
store.put(credentials)
stored_credentials = store.get()
self.assertNotEquals(None, stored_credentials)
self.assertIsNotNone(stored_credentials)
self.assertEqual(credentials.access_token,
stored_credentials.access_token)
store.delete()
stored_credentials = store.get()
self.assertEquals(None, stored_credentials)
self.assertIsNone(stored_credentials)
def test_multistore_file_custom_string_key(self):
credentials = self._create_test_credentials()
@@ -258,7 +256,7 @@ class MultistoreFileTests(unittest2.TestCase):
store.put(credentials)
stored_credentials = store.get()
self.assertNotEquals(None, stored_credentials)
self.assertIsNotNone(stored_credentials)
self.assertEqual(credentials.access_token,
stored_credentials.access_token)
@@ -266,14 +264,14 @@ class MultistoreFileTests(unittest2.TestCase):
multistore_file.get_credential_storage_custom_string_key(
FILENAME, {'key': 'mykey'})
stored_credentials = store.get()
self.assertNotEquals(None, stored_credentials)
self.assertIsNotNone(stored_credentials)
self.assertEqual(credentials.access_token,
stored_credentials.access_token)
store.delete()
stored_credentials = store.get()
self.assertEquals(None, stored_credentials)
self.assertIsNone(stored_credentials)
def test_multistore_file_backwards_compatibility(self):
credentials = self._create_test_credentials()

102
tests/test_file.py
View File

@@ -24,13 +24,16 @@ import os
import pickle
import stat
import tempfile
import warnings
import mock
import six
from six.moves import http_client
import unittest2
from oauth2client import client
from oauth2client import file
from oauth2client import util
from .http_mock import HttpMockSequence
try:
@@ -54,6 +57,7 @@ class OAuth2ClientFileTests(unittest2.TestCase):
pass
def setUp(self):
warnings.simplefilter("ignore")
try:
os.unlink(FILENAME)
except OSError:
@@ -74,19 +78,31 @@ class OAuth2ClientFileTests(unittest2.TestCase):
user_agent)
return credentials
def test_non_existent_file_storage(self):
s = file.Storage(FILENAME)
credentials = s.get()
self.assertEquals(None, credentials)
@mock.patch('warnings.warn')
def test_non_existent_file_storage(self, warn_mock):
storage = file.Storage(FILENAME)
credentials = storage.get()
warn_mock.assert_called_with(
util._MISSING_FILE_MESSAGE.format(FILENAME))
self.assertIsNone(credentials)
def test_directory_file_storage(self):
storage = file.Storage(FILENAME)
os.mkdir(FILENAME)
try:
with self.assertRaises(IOError):
storage.get()
finally:
os.rmdir(FILENAME)
@unittest2.skipIf(not hasattr(os, 'symlink'), 'No symlink available')
def test_no_sym_link_credentials(self):
SYMFILENAME = FILENAME + '.sym'
os.symlink(FILENAME, SYMFILENAME)
s = file.Storage(SYMFILENAME)
storage = file.Storage(SYMFILENAME)
try:
with self.assertRaises(file.CredentialsFileSymbolicLinkError):
s.get()
with self.assertRaises(IOError):
storage.get()
finally:
os.unlink(SYMFILENAME)
@@ -94,20 +110,20 @@ class OAuth2ClientFileTests(unittest2.TestCase):
# Write a file with a pickled OAuth2Credentials.
credentials = self._create_test_credentials()
f = open(FILENAME, 'wb')
pickle.dump(credentials, f)
f.close()
credentials_file = open(FILENAME, 'wb')
pickle.dump(credentials, credentials_file)
credentials_file.close()
# Storage should be not be able to read that object, as the capability
# to read and write credentials as pickled objects has been removed.
s = file.Storage(FILENAME)
read_credentials = s.get()
self.assertEquals(None, read_credentials)
storage = file.Storage(FILENAME)
read_credentials = storage.get()
self.assertIsNone(read_credentials)
# Now write it back out and confirm it has been rewritten as JSON
s.put(credentials)
with open(FILENAME) as f:
data = json.load(f)
storage.put(credentials)
with open(FILENAME) as credentials_file:
data = json.load(credentials_file)
self.assertEquals(data['access_token'], 'foo')
self.assertEquals(data['_class'], 'OAuth2Credentials')
@@ -118,12 +134,12 @@ class OAuth2ClientFileTests(unittest2.TestCase):
datetime.timedelta(minutes=15))
credentials = self._create_test_credentials(expiration=expiration)
s = file.Storage(FILENAME)
s.put(credentials)
credentials = s.get()
storage = file.Storage(FILENAME)
storage.put(credentials)
credentials = storage.get()
new_cred = copy.copy(credentials)
new_cred.access_token = 'bar'
s.put(new_cred)
storage.put(new_cred)
access_token = '1/3w'
token_response = {'access_token': access_token, 'expires_in': 3600}
@@ -141,12 +157,12 @@ class OAuth2ClientFileTests(unittest2.TestCase):
datetime.timedelta(minutes=15))
credentials = self._create_test_credentials(expiration=expiration)
s = file.Storage(FILENAME)
s.put(credentials)
credentials = s.get()
storage = file.Storage(FILENAME)
storage.put(credentials)
credentials = storage.get()
new_cred = copy.copy(credentials)
new_cred.access_token = 'bar'
s.put(new_cred)
storage.put(new_cred)
access_token = '1/3w'
token_response = {'access_token': access_token, 'expires_in': 3600}
@@ -170,12 +186,12 @@ class OAuth2ClientFileTests(unittest2.TestCase):
datetime.timedelta(minutes=15))
credentials = self._create_test_credentials(expiration=expiration)
s = file.Storage(FILENAME)
s.put(credentials)
credentials = s.get()
storage = file.Storage(FILENAME)
storage.put(credentials)
credentials = storage.get()
new_cred = copy.copy(credentials)
new_cred.access_token = 'bar'
s.put(new_cred)
storage.put(new_cred)
credentials._refresh(None)
self.assertEquals(credentials.access_token, 'bar')
@@ -185,12 +201,12 @@ class OAuth2ClientFileTests(unittest2.TestCase):
datetime.timedelta(minutes=15))
credentials = self._create_test_credentials(expiration=expiration)
s = file.Storage(FILENAME)
s.put(credentials)
credentials = s.get()
storage = file.Storage(FILENAME)
storage.put(credentials)
credentials = storage.get()
new_cred = copy.copy(credentials)
new_cred.access_token = 'bar'
s.put(new_cred)
storage.put(new_cred)
valid_access_token = '1/3w'
token_response = {'access_token': valid_access_token,
@@ -215,13 +231,13 @@ class OAuth2ClientFileTests(unittest2.TestCase):
def test_credentials_delete(self):
credentials = self._create_test_credentials()
s = file.Storage(FILENAME)
s.put(credentials)
credentials = s.get()
self.assertNotEquals(None, credentials)
s.delete()
credentials = s.get()
self.assertEquals(None, credentials)
storage = file.Storage(FILENAME)
storage.put(credentials)
credentials = storage.get()
self.assertIsNotNone(credentials)
storage.delete()
credentials = storage.get()
self.assertIsNone(credentials)
def test_access_token_credentials(self):
access_token = 'foo'
@@ -229,11 +245,11 @@ class OAuth2ClientFileTests(unittest2.TestCase):
credentials = client.AccessTokenCredentials(access_token, user_agent)
s = file.Storage(FILENAME)
credentials = s.put(credentials)
credentials = s.get()
storage = file.Storage(FILENAME)
credentials = storage.put(credentials)
credentials = storage.get()
self.assertNotEquals(None, credentials)
self.assertIsNotNone(credentials)
self.assertEquals('foo', credentials.access_token)
self.assertTrue(os.path.exists(FILENAME))