Skip to content

Commit

Permalink
Remove hardcoded profile scope
Browse files Browse the repository at this point in the history
  • Loading branch information
rayluo committed May 18, 2021
1 parent ba5e90b commit 06f2abb
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 10 deletions.
15 changes: 14 additions & 1 deletion msal/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,13 @@ def get_accounts(self, username=None):
lowercase_username = username.lower()
accounts = [a for a in accounts
if a["username"].lower() == lowercase_username]
if not accounts:
logger.warning((
"get_accounts(username='{}') finds no account. "
"If tokens were acquired without 'profile' scope, "
"they would contain no username for filtering. "
"Consider calling get_accounts(username=None) instead."
).format(username))
# Does not further filter by existing RTs here. It probably won't matter.
# Because in most cases Accounts and RTs co-exist.
# Even in the rare case when an RT is revoked and then removed,
Expand Down Expand Up @@ -1251,7 +1258,13 @@ def _acquire_token_by_username_password_federated(
self.client.grant_assertion_encoders.setdefault( # Register a non-standard type
grant_type, self.client.encode_saml_assertion)
return self.client.obtain_token_by_assertion(
wstrust_result["token"], grant_type, scope=scopes, **kwargs)
wstrust_result["token"], grant_type, scope=scopes,
on_obtaining_tokens=lambda event: self.token_cache.add(dict(
event,
environment=self.authority.instance,
username=username, # Useful in case IDT contains no such info
)),
**kwargs)


class PublicClientApplication(ClientApplication): # browser app or mobile app
Expand Down
8 changes: 6 additions & 2 deletions msal/token_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,13 @@ def wipe(dictionary, sensitive_fields): # Masks sensitive info
if sensitive in dictionary:
dictionary[sensitive] = "********"
wipe(event.get("data", {}),
("password", "client_secret", "refresh_token", "assertion", "username"))
("password", "client_secret", "refresh_token", "assertion"))
try:
return self.__add(event, now=now)
finally:
wipe(event.get("response", {}), ("access_token", "refresh_token"))
wipe(event.get("response", {}), ( # These claims were useful during __add()
"access_token", "refresh_token", "username"))
wipe(event, ["username"]) # Needed for federated ROPC
logger.debug("event=%s", json.dumps(
# We examined and concluded that this log won't have Log Injection risk,
# because the event payload is already in JSON so CR/LF will be escaped.
Expand Down Expand Up @@ -184,6 +186,8 @@ def __add(self, event, now=None):
"oid", id_token_claims.get("sub")),
"username": id_token_claims.get("preferred_username") # AAD
or id_token_claims.get("upn") # ADFS 2019
or data.get("username") # Falls back to ROPC username
or event.get("username") # Falls back to Federated ROPC username
or "", # The schema does not like null
"authority_type":
self.AuthorityType.ADFS if realm == "adfs"
Expand Down
15 changes: 8 additions & 7 deletions tests/test_e2e.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,9 @@ def _test_username_password(self,
result = self.app.acquire_token_by_username_password(
username, password, scopes=scope)
self.assertLoosely(result)
# self.assertEqual(None, result.get("error"), str(result))
self.assertCacheWorksForUser(
result, scope,
username=username if ".b2clogin.com" not in authority else None,
username=username, # Our implementation works even when "profile" scope was not requested, or when profile claims is unavailable in B2C
)

def _test_device_flow(
Expand Down Expand Up @@ -554,11 +553,13 @@ def _test_acquire_token_obo(self, config_pca, config_cca):
# Assuming you already did that (which is not shown in this test case),
# the following part shows one of the ways to obtain an AT from cache.
username = cca_result.get("id_token_claims", {}).get("preferred_username")
self.assertEqual(config_cca["username"], username)
if username: # A precaution so that we won't use other user's token
account = cca.get_accounts(username=username)[0]
result = cca.acquire_token_silent(config_cca["scope"], account)
self.assertEqual(cca_result["access_token"], result["access_token"])
if username: # It means CCA have requested an IDT w/ "profile" scope
self.assertEqual(config_cca["username"], username)
accounts = cca.get_accounts(username=username)
assert len(accounts) == 1, "App is expected to partition token cache per user"
account = accounts[0]
result = cca.acquire_token_silent(config_cca["scope"], account)
self.assertEqual(cca_result["access_token"], result["access_token"])

def _test_acquire_token_by_client_secret(
self, client_id=None, client_secret=None, authority=None, scope=None,
Expand Down

0 comments on commit 06f2abb

Please sign in to comment.