Update the framename to reflect authority and scopes

[sameerag]

This PR fixes #1225 by making the frame name unique for scopes+authority when making silent calls

[coveralls]

Coverage increased (+0.01%) to 79.503% when pulling 7277a7f on unique-framename into 9274fac on dev.

[keystroke]

@jasonnutter @sameerag thanks for looking into this! I wonder, should the frame name / identifier also include a moniker for "silent" or "popup" so otherwise-duplicate token requests don't collide for these different operations? Or silent vs. popup is disambiguated via some other mechanism?

[keystroke]

@sameerag I attempted to hack your changes into the MSAL source (code shown below) using the non-minified version.

I see that the target function is only invoked a single time during the concurrent requests.

/**
 * @hidden
 * Acquires access token using a hidden iframe.
 * @ignore
 */
UserAgentApplication.prototype.renewToken = function (scopes, resolve, reject, account, serverAuthenticationRequest) {
    var scope = scopes.join(" ").toLowerCase();
    this.logger.verbose("renewToken is called for scope:" + scope);
    var frameName = "msalRenewFrame" + scope + serverAuthenticationRequest.authority;
    console.log("Using frame name: " + frameName, serverAuthenticationRequest);
    var frameHandle = WindowUtils_1.WindowUtils.addHiddenIFrame(frameName, this.logger);
    this.updateCacheEntries(serverAuthenticationRequest, account);
    this.logger.verbose("Renew token Expected state: " + serverAuthenticationRequest.state);
    // Build urlNavigate with "prompt=none" and navigate to URL in hidden iFrame
    var urlNavigate = UrlUtils_1.UrlUtils.urlRemoveQueryStringParameter(UrlUtils_1.UrlUtils.createNavigateUrl(serverAuthenticationRequest), Constants_1.Constants.prompt) + Constants_1.Constants.prompt_none + Constants_1.Constants.response_mode_fragment;
    window.renewStates.push(serverAuthenticationRequest.state);
    window.requestType = Constants_1.Constants.renewToken;
    this.registerCallback(serverAuthenticationRequest.state, scope, resolve, reject);
    this.logger.infoPii("Navigate to:" + urlNavigate);
    frameHandle.src = "about:blank";
    this.loadIframeTimeout(urlNavigate, frameName, scope).catch(function (error) { return reject(error); });
};

I made the similair change to the other function ("renewIdToken") as shown above. When I run my sample again using the concurrent discovery flow, I see that above function is only called once, despite calling acquire token silent 10 times with different authority values:

However, when I included my "disambiguation" code from the sample to force unique scope strings for each token request, I do see that this function is called for each set of scopes:

function beginTenantDiscoveryConcurrent(loginToken) {
    console.log('Beginning tenant discovery concurrent:', loginToken);
    for (let i = 1; i <= 9; i++) {
        let tenant = 'azurestackci0' + i + '.onmicrosoft.com';
        let disambiguation = '';
        for (let j = 0; j < i; j++) {
            disambiguation += ' openid'
        }
        let scopes = ['openid', disambiguation];
        msal.acquireTokenSilent({
            forceRefresh: true,
            scopes: scopes, //['openid'],
            authority: 'https://login.microsoftonline.com/' + tenant
        }).then(function (token) {
            console.log('Tenant ' + tenant + '(fromCache: ' + token.fromCache + '):', getAccessTokenClaims(token).tid);
        }).catch(function (error) {
            console.log('Tenant ' + tenant + ':', error.message);
        });
    }
}

If I dig into the code of acquire token silent a bit more, I see this code which looks like it is caching windows requests only by scope here, and not including authority (or popup vs silent related to what I was discussing with @jasonnutter):

Please note I'm not entirely certain in above investigation, but it does seem to be the issue. Please let me know if I've missed something.

Thanks again for looking into this!

[keystroke]

@sameerag I further hacked the code to replace all references of:

window.activeRenewals[scope]

to:

window.activeRenewals[scope + authority]

I can verify that this change works with my specific sample, however there are some further design issues.

First, the signature and context of this function does not have any authority information provided to it, so there was no way for me to even "hack" the code to include the correct authority; instead the function signature would need to be modified to have authority context:

/**
 * @hidden
 * Used to add the developer requested callback to the array of callbacks for the specified scopes. The updated array is stored on the window object
 * @param {string} expectedState - Unique state identifier (guid).
 * @param {string} scope - Developer requested permissions. Not all scopes are guaranteed to be included in the access token returned.
 * @param {Function} resolve - The resolve function of the promise object.
 * @param {Function} reject - The reject function of the promise object.
 * @ignore
 */
UserAgentApplication.prototype.registerCallback = function (expectedState, scope, resolve, reject) {
    var _this = this;
    // track active renewals
    window.activeRenewals[scope] = expectedState;

Second, I found this function that did have enough context for me to hack the code to include an authority reference, though of course a better design might always pass scope and authority around everywhere. Honestly, scopes are meaningless without the associated authority in general, so probably through the entire code base you want to associate the two concepts.

/**
 * @hidden
 * Calling _loadFrame but with a timeout to signal failure in loadframeStatus. Callbacks are left.
 * registered when network errors occur and subsequent token requests for same resource are registered to the pending request.
 * @ignore
 */
UserAgentApplication.prototype.loadIframeTimeout = function (urlNavigate, frameName, scope) {
    return tslib_1.__awaiter(this, void 0, Promise, function () {
        var expectedState, iframe, hash, error_2;
        return tslib_1.__generator(this, function (_a) {
            switch (_a.label) {
                case 0:
                    let hackedAuthority = frameName.substr(frameName.indexOf('https:'));
                    expectedState = window.activeRenewals[scope + hackedAuthority];

Those were the only references in the code I found to "window.activeRenewals" so it is promising. In fact, the problematic method above (registerCallback) is called 4 times in the code base, and in each call, it has a handle to "serverAuthenticationRequest" which has the authority parameter! So instead of just passing the expected state to that function, it could pass the authority as well, though I think for a cleaner code you might consider just passing the entire "serverAuthenticationRequest" object to this function?

IDK, I'll let you folks take a look. But pretty sure these are the necessary fixes.

[keystroke]

I have submitted comments in the conversation tab after testing and investigating code changes. Thanks!

[sameerag]

@keystroke you could be right as our renewal code blocks concurrency if the request is made for the same scopes, and that could be executed before creating an iframe. I will check this more closely and make changes accordingly.

[jasonnutter]

If just changing the frame name to include authority works and provides value on its own, I would like to scope this PR to just include that change. Any further improvements can be made incrementally.

[sameerag]

I am scoping this PR to name change only and will bring up a new PR to address the concurrency check accounting for authority. @jasonnutter and @keystroke, can you help with the reviews, I have addressed the feedback.

[jasonnutter]

Looks good, thanks for the test!

[keystroke]

@jasonnutter @sameerag I understand you are trying to scope this PR to the first half of a potential solution, however I'm not sure of the value of that, can you take a look at the changes in #1279 to consider a more complete / direct approach?

[jmckennon]

Looks good to me!

[keystroke]

This change does not fix the issue without additional changes, and those additional changes may "invalidate" this change, please see PR #1279 to consider additional modifications to some function signatures, including the introduction of a "request fingerprint" that ought to be used in a few places where only scopes are passed or "frame name" is passed.

[jasonnutter]

@keystroke Gotya. @sameerag and I will chat and follow up. Thanks!

[jasonnutter]

Updates look good!

[jasonnutter]

@sameerag Is this ready to be merged?