Client.obtain_token_by_refresh_token fails when token_item is a str

[jiasli]

Describe the bug
Even though Client.obtain_token_by_refresh_token checks the type of token_item at

microsoft-authentication-library-for-python/msal/oauth2cli/oauth2.py

Lines 801 to 802 in 381fe58

	rt_getter(token_item)
	if not isinstance(token_item, string_types) else token_item,

and calls rt_getter accordingly, it doesn't check the type of token_item at

microsoft-authentication-library-for-python/msal/oauth2cli/oauth2.py

Lines 809 to 810 in 381fe58

	if on_updating_rt is not False and RT in resp:
	(on_updating_rt or self.on_updating_rt)(token_item, resp[RT])

to make sure token_item is converted to a dict before calling self.on_updating_rt.

This causes update_rt to fail at L248:

microsoft-authentication-library-for-python/msal/token_cache.py

Lines 247 to 248 in 0845404

	def update_rt(self, rt_item, new_rt):
	assert rt_item.get("credential_type") == self.CredentialType.REFRESH_TOKEN

To Reproduce
Pass a refresh token in str as token_item to Client.obtain_token_by_refresh_token.

Expected behavior
Either:

• Client.obtain_token_by_refresh_token should succeed with token_item as a str either by
  • converting token_item to a dict before calling on_updating_rt
  • forcing on_updating_rt=False
• Client.obtain_token_by_refresh_token should reject token_item as a str

What you see instead

  File "D:\cli\microsoft-authentication-library-for-python\msal\application.py", line 714, in acquire_token_silent
    result = self.acquire_token_silent_with_error(
  File "D:\cli\microsoft-authentication-library-for-python\msal\application.py", line 767, in acquire_token_silent_with_error
    result = self._acquire_token_silent_from_cache_and_possibly_refresh_it(
  File "D:\cli\microsoft-authentication-library-for-python\msal\application.py", line 840, in _acquire_token_silent_from_cache_and_possibly_refresh_it
    return self._acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family(
  File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\_msal.py", line 17, in _acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family
    return self._acquire_token_silent_by_finding_specific_refresh_token(
  File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\_msal.py", line 28, in _acquire_token_silent_by_finding_specific_refresh_token
    response = client.obtain_token_by_refresh_token(
  File "D:\cli\microsoft-authentication-library-for-python\msal\oauth2cli\oauth2.py", line 810, in obtain_token_by_refresh_token
    (on_updating_rt or self.on_updating_rt)(token_item, resp[RT])
  File "D:\cli\microsoft-authentication-library-for-python\msal\token_cache.py", line 248, in update_rt
    assert rt_item.get("credential_type") == self.CredentialType.REFRESH_TOKEN
AttributeError: 'str' object has no attribute 'get'

The MSAL Python version you are using
1.8.0

Additional context
This issue is revealed by Azure/azure-cli#16672 (comment).

[rayluo]

Jiashuo Li (@jiasli) , thanks for your high quality report. You clearly did your homework! That helps a lot! After examination, I think: (1) this is not a bug inside MSAL, but rather (2) a misusage of Client.obtain_token_by_refresh_token() (or its public API ClientApplication.acquire_token_by_refresh_token()). Let me explain.

1. First of all, Client.obtain_token_by_refresh_token() is an internal API. Its incoming parameter token_item is designed to be either a string or a flexible structure (so that also means one of the proposed workaround "Client.obtain_token_by_refresh_token should reject token_item as a str" would conflict the original design).

   And, why we do not need the string check for the callback on_updating_rt()? That is because that callback is also an input parameter, so, it is the caller's responsibility to ensure the token_item they fed in, would also be accepted by the on_updating_rt() they fed in. The current implementation does not hardcode a type check, thus provide maximal flexibility to allow the caller to provide a pair of RT and a SUITABLE on_updating_rt(), as long as they match each other.

2. For example, this is one of the right usage pattern. The public API level ClientApplication.acquire_token_by_refresh_token() chooses to define an RT as a str, so it also uses a proper on_updating_rt. It is somewhat tricky, but then again, that was why we wrapped those details inside a public API acquire_token_by_refresh_token() which was designed to handle raw RT migration scenario (in other words, this API assumes the incoming RT is a one-time migration, so you won't need an on_updating_rt() behavior for it).

   Now my question to you is, under what scenario you would have that "To Reproduce: Pass a refresh token in str as token_item to Client.obtain_token_by_refresh_token"? Is it triggered by using public API acquire_token_by_refresh_token() for ssh login?

[jiasli]

Thanks Ray Luo (@rayluo) for the detailed explanation.

The required compatibility between the provided RT in token_item and on_updating_rt answers the initial concern.

Now my question to you is, under what scenario you would have that "To Reproduce: Pass a refresh token in str as token_item to Client.obtain_token_by_refresh_token"? Is it triggered by using public API acquire_token_by_refresh_token() for ssh login?

Yes, exactly. As ClientApplication.acquire_token_by_refresh_token doesn't expose kwargs, I have to pass RT in str as token_item to Client.obtain_token_by_refresh_token. Once ClientApplication.acquire_token_by_refresh_token (which is of course the better choice) supports passing kwargs, I wouldn't need to call Client.obtain_token_by_refresh_token directly and thus not hitting this "issue" anymore.

[rayluo]

OK then, let's close this Q&A here, and shift our subsequent discussion on the other issue.

[jiasli]

By the way, this is exactly why I find the idea of overriding MSAL internal methods problematic, as it may be difficult to follow MSAL's internal intricacy and adapt to MSAL's changes.