Skip to content

Commit

Permalink
[SYNPY-1436] avoid duplicate user profile call during login (#1124)
Browse files Browse the repository at this point in the history
* add diaplayname to SynapseAuthTokenCredentials
* simplify display_name creation
  • Loading branch information
danlu1 authored Aug 1, 2024
1 parent 52aa513 commit bc46199
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 24 deletions.
27 changes: 15 additions & 12 deletions synapseclient/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
The `Synapse` object encapsulates a connection to the Synapse service and is used for building projects, uploading and
retrieving data, and recording provenance of data analysis.
"""

import asyncio
import collections
import collections.abc
Expand Down Expand Up @@ -770,12 +771,7 @@ def login(
raise SynapseNoCredentialsError("No credentials provided.")

if not silent:
profile = self.getUserProfile()
display_name = (
profile["displayName"]
if "displayName" in profile
else self.credentials.username
)
display_name = self.credentials.displayname or self.credentials.username
self.logger.info(f"Welcome, {display_name}!\n")

if cache_client:
Expand Down Expand Up @@ -1958,9 +1954,14 @@ def store(
upload_file_handle_async(
self,
parent_id_for_upload,
local_state["path"]
if (synapseStore or local_state_fh.get("externalURL") is None)
else local_state_fh.get("externalURL"),
(
local_state["path"]
if (
synapseStore
or local_state_fh.get("externalURL") is None
)
else local_state_fh.get("externalURL")
),
synapse_store=synapseStore,
md5=local_file_md5_hex or local_state_fh.get("contentMd5"),
file_size=local_state_fh.get("contentSize"),
Expand Down Expand Up @@ -3130,9 +3131,11 @@ def _convertProvenanceList(self, usedList: list, limitSearch: str = None) -> lis
if usedList is None:
return None
usedList = [
self.get(target, limitSearch=limitSearch)
if (os.path.isfile(target) if isinstance(target, str) else False)
else target
(
self.get(target, limitSearch=limitSearch)
if (os.path.isfile(target) if isinstance(target, str) else False)
else target
)
for target in usedList
]
return usedList
Expand Down
21 changes: 19 additions & 2 deletions synapseclient/core/credentials/cred_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,14 @@ def _validate_token(cls, token):
# ValueError if the split string is not base64 encoded or if the decoded base64 is not json
pass

def __init__(self, token, username=None):
def __init__(
self, token: str, username: str = None, displayname: str = None
) -> None:
self._validate_token(token)

self._token = token
self.username = username
self.displayname = displayname

@property
def username(self) -> str:
Expand All @@ -76,6 +79,15 @@ def username(self) -> str:
def username(self, username: str) -> None:
self._username = username

@property
def displayname(self) -> str:
"""The displayname associated with this token."""
return self._displayname

@displayname.setter
def displayname(self, displayname: str) -> None:
self._displayname = displayname

@property
def secret(self) -> str:
"""The bearer token."""
Expand All @@ -86,7 +98,12 @@ def __call__(self, r):
return r

def __repr__(self):
return f"SynapseAuthTokenCredentials(username='{self.username}', token='{self.secret}')"
return (
f"SynapseAuthTokenCredentials("
f"username='{self.username}', "
f"displayname='{self.displayname}', "
f"token='{self.secret}')"
)


# a class that just contains args passed form synapse client login
Expand Down
4 changes: 3 additions & 1 deletion synapseclient/core/credentials/credential_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ def _create_synapse_credential(
profile = syn.restGET("/userProfile", auth=credentials)
profile_username = profile.get("userName")
profile_emails = profile.get("emails", [])
profile_displayname = profile.get("displayName")

if username and (
username != profile_username and username not in profile_emails
Expand All @@ -88,8 +89,9 @@ def _create_synapse_credential(
"username/email and auth_token both provided but username does not "
"match token profile"
)

credentials.username = profile_username
credentials.displayname = profile_displayname

return credentials

return None
Expand Down
18 changes: 15 additions & 3 deletions tests/unit/synapseclient/core/credentials/unit_test_cred_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ class TestSynapseAuthTokenCredentials:
def setup_method(self):
self.username = "ahhhhhhhhhhhhhh"
self.auth_token = "opensesame"
self.displayname = "hhhhhaaaa"
self.credentials = SynapseAuthTokenCredentials(
self.auth_token, username=self.username
self.auth_token, username=self.username, displayname=self.displayname
)
self.KEYRING_NAME = "SYNAPSE.ORG_CLIENT_AUTH_TOKEN"

Expand All @@ -26,6 +27,15 @@ def test_username_setter(self):
credentials.username = self.username
assert credentials.username is self.username

def test_displayname(self):
assert self.displayname == self.credentials.displayname

def test_displayname_setter(self):
credentials = SynapseAuthTokenCredentials(self.auth_token)
assert credentials.displayname is None
credentials.displayname = self.displayname
assert credentials.displayname is self.displayname

def test_secret(self):
assert self.credentials.secret == self.auth_token

Expand All @@ -44,8 +54,10 @@ def test_call(self):

def test_repr(self):
assert (
f"SynapseAuthTokenCredentials(username='{self.username}', token='{self.auth_token}')"
== repr(self.credentials)
f"SynapseAuthTokenCredentials("
f"username='{self.username}', "
f"displayname='{self.displayname}', "
f"token='{self.auth_token}')" == repr(self.credentials)
)

def test_tokens_validated(self, mocker):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ def test_get_credentials__multiple_providers(self) -> None:
cred_provider2.get_synapse_credentials.return_value = (
SynapseAuthTokenCredentials(
username="asdf",
displayname="aaaa",
token="ghjk",
)
)
Expand Down Expand Up @@ -174,13 +175,14 @@ def test_create_synapse_credential_username_not_None_auth_token_is_not_None(
assert creds is mock_creds

@pytest.mark.parametrize(
"login_username,profile_username,profile_emails",
"login_username,profile_username,profile_emails,profile_displayname",
(
("foo", "foo", ["[email protected]"]), # username matches
("foo", "foo", ["[email protected]"], "foo"), # username matches
(
"[email protected]",
"foo",
["[email protected]", "[email protected]", "[email protected]"],
"foo",
), # email matches
),
)
Expand All @@ -190,6 +192,7 @@ def test_create_synapse_credential__username_auth_token_match(
login_username,
profile_username,
profile_emails,
profile_displayname,
) -> None:
"""Verify that if both a username/email and a auth token are provided, the login is successful
if the token matches either the username or a profile email address."""
Expand All @@ -198,13 +201,15 @@ def test_create_synapse_credential__username_auth_token_match(
mock_rest_get.return_value = {
"userName": profile_username,
"emails": profile_emails,
"displayName": profile_displayname,
}

cred = self.provider._create_synapse_credential(
syn=self.syn, username=login_username, auth_token=self.auth_token
)
assert cred.secret == self.auth_token
assert cred.username == profile_username
assert cred.displayname == profile_displayname

def test_create_synapse_credential__username_auth_token_mismatch(
self, mocker
Expand All @@ -217,6 +222,7 @@ def test_create_synapse_credential__username_auth_token_mismatch(
mock_rest_get.return_value = {
"userName": "foo",
"emails": ["[email protected]", "[email protected]"],
"displayName": "foo",
}

with pytest.raises(SynapseAuthenticationError) as ex:
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/synapseclient/unit_test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@ def setup_method(self) -> None:
auth_token="hunter2", username="AzureDiamond"
)
self.synapse_creds = SynapseAuthTokenCredentials(
token="hunter2", username="AzureDiamond"
token="hunter2",
displayname="Azure Diamond",
username="AzureDiamond",
)

self.mocked_credential_chain = create_autospec(SynapseCredentialsProviderChain)
Expand Down Expand Up @@ -139,9 +141,7 @@ def test_login_credentials_returned(self) -> None:
assert self.synapse_creds == self.syn.credentials

def test_login_silent_is_false(self) -> None:
with patch.object(self.syn, "getUserProfile"), patch.object(
self.syn, "logger"
) as mocked_logger:
with patch.object(self.syn, "logger") as mocked_logger:
# method under test
self.syn.login(silent=False, **self.login_args)

Expand Down

0 comments on commit bc46199

Please sign in to comment.