From b48bee8e3a77b0114d347d5d7f70d4f5fdf44e9b Mon Sep 17 00:00:00 2001 From: Jatin Garg <48029724+jatgarg@users.noreply.github.com> Date: Tue, 17 Sep 2024 15:01:36 -0700 Subject: [PATCH] Handle Location redirection for getSharingInformation call (#22551) ## Description [AB#15188](https://dev.azure.com/fluidframework/internal/_workitems/edit/15188) Handle location redirection when calling getSharingInformation in ODSP Driver. Since this call does not include the v2.0 which calls the API without the drive and item ID, these calls fail if the tenant has been renamed. Sometimes this creates issues when creating a new page, since the user could not get the sharing link to the new page. GetFileItem call also returns the new siteUrl to which the file has been moved. Use that to detect the new domain and then change the resolved url accordingly. Co-authored-by: Jatin Garg --- .../drivers/odsp-driver/src/getFileLink.ts | 76 +++++++--- .../odsp-driver/src/test/getFileLink.spec.ts | 133 ++++++++---------- 2 files changed, 115 insertions(+), 94 deletions(-) diff --git a/packages/drivers/odsp-driver/src/getFileLink.ts b/packages/drivers/odsp-driver/src/getFileLink.ts index 3c5a63e4ce58..9841655b5543 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 IOdspResolvedUrl, } from "@fluidframework/odsp-driver-definitions/internal"; import { ITelemetryLoggerExt, @@ -44,10 +44,10 @@ const fileLinkCache = new Map>(); */ export async function getFileLink( getToken: TokenFetcher, - odspUrlParts: IOdspUrlParts, + resolvedUrl: IOdspResolvedUrl, logger: ITelemetryLoggerExt, ): Promise { - const cacheKey = `${odspUrlParts.siteUrl}_${odspUrlParts.driveId}_${odspUrlParts.itemId}`; + const cacheKey = `${resolvedUrl.siteUrl}_${resolvedUrl.driveId}_${resolvedUrl.itemId}`; const maybeFileLinkCacheEntry = fileLinkCache.get(cacheKey); if (maybeFileLinkCacheEntry !== undefined) { return maybeFileLinkCacheEntry; @@ -61,7 +61,7 @@ export async function getFileLink( async () => runWithRetryForCoherencyAndServiceReadOnlyErrors( async () => - getFileLinkWithLocationRedirectionHandling(getToken, odspUrlParts, logger), + getFileLinkWithLocationRedirectionHandling(getToken, resolvedUrl, logger), "getFileLinkCore", logger, ), @@ -116,32 +116,41 @@ export async function getFileLink( */ async function getFileLinkWithLocationRedirectionHandling( getToken: TokenFetcher, - odspUrlParts: IOdspUrlParts, + resolvedUrl: IOdspResolvedUrl, 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. Set max number of redirection to 5. let lastError: unknown; + let locationRedirected = false; for (let count = 1; count <= 5; count++) { try { - return await getFileLinkCore(getToken, odspUrlParts, logger); + const fileItem = await getFileItemLite(getToken, resolvedUrl, logger, true); + // Sometimes the siteUrl in the actual file is different from the siteUrl in the resolvedUrl due to location + // redirection. This creates issues in the getSharingInformation call. So we need to update the siteUrl in the + // resolvedUrl to the siteUrl in the fileItem which is the updated siteUrl. + const oldSiteDomain = new URL(resolvedUrl.siteUrl).origin; + const newSiteDomain = new URL(fileItem.sharepointIds.siteUrl).origin; + if (oldSiteDomain !== newSiteDomain) { + locationRedirected = true; + logger.sendTelemetryEvent({ + eventName: "LocationRedirectionErrorForGetOdspFileLink", + retryCount: count, + }); + renameTenantInOdspResolvedUrl(resolvedUrl, newSiteDomain); + } + return await getFileLinkCore(getToken, resolvedUrl, logger, fileItem); } catch (error: unknown) { lastError = error; + // If the getSharingLink call fails with the 401/403/404 error, then it could be due to that the file has moved + // to another location. This could occur in case we have more than 1 tenant rename. In that case, we should retry + // the getFileItemLite call to get the updated fileItem. if ( isFluidError(error) && - error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError && - hasRedirectionLocation(error) && - error.redirectLocation !== undefined + locationRedirected && + (error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError || + error.errorType === OdspErrorTypes.authorizationError) ) { - 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; @@ -154,9 +163,8 @@ async function getFileLinkCore( getToken: TokenFetcher, odspUrlParts: IOdspUrlParts, logger: ITelemetryLoggerExt, + fileItem: FileItemLite, ): Promise { - const fileItem = await getFileItemLite(getToken, odspUrlParts, logger, true); - // ODSP link requires extra call to return link that is resistant to file being renamed or moved to different folder return PerformanceEvent.timedExecAsync( logger, @@ -194,7 +202,6 @@ async function getFileLinkCore( headers: { "Content-Type": "application/json;odata=verbose", "Accept": "application/json;odata=verbose", - "redirect": "manual", ...headers, }, }; @@ -281,7 +288,6 @@ async function getFileItemLite( ); const headers = getHeadersWithAuth(authHeader); - headers.redirect = "manual"; const requestInit = { method, headers }; const response = await fetchHelper(url, requestInit); additionalProps = response.propsToLog; @@ -302,3 +308,29 @@ async function getFileItemLite( }, ); } + +/** + * It takes a resolved url with old siteUrl and patches resolved url with updated site url domain. + * @param odspResolvedUrl - Previous odsp resolved url with older site url. + * @param newSiteDomain - New site domain after the tenant rename. + */ +function renameTenantInOdspResolvedUrl( + odspResolvedUrl: IOdspResolvedUrl, + newSiteDomain: string, +): void { + const newSiteUrl = `${newSiteDomain}${new URL(odspResolvedUrl.siteUrl).pathname}`; + odspResolvedUrl.siteUrl = newSiteUrl; + + if (odspResolvedUrl.endpoints.attachmentGETStorageUrl) { + odspResolvedUrl.endpoints.attachmentGETStorageUrl = `${newSiteDomain}${new URL(odspResolvedUrl.endpoints.attachmentGETStorageUrl).pathname}`; + } + if (odspResolvedUrl.endpoints.attachmentPOSTStorageUrl) { + odspResolvedUrl.endpoints.attachmentPOSTStorageUrl = `${newSiteDomain}${new URL(odspResolvedUrl.endpoints.attachmentPOSTStorageUrl).pathname}`; + } + if (odspResolvedUrl.endpoints.deltaStorageUrl) { + odspResolvedUrl.endpoints.deltaStorageUrl = `${newSiteDomain}${new URL(odspResolvedUrl.endpoints.deltaStorageUrl).pathname}`; + } + if (odspResolvedUrl.endpoints.snapshotStorageUrl) { + odspResolvedUrl.endpoints.snapshotStorageUrl = `${newSiteDomain}${new URL(odspResolvedUrl.endpoints.snapshotStorageUrl).pathname}`; + } +} diff --git a/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts b/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts index e73e77882bbf..3ae2ecd0b76e 100644 --- a/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts +++ b/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts @@ -5,6 +5,7 @@ import { strict as assert } from "node:assert"; +import type { IOdspResolvedUrl } from "@fluidframework/odsp-driver-definitions/internal"; import { MockLogger } from "@fluidframework/telemetry-utils/internal"; import { getFileLink } from "../getFileLink.js"; @@ -27,7 +28,17 @@ describe("getFileLink", () => { const fileItemResponse = { webDavUrl: "fetchDavUrl", webUrl: "fetchWebUrl", - sharepointIds: { listItemUniqueId: "fetchFileId" }, + sharepointIds: { listItemUniqueId: "fetchFileId", siteUrl }, + }; + + const getOdspResolvedUrl = (itemId: string): IOdspResolvedUrl => { + return { + siteUrl, + driveId, + itemId, + odspResolvedUrl: true, + endpoints: {}, + } as unknown as IOdspResolvedUrl; }; afterEach(() => { @@ -39,7 +50,7 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId4" }, + getOdspResolvedUrl("itemId4"), logger.toTelemetryLogger(), ), [ @@ -60,7 +71,7 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId5" }, + getOdspResolvedUrl("itemId5"), logger.toTelemetryLogger(), ), [ @@ -78,7 +89,7 @@ describe("getFileLink", () => { mockFetchSingle(async () => { return getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId6" }, + getOdspResolvedUrl("itemId6"), logger.toTelemetryLogger(), ); }, notFound), @@ -91,7 +102,7 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId7" }, + getOdspResolvedUrl("itemId7"), logger.toTelemetryLogger(), ), [ @@ -109,7 +120,7 @@ describe("getFileLink", () => { // Should be present in cache now and subsequent calls should fetch from cache. const sharelink2 = await getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId7" }, + getOdspResolvedUrl("itemId7"), logger.toTelemetryLogger(), ); assert.strictEqual( @@ -125,7 +136,7 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId7" }, + getOdspResolvedUrl("itemId7"), logger.toTelemetryLogger(), ), [ @@ -150,21 +161,18 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId8" }, + getOdspResolvedUrl("itemId8"), logger.toTelemetryLogger(), ), [ async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 308, ), - async (): Promise => okResponse({}, fileItemResponse), async (): Promise => okResponse({}, { d: { directUrl: "sharelink" } }), ], ); @@ -176,7 +184,7 @@ describe("getFileLink", () => { // Should be present in cache now and subsequent calls should fetch from cache. const sharelink2 = await getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId8" }, + getOdspResolvedUrl("itemId8"), logger.toTelemetryLogger(), ); assert.strictEqual( @@ -191,31 +199,27 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId9" }, + getOdspResolvedUrl("itemId9"), logger.toTelemetryLogger(), ), [ async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 302, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl }, }, - 307, ), - async (): Promise => okResponse({}, fileItemResponse), async (): Promise => okResponse({}, { d: { directUrl: "sharelink" } }), ], ); @@ -227,7 +231,7 @@ describe("getFileLink", () => { // Should be present in cache now and subsequent calls should fetch from cache. const sharelink2 = await getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId9" }, + getOdspResolvedUrl("itemId9"), logger.toTelemetryLogger(), ); assert.strictEqual( @@ -242,70 +246,55 @@ describe("getFileLink", () => { mockFetchMultiple(async () => { return getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId10" }, + getOdspResolvedUrl("itemId10"), logger.toTelemetryLogger(), ); }, [ async (): Promise => - createResponse( - { Location: newSiteUrl }, - { - error: { - message: "locationMoved", - }, - }, - 308, - ), - async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 308, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl }, }, - 308, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 308, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl }, }, - 308, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 308, ), + notFound, ]), "File link should reject when not found", );