Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrating to Google Auth Library #180

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0236bf9
Update setup.py with new google-auth dependencies
junpeng-jp May 28, 2022
0b67559
Update threadsafe code following google recommendations
junpeng-jp May 28, 2022
799b005
Migration to google-auth library (#89)
junpeng-jp May 28, 2022
2a0a767
Implement lockfile storage (#89)
junpeng-jp May 28, 2022
9230969
Cleanup of oauth tests & remove service config read/write checks
junpeng-jp May 28, 2022
2a688e2
Add oauth test 10: google.auth.default auth
junpeng-jp May 28, 2022
a3bc162
Deprecate command line auth (#173)
junpeng-jp May 28, 2022
c9a0f90
Remove dependencies on oauth2client
junpeng-jp May 28, 2022
012f9ed
Changed mocker.spy to track calls to FileBackend's __init__
junpeng-jp Jun 1, 2022
015a84f
Add simulated token expiry test
junpeng-jp Jun 5, 2022
51af33c
Reverted DEFAULT_SETTINGS to a class static variable
junpeng-jp Jul 24, 2022
0b1c17b
Fixed typo in GoogleAuth __init__ documentation
junpeng-jp Jul 24, 2022
9ea5409
Removed non-essential improvements to settings.py
junpeng-jp Jul 24, 2022
d8c1571
Remove unnecessary edits to .yaml cred files
junpeng-jp Jul 24, 2022
35e9ae3
Update incorrect documentation for oauth Test 10
junpeng-jp Jul 24, 2022
badbd4f
Add back LoadAuth & thread_local HTTP
junpeng-jp Jul 24, 2022
08d067c
Remove Refresh method as it is now handled by google auth library
junpeng-jp Jul 24, 2022
2477cbe
Add back "Authentication successful" message
junpeng-jp Jul 24, 2022
91a4f63
Clean-up LocalWebserverAuth & added OSError documentation
junpeng-jp Jul 24, 2022
9a9af28
Allow trying of new ports for all OSErrors
junpeng-jp Jul 30, 2022
46ba0c1
Allow manual overwrite of self.http for API resources
junpeng-jp Jul 30, 2022
710eee1
Deprecate CommandLineAuth
junpeng-jp Jul 30, 2022
6b3e901
Added Thread Locking & updated credential write logic
junpeng-jp Jul 30, 2022
82a8960
Add threading import
junpeng-jp Jul 30, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
667 changes: 318 additions & 349 deletions pydrive2/auth.py

Large diffs are not rendered by default.

58 changes: 58 additions & 0 deletions pydrive2/auth_helpers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
_OLD_CLIENT_CONFIG_KEYS = frozenset(
(
"client_id",
"client_secret",
"auth_uri",
"token_uri",
"revoke_uri",
"redirect_uri",
)
)

_CLIENT_CONFIG_KEYS = frozenset(
(
"client_id",
"client_secret",
"auth_uri",
"token_uri",
"redirect_uris",
)
)


def verify_client_config(client_config, with_oauth_type=True):
"""Verifies that format of the client config
loaded from a Google-format client secrets file.
"""

oauth_type = None
config = client_config

if with_oauth_type:
if "web" in client_config:
oauth_type = "web"
config = config["web"]

elif "installed" in client_config:
oauth_type = "installed"
config = config["installed"]
else:
raise ValueError(
"Client secrets must be for a web or installed app"
)

# This is the older format of client config
if _OLD_CLIENT_CONFIG_KEYS.issubset(config.keys()):
config["redirect_uris"] = [config["redirect_uri"]]

# by default, the redirect uri is the first in the list
if "redirect_uri" not in config:
config["redirect_uri"] = config["redirect_uris"][0]

if "revoke_uri" not in config:
config["revoke_uri"] = "https://oauth2.googleapis.com/revoke"

if not _CLIENT_CONFIG_KEYS.issubset(config.keys()):
raise ValueError("Client secrets is not in the correct format.")

return oauth_type, config
6 changes: 5 additions & 1 deletion pydrive2/drive.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,8 @@ def GetAbout(self):

:returns: A dictionary of Google Drive information like user, usage, quota etc.
"""
return self.auth.service.about().get().execute(http=self.http)
return (
self.auth.service.about()
.get()
.execute(http=self.auth.Get_Http_Object())
)
35 changes: 21 additions & 14 deletions pydrive2/files.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def _GetList(self):
self.metadata = (
self.auth.service.files()
.list(**dict(self))
.execute(http=self.http)
.execute(http=self.auth.Get_Http_Object())
)
except errors.HttpError as error:
raise ApiRequestError(error)
Expand Down Expand Up @@ -440,7 +440,7 @@ def FetchMetadata(self, fields=None, fetch_all=False):
# Teamdrive support
supportsAllDrives=True,
)
.execute(http=self.http)
.execute(http=self.auth.Get_Http_Object())
)
except errors.HttpError as error:
raise ApiRequestError(error)
Expand Down Expand Up @@ -543,7 +543,7 @@ def InsertPermission(self, new_permission, param=None):
permission = (
self.auth.service.permissions()
.insert(**param)
.execute(http=self.http)
.execute(http=self.auth.Get_Http_Object())
)
except errors.HttpError as error:
raise ApiRequestError(error)
Expand Down Expand Up @@ -574,7 +574,7 @@ def GetPermissions(self):
# Teamdrive support
supportsAllDrives=True,
)
.execute(http=self.http)
.execute(http=self.auth.Get_Http_Object())
).get("items")

if permissions:
Expand All @@ -594,13 +594,13 @@ def DeletePermission(self, permission_id):
return self._DeletePermission(permission_id)

def _WrapRequest(self, request):
"""Replaces request.http with self.http.
"""Replaces request.http with self.auth.Get_Http_Object().

Ensures thread safety. Similar to other places where we call
`.execute(http=self.http)` to pass a client from the thread local storage.
"""
if self.http:
request.http = self.http
if self.auth:
request.http = self.auth.Get_Http_Object()
return request

@LoadAuth
Expand All @@ -624,7 +624,7 @@ def _FilesInsert(self, param=None):
metadata = (
self.auth.service.files()
.insert(**param)
.execute(http=self.http)
.execute(http=self.auth.Get_Http_Object())
)
except errors.HttpError as error:
raise ApiRequestError(error)
Expand All @@ -648,7 +648,9 @@ def _FilesUnTrash(self, param=None):
param["supportsAllDrives"] = True

try:
self.auth.service.files().untrash(**param).execute(http=self.http)
self.auth.service.files().untrash(**param).execute(
http=self.auth.Get_Http_Object()
)
except errors.HttpError as error:
raise ApiRequestError(error)
else:
Expand All @@ -672,7 +674,9 @@ def _FilesTrash(self, param=None):
param["supportsAllDrives"] = True

try:
self.auth.service.files().trash(**param).execute(http=self.http)
self.auth.service.files().trash(**param).execute(
http=self.auth.Get_Http_Object()
)
except errors.HttpError as error:
raise ApiRequestError(error)
else:
Expand All @@ -697,7 +701,9 @@ def _FilesDelete(self, param=None):
param["supportsAllDrives"] = True

try:
self.auth.service.files().delete(**param).execute(http=self.http)
self.auth.service.files().delete(**param).execute(
http=self.auth.Get_Http_Object()
)
except errors.HttpError as error:
raise ApiRequestError(error)
else:
Expand Down Expand Up @@ -726,7 +732,7 @@ def _FilesUpdate(self, param=None):
metadata = (
self.auth.service.files()
.update(**param)
.execute(http=self.http)
.execute(http=self.auth.Get_Http_Object())
)
except errors.HttpError as error:
raise ApiRequestError(error)
Expand Down Expand Up @@ -756,7 +762,7 @@ def _FilesPatch(self, param=None):
metadata = (
self.auth.service.files()
.patch(**param)
.execute(http=self.http)
.execute(http=self.auth.Get_Http_Object())
)
except errors.HttpError as error:
raise ApiRequestError(error)
Expand Down Expand Up @@ -785,7 +791,8 @@ def _DownloadFromUrl(self, url):
:returns: str -- content of downloaded file in string.
:raises: ApiRequestError
"""
resp, content = self.http.request(url)
http = self.auth.Get_Http_Object()
resp, content = http.request(url)
if resp.status != 200:
raise ApiRequestError(errors.HttpError(resp, content, uri=url))
return content
Expand Down
1 change: 1 addition & 0 deletions pydrive2/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
"client_service_email": {"type": str, "required": False},
"client_pkcs12_file_path": {"type": str, "required": False},
"client_json_file_path": {"type": str, "required": False},
"use_default": {"type": bool, "required": False},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we skip for the sake of this PR the default credentials auth? I think it should be done outside of the ServiceAuth + probably we should be using a different setting for this (don't know which one yet)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll shift this into a subsequent PR once we've migrated to new google auth library

},
},
"oauth_scope": {
Expand Down
117 changes: 117 additions & 0 deletions pydrive2/storage.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import os
import json
import warnings
from filelock import FileLock


_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 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))


class CredentialBackend(object):
"""Adapter that provides a consistent interface to read and write credential files"""

def _read_credentials(self, rpath):
"""Specific implementation of how the storage object should retrieve a file."""
return NotImplementedError

def _store_credentials(self, credential, rpath):
"""Specific implementation of how the storage object should write a file"""
return NotImplementedError

def _delete_credentials(self, rpath):
"""Specific implementation of how the storage object should delete a file."""
return NotImplementedError

def read_credentials(self, rpath):
"""Reads a credential config file and returns the config as a dictionary
:param fname: host name of the local web server.
:type host_name: str.`
:return: A credential file
"""
return self._read_credentials(rpath)

def store_credentials(self, credential, rpath):
"""Write a credential to
The Storage lock must be held when this is called.
Args:
credentials: Credentials, the credentials to store.
"""
self._store_credentials(credential, rpath)

def delete_credentials(self, rpath):
"""Delete credential.
Frees any resources associated with storing the credential.
The Storage lock must *not* be held when this is called.

Returns:
None
"""
self._delete_credentials(rpath)


class FileBackend(CredentialBackend):
# https://stackoverflow.com/questions/37084682/is-oauth-thread-safe
"""Read and write credentials to a file backend with File Locking"""

def __init__(self):
self._locks = {}

def createLock(self, rpath):
self._locks[rpath] = FileLock("{}.lock".format(rpath))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous implementation was Semaphore / Mutex or something like this-based. File based locks are problematic. Primarily because of something happens you would have to go and delete them manually. Let's please stick for now with what we had in the code before. For multi-threaded safety it's enough to use an in-memory lock.

When we write into a file (new credentials), we can guarantee multi-process safety with writing into a temp file first and then doing mv.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly confused by the purpose of any sort of thread locks for the Credentials object. I'm under the impression that there should only be one GoogleAuth object with self.credentials shared across all threads & the credentials would be refreshed synchronously.

Some sources that suggest that credentials should be synchronously refreshed:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update this to the create temp file -> os.replace(temp_file, actual_filel) implementation instead of filelock

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the credentials would be refreshed synchronously.

I think that's why those locks exist, since in the current implementation multiple threads could try to refresh (and write it back to a file) them simultaneously. With the current implementation I'm not sure we are writing the credentials on refresh. It would be another difference in the implementation that we need to double check and see if there are some important implications.

I will update this to the create temp file -> os.replace(temp_file, actual_filel) implementation instead of filelock

thanks.


def getLock(self, rpath):
if rpath not in self._locks:
self.createLock(rpath)
return self._locks[rpath]

def _create_file_if_needed(self, rpath):
"""Create an empty file if necessary.
This method will not initialize the file. Instead it implements a
simple version of "touch" to ensure the file has been created.
"""
if not os.path.exists(rpath):
old_umask = os.umask(0o177)
try:
open(rpath, "a+b").close()
finally:
os.umask(old_umask)

def _read_credentials(self, rpath):
"""Reads a local json file and parses the information into a info dictionary.
Returns:
Raises:
"""
with self.getLock(rpath):
with open(rpath, "r") as json_file:
return json.load(json_file)

def _store_credentials(self, credentials, rpath):
"""Writes current credentials to a local json file.
Args:
Raises:
"""
with self.getLock(rpath):
self._create_file_if_needed(rpath)
validate_file(rpath)

with open(rpath, "w") as json_file:
json_file.write(credentials.to_json())

def _delete_credentials(self, rpath):
"""Delete Credentials file.
Args:
credentials: Credentials, the credentials to store.
"""
with self.getLock(rpath):
os.unlink(rpath)
11 changes: 11 additions & 0 deletions pydrive2/test/settings/default_user.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
client_config_backend: file
client_config_file: tmp/pydrive2/user.json

save_credentials: True
save_credentials_backend: file
save_credentials_file: credentials/default_user.dat

oauth_scope:
- https://www.googleapis.com/auth/drive

get_refresh_token: True
9 changes: 9 additions & 0 deletions pydrive2/test/settings/default_user_no_refresh.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
client_config_backend: file
client_config_file: tmp/pydrive2/user.json

save_credentials: True
save_credentials_backend: file
save_credentials_file: credentials/default_user_no_refresh.dat

oauth_scope:
- https://www.googleapis.com/auth/drive
5 changes: 5 additions & 0 deletions pydrive2/test/settings/test_oauth_test_10.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
client_config_backend: service
service_config:
use_default: True
oauth_scope:
- https://www.googleapis.com/auth/drive
Loading