Fix: Creating new Loop pages after SharePoint domain change

[Philip-Scott]

Description

After a tenant changes their domain, this lead existing workspaces on the Loop app to stop allowing the creation of new pages. After a redirect, the authorization headers are not sent by the browser. And even if were to just retry with the headers set, SharePoint requires for the auth tokens to be generated for that particular domain.

After these changes, I was able to get new pages to attach again. What I did was catch the redirect on the fetchHelper( function, and allow getFileLink to handle the redirect for us. For this to work, we need to request new tokens for the new domain.

The fix is sadly not perfect as we need to go through the following flow in order to detect and retry the redirect error.

• The first call to getShareURL against the original URL gives us the 308
• The second one that gets made automatically by the browser to follow the 308 will be made against the new domain, but will fail due to tokens
• We need to catch that second one, and retry with new tokens. This call will succeed

[msfluid-bot]

Baseline CI build failed, cannot generate bundle analysis at this time

---

Baseline commit: 4c94050

Generated by 🚫 dangerJS against bfd5f75

[agarwal-navin]

@vladsud @pragya91 Can you guys please take a look at this PR? @jatgarg is not available right now and you both have the most context here.

[pragya91]

Let's add a test for these new changes in getFileLink.spec.ts. You can mock throwing of the 403 error with redirected property in the response and see if the code works as expected.

[pragya91]

What is the reason for this loop? My understanding from the changes in the code here is that we only want to perform a retry once after we get the 403 error. If this is true, can we structure the code such that we perform retry within the catch block?

let { url, requestInit } = await getRequestInformation...
try {
    response = await fetchHelper();
}catch(e){
    // 403 error occured and we need to retry with correct headers here
    if(error.redirectLocation){
        { url, requestInit } = = await getRequestInformation...
         response = await fetchHelper();
    }
}
return response;

[Philip-Scott]

Redirects can actually be chained together! One redirect may point to another redirect and so on.

This was actually a pattern taken from packages/loader/container-loader/src/location-redirection-utilities/resolveWithLocationRedirection.ts

[pragya91]

Please add comments to explain this logic.

[pragya91]

Checking for fileNotFoundOrAccessDeniedError error type seems unrelated to this change.

[Philip-Scott]

That is actually the error type we get when throwing a OdspRedirectError that I myself throw on odspErrorUtils.ts.

I'm checking for both that error that I throw, as well as that there is a redirect URL available before doing any redirect logic

[vladsud]

Consider doing the following:

class OdspRedirectError {
	public static errorType = DriverErrorTypes.fileNotFoundOrAccessDeniedError;
	readonly errorType = OdspRedirectError.type;
...
}

if (error?.errorType !== OdspRedirectError .Type || !error?.redirectLocation)  {
    throw error;
}

or even better - way more readable.

class OdspRedirectError {
public static errorType = DriverErrorTypes.fileNotFoundOrAccessDeniedError;
readonly errorType = OdspRedirectError.type;

    static public isRedirectError(error: unknown) {
         return error?.errorType  === OdspRedirectError.errorType && !!error?.redirect;
    }

...
}

if (!OdspRedirectError(error)) {
throw error;
}
...
}

Notice that error does not have to be object (i.e. it can be undefined, null, etc. - we never know)

[pragya91]

Changing input parameters of a function is not the best programming practice. (Although I do see it in some parts of the existing code already, but let's avoid adding any new ones.) If possible, always return the new value to the caller, and let the caller assign the value as it may.
(Might be irrelevant if we changed the code here to what I suggested above)

[pragya91]

Recommend adding code docs to explain the purpose of this function.

[pragya91]

@Philip-Scott As far as I remember the GetSharingInformation is not a blocking call for attach functionality to go through. However, this api is called when host tries to request the getAbsoluteUrl() on the resolver. Just wanted to highlight that, since the PR talks about the "unable to attach" issue.

[vladsud]

Should we assert that response?.redirected => 401 or 403? I.e. that we are not missing cases?

[vladsud]

Does 404 / @error.redirectLocation case also have redirect property on response?

[Philip-Scott]

I have not seen a 404 redirect error for me to test that this is valid and works. Do you know how one could trigger a 404 redirection error?

[vladsud]

Well, the case below (for 404) has code that deals with redirects. I do not know much about how redirects work (i.e. what error codes can be returned) - @jatgarg & @marcmasmsft are rigth people to consult here.

[jatgarg]

Yes 404 could be triggered due to domain change.

[vladsud]

Every place that calls storageTokenFetcher() asserts that non-null result is returned. Given that's a wrapper produced by driver, can we at least move this assert into toInstrumentedOdspTokenFetcher() itself?
Or better, TokenFetcher type and force callers to change / assert? That's a bit bigger change, but having null on the type does not help.

[Philip-Scott]

Mmmm that is actually part of the original code before moving it out to its own function. We can probably follow that up on a separate PR to keep this one small, is that alright?

[vladsud]

raised #20409 to fix it

[vladsud]

Please add comments with arg names where it's not very clear what is the arg semantics

true, // forceAccessTokenViaAuthorizationHeader

[christiango]

Looks like this is now being handled with #21277