Skip to content

acquire_token_by_refresh_token() for RT migration #193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 14, 2020

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented May 14, 2020

This PR implements the right-hand side of this comparison. We can go with this direction, and then this PR will close #191.

The API calling pattern is largely the same as #191, if we do not really need to use its return value:

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")
    # The new RT is saved inside MSAL's cache.
    # You could use the new AT, IDT from result at this point

There are still two higher level RT migration strategies:

  • One approach is to migrate RTs IN A BATCH within 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.

    if os.path.exists("adal_token_cache.bin"):
        for rt, scopes in get_legacy_rt(...):
            app.acquire_token_by_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,
        # i.e. acquire_token_silent(scopes)
    main_program(...)  # Main program starts

    With this strategy, you may safely discard the return value of each acquire_token_by_refresh_token() call, DURING the migration.

    The comparison: acquire_token_by_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 approach is to migrate RTs GRADUALLY across many days or even weeks, when the end user is using our app (such as Azure CLI) frequently. But then you would have to use acquire_token_by_refresh_token() in your normal code path. The following demo is copied from here.

    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(...)
        }
    }

@henrik-me
Copy link

this one gets my vote

def acquire_token_by_refresh_token(self, refresh_token, scopes):
"""Acquire token(s) based on a refresh token (RT) obtained from elsewhere.

In normal MSAL scenarios, you do NOT need to use this method.

Choose a reason for hiding this comment

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

In normal MSAL scenarios, you do NOT need to use this method. [](start = 8, length = 61)

https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/1cef43d185bb0a1dcc013d2ec232ae9d4c6c053a/src/client/Microsoft.Identity.Client/IByRefreshToken.cs:
/// Acquires an access token from an existing refresh token and stores it, and the refresh token, in
/// the user token cache, where it will be available for further AcquireTokenSilent calls.
/// This method can be used in migration to MSAL from ADAL v2, and in various integration
/// scenarios where you have a RefreshToken available.
/// See https://aka.ms/msal-net-migration-adal2-msal2.

You can also get inspiration on the text from the Java API. Should be similar.

Please add aka.ms links to and create a wiki page allowing you to explain this in more details as well as updating content and linking to it easily when customers have questions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We probably do not need to create yet another wiki page. After this PR is released, we can modify the existing doc accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After this PR is released, we can modify the existing doc accordingly.

UPDATE:
A new doc PR for the migration docs has been created just now.

@rayluo rayluo force-pushed the acquire_token_by_refresh_token branch from db912cd to 3917e41 Compare May 14, 2020 17:37
@rayluo
Copy link
Collaborator Author

rayluo commented May 14, 2020

Quoted a test feedback from @qianwens:

I have tried your PR and it works perfectly. I can use an ADAL refresh token to generate the MSAL token cache with your method and CLI command works well with the generated MSAL token.

Thanks very much for implementing this function in such a short time.

@rayluo rayluo force-pushed the acquire_token_by_refresh_token branch from 3917e41 to fb5beea Compare May 14, 2020 21:52
Copy link
Contributor

@abhidnya13 abhidnya13 left a comment

Choose a reason for hiding this comment

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

🚢

@rayluo rayluo force-pushed the acquire_token_by_refresh_token branch from fb5beea to e3fa226 Compare May 14, 2020 23:54
Remove distracting test setup from sample

Demonstrate the migration-in-batch pattern
@rayluo rayluo merged commit 652dcbf into dev May 14, 2020
@rayluo rayluo deleted the acquire_token_by_refresh_token branch May 14, 2020 23:59
@rayluo rayluo mentioned this pull request May 15, 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.

3 participants