From b22032f36df89fc0fd232e87443e882299e3f861 Mon Sep 17 00:00:00 2001 From: Jatin Garg Date: Sat, 1 Jun 2024 22:56:43 -0700 Subject: [PATCH 1/6] Handle location redirection URl in getSharinkgInfo call in odsp driver --- .../drivers/odsp-driver/src/getFileLink.ts | 56 ++++++++++- .../odsp-driver/src/test/getFileLink.spec.ts | 98 +++++++++++++++++++ 2 files changed, 152 insertions(+), 2 deletions(-) diff --git a/packages/drivers/odsp-driver/src/getFileLink.ts b/packages/drivers/odsp-driver/src/getFileLink.ts index 339f29bba34c..af99b03d3a2b 100644 --- a/packages/drivers/odsp-driver/src/getFileLink.ts +++ b/packages/drivers/odsp-driver/src/getFileLink.ts @@ -11,8 +11,13 @@ import { OdspErrorTypes, OdspResourceTokenFetchOptions, TokenFetcher, + type IOdspErrorAugmentations, } from "@fluidframework/odsp-driver-definitions/internal"; -import { ITelemetryLoggerExt, PerformanceEvent } from "@fluidframework/telemetry-utils/internal"; +import { + ITelemetryLoggerExt, + PerformanceEvent, + isFluidError, +} from "@fluidframework/telemetry-utils/internal"; import { getUrlAndHeadersWithAuth } from "./getUrlAndHeadersWithAuth.js"; import { @@ -55,7 +60,12 @@ export async function getFileLink( fileLinkCore = await runWithRetry( async () => runWithRetryForCoherencyAndServiceReadOnlyErrors( - async () => getFileLinkCore(getToken, odspUrlParts, logger), + async () => + getFileLinkWithLocationRedirectionHandling( + getToken, + odspUrlParts, + logger, + ), "getFileLinkCore", logger, ), @@ -95,6 +105,46 @@ export async function getFileLink( return fileLink; } +/** + * Handles location redirection while fulfilling the getFilelink call. + * @param getToken - token fetcher to fetch the token. + * @param odspUrlParts - parts of odsp resolved url. + * @param logger - logger to send events. + * @returns Response from the API call. + * @alpha + */ +async function getFileLinkWithLocationRedirectionHandling( + getToken: TokenFetcher, + odspUrlParts: IOdspUrlParts, + logger: ITelemetryLoggerExt, +): Promise { + // We can have chains of location redirection one after the other, so have a for loop + // so that we can keep handling the same type of error. + for (;;) { + try { + return await getFileLinkCore(getToken, odspUrlParts, logger); + } catch (error: unknown) { + if ( + isFluidError(error) && + error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError + ) { + logger.sendTelemetryEvent({ + eventName: "LocationRedirectionErrorForGetOdspFileLink", + }); + const redirectLocation = (error as IOdspErrorAugmentations).redirectLocation; + if (redirectLocation !== undefined) { + // Generate the new SiteUrl from the redirection location. + const newSiteDomain = new URL(redirectLocation).origin; + const newSiteUrl = `${newSiteDomain}${new URL(odspUrlParts.siteUrl).pathname}`; + odspUrlParts.siteUrl = newSiteUrl; + continue; + } + } + throw error; + } + } +} + async function getFileLinkCore( getToken: TokenFetcher, odspUrlParts: IOdspUrlParts, @@ -138,6 +188,7 @@ async function getFileLinkCore( headers: { "Content-Type": "application/json;odata=verbose", "Accept": "application/json;odata=verbose", + "prefer": "manualredirect", ...headers, }, }; @@ -219,6 +270,7 @@ async function getFileItemLite( storageToken, forceAccessTokenViaAuthorizationHeader, ); + headers.prefer = "manualredirect"; const requestInit = { method: "GET", headers }; const response = await fetchHelper(url, requestInit); additionalProps = response.propsToLog; diff --git a/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts b/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts index 313feca913c8..81d64e60dd9c 100644 --- a/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts +++ b/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts @@ -20,6 +20,7 @@ import { describe("getFileLink", () => { const siteUrl = "https://microsoft.sharepoint-df.com/siteUrl"; + const newSiteUrl = "https://microsoft.sharepoint.com/siteUrl"; const driveId = "driveId"; const logger = new MockLogger(); const storageTokenFetcher = async (): Promise => "StorageToken"; @@ -145,4 +146,101 @@ describe("getFileLink", () => { "did not retries 5 times", ); }); + + it("should handle location redirection once", async () => { + const result = await mockFetchMultiple( + async () => + getFileLink( + storageTokenFetcher, + { siteUrl, driveId, itemId: "itemId8" }, + logger.toTelemetryLogger(), + ), + [ + async (): Promise => + createResponse( + {}, + { + error: { + "message": "locationMoved", + "@error.redirectLocation": newSiteUrl, + }, + }, + 404, + ), + async (): Promise => okResponse({}, fileItemResponse), + async (): Promise => + okResponse({}, { d: { directUrl: "sharelink" } }), + ], + ); + assert.strictEqual( + result, + "sharelink", + "File link should match url returned from sharing information", + ); + // Should be present in cache now and subsequent calls should fetch from cache. + const sharelink2 = await getFileLink( + storageTokenFetcher, + { siteUrl, driveId, itemId: "itemId8" }, + logger.toTelemetryLogger(), + ); + assert.strictEqual( + sharelink2, + "sharelink", + "File link should match url returned from sharing information from cache", + ); + }); + + it("should handle location redirection multiple times", async () => { + const result = await mockFetchMultiple( + async () => + getFileLink( + storageTokenFetcher, + { siteUrl, driveId, itemId: "itemId9" }, + logger.toTelemetryLogger(), + ), + [ + async (): Promise => + createResponse( + {}, + { + error: { + "message": "locationMoved", + "@error.redirectLocation": newSiteUrl, + }, + }, + 404, + ), + async (): Promise => + createResponse( + {}, + { + error: { + "message": "locationMoved", + "@error.redirectLocation": newSiteUrl, + }, + }, + 404, + ), + async (): Promise => okResponse({}, fileItemResponse), + async (): Promise => + okResponse({}, { d: { directUrl: "sharelink" } }), + ], + ); + assert.strictEqual( + result, + "sharelink", + "File link should match url returned from sharing information", + ); + // Should be present in cache now and subsequent calls should fetch from cache. + const sharelink2 = await getFileLink( + storageTokenFetcher, + { siteUrl, driveId, itemId: "itemId9" }, + logger.toTelemetryLogger(), + ); + assert.strictEqual( + sharelink2, + "sharelink", + "File link should match url returned from sharing information from cache", + ); + }); }); From bc60c9ed3ad72ec5c37e2c1a72316b9f1da232e8 Mon Sep 17 00:00:00 2001 From: Jatin Garg Date: Mon, 3 Jun 2024 10:32:05 -0700 Subject: [PATCH 2/6] add comment --- packages/drivers/odsp-driver/src/getFileLink.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/drivers/odsp-driver/src/getFileLink.ts b/packages/drivers/odsp-driver/src/getFileLink.ts index af99b03d3a2b..3f3530de54ea 100644 --- a/packages/drivers/odsp-driver/src/getFileLink.ts +++ b/packages/drivers/odsp-driver/src/getFileLink.ts @@ -183,6 +183,10 @@ async function getFileLinkCore( storageToken, true, ); + // The location of file can move on Spo in which case server returns 308(Permanent Redirect) error. + // Adding below header will make VROOM API return 404 instead of 308 and browser can not intercept it. + // This error thrown by server will contain the new redirect location. Look at the 404 error parsing + // for further reference here: \packages\utils\odsp-doclib-utils\src\odspErrorUtils.ts const requestInit = { method: "POST", headers: { From f5fa5b84c13e49cebde9082d6ac420dd27acc951 Mon Sep 17 00:00:00 2001 From: Jatin Garg Date: Tue, 11 Jun 2024 22:29:50 -0700 Subject: [PATCH 3/6] Pr sugg --- packages/drivers/odsp-driver/src/getFileLink.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/drivers/odsp-driver/src/getFileLink.ts b/packages/drivers/odsp-driver/src/getFileLink.ts index 3f3530de54ea..d3ce7a2901ff 100644 --- a/packages/drivers/odsp-driver/src/getFileLink.ts +++ b/packages/drivers/odsp-driver/src/getFileLink.ts @@ -106,7 +106,9 @@ export async function getFileLink( } /** - * Handles location redirection while fulfilling the getFilelink call. + * Handles location redirection while fulfilling the getFilelink call. We don't want browser to handle + * the redirection as the browser will retry with same auth token which will not work as we need app + * to regenerate tokens for the new site domain. * @param getToken - token fetcher to fetch the token. * @param odspUrlParts - parts of odsp resolved url. * @param logger - logger to send events. @@ -128,11 +130,11 @@ async function getFileLinkWithLocationRedirectionHandling( isFluidError(error) && error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError ) { - logger.sendTelemetryEvent({ - eventName: "LocationRedirectionErrorForGetOdspFileLink", - }); const redirectLocation = (error as IOdspErrorAugmentations).redirectLocation; if (redirectLocation !== undefined) { + logger.sendTelemetryEvent({ + eventName: "LocationRedirectionErrorForGetOdspFileLink", + }); // Generate the new SiteUrl from the redirection location. const newSiteDomain = new URL(redirectLocation).origin; const newSiteUrl = `${newSiteDomain}${new URL(odspUrlParts.siteUrl).pathname}`; @@ -184,7 +186,7 @@ async function getFileLinkCore( true, ); // The location of file can move on Spo in which case server returns 308(Permanent Redirect) error. - // Adding below header will make VROOM API return 404 instead of 308 and browser can not intercept it. + // Adding prefer: manualredirect header will make VROOM API return 404 instead of 308 and browser can not intercept it. // This error thrown by server will contain the new redirect location. Look at the 404 error parsing // for further reference here: \packages\utils\odsp-doclib-utils\src\odspErrorUtils.ts const requestInit = { @@ -274,6 +276,10 @@ async function getFileItemLite( storageToken, forceAccessTokenViaAuthorizationHeader, ); + // The location of file can move on Spo in which case server returns 308(Permanent Redirect) error. + // Adding prefer: manualredirect header will make VROOM API return 404 instead of 308 and browser can not intercept it. + // This error thrown by server will contain the new redirect location. Look at the 404 error parsing + // for further reference here: \packages\utils\odsp-doclib-utils\src\odspErrorUtils.ts headers.prefer = "manualredirect"; const requestInit = { method: "GET", headers }; const response = await fetchHelper(url, requestInit); From 81eda74f284ff994068f5113884190f06c378071 Mon Sep 17 00:00:00 2001 From: Jatin Garg Date: Wed, 12 Jun 2024 19:18:42 -0700 Subject: [PATCH 4/6] revise --- .../drivers/odsp-driver/src/getFileLink.ts | 49 +++++----- .../odsp-driver/src/test/getFileLink.spec.ts | 95 ++++++++++++++++--- packages/utils/odsp-doclib-utils/src/index.ts | 1 + .../odsp-doclib-utils/src/odspErrorUtils.ts | 26 +++++ 4 files changed, 133 insertions(+), 38 deletions(-) diff --git a/packages/drivers/odsp-driver/src/getFileLink.ts b/packages/drivers/odsp-driver/src/getFileLink.ts index d3ce7a2901ff..14960f7a9995 100644 --- a/packages/drivers/odsp-driver/src/getFileLink.ts +++ b/packages/drivers/odsp-driver/src/getFileLink.ts @@ -6,12 +6,12 @@ import type { ITelemetryBaseProperties } from "@fluidframework/core-interfaces"; import { assert } from "@fluidframework/core-utils/internal"; import { NonRetryableError, runWithRetry } from "@fluidframework/driver-utils/internal"; +import { hasRedirectionLocation } from "@fluidframework/odsp-doclib-utils/internal"; import { IOdspUrlParts, OdspErrorTypes, OdspResourceTokenFetchOptions, TokenFetcher, - type IOdspErrorAugmentations, } from "@fluidframework/odsp-driver-definitions/internal"; import { ITelemetryLoggerExt, @@ -108,7 +108,8 @@ export async function getFileLink( /** * Handles location redirection while fulfilling the getFilelink call. We don't want browser to handle * the redirection as the browser will retry with same auth token which will not work as we need app - * to regenerate tokens for the new site domain. + * to regenerate tokens for the new site domain. So when we will make the network calls below we will set + * the redirect:manual header to manually handle these redirects. * @param getToken - token fetcher to fetch the token. * @param odspUrlParts - parts of odsp resolved url. * @param logger - logger to send events. @@ -121,30 +122,34 @@ async function getFileLinkWithLocationRedirectionHandling( logger: ITelemetryLoggerExt, ): Promise { // We can have chains of location redirection one after the other, so have a for loop - // so that we can keep handling the same type of error. - for (;;) { + // so that we can keep handling the same type of error. Set max number of redirection to 5. + let lastError: unknown; + for (let count = 1; count <= 5; count++) { try { return await getFileLinkCore(getToken, odspUrlParts, logger); } catch (error: unknown) { + lastError = error; if ( isFluidError(error) && - error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError + error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError && + hasRedirectionLocation(error) && + error.redirectLocation !== undefined ) { - const redirectLocation = (error as IOdspErrorAugmentations).redirectLocation; - if (redirectLocation !== undefined) { - logger.sendTelemetryEvent({ - eventName: "LocationRedirectionErrorForGetOdspFileLink", - }); - // Generate the new SiteUrl from the redirection location. - const newSiteDomain = new URL(redirectLocation).origin; - const newSiteUrl = `${newSiteDomain}${new URL(odspUrlParts.siteUrl).pathname}`; - odspUrlParts.siteUrl = newSiteUrl; - continue; - } + const redirectLocation = error.redirectLocation; + logger.sendTelemetryEvent({ + eventName: "LocationRedirectionErrorForGetOdspFileLink", + retryCount: count, + }); + // Generate the new SiteUrl from the redirection location. + const newSiteDomain = new URL(redirectLocation).origin; + const newSiteUrl = `${newSiteDomain}${new URL(odspUrlParts.siteUrl).pathname}`; + odspUrlParts.siteUrl = newSiteUrl; + continue; } throw error; } } + throw lastError; } async function getFileLinkCore( @@ -185,16 +190,12 @@ async function getFileLinkCore( storageToken, true, ); - // The location of file can move on Spo in which case server returns 308(Permanent Redirect) error. - // Adding prefer: manualredirect header will make VROOM API return 404 instead of 308 and browser can not intercept it. - // This error thrown by server will contain the new redirect location. Look at the 404 error parsing - // for further reference here: \packages\utils\odsp-doclib-utils\src\odspErrorUtils.ts const requestInit = { method: "POST", headers: { "Content-Type": "application/json;odata=verbose", "Accept": "application/json;odata=verbose", - "prefer": "manualredirect", + "redirect": "manual", ...headers, }, }; @@ -276,11 +277,7 @@ async function getFileItemLite( storageToken, forceAccessTokenViaAuthorizationHeader, ); - // The location of file can move on Spo in which case server returns 308(Permanent Redirect) error. - // Adding prefer: manualredirect header will make VROOM API return 404 instead of 308 and browser can not intercept it. - // This error thrown by server will contain the new redirect location. Look at the 404 error parsing - // for further reference here: \packages\utils\odsp-doclib-utils\src\odspErrorUtils.ts - headers.prefer = "manualredirect"; + headers.redirect = "manual"; const requestInit = { method: "GET", headers }; const response = await fetchHelper(url, requestInit); additionalProps = response.propsToLog; diff --git a/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts b/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts index 81d64e60dd9c..b64b5ea64b59 100644 --- a/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts +++ b/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts @@ -158,14 +158,13 @@ describe("getFileLink", () => { [ async (): Promise => createResponse( - {}, + { Location: newSiteUrl }, { error: { - "message": "locationMoved", - "@error.redirectLocation": newSiteUrl, + message: "locationMoved", }, }, - 404, + 308, ), async (): Promise => okResponse({}, fileItemResponse), async (): Promise => @@ -201,25 +200,23 @@ describe("getFileLink", () => { [ async (): Promise => createResponse( - {}, + { Location: newSiteUrl }, { error: { - "message": "locationMoved", - "@error.redirectLocation": newSiteUrl, + message: "locationMoved", }, }, - 404, + 302, ), async (): Promise => createResponse( - {}, + { Location: newSiteUrl }, { error: { - "message": "locationMoved", - "@error.redirectLocation": newSiteUrl, + message: "locationMoved", }, }, - 404, + 307, ), async (): Promise => okResponse({}, fileItemResponse), async (): Promise => @@ -243,4 +240,78 @@ describe("getFileLink", () => { "File link should match url returned from sharing information from cache", ); }); + + it("should handle location redirection max 5 times", async () => { + await assert.rejects( + mockFetchMultiple(async () => { + return getFileLink( + storageTokenFetcher, + { siteUrl, driveId, itemId: "itemId10" }, + logger.toTelemetryLogger(), + ); + }, [ + async (): Promise => + createResponse( + { Location: newSiteUrl }, + { + error: { + message: "locationMoved", + }, + }, + 308, + ), + async (): Promise => + createResponse( + { Location: newSiteUrl }, + { + error: { + message: "locationMoved", + }, + }, + 308, + ), + async (): Promise => + createResponse( + { Location: newSiteUrl }, + { + error: { + message: "locationMoved", + }, + }, + 308, + ), + async (): Promise => + createResponse( + { Location: newSiteUrl }, + { + error: { + message: "locationMoved", + }, + }, + 308, + ), + async (): Promise => + createResponse( + { Location: newSiteUrl }, + { + error: { + message: "locationMoved", + }, + }, + 308, + ), + async (): Promise => + createResponse( + { Location: newSiteUrl }, + { + error: { + message: "locationMoved", + }, + }, + 308, + ), + ]), + "File link should reject when not found", + ); + }); }); diff --git a/packages/utils/odsp-doclib-utils/src/index.ts b/packages/utils/odsp-doclib-utils/src/index.ts index c0be595d5f49..9d5ddbaa6c0a 100644 --- a/packages/utils/odsp-doclib-utils/src/index.ts +++ b/packages/utils/odsp-doclib-utils/src/index.ts @@ -41,6 +41,7 @@ export { fetchIncorrectResponse, getSPOAndGraphRequestIdsFromResponse, hasFacetCodes, + hasRedirectionLocation, OdspErrorResponse, OdspErrorResponseInnerError, OdspRedirectError, diff --git a/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts b/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts index 1bc5c32060f6..bcd90efb6e3f 100644 --- a/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts +++ b/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts @@ -219,6 +219,18 @@ export function createOdspNetworkError( const driverProps = { driverVersion, statusCode, ...props }; switch (statusCode) { + // The location of file can move on Spo. If the redirect:manual header is added to network call + // it causes browser to not handle the redirects. Location header in such cases will contain the new location. + case 301: + case 302: + case 303: + case 307: + case 308: + redirectLocation = response?.headers.get("Location") ?? undefined; + if (redirectLocation !== undefined) { + error = new OdspRedirectError(errorMessage, redirectLocation, driverProps); + break; + } case 400: if (innerMostErrorCode === "fluidInvalidSchema") { error = new FluidInvalidSchemaError(errorMessage, driverProps); @@ -446,3 +458,17 @@ function numberFromHeader(header: string | null): number | undefined { export function hasFacetCodes(x: any): x is Pick { return Array.isArray(x?.facetCodes); } + +/** + * @internal + */ +export function hasRedirectionLocation( + x: unknown, +): x is Pick { + return ( + x !== null && + typeof x === "object" && + "redirectLocation" in x && + typeof x?.redirectLocation === "string" + ); +} From 36a96570613fb1ec0b148b5a0a245a5137407f1e Mon Sep 17 00:00:00 2001 From: Jatin Garg Date: Wed, 12 Jun 2024 21:27:48 -0700 Subject: [PATCH 5/6] revise --- packages/drivers/odsp-driver/src/getFileLink.ts | 1 + packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/packages/drivers/odsp-driver/src/getFileLink.ts b/packages/drivers/odsp-driver/src/getFileLink.ts index 14960f7a9995..f875043a5a31 100644 --- a/packages/drivers/odsp-driver/src/getFileLink.ts +++ b/packages/drivers/odsp-driver/src/getFileLink.ts @@ -110,6 +110,7 @@ export async function getFileLink( * the redirection as the browser will retry with same auth token which will not work as we need app * to regenerate tokens for the new site domain. So when we will make the network calls below we will set * the redirect:manual header to manually handle these redirects. + * Refer: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/308 * @param getToken - token fetcher to fetch the token. * @param odspUrlParts - parts of odsp resolved url. * @param logger - logger to send events. diff --git a/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts b/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts index bcd90efb6e3f..0f48f86892b7 100644 --- a/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts +++ b/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts @@ -221,6 +221,7 @@ export function createOdspNetworkError( switch (statusCode) { // The location of file can move on Spo. If the redirect:manual header is added to network call // it causes browser to not handle the redirects. Location header in such cases will contain the new location. + // Refer: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/308 case 301: case 302: case 303: From 54c078ddacd3e35800413e6cfb48f3e2987cc9e9 Mon Sep 17 00:00:00 2001 From: Jatin Garg Date: Thu, 13 Jun 2024 10:44:47 -0700 Subject: [PATCH 6/6] commment --- .../odsp-doclib-utils/api-report/odsp-doclib-utils.api.md | 3 +++ packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts | 1 + 2 files changed, 4 insertions(+) diff --git a/packages/utils/odsp-doclib-utils/api-report/odsp-doclib-utils.api.md b/packages/utils/odsp-doclib-utils/api-report/odsp-doclib-utils.api.md index fd26c28f7d39..de8a9e15f8a1 100644 --- a/packages/utils/odsp-doclib-utils/api-report/odsp-doclib-utils.api.md +++ b/packages/utils/odsp-doclib-utils/api-report/odsp-doclib-utils.api.md @@ -82,6 +82,9 @@ export function getSPOAndGraphRequestIdsFromResponse(headers: { // @internal (undocumented) export function hasFacetCodes(x: any): x is Pick; +// @internal (undocumented) +export function hasRedirectionLocation(x: unknown): x is Pick; + // @alpha (undocumented) export interface IOdspAuthRequestInfo { // (undocumented) diff --git a/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts b/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts index 0f48f86892b7..1fe25f074a16 100644 --- a/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts +++ b/packages/utils/odsp-doclib-utils/src/odspErrorUtils.ts @@ -232,6 +232,7 @@ export function createOdspNetworkError( error = new OdspRedirectError(errorMessage, redirectLocation, driverProps); break; } + // Don't break here. Let it be a generic network error if redirectLocation is not there. case 400: if (innerMostErrorCode === "fluidInvalidSchema") { error = new FluidInvalidSchemaError(errorMessage, driverProps);