Refactor internal MSAL client constructors (#21117)

chlowell · web-flow · commit 9d202e9ffdfc · 2023-07-10T13:30:08.000-07:00
diff --git a/sdk/azidentity/azidentity.go b/sdk/azidentity/azidentity.go
@@ -50,46 +50,55 @@ var (
 	disableCP1 = strings.ToLower(os.Getenv("AZURE_IDENTITY_DISABLE_CP1")) == "true"
 )
 
-var getConfidentialClient = func(clientID, tenantID string, cred confidential.Credential, co *azcore.ClientOptions, additionalOpts ...confidential.Option) (confidentialClient, error) {
+type msalClientOptions struct {
+	azcore.ClientOptions
+
+	DisableInstanceDiscovery bool
+	// SendX5C applies only to confidential clients authenticating with a cert
+	SendX5C bool
+}
+
+var getConfidentialClient = func(clientID, tenantID string, cred confidential.Credential, opts msalClientOptions) (confidentialClient, error) {
 	if !validTenantID(tenantID) {
 		return confidential.Client{}, errors.New(tenantIDValidationErr)
 	}
-	authorityHost, err := setAuthorityHost(co.Cloud)
+	authorityHost, err := setAuthorityHost(opts.Cloud)
 	if err != nil {
 		return confidential.Client{}, err
 	}
 	authority := runtime.JoinPaths(authorityHost, tenantID)
 	o := []confidential.Option{
 		confidential.WithAzureRegion(os.Getenv(azureRegionalAuthorityName)),
-		confidential.WithHTTPClient(newPipelineAdapter(co)),
+		confidential.WithHTTPClient(newPipelineAdapter(&opts.ClientOptions)),
 	}
 	if !disableCP1 {
 		o = append(o, confidential.WithClientCapabilities(cp1))
 	}
-	o = append(o, additionalOpts...)
-	if strings.ToLower(tenantID) == "adfs" {
+	if opts.SendX5C {
+		o = append(o, confidential.WithX5C())
+	}
+	if opts.DisableInstanceDiscovery || strings.ToLower(tenantID) == "adfs" {
 		o = append(o, confidential.WithInstanceDiscovery(false))
 	}
 	return confidential.New(authority, clientID, cred, o...)
 }
 
-var getPublicClient = func(clientID, tenantID string, co *azcore.ClientOptions, additionalOpts ...public.Option) (public.Client, error) {
+var getPublicClient = func(clientID, tenantID string, opts msalClientOptions) (public.Client, error) {
 	if !validTenantID(tenantID) {
 		return public.Client{}, errors.New(tenantIDValidationErr)
 	}
-	authorityHost, err := setAuthorityHost(co.Cloud)
+	authorityHost, err := setAuthorityHost(opts.Cloud)
 	if err != nil {
 		return public.Client{}, err
 	}
 	o := []public.Option{
 		public.WithAuthority(runtime.JoinPaths(authorityHost, tenantID)),
-		public.WithHTTPClient(newPipelineAdapter(co)),
+		public.WithHTTPClient(newPipelineAdapter(&opts.ClientOptions)),
 	}
 	if !disableCP1 {
 		o = append(o, public.WithClientCapabilities(cp1))
 	}
-	o = append(o, additionalOpts...)
-	if strings.ToLower(tenantID) == "adfs" {
+	if opts.DisableInstanceDiscovery || strings.ToLower(tenantID) == "adfs" {
 		o = append(o, public.WithInstanceDiscovery(false))
 	}
 	return public.New(clientID, o...)
diff --git a/sdk/azidentity/client_assertion_credential.go b/sdk/azidentity/client_assertion_credential.go
@@ -56,7 +56,11 @@ func NewClientAssertionCredential(tenantID, clientID string, getAssertion func(c
 			return getAssertion(ctx)
 		},
 	)
-	c, err := getConfidentialClient(clientID, tenantID, cred, &options.ClientOptions, confidential.WithInstanceDiscovery(!options.DisableInstanceDiscovery))
+	msalOpts := msalClientOptions{
+		ClientOptions:            options.ClientOptions,
+		DisableInstanceDiscovery: options.DisableInstanceDiscovery,
+	}
+	c, err := getConfidentialClient(clientID, tenantID, cred, msalOpts)
 	if err != nil {
 		return nil, err
 	}
diff --git a/sdk/azidentity/client_certificate_credential.go b/sdk/azidentity/client_certificate_credential.go
@@ -58,12 +58,12 @@ func NewClientCertificateCredential(tenantID string, clientID string, certs []*x
 	if err != nil {
 		return nil, err
 	}
-	var o []confidential.Option
-	if options.SendCertificateChain {
-		o = append(o, confidential.WithX5C())
+	msalOpts := msalClientOptions{
+		ClientOptions:            options.ClientOptions,
+		DisableInstanceDiscovery: options.DisableInstanceDiscovery,
+		SendX5C:                  options.SendCertificateChain,
 	}
-	o = append(o, confidential.WithInstanceDiscovery(!options.DisableInstanceDiscovery))
-	c, err := getConfidentialClient(clientID, tenantID, cred, &options.ClientOptions, o...)
+	c, err := getConfidentialClient(clientID, tenantID, cred, msalOpts)
 	if err != nil {
 		return nil, err
 	}
diff --git a/sdk/azidentity/client_secret_credential.go b/sdk/azidentity/client_secret_credential.go
@@ -46,9 +46,11 @@ func NewClientSecretCredential(tenantID string, clientID string, clientSecret st
 	if err != nil {
 		return nil, err
 	}
-	c, err := getConfidentialClient(
-		clientID, tenantID, cred, &options.ClientOptions, confidential.WithInstanceDiscovery(!options.DisableInstanceDiscovery),
-	)
+	msalOpts := msalClientOptions{
+		ClientOptions:            options.ClientOptions,
+		DisableInstanceDiscovery: options.DisableInstanceDiscovery,
+	}
+	c, err := getConfidentialClient(clientID, tenantID, cred, msalOpts)
 	if err != nil {
 		return nil, err
 	}
diff --git a/sdk/azidentity/device_code_credential.go b/sdk/azidentity/device_code_credential.go
@@ -87,9 +87,11 @@ func NewDeviceCodeCredential(options *DeviceCodeCredentialOptions) (*DeviceCodeC
 		cp = *options
 	}
 	cp.init()
-	c, err := getPublicClient(
-		cp.ClientID, cp.TenantID, &cp.ClientOptions, public.WithInstanceDiscovery(!cp.DisableInstanceDiscovery),
-	)
+	msalOpts := msalClientOptions{
+		ClientOptions:            cp.ClientOptions,
+		DisableInstanceDiscovery: cp.DisableInstanceDiscovery,
+	}
+	c, err := getPublicClient(cp.ClientID, cp.TenantID, msalOpts)
 	if err != nil {
 		return nil, err
 	}
diff --git a/sdk/azidentity/interactive_browser_credential.go b/sdk/azidentity/interactive_browser_credential.go
@@ -69,7 +69,11 @@ func NewInteractiveBrowserCredential(options *InteractiveBrowserCredentialOption
 		cp = *options
 	}
 	cp.init()
-	c, err := getPublicClient(cp.ClientID, cp.TenantID, &cp.ClientOptions, public.WithInstanceDiscovery(!cp.DisableInstanceDiscovery))
+	msalOpts := msalClientOptions{
+		ClientOptions:            cp.ClientOptions,
+		DisableInstanceDiscovery: cp.DisableInstanceDiscovery,
+	}
+	c, err := getPublicClient(cp.ClientID, cp.TenantID, msalOpts)
 	if err != nil {
 		return nil, err
 	}
diff --git a/sdk/azidentity/on_behalf_of_credential.go b/sdk/azidentity/on_behalf_of_credential.go
@@ -72,12 +72,12 @@ func newOnBehalfOfCredential(tenantID, clientID, userAssertion string, cred conf
 	if options == nil {
 		options = &OnBehalfOfCredentialOptions{}
 	}
-	opts := []confidential.Option{}
-	if options.SendCertificateChain {
-		opts = append(opts, confidential.WithX5C())
+	msalOpts := msalClientOptions{
+		ClientOptions:            options.ClientOptions,
+		DisableInstanceDiscovery: options.DisableInstanceDiscovery,
+		SendX5C:                  options.SendCertificateChain,
 	}
-	opts = append(opts, confidential.WithInstanceDiscovery(!options.DisableInstanceDiscovery))
-	c, err := getConfidentialClient(clientID, tenantID, cred, &options.ClientOptions, opts...)
+	c, err := getConfidentialClient(clientID, tenantID, cred, msalOpts)
 	if err != nil {
 		return nil, err
 	}
diff --git a/sdk/azidentity/on_behalf_of_credential_test.go b/sdk/azidentity/on_behalf_of_credential_test.go
@@ -20,22 +20,21 @@ func TestOnBehalfOfCredential(t *testing.T) {
 	realGetClient := getConfidentialClient
 	t.Cleanup(func() { getConfidentialClient = realGetClient })
 	expectedAssertion := "user-assertion"
+	certs, key := allCertTests[0].certs, allCertTests[0].key
 	for _, test := range []struct {
 		ctor    func(policy.Transporter) (*OnBehalfOfCredential, error)
 		name    string
 		sendX5C bool
 	}{
 		{
 			ctor: func(tp policy.Transporter) (*OnBehalfOfCredential, error) {
-				certs, key := allCertTests[0].certs, allCertTests[0].key
 				o := OnBehalfOfCredentialOptions{ClientOptions: policy.ClientOptions{Transport: tp}}
 				return NewOnBehalfOfCredentialWithCertificate(fakeTenantID, fakeClientID, expectedAssertion, certs, key, &o)
 			},
 			name: "certificate",
 		},
 		{
 			ctor: func(tp policy.Transporter) (*OnBehalfOfCredential, error) {
-				certs, key := allCertTests[0].certs, allCertTests[0].key
 				o := OnBehalfOfCredentialOptions{ClientOptions: policy.ClientOptions{Transport: tp}, SendCertificateChain: true}
 				return NewOnBehalfOfCredentialWithCertificate(fakeTenantID, fakeClientID, expectedAssertion, certs, key, &o)
 			},
@@ -68,6 +67,9 @@ func TestOnBehalfOfCredential(t *testing.T) {
 				if assertion := r.FormValue("assertion"); assertion != expectedAssertion {
 					t.Errorf(`unexpected assertion "%s"`, assertion)
 				}
+				if test.sendX5C {
+					validateX5C(t, certs)(r)
+				}
 			}}
 			cred, err := test.ctor(&srv)
 			if err != nil {
diff --git a/sdk/azidentity/username_password_credential.go b/sdk/azidentity/username_password_credential.go
@@ -48,7 +48,11 @@ func NewUsernamePasswordCredential(tenantID string, clientID string, username st
 	if options == nil {
 		options = &UsernamePasswordCredentialOptions{}
 	}
-	c, err := getPublicClient(clientID, tenantID, &options.ClientOptions, public.WithInstanceDiscovery(!options.DisableInstanceDiscovery))
+	msalOpts := msalClientOptions{
+		ClientOptions:            options.ClientOptions,
+		DisableInstanceDiscovery: options.DisableInstanceDiscovery,
+	}
+	c, err := getPublicClient(clientID, tenantID, msalOpts)
 	if err != nil {
 		return nil, err
 	}

Original file line number	Diff line number	Diff line change
`@@ -50,46 +50,55 @@ var (`
`50`	`50`	`disableCP1 = strings.ToLower(os.Getenv("AZURE_IDENTITY_DISABLE_CP1")) == "true"`
`51`	`51`	`)`
`52`	`52`
`53`		`-var getConfidentialClient = func(clientID, tenantID string, cred confidential.Credential, co *azcore.ClientOptions, additionalOpts ...confidential.Option) (confidentialClient, error) {`
	`53`	`+type msalClientOptions struct {`
	`54`	`+ azcore.ClientOptions`
	`55`	`+`
	`56`	`+ DisableInstanceDiscovery bool`
	`57`	`+ // SendX5C applies only to confidential clients authenticating with a cert`
	`58`	`+ SendX5C bool`
	`59`	`+}`
	`60`	`+`
	`61`	`+var getConfidentialClient = func(clientID, tenantID string, cred confidential.Credential, opts msalClientOptions) (confidentialClient, error) {`
`54`	`62`	`if !validTenantID(tenantID) {`
`55`	`63`	`return confidential.Client{}, errors.New(tenantIDValidationErr)`
`56`	`64`	`}`
`57`		`- authorityHost, err := setAuthorityHost(co.Cloud)`
	`65`	`+ authorityHost, err := setAuthorityHost(opts.Cloud)`
`58`	`66`	`if err != nil {`
`59`	`67`	`return confidential.Client{}, err`
`60`	`68`	`}`
`61`	`69`	`authority := runtime.JoinPaths(authorityHost, tenantID)`
`62`	`70`	`o := []confidential.Option{`
`63`	`71`	`confidential.WithAzureRegion(os.Getenv(azureRegionalAuthorityName)),`
`64`		`- confidential.WithHTTPClient(newPipelineAdapter(co)),`
	`72`	`+ confidential.WithHTTPClient(newPipelineAdapter(&opts.ClientOptions)),`
`65`	`73`	`}`
`66`	`74`	`if !disableCP1 {`
`67`	`75`	`o = append(o, confidential.WithClientCapabilities(cp1))`
`68`	`76`	`}`
`69`		`- o = append(o, additionalOpts...)`
`70`		`- if strings.ToLower(tenantID) == "adfs" {`
	`77`	`+ if opts.SendX5C {`
	`78`	`+ o = append(o, confidential.WithX5C())`
	`79`	`+ }`
	`80`	`+ if opts.DisableInstanceDiscovery \|\| strings.ToLower(tenantID) == "adfs" {`
`71`	`81`	`o = append(o, confidential.WithInstanceDiscovery(false))`
`72`	`82`	`}`
`73`	`83`	`return confidential.New(authority, clientID, cred, o...)`
`74`	`84`	`}`
`75`	`85`
`76`		`-var getPublicClient = func(clientID, tenantID string, co *azcore.ClientOptions, additionalOpts ...public.Option) (public.Client, error) {`
	`86`	`+var getPublicClient = func(clientID, tenantID string, opts msalClientOptions) (public.Client, error) {`
`77`	`87`	`if !validTenantID(tenantID) {`
`78`	`88`	`return public.Client{}, errors.New(tenantIDValidationErr)`
`79`	`89`	`}`
`80`		`- authorityHost, err := setAuthorityHost(co.Cloud)`
	`90`	`+ authorityHost, err := setAuthorityHost(opts.Cloud)`
`81`	`91`	`if err != nil {`
`82`	`92`	`return public.Client{}, err`
`83`	`93`	`}`
`84`	`94`	`o := []public.Option{`
`85`	`95`	`public.WithAuthority(runtime.JoinPaths(authorityHost, tenantID)),`
`86`		`- public.WithHTTPClient(newPipelineAdapter(co)),`
	`96`	`+ public.WithHTTPClient(newPipelineAdapter(&opts.ClientOptions)),`
`87`	`97`	`}`
`88`	`98`	`if !disableCP1 {`
`89`	`99`	`o = append(o, public.WithClientCapabilities(cp1))`
`90`	`100`	`}`
`91`		`- o = append(o, additionalOpts...)`
`92`		`- if strings.ToLower(tenantID) == "adfs" {`
	`101`	`+ if opts.DisableInstanceDiscovery \|\| strings.ToLower(tenantID) == "adfs" {`
`93`	`102`	`o = append(o, public.WithInstanceDiscovery(false))`
`94`	`103`	`}`
`95`	`104`	`return public.New(clientID, o...)`
Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,11 @@ func NewClientAssertionCredential(tenantID, clientID string, getAssertion func(c`
`56`	`56`	`return getAssertion(ctx)`
`57`	`57`	`},`
`58`	`58`	`)`
`59`		`- c, err := getConfidentialClient(clientID, tenantID, cred, &options.ClientOptions, confidential.WithInstanceDiscovery(!options.DisableInstanceDiscovery))`
	`59`	`+ msalOpts := msalClientOptions{`
	`60`	`+ ClientOptions: options.ClientOptions,`
	`61`	`+ DisableInstanceDiscovery: options.DisableInstanceDiscovery,`
	`62`	`+ }`
	`63`	`+ c, err := getConfidentialClient(clientID, tenantID, cred, msalOpts)`
`60`	`64`	`if err != nil {`
`61`	`65`	`return nil, err`
`62`	`66`	`}`
Original file line number	Diff line number	Diff line change
`@@ -58,12 +58,12 @@ func NewClientCertificateCredential(tenantID string, clientID string, certs []*x`
`58`	`58`	`if err != nil {`
`59`	`59`	`return nil, err`
`60`	`60`	`}`
`61`		`- var o []confidential.Option`
`62`		`- if options.SendCertificateChain {`
`63`		`- o = append(o, confidential.WithX5C())`
	`61`	`+ msalOpts := msalClientOptions{`
	`62`	`+ ClientOptions: options.ClientOptions,`
	`63`	`+ DisableInstanceDiscovery: options.DisableInstanceDiscovery,`
	`64`	`+ SendX5C: options.SendCertificateChain,`
`64`	`65`	`}`
`65`		`- o = append(o, confidential.WithInstanceDiscovery(!options.DisableInstanceDiscovery))`
`66`		`- c, err := getConfidentialClient(clientID, tenantID, cred, &options.ClientOptions, o...)`
	`66`	`+ c, err := getConfidentialClient(clientID, tenantID, cred, msalOpts)`
`67`	`67`	`if err != nil {`
`68`	`68`	`return nil, err`
`69`	`69`	`}`
Original file line number	Diff line number	Diff line change
`@@ -69,7 +69,11 @@ func NewInteractiveBrowserCredential(options *InteractiveBrowserCredentialOption`
`69`	`69`	`cp = *options`
`70`	`70`	`}`
`71`	`71`	`cp.init()`
`72`		`- c, err := getPublicClient(cp.ClientID, cp.TenantID, &cp.ClientOptions, public.WithInstanceDiscovery(!cp.DisableInstanceDiscovery))`
	`72`	`+ msalOpts := msalClientOptions{`
	`73`	`+ ClientOptions: cp.ClientOptions,`
	`74`	`+ DisableInstanceDiscovery: cp.DisableInstanceDiscovery,`
	`75`	`+ }`
	`76`	`+ c, err := getPublicClient(cp.ClientID, cp.TenantID, msalOpts)`
`73`	`77`	`if err != nil {`
`74`	`78`	`return nil, err`
`75`	`79`	`}`
Original file line number	Diff line number	Diff line change
`@@ -72,12 +72,12 @@ func newOnBehalfOfCredential(tenantID, clientID, userAssertion string, cred conf`
`72`	`72`	`if options == nil {`
`73`	`73`	`options = &OnBehalfOfCredentialOptions{}`
`74`	`74`	`}`
`75`		`- opts := []confidential.Option{}`
`76`		`- if options.SendCertificateChain {`
`77`		`- opts = append(opts, confidential.WithX5C())`
	`75`	`+ msalOpts := msalClientOptions{`
	`76`	`+ ClientOptions: options.ClientOptions,`
	`77`	`+ DisableInstanceDiscovery: options.DisableInstanceDiscovery,`
	`78`	`+ SendX5C: options.SendCertificateChain,`
`78`	`79`	`}`
`79`		`- opts = append(opts, confidential.WithInstanceDiscovery(!options.DisableInstanceDiscovery))`
`80`		`- c, err := getConfidentialClient(clientID, tenantID, cred, &options.ClientOptions, opts...)`
	`80`	`+ c, err := getConfidentialClient(clientID, tenantID, cred, msalOpts)`
`81`	`81`	`if err != nil {`
`82`	`82`	`return nil, err`
`83`	`83`	`}`
Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,11 @@ func NewUsernamePasswordCredential(tenantID string, clientID string, username st`
`48`	`48`	`if options == nil {`
`49`	`49`	`options = &UsernamePasswordCredentialOptions{}`
`50`	`50`	`}`
`51`		`- c, err := getPublicClient(clientID, tenantID, &options.ClientOptions, public.WithInstanceDiscovery(!options.DisableInstanceDiscovery))`
	`51`	`+ msalOpts := msalClientOptions{`
	`52`	`+ ClientOptions: options.ClientOptions,`
	`53`	`+ DisableInstanceDiscovery: options.DisableInstanceDiscovery,`
	`54`	`+ }`
	`55`	`+ c, err := getPublicClient(clientID, tenantID, msalOpts)`
`52`	`56`	`if err != nil {`
`53`	`57`	`return nil, err`
`54`	`58`	`}`