From dc542ab89ebda22e5c082128c476ffb65c79e1dd Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Tue, 24 Nov 2015 13:40:26 -0800 Subject: [PATCH] Add two new storage classes for #319. * LockedStorage - implements a generic storage base class that uses a `threading.Lock` like-object. * DictionaryStorage - implements an optionally-locked storage over a dictionary-like object. Additionally: * Updated `file.Storage` to use `LockedStorage`. * Remove `flask_util.FlaskSessionStorage` and replaced it with `DictionaryStorage`. --- .../oauth2client.dictionary_storage.rst | 7 ++ docs/source/oauth2client.locked_storage.rst | 7 ++ docs/source/oauth2client.rst | 2 + oauth2client/appengine.py | 2 + oauth2client/client.py | 16 +++- oauth2client/dictionary_storage.py | 72 +++++++++++++++ oauth2client/django_orm.py | 1 + oauth2client/file.py | 17 +--- oauth2client/flask_util.py | 32 +------ oauth2client/keyring_storage.py | 17 +--- tests/test_dictionary_storage.py | 84 ++++++++++++++++++ tests/test_locked_storage.py | 87 +++++++++++++++++++ 12 files changed, 279 insertions(+), 65 deletions(-) create mode 100644 docs/source/oauth2client.dictionary_storage.rst create mode 100644 docs/source/oauth2client.locked_storage.rst create mode 100644 oauth2client/dictionary_storage.py create mode 100644 tests/test_dictionary_storage.py create mode 100644 tests/test_locked_storage.py diff --git a/docs/source/oauth2client.dictionary_storage.rst b/docs/source/oauth2client.dictionary_storage.rst new file mode 100644 index 000000000..952a7ba9b --- /dev/null +++ b/docs/source/oauth2client.dictionary_storage.rst @@ -0,0 +1,7 @@ +oauth2client.dictionary_storage module +====================================== + +.. automodule:: oauth2client.dictionary_storage + :members: + :undoc-members: + :show-inheritance: diff --git a/docs/source/oauth2client.locked_storage.rst b/docs/source/oauth2client.locked_storage.rst new file mode 100644 index 000000000..9042b3285 --- /dev/null +++ b/docs/source/oauth2client.locked_storage.rst @@ -0,0 +1,7 @@ +oauth2client.locked_storage module +================================== + +.. automodule:: oauth2client.locked_storage + :members: + :undoc-members: + :show-inheritance: diff --git a/docs/source/oauth2client.rst b/docs/source/oauth2client.rst index ba0626adc..c65b2ff4d 100644 --- a/docs/source/oauth2client.rst +++ b/docs/source/oauth2client.rst @@ -11,12 +11,14 @@ Submodules oauth2client.clientsecrets oauth2client.crypt oauth2client.devshell + oauth2client.dictionary_storage oauth2client.django_orm oauth2client.file oauth2client.flask_util oauth2client.gce oauth2client.keyring_storage oauth2client.locked_file + oauth2client.locked_storage oauth2client.multistore_file oauth2client.service_account oauth2client.tools diff --git a/oauth2client/appengine.py b/oauth2client/appengine.py index fcbef3e1b..a7e5dedc6 100644 --- a/oauth2client/appengine.py +++ b/oauth2client/appengine.py @@ -408,6 +408,8 @@ def __init__(self, model, key_name, property_name, cache=None, user=None): user: users.User object, optional. Can be used to grab user ID as a key_name if no key name is specified. """ + super(StorageByKeyName, self).__init__() + if key_name is None: if user is None: raise ValueError('StorageByKeyName called with no ' diff --git a/oauth2client/client.py b/oauth2client/client.py index cd5959f24..ce79d2939 100644 --- a/oauth2client/client.py +++ b/oauth2client/client.py @@ -340,21 +340,31 @@ class Storage(object): such that multiple processes and threads can operate on a single store. """ + def __init__(self, lock=None): + """Create a Storage instance. + + Args: + lock: An optional threading.Lock-like object. Must implement at + least acquire() and release(). Does not need to be re-entrant. + """ + self._lock = lock def acquire_lock(self): """Acquires any lock necessary to access this Storage. This lock is not reentrant. """ - pass + if self._lock is not None: + self._lock.acquire() def release_lock(self): """Release the Storage lock. Trying to release a lock that isn't held will result in a - RuntimeError. + RuntimeError in the case of a threading.Lock or multiprocessing.Lock. """ - pass + if self._lock is not None: + self._lock.release() def locked_get(self): """Retrieve credential. diff --git a/oauth2client/dictionary_storage.py b/oauth2client/dictionary_storage.py new file mode 100644 index 000000000..35d975f90 --- /dev/null +++ b/oauth2client/dictionary_storage.py @@ -0,0 +1,72 @@ +# Copyright 2014 Google Inc. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Dictionary storage for OAuth2 Credentials.""" + +from oauth2client.client import OAuth2Credentials +from oauth2client.client import Storage + + +class DictionaryStorage(Storage): + """Store and retrieve credentials to and from a dictionary-like object.""" + + def __init__(self, dictionary, key, lock=None): + """Construct a DictionaryStorage instance. + + Args: + dictionary: A dictionary or dictionary-like object. + key: A hashable or a function returning a hashable. The credentials + will be stored in ``dictionary[key]``. + lock: An optional threading.Lock-like object. The lock will be + acquired before anything is written or read from the + dictionary. + """ + super(DictionaryStorage, self).__init__(lock=lock) + self._dictionary = dictionary + self._key = key + + def _get_key(self): + """Return the key for storing the credentials. + + If self._key is a callable, it will return the result of calling + self._key. + """ + if callable(self._key): + return self._key() + else: + return self._key + + def locked_get(self): + """Retrieve the credentials from the dictionary, if they exist.""" + key = self._get_key() + serialized = self._dictionary.get(key) + + if serialized is None: + return None + + credentials = OAuth2Credentials.from_json(serialized) + credentials.set_store(self) + + return credentials + + def locked_put(self, credentials): + """Save the credentials to the dictionary.""" + key = self._get_key() + serialized = credentials.to_json() + self._dictionary[key] = serialized + + def locked_delete(self): + """Remove the credentials from the dictionary, if they exist.""" + key = self._get_key() + self._dictionary.pop(key, None) diff --git a/oauth2client/django_orm.py b/oauth2client/django_orm.py index e6d31a602..cd22c1b71 100644 --- a/oauth2client/django_orm.py +++ b/oauth2client/django_orm.py @@ -124,6 +124,7 @@ def __init__(self, model_class, key_name, key_value, property_name): property_name: string, name of the property that is an CredentialsProperty """ + super(Storage, self).__init__() self.model_class = model_class self.key_name = key_name self.key_value = key_value diff --git a/oauth2client/file.py b/oauth2client/file.py index d0dd174f9..d48235919 100644 --- a/oauth2client/file.py +++ b/oauth2client/file.py @@ -36,29 +36,14 @@ class Storage(BaseStorage): """Store and retrieve a single credential to and from a file.""" def __init__(self, filename): + super(Storage, self).__init__(lock=threading.Lock()) self._filename = filename - self._lock = threading.Lock() def _validate_file(self): if os.path.islink(self._filename): raise CredentialsFileSymbolicLinkError( 'File: %s is a symbolic link.' % self._filename) - def acquire_lock(self): - """Acquires any lock necessary to access this Storage. - - This lock is not reentrant. - """ - self._lock.acquire() - - def release_lock(self): - """Release the Storage lock. - - Trying to release a lock that isn't held will result in a - RuntimeError. - """ - self._lock.release() - def locked_get(self): """Retrieve Credential from file. diff --git a/oauth2client/flask_util.py b/oauth2client/flask_util.py index 52eea3ef1..e33556f4e 100644 --- a/oauth2client/flask_util.py +++ b/oauth2client/flask_util.py @@ -185,7 +185,7 @@ def requires_calendar(): from oauth2client.client import FlowExchangeError from oauth2client.client import OAuth2Credentials from oauth2client.client import OAuth2WebServerFlow -from oauth2client.client import Storage +from oauth2client.dictionary_storage import DictionaryStorage from oauth2client import clientsecrets @@ -262,7 +262,7 @@ def init_app(self, app, scopes=None, client_secrets_file=None, self.flow_kwargs = kwargs if storage is None: - storage = FlaskSessionStorage() + storage = DictionaryStorage(session, _CREDENTIALS_KEY) self.storage = storage if scopes is None: @@ -546,31 +546,3 @@ def http(self, *args, **kwargs): if not self.credentials: raise ValueError('No credentials available.') return self.credentials.authorize(httplib2.Http(*args, **kwargs)) - - -class FlaskSessionStorage(Storage): - """Storage implementation that uses Flask sessions. - - Note that flask's default sessions are signed but not encrypted. Users - can see their own credentials and non-https connections can intercept user - credentials. We strongly recommend using a server-side session - implementation. - """ - - def locked_get(self): - serialized = session.get(_CREDENTIALS_KEY) - - if serialized is None: - return None - - credentials = OAuth2Credentials.from_json(serialized) - credentials.set_store(self) - - return credentials - - def locked_put(self, credentials): - session[_CREDENTIALS_KEY] = credentials.to_json() - - def locked_delete(self): - if _CREDENTIALS_KEY in session: - del session[_CREDENTIALS_KEY] diff --git a/oauth2client/keyring_storage.py b/oauth2client/keyring_storage.py index 0a4c2857b..431b67b7a 100644 --- a/oauth2client/keyring_storage.py +++ b/oauth2client/keyring_storage.py @@ -59,24 +59,9 @@ def __init__(self, service_name, user_name): credentials are stored. user_name: string, The name of the user to store credentials for. """ + super(Storage, self).__init__(lock=threading.Lock()) self._service_name = service_name self._user_name = user_name - self._lock = threading.Lock() - - def acquire_lock(self): - """Acquires any lock necessary to access this Storage. - - This lock is not reentrant. - """ - self._lock.acquire() - - def release_lock(self): - """Release the Storage lock. - - Trying to release a lock that isn't held will result in a - RuntimeError. - """ - self._lock.release() def locked_get(self): """Retrieve Credential from file. diff --git a/tests/test_dictionary_storage.py b/tests/test_dictionary_storage.py new file mode 100644 index 000000000..1bdfa1700 --- /dev/null +++ b/tests/test_dictionary_storage.py @@ -0,0 +1,84 @@ +# Copyright 2014 Google Inc. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Unit tests for oauth2client.dictionary_storage""" + +import unittest + +from oauth2client import GOOGLE_TOKEN_URI +from oauth2client.client import OAuth2Credentials +from oauth2client.dictionary_storage import DictionaryStorage + + +__author__ = 'jonwayne@google.com (Jon Wayne Parrott)' + + +class DictionaryStorageTests(unittest.TestCase): + + def _generate_credentials(self, scopes=None): + return OAuth2Credentials( + 'access_tokenz', + 'client_idz', + 'client_secretz', + 'refresh_tokenz', + '3600', + GOOGLE_TOKEN_URI, + 'Test', + id_token={ + 'sub': '123', + 'email': 'user@example.com' + }, + scopes=scopes) + + def test_string_key(self): + dictionary = {} + key = 'credentials' + credentials = self._generate_credentials() + storage = DictionaryStorage(dictionary, key) + + storage.put(credentials) + + self.assertTrue(key in dictionary) + + retrieved = storage.get() + + self.assertEqual(retrieved.access_token, credentials.access_token) + + storage.delete() + + self.assertTrue(key not in dictionary) + self.assertTrue(storage.get() is None) + + def test_function_key(self): + dictionary = {} + key = 'credentials' + credentials = self._generate_credentials() + storage = DictionaryStorage(dictionary, lambda: key) + + storage.put(credentials) + + self.assertTrue(key in dictionary) + + retrieved = storage.get() + + self.assertEqual(retrieved.access_token, credentials.access_token) + + storage.delete() + + self.assertTrue(key not in dictionary) + self.assertTrue(storage.get() is None) + + +if __name__ == '__main__': # pragma: NO COVER + unittest.main() diff --git a/tests/test_locked_storage.py b/tests/test_locked_storage.py new file mode 100644 index 000000000..4f4b8df0c --- /dev/null +++ b/tests/test_locked_storage.py @@ -0,0 +1,87 @@ +# # Copyright 2014 Google Inc. All rights reserved. +# # +# # Licensed under the Apache License, Version 2.0 (the "License"); +# # you may not use this file except in compliance with the License. +# # You may obtain a copy of the License at +# # +# # http://www.apache.org/licenses/LICENSE-2.0 +# # +# # Unless required by applicable law or agreed to in writing, software +# # distributed under the License is distributed on an "AS IS" BASIS, +# # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# # See the License for the specific language governing permissions and +# # limitations under the License. + +# """Unit tests for oauth2client.locked_storage""" + +# import unittest + +# import mock + +# from oauth2client import locked_storage + + +# __author__ = 'jonwayne@google.com (Jon Wayne Parrott)' + + +# class MockLockedStorageClass(locked_storage.LockedStorage): +# put_called = False +# get_called = False +# delete_called = False + +# def locked_put(self, credentials): +# self.put_called = True + +# def locked_get(self): +# self.get_called = True + +# def locked_delete(self): +# self.delete_called = True + + +# class LockedStorageTests(unittest.TestCase): + +# def test_put(self): +# lock = mock.Mock() +# storage = MockLockedStorageClass(lock=lock) + +# storage.put(object()) + +# self.assertTrue(storage.put_called) +# self.assertTrue(lock.acquire.called) +# self.assertTrue(lock.release.called) + +# def test_get(self): +# lock = mock.Mock() +# storage = MockLockedStorageClass(lock=lock) + +# storage.get() + +# self.assertTrue(storage.get_called) +# self.assertTrue(lock.acquire.called) +# self.assertTrue(lock.release.called) + +# def test_delete(self): +# lock = mock.Mock() +# storage = MockLockedStorageClass(lock=lock) + +# storage.delete() + +# self.assertTrue(storage.delete_called) +# self.assertTrue(lock.acquire.called) +# self.assertTrue(lock.release.called) + +# def test_no_lock(self): +# storage = MockLockedStorageClass(lock=None) + +# storage.put(object()) +# storage.get() +# storage.delete() + +# self.assertTrue(storage.put_called) +# self.assertTrue(storage.get_called) +# self.assertTrue(storage.delete_called) + + +# if __name__ == '__main__': # pragma: NO COVER +# unittest.main()