Skip to content

Conversation

@rayluo
Copy link
Contributor

@rayluo rayluo commented May 12, 2020

This PR proposes an experimental API for migrating refresh token (RT) from other source (typically ADAL Python, but not necessarily only ADAL Python) to MSAL Python.

TL;DR: This PR proposes an import_refresh_token(old_rt_string, scope_list) as first-class citizen (i.e. not hiding from main API). As hinted by its name, its successful return value contains no RT, not even AT, not currently return an ID token (this one is TBD).

app = msal.PublicClientApplication(
    "client_id", authority="https://login.microsoftonline.com/...", token_cache=...)

# NEW API
result = app.import_refresh_token(old_rt, ["scope1", "scope2"])  # <-- NEW API
if "error" in result:
    print("Migration unsuccessful. Error: ", json.dumps(result, indent=2))
else:
    print("Migration is successful")

# From now on, the RT is saved inside MSAL's cache,
# and becomes available in normal MSAL coding pattern. For example:
...
result = app.acquire_token_silent(["scope1"], ...)

So that there will be no way to (ab)use such migration API alone as a normal flow. The programming model would be to complete the RT migration upfront, and then come back to the normal MSAL coding model.

Reviewers can focus on its usage pattern (highlighted at the bottom of this page), rather than its actual implementation.

Background:

  1. To simplify token migration, we do not need to migrate existing access token (AT). We just need to migrate RTs. Because: (1) ATs are short-lived, old ATs might already been expired when the migration happens. (2) Once we have RTs migrated, they will be automatically used to obtain new ATs.
  2. To migrate RTs, a shortcut is to use the old RT to redeem an AT, during such process, a new RT will typically also return, and then MSAL's existing token cache mechanism will kick in, and store the new RT and AT properly. This means to call an imaginary API roughly named as acquire_token_by_refresh_token(...).
  3. However, historically, ADAL/MSAL has been designed to downplay the RT concept.
    • Many MSALs do not provide an acquire_token_by_refresh_token(rt, scope, ...) API. Instead, MSALs provide a higher level API acquire_token_silent(scope, ...) which uses RT from internal cache. For example, MSAL .Net only provides AcquireTokenByRefreshToken(...) in an indirect way:

      As this method is intended for scenarios that are not typical, it is not readily accessible with the IConfidentialClientApplication without first casting it to IByRefreshToken.
      You will see an access token and ID token returned in your AuthenticationResult while your new refresh token is stored in the cache.

      But, even so, an AcquireTokenByRefreshToken() is arguably still provided and documented, and it can be used to acquire AT.

    • MSAL Java provides AcquireTokenByRefreshToken(...) out-of-the-box. It returns no RT, either, but it returns AT, etc..

@bgavrilMS
Copy link
Member

There is a similar feature in MSAL.NET that @trwalke implemented - see https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/Adal-to-Msal#adalnet-v2x-migration-to-msalnet-v3x

@rayluo
Copy link
Contributor Author

rayluo commented May 12, 2020

A comparison between 2 flavors of API:

PR #191: If we use a dedicated API which does NOT return an AT PR #193: If we use an acquire_token_by_refresh_token() which returns an AT
app = msal.PublicClientApplication(
    "client_id", ...)

# Proposal A
result = app.import_refresh_token(
    old_rt, ["scope1", "scope2"])
if "error" in result:
    print(
        "Error in Migration:",
        json.dumps(result))
else:
    print("Migration is successful")

# From now on,
# the RT is saved inside MSAL's cache,
# and becomes available ONLY in normal
# MSAL coding pattern. For example:
...
result = app.acquire_token_silent(
    ["scope1"], ...)
use_the_token(result["access_token"])
app = msal.PublicClientApplication(
    "client_id", ...)

# Proposal B
result = app.acquire_token_by_refresh_token(
    old_rt, ["scope1", "scope2"])
if "error" in result:
    print(
        "Error in Migration:",
        json.dumps(result))
else:
    print("Migration is successful")

# From now on,
# the RT is saved inside MSAL's cache.

# But at the same time,
# the app could skip the normal
# MSAL coding pattern. For example:
use_the_token(result["access_token"])

@bgavrilMS
Copy link
Member

bgavrilMS commented May 13, 2020

I believe the idea with acquire_token_by_refresh_token is that it can be used in a similar style as other acquire_token_* methods.

try:
       # if this works, migration is not needed or it already happened
       acquire_token_silent 
except UIRequiredException:
       # migration path ... can be removed after some time
       rt = get_legacy_refresh_token
       acquire_token_by_refersh_token 
except UIRequiredException:
      # no RT available, get a new one
      acquire_token_by_X

What are your thoughts on full scenario using import_refresh_token ?

CC @jmprieur who designed the API originally

@rayluo
Copy link
Contributor Author

rayluo commented May 13, 2020

There has been some offline discussion since sending out this PR. We will end up choosing one solution soon. But here in this comment, for the sake of answering @bgavrilMS 's question above, and leaving a trace here for future reference:

  • The formula: import_refresh_token() + acquire_token_silent() = MSAL .Net's it is not readily accessible with the IConfidentialClientApplication without first casting it to IByRefreshToken. + acquire_token_by_refresh_token()

    • MSAL .Net's "needing to first cast it to a different interface" is an implicit yet required step, which effectively becomes part of that acquire_token_by_refresh_token() API calling pattern. A perception of "there is only one AcquireTokenByRefreshToken() API needed to be called", is not the full picture.

    • In Python language, there is technical limitation to do magic as "to cast an object as another", so one reason of the import_refresh_token() idea is to satisfy that original desire to hide the AcquireTokenByRefreshToken() behind a cast. BTW, MSAL C++ happens to come up with a migration function with exact same name void ImportRefreshToken().

    • That being said, I'm also told that "hiding AcquireTokenByRefreshToken() behind a casting" is not a hard requirement, this time.

  • Regardless of which flavor of API we end up using (because they ARE equivalent - see the "formula" above), an MSAL-powered app can still have freedom to choose their own two migration strategies.

    • One approach is to migrate RTs IN A BATCH within first few minutes, when a newer version of app (Azure CLI) is run for its first time on an end user's machine and detects a legacy RT persistence.

      for rt, scopes in get_legacy_rt(...):
          app.import_refresh_token(rt, scopes)
      os.rename("adal_token_cache.bin", "legacy_cache_to_be_deleted.bin")
      # Migration completed. From now on, everything follows the normal MSAL way

      With this strategy, you do NOT even need to call an acquire_token_silent() immediately DURING the migration.

      The revised formula: import_refresh_token()MSAL .Net's it is not readily accessible with the IConfidentialClientApplication without first casting it to IByRefreshToken. + acquire_token_by_refresh_token()

    • Another is to migrate RTs GRADUALLY across many days or even weeks, when the end user is using our app (such as Azure CLI) daily. That pattern is roughly demonstrated in C# (derived from @bgavrilMS 's question above, modified with nested try...catch...).

      try
      {
             // if this works, migration is not needed or it already happened
             AcquireTokenSilent (...)
      }
      catch UIRequiredException
      {
          try
          {
                 // migration path ... can be removed after some time
                 var rt = GetLegacyRefreshToken(...)
                 AcquireTokenByRefershToken(...)
          }
          catch UIRequiredException
          {
                // no RT available, get a new one
                AcquireTokenByXyz(...)
          }
      }

@rayluo rayluo changed the title A built-in API for RT migration import_refresh_token() for RT migration May 14, 2020
@rayluo rayluo changed the title import_refresh_token() for RT migration import_refresh_token() for RT migration May 14, 2020
@abhidnya13
Copy link
Contributor

Put my review comments on the other PR 193 to avoid duplication of comments

@henrik-me
Copy link

With this strategy, you do NOT even need to call an acquire_token_silent() immediately DURING the migration.

And the way this is done in Java and .NET you don't need to get the result and do anything with it nor do you need to call acquire_token_silent if you needed to use the result right away.

acquire_token_by_refresh_token() gets my vote

@rayluo
Copy link
Contributor Author

rayluo commented May 14, 2020

With this strategy, you do NOT even need to call an acquire_token_silent() immediately DURING the migration.

And the way this is done in Java and .NET you don't need to get the result and do anything with it nor do you need to call acquire_token_silent if you needed to use the result right away.

This is true. Regardless of whether we implement import_refresh_token() or acquire_token_by_refresh_token(), migrating-in-a-batch seems a better migration strategy. Interestingly, import_refresh_token()'s returning-nothing behavior will presumably lead app developer to the migrate-in-batch route; while on the contrary, acquire_token_by_refresh_token()'s returning-AT behavior could mislead app developer to Migrate-Gradually route.

acquire_token_by_refresh_token() gets my vote

Yep. #193 implements acquire_token_by_refresh_token().

@rayluo rayluo closed this in #193 May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants