[Identity] DeviceCodeCredential constructor unification (Azure#14443)

DeviceCodeCredential stopped having required parameters a while ago, when we started completing the values for tenant ID and client ID by default. However, even though our guidelines establish that we should group all of the optional parameters in option bags, the signature of this constructor never changed.

As part of my recent Identity changes, I've made it so the DeviceCodeCredential constructor could also accept only receiving the optional parameters. The intention there was to not do a breaking change initially, but since we're targeting a major version release, having this constructor overload feels unnecessary.

This PR makes it so DeviceCodeCredential only accepts an options bag as the first parameter, without overloads.

While doing these changes, I was able to spot a bug I introduced in my recent changes (currently on master, but not released) by which clientId and tenantId were not properly passed through if sent only through the options bag. While this is fixed by this PR too, that fix caused the recordings not to match, as some of the recordings were using the default values for clientId and tenantId instead of the ones passed through. I've fixed the tests to match what the recordings represented, which essentially allows us to test that the default values for tenantId and clientId can work, meaning that now I'm treating the device code tests as if sometimes used the default parameters.

Fixes: Azure#14420

Author: sadasant

sdk/identity/identity/review/identity.api.md

@@ -126,7 +126,6 @@ export function deserializeAuthenticationRecord(serializedRecord: string): Authe
 // @public
 export class DeviceCodeCredential implements TokenCredential {
     constructor(options?: DeviceCodeCredentialOptions);
-    constructor(tenantId?: string, clientId?: string, userPromptCallback?: DeviceCodePromptCallback, options?: DeviceCodeCredentialOptions);
     authenticate(scopes: string | string[], options?: GetTokenOptions): Promise<AuthenticationRecord | undefined>;
     getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>;
     }

sdk/identity/identity/src/credentials/deviceCodeCredential.ts

@@ -7,11 +7,7 @@ import { MsalDeviceCode } from "../msal/nodeFlows/msalDeviceCode";
 import { MsalFlow } from "../msal/flows";
 import { AuthenticationRecord } from "../msal/types";
 import { trace } from "../util/tracing";
-import {
-  DeviceCodeCredentialOptions,
-  DeviceCodeInfo,
-  DeviceCodePromptCallback
-} from "./deviceCodeCredentialOptions";
+import { DeviceCodeCredentialOptions, DeviceCodeInfo } from "./deviceCodeCredentialOptions";
 
 const logger = credentialLogger("DeviceCodeCredential");
 
@@ -35,41 +31,13 @@ export class DeviceCodeCredential implements TokenCredential {
    * Creates an instance of DeviceCodeCredential with the details needed
    * to initiate the device code authorization flow with Azure Active Directory.
    *
-   * @param tenantId - The Azure Active Directory tenant (directory) ID or name.
-   *                   The default value is 'organizations'.
-   *                   'organizations' may be used when dealing with multi-tenant scenarios.
-   *                   Users can also pass the options as the first parameter, and skip the other parammeters entirely.
-   * @param clientId - The client (application) ID of an App Registration in the tenant.
-   *                   By default we will try to use the Azure CLI's client ID to authenticate.
-   * @param userPromptCallback - A callback function that will be invoked to show
-   *                             {@link DeviceCodeInfo} to the user. If left unassigned, we will automatically log the device code information and the authentication instructions in the console.
    * @param options - Options for configuring the client which makes the authentication requests.
    */
-  constructor(options?: DeviceCodeCredentialOptions);
-  constructor(
-    tenantId?: string,
-    clientId?: string,
-    userPromptCallback?: DeviceCodePromptCallback,
-    options?: DeviceCodeCredentialOptions
-  );
-  constructor(
-    tenantIdOrOptions?: string | DeviceCodeCredentialOptions,
-    clientId?: string,
-    userPromptCallback?: DeviceCodePromptCallback,
-    options?: DeviceCodeCredentialOptions
-  ) {
-    let tenantId: string | undefined;
-    if (typeof tenantIdOrOptions === "string") {
-      tenantId = tenantIdOrOptions;
-    } else {
-      options = tenantIdOrOptions;
-    }
+  constructor(options?: DeviceCodeCredentialOptions) {
     this.msalFlow = new MsalDeviceCode({
       ...options,
       logger,
-      clientId,
-      tenantId,
-      userPromptCallback: userPromptCallback || defaultDeviceCodePromptCallback,
+      userPromptCallback: options?.userPromptCallback || defaultDeviceCodePromptCallback,
       tokenCredentialOptions: options || {}
     });
     this.disableAutomaticAuthentication = options?.disableAutomaticAuthentication;

sdk/identity/identity/test/internal/identityClient.spec.ts

@@ -77,8 +77,9 @@ describe("IdentityClient", function() {
     );
   });
 
-  it("throws an exception when an Env AZURE_AUTHORITY_HOST using 'http' is provided", /** @this Mocha.Context */ async function() {
+  it("throws an exception when an Env AZURE_AUTHORITY_HOST using 'http' is provided", async function() {
     if (!isNode) {
+      // eslint-disable-next-line no-invalid-this
       return this.skip();
     }
     process.env.AZURE_AUTHORITY_HOST = "http://totallyinsecure.lol";

sdk/identity/identity/test/internal/node/deviceCodeCredential.spec.ts

@@ -41,7 +41,10 @@ describe("DeviceCodeCredential (internal)", function() {
     if (isLiveMode()) {
       this.skip();
     }
-    const credential = new DeviceCodeCredential(env.AZURE_TENANT_ID, env.AZURE_CLIENT_ID);
+    const credential = new DeviceCodeCredential({
+      tenantId: env.AZURE_TENANT_ID,
+      clientId: env.AZURE_CLIENT_ID
+    });
 
     await credential.getToken(scope);
     assert.equal(getTokenSilentSpy.callCount, 1);
@@ -77,8 +80,6 @@ describe("DeviceCodeCredential (internal)", function() {
     persistence?.save("");
 
     const credential = new DeviceCodeCredential({
-      tenantId: env.AZURE_TENANT_ID,
-      clientId: env.AZURE_CLIENT_ID,
       tokenCachePersistenceOptions
     });
 
@@ -113,8 +114,6 @@ describe("DeviceCodeCredential (internal)", function() {
     persistence?.save("");
 
     const credential = new DeviceCodeCredential({
-      tenantId: env.AZURE_TENANT_ID,
-      clientId: env.AZURE_CLIENT_ID,
       tokenCachePersistenceOptions
     });
 
@@ -156,8 +155,6 @@ describe("DeviceCodeCredential (internal)", function() {
     persistence?.save("");
 
     const credential = new DeviceCodeCredential({
-      tenantId: env.AZURE_TENANT_ID,
-      clientId: env.AZURE_CLIENT_ID,
       // To be able to re-use the account, the Token Cache must also have been provided.
       // TODO: Perhaps make the account parameter part of the tokenCachePersistenceOptions?
       tokenCachePersistenceOptions
@@ -169,8 +166,6 @@ describe("DeviceCodeCredential (internal)", function() {
     assert.equal(doGetTokenSpy.callCount, 1);
 
     const credential2 = new DeviceCodeCredential({
-      tenantId: env.AZURE_TENANT_ID,
-      clientId: env.AZURE_CLIENT_ID,
       authenticationRecord: account,
       // To be able to re-use the account, the Token Cache must also have been provided.
       // TODO: Perhaps make the account parameter part of the tokenCachePersistenceOptions?

sdk/identity/identity/test/public/node/deviceCodeCredential.spec.ts

@@ -37,7 +37,10 @@ describe("DeviceCodeCredential", function() {
     if (isLiveMode()) {
       this.skip();
     }
-    const credential = new DeviceCodeCredential(env.AZURE_TENANT_ID, env.AZURE_CLIENT_ID);
+    const credential = new DeviceCodeCredential({
+      tenantId: env.AZURE_TENANT_ID,
+      clientId: env.AZURE_CLIENT_ID
+    });
 
     const token = await credential.getToken(scope);
     assert.ok(token?.token);
@@ -52,7 +55,11 @@ describe("DeviceCodeCredential", function() {
     const callback: DeviceCodePromptCallback = (info) => {
       console.log("CUSTOMIZED PROMPT CALLBACK", info.message);
     };
-    const credential = new DeviceCodeCredential(env.AZURE_TENANT_ID, env.AZURE_CLIENT_ID, callback);
+    const credential = new DeviceCodeCredential({
+      tenantId: env.AZURE_TENANT_ID,
+      clientId: env.AZURE_CLIENT_ID,
+      userPromptCallback: callback
+    });
 
     const token = await credential.getToken(scope);
     assert.ok(token?.token);
@@ -61,7 +68,10 @@ describe("DeviceCodeCredential", function() {
 
   // Setting the MSAL options to cancel doesn't seem to be cancelling MSAL. I'm waiting for them to mention how to do this.
   it.skip("allows cancelling the authentication", async function() {
-    const credential = new DeviceCodeCredential(env.AZURE_TENANT_ID, env.AZURE_CLIENT_ID);
+    const credential = new DeviceCodeCredential({
+      tenantId: env.AZURE_TENANT_ID,
+      clientId: env.AZURE_CLIENT_ID
+    });
 
     const controller = new AbortController();
     const getTokenPromise = credential.getToken(scope, {
@@ -87,8 +97,6 @@ describe("DeviceCodeCredential", function() {
       this.skip();
     }
     const credential = new DeviceCodeCredential({
-      tenantId: env.AZURE_TENANT_ID,
-      clientId: env.AZURE_CLIENT_ID,
       disableAutomaticAuthentication: true
     });
 
@@ -114,7 +122,10 @@ describe("DeviceCodeCredential", function() {
     }
     await testTracing({
       test: async (spanOptions) => {
-        const credential = new DeviceCodeCredential(env.AZURE_TENANT_ID, env.AZURE_CLIENT_ID);
+        const credential = new DeviceCodeCredential({
+          tenantId: env.AZURE_TENANT_ID,
+          clientId: env.AZURE_CLIENT_ID
+        });
 
         await credential.getToken(scope, {
           tracingOptions: {