Add public API for long-running OBO methods with custom cache key and do not use refresh token for normal OBO

[pmaytak]

Fixes #2733.

Changes proposed in this request

• New public API for long-running OBO
  • Adds InitiateLongRunningProcessInWebApi that allows to specify a cache key which will be used for searching for an OBO access and refresh tokens instead of a user assertion hash as usual.
  • Adds AcquireTokenInLongRunningProcess to retrieve the token from the cache with that key without providing a user assertion.
  • Created new interface ILongRunningWebApi.
• For normal OBO flow, removes refresh token from the token response.
  • Also optimization - if access token is not found, skips checking for refresh tokens.

Variations of assertions and keys for different OBO methods:

OBO method	User assertion	User provided cache key
InitiateLongRunningProcessInWebApi	Not null	Not null
AcquireTokenInLongRunningProcess	Null	Not null
AcquireTokenOnBehalfOf	Not null	Null

Logic flow:
InitiateLongRunningProcessInWebApi (scopes, assertion, key)

• AT is not found in cache by cache key - return null, look for RT
  • RT is found by cache key - Fetch new AT using existing RT and save with cache key
  • RT is not found by cache key - Fetch new AT with assertion and save with cache key
• AT is found in cache by cache key - return it

AcquireTokenInLongRunningProcess (scopes, key)

• AT is not found in cache by cache key - throw exception (key expected to exist)
• AT is found in cache by cache key
  • If expired - look for RT
    • RT is found by cache key - Fetch new AT using existing RT and save with cache key
    • RT is not found by cache key - throw exception (RT is expected to exist)
  • If AT is valid - return it

Testing
Manual tests. Added tests for new public API flows. Tests for when calling both, normal and long-running OBO methods. Tests that suggested cache expiry should work for normal OBO. Tests that normal OBO should not have a refresh token and should not use that flow.

Performance impact
Less memory used for normal OBO since we don't cache RT anymore.

[jmprieur]

@pmaytak, as you said, there is a risk that the API is miss used

I think we might want to keep AcquireTokenOnBehalfOf as is today, and add 2 new methods:

• InitiateLongRunningProcessInWebAPI(scopes, jwt, ref longRunningProcessSessionKey=null). This method enables the developer to initiate a long running process in a web API, by specifying the jwt used to call the web API, the scopes, and optionally a long running process session key. This is really a string that either the developer provides, or is otherwise returned by the method (and could be the jwt hash)
• AcquireTokenInLongRunningProcess(scopes, longRunningProcessSessionKey) which acquires the token.

Both methods could return AuthenticationResult.
We need to decide what happens if the first method is called with an existing key (probably throw), and if the second is called with an un-existant key (throw for sure)

Example of usage:

This method initiates the long running process.

         /// <summary>
        /// This methods the processing of user data where the web API periodically checks the user
        /// date (think of OneDrive producing albums)
        /// </summary>
        private void RegisterPeriodicCallbackForLongProcessing()
        {
            // Get the token incoming to the web API - we could do better here.
            string key = null;            
            cca.InitiateLongRunningProcessInWebAPI(scopes, jwt, ref key);

            // Build the URL to the callback controller, based on the request.
            var request = HttpContext.Request;
            string url = request.Scheme + "://" + request.Host + request.Path.Value.Replace("todolist", "callback") + $"?key={key}";

            // Setup a timer so that the API calls back the callback every 10 mins.
            Timer timer = new Timer(async (state) =>
            {
                HttpClient httpClient = new HttpClient();
                
                var message = await httpClient.GetAsync(url);
            }, null, 1000, 1000 * 60 * 1);  // Callback every minute
        }

And here is the callback controller

using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Logging;
using Microsoft.Identity.Web;
using Microsoft.Identity.Web.Resource;
using System;
using System.Collections.Generic;
using System.IdentityModel.Tokens.Jwt;
using System.Linq;
using System.Net.Http;
using System.Security.Claims;
using System.Threading;
using System.Threading.Tasks;
using TodoListService.Models;

namespace TodoListService.Controllers
{
    [Authorize]
    [Route("api/[controller]")]
    //[RequiredScope("access_as_user")] 
    public class CallbackController : Controller
    {
        private readonly IConfidentialClientApplication cca;    

/* constructor omitted */

        // GET: api/values
        // [RequiredScope("access_as_user")]
        [HttpGet]
        [AllowAnonymous]
        public async Task GetAsync(string key)
        {
                var result = await cca.AcquireTokenInLongRunningProcess(new string[] { "user.read" }, key).ConfigureAwait(false); 
            }
        }
    }
}

[pmaytak]

@jmprieur
I implemented the changes in commits dfb40f7 and e667465.

Tests are in OboRequestTests. (Test) for AcquireTokenInLongRunningProcess when key does not exist. Tests for InitiateLongRunningProcessInWebAPI when cache key exists.

Should we specifically mention OBO in the method names to make it more clear? Something like AcquireTokenForOboInLongRunningProcess and InitiateLongRunningProcessForOboInWebApialthough it does make names longer.

[bgavrilMS]

Shouldn't this throw an MsalUiRequiredException instead? CC @jmprieur for exception behavior.

[trwalke]

Im not sure it needs to, it may be that the developer simply hasnt supplied the already acquired userAssertion using InitiateLongRunningProcess right?

[trwalke]

Minor comment

[bgavrilMS]

LGTM, main comment is around integration test

[bgavrilMS]

@jmprieur - what should we suggest to ppl to use here? A web api should decrypt and parse the assertion, so they have access to the sid claim. I remember @hpsin suggesting to use this claim as key.

[bgavrilMS]

@pmaytak - let's add sid claim as the suggested key to use, and add a remark explaining that MSAL cannot read tokens on its own.

[hpsin]

@bgavrilMS is it correct to read this as "MSAL performs the OBO without validating who the token is for"? I'd suggest we vet this path for confused deputy attack surface.

[bgavrilMS]

@hpsin - MSAL doesn't look at incoming token at all. It's up to the web api to perform any validations. I believe Ms.Id.Web does some of that.

I may not understand the question, but allow me to elaborate.

Today, we advise people wanting to implement the "long running process OBO" scenario to keep around the incoming_assertion. This is to avoid the "confused deputy" attack, i.e. the AT / RT from OBO can only be retrieved if the original assertion is found. However, this is pretty complicated, and we end up asking folks to keep around an expired JWT. They mostly keep it around in their own cache as "username" -> "original_jwt".

AcquireTokenInLongRunningProcess(scopes, key) aims to fix that. It is a slightly more flexible version of AcquireTokenOnBehalfOf(scopes, incomming_assertion), to allow the web api to let go of the expired incomming_assertion and to remember only the sid. Internally, MSAL will locate the AT / RT in its caches by this key.

Shall we meet to discuss this in more detail?

[hpsin]

Thanks, I think that answers my question in general. Do we have any kind of ratchet in place that tries to ensure that the web api has performed validation?

[jmprieur]

@pmaytak - let's add sid claim as the suggested key to use, and add a remark explaining that MSAL cannot read tokens on its own.

We cannot crack-open the token, so the claim would have to be provided by the application to MSAL.NET. Also, sid is an optional claim, This requires an aka.ms link.

[trwalke]

Minor comments, Logic looks good overall

[trwalke]

one more thing, is there a test that validates that the normal OBO flow removes refresh token from the token response?

[pmaytak]

one more thing, is there a test that validates that the normal OBO flow removes refresh token from the token response?

This one tests that if AT is expired, OBO call does OBO flow and not RT flow.

microsoft-authentication-library-for-dotnet/tests/Microsoft.Identity.Test.Unit/RequestsTests/OboRequestTests.cs

Line 74 in b4e8fbd

mockTokenRequestHttpHandlerRefresh.ExpectedPostData = new Dictionary<string, string> { { OAuth2Parameter.GrantType, OAuth2GrantType.JwtBearer } };

This one tests that when AT is returned from cache, there are not RT tokens in cache.

microsoft-authentication-library-for-dotnet/tests/Microsoft.Identity.Test.Unit/RequestsTests/OboRequestTests.cs

Line 156 in b4e8fbd

Assert.AreEqual(0, cca.UserTokenCacheInternal.Accessor.GetAllRefreshTokens().Count);

In contrast, this tests that RT exists for long-running OBO.

microsoft-authentication-library-for-dotnet/tests/Microsoft.Identity.Test.Unit/RequestsTests/OboRequestTests.cs

Line 187 in b4e8fbd

MsalRefreshTokenCacheItem cachedRefreshToken = cca.UserTokenCacheInternal.Accessor.GetAllRefreshTokens().Single();

@trwalke Should I add some other tests?

[bgavrilMS]

Logic flow: InitiateLongRunningProcessInWebApi (scopes, assertion, key)

• AT is not found in cache by cache key - return null, look for RT

  • RT is found by cache key - Fetch new AT using existing RT and save with cache key
  • RT is not found by cache key - Fetch new AT with assertion and save with cache key

• AT is found in cache by cache key - throw exception (key is not expected to exist)

Should we really throw exceptions if Initiate finds an AT with the same key? Why not just override the existing token? Imagine this scenario:

• app developer decides the key is oid (this is the case of some services today!)
• service uses L2 caching
• machine 1 calls Initiate with key oid1
• machine 2 also calls initiate with key oid1 EXCEPTION

CC @jmprieur for opinion here.

[pmaytak]

Logic flow: InitiateLongRunningProcessInWebApi (scopes, assertion, key)

• AT is not found in cache by cache key - return null, look for RT

  • RT is found by cache key - Fetch new AT using existing RT and save with cache key
  • RT is not found by cache key - Fetch new AT with assertion and save with cache key

• AT is found in cache by cache key - throw exception (key is not expected to exist)

Should we really throw exceptions if Initiate finds an AT with the same key? Why not just override the existing token? Imagine this scenario:

• app developer decides the key is oid (this is the case of some services today!)
• service uses L2 caching
• machine 1 calls Initiate with key oid1
• machine 2 also calls initiate with key oid1 EXCEPTION

CC @jmprieur for opinion here.

@bgavrilMS Removed that exception in 627b4d9, returns the token now.

[jmprieur]

I think we need to improve a few XLM comments, otherwise LGTM

tested with Identity.Web and an id.web dev sample: AzureAD/microsoft-identity-web#1523