Skip to content

Commit

Permalink
Issue ref delta-io#269: OAuth 2.0 Credential Format for Delta Sharing…
Browse files Browse the repository at this point in the history
… Python Client Pull Request

Signed-off-by: dialberg <[email protected]>
  • Loading branch information
dialberg committed May 12, 2023
1 parent 7f01260 commit 33bd3ba
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 14 deletions.
9 changes: 9 additions & 0 deletions NOTICE.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
###############################################################################################
#
# Copyright © 2023, 2023, Oracle and/or its affiliates.
# Issue ref #269: OAuth 2.0 Credential Format for Delta Sharing Client
# Code update:
# - protocol.py
# - rest_client.py
#
###############################################################################################
73 changes: 62 additions & 11 deletions python/delta_sharing/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,18 @@

@dataclass(frozen=True)
class DeltaSharingProfile:
CURRENT: ClassVar[int] = 1
CURRENT: ClassVar[int] = 2

share_credentials_version: int
endpoint: str
bearer_token: str
bearer_token: Optional[str] = None
expiration_time: Optional[str] = None
type: Optional[str] = None
token_endpoint: Optional[str] = None
client_id: Optional[str] = None
client_secret: Optional[str] = None
username: Optional[str] = None
password: Optional[str] = None

def __post_init__(self):
if self.share_credentials_version > DeltaSharingProfile.CURRENT:
Expand All @@ -55,18 +61,63 @@ def read_from_file(profile: Union[str, IO, Path]) -> "DeltaSharingProfile":
@staticmethod
def from_json(json) -> "DeltaSharingProfile":
if isinstance(json, (str, bytes, bytearray)):
json = loads(json)
json = loads(json)

share_credentials_version = int(json["shareCredentialsVersion"])
endpoint = json["endpoint"]
if endpoint.endswith("/"):
if endpoint is not None and endpoint.endswith("/"):
endpoint = endpoint[:-1]
expiration_time = json.get("expirationTime")
return DeltaSharingProfile(
share_credentials_version=int(json["shareCredentialsVersion"]),
endpoint=endpoint,
bearer_token=json["bearerToken"],
expiration_time=expiration_time,
)

if share_credentials_version == 1:
return DeltaSharingProfile(
share_credentials_version=share_credentials_version,
endpoint=endpoint,
bearer_token = json["bearerToken"],
expiration_time = json.get("expirationTime"),
)
elif share_credentials_version == 2:
type = json["type"]
if type == "persistent_oauth2.0":
token_endpoint = json["tokenEndpoint"]
if token_endpoint is not None and token_endpoint.endswith("/"):
token_endpoint = token_endpoint[:-1]
return DeltaSharingProfile(
share_credentials_version=share_credentials_version,
type=type,
endpoint=endpoint,
token_endpoint = token_endpoint,
client_id = json["clientID"],

This comment has been minimized.

Copy link
@zhuansunxt

zhuansunxt May 18, 2023

This should be clientId

This comment has been minimized.

Copy link
@dialberg

dialberg May 23, 2023

Author Owner

Fixed.

client_secret = json["clientSecret"],
)
elif type == "bearer_token":
return DeltaSharingProfile(
share_credentials_version=share_credentials_version,
type=type,
endpoint=endpoint,
bearer_token = json["bearerToken"],
expiration_time = json.get("expirationTime")
)
elif type == "basic":
return DeltaSharingProfile(
share_credentials_version=share_credentials_version,
type=type,
endpoint=endpoint,
username = json["username"],
password = json["password"],
)
else:
raise ValueError(
"The current release does not supports {type} type. "
"Please check type."
)
else:
raise ValueError(
"'shareCredentialsVersion' in the profile is "
f"{share_credentials_version} which is too new. "
f"The current release supports version {DeltaSharingProfile.CURRENT} and below. "
"Please upgrade to a newer release."
)


@dataclass(frozen=True)
class Share:
Expand Down
50 changes: 47 additions & 3 deletions python/delta_sharing/rest_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,16 +143,60 @@ def __init__(self, profile: DeltaSharingProfile, num_retries=10):
self._profile = profile
self._num_retries = num_retries
self._sleeper = lambda sleep_ms: time.sleep(sleep_ms / 1000)

self.auth_session(profile)

def auth_session(self, profile) -> requests.Session:
self._session = requests.Session()
self.auth_broker(profile)
if urlparse(profile.endpoint).hostname == "localhost":
self._session.verify = False

def auth_broker(self, profile):

This comment has been minimized.

Copy link
@zhuansunxt

zhuansunxt May 18, 2023

It'll be great to add some basic unit test for different auth scheme.

This comment has been minimized.

Copy link
@dialberg

dialberg May 23, 2023

Author Owner

Unit tests added.

if profile.share_credentials_version == 2:
if profile.type == "persistent_oauth2.0":
self.auth_persistent_oauth2(profile)
elif profile.type == "bearer_token":
self.auth_bearer_token(profile)
elif profile.type == "basic":
self.auth_basic(profile)
else:
self.auth_bearer_token(profile)
else:
self.auth_bearer_token(profile)

def auth_bearer_token(self, profile):
self._session.headers.update(
{
"Authorization": f"Bearer {profile.bearer_token}",
"User-Agent": DataSharingRestClient.USER_AGENT,
}
)
if urlparse(profile.endpoint).hostname == "localhost":
self._session.verify = False

def auth_persistent_oauth2(self, profile):
response = requests.post(profile.token_endpoint,

This comment has been minimized.

Copy link
@zhuansunxt

zhuansunxt May 18, 2023

See below as an example to call an oauth2.0 token endpoint. I think you need to specify the Accept and Content-type header too based on delta-io#269.

curl -X POST \
--
  | -H "Authorization: Basic $(echo -n $CLIENT_ID:$CLIENT_SECRET | base64)" \
  | -H "Content-Type: application/x-www-form-urlencoded" \
  | -H "Accept: application/json" \
  | -d "grant_type=client_credentials" \
  | https://example.api.com/oauth/v1/token

This comment has been minimized.

Copy link
@dialberg

dialberg May 23, 2023

Author Owner

Fixed.

data = {"grant_type": "client_credentials"},
auth = (profile.client_id, profile.client_secret),)
bearer_token = "{}".format(response.json()["access_token"])

self._session.headers.update(
{
"Authorization": f"Bearer {bearer_token}",
"User-Agent": DataSharingRestClient.USER_AGENT,
}
)

def auth_basic(self, profile):

This comment has been minimized.

Copy link
@zhuansunxt

zhuansunxt May 18, 2023

Isn't the basic auth about using the username + password to talk to the api endpoint directly? If the type is basic, there is no token_endpoint as part of the profile and there is no need to exchange an access token.

This comment has been minimized.

Copy link
@dialberg

dialberg May 23, 2023

Author Owner

Fixed.

response = requests.post(profile.token_endpoint,
data = {"grant_type": "client_credentials"},
auth = (profile.username, profile.password),)
bearer_token = "{}".format(response.json()["access_token"])

self._session.headers.update(
{
"Authorization": f"Bearer {bearer_token}",
"User-Agent": DataSharingRestClient.USER_AGENT,
}
)

@retry_with_exponential_backoff
def list_shares(
Expand Down

3 comments on commit 33bd3ba

@davidgreenfield
Copy link

Choose a reason for hiding this comment

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

This looks good to me and is accordance with the proposal as outlined here: delta-io#269

@zhuansunxt
Copy link

Choose a reason for hiding this comment

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

Hi @dialberg , I left some comments about the code change here.

@dialberg
Copy link
Owner Author

Choose a reason for hiding this comment

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

I have fixed the comments and added unit tests.

Please sign in to comment.