From 16adfc971f2ec0167c7e267c07193921171f502d Mon Sep 17 00:00:00 2001 From: Dipti Pai Date: Fri, 6 Sep 2024 10:25:42 -0700 Subject: [PATCH] Address review comments Signed-off-by: Dipti Pai --- auth/auth.go | 17 ---- auth/azure/{token_provider.go => client.go} | 80 +++++++++---------- ...{token_provider_test.go => client_test.go} | 11 +-- git/credentials.go | 11 ++- git/credentials_test.go | 6 +- git/gogit/client.go | 22 ++--- git/options.go | 2 +- 7 files changed, 63 insertions(+), 86 deletions(-) delete mode 100644 auth/auth.go rename auth/azure/{token_provider.go => client.go} (76%) rename auth/azure/{token_provider_test.go => client_test.go} (88%) diff --git a/auth/auth.go b/auth/auth.go deleted file mode 100644 index b5fdec4c..00000000 --- a/auth/auth.go +++ /dev/null @@ -1,17 +0,0 @@ -/* -Copyright 2024 The Flux authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package auth diff --git a/auth/azure/token_provider.go b/auth/azure/client.go similarity index 76% rename from auth/azure/token_provider.go rename to auth/azure/client.go index 56872eb4..632b11c8 100644 --- a/auth/azure/token_provider.go +++ b/auth/azure/client.go @@ -36,55 +36,27 @@ const ( AzureDevOpsRestApiScope = "499b84ac-1321-427f-aa17-267ca6975798/.default" ) -// Provider is an authentication provider for Azure. -type Provider struct { +// Client is an authentication provider for Azure. +type Client struct { credential azcore.TokenCredential scopes []string proxyURL *url.URL } -// ProviderOptFunc enables specifying options for the provider. -type ProviderOptFunc func(*Provider) +// OptFunc enables specifying options for the provider. +type OptFunc func(*Client) -// NewProvider returns a new authentication provider for Azure. -func NewProvider(opts ...ProviderOptFunc) *Provider { - p := &Provider{} +// New returns a new authentication provider for Azure. It configures +// credentials using a default credential chain with options. +// https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azidentity#NewDefaultAzureCredential +// The default scope is to ARM endpoint in Azure Cloud. The scope is overridden +// using OptFunc. +func New(opts ...OptFunc) (*Client, error) { + p := &Client{} for _, opt := range opts { opt(p) } - return p -} -// WithCredential configures the credential to use to fetch the resource manager -// token. -func WithCredential(cred azcore.TokenCredential) ProviderOptFunc { - return func(p *Provider) { - p.credential = cred - } -} - -// WithAzureDevOpsScope() configures the scope to access Azure DevOps Rest API -// needed to access Azure DevOps Repositories -func WithAzureDevOpsScope() ProviderOptFunc { - return func(p *Provider) { - p.scopes = []string{AzureDevOpsRestApiScope} - } -} - -// WithProxyURL sets the proxy URL to use with NewDefaultAzureCredential. -func WithProxyURL(proxyURL *url.URL) ProviderOptFunc { - return func(p *Provider) { - p.proxyURL = proxyURL - } -} - -// GetToken requests an access token from Microsoft Entra ID by using a default -// credential chain. -// https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azidentity#NewDefaultAzureCredential -// The default scope is to ARM endpoint in Azure Cloud. The scope is overridden -// using ProviderOptFunc. -func (p *Provider) GetToken(ctx context.Context) (azcore.AccessToken, error) { - var accessToken azcore.AccessToken clientOpts := &azidentity.DefaultAzureCredentialOptions{} if p.proxyURL != nil { @@ -96,7 +68,7 @@ func (p *Provider) GetToken(ctx context.Context) (azcore.AccessToken, error) { if p.credential == nil { cred, err := azidentity.NewDefaultAzureCredential(clientOpts) if err != nil { - return accessToken, err + return nil, err } p.credential = cred } @@ -105,6 +77,34 @@ func (p *Provider) GetToken(ctx context.Context) (azcore.AccessToken, error) { p.scopes = []string{cloud.AzurePublic.Services[cloud.ResourceManager].Endpoint + "/" + ".default"} } + return p, nil +} + +// WithCredential configures the credential to use to fetch the resource manager +// token. +func WithCredential(cred azcore.TokenCredential) OptFunc { + return func(p *Client) { + p.credential = cred + } +} + +// WithAzureDevOpsScope() configures the scope to access Azure DevOps Rest API +// needed to access Azure DevOps Repositories +func WithAzureDevOpsScope() OptFunc { + return func(p *Client) { + p.scopes = []string{AzureDevOpsRestApiScope} + } +} + +// WithProxyURL sets the proxy URL to use with NewDefaultAzureCredential. +func WithProxyURL(proxyURL *url.URL) OptFunc { + return func(p *Client) { + p.proxyURL = proxyURL + } +} + +// GetToken +func (p *Client) GetToken(ctx context.Context) (azcore.AccessToken, error) { return p.credential.GetToken(ctx, policy.TokenRequestOptions{ Scopes: p.scopes, }) diff --git a/auth/azure/token_provider_test.go b/auth/azure/client_test.go similarity index 88% rename from auth/azure/token_provider_test.go rename to auth/azure/client_test.go index fa08a3df..600373fc 100644 --- a/auth/azure/token_provider_test.go +++ b/auth/azure/client_test.go @@ -30,7 +30,7 @@ func TestGetProviderToken(t *testing.T) { tests := []struct { name string tokenCred azcore.TokenCredential - opts []ProviderOptFunc + opts []OptFunc wantToken string wantScope string wantErr error @@ -40,7 +40,7 @@ func TestGetProviderToken(t *testing.T) { tokenCred: &FakeTokenCredential{ Token: "foo", }, - opts: []ProviderOptFunc{WithAzureDevOpsScope()}, + opts: []OptFunc{WithAzureDevOpsScope()}, wantScope: AzureDevOpsRestApiScope, wantToken: "foo", }, @@ -64,11 +64,12 @@ func TestGetProviderToken(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - provider := NewProvider(tt.opts...) - provider.credential = tt.tokenCred + tt.opts = append(tt.opts, WithCredential(tt.tokenCred)) + client, err := New(tt.opts...) + g.Expect(err).ToNot(HaveOccurred()) str := "" ctx := context.WithValue(context.TODO(), "scope", &str) - token, err := provider.GetToken(ctx) + token, err := client.GetToken(ctx) if tt.wantErr != nil { g.Expect(err).To(HaveOccurred()) diff --git a/git/credentials.go b/git/credentials.go index 7c36584f..fded3442 100644 --- a/git/credentials.go +++ b/git/credentials.go @@ -31,7 +31,7 @@ const ( // Credentials contains authentication data needed in order to access a Git // repository. type Credentials struct { - BearerToken string `json:"bearerToken,omitempty"` + BearerToken string } // GetCredentials returns authentication credentials for accessing the provided @@ -50,18 +50,23 @@ func GetCredentials(ctx context.Context, url string, providerOpts *ProviderOptio case ProviderAzure: opts := providerOpts.AzureOpts if providerOpts.AzureOpts == nil { - opts = []azure.ProviderOptFunc{ + opts = []azure.OptFunc{ azure.WithAzureDevOpsScope(), } } - azureProvider := azure.NewProvider(opts...) + azureProvider, err := azure.New(opts...) + if err != nil { + return nil, expiresOn, err + } accessToken, err := azureProvider.GetToken(ctx) if err != nil { return nil, expiresOn, err } + creds = Credentials{ BearerToken: accessToken.Token, } + return &creds, accessToken.ExpiresOn, nil default: return nil, expiresOn, fmt.Errorf("invalid provider") diff --git a/git/credentials_test.go b/git/credentials_test.go index 922bccb8..7e23ac49 100644 --- a/git/credentials_test.go +++ b/git/credentials_test.go @@ -40,7 +40,7 @@ func TestGetCredentials(t *testing.T) { url: "https://dev.azure.com/foo/bar/_git/baz", provider: &ProviderOptions{ Name: ProviderAzure, - AzureOpts: []azure.ProviderOptFunc{ + AzureOpts: []azure.OptFunc{ azure.WithCredential(&azure.FakeTokenCredential{ Token: "ado-token", ExpiresOn: expiresAt, @@ -58,7 +58,7 @@ func TestGetCredentials(t *testing.T) { url: "https://dev.azure.com/foo/bar/_git/baz", provider: &ProviderOptions{ Name: ProviderAzure, - AzureOpts: []azure.ProviderOptFunc{ + AzureOpts: []azure.OptFunc{ azure.WithCredential(&azure.FakeTokenCredential{ Token: "ado-token", ExpiresOn: expiresAt, @@ -71,7 +71,7 @@ func TestGetCredentials(t *testing.T) { wantScope: cloud.AzurePublic.Services[cloud.ResourceManager].Endpoint + "/" + ".default", }, { - name: "get credentials from azure", + name: "dummy provider", url: "https://dev.azure.com/foo/bar/_git/baz", provider: &ProviderOptions{ Name: "dummy", diff --git a/git/gogit/client.go b/git/gogit/client.go index 5f2268f9..931551ad 100644 --- a/git/gogit/client.go +++ b/git/gogit/client.go @@ -25,9 +25,6 @@ import ( "path/filepath" "time" - "github.com/fluxcd/pkg/auth/azure" - "github.com/fluxcd/pkg/git" - "github.com/fluxcd/pkg/git/repository" "github.com/go-git/go-billy/v5" "github.com/go-git/go-billy/v5/memfs" "github.com/go-git/go-billy/v5/osfs" @@ -41,6 +38,10 @@ import ( "github.com/go-git/go-git/v5/storage" "github.com/go-git/go-git/v5/storage/filesystem" "github.com/go-git/go-git/v5/storage/memory" + + "github.com/fluxcd/pkg/auth/azure" + "github.com/fluxcd/pkg/git" + "github.com/fluxcd/pkg/git/repository" ) func init() { @@ -253,7 +254,7 @@ func (g *Client) Clone(ctx context.Context, url string, cfg repository.CloneConf if g.authOpts != nil && g.authOpts.ProviderOpts != nil && g.authOpts.BearerToken == "" { if g.proxy.URL != "" { - proxyURL, err := g.getProxyURL(ctx) + proxyURL, err := g.proxy.FullURL() if err != nil { return nil, err } @@ -325,19 +326,6 @@ func (g *Client) validateUrl(u string) error { return nil } -// getProxyURL constructs the proxy URL from url address, username and password -func (g *Client) getProxyURL(ctx context.Context) (*url.URL, error) { - proxyURL, err := url.Parse(g.proxy.URL) - if err != nil { - return nil, fmt.Errorf("failed to parse proxy address '%s': %w", g.proxy.URL, err) - } - - if g.proxy.Username != "" || g.proxy.Password != "" { - proxyURL.User = url.UserPassword(g.proxy.Username, g.proxy.Password) - } - return proxyURL, nil -} - func (g *Client) writeFile(path string, reader io.Reader) error { if g.repository == nil { return git.ErrNoGitRepository diff --git a/git/options.go b/git/options.go index c7439769..6568caf0 100644 --- a/git/options.go +++ b/git/options.go @@ -55,7 +55,7 @@ type AuthOptions struct { // providers. type ProviderOptions struct { Name string - AzureOpts []azure.ProviderOptFunc + AzureOpts []azure.OptFunc } // KexAlgos hosts the key exchange algorithms to be used for SSH connections.