Skip to content
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.

Sign up for GitHub

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

Fix: Creating new Loop pages after SharePoint domain change #19443

Closed
wants to merge 13 commits into from
Closed

Fix: Creating new Loop pages after SharePoint domain change #19443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6c6dd6f
Fix file link after redirect
Philip-Scott Feb 1, 2024
0123021
Prettier fix
Philip-Scott Feb 1, 2024
496fdfd
Revert eslint ignore
Philip-Scott Feb 1, 2024
8c9f544
Merge branch 'main' into philip-scott/fix-get-file-line-after-redirect
Philip-Scott Feb 1, 2024
73700ba
Throw the error and mimic resolveWithLocationRedirectionHandling
Philip-Scott Feb 9, 2024
b688feb
Merge remote-tracking branch 'refs/remotes/origin/philip-scott/fix-ge…
Philip-Scott Feb 9, 2024
eeac31d
prettier:fix
Philip-Scott Feb 9, 2024
cc50bab
Remove that ugly hack
Philip-Scott Feb 9, 2024
fec84e8
Merge branch 'main' into philip-scott/fix-get-file-line-after-redirect
Philip-Scott Feb 9, 2024
ab6ccca
Use OdspRedirectError instead
Philip-Scott Feb 12, 2024
dcea3ec
Merge branch 'philip-scott/fix-get-file-line-after-redirect' of https…
Philip-Scott Feb 12, 2024
d2816ab
Remove extra space
Philip-Scott Feb 12, 2024
bfd5f75
Merge branch 'main' into philip-scott/fix-get-file-line-after-redirect
Philip-Scott Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 89 additions & 33 deletions packages/drivers/odsp-driver/src/getFileLink.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@ import {
OdspResourceTokenFetchOptions,
TokenFetcher,
} from "@fluidframework/odsp-driver-definitions";
import { DriverErrorTypes } from "@fluidframework/driver-definitions";
import { getUrlAndHeadersWithAuth } from "./getUrlAndHeadersWithAuth";
import {
IOdspResponse,
TokenFetchOptionsEx,
fetchHelper,
getWithRetryForTokenRefresh,
toInstrumentedOdspTokenFetcher,
Expand Down Expand Up @@ -90,6 +93,86 @@ export async function getFileLink(
return fileLink;
}

async function getRequestInformation(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

getToken: TokenFetcher<OdspResourceTokenFetchOptions>,
odspUrlParts: IOdspUrlParts,
logger: ITelemetryLoggerExt,
options: TokenFetchOptionsEx,
fileItem: FileItemLite,
) {
const storageTokenFetcher = toInstrumentedOdspTokenFetcher(
logger,
odspUrlParts,
getToken,
true /* throwOnNullToken */,
);
const storageToken = await storageTokenFetcher(options, "GetFileLinkCore");
assert(
storageToken !== null,
0x2bb /* "Instrumented token fetcher with throwOnNullToken = true should never return null" */,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

raised #20409 to fix it

);

// IMPORTANT: In past we were using GetFileByUrl() API to get to the list item that was corresponding
// to the file. This was intentionally replaced with GetFileById() to solve the following issue:
// GetFileByUrl() uses webDavUrl to locate list item. This API does not work for Consumer scenarios
// where webDavUrl is constructed using legacy ODC format for backward compatibility reasons.
// GetFileByUrl() does not understand that format and thus fails. GetFileById() relies on file item
// unique guid (sharepointIds.listItemUniqueId) and it works uniformly across Consumer and Commercial.
const { url, headers } = getUrlAndHeadersWithAuth(
`${
odspUrlParts.siteUrl
}/_api/web/GetFileById(@a1)/ListItemAllFields/GetSharingInformation?@a1=guid${encodeURIComponent(
`'${fileItem.sharepointIds.listItemUniqueId}'`,
)}`,
storageToken,
true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

true, // forceAccessTokenViaAuthorizationHeader

);
const requestInit = {
method: "POST",
headers: {
"Content-Type": "application/json;odata=verbose",
"Accept": "application/json;odata=verbose",
...headers,
},
};

return { url, requestInit };
}

async function fetchWithLocationRedirectionHandling<T>(
getToken: TokenFetcher<OdspResourceTokenFetchOptions>,
odspUrlParts: IOdspUrlParts,
logger: ITelemetryLoggerExt,
options: TokenFetchOptionsEx,
fileItem: FileItemLite,
): Promise<IOdspResponse<Response>> {
for (;;) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comments to explain this logic.

const { url, requestInit } = await getRequestInformation(
getToken,
odspUrlParts,
logger,
options,
fileItem,
);

try {
return await fetchHelper(url, requestInit);
} catch (error: any) {
if (
error.errorType !== DriverErrorTypes.fileNotFoundOrAccessDeniedError ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking for fileNotFoundOrAccessDeniedError error type seems unrelated to this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

!error.redirectLocation
) {
throw error;
}

const newSiteDomain = new URL(error.redirectLocation).origin;
vladsud marked this conversation as resolved.
Show resolved Hide resolved
const oldSiteDomain = new URL(odspUrlParts.siteUrl).origin;

odspUrlParts.siteUrl = odspUrlParts.siteUrl.replace(oldSiteDomain, newSiteDomain);
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

}
}
}

async function getFileLinkCore(
getToken: TokenFetcher<OdspResourceTokenFetchOptions>,
odspUrlParts: IOdspUrlParts,
Expand All @@ -106,42 +189,15 @@ async function getFileLinkCore(
let additionalProps;
const fileLink = await getWithRetryForTokenRefresh(async (options) => {
attempts++;
const storageTokenFetcher = toInstrumentedOdspTokenFetcher(
logger,
odspUrlParts,

const response = await fetchWithLocationRedirectionHandling(
getToken,
true /* throwOnNullToken */,
);
const storageToken = await storageTokenFetcher(options, "GetFileLinkCore");
assert(
storageToken !== null,
0x2bb /* "Instrumented token fetcher with throwOnNullToken = true should never return null" */,
odspUrlParts,
logger,
options,
fileItem,
);

// IMPORTANT: In past we were using GetFileByUrl() API to get to the list item that was corresponding
// to the file. This was intentionally replaced with GetFileById() to solve the following issue:
// GetFileByUrl() uses webDavUrl to locate list item. This API does not work for Consumer scenarios
// where webDavUrl is constructed using legacy ODC format for backward compatibility reasons.
// GetFileByUrl() does not understand that format and thus fails. GetFileById() relies on file item
// unique guid (sharepointIds.listItemUniqueId) and it works uniformly across Consumer and Commercial.
const { url, headers } = getUrlAndHeadersWithAuth(
`${
odspUrlParts.siteUrl
}/_api/web/GetFileById(@a1)/ListItemAllFields/GetSharingInformation?@a1=guid${encodeURIComponent(
`'${fileItem.sharepointIds.listItemUniqueId}'`,
)}`,
storageToken,
true,
);
const requestInit = {
method: "POST",
headers: {
"Content-Type": "application/json;odata=verbose",
"Accept": "application/json;odata=verbose",
...headers,
},
};
const response = await fetchHelper(url, requestInit);
additionalProps = response.propsToLog;

const sharingInfo = await response.content.json();
Expand Down
9 changes: 6 additions & 3 deletions packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,12 @@ export function createOdspNetworkError(
break;
case 401:
case 403:
// The server throws 403 status code with innerMostError code as "serviceReadOnly" for cases where the
// database on server becomes readonly. The driver retries for such cases with exponential backup logic.
if (innerMostErrorCode === OdspServiceReadOnlyErrorCode) {
// After a redirect due to a domain change, auth headers are not sent by the browser and can trigger a 403.
if (response?.redirected) {
Philip-Scott marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes 404 could be triggered due to domain change.

error = new OdspRedirectError(errorMessage, response.url, driverProps);
} else if (innerMostErrorCode === OdspServiceReadOnlyErrorCode) {
// The server throws 403 status code with innerMostError code as "serviceReadOnly" for cases where the
// database on server becomes readonly. The driver retries for such cases with exponential backup logic.
error = new RetryableError(
errorMessage,
OdspErrorTypes.serviceReadOnly,
Expand Down
Loading