diff --git a/packages/fx-core/src/component/driver/oauth/update.ts b/packages/fx-core/src/component/driver/oauth/update.ts index d3fba99de2..726b895fa9 100644 --- a/packages/fx-core/src/component/driver/oauth/update.ts +++ b/packages/fx-core/src/component/driver/oauth/update.ts @@ -19,7 +19,7 @@ import { import { OauthNameTooLongError } from "./error/oauthNameTooLong"; import { UpdateOauthArgs } from "./interface/updateOauthArgs"; import { logMessageKeys } from "./utility/constants"; -import { getandValidateOauthInfoFromSpec } from "./utility/utility"; +import { getandValidateOauthInfoFromSpec, OauthInfo } from "./utility/utility"; import { OauthDisablePKCEError } from "./error/oauthDisablePKCEError"; const actionName = "oauth/update"; // DO NOT MODIFY the name @@ -44,7 +44,6 @@ export class UpdateOauthDriver implements StepDriver { this.validateArgs(args); const authInfo = await getandValidateOauthInfoFromSpec(args, context, actionName); - const domain = authInfo.domain; const appStudioTokenRes = await context.m365TokenProvider.getAccessToken({ scopes: AppStudioScopes, }); @@ -61,7 +60,7 @@ export class UpdateOauthDriver implements StepDriver { throw new OauthDisablePKCEError(actionName); } - const diffMsgs = this.compareOauthRegistration(getOauthRes, args, domain); + const diffMsgs = this.compareOauthRegistration(getOauthRes, args, authInfo); // If there is no difference, skip the update if (!diffMsgs || diffMsgs.length === 0) { const summary = getLocalizedString(logMessageKeys.skipUpdateOauth); @@ -76,7 +75,9 @@ export class UpdateOauthDriver implements StepDriver { // If there is difference, ask user to confirm the update // Skip confirm if only targetUrlsShouldStartWith is different when the url contains devtunnel - if (!this.shouldSkipConfirm(diffMsgs, getOauthRes.targetUrlsShouldStartWith, domain)) { + if ( + !this.shouldSkipConfirm(diffMsgs, getOauthRes.targetUrlsShouldStartWith, authInfo.domain) + ) { const userConfirm = await context.ui!.confirm!({ name: "confirm-update-oauth", title: getLocalizedString("driver.oauth.confirm.update", diffMsgs.join(",\n")), @@ -87,7 +88,7 @@ export class UpdateOauthDriver implements StepDriver { } } - const oauth = this.mapArgsToOauthRegistration(args, domain); + const oauth = this.mapArgsToOauthRegistration(args, authInfo); await teamsDevPortalClient.updateOauthRegistration( appStudioToken, oauth, @@ -179,7 +180,7 @@ export class UpdateOauthDriver implements StepDriver { private compareOauthRegistration( current: OauthRegistration, input: UpdateOauthArgs, - domain: string[] + authInfo: OauthInfo ): string[] { const diffMsgs: string[] = []; if (current.description !== input.name) { @@ -201,6 +202,7 @@ export class UpdateOauthDriver implements StepDriver { } // Compare domain + const domain = authInfo.domain; if ( current.targetUrlsShouldStartWith.length !== domain.length || !current.targetUrlsShouldStartWith.every((value) => domain.includes(value)) || @@ -213,7 +215,46 @@ export class UpdateOauthDriver implements StepDriver { ); } - if (current.isPKCEEnabled !== input.isPKCEEnabled) { + // TODO: Need to separate the logic for different flows + // Compare authorizationEndpoint + if ( + authInfo.authorizationEndpoint && + current.authorizationEndpoint !== authInfo.authorizationEndpoint + ) { + diffMsgs.push( + `authorizationEndpoint: ${current.authorizationEndpoint} => ${authInfo.authorizationEndpoint}` + ); + } + + // Compare tokenExchangeEndpoint + if ( + authInfo.tokenExchangeEndpoint && + current.tokenExchangeEndpoint !== authInfo.tokenExchangeEndpoint + ) { + diffMsgs.push( + `tokenExchangeEndpoint: ${current.tokenExchangeEndpoint} => ${authInfo.tokenExchangeEndpoint}` + ); + } + + // Compare tokenRefreshEndpoint + if (current.tokenRefreshEndpoint !== authInfo.tokenRefreshEndpoint) { + diffMsgs.push( + `tokenRefreshEndpoint: ${current.tokenRefreshEndpoint ?? "Undefined"} => ${ + authInfo.tokenRefreshEndpoint ?? "Undefined" + }` + ); + } + + // Compare scopes + if (!this.compareScopes(current.scopes, authInfo.scopes)) { + diffMsgs.push( + `scopes: ${current.scopes.join(",")} => ${ + authInfo.scopes ? authInfo.scopes.join(",") : "Undefined" + }` + ); + } + + if (!!current.isPKCEEnabled !== !!input.isPKCEEnabled) { diffMsgs.push( `isPKCEEnabled: ${(!!current.isPKCEEnabled).toString()} => ${(!!input.isPKCEEnabled).toString()}` ); @@ -233,7 +274,10 @@ export class UpdateOauthDriver implements StepDriver { ); } - private mapArgsToOauthRegistration(args: UpdateOauthArgs, domain: string[]): OauthRegistration { + private mapArgsToOauthRegistration( + args: UpdateOauthArgs, + authInfo: OauthInfo + ): OauthRegistration { const targetAudience = args.targetAudience ? (args.targetAudience as OauthRegistrationTargetAudience) : undefined; @@ -243,11 +287,24 @@ export class UpdateOauthDriver implements StepDriver { return { description: args.name, - targetUrlsShouldStartWith: domain, + targetUrlsShouldStartWith: authInfo.domain, applicableToApps: applicableToApps, m365AppId: applicableToApps === OauthRegistrationAppType.SpecificApp ? args.appId : "", targetAudience: targetAudience, isPKCEEnabled: !!args.isPKCEEnabled, + authorizationEndpoint: authInfo.authorizationEndpoint, + tokenExchangeEndpoint: authInfo.tokenExchangeEndpoint, + tokenRefreshEndpoint: authInfo.tokenRefreshEndpoint, + scopes: authInfo.scopes ?? [], } as OauthRegistration; } + + private compareScopes(current: string[], input: string[] | undefined): boolean { + return ( + !!input && + current.length === input.length && + current.every((value) => input.includes(value)) && + input.every((value) => current.includes(value)) + ); + } } diff --git a/packages/fx-core/src/component/driver/oauth/utility/utility.ts b/packages/fx-core/src/component/driver/oauth/utility/utility.ts index 933783bd48..8c62346b44 100644 --- a/packages/fx-core/src/component/driver/oauth/utility/utility.ts +++ b/packages/fx-core/src/component/driver/oauth/utility/utility.ts @@ -60,46 +60,42 @@ export async function getandValidateOauthInfoFromSpec( }); validateDomain(domains, actionName); - if ("flow" in args) { - const authInfoArray = operations - .map((value) => { - let authInfo; - switch (args.flow) { - case "authorizationCode": - default: - authInfo = (value.auth?.authScheme as OpenAPIV3.OAuth2SecurityScheme).flows - .authorizationCode; - } - return { - authorizationUrl: authInfo!.authorizationUrl, - tokenUrl: authInfo!.tokenUrl, - refreshUrl: authInfo!.refreshUrl, - scopes: Object.keys(authInfo!.scopes), - }; - }) - .reduce((accumulator: AuthInfo[], currentValue) => { - if (!accumulator.find((item) => isEqual(item, currentValue))) { - accumulator.push(currentValue); - } - return accumulator; - }, []); + // Need to separate the logic for different flows + const flow = "flow" in args ? args.flow : "authorizationCode"; + const authInfoArray = operations + .map((value) => { + let authInfo; + switch (flow) { + case "authorizationCode": + default: + authInfo = (value.auth?.authScheme as OpenAPIV3.OAuth2SecurityScheme).flows + .authorizationCode; + } + return { + authorizationUrl: authInfo!.authorizationUrl, + tokenUrl: authInfo!.tokenUrl, + refreshUrl: authInfo!.refreshUrl, + scopes: Object.keys(authInfo!.scopes), + }; + }) + .reduce((accumulator: AuthInfo[], currentValue) => { + if (!accumulator.find((item) => isEqual(item, currentValue))) { + accumulator.push(currentValue); + } + return accumulator; + }, []); - if (authInfoArray.length !== 1) { - throw new OauthAuthInfoInvalid(actionName); - } - const authInfo = authInfoArray[0]; - return { - domain: domains, - authorizationEndpoint: authInfo.authorizationUrl, - tokenExchangeEndpoint: authInfo.tokenUrl, - tokenRefreshEndpoint: authInfo.refreshUrl, - scopes: authInfo.scopes, - }; - } else { - return { - domain: domains, - }; + if (authInfoArray.length !== 1) { + throw new OauthAuthInfoInvalid(actionName); } + const authInfo = authInfoArray[0]; + return { + domain: domains, + authorizationEndpoint: authInfo.authorizationUrl, + tokenExchangeEndpoint: authInfo.tokenUrl, + tokenRefreshEndpoint: authInfo.refreshUrl, + scopes: authInfo.scopes, + }; } function validateDomain(domain: string[], actionName: string): void { diff --git a/packages/fx-core/tests/component/driver/oauth/update.test.ts b/packages/fx-core/tests/component/driver/oauth/update.test.ts index 3278dabc8d..fc0ce4a086 100644 --- a/packages/fx-core/tests/component/driver/oauth/update.test.ts +++ b/packages/fx-core/tests/component/driver/oauth/update.test.ts @@ -117,6 +117,7 @@ describe("CreateOauthDriver", () => { authorizationCode: { authorizationUrl: "https://test", tokenUrl: "https://test", + refreshUrl: "https://test", scopes: { mockedScopes: "mockedScopes", }, @@ -137,6 +138,10 @@ describe("CreateOauthDriver", () => { expect((config as ConfirmConfig).title.includes("m365AppId")).to.be.true; expect((config as ConfirmConfig).title.includes("targetAudience")).to.be.true; expect((config as ConfirmConfig).title.includes("isPKCEEnabled")).to.be.true; + expect((config as ConfirmConfig).title.includes("authorizationEndpoint")).to.be.true; + expect((config as ConfirmConfig).title.includes("tokenExchangeEndpoint")).to.be.true; + expect((config as ConfirmConfig).title.includes("tokenRefreshEndpoint")).to.be.true; + expect((config as ConfirmConfig).title.includes("scopes")).to.be.true; return ok({ type: "success", value: true }); }); @@ -251,7 +256,7 @@ describe("CreateOauthDriver", () => { clientSecret: "mockedClientSecret", authorizationEndpoint: "mockedAuthorizationEndpoint", tokenExchangeEndpoint: "mockedTokenExchangeEndpoint", - scopes: ["mockedScope"], + scopes: ["mockedScopes"], }); sinon.stub(SpecParser.prototype, "list").resolves({ APIs: [ @@ -265,8 +270,8 @@ describe("CreateOauthDriver", () => { type: "oauth2", flows: { authorizationCode: { - authorizationUrl: "https://test", - tokenUrl: "https://test", + authorizationUrl: "mockedAuthorizationEndpoint", + tokenUrl: "mockedTokenExchangeEndpoint", scopes: { mockedScopes: "mockedScopes", }, @@ -290,7 +295,7 @@ describe("CreateOauthDriver", () => { authorizationUrl: "https://test", tokenUrl: "https://test", scopes: { - mockedScopes: "mockedScopes", + mockedScopes: "mockedScope", }, }, }, @@ -344,7 +349,7 @@ describe("CreateOauthDriver", () => { clientSecret: "mockedClientSecret", authorizationEndpoint: "mockedAuthorizationEndpoint", tokenExchangeEndpoint: "mockedTokenExchangeEndpoint", - scopes: ["mockedScope"], + scopes: ["mockedScopes"], }); sinon.stub(SpecParser.prototype, "list").resolves({ APIs: [ @@ -358,8 +363,8 @@ describe("CreateOauthDriver", () => { type: "oauth2", flows: { authorizationCode: { - authorizationUrl: "https://test", - tokenUrl: "https://test", + authorizationUrl: "mockedAuthorizationEndpoint", + tokenUrl: "mockedTokenExchangeEndpoint", scopes: { mockedScopes: "mockedScopes", }, @@ -684,4 +689,111 @@ describe("CreateOauthDriver", () => { expect(result.result.error.source).to.equal("oauthUpdate"); } }); + + it("should update if tokenRefreshEndpoint and scopes are undefined", async () => { + sinon.stub(teamsDevPortalClient, "updateOauthRegistration").resolves({ + description: "mockedDescription", + targetUrlsShouldStartWith: ["https://test2"], + applicableToApps: OauthRegistrationAppType.SpecificApp, + targetAudience: OauthRegistrationTargetAudience.HomeTenant, + m365AppId: "mockedAppId", + clientId: "mockedClientId", + clientSecret: "mockedClientSecret", + authorizationEndpoint: "mockedAuthorizationEndpoint", + tokenExchangeEndpoint: "mockedTokenExchangeEndpoint", + scopes: ["mockedScope"], + isPKCEEnabled: true, + }); + sinon.stub(teamsDevPortalClient, "getOauthRegistrationById").resolves({ + oAuthConfigId: "mockedRegistrationId", + description: "mockedDescription", + targetUrlsShouldStartWith: ["https://test"], + applicableToApps: OauthRegistrationAppType.AnyApp, + targetAudience: OauthRegistrationTargetAudience.AnyTenant, + clientId: "mockedClientId", + clientSecret: "mockedClientSecret", + authorizationEndpoint: "mockedAuthorizationEndpoint", + tokenExchangeEndpoint: "mockedTokenExchangeEndpoint", + tokenRefreshEndpoint: "mockedTokenRefreshEndpoint", + scopes: ["mockedScope"], + isPKCEEnabled: false, + }); + sinon.stub(SpecParser.prototype, "list").resolves({ + APIs: [ + { + api: "api", + server: "https://test", + operationId: "get", + auth: { + name: "test", + authScheme: { + type: "oauth2", + flows: { + authorizationCode: { + authorizationUrl: "https://test", + tokenUrl: "https://test", + scopes: { + mockedScopes: "mockedScopes", + }, + }, + }, + }, + }, + isValid: true, + reason: [], + }, + { + api: "api2", + server: "https://test", + operationId: "get", + auth: { + name: "test2", + authScheme: { + type: "oauth2", + flows: { + authorizationCode: { + authorizationUrl: "https://test", + tokenUrl: "https://test", + scopes: {}, + }, + }, + }, + }, + isValid: true, + reason: [], + }, + ], + allAPICount: 1, + validAPICount: 1, + }); + sinon.stub(mockedDriverContext.ui, "confirm").callsFake(async (config) => { + expect((config as ConfirmConfig).title.includes("description")).to.be.true; + expect((config as ConfirmConfig).title.includes("applicableToApps")).to.be.true; + expect((config as ConfirmConfig).title.includes("m365AppId")).to.be.true; + expect((config as ConfirmConfig).title.includes("targetAudience")).to.be.true; + expect((config as ConfirmConfig).title.includes("isPKCEEnabled")).to.be.true; + expect((config as ConfirmConfig).title.includes("authorizationEndpoint")).to.be.true; + expect((config as ConfirmConfig).title.includes("tokenExchangeEndpoint")).to.be.true; + expect((config as ConfirmConfig).title.includes("tokenRefreshEndpoint")).to.be.true; + expect((config as ConfirmConfig).title.includes("scopes")).to.be.true; + return ok({ type: "success", value: true }); + }); + + const args: UpdateOauthArgs = { + name: "test2", + appId: "mockedAppId", + apiSpecPath: "mockedPath", + targetAudience: "HomeTenant", + applicableToApps: "SpecificApp", + configurationId: "mockedRegistrationId", + isPKCEEnabled: true, + }; + + const result = await updateOauthDriver.execute(args, mockedDriverContext); + expect(result.result.isOk()).to.be.true; + if (result.result.isOk()) { + expect(result.result.value.size).to.equal(0); + expect(result.summaries.length).to.equal(1); + } + }); });