diff --git a/oauth2client/contrib/_fcntl_opener.py b/oauth2client/contrib/_fcntl_opener.py index ae6c85b..c9777a9 100644 --- a/oauth2client/contrib/_fcntl_opener.py +++ b/oauth2client/contrib/_fcntl_opener.py @@ -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: diff --git a/oauth2client/contrib/_win32_opener.py b/oauth2client/contrib/_win32_opener.py index 34b4f48..6fa0196 100644 --- a/oauth2client/contrib/_win32_opener.py +++ b/oauth2client/contrib/_win32_opener.py @@ -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: diff --git a/oauth2client/contrib/locked_file.py b/oauth2client/contrib/locked_file.py index 0d28ebb..9c880d7 100644 --- a/oauth2client/contrib/locked_file.py +++ b/oauth2client/contrib/locked_file.py @@ -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: diff --git a/oauth2client/file.py b/oauth2client/file.py index feede11..3d8e41a 100644 --- a/oauth2client/file.py +++ b/oauth2client/file.py @@ -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() diff --git a/oauth2client/util.py b/oauth2client/util.py index e3ba62b..18481c9 100644 --- a/oauth2client/util.py +++ b/oauth2client/util.py @@ -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)) diff --git a/tests/contrib/test_multistore_file.py b/tests/contrib/test_multistore_file.py index b5cb598..ae0664d 100644 --- a/tests/contrib/test_multistore_file.py +++ b/tests/contrib/test_multistore_file.py @@ -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() diff --git a/tests/test_file.py b/tests/test_file.py index 924acb4..7fdb627 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -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))