Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for authfilecredential #7044

Closed
wants to merge 6 commits into from
Closed

Conversation

Luyunmt
Copy link

@Luyunmt Luyunmt commented Jan 21, 2020

Azure/azure-sdk-for-net#9312

The purpose of this PR is explained in this or a referenced issue.
Add Azure AuthFile credential into azidentity.

The PR does not update generated files.
These files are managed by the codegen framework at [Azure/autorest.go][].
Don't include any generated files.

The PR targets the latest branch.
This PR would target track2 branch.

Tests are included and/or updated for code changes.
unit tests are included.

Updates to [CHANGELOG.md][] are included.
No any packages changed, so it don't include the CHANGELOG.md

Apache v2 license headers are included in each file.
Included
[Azure/autorest.go]: https://github.com/Azure/autorest.go
[CHANGELOG.md]: https://github.com/Azure/azure-sdk-for-go/blob/master/CHANGELOG.md

sdk/azidentity/auth_file_credential.go Outdated Show resolved Hide resolved
sdk/azidentity/auth_file_credential.go Outdated Show resolved Hide resolved
sdk/azidentity/auth_file_credential.go Outdated Show resolved Hide resolved
sdk/azidentity/auth_file_credential.go Outdated Show resolved Hide resolved
@azuresdkci
Copy link

Can one of the admins verify this patch?

@jongio
Copy link
Member

jongio commented Mar 9, 2020

@JeffreyRichter / @catalinaperalta - Can you please review these and help us get to sign-off for April release? Thanks

Copy link
Member

@JeffreyRichter JeffreyRichter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, how about we rewrite this whole file into just the code I show below. I haven't compiled or tested this but I think it should work and it is greatly simplified (just 1 function now), much easier to test, and simpler conceptually for a customer.

// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package azidentity

import (
"encoding/json"
"fmt"
"io/ioutil"
"net/url"
)

// NewClientSecretCredentialFromAuthenticationFile creates a ClientSecrentCredential initialized from the specified Authentication file.
func NewClientSecretCredentialFromAuthenticationFile(filePath string, options *TokenCredentialOptions) (*ClientSecretCredential, error) {
// Read the SDK Authentication file into memory
authData, err := ioutil.ReadFile(filePath)
if err != nil {
return nil, err
}

// Build the ClientSecretCredential from the Authentication File data
token := struct {
	clientID                       string `json:"clientId"`
	clientSecret                   string `json:"clientSecret"`
	subscriptionID                 string `json:"subscriptionId"`
	tenantID                       string `json:"tenantId"`
	activeDirectoryEndpointURL     string `json:"activeDirectoryEndpointUrl"`
	resourceManagerEndpointURL     string `json:"resourceManagerEndpointUrl"`
	activeDirectoryGraphResourceID string `json:"activeDirectoryGraphResourceId"`
	sqlManagementEndpointURL       string `json:"sqlManagementEndpointUrl"`
	galleryEndpointURL             string `json:"galleryEndpointUrl"`
	managementEndpointURL          string `json:"managementEndpointUrl"`
}{}

if err = json.Unmarshal(authData, &token); err != nil {
	return nil, err
}

// Parse string activeDirectoryEndpointUrl to a Url.
var tco = *options // Make a copy of the passed-inoption to not change the customer's options
tco.AuthorityHost, err = url.Parse(token.activeDirectoryEndpointURL)
if err != nil {
	return nil, fmt.Errorf("failed to parse ActiveDirectoryEndpointUrl in auth file: %w", err)
}
return NewClientSecretCredential(token.tenantID, token.clientID, token.clientSecret, &tco)

}


// NewAuthFileCredential creates an instance of the AuthFileCredential class based on information in given SDK Auth file.
func NewAuthFileCredential(filePath string, options *TokenCredentialOptions) (*AuthFileCredential, error) {
options, err := options.setDefaultValues()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not following our pattern for options. The pattern is that customer can call a package function to get the initial set of options (with the default already in it), then optionally modify some fields and then pass the pointer to the modified options struct into this function. OR, the customer can simply pass nil for this argument (as a convenience) and internally, the function will call the function to get the default options and just use that. Please change this code and probably change your setDefaultValues() function to a package function that just returns a pointer to a TokenCredentialOptions struct that is already initialized to defaults. This change also simplifies the error handling.

Copy link
Author

@Luyunmt Luyunmt Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function setDefaultValues is defined for TokenCredentialOptions in file azidentity.go. it is called in many other credentials. Change it to package function would affect other credentials.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@catalinaperalta & @jhendrixMSFT : Please look into setDefaultValue - it seems that this is not following our accepted options pattern.

sdk/azidentity/auth_file_credential.go Outdated Show resolved Hide resolved
sdk/azidentity/auth_file_credential.go Outdated Show resolved Hide resolved
func (c *AuthFileCredential) GetToken(ctx context.Context, opts azcore.TokenRequestOptions) (*azcore.AccessToken, error) {
err := c.ensureCredential()
if err != nil {
return nil, &AuthenticationFailedError{msg: "Error parsing SDK Auth File", inner: err}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How dos anyone know what the format of this file is supposed to be like? Can we put a link to the format in the docs somewhere and maybe even in this error message so a customer can see what's wrong and fix it.

sdk/azidentity/auth_file_credential.go Outdated Show resolved Hide resolved
sdk/azidentity/auth_file_credential.go Outdated Show resolved Hide resolved
sdk/azidentity/auth_file_credential.go Outdated Show resolved Hide resolved
@JeffreyRichter
Copy link
Member

JeffreyRichter commented Mar 9, 2020 via email

@jongio
Copy link
Member

jongio commented Mar 23, 2020

This is on hold pending further discussion

@tzhanl tzhanl closed this Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants