ManagedIdentityCredential should not do in-place mutation of Scopes

[kostrse]

Bug Report

ManagedIdentityCredential receives a list of scopes as input and internally converts them to a resource ID (#14693).

There's a problem that ManagedIdentityCredential is actually mutates the original array, which creates an unexpected side-effect for the calling code:

var credential azcore.TokenCredential
var token *azcore.AccessToken
var err error

clientId := ""
scopes := []string { "htttps://management.azure.com/.default" }

if credential, err = azidentity.NewManagedIdentityCredential(clientId, nil); err != nil {
	return err
}

fmt.Printf("scopes: %s\n", scopes)
// scopes: [htttps://management.azure.com/.default]

if token, err = c.credential.GetToken(ctx, azcore.TokenRequestOptions{Scopes: scopes}); err != nil {
	return err
}

fmt.Printf("scopes: %s\n", scopes)
// scopes: [htttps://management.azure.com] <-- INPUT ARRAY HAS CHANGED

This line is responsible for in-place mutation of the input array.

Mutation should not happen since conversion from scopes to resource ID is implementation detail which should not affect calling code.

[kostrse]

I my opinion you should not do any transformations with scopes in ManagedIdentityCredential and instead convert the scopes to resource ID in createXxxRequest methods of managed_identity_client.

managed_identity_client supports several implementations of token endpoints and whether a given token endpoint supports scopes or not is implementation detail of that particular endpoint. It just happens now that today no one endpoint actually supports scopes, but it may change in future.

So, createXxxRequest impelementations that actually support scopes (in future), would be able to pass scopes as is, and endpoints which still do not support scopes, would continue converting them to resource ID, on case by case basis.

[chlowell]

Thanks, that's a good suggestion. I also find the division of responsibilities between ManagedIdentityCredential and managedIdentityClient somewhat awkward. I left that division alone in my PR because I would probably want to refactor more than the scope -> resource conversion, and there's no need for that today. One endpoint diverging from the others in the way you describe would be a good excuse for a larger refactoring though.