From 07c465a383d48e2c97027157612fc4fd783b3a51 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Thu, 3 Nov 2022 16:42:53 -0700 Subject: [PATCH 01/15] Adds new fics request builders --- .../graphsdk/application_request_builders.go | 29 +++- .../entity_list_request_builder_test.go | 4 +- cli/azd/pkg/graphsdk/fic_models.go | 14 ++ cli/azd/pkg/graphsdk/fic_request_builders.go | 141 ++++++++++++++++++ cli/azd/pkg/graphsdk/graph_client.go | 2 +- cli/azd/pkg/graphsdk/graph_client_test.go | 106 +++++++++++++ .../pkg/graphsdk/service_principal_models.go | 4 - .../service_principal_request_builders.go | 22 +++ 8 files changed, 314 insertions(+), 8 deletions(-) create mode 100644 cli/azd/pkg/graphsdk/fic_models.go create mode 100644 cli/azd/pkg/graphsdk/fic_request_builders.go create mode 100644 cli/azd/pkg/graphsdk/graph_client_test.go diff --git a/cli/azd/pkg/graphsdk/application_request_builders.go b/cli/azd/pkg/graphsdk/application_request_builders.go index 36116a726e1..bc27c14184e 100644 --- a/cli/azd/pkg/graphsdk/application_request_builders.go +++ b/cli/azd/pkg/graphsdk/application_request_builders.go @@ -14,7 +14,7 @@ type ApplicationListRequestBuilder struct { *EntityListRequestBuilder[ApplicationListRequestBuilder] } -func NewApplicationsRequestBuilder(client *GraphClient) *ApplicationListRequestBuilder { +func NewApplicationListRequestBuilder(client *GraphClient) *ApplicationListRequestBuilder { builder := &ApplicationListRequestBuilder{} builder.EntityListRequestBuilder = newEntityListRequestBuilder(builder, client) @@ -74,6 +74,14 @@ func NewApplicationItemRequestBuilder(client *GraphClient, id string) *Applicati return builder } +func (c *ApplicationItemRequestBuilder) FederatedIdentityCredentials() *FederatedIdentityCredentialListRequestBuilder { + return NewFederatedIdentityCredentialListRequestBuilder(c.client, c.id) +} + +func (c *ApplicationItemRequestBuilder) FederatedIdentityCredentialById(id string) *FederatedIdentityCredentialItemRequestBuilder { + return NewFederatedIdentityCredentialItemRequestBuilder(c.client, c.id, id) +} + // Gets a Microsoft Graph Application for the specified application identifier func (c *ApplicationItemRequestBuilder) Get(ctx context.Context) (*Application, error) { req, err := runtime.NewRequest(ctx, http.MethodGet, fmt.Sprintf("%s/applications/%s", c.client.host, c.id)) @@ -93,6 +101,25 @@ func (c *ApplicationItemRequestBuilder) Get(ctx context.Context) (*Application, return httputil.ReadRawResponse[Application](res) } +// Gets a Microsoft Graph Application for the specified application identifier +func (c *ApplicationItemRequestBuilder) Delete(ctx context.Context) error { + req, err := runtime.NewRequest(ctx, http.MethodDelete, fmt.Sprintf("%s/applications/%s", c.client.host, c.id)) + if err != nil { + return fmt.Errorf("failed creating request: %w", err) + } + + res, err := c.client.pipeline.Do(req) + if err != nil { + return httputil.HandleRequestError(res, err) + } + + if !runtime.HasStatusCode(res, http.StatusNoContent) { + return runtime.NewResponseError(res) + } + + return nil +} + func (c *ApplicationItemRequestBuilder) RemovePassword(ctx context.Context, keyId string) error { req, err := runtime.NewRequest( ctx, diff --git a/cli/azd/pkg/graphsdk/entity_list_request_builder_test.go b/cli/azd/pkg/graphsdk/entity_list_request_builder_test.go index 9bc79ea3ee7..16bfc972232 100644 --- a/cli/azd/pkg/graphsdk/entity_list_request_builder_test.go +++ b/cli/azd/pkg/graphsdk/entity_list_request_builder_test.go @@ -36,7 +36,7 @@ func TestEntityListRequestBuilder(t *testing.T) { expectedFilter := "displayName eq 'APPLICATION'" expectedTop := 10 - appRequestBuilder := graphsdk.NewApplicationsRequestBuilder(graphClient). + appRequestBuilder := graphsdk.NewApplicationListRequestBuilder(graphClient). Filter(expectedFilter). Top(expectedTop) @@ -56,7 +56,7 @@ func TestEntityListRequestBuilder(t *testing.T) { graphClient, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - appRequestBuilder := graphsdk.NewApplicationsRequestBuilder(graphClient) + appRequestBuilder := graphsdk.NewApplicationListRequestBuilder(graphClient) var res *http.Response ctx := runtime.WithCaptureResponse(*mockContext.Context, &res) diff --git a/cli/azd/pkg/graphsdk/fic_models.go b/cli/azd/pkg/graphsdk/fic_models.go new file mode 100644 index 00000000000..c9835a137e9 --- /dev/null +++ b/cli/azd/pkg/graphsdk/fic_models.go @@ -0,0 +1,14 @@ +package graphsdk + +type FederatedIdentityCredentialListResponse struct { + Value []FederatedIdentityCredential `json:"value"` +} + +type FederatedIdentityCredential struct { + Id *string `json:"id"` + Name string `json:"name"` + Issuer string `json:"issuer"` + Subject string `json:"subject"` + Description *string `json:"description"` + Audiences []string `json:"audiences"` +} diff --git a/cli/azd/pkg/graphsdk/fic_request_builders.go b/cli/azd/pkg/graphsdk/fic_request_builders.go new file mode 100644 index 00000000000..c3fb965c2a3 --- /dev/null +++ b/cli/azd/pkg/graphsdk/fic_request_builders.go @@ -0,0 +1,141 @@ +package graphsdk + +import ( + "context" + "fmt" + "net/http" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" + "github.com/azure/azure-dev/cli/azd/pkg/httputil" +) + +type FederatedIdentityCredentialListRequestBuilder struct { + *EntityListRequestBuilder[FederatedIdentityCredentialListRequestBuilder] + applicationId string +} + +func NewFederatedIdentityCredentialListRequestBuilder(client *GraphClient, applicationId string) *FederatedIdentityCredentialListRequestBuilder { + builder := &FederatedIdentityCredentialListRequestBuilder{ + applicationId: applicationId, + } + builder.EntityListRequestBuilder = newEntityListRequestBuilder(builder, client) + + return builder +} + +// Gets a list of applications that the current logged in user has access to. +func (c *FederatedIdentityCredentialListRequestBuilder) Get(ctx context.Context) (*FederatedIdentityCredentialListResponse, error) { + req, err := c.createRequest(ctx, http.MethodGet, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials", c.client.host, c.applicationId)) + if err != nil { + return nil, fmt.Errorf("failed creating request: %w", err) + } + + res, err := c.client.pipeline.Do(req) + if err != nil { + return nil, httputil.HandleRequestError(res, err) + } + + if !runtime.HasStatusCode(res, http.StatusOK) { + return nil, runtime.NewResponseError(res) + } + + return httputil.ReadRawResponse[FederatedIdentityCredentialListResponse](res) +} + +func (c *FederatedIdentityCredentialListRequestBuilder) Post(ctx context.Context, federatedIdentityCredential *FederatedIdentityCredential) (*FederatedIdentityCredential, error) { + req, err := c.createRequest(ctx, http.MethodPost, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials", c.client.host, c.applicationId)) + if err != nil { + return nil, fmt.Errorf("failed creating request: %w", err) + } + + err = SetHttpRequestBody(req, federatedIdentityCredential) + if err != nil { + return nil, err + } + + res, err := c.client.pipeline.Do(req) + if err != nil { + return nil, httputil.HandleRequestError(res, err) + } + + if !runtime.HasStatusCode(res, http.StatusCreated) { + return nil, runtime.NewResponseError(res) + } + + return httputil.ReadRawResponse[FederatedIdentityCredential](res) +} + +type FederatedIdentityCredentialItemRequestBuilder struct { + *EntityItemRequestBuilder[FederatedIdentityCredentialItemRequestBuilder] + applicationId string +} + +func NewFederatedIdentityCredentialItemRequestBuilder(client *GraphClient, applicationId string, id string) *FederatedIdentityCredentialItemRequestBuilder { + builder := &FederatedIdentityCredentialItemRequestBuilder{ + applicationId: applicationId, + } + builder.EntityItemRequestBuilder = newEntityItemRequestBuilder(builder, client, id) + + return builder +} + +// Gets a Microsoft Graph Application for the specified application identifier +func (c *FederatedIdentityCredentialItemRequestBuilder) Get(ctx context.Context) (*FederatedIdentityCredential, error) { + req, err := runtime.NewRequest(ctx, http.MethodGet, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials/%s", c.client.host, c.applicationId, c.id)) + if err != nil { + return nil, fmt.Errorf("failed creating request: %w", err) + } + + res, err := c.client.pipeline.Do(req) + if err != nil { + return nil, httputil.HandleRequestError(res, err) + } + + if !runtime.HasStatusCode(res, http.StatusOK) { + return nil, runtime.NewResponseError(res) + } + + return httputil.ReadRawResponse[FederatedIdentityCredential](res) +} + +func (c *FederatedIdentityCredentialItemRequestBuilder) Update(ctx context.Context, federatedIdentityCredential *FederatedIdentityCredential) error { + req, err := runtime.NewRequest(ctx, http.MethodPatch, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials/%s", c.client.host, c.applicationId, c.id)) + if err != nil { + return fmt.Errorf("failed creating request: %w", err) + } + + err = SetHttpRequestBody(req, federatedIdentityCredential) + if err != nil { + return err + } + + res, err := c.client.pipeline.Do(req) + if err != nil { + return httputil.HandleRequestError(res, err) + } + + if !runtime.HasStatusCode(res, http.StatusNoContent) { + return runtime.NewResponseError(res) + } + + return nil +} + +// Gets a Microsoft Graph Application for the specified application identifier +func (c *FederatedIdentityCredentialItemRequestBuilder) Delete(ctx context.Context) error { + req, err := runtime.NewRequest(ctx, http.MethodDelete, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials/%s", c.client.host, c.applicationId, c.id)) + if err != nil { + return fmt.Errorf("failed creating request: %w", err) + } + + res, err := c.client.pipeline.Do(req) + if err != nil { + return httputil.HandleRequestError(res, err) + } + + if !runtime.HasStatusCode(res, http.StatusNoContent) { + return runtime.NewResponseError(res) + } + + return nil +} diff --git a/cli/azd/pkg/graphsdk/graph_client.go b/cli/azd/pkg/graphsdk/graph_client.go index 3b0f63bcd51..cebb75c8f2a 100644 --- a/cli/azd/pkg/graphsdk/graph_client.go +++ b/cli/azd/pkg/graphsdk/graph_client.go @@ -35,7 +35,7 @@ func (c *GraphClient) Me() *MeItemRequestBuilder { // Applications func (c *GraphClient) Applications() *ApplicationListRequestBuilder { - return NewApplicationsRequestBuilder(c) + return NewApplicationListRequestBuilder(c) } func (c *GraphClient) ApplicationById(id string) *ApplicationItemRequestBuilder { diff --git a/cli/azd/pkg/graphsdk/graph_client_test.go b/cli/azd/pkg/graphsdk/graph_client_test.go new file mode 100644 index 00000000000..cc79265a4eb --- /dev/null +++ b/cli/azd/pkg/graphsdk/graph_client_test.go @@ -0,0 +1,106 @@ +package graphsdk_test + +import ( + "context" + "errors" + "net/http" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/azure/azure-dev/cli/azd/pkg/azsdk" + "github.com/azure/azure-dev/cli/azd/pkg/convert" + "github.com/azure/azure-dev/cli/azd/pkg/graphsdk" + "github.com/stretchr/testify/require" +) + +// Testing for temp integration testing during development +func Test_GraphClientRequest(t *testing.T) { + ctx := context.Background() + credential, err := azidentity.NewAzureCLICredential(nil) + require.NoError(t, err) + + clientOptions := azsdk.NewClientOptionsBuilder().BuildCoreClientOptions() + client, err := graphsdk.NewGraphClient(credential, clientOptions) + require.NoError(t, err) + require.NotNil(t, client) + + appsResponse, err := client. + Applications(). + Filter("displayName eq 'wabrez-spn-go-test'"). + Get(ctx) + + require.NoError(t, err) + require.NotNil(t, appsResponse) + + application := appsResponse.Value[0] + + ficsResponse, err := client. + ApplicationById(*application.Id). + FederatedIdentityCredentials(). + Get(ctx) + + require.NoError(t, err) + require.NotNil(t, ficsResponse) + + var fic graphsdk.FederatedIdentityCredential + + if len(ficsResponse.Value) == 0 { + createFic := graphsdk.FederatedIdentityCredential{ + Name: "mainfic", + Issuer: "https://token.actions.githubusercontent.com", + Subject: "repo:${REPO}:ref:refs/heads/main", + Description: convert.RefOf("main"), + Audiences: []string{ + "api://AzureADTokenExchange", + }, + } + + newFic, err := client. + ApplicationById(*application.Id). + FederatedIdentityCredentials(). + Post(ctx, &createFic) + + require.NoError(t, err) + require.NotNil(t, newFic) + + fic = *newFic + } else { + fic = ficsResponse.Value[0] + } + + existingFic, err := client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById(*fic.Id). + Get(ctx) + + require.NoError(t, err) + require.NotNil(t, existingFic) + + existingFic.Description = convert.RefOf("updated") + err = client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById(*fic.Id). + Update(ctx, existingFic) + + require.NoError(t, err) + + err = client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById(*fic.Id). + Delete(ctx) + + require.NoError(t, err) + + getFic, err := client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById(*fic.Id). + Get(ctx) + + require.Error(t, err) + require.Nil(t, getFic) + + var httpErr *azcore.ResponseError + require.True(t, errors.As(err, &httpErr)) + require.Equal(t, http.StatusNotFound, httpErr.StatusCode) +} diff --git a/cli/azd/pkg/graphsdk/service_principal_models.go b/cli/azd/pkg/graphsdk/service_principal_models.go index 39ad31ed318..a01fba65b97 100644 --- a/cli/azd/pkg/graphsdk/service_principal_models.go +++ b/cli/azd/pkg/graphsdk/service_principal_models.go @@ -19,7 +19,3 @@ type ServicePrincipalCreateRequest struct { type ServicePrincipalListResponse struct { Value []ServicePrincipal `json:"value"` } - -type ServicePrincipalListRequestBuilder struct { - *EntityListRequestBuilder[ServicePrincipalListRequestBuilder] -} diff --git a/cli/azd/pkg/graphsdk/service_principal_request_builders.go b/cli/azd/pkg/graphsdk/service_principal_request_builders.go index 2a431a10e4f..fbae80c6b89 100644 --- a/cli/azd/pkg/graphsdk/service_principal_request_builders.go +++ b/cli/azd/pkg/graphsdk/service_principal_request_builders.go @@ -9,6 +9,10 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/httputil" ) +type ServicePrincipalListRequestBuilder struct { + *EntityListRequestBuilder[ServicePrincipalListRequestBuilder] +} + func NewServicePrincipalListRequestBuilder(client *GraphClient) *ServicePrincipalListRequestBuilder { builder := &ServicePrincipalListRequestBuilder{} builder.EntityListRequestBuilder = newEntityListRequestBuilder(builder, client) @@ -94,3 +98,21 @@ func (b *ServicePrincipalItemRequestBuilder) Get(ctx context.Context) (*ServiceP return httputil.ReadRawResponse[ServicePrincipal](res) } + +func (b *ServicePrincipalItemRequestBuilder) Delete(ctx context.Context) error { + req, err := b.createRequest(ctx, http.MethodDelete, fmt.Sprintf("%s/servicePrincipals/%s", b.client.host, b.id)) + if err != nil { + return fmt.Errorf("failed creating request: %w", err) + } + + res, err := b.client.pipeline.Do(req) + if err != nil { + return httputil.HandleRequestError(res, err) + } + + if !runtime.HasStatusCode(res, http.StatusNoContent) { + return runtime.NewResponseError(res) + } + + return nil +} From 8f30ce38c9e95d275cb6032518e91fd5787338de Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Thu, 3 Nov 2022 16:43:56 -0700 Subject: [PATCH 02/15] Updates long lines --- .../graphsdk/application_request_builders.go | 4 +- cli/azd/pkg/graphsdk/fic_request_builders.go | 55 +++++++++++++++---- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/cli/azd/pkg/graphsdk/application_request_builders.go b/cli/azd/pkg/graphsdk/application_request_builders.go index bc27c14184e..91fa6693e24 100644 --- a/cli/azd/pkg/graphsdk/application_request_builders.go +++ b/cli/azd/pkg/graphsdk/application_request_builders.go @@ -78,7 +78,9 @@ func (c *ApplicationItemRequestBuilder) FederatedIdentityCredentials() *Federate return NewFederatedIdentityCredentialListRequestBuilder(c.client, c.id) } -func (c *ApplicationItemRequestBuilder) FederatedIdentityCredentialById(id string) *FederatedIdentityCredentialItemRequestBuilder { +func (c *ApplicationItemRequestBuilder) FederatedIdentityCredentialById( + id string, +) *FederatedIdentityCredentialItemRequestBuilder { return NewFederatedIdentityCredentialItemRequestBuilder(c.client, c.id, id) } diff --git a/cli/azd/pkg/graphsdk/fic_request_builders.go b/cli/azd/pkg/graphsdk/fic_request_builders.go index c3fb965c2a3..891120c3f1d 100644 --- a/cli/azd/pkg/graphsdk/fic_request_builders.go +++ b/cli/azd/pkg/graphsdk/fic_request_builders.go @@ -14,7 +14,10 @@ type FederatedIdentityCredentialListRequestBuilder struct { applicationId string } -func NewFederatedIdentityCredentialListRequestBuilder(client *GraphClient, applicationId string) *FederatedIdentityCredentialListRequestBuilder { +func NewFederatedIdentityCredentialListRequestBuilder( + client *GraphClient, + applicationId string, +) *FederatedIdentityCredentialListRequestBuilder { builder := &FederatedIdentityCredentialListRequestBuilder{ applicationId: applicationId, } @@ -24,8 +27,14 @@ func NewFederatedIdentityCredentialListRequestBuilder(client *GraphClient, appli } // Gets a list of applications that the current logged in user has access to. -func (c *FederatedIdentityCredentialListRequestBuilder) Get(ctx context.Context) (*FederatedIdentityCredentialListResponse, error) { - req, err := c.createRequest(ctx, http.MethodGet, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials", c.client.host, c.applicationId)) +func (c *FederatedIdentityCredentialListRequestBuilder) Get( + ctx context.Context, +) (*FederatedIdentityCredentialListResponse, error) { + req, err := c.createRequest( + ctx, + http.MethodGet, + fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials", c.client.host, c.applicationId), + ) if err != nil { return nil, fmt.Errorf("failed creating request: %w", err) } @@ -42,8 +51,15 @@ func (c *FederatedIdentityCredentialListRequestBuilder) Get(ctx context.Context) return httputil.ReadRawResponse[FederatedIdentityCredentialListResponse](res) } -func (c *FederatedIdentityCredentialListRequestBuilder) Post(ctx context.Context, federatedIdentityCredential *FederatedIdentityCredential) (*FederatedIdentityCredential, error) { - req, err := c.createRequest(ctx, http.MethodPost, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials", c.client.host, c.applicationId)) +func (c *FederatedIdentityCredentialListRequestBuilder) Post( + ctx context.Context, + federatedIdentityCredential *FederatedIdentityCredential, +) (*FederatedIdentityCredential, error) { + req, err := c.createRequest( + ctx, + http.MethodPost, + fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials", c.client.host, c.applicationId), + ) if err != nil { return nil, fmt.Errorf("failed creating request: %w", err) } @@ -70,7 +86,11 @@ type FederatedIdentityCredentialItemRequestBuilder struct { applicationId string } -func NewFederatedIdentityCredentialItemRequestBuilder(client *GraphClient, applicationId string, id string) *FederatedIdentityCredentialItemRequestBuilder { +func NewFederatedIdentityCredentialItemRequestBuilder( + client *GraphClient, + applicationId string, + id string, +) *FederatedIdentityCredentialItemRequestBuilder { builder := &FederatedIdentityCredentialItemRequestBuilder{ applicationId: applicationId, } @@ -81,7 +101,11 @@ func NewFederatedIdentityCredentialItemRequestBuilder(client *GraphClient, appli // Gets a Microsoft Graph Application for the specified application identifier func (c *FederatedIdentityCredentialItemRequestBuilder) Get(ctx context.Context) (*FederatedIdentityCredential, error) { - req, err := runtime.NewRequest(ctx, http.MethodGet, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials/%s", c.client.host, c.applicationId, c.id)) + req, err := runtime.NewRequest( + ctx, + http.MethodGet, + fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials/%s", c.client.host, c.applicationId, c.id), + ) if err != nil { return nil, fmt.Errorf("failed creating request: %w", err) } @@ -98,8 +122,15 @@ func (c *FederatedIdentityCredentialItemRequestBuilder) Get(ctx context.Context) return httputil.ReadRawResponse[FederatedIdentityCredential](res) } -func (c *FederatedIdentityCredentialItemRequestBuilder) Update(ctx context.Context, federatedIdentityCredential *FederatedIdentityCredential) error { - req, err := runtime.NewRequest(ctx, http.MethodPatch, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials/%s", c.client.host, c.applicationId, c.id)) +func (c *FederatedIdentityCredentialItemRequestBuilder) Update( + ctx context.Context, + federatedIdentityCredential *FederatedIdentityCredential, +) error { + req, err := runtime.NewRequest( + ctx, + http.MethodPatch, + fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials/%s", c.client.host, c.applicationId, c.id), + ) if err != nil { return fmt.Errorf("failed creating request: %w", err) } @@ -123,7 +154,11 @@ func (c *FederatedIdentityCredentialItemRequestBuilder) Update(ctx context.Conte // Gets a Microsoft Graph Application for the specified application identifier func (c *FederatedIdentityCredentialItemRequestBuilder) Delete(ctx context.Context) error { - req, err := runtime.NewRequest(ctx, http.MethodDelete, fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials/%s", c.client.host, c.applicationId, c.id)) + req, err := runtime.NewRequest( + ctx, + http.MethodDelete, + fmt.Sprintf("%s/applications/%s/federatedIdentityCredentials/%s", c.client.host, c.applicationId, c.id), + ) if err != nil { return fmt.Errorf("failed creating request: %w", err) } From f6490b0e5688dbb3b46d6246aabcbcf8469c745b Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Fri, 4 Nov 2022 13:21:09 -0700 Subject: [PATCH 03/15] Adds support for oidc federeated credentials in pipeline config --- cli/azd/cmd/pipeline.go | 6 + .../pkg/commands/pipeline/azdo_provider.go | 1 + .../pkg/commands/pipeline/github_provider.go | 172 +++++++++++++++++- cli/azd/pkg/commands/pipeline/pipeline.go | 4 +- .../pkg/commands/pipeline/pipeline_manager.go | 9 + cli/azd/pkg/graphsdk/graph_client_test.go | 2 +- 6 files changed, 187 insertions(+), 7 deletions(-) diff --git a/cli/azd/cmd/pipeline.go b/cli/azd/cmd/pipeline.go index d41d38eb47d..1880375a484 100644 --- a/cli/azd/cmd/pipeline.go +++ b/cli/azd/cmd/pipeline.go @@ -35,6 +35,12 @@ func (pc *pipelineConfigFlags) Bind(local *pflag.FlagSet, global *internal.Globa "origin", "The name of the git remote to configure the pipeline to run on.", ) + local.StringVar( + &pc.PipelineAuthTypeName, + "auth-type", + "", + "The authentication type used between the pipeline provider and Azure for deployment (Only valid for GitHub provider)", + ) local.StringVar(&pc.PipelineRoleName, "principal-role", "Contributor", "The role to assign to the service principal.") local.StringVar(&pc.PipelineProvider, "provider", "", "The pipeline provider to use (GitHub and Azdo supported).") pc.global = global diff --git a/cli/azd/pkg/commands/pipeline/azdo_provider.go b/cli/azd/pkg/commands/pipeline/azdo_provider.go index ee881dc9800..b9d33c4750f 100644 --- a/cli/azd/pkg/commands/pipeline/azdo_provider.go +++ b/cli/azd/pkg/commands/pipeline/azdo_provider.go @@ -596,6 +596,7 @@ func (p *AzdoCiProvider) configureConnection( repoDetails *gitRepositoryDetails, provisioningProvider provisioning.Options, credentials json.RawMessage, + authType PipelineAuthType, console input.Console) error { azureCredentials, err := parseCredentials(ctx, credentials) diff --git a/cli/azd/pkg/commands/pipeline/github_provider.go b/cli/azd/pkg/commands/pipeline/github_provider.go index 06aeabf5a2d..6a7e362c647 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider.go +++ b/cli/azd/pkg/commands/pipeline/github_provider.go @@ -14,12 +14,18 @@ import ( "regexp" "strings" + "github.com/azure/azure-dev/cli/azd/pkg/azsdk" + "github.com/azure/azure-dev/cli/azd/pkg/convert" "github.com/azure/azure-dev/cli/azd/pkg/environment" githubRemote "github.com/azure/azure-dev/cli/azd/pkg/github" + "github.com/azure/azure-dev/cli/azd/pkg/graphsdk" + "github.com/azure/azure-dev/cli/azd/pkg/httputil" + "github.com/azure/azure-dev/cli/azd/pkg/identity" "github.com/azure/azure-dev/cli/azd/pkg/infra/provisioning" "github.com/azure/azure-dev/cli/azd/pkg/input" "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/azure/azure-dev/cli/azd/pkg/tools" + "github.com/azure/azure-dev/cli/azd/pkg/tools/azcli" "github.com/azure/azure-dev/cli/azd/pkg/tools/git" "github.com/azure/azure-dev/cli/azd/pkg/tools/github" ) @@ -358,17 +364,54 @@ func (p *GitHubCiProvider) configureConnection( repoDetails *gitRepositoryDetails, infraOptions provisioning.Options, credentials json.RawMessage, + authType PipelineAuthType, console input.Console) error { repoSlug := repoDetails.owner + "/" + repoDetails.repoName console.Message(ctx, fmt.Sprintf("Configuring repository %s.\n", repoSlug)) - console.Message(ctx, "Setting AZURE_CREDENTIALS GitHub repo secret.\n") ghCli := github.NewGitHubCli(ctx) if err := p.ensureAuthorizedForRepoSecrets(ctx, ghCli, console, repoSlug); err != nil { return fmt.Errorf("ensuring authorization: %w", err) } + // Default to OIDC if not specified + if authType == "" { + authType = AuthTypeOidc + } + + // OIDC + Terraform is not a valid combination - warn user. + if authType == AuthTypeOidc && infraOptions.Provider == provisioning.Terraform { + console.Message(ctx, output.WithWarningFormat("WARNING: Terraform provisioning does not support OIDC authentication, defaulting to Service Principal with client secret.\n")) + authType = AuthTypeClientSecret + } + + var authErr error + + switch authType { + case AuthTypeClientSecret: + authErr = p.configureClientSecretAuth(ctx, azdEnvironment, infraOptions, repoSlug, credentials, console) + default: + authErr = p.configureOidcAuth(ctx, azdEnvironment, infraOptions, repoSlug, credentials, console) + } + + if authErr != nil { + return fmt.Errorf("failed configuring authentication: %w", authErr) + } + + console.Message(ctx, fmt.Sprintf( + `GitHub Action secrets are now configured. + See your .github/workflows folder for details on which actions will be enabled. + You can view the GitHub Actions here: https://github.com/%s/actions`, repoSlug)) + + return nil +} + +// Configures Github for standard Service Principal authentication with client id & secret +func (p *GitHubCiProvider) configureClientSecretAuth(ctx context.Context, azdEnvironment *environment.Environment, infraOptions provisioning.Options, repoSlug string, credentials json.RawMessage, console input.Console) error { + ghCli := github.NewGitHubCli(ctx) + console.Message(ctx, "Setting AZURE_CREDENTIALS GitHub repo secret.\n") + // set azure credential for pipelines can log in to Azure if err := ghCli.SetSecret(ctx, repoSlug, "AZURE_CREDENTIALS", string(credentials)); err != nil { return fmt.Errorf("failed setting AZURE_CREDENTIALS secret: %w", err) @@ -431,10 +474,96 @@ func (p *GitHubCiProvider) configureConnection( } } - console.Message(ctx, fmt.Sprintf( - `GitHub Action secrets are now configured. - See your .github/workflows folder for details on which actions will be enabled. - You can view the GitHub Actions here: https://github.com/%s/actions`, repoSlug)) + return nil +} + +// Configures Github for OIDC authentication using registered application with federated identity credentials +func (p *GitHubCiProvider) configureOidcAuth(ctx context.Context, azdEnvironment *environment.Environment, infraOptions provisioning.Options, repoSlug string, credentials json.RawMessage, console input.Console) error { + ghCli := github.NewGitHubCli(ctx) + + var azureCredentials azcli.AzureCredentials + if err := json.Unmarshal(credentials, &azureCredentials); err != nil { + return fmt.Errorf("failed unmarshalling azure credentials: %w", err) + } + + err := applyFederatedCredentials(ctx, repoSlug, &azureCredentials) + if err != nil { + return err + } + + githubSecrets := map[string]string{ + environment.EnvNameEnvVarName: azdEnvironment.GetEnvName(), + environment.LocationEnvVarName: azdEnvironment.GetLocation(), + environment.TenantIdEnvVarName: azureCredentials.TenantId, + environment.SubscriptionIdEnvVarName: azureCredentials.SubscriptionId, + "AZURE_CLIENT_ID": azureCredentials.ClientId, + } + + for key, value := range githubSecrets { + console.Message(ctx, fmt.Sprintf("Setting %s GitHub repo secret.\n", key)) + if err := ghCli.SetSecret(ctx, repoSlug, key, value); err != nil { + return fmt.Errorf("failed setting github secret '%s': %w", key, err) + } + } + + return nil +} + +const ( + federatedIdentityIssuer = "https://token.actions.githubusercontent.com" + federatedIdentityAudience = "api://AzureADTokenExchange" +) + +func applyFederatedCredentials(ctx context.Context, repoSlug string, azureCredentails *azcli.AzureCredentials) error { + graphClient, err := createGraphClient(ctx) + if err != nil { + return err + } + + appsResponse, err := graphClient. + Applications(). + Filter(fmt.Sprintf("appId eq '%s'", azureCredentails.ClientId)). + Get(ctx) + if err != nil || len(appsResponse.Value) == 0 { + return fmt.Errorf("failed finding matching application: %w", err) + } + + application := appsResponse.Value[0] + + existingCredsResponse, err := graphClient. + ApplicationById(*application.Id). + FederatedIdentityCredentials(). + Get(ctx) + + if err != nil { + return fmt.Errorf("failed retrieving federated credentials: %w", err) + } + + // List of desired federated credentials + federatedCredentials := []graphsdk.FederatedIdentityCredential{ + { + Name: "main", + Issuer: federatedIdentityIssuer, + Subject: fmt.Sprintf("repo:%s:ref:refs/heads/main", repoSlug), + Description: convert.RefOf("Created by Azure Developer CLI"), + Audiences: []string{federatedIdentityAudience}, + }, + { + Name: "pull_request", + Issuer: federatedIdentityIssuer, + Subject: fmt.Sprintf("repo:%s:pull_request", repoSlug), + Description: convert.RefOf("Created by Azure Developer CLI"), + Audiences: []string{federatedIdentityAudience}, + }, + } + + // Ensure the credential exists otherwise create a new one. + for _, fic := range federatedCredentials { + err := ensureFederatedCredential(ctx, graphClient, &application, existingCredsResponse.Value, &fic) + if err != nil { + return err + } + } return nil } @@ -591,3 +720,36 @@ func getRemoteUrlFromPrompt(ctx context.Context, remoteName string, console inpu return remoteUrl, nil } + +// Ensures that the federated credential exists on the application otherwise create a new one +func ensureFederatedCredential(ctx context.Context, graphClient *graphsdk.GraphClient, application *graphsdk.Application, existingCredentials []graphsdk.FederatedIdentityCredential, repoCredential *graphsdk.FederatedIdentityCredential) error { + // If a federated credential already exists for the same subject then nothing to do. + for _, existing := range existingCredentials { + if existing.Subject == repoCredential.Subject { + log.Printf("federated credential with subject '%s' already exists on application '%s'", repoCredential.Subject, *application.Id) + return nil + } + } + + // Otherwise create the new federated credential + _, err := graphClient. + ApplicationById(*application.Id). + FederatedIdentityCredentials(). + Post(ctx, repoCredential) + + if err != nil { + return fmt.Errorf("failed created federated credential: %w", err) + } + + return nil +} + +func createGraphClient(ctx context.Context) (*graphsdk.GraphClient, error) { + creds := identity.GetCredentials(ctx) + graphOptions := azsdk. + NewClientOptionsBuilder(). + WithTransport(httputil.GetHttpClient(ctx)). + BuildCoreClientOptions() + + return graphsdk.NewGraphClient(creds, graphOptions) +} diff --git a/cli/azd/pkg/commands/pipeline/pipeline.go b/cli/azd/pkg/commands/pipeline/pipeline.go index 0cf5ff72be9..5fae941df25 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline.go +++ b/cli/azd/pkg/commands/pipeline/pipeline.go @@ -90,7 +90,9 @@ type CiProvider interface { gitRepo *gitRepositoryDetails, provisioningProvider provisioning.Options, credential json.RawMessage, - console input.Console) error + authType PipelineAuthType, + console input.Console, + ) error } func folderExists(folderPath string) bool { diff --git a/cli/azd/pkg/commands/pipeline/pipeline_manager.go b/cli/azd/pkg/commands/pipeline/pipeline_manager.go index 5cbeb7eea90..9f15fbbddb0 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline_manager.go +++ b/cli/azd/pkg/commands/pipeline/pipeline_manager.go @@ -21,11 +21,19 @@ import ( "github.com/sethvargo/go-retry" ) +type PipelineAuthType string + +const ( + AuthTypeOidc PipelineAuthType = "oidc" + AuthTypeClientSecret PipelineAuthType = "client-secret" +) + type PipelineManagerArgs struct { PipelineServicePrincipalName string PipelineRemoteName string PipelineRoleName string PipelineProvider string + PipelineAuthTypeName string } // PipelineManager takes care of setting up the scm and pipeline. @@ -263,6 +271,7 @@ func (manager *PipelineManager) Configure(ctx context.Context) error { gitRepoInfo, prj.Infra, credentials, + PipelineAuthType(manager.PipelineAuthTypeName), inputConsole) if err != nil { return err diff --git a/cli/azd/pkg/graphsdk/graph_client_test.go b/cli/azd/pkg/graphsdk/graph_client_test.go index cc79265a4eb..3baef1502d8 100644 --- a/cli/azd/pkg/graphsdk/graph_client_test.go +++ b/cli/azd/pkg/graphsdk/graph_client_test.go @@ -49,7 +49,7 @@ func Test_GraphClientRequest(t *testing.T) { createFic := graphsdk.FederatedIdentityCredential{ Name: "mainfic", Issuer: "https://token.actions.githubusercontent.com", - Subject: "repo:${REPO}:ref:refs/heads/main", + Subject: "repo:wbreza/azd-time-sample:ref:refs/heads/main", Description: convert.RefOf("main"), Audiences: []string{ "api://AzureADTokenExchange", From 03686efb473ba9149d5f1c1343a9b31e4a72a395 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Fri, 4 Nov 2022 14:36:26 -0700 Subject: [PATCH 04/15] Updates github workflow to support both service principal and oidc --- .../pkg/commands/pipeline/github_provider.go | 39 ++++++++++++++++--- .../.github/workflows/bicep/azure-dev.yml | 20 +++++++++- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/cli/azd/pkg/commands/pipeline/github_provider.go b/cli/azd/pkg/commands/pipeline/github_provider.go index 6a7e362c647..19ce2492237 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider.go +++ b/cli/azd/pkg/commands/pipeline/github_provider.go @@ -382,7 +382,12 @@ func (p *GitHubCiProvider) configureConnection( // OIDC + Terraform is not a valid combination - warn user. if authType == AuthTypeOidc && infraOptions.Provider == provisioning.Terraform { - console.Message(ctx, output.WithWarningFormat("WARNING: Terraform provisioning does not support OIDC authentication, defaulting to Service Principal with client secret.\n")) + console.Message( + ctx, + output.WithWarningFormat( + "WARNING: Terraform provisioning does not support OIDC authentication, defaulting to Service Principal with client secret.\n", + ), + ) authType = AuthTypeClientSecret } @@ -408,7 +413,14 @@ func (p *GitHubCiProvider) configureConnection( } // Configures Github for standard Service Principal authentication with client id & secret -func (p *GitHubCiProvider) configureClientSecretAuth(ctx context.Context, azdEnvironment *environment.Environment, infraOptions provisioning.Options, repoSlug string, credentials json.RawMessage, console input.Console) error { +func (p *GitHubCiProvider) configureClientSecretAuth( + ctx context.Context, + azdEnvironment *environment.Environment, + infraOptions provisioning.Options, + repoSlug string, + credentials json.RawMessage, + console input.Console, +) error { ghCli := github.NewGitHubCli(ctx) console.Message(ctx, "Setting AZURE_CREDENTIALS GitHub repo secret.\n") @@ -478,7 +490,14 @@ func (p *GitHubCiProvider) configureClientSecretAuth(ctx context.Context, azdEnv } // Configures Github for OIDC authentication using registered application with federated identity credentials -func (p *GitHubCiProvider) configureOidcAuth(ctx context.Context, azdEnvironment *environment.Environment, infraOptions provisioning.Options, repoSlug string, credentials json.RawMessage, console input.Console) error { +func (p *GitHubCiProvider) configureOidcAuth( + ctx context.Context, + azdEnvironment *environment.Environment, + infraOptions provisioning.Options, + repoSlug string, + credentials json.RawMessage, + console input.Console, +) error { ghCli := github.NewGitHubCli(ctx) var azureCredentials azcli.AzureCredentials @@ -722,11 +741,21 @@ func getRemoteUrlFromPrompt(ctx context.Context, remoteName string, console inpu } // Ensures that the federated credential exists on the application otherwise create a new one -func ensureFederatedCredential(ctx context.Context, graphClient *graphsdk.GraphClient, application *graphsdk.Application, existingCredentials []graphsdk.FederatedIdentityCredential, repoCredential *graphsdk.FederatedIdentityCredential) error { +func ensureFederatedCredential( + ctx context.Context, + graphClient *graphsdk.GraphClient, + application *graphsdk.Application, + existingCredentials []graphsdk.FederatedIdentityCredential, + repoCredential *graphsdk.FederatedIdentityCredential, +) error { // If a federated credential already exists for the same subject then nothing to do. for _, existing := range existingCredentials { if existing.Subject == repoCredential.Subject { - log.Printf("federated credential with subject '%s' already exists on application '%s'", repoCredential.Subject, *application.Id) + log.Printf( + "federated credential with subject '%s' already exists on application '%s'", + repoCredential.Subject, + *application.Id, + ) return nil } } diff --git a/templates/common/.github/workflows/bicep/azure-dev.yml b/templates/common/.github/workflows/bicep/azure-dev.yml index 9f95d3e1219..977d6fbfccc 100644 --- a/templates/common/.github/workflows/bicep/azure-dev.yml +++ b/templates/common/.github/workflows/bicep/azure-dev.yml @@ -9,16 +9,34 @@ on: - main - master +permissions: + id-token: write + contents: read + jobs: build: runs-on: ubuntu-latest container: image: mcr.microsoft.com/azure-dev-cli-apps:latest + env: + AZURE_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }} + AZURE_TENANT_ID: ${{ secrets.AZURE_TENANT_ID }} + AZURE_SUBSCRIPTION_ID: ${{ secrets.AZURE_SUBSCRIPTION_ID }} + AZURE_CREDENTIALS: ${{ secrets.AZURE_CREDENTIALS }} steps: - name: Checkout uses: actions/checkout@v2 - - name: Log in with Azure + - name: Log in with Azure (Federated Credentials) + if: ${{ env.AZURE_CLIENT_ID != '' }} + uses: azure/login@v1 + with: + client-id: ${{ secrets.AZURE_CLIENT_ID }} + tenant-id: ${{ secrets.AZURE_TENANT_ID }} + subscription-id: ${{ secrets.AZURE_SUBSCRIPTION_ID }} + + - name: Log in with Azure (Service Principal) + if: ${{ env.AZURE_CREDENTIALS != '' }} uses: azure/login@v1 with: creds: ${{ secrets.AZURE_CREDENTIALS }} From 30713c53830ee5e82db76577646dc8e2b5854b88 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Mon, 7 Nov 2022 12:23:48 -0800 Subject: [PATCH 05/15] Addresses PR feedback. --- .vscode/cspell.global.yaml | 4 ++ .../pkg/commands/pipeline/azdo_provider.go | 4 ++ .../pkg/commands/pipeline/github_provider.go | 38 +++++++++---------- .../pkg/commands/pipeline/pipeline_manager.go | 2 + 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/.vscode/cspell.global.yaml b/.vscode/cspell.global.yaml index 0982becc3f7..181d733930c 100644 --- a/.vscode/cspell.global.yaml +++ b/.vscode/cspell.global.yaml @@ -57,6 +57,7 @@ ignoreWords: - execfn - execmock - fdfp + - fics - Frontdoor - graphsdk - hndl @@ -68,6 +69,7 @@ ignoreWords: - kubernetes - kusto - magefile + - mainfic - menuid - migr - mockconfig @@ -80,6 +82,8 @@ ignoreWords: - odata - osdisk - osexec + - OIDC + - Oidc - pcert - pdnsz - Peerings diff --git a/cli/azd/pkg/commands/pipeline/azdo_provider.go b/cli/azd/pkg/commands/pipeline/azdo_provider.go index b9d33c4750f..f6e1222e841 100644 --- a/cli/azd/pkg/commands/pipeline/azdo_provider.go +++ b/cli/azd/pkg/commands/pipeline/azdo_provider.go @@ -599,6 +599,10 @@ func (p *AzdoCiProvider) configureConnection( authType PipelineAuthType, console input.Console) error { + if authType == AuthTypeOidc { + return fmt.Errorf("Azure DevOps does not support OIDC authentication. Service Principal with client ID and client secret must be used. %w", ErrAuthNotSupported) + } + azureCredentials, err := parseCredentials(ctx, credentials) if err != nil { return err diff --git a/cli/azd/pkg/commands/pipeline/github_provider.go b/cli/azd/pkg/commands/pipeline/github_provider.go index 19ce2492237..103ed5e12e3 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider.go +++ b/cli/azd/pkg/commands/pipeline/github_provider.go @@ -367,28 +367,28 @@ func (p *GitHubCiProvider) configureConnection( authType PipelineAuthType, console input.Console) error { - repoSlug := repoDetails.owner + "/" + repoDetails.repoName - console.Message(ctx, fmt.Sprintf("Configuring repository %s.\n", repoSlug)) - - ghCli := github.NewGitHubCli(ctx) - if err := p.ensureAuthorizedForRepoSecrets(ctx, ghCli, console, repoSlug); err != nil { - return fmt.Errorf("ensuring authorization: %w", err) - } - - // Default to OIDC if not specified - if authType == "" { - authType = AuthTypeOidc - } + // Oidc + Terraform is not a supported combination + if infraOptions.Provider == provisioning.Terraform { + // Throw error if Oidc is explicitly requested + if authType == AuthTypeOidc { + return fmt.Errorf("Terraform does not support OIDC authentication. Service Principal with client ID and client secret must be used. %w", ErrAuthNotSupported) + } - // OIDC + Terraform is not a valid combination - warn user. - if authType == AuthTypeOidc && infraOptions.Provider == provisioning.Terraform { + // If not explicitly set, show warning console.Message( ctx, output.WithWarningFormat( - "WARNING: Terraform provisioning does not support OIDC authentication, defaulting to Service Principal with client secret.\n", + "WARNING: Terraform provisioning does not support OIDC authentication, defaulting to Service Principal with client ID and client secret.\n", ), ) - authType = AuthTypeClientSecret + } + + repoSlug := repoDetails.owner + "/" + repoDetails.repoName + console.Message(ctx, fmt.Sprintf("Configuring repository %s.\n", repoSlug)) + + ghCli := github.NewGitHubCli(ctx) + if err := p.ensureAuthorizedForRepoSecrets(ctx, ghCli, console, repoSlug); err != nil { + return fmt.Errorf("ensuring authorization: %w", err) } var authErr error @@ -533,7 +533,7 @@ const ( federatedIdentityAudience = "api://AzureADTokenExchange" ) -func applyFederatedCredentials(ctx context.Context, repoSlug string, azureCredentails *azcli.AzureCredentials) error { +func applyFederatedCredentials(ctx context.Context, repoSlug string, azureCredentials *azcli.AzureCredentials) error { graphClient, err := createGraphClient(ctx) if err != nil { return err @@ -541,7 +541,7 @@ func applyFederatedCredentials(ctx context.Context, repoSlug string, azureCreden appsResponse, err := graphClient. Applications(). - Filter(fmt.Sprintf("appId eq '%s'", azureCredentails.ClientId)). + Filter(fmt.Sprintf("appId eq '%s'", azureCredentials.ClientId)). Get(ctx) if err != nil || len(appsResponse.Value) == 0 { return fmt.Errorf("failed finding matching application: %w", err) @@ -767,7 +767,7 @@ func ensureFederatedCredential( Post(ctx, repoCredential) if err != nil { - return fmt.Errorf("failed created federated credential: %w", err) + return fmt.Errorf("failed creating federated credential: %w", err) } return nil diff --git a/cli/azd/pkg/commands/pipeline/pipeline_manager.go b/cli/azd/pkg/commands/pipeline/pipeline_manager.go index 9f15fbbddb0..e4dd619b104 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline_manager.go +++ b/cli/azd/pkg/commands/pipeline/pipeline_manager.go @@ -28,6 +28,8 @@ const ( AuthTypeClientSecret PipelineAuthType = "client-secret" ) +var ErrAuthNotSupported = errors.New("pipeline authentication configuration is not supported") + type PipelineManagerArgs struct { PipelineServicePrincipalName string PipelineRemoteName string From 13e48c3f916abc65a7d018e70bf068f49187200b Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Mon, 7 Nov 2022 13:55:53 -0800 Subject: [PATCH 06/15] Adds unit tests for new graph client request builders --- .../application_request_builders_test.go | 147 +++++++++----- .../pkg/graphsdk/fic_request_builders_test.go | 190 ++++++++++++++++++ cli/azd/pkg/graphsdk/graph_client_test.go | 103 ++-------- ...service_principal_request_builders_test.go | 112 ++++++++--- cli/azd/pkg/tools/azcli/ad_test.go | 16 +- cli/azd/test/mocks/graphsdk/mocks.go | 94 ++++++++- 6 files changed, 480 insertions(+), 182 deletions(-) create mode 100644 cli/azd/pkg/graphsdk/fic_request_builders_test.go diff --git a/cli/azd/pkg/graphsdk/application_request_builders_test.go b/cli/azd/pkg/graphsdk/application_request_builders_test.go index 6cd018a4a6e..2c2565ffc70 100644 --- a/cli/azd/pkg/graphsdk/application_request_builders_test.go +++ b/cli/azd/pkg/graphsdk/application_request_builders_test.go @@ -2,9 +2,11 @@ package graphsdk_test import ( "context" + "errors" "net/http" "testing" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/azure/azure-dev/cli/azd/pkg/convert" "github.com/azure/azure-dev/cli/azd/pkg/graphsdk" "github.com/azure/azure-dev/cli/azd/test/mocks" @@ -12,18 +14,24 @@ import ( "github.com/stretchr/testify/require" ) +var ( + applications []graphsdk.Application = []graphsdk.Application{ + { + Id: convert.RefOf("1"), + AppId: convert.RefOf("app-01"), + DisplayName: "App 1", + }, + { + Id: convert.RefOf("2"), + AppId: convert.RefOf("app-02"), + DisplayName: "App 2", + }, + } +) + func TestGetApplicationList(t *testing.T) { t.Run("Success", func(t *testing.T) { - expected := []graphsdk.Application{ - { - Id: convert.RefOf("1"), - DisplayName: "App 1", - }, - { - Id: convert.RefOf("2"), - DisplayName: "App 2", - }, - } + expected := append([]graphsdk.Application{}, applications...) mockContext := mocks.NewMockContext(context.Background()) graphsdk_mocks.RegisterApplicationListMock(mockContext, http.StatusOK, expected) @@ -44,7 +52,10 @@ func TestGetApplicationList(t *testing.T) { client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - res, err := client.Applications().Get(*mockContext.Context) + res, err := client. + Applications(). + Get(*mockContext.Context) + require.Error(t, err) require.Nil(t, res) }) @@ -52,20 +63,18 @@ func TestGetApplicationList(t *testing.T) { func TestGetApplicationById(t *testing.T) { t.Run("Success", func(t *testing.T) { - expected := graphsdk.Application{ - Id: convert.RefOf("1"), - AppId: convert.RefOf("app-1"), - DisplayName: "App 1", - PasswordCredentials: []*graphsdk.ApplicationPasswordCredential{}, - } + expected := applications[0] mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterApplicationItemMock(mockContext, http.StatusOK, *expected.Id, &expected) + graphsdk_mocks.RegisterApplicationGetItemMock(mockContext, http.StatusOK, *expected.Id, &expected) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - actual, err := client.ApplicationById(*expected.Id).Get(*mockContext.Context) + actual, err := client. + ApplicationById(*expected.Id). + Get(*mockContext.Context) + require.NoError(t, err) require.NotNil(t, actual) require.Equal(t, *expected.Id, *actual.Id) @@ -75,12 +84,15 @@ func TestGetApplicationById(t *testing.T) { t.Run("Error", func(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterApplicationItemMock(mockContext, http.StatusNotFound, "bad-id", nil) + graphsdk_mocks.RegisterApplicationGetItemMock(mockContext, http.StatusNotFound, "bad-id", nil) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - res, err := client.ApplicationById("bad-id").Get(*mockContext.Context) + res, err := client. + ApplicationById("bad-id"). + Get(*mockContext.Context) + require.Error(t, err) require.Nil(t, res) }) @@ -88,20 +100,18 @@ func TestGetApplicationById(t *testing.T) { func TestCreateApplication(t *testing.T) { t.Run("Success", func(t *testing.T) { - expected := graphsdk.Application{ - Id: convert.RefOf("1"), - AppId: convert.RefOf("app-1"), - DisplayName: "App 1", - PasswordCredentials: []*graphsdk.ApplicationPasswordCredential{}, - } + expected := applications[0] mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterApplicationCreateMock(mockContext, http.StatusCreated, &expected) + graphsdk_mocks.RegisterApplicationCreateItemMock(mockContext, http.StatusCreated, &expected) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - actual, err := client.Applications().Post(*mockContext.Context, &expected) + actual, err := client. + Applications(). + Post(*mockContext.Context, &expected) + require.NoError(t, err) require.NotNil(t, actual) require.Equal(t, *expected.Id, *actual.Id) @@ -111,25 +121,58 @@ func TestCreateApplication(t *testing.T) { t.Run("Error", func(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterApplicationCreateMock(mockContext, http.StatusBadRequest, nil) + graphsdk_mocks.RegisterApplicationCreateItemMock(mockContext, http.StatusBadRequest, nil) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - res, err := client.Applications().Post(*mockContext.Context, &graphsdk.Application{}) + res, err := client. + Applications(). + Post(*mockContext.Context, &graphsdk.Application{}) + require.Error(t, err) require.Nil(t, res) }) } +func TestDeleteApplication(t *testing.T) { + applicationId := "app-to-delete" + + t.Run("Success", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterApplicationDeleteItemMock(mockContext, applicationId, http.StatusNoContent) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + err = client. + ApplicationById(applicationId). + Delete(*mockContext.Context) + + require.NoError(t, err) + }) + + t.Run("Error", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterApplicationDeleteItemMock(mockContext, applicationId, http.StatusNotFound) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + err = client. + ApplicationById(applicationId). + Delete(*mockContext.Context) + + require.Error(t, err) + var httpErr *azcore.ResponseError + require.True(t, errors.As(err, &httpErr)) + require.Equal(t, http.StatusNotFound, httpErr.StatusCode) + }) +} + func TestApplicationAddPassword(t *testing.T) { t.Run("Success", func(t *testing.T) { - app := graphsdk.Application{ - Id: convert.RefOf("1"), - AppId: convert.RefOf("app-1"), - DisplayName: "App 1", - PasswordCredentials: []*graphsdk.ApplicationPasswordCredential{}, - } + application := applications[0] mockCredential := graphsdk.ApplicationPasswordCredential{ KeyId: convert.RefOf("key1"), @@ -138,12 +181,15 @@ func TestApplicationAddPassword(t *testing.T) { } mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterApplicationAddPasswordMock(mockContext, http.StatusOK, *app.Id, &mockCredential) + graphsdk_mocks.RegisterApplicationAddPasswordMock(mockContext, http.StatusOK, *application.Id, &mockCredential) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - actual, err := client.ApplicationById(*app.Id).AddPassword(*mockContext.Context) + actual, err := client. + ApplicationById(*application.Id). + AddPassword(*mockContext.Context) + require.NoError(t, err) require.NotNil(t, actual) require.Equal(t, *mockCredential.KeyId, *actual.KeyId) @@ -158,39 +204,40 @@ func TestApplicationAddPassword(t *testing.T) { client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - actual, err := client.ApplicationById("bad-app-id").AddPassword(*mockContext.Context) + actual, err := client. + ApplicationById("bad-app-id"). + AddPassword(*mockContext.Context) + require.Error(t, err) require.Nil(t, actual) }) } func TestApplicationRemovePassword(t *testing.T) { - app := graphsdk.Application{ - Id: convert.RefOf("1"), - AppId: convert.RefOf("app-1"), - DisplayName: "App 1", - PasswordCredentials: []*graphsdk.ApplicationPasswordCredential{}, - } + application := applications[0] t.Run("Success", func(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterApplicationRemovePasswordMock(mockContext, http.StatusNoContent, *app.Id) + graphsdk_mocks.RegisterApplicationRemovePasswordMock(mockContext, http.StatusNoContent, *application.Id) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - err = client.ApplicationById(*app.Id).RemovePassword(*mockContext.Context, "key1") + err = client. + ApplicationById(*application.Id). + RemovePassword(*mockContext.Context, "key1") + require.NoError(t, err) }) t.Run("Error", func(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterApplicationRemovePasswordMock(mockContext, http.StatusNotFound, *app.Id) + graphsdk_mocks.RegisterApplicationRemovePasswordMock(mockContext, http.StatusNotFound, *application.Id) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - err = client.ApplicationById(*app.Id).RemovePassword(*mockContext.Context, "bad-key-id") + err = client.ApplicationById(*application.Id).RemovePassword(*mockContext.Context, "bad-key-id") require.Error(t, err) }) } diff --git a/cli/azd/pkg/graphsdk/fic_request_builders_test.go b/cli/azd/pkg/graphsdk/fic_request_builders_test.go new file mode 100644 index 00000000000..2eea0fa671d --- /dev/null +++ b/cli/azd/pkg/graphsdk/fic_request_builders_test.go @@ -0,0 +1,190 @@ +package graphsdk_test + +import ( + "context" + "errors" + "net/http" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/azure/azure-dev/cli/azd/pkg/convert" + "github.com/azure/azure-dev/cli/azd/pkg/graphsdk" + "github.com/azure/azure-dev/cli/azd/test/mocks" + graphsdk_mocks "github.com/azure/azure-dev/cli/azd/test/mocks/graphsdk" + "github.com/stretchr/testify/require" +) + +var ( + application graphsdk.Application = graphsdk.Application{ + Id: convert.RefOf("application-id"), + DisplayName: "application name", + Description: convert.RefOf("app description"), + } + + federatedCredentials []graphsdk.FederatedIdentityCredential = []graphsdk.FederatedIdentityCredential{ + { + Id: convert.RefOf("cred-01"), + Name: "Credential 1", + Issuer: "ISSUER", + Subject: "SUBJECT", + Description: convert.RefOf("DESCRIPTION"), + Audiences: []string{"AUDIENCE"}, + }, + { + Id: convert.RefOf("cred-02"), + Name: "Credential 2", + Issuer: "ISSUER", + Subject: "SUBJECT", + Description: convert.RefOf("DESCRIPTION"), + }, + } +) + +func TestGetFederatedCredentialList(t *testing.T) { + t.Run("Success", func(t *testing.T) { + expected := append([]graphsdk.FederatedIdentityCredential{}, federatedCredentials...) + + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialsListMock(mockContext, *application.Id, http.StatusOK, expected) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + res, err := client. + ApplicationById(*application.Id). + FederatedIdentityCredentials(). + Get(*mockContext.Context) + + require.NoError(t, err) + require.NotNil(t, res) + require.Equal(t, expected, res.Value) + }) + + t.Run("Error", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialsListMock(mockContext, *application.Id, http.StatusUnauthorized, nil) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + res, err := client. + ApplicationById(*application.Id). + FederatedIdentityCredentials(). + Get(*mockContext.Context) + + require.Error(t, err) + require.Nil(t, res) + }) +} + +func TestGetFederatedCredentialById(t *testing.T) { + t.Run("Success", func(t *testing.T) { + expected := federatedCredentials[0] + + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialGetItemMock(mockContext, *application.Id, *expected.Id, http.StatusOK, &expected) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + actual, err := client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById(*expected.Id). + Get(*mockContext.Context) + + require.NoError(t, err) + require.NotNil(t, actual) + require.Equal(t, *expected.Id, *actual.Id) + require.Equal(t, expected.Name, actual.Name) + require.Equal(t, expected.Issuer, actual.Issuer) + }) + + t.Run("Error", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialGetItemMock(mockContext, *application.Id, "bad-id", http.StatusNotFound, nil) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + res, err := client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById("bad-id"). + Get(*mockContext.Context) + + require.Error(t, err) + require.Nil(t, res) + }) +} + +func TestCreateFederatedCredential(t *testing.T) { + t.Run("Success", func(t *testing.T) { + expected := federatedCredentials[0] + + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialCreateItemMock(mockContext, *application.Id, http.StatusCreated, &expected) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + actual, err := client.ApplicationById(*application.Id).FederatedIdentityCredentials().Post(*mockContext.Context, &expected) + require.NoError(t, err) + require.NotNil(t, actual) + require.Equal(t, *expected.Id, *actual.Id) + require.Equal(t, expected.Name, actual.Name) + require.Equal(t, expected.Issuer, actual.Issuer) + }) + + t.Run("Error", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialCreateItemMock(mockContext, *application.Id, http.StatusBadRequest, nil) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + res, err := client. + ApplicationById(*application.Id). + FederatedIdentityCredentials(). + Post(*mockContext.Context, &graphsdk.FederatedIdentityCredential{}) + + require.Error(t, err) + require.Nil(t, res) + }) +} + +func TestDeleteFederatedCredential(t *testing.T) { + credentialId := "credential-to-delete" + + t.Run("Success", func(t *testing.T) { + + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialDeleteItemMock(mockContext, *application.Id, credentialId, http.StatusNoContent) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + err = client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById(credentialId). + Delete(*mockContext.Context) + + require.NoError(t, err) + }) + + t.Run("Error", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialDeleteItemMock(mockContext, *application.Id, credentialId, http.StatusNotFound) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + err = client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById(credentialId). + Delete(*mockContext.Context) + require.Error(t, err) + + var httpErr *azcore.ResponseError + require.True(t, errors.As(err, &httpErr)) + require.Equal(t, http.StatusNotFound, httpErr.StatusCode) + }) +} diff --git a/cli/azd/pkg/graphsdk/graph_client_test.go b/cli/azd/pkg/graphsdk/graph_client_test.go index 3baef1502d8..b0330812b96 100644 --- a/cli/azd/pkg/graphsdk/graph_client_test.go +++ b/cli/azd/pkg/graphsdk/graph_client_test.go @@ -6,101 +6,26 @@ import ( "net/http" "testing" - "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" - "github.com/azure/azure-dev/cli/azd/pkg/azsdk" - "github.com/azure/azure-dev/cli/azd/pkg/convert" - "github.com/azure/azure-dev/cli/azd/pkg/graphsdk" + "github.com/azure/azure-dev/cli/azd/test/mocks" + graphsdk_mocks "github.com/azure/azure-dev/cli/azd/test/mocks/graphsdk" "github.com/stretchr/testify/require" ) -// Testing for temp integration testing during development -func Test_GraphClientRequest(t *testing.T) { - ctx := context.Background() - credential, err := azidentity.NewAzureCLICredential(nil) - require.NoError(t, err) - - clientOptions := azsdk.NewClientOptionsBuilder().BuildCoreClientOptions() - client, err := graphsdk.NewGraphClient(credential, clientOptions) +// Testing simulates requests that have a pre-flight error like +// acquiring token or DNS issues (host not found) +func Test_GraphClientRequest_With_Preflight_Error(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) require.NotNil(t, client) - appsResponse, err := client. - Applications(). - Filter("displayName eq 'wabrez-spn-go-test'"). - Get(ctx) - - require.NoError(t, err) - require.NotNil(t, appsResponse) - - application := appsResponse.Value[0] - - ficsResponse, err := client. - ApplicationById(*application.Id). - FederatedIdentityCredentials(). - Get(ctx) - - require.NoError(t, err) - require.NotNil(t, ficsResponse) - - var fic graphsdk.FederatedIdentityCredential - - if len(ficsResponse.Value) == 0 { - createFic := graphsdk.FederatedIdentityCredential{ - Name: "mainfic", - Issuer: "https://token.actions.githubusercontent.com", - Subject: "repo:wbreza/azd-time-sample:ref:refs/heads/main", - Description: convert.RefOf("main"), - Audiences: []string{ - "api://AzureADTokenExchange", - }, - } - - newFic, err := client. - ApplicationById(*application.Id). - FederatedIdentityCredentials(). - Post(ctx, &createFic) - - require.NoError(t, err) - require.NotNil(t, newFic) - - fic = *newFic - } else { - fic = ficsResponse.Value[0] - } - - existingFic, err := client. - ApplicationById(*application.Id). - FederatedIdentityCredentialById(*fic.Id). - Get(ctx) - - require.NoError(t, err) - require.NotNil(t, existingFic) - - existingFic.Description = convert.RefOf("updated") - err = client. - ApplicationById(*application.Id). - FederatedIdentityCredentialById(*fic.Id). - Update(ctx, existingFic) - - require.NoError(t, err) - - err = client. - ApplicationById(*application.Id). - FederatedIdentityCredentialById(*fic.Id). - Delete(ctx) - - require.NoError(t, err) - - getFic, err := client. - ApplicationById(*application.Id). - FederatedIdentityCredentialById(*fic.Id). - Get(ctx) + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodGet && request.URL.Path == "/v1.0/me" + }).RespondFn(func(request *http.Request) (*http.Response, error) { + return nil, errors.New("some error before request could be made") + }) + res, err := client.Me().Get(*mockContext.Context) + require.Nil(t, res) require.Error(t, err) - require.Nil(t, getFic) - - var httpErr *azcore.ResponseError - require.True(t, errors.As(err, &httpErr)) - require.Equal(t, http.StatusNotFound, httpErr.StatusCode) } diff --git a/cli/azd/pkg/graphsdk/service_principal_request_builders_test.go b/cli/azd/pkg/graphsdk/service_principal_request_builders_test.go index 551b309759f..eb813200128 100644 --- a/cli/azd/pkg/graphsdk/service_principal_request_builders_test.go +++ b/cli/azd/pkg/graphsdk/service_principal_request_builders_test.go @@ -2,9 +2,11 @@ package graphsdk_test import ( "context" + "errors" "net/http" "testing" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/azure/azure-dev/cli/azd/pkg/convert" "github.com/azure/azure-dev/cli/azd/pkg/graphsdk" "github.com/azure/azure-dev/cli/azd/test/mocks" @@ -12,18 +14,22 @@ import ( "github.com/stretchr/testify/require" ) +var ( + servicePrincipals []graphsdk.ServicePrincipal = []graphsdk.ServicePrincipal{ + { + Id: convert.RefOf("1"), + DisplayName: "SPN 1", + }, + { + Id: convert.RefOf("2"), + DisplayName: "SPN 2", + }, + } +) + func TestGetServicePrincipalList(t *testing.T) { t.Run("Success", func(t *testing.T) { - expected := []graphsdk.ServicePrincipal{ - { - Id: convert.RefOf("1"), - DisplayName: "SPN 1", - }, - { - Id: convert.RefOf("2"), - DisplayName: "SPN 2", - }, - } + expected := append([]graphsdk.ServicePrincipal{}, servicePrincipals...) mockContext := mocks.NewMockContext(context.Background()) graphsdk_mocks.RegisterServicePrincipalListMock(mockContext, http.StatusOK, expected) @@ -31,7 +37,10 @@ func TestGetServicePrincipalList(t *testing.T) { client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - servicePrincipals, err := client.ServicePrincipals().Get(*mockContext.Context) + servicePrincipals, err := client. + ServicePrincipals(). + Get(*mockContext.Context) + require.NoError(t, err) require.NotNil(t, servicePrincipals) require.Equal(t, expected, servicePrincipals.Value) @@ -44,7 +53,10 @@ func TestGetServicePrincipalList(t *testing.T) { client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - res, err := client.ServicePrincipals().Get(*mockContext.Context) + res, err := client. + ServicePrincipals(). + Get(*mockContext.Context) + require.Error(t, err) require.Nil(t, res) }) @@ -52,19 +64,18 @@ func TestGetServicePrincipalList(t *testing.T) { func TestGetServicePrincipalById(t *testing.T) { t.Run("Success", func(t *testing.T) { - expected := graphsdk.ServicePrincipal{ - Id: convert.RefOf("1"), - AppId: "app-1", - DisplayName: "App 1", - } + expected := servicePrincipals[0] mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterServicePrincipalItemMock(mockContext, http.StatusOK, *expected.Id, &expected) + graphsdk_mocks.RegisterServicePrincipalGetItemMock(mockContext, http.StatusOK, *expected.Id, &expected) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - actual, err := client.ServicePrincipalById(*expected.Id).Get(*mockContext.Context) + actual, err := client. + ServicePrincipalById(*expected.Id). + Get(*mockContext.Context) + require.NoError(t, err) require.NotNil(t, actual) require.Equal(t, *expected.Id, *actual.Id) @@ -74,12 +85,15 @@ func TestGetServicePrincipalById(t *testing.T) { t.Run("Error", func(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterServicePrincipalItemMock(mockContext, http.StatusNotFound, "bad-id", nil) + graphsdk_mocks.RegisterServicePrincipalGetItemMock(mockContext, http.StatusNotFound, "bad-id", nil) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - res, err := client.ServicePrincipalById("bad-id").Get(*mockContext.Context) + res, err := client. + ServicePrincipalById("bad-id"). + Get(*mockContext.Context) + require.Error(t, err) require.Nil(t, res) }) @@ -87,19 +101,18 @@ func TestGetServicePrincipalById(t *testing.T) { func TestCreateServicePrincipal(t *testing.T) { t.Run("Success", func(t *testing.T) { - expected := graphsdk.ServicePrincipal{ - Id: convert.RefOf("1"), - AppId: "app-1", - DisplayName: "App 1", - } + expected := servicePrincipals[0] mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterServicePrincipalCreateMock(mockContext, http.StatusCreated, &expected) + graphsdk_mocks.RegisterServicePrincipalCreateItemMock(mockContext, http.StatusCreated, &expected) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - actual, err := client.ServicePrincipals().Post(*mockContext.Context, &expected) + actual, err := client. + ServicePrincipals(). + Post(*mockContext.Context, &expected) + require.NoError(t, err) require.NotNil(t, actual) require.Equal(t, *expected.Id, *actual.Id) @@ -109,13 +122,52 @@ func TestCreateServicePrincipal(t *testing.T) { t.Run("Error", func(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterServicePrincipalCreateMock(mockContext, http.StatusBadRequest, nil) + graphsdk_mocks.RegisterServicePrincipalCreateItemMock(mockContext, http.StatusBadRequest, nil) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - res, err := client.ServicePrincipals().Post(*mockContext.Context, &graphsdk.ServicePrincipal{}) + res, err := client. + ServicePrincipals(). + Post(*mockContext.Context, &graphsdk.ServicePrincipal{}) + require.Error(t, err) require.Nil(t, res) }) } + +func TestDeleteServicePrincipal(t *testing.T) { + servicePrincipalId := "spn-to-delete" + + t.Run("Success", func(t *testing.T) { + + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterServicePrincipalDeleteItemMock(mockContext, servicePrincipalId, http.StatusNoContent) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + err = client. + ServicePrincipalById(servicePrincipalId). + Delete(*mockContext.Context) + + require.NoError(t, err) + }) + + t.Run("Error", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterServicePrincipalDeleteItemMock(mockContext, servicePrincipalId, http.StatusNotFound) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + err = client. + ServicePrincipalById(servicePrincipalId). + Delete(*mockContext.Context) + + require.Error(t, err) + var httpErr *azcore.ResponseError + require.True(t, errors.As(err, &httpErr)) + require.Equal(t, http.StatusNotFound, httpErr.StatusCode) + }) +} diff --git a/cli/azd/pkg/tools/azcli/ad_test.go b/cli/azd/pkg/tools/azcli/ad_test.go index 58b76dd5e55..9875ef211e5 100644 --- a/cli/azd/pkg/tools/azcli/ad_test.go +++ b/cli/azd/pkg/tools/azcli/ad_test.go @@ -94,11 +94,11 @@ func Test_CreateOrUpdateServicePrincipal(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) graphsdk_mocks.RegisterApplicationListMock(mockContext, http.StatusOK, []graphsdk.Application{}) graphsdk_mocks.RegisterServicePrincipalListMock(mockContext, http.StatusOK, []graphsdk.ServicePrincipal{}) - graphsdk_mocks.RegisterApplicationCreateMock(mockContext, http.StatusCreated, &newApplication) - graphsdk_mocks.RegisterServicePrincipalCreateMock(mockContext, http.StatusCreated, &servicePrincipal) + graphsdk_mocks.RegisterApplicationCreateItemMock(mockContext, http.StatusCreated, &newApplication) + graphsdk_mocks.RegisterServicePrincipalCreateItemMock(mockContext, http.StatusCreated, &servicePrincipal) graphsdk_mocks.RegisterApplicationAddPasswordMock(mockContext, http.StatusOK, *newApplication.Id, credential) graphsdk_mocks.RegisterRoleDefinitionListMock(mockContext, http.StatusOK, roleDefinitions) - graphsdk_mocks.RegisterRoleAssignmentMock(mockContext, http.StatusCreated) + graphsdk_mocks.RegisterRoleAssignmentPutMock(mockContext, http.StatusCreated) azCli := GetAzCli(*mockContext.Context) rawMessage, err := azCli.CreateOrUpdateServicePrincipal( @@ -125,7 +125,7 @@ func Test_CreateOrUpdateServicePrincipal(t *testing.T) { graphsdk_mocks.RegisterApplicationRemovePasswordMock(mockContext, http.StatusNoContent, *newApplication.Id) graphsdk_mocks.RegisterApplicationAddPasswordMock(mockContext, http.StatusOK, *newApplication.Id, credential) graphsdk_mocks.RegisterRoleDefinitionListMock(mockContext, http.StatusOK, roleDefinitions) - graphsdk_mocks.RegisterRoleAssignmentMock(mockContext, http.StatusCreated) + graphsdk_mocks.RegisterRoleAssignmentPutMock(mockContext, http.StatusCreated) azCli := GetAzCli(*mockContext.Context) rawMessage, err := azCli.CreateOrUpdateServicePrincipal( @@ -153,7 +153,7 @@ func Test_CreateOrUpdateServicePrincipal(t *testing.T) { graphsdk_mocks.RegisterApplicationAddPasswordMock(mockContext, http.StatusOK, *newApplication.Id, credential) graphsdk_mocks.RegisterRoleDefinitionListMock(mockContext, http.StatusOK, roleDefinitions) // Note how role assignment returns a 409 conflict - graphsdk_mocks.RegisterRoleAssignmentMock(mockContext, http.StatusConflict) + graphsdk_mocks.RegisterRoleAssignmentPutMock(mockContext, http.StatusConflict) azCli := GetAzCli(*mockContext.Context) rawMessage, err := azCli.CreateOrUpdateServicePrincipal( @@ -172,8 +172,8 @@ func Test_CreateOrUpdateServicePrincipal(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) graphsdk_mocks.RegisterApplicationListMock(mockContext, http.StatusOK, []graphsdk.Application{}) graphsdk_mocks.RegisterServicePrincipalListMock(mockContext, http.StatusOK, []graphsdk.ServicePrincipal{}) - graphsdk_mocks.RegisterApplicationCreateMock(mockContext, http.StatusCreated, &newApplication) - graphsdk_mocks.RegisterServicePrincipalCreateMock(mockContext, http.StatusCreated, &servicePrincipal) + graphsdk_mocks.RegisterApplicationCreateItemMock(mockContext, http.StatusCreated, &newApplication) + graphsdk_mocks.RegisterServicePrincipalCreateItemMock(mockContext, http.StatusCreated, &servicePrincipal) graphsdk_mocks.RegisterApplicationAddPasswordMock(mockContext, http.StatusOK, *newApplication.Id, credential) // Note how retrieval of matching role assignments is empty graphsdk_mocks.RegisterRoleDefinitionListMock(mockContext, http.StatusOK, []*armauthorization.RoleDefinition{}) @@ -194,7 +194,7 @@ func Test_CreateOrUpdateServicePrincipal(t *testing.T) { graphsdk_mocks.RegisterApplicationListMock(mockContext, http.StatusOK, []graphsdk.Application{}) graphsdk_mocks.RegisterServicePrincipalListMock(mockContext, http.StatusOK, []graphsdk.ServicePrincipal{}) // Note that the application creation returns an unauthorized error - graphsdk_mocks.RegisterApplicationCreateMock(mockContext, http.StatusUnauthorized, nil) + graphsdk_mocks.RegisterApplicationCreateItemMock(mockContext, http.StatusUnauthorized, nil) azCli := GetAzCli(*mockContext.Context) rawMessage, err := azCli.CreateOrUpdateServicePrincipal( diff --git a/cli/azd/test/mocks/graphsdk/mocks.go b/cli/azd/test/mocks/graphsdk/mocks.go index 13830ba3bb7..76a83453fc3 100644 --- a/cli/azd/test/mocks/graphsdk/mocks.go +++ b/cli/azd/test/mocks/graphsdk/mocks.go @@ -43,7 +43,7 @@ func RegisterApplicationListMock(mockContext *mocks.MockContext, statusCode int, }) } -func RegisterApplicationItemMock( +func RegisterApplicationGetItemMock( mockContext *mocks.MockContext, statusCode int, appId string, @@ -60,7 +60,19 @@ func RegisterApplicationItemMock( }) } -func RegisterApplicationCreateMock(mockContext *mocks.MockContext, statusCode int, application *graphsdk.Application) { +func RegisterApplicationDeleteItemMock( + mockContext *mocks.MockContext, + appId string, + statusCode int, +) { + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodDelete && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s", appId)) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + return mocks.CreateEmptyHttpResponse(request, statusCode) + }) +} + +func RegisterApplicationCreateItemMock(mockContext *mocks.MockContext, statusCode int, application *graphsdk.Application) { mockContext.HttpClient.When(func(request *http.Request) bool { return request.Method == http.MethodPost && strings.Contains(request.URL.Path, "/applications") }).RespondFn(func(request *http.Request) (*http.Response, error) { @@ -123,7 +135,7 @@ func RegisterServicePrincipalListMock( }) } -func RegisterServicePrincipalItemMock( +func RegisterServicePrincipalGetItemMock( mockContext *mocks.MockContext, statusCode int, spnId string, @@ -141,7 +153,20 @@ func RegisterServicePrincipalItemMock( }) } -func RegisterServicePrincipalCreateMock( +func RegisterServicePrincipalDeleteItemMock( + mockContext *mocks.MockContext, + spnId string, + statusCode int, +) { + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodDelete && + strings.Contains(request.URL.Path, fmt.Sprintf("/servicePrincipals/%s", spnId)) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + return mocks.CreateEmptyHttpResponse(request, statusCode) + }) +} + +func RegisterServicePrincipalCreateItemMock( mockContext *mocks.MockContext, statusCode int, servicePrincipal *graphsdk.ServicePrincipal, @@ -192,7 +217,7 @@ func RegisterRoleDefinitionListMock( }) } -func RegisterRoleAssignmentMock(mockContext *mocks.MockContext, statusCode int) { +func RegisterRoleAssignmentPutMock(mockContext *mocks.MockContext, statusCode int) { mockContext.HttpClient.When(func(request *http.Request) bool { return request.Method == http.MethodPut && strings.Contains(request.URL.Path, "/providers/Microsoft.Authorization/roleAssignments/") @@ -219,3 +244,62 @@ func RegisterRoleAssignmentMock(mockContext *mocks.MockContext, statusCode int) } }) } + +func RegisterFederatedCredentialsListMock(mockContext *mocks.MockContext, applicationId string, statusCode int, federatedCredentials []graphsdk.FederatedIdentityCredential) { + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodGet && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s/federatedIdentityCredentials", applicationId)) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + listResponse := graphsdk.FederatedIdentityCredentialListResponse{ + Value: federatedCredentials, + } + + if federatedCredentials == nil { + return mocks.CreateEmptyHttpResponse(request, statusCode) + } + + return mocks.CreateHttpResponseWithBody(request, statusCode, listResponse) + }) +} + +func RegisterFederatedCredentialCreateItemMock(mockContext *mocks.MockContext, applicationId string, statusCode int, federatedCredential *graphsdk.FederatedIdentityCredential) { + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodPost && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s", applicationId)) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + if federatedCredential == nil { + return mocks.CreateEmptyHttpResponse(request, statusCode) + } + + return mocks.CreateHttpResponseWithBody(request, statusCode, federatedCredential) + }) +} + +func RegisterFederatedCredentialGetItemMock( + mockContext *mocks.MockContext, + appId string, + federatedCredentialId string, + statusCode int, + federatedCredential *graphsdk.FederatedIdentityCredential, +) { + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodGet && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s/federatedIdentityCredentials/%s", appId, federatedCredentialId)) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + if federatedCredential == nil { + return mocks.CreateEmptyHttpResponse(request, statusCode) + } + + return mocks.CreateHttpResponseWithBody(request, statusCode, federatedCredential) + }) +} + +func RegisterFederatedCredentialDeleteItemMock( + mockContext *mocks.MockContext, + appId string, + federatedCredentialId string, + statusCode int, +) { + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodDelete && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s/federatedIdentityCredentials/%s", appId, federatedCredentialId)) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + return mocks.CreateEmptyHttpResponse(request, statusCode) + }) +} From 697fc672c2d2d4ab593ed1a7bcbd3982be0cfbf3 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Mon, 7 Nov 2022 14:23:56 -0800 Subject: [PATCH 07/15] Addresses PR feedback --- .../pkg/commands/pipeline/azdo_provider.go | 5 +- .../pkg/commands/pipeline/github_provider.go | 6 +- .../pkg/graphsdk/fic_request_builders_test.go | 79 +++++++++++++++++-- cli/azd/test/mocks/graphsdk/mocks.go | 52 ++++++++++-- 4 files changed, 127 insertions(+), 15 deletions(-) diff --git a/cli/azd/pkg/commands/pipeline/azdo_provider.go b/cli/azd/pkg/commands/pipeline/azdo_provider.go index f6e1222e841..5b5540033c3 100644 --- a/cli/azd/pkg/commands/pipeline/azdo_provider.go +++ b/cli/azd/pkg/commands/pipeline/azdo_provider.go @@ -600,7 +600,10 @@ func (p *AzdoCiProvider) configureConnection( console input.Console) error { if authType == AuthTypeOidc { - return fmt.Errorf("Azure DevOps does not support OIDC authentication. Service Principal with client ID and client secret must be used. %w", ErrAuthNotSupported) + return fmt.Errorf( + "Azure DevOps does not support OIDC authentication. Service Principal with client ID and client secret must be used. %w", + ErrAuthNotSupported, + ) } azureCredentials, err := parseCredentials(ctx, credentials) diff --git a/cli/azd/pkg/commands/pipeline/github_provider.go b/cli/azd/pkg/commands/pipeline/github_provider.go index 103ed5e12e3..2d611b89263 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider.go +++ b/cli/azd/pkg/commands/pipeline/github_provider.go @@ -371,13 +371,17 @@ func (p *GitHubCiProvider) configureConnection( if infraOptions.Provider == provisioning.Terraform { // Throw error if Oidc is explicitly requested if authType == AuthTypeOidc { - return fmt.Errorf("Terraform does not support OIDC authentication. Service Principal with client ID and client secret must be used. %w", ErrAuthNotSupported) + return fmt.Errorf( + "Terraform does not support OIDC authentication. Service Principal with client ID and client secret must be used. %w", + ErrAuthNotSupported, + ) } // If not explicitly set, show warning console.Message( ctx, output.WithWarningFormat( + //nolint:lll "WARNING: Terraform provisioning does not support OIDC authentication, defaulting to Service Principal with client ID and client secret.\n", ), ) diff --git a/cli/azd/pkg/graphsdk/fic_request_builders_test.go b/cli/azd/pkg/graphsdk/fic_request_builders_test.go index 2eea0fa671d..0bbd32b30ee 100644 --- a/cli/azd/pkg/graphsdk/fic_request_builders_test.go +++ b/cli/azd/pkg/graphsdk/fic_request_builders_test.go @@ -82,7 +82,13 @@ func TestGetFederatedCredentialById(t *testing.T) { expected := federatedCredentials[0] mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterFederatedCredentialGetItemMock(mockContext, *application.Id, *expected.Id, http.StatusOK, &expected) + graphsdk_mocks.RegisterFederatedCredentialGetItemMock( + mockContext, + *application.Id, + *expected.Id, + http.StatusOK, + &expected, + ) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) @@ -101,7 +107,13 @@ func TestGetFederatedCredentialById(t *testing.T) { t.Run("Error", func(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterFederatedCredentialGetItemMock(mockContext, *application.Id, "bad-id", http.StatusNotFound, nil) + graphsdk_mocks.RegisterFederatedCredentialGetItemMock( + mockContext, + *application.Id, + "bad-id", + http.StatusNotFound, + nil, + ) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) @@ -126,7 +138,9 @@ func TestCreateFederatedCredential(t *testing.T) { client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) - actual, err := client.ApplicationById(*application.Id).FederatedIdentityCredentials().Post(*mockContext.Context, &expected) + actual, err := client.ApplicationById(*application.Id). + FederatedIdentityCredentials(). + Post(*mockContext.Context, &expected) require.NoError(t, err) require.NotNil(t, actual) require.Equal(t, *expected.Id, *actual.Id) @@ -151,13 +165,61 @@ func TestCreateFederatedCredential(t *testing.T) { }) } +func TestPatchFederatedCredential(t *testing.T) { + expected := federatedCredentials[0] + + t.Run("Success", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialPatchItemMock( + mockContext, + *application.Id, + *expected.Id, + http.StatusNoContent, + ) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + err = client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById(*expected.Id). + Update(*mockContext.Context, &expected) + + require.NoError(t, err) + }) + + t.Run("Error", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + graphsdk_mocks.RegisterFederatedCredentialPatchItemMock( + mockContext, + *application.Id, + *expected.Id, + http.StatusBadRequest, + ) + + client, err := graphsdk_mocks.CreateGraphClient(mockContext) + require.NoError(t, err) + + err = client. + ApplicationById(*application.Id). + FederatedIdentityCredentialById(*expected.Id). + Update(*mockContext.Context, &graphsdk.FederatedIdentityCredential{}) + + require.Error(t, err) + }) +} + func TestDeleteFederatedCredential(t *testing.T) { credentialId := "credential-to-delete" t.Run("Success", func(t *testing.T) { - mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterFederatedCredentialDeleteItemMock(mockContext, *application.Id, credentialId, http.StatusNoContent) + graphsdk_mocks.RegisterFederatedCredentialDeleteItemMock( + mockContext, + *application.Id, + credentialId, + http.StatusNoContent, + ) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) @@ -172,7 +234,12 @@ func TestDeleteFederatedCredential(t *testing.T) { t.Run("Error", func(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) - graphsdk_mocks.RegisterFederatedCredentialDeleteItemMock(mockContext, *application.Id, credentialId, http.StatusNotFound) + graphsdk_mocks.RegisterFederatedCredentialDeleteItemMock( + mockContext, + *application.Id, + credentialId, + http.StatusNotFound, + ) client, err := graphsdk_mocks.CreateGraphClient(mockContext) require.NoError(t, err) diff --git a/cli/azd/test/mocks/graphsdk/mocks.go b/cli/azd/test/mocks/graphsdk/mocks.go index 76a83453fc3..713ba22e871 100644 --- a/cli/azd/test/mocks/graphsdk/mocks.go +++ b/cli/azd/test/mocks/graphsdk/mocks.go @@ -66,7 +66,8 @@ func RegisterApplicationDeleteItemMock( statusCode int, ) { mockContext.HttpClient.When(func(request *http.Request) bool { - return request.Method == http.MethodDelete && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s", appId)) + return request.Method == http.MethodDelete && + strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s", appId)) }).RespondFn(func(request *http.Request) (*http.Response, error) { return mocks.CreateEmptyHttpResponse(request, statusCode) }) @@ -245,9 +246,15 @@ func RegisterRoleAssignmentPutMock(mockContext *mocks.MockContext, statusCode in }) } -func RegisterFederatedCredentialsListMock(mockContext *mocks.MockContext, applicationId string, statusCode int, federatedCredentials []graphsdk.FederatedIdentityCredential) { +func RegisterFederatedCredentialsListMock( + mockContext *mocks.MockContext, + applicationId string, + statusCode int, + federatedCredentials []graphsdk.FederatedIdentityCredential, +) { mockContext.HttpClient.When(func(request *http.Request) bool { - return request.Method == http.MethodGet && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s/federatedIdentityCredentials", applicationId)) + return request.Method == http.MethodGet && + strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s/federatedIdentityCredentials", applicationId)) }).RespondFn(func(request *http.Request) (*http.Response, error) { listResponse := graphsdk.FederatedIdentityCredentialListResponse{ Value: federatedCredentials, @@ -261,9 +268,15 @@ func RegisterFederatedCredentialsListMock(mockContext *mocks.MockContext, applic }) } -func RegisterFederatedCredentialCreateItemMock(mockContext *mocks.MockContext, applicationId string, statusCode int, federatedCredential *graphsdk.FederatedIdentityCredential) { +func RegisterFederatedCredentialCreateItemMock( + mockContext *mocks.MockContext, + applicationId string, + statusCode int, + federatedCredential *graphsdk.FederatedIdentityCredential, +) { mockContext.HttpClient.When(func(request *http.Request) bool { - return request.Method == http.MethodPost && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s", applicationId)) + return request.Method == http.MethodPost && + strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s", applicationId)) }).RespondFn(func(request *http.Request) (*http.Response, error) { if federatedCredential == nil { return mocks.CreateEmptyHttpResponse(request, statusCode) @@ -273,6 +286,23 @@ func RegisterFederatedCredentialCreateItemMock(mockContext *mocks.MockContext, a }) } +func RegisterFederatedCredentialPatchItemMock( + mockContext *mocks.MockContext, + applicationId string, + credentialId string, + statusCode int, +) { + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodPatch && + strings.Contains( + request.URL.Path, + fmt.Sprintf("/applications/%s/federatedIdentityCredentials/%s", applicationId, credentialId), + ) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + return mocks.CreateEmptyHttpResponse(request, statusCode) + }) +} + func RegisterFederatedCredentialGetItemMock( mockContext *mocks.MockContext, appId string, @@ -281,7 +311,11 @@ func RegisterFederatedCredentialGetItemMock( federatedCredential *graphsdk.FederatedIdentityCredential, ) { mockContext.HttpClient.When(func(request *http.Request) bool { - return request.Method == http.MethodGet && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s/federatedIdentityCredentials/%s", appId, federatedCredentialId)) + return request.Method == http.MethodGet && + strings.Contains( + request.URL.Path, + fmt.Sprintf("/applications/%s/federatedIdentityCredentials/%s", appId, federatedCredentialId), + ) }).RespondFn(func(request *http.Request) (*http.Response, error) { if federatedCredential == nil { return mocks.CreateEmptyHttpResponse(request, statusCode) @@ -298,7 +332,11 @@ func RegisterFederatedCredentialDeleteItemMock( statusCode int, ) { mockContext.HttpClient.When(func(request *http.Request) bool { - return request.Method == http.MethodDelete && strings.Contains(request.URL.Path, fmt.Sprintf("/applications/%s/federatedIdentityCredentials/%s", appId, federatedCredentialId)) + return request.Method == http.MethodDelete && + strings.Contains( + request.URL.Path, + fmt.Sprintf("/applications/%s/federatedIdentityCredentials/%s", appId, federatedCredentialId), + ) }).RespondFn(func(request *http.Request) (*http.Response, error) { return mocks.CreateEmptyHttpResponse(request, statusCode) }) From d1839c287bb7dbe8538d448804b701c1d3f04127 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Mon, 7 Nov 2022 14:47:21 -0800 Subject: [PATCH 08/15] Renames 'oidc' to 'federated' --- .../pkg/commands/pipeline/azdo_provider.go | 4 ++-- .../pkg/commands/pipeline/github_provider.go | 22 +++++++++---------- .../pkg/commands/pipeline/pipeline_manager.go | 4 ++-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/cli/azd/pkg/commands/pipeline/azdo_provider.go b/cli/azd/pkg/commands/pipeline/azdo_provider.go index 5b5540033c3..b05a482c4b8 100644 --- a/cli/azd/pkg/commands/pipeline/azdo_provider.go +++ b/cli/azd/pkg/commands/pipeline/azdo_provider.go @@ -599,9 +599,9 @@ func (p *AzdoCiProvider) configureConnection( authType PipelineAuthType, console input.Console) error { - if authType == AuthTypeOidc { + if authType == AuthTypeFederated { return fmt.Errorf( - "Azure DevOps does not support OIDC authentication. Service Principal with client ID and client secret must be used. %w", + "Azure DevOps does not support federated authentication. Service Principal with client ID and client secret must be used. %w", ErrAuthNotSupported, ) } diff --git a/cli/azd/pkg/commands/pipeline/github_provider.go b/cli/azd/pkg/commands/pipeline/github_provider.go index 2d611b89263..5e9191d4215 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider.go +++ b/cli/azd/pkg/commands/pipeline/github_provider.go @@ -367,12 +367,12 @@ func (p *GitHubCiProvider) configureConnection( authType PipelineAuthType, console input.Console) error { - // Oidc + Terraform is not a supported combination + // Federated Auth + Terraform is not a supported combination if infraOptions.Provider == provisioning.Terraform { - // Throw error if Oidc is explicitly requested - if authType == AuthTypeOidc { + // Throw error if Federated auth is explicitly requested + if authType == AuthTypeFederated { return fmt.Errorf( - "Terraform does not support OIDC authentication. Service Principal with client ID and client secret must be used. %w", + "Terraform does not support federated authentication. Service Principal with client ID and client secret must be used. %w", ErrAuthNotSupported, ) } @@ -382,7 +382,7 @@ func (p *GitHubCiProvider) configureConnection( ctx, output.WithWarningFormat( //nolint:lll - "WARNING: Terraform provisioning does not support OIDC authentication, defaulting to Service Principal with client ID and client secret.\n", + "WARNING: Terraform provisioning does not support federated authentication, defaulting to Service Principal with client ID and client secret.\n", ), ) } @@ -398,10 +398,10 @@ func (p *GitHubCiProvider) configureConnection( var authErr error switch authType { - case AuthTypeClientSecret: - authErr = p.configureClientSecretAuth(ctx, azdEnvironment, infraOptions, repoSlug, credentials, console) + case AuthTypeClientCredentials: + authErr = p.configureClientCredentialsAuth(ctx, azdEnvironment, infraOptions, repoSlug, credentials, console) default: - authErr = p.configureOidcAuth(ctx, azdEnvironment, infraOptions, repoSlug, credentials, console) + authErr = p.configureFederatedAuth(ctx, azdEnvironment, infraOptions, repoSlug, credentials, console) } if authErr != nil { @@ -417,7 +417,7 @@ func (p *GitHubCiProvider) configureConnection( } // Configures Github for standard Service Principal authentication with client id & secret -func (p *GitHubCiProvider) configureClientSecretAuth( +func (p *GitHubCiProvider) configureClientCredentialsAuth( ctx context.Context, azdEnvironment *environment.Environment, infraOptions provisioning.Options, @@ -493,8 +493,8 @@ func (p *GitHubCiProvider) configureClientSecretAuth( return nil } -// Configures Github for OIDC authentication using registered application with federated identity credentials -func (p *GitHubCiProvider) configureOidcAuth( +// Configures Github for federated authentication using registered application with federated identity credentials +func (p *GitHubCiProvider) configureFederatedAuth( ctx context.Context, azdEnvironment *environment.Environment, infraOptions provisioning.Options, diff --git a/cli/azd/pkg/commands/pipeline/pipeline_manager.go b/cli/azd/pkg/commands/pipeline/pipeline_manager.go index e4dd619b104..f1a794a49cb 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline_manager.go +++ b/cli/azd/pkg/commands/pipeline/pipeline_manager.go @@ -24,8 +24,8 @@ import ( type PipelineAuthType string const ( - AuthTypeOidc PipelineAuthType = "oidc" - AuthTypeClientSecret PipelineAuthType = "client-secret" + AuthTypeFederated PipelineAuthType = "federated" + AuthTypeClientCredentials PipelineAuthType = "client-credentials" ) var ErrAuthNotSupported = errors.New("pipeline authentication configuration is not supported") From b8e36b84911cb9d9575d34d6ea25e0ef2c66c74e Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Mon, 7 Nov 2022 14:53:22 -0800 Subject: [PATCH 09/15] Updates github workflow --- templates/common/.github/workflows/bicep/azure-dev.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/templates/common/.github/workflows/bicep/azure-dev.yml b/templates/common/.github/workflows/bicep/azure-dev.yml index 977d6fbfccc..313ceca8b5f 100644 --- a/templates/common/.github/workflows/bicep/azure-dev.yml +++ b/templates/common/.github/workflows/bicep/azure-dev.yml @@ -9,6 +9,7 @@ on: - main - master +# https://learn.microsoft.com/en-us/azure/developer/github/connect-from-azure?tabs=azure-portal%2Clinux#set-up-azure-login-with-openid-connect-authentication permissions: id-token: write contents: read @@ -35,7 +36,7 @@ jobs: tenant-id: ${{ secrets.AZURE_TENANT_ID }} subscription-id: ${{ secrets.AZURE_SUBSCRIPTION_ID }} - - name: Log in with Azure (Service Principal) + - name: Log in with Azure (Client Credentials) if: ${{ env.AZURE_CREDENTIALS != '' }} uses: azure/login@v1 with: From d199acbd6962f54cacd645ad92e80d749094fca4 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Mon, 7 Nov 2022 15:09:32 -0800 Subject: [PATCH 10/15] Adds output formatting messages --- .../pkg/commands/pipeline/github_provider.go | 17 ++++++++++------- cli/azd/pkg/commands/pipeline/pipeline.go | 5 +++-- .../pkg/commands/pipeline/pipeline_manager.go | 3 ++- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/cli/azd/pkg/commands/pipeline/github_provider.go b/cli/azd/pkg/commands/pipeline/github_provider.go index 5e9191d4215..0920c04d156 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider.go +++ b/cli/azd/pkg/commands/pipeline/github_provider.go @@ -388,7 +388,7 @@ func (p *GitHubCiProvider) configureConnection( } repoSlug := repoDetails.owner + "/" + repoDetails.repoName - console.Message(ctx, fmt.Sprintf("Configuring repository %s.\n", repoSlug)) + console.Message(ctx, fmt.Sprintf("Configuring repository %s.\n", output.WithHighLightFormat(repoSlug))) ghCli := github.NewGitHubCli(ctx) if err := p.ensureAuthorizedForRepoSecrets(ctx, ghCli, console, repoSlug); err != nil { @@ -426,7 +426,7 @@ func (p *GitHubCiProvider) configureClientCredentialsAuth( console input.Console, ) error { ghCli := github.NewGitHubCli(ctx) - console.Message(ctx, "Setting AZURE_CREDENTIALS GitHub repo secret.\n") + console.Message(ctx, fmt.Sprintf("Setting %s GitHub repo secret.\n", output.WithHighLightFormat("AZURE_CREDENTIALS"))) // set azure credential for pipelines can log in to Azure if err := ghCli.SetSecret(ctx, repoSlug, "AZURE_CREDENTIALS", string(credentials)); err != nil { @@ -483,7 +483,7 @@ func (p *GitHubCiProvider) configureClientCredentialsAuth( environment.EnvNameEnvVarName, environment.LocationEnvVarName, environment.SubscriptionIdEnvVarName} { - console.Message(ctx, fmt.Sprintf("Setting %s GitHub repo secret.\n", envName)) + console.Message(ctx, fmt.Sprintf("Setting %s GitHub repo secret.\n", output.WithHighLightFormat(envName))) if err := ghCli.SetSecret(ctx, repoSlug, envName, azdEnvironment.Values[envName]); err != nil { return fmt.Errorf("failed setting %s secret: %w", envName, err) @@ -509,7 +509,7 @@ func (p *GitHubCiProvider) configureFederatedAuth( return fmt.Errorf("failed unmarshalling azure credentials: %w", err) } - err := applyFederatedCredentials(ctx, repoSlug, &azureCredentials) + err := applyFederatedCredentials(ctx, repoSlug, &azureCredentials, console) if err != nil { return err } @@ -523,7 +523,7 @@ func (p *GitHubCiProvider) configureFederatedAuth( } for key, value := range githubSecrets { - console.Message(ctx, fmt.Sprintf("Setting %s GitHub repo secret.\n", key)) + console.Message(ctx, fmt.Sprintf("Setting %s GitHub repo secret.\n", output.WithHighLightFormat(key))) if err := ghCli.SetSecret(ctx, repoSlug, key, value); err != nil { return fmt.Errorf("failed setting github secret '%s': %w", key, err) } @@ -537,7 +537,7 @@ const ( federatedIdentityAudience = "api://AzureADTokenExchange" ) -func applyFederatedCredentials(ctx context.Context, repoSlug string, azureCredentials *azcli.AzureCredentials) error { +func applyFederatedCredentials(ctx context.Context, repoSlug string, azureCredentials *azcli.AzureCredentials, console input.Console) error { graphClient, err := createGraphClient(ctx) if err != nil { return err @@ -582,7 +582,7 @@ func applyFederatedCredentials(ctx context.Context, repoSlug string, azureCreden // Ensure the credential exists otherwise create a new one. for _, fic := range federatedCredentials { - err := ensureFederatedCredential(ctx, graphClient, &application, existingCredsResponse.Value, &fic) + err := ensureFederatedCredential(ctx, graphClient, &application, existingCredsResponse.Value, &fic, console) if err != nil { return err } @@ -751,6 +751,7 @@ func ensureFederatedCredential( application *graphsdk.Application, existingCredentials []graphsdk.FederatedIdentityCredential, repoCredential *graphsdk.FederatedIdentityCredential, + console input.Console, ) error { // If a federated credential already exists for the same subject then nothing to do. for _, existing := range existingCredentials { @@ -774,6 +775,8 @@ func ensureFederatedCredential( return fmt.Errorf("failed creating federated credential: %w", err) } + console.Message(ctx, fmt.Sprintf("Created federated identity credential for GitHub with subject %s\n", output.WithHighLightFormat(repoCredential.Subject))) + return nil } diff --git a/cli/azd/pkg/commands/pipeline/pipeline.go b/cli/azd/pkg/commands/pipeline/pipeline.go index 5fae941df25..536302dadcc 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline.go +++ b/cli/azd/pkg/commands/pipeline/pipeline.go @@ -15,6 +15,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/environment/azdcontext" "github.com/azure/azure-dev/cli/azd/pkg/infra/provisioning" "github.com/azure/azure-dev/cli/azd/pkg/input" + "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/azure/azure-dev/cli/azd/pkg/project" "github.com/azure/azure-dev/cli/azd/pkg/tools" ) @@ -189,14 +190,14 @@ func DetectProviders( if overrideWith == azdoLabel || hasAzDevOpsFolder && !hasGitHubFolder { // Azdo only either by override or by finding only that folder _ = savePipelineProviderToEnv(azdoLabel, env) - console.Message(ctx, "Using pipeline provider: Azure DevOps") + console.Message(ctx, fmt.Sprintf("Using pipeline provider: %s", output.WithHighLightFormat("Azure DevOps"))) return createAzdoScmProvider(env, azdContext), createAzdoCiProvider(env, azdContext), nil } // Both folders exists and no override value. Default to GitHub // Or override value is github and the folder is available _ = savePipelineProviderToEnv(gitHubLabel, env) - console.Message(ctx, "Using pipeline provider: GitHub") + console.Message(ctx, fmt.Sprintf("Using pipeline provider: %s", output.WithHighLightFormat("GitHub"))) return &GitHubScmProvider{}, &GitHubCiProvider{}, nil } diff --git a/cli/azd/pkg/commands/pipeline/pipeline_manager.go b/cli/azd/pkg/commands/pipeline/pipeline_manager.go index f1a794a49cb..c76d09120a1 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline_manager.go +++ b/cli/azd/pkg/commands/pipeline/pipeline_manager.go @@ -14,6 +14,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/environment" "github.com/azure/azure-dev/cli/azd/pkg/environment/azdcontext" "github.com/azure/azure-dev/cli/azd/pkg/input" + "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/azure/azure-dev/cli/azd/pkg/project" "github.com/azure/azure-dev/cli/azd/pkg/tools" "github.com/azure/azure-dev/cli/azd/pkg/tools/azcli" @@ -243,7 +244,7 @@ func (manager *PipelineManager) Configure(ctx context.Context) error { inputConsole.Message( ctx, - fmt.Sprintf("Creating or updating service principal %s.\n", manager.PipelineServicePrincipalName), + fmt.Sprintf("Creating or updating service principal %s.\n", output.WithHighLightFormat(manager.PipelineServicePrincipalName)), ) credentials, err := azCli.CreateOrUpdateServicePrincipal( From 2d2455d420fe4aed116d7e6f4c0df0419ea15253 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Mon, 7 Nov 2022 15:47:26 -0800 Subject: [PATCH 11/15] Update github pipeline provider to default to client credentials for terraform projects --- cli/azd/pkg/commands/pipeline/github_provider.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cli/azd/pkg/commands/pipeline/github_provider.go b/cli/azd/pkg/commands/pipeline/github_provider.go index 0920c04d156..4aa0073facc 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider.go +++ b/cli/azd/pkg/commands/pipeline/github_provider.go @@ -385,6 +385,8 @@ func (p *GitHubCiProvider) configureConnection( "WARNING: Terraform provisioning does not support federated authentication, defaulting to Service Principal with client ID and client secret.\n", ), ) + + authType = AuthTypeClientCredentials } repoSlug := repoDetails.owner + "/" + repoDetails.repoName From 281f8852e48f2fca208c52dccb04509266e7215c Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Mon, 7 Nov 2022 16:08:16 -0800 Subject: [PATCH 12/15] Adds validation check for --auth-type flag --- cli/azd/pkg/commands/pipeline/pipeline_manager.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cli/azd/pkg/commands/pipeline/pipeline_manager.go b/cli/azd/pkg/commands/pipeline/pipeline_manager.go index c76d09120a1..7b35f010ae8 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline_manager.go +++ b/cli/azd/pkg/commands/pipeline/pipeline_manager.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "log" + "strings" "time" "github.com/azure/azure-dev/cli/azd/internal" @@ -20,6 +21,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/tools/azcli" "github.com/azure/azure-dev/cli/azd/pkg/tools/git" "github.com/sethvargo/go-retry" + "golang.org/x/exp/slices" ) type PipelineAuthType string @@ -71,6 +73,14 @@ func (i *PipelineManager) requiredTools(ctx context.Context) []tools.ExternalToo // preConfigureCheck invoke the validations from each provider. func (i *PipelineManager) preConfigureCheck(ctx context.Context) error { + // Validate the authentication types + // auth-type argument must either be an empty string or one of the following values. + validAuthTypes := []string{string(AuthTypeFederated), string(AuthTypeClientCredentials)} + pipelineAuthType := strings.TrimSpace(i.PipelineManagerArgs.PipelineAuthTypeName) + if pipelineAuthType != "" && !slices.Contains(validAuthTypes, pipelineAuthType) { + return fmt.Errorf("pipeline authentication type '%s' is not valid. Valid authentication types are '%s'", i.PipelineManagerArgs.PipelineAuthTypeName, strings.Join(validAuthTypes, ", ")) + } + console := input.GetConsole(ctx) if err := i.ScmProvider.preConfigureCheck(ctx, console); err != nil { return fmt.Errorf("pre-config check error from %s provider: %w", i.ScmProvider.name(), err) From cca4a50f858e11ff042ed9d9b6a0d24ad243cfc3 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Mon, 7 Nov 2022 16:34:28 -0800 Subject: [PATCH 13/15] Fixes long long lint errors --- cli/azd/pkg/commands/pipeline/azdo_provider.go | 1 + cli/azd/pkg/commands/pipeline/github_provider.go | 16 ++++++++++++++-- .../pkg/commands/pipeline/pipeline_manager.go | 11 +++++++++-- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/cli/azd/pkg/commands/pipeline/azdo_provider.go b/cli/azd/pkg/commands/pipeline/azdo_provider.go index b05a482c4b8..a50345850da 100644 --- a/cli/azd/pkg/commands/pipeline/azdo_provider.go +++ b/cli/azd/pkg/commands/pipeline/azdo_provider.go @@ -601,6 +601,7 @@ func (p *AzdoCiProvider) configureConnection( if authType == AuthTypeFederated { return fmt.Errorf( + //nolint:lll "Azure DevOps does not support federated authentication. Service Principal with client ID and client secret must be used. %w", ErrAuthNotSupported, ) diff --git a/cli/azd/pkg/commands/pipeline/github_provider.go b/cli/azd/pkg/commands/pipeline/github_provider.go index 4aa0073facc..569317d243a 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider.go +++ b/cli/azd/pkg/commands/pipeline/github_provider.go @@ -372,6 +372,7 @@ func (p *GitHubCiProvider) configureConnection( // Throw error if Federated auth is explicitly requested if authType == AuthTypeFederated { return fmt.Errorf( + //nolint:lll "Terraform does not support federated authentication. Service Principal with client ID and client secret must be used. %w", ErrAuthNotSupported, ) @@ -539,7 +540,12 @@ const ( federatedIdentityAudience = "api://AzureADTokenExchange" ) -func applyFederatedCredentials(ctx context.Context, repoSlug string, azureCredentials *azcli.AzureCredentials, console input.Console) error { +func applyFederatedCredentials( + ctx context.Context, + repoSlug string, + azureCredentials *azcli.AzureCredentials, + console input.Console, +) error { graphClient, err := createGraphClient(ctx) if err != nil { return err @@ -777,7 +783,13 @@ func ensureFederatedCredential( return fmt.Errorf("failed creating federated credential: %w", err) } - console.Message(ctx, fmt.Sprintf("Created federated identity credential for GitHub with subject %s\n", output.WithHighLightFormat(repoCredential.Subject))) + console.Message( + ctx, + fmt.Sprintf( + "Created federated identity credential for GitHub with subject %s\n", + output.WithHighLightFormat(repoCredential.Subject), + ), + ) return nil } diff --git a/cli/azd/pkg/commands/pipeline/pipeline_manager.go b/cli/azd/pkg/commands/pipeline/pipeline_manager.go index 7b35f010ae8..ab5f931f9cc 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline_manager.go +++ b/cli/azd/pkg/commands/pipeline/pipeline_manager.go @@ -78,7 +78,11 @@ func (i *PipelineManager) preConfigureCheck(ctx context.Context) error { validAuthTypes := []string{string(AuthTypeFederated), string(AuthTypeClientCredentials)} pipelineAuthType := strings.TrimSpace(i.PipelineManagerArgs.PipelineAuthTypeName) if pipelineAuthType != "" && !slices.Contains(validAuthTypes, pipelineAuthType) { - return fmt.Errorf("pipeline authentication type '%s' is not valid. Valid authentication types are '%s'", i.PipelineManagerArgs.PipelineAuthTypeName, strings.Join(validAuthTypes, ", ")) + return fmt.Errorf( + "pipeline authentication type '%s' is not valid. Valid authentication types are '%s'", + i.PipelineManagerArgs.PipelineAuthTypeName, + strings.Join(validAuthTypes, ", "), + ) } console := input.GetConsole(ctx) @@ -254,7 +258,10 @@ func (manager *PipelineManager) Configure(ctx context.Context) error { inputConsole.Message( ctx, - fmt.Sprintf("Creating or updating service principal %s.\n", output.WithHighLightFormat(manager.PipelineServicePrincipalName)), + fmt.Sprintf( + "Creating or updating service principal %s.\n", + output.WithHighLightFormat(manager.PipelineServicePrincipalName), + ), ) credentials, err := azCli.CreateOrUpdateServicePrincipal( From d929b95cb3f2954e625fbe19f4164d9f4a8b856e Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Tue, 8 Nov 2022 13:19:49 -0800 Subject: [PATCH 14/15] Address PR feedback and moves auth type checks to preconfigcheck phase --- .vscode/cspell.global.yaml | 3 +- .../pkg/commands/pipeline/azdo_provider.go | 33 ++++++--- .../commands/pipeline/azdo_provider_test.go | 54 +++++++++++++- .../pkg/commands/pipeline/github_provider.go | 73 ++++++++++++------- .../commands/pipeline/github_provider_test.go | 72 ++++++++++++++++++ cli/azd/pkg/commands/pipeline/pipeline.go | 7 +- .../pkg/commands/pipeline/pipeline_manager.go | 25 ++++--- 7 files changed, 214 insertions(+), 53 deletions(-) diff --git a/.vscode/cspell.global.yaml b/.vscode/cspell.global.yaml index 181d733930c..eac62c1c238 100644 --- a/.vscode/cspell.global.yaml +++ b/.vscode/cspell.global.yaml @@ -82,8 +82,7 @@ ignoreWords: - odata - osdisk - osexec - - OIDC - - Oidc + - oidc - pcert - pdnsz - Peerings diff --git a/cli/azd/pkg/commands/pipeline/azdo_provider.go b/cli/azd/pkg/commands/pipeline/azdo_provider.go index a50345850da..8604d5ab4c1 100644 --- a/cli/azd/pkg/commands/pipeline/azdo_provider.go +++ b/cli/azd/pkg/commands/pipeline/azdo_provider.go @@ -57,7 +57,12 @@ func (p *AzdoScmProvider) requiredTools(_ context.Context) []tools.ExternalTool // preConfigureCheck check the current state of external tools and any // other dependency to be as expected for execution. -func (p *AzdoScmProvider) preConfigureCheck(ctx context.Context, console input.Console) error { +func (p *AzdoScmProvider) preConfigureCheck( + ctx context.Context, + console input.Console, + pipelineManagerArgs PipelineManagerArgs, + infraOptions provisioning.Options, +) error { _, err := azdo.EnsurePatExists(ctx, p.Env, console) if err != nil { return err @@ -572,7 +577,23 @@ func (p *AzdoCiProvider) requiredTools(_ context.Context) []tools.ExternalTool { } // preConfigureCheck nil for Azdo -func (p *AzdoCiProvider) preConfigureCheck(ctx context.Context, console input.Console) error { +func (p *AzdoCiProvider) preConfigureCheck( + ctx context.Context, + console input.Console, + pipelineManagerArgs PipelineManagerArgs, + infraOptions provisioning.Options, +) error { + authType := PipelineAuthType(pipelineManagerArgs.PipelineAuthTypeName) + + if authType == AuthTypeFederated { + return fmt.Errorf( + //nolint:lll + "Azure DevOps does not support federated authentication. To explicitly use client credentials set the %s flag. %w", + output.WithBackticks("--auth-type client-credentials"), + ErrAuthNotSupported, + ) + } + _, err := azdo.EnsurePatExists(ctx, p.Env, console) if err != nil { return err @@ -599,14 +620,6 @@ func (p *AzdoCiProvider) configureConnection( authType PipelineAuthType, console input.Console) error { - if authType == AuthTypeFederated { - return fmt.Errorf( - //nolint:lll - "Azure DevOps does not support federated authentication. Service Principal with client ID and client secret must be used. %w", - ErrAuthNotSupported, - ) - } - azureCredentials, err := parseCredentials(ctx, credentials) if err != nil { return err diff --git a/cli/azd/pkg/commands/pipeline/azdo_provider_test.go b/cli/azd/pkg/commands/pipeline/azdo_provider_test.go index 8d8acbdda81..fdb594795c9 100644 --- a/cli/azd/pkg/commands/pipeline/azdo_provider_test.go +++ b/cli/azd/pkg/commands/pipeline/azdo_provider_test.go @@ -5,11 +5,13 @@ package pipeline import ( "context" + "errors" "path" "testing" "github.com/azure/azure-dev/cli/azd/pkg/azdo" "github.com/azure/azure-dev/cli/azd/pkg/environment" + "github.com/azure/azure-dev/cli/azd/pkg/infra/provisioning" "github.com/azure/azure-dev/cli/azd/pkg/input" "github.com/azure/azure-dev/cli/azd/test/mocks/console" "github.com/azure/azure-dev/cli/azd/test/ostest" @@ -89,7 +91,7 @@ func Test_azdo_provider_preConfigureCheck(t *testing.T) { ctx := context.Background() // act - e := provider.preConfigureCheck(ctx, testConsole) + e := provider.preConfigureCheck(ctx, testConsole, PipelineManagerArgs{}, provisioning.Options{}) // assert require.NoError(t, e) @@ -108,14 +110,46 @@ func Test_azdo_provider_preConfigureCheck(t *testing.T) { ctx := context.Background() // act - e := provider.preConfigureCheck(ctx, testConsole) + e := provider.preConfigureCheck(ctx, testConsole, PipelineManagerArgs{}, provisioning.Options{}) // assert require.Nil(t, e) // PAT is not persisted to .env require.EqualValues(t, "", provider.Env.Values[azdo.AzDoPatName]) }) +} + +func Test_azdo_ci_provider_preconfigure_check(t *testing.T) { + t.Run("success with default options", func(t *testing.T) { + ctx := context.Background() + provider := getAzdoCiProviderTestHarness() + testConsole := console.NewMockConsole() + testPat := "testPAT12345" + testConsole.WhenPrompt(func(options input.ConsoleOptions) bool { + return options.Message == "Personal Access Token (PAT):" + }).Respond(testPat) + + pipelineManagerArgs := PipelineManagerArgs{ + PipelineAuthTypeName: "", + } + + err := provider.preConfigureCheck(ctx, testConsole, pipelineManagerArgs, provisioning.Options{}) + require.NoError(t, err) + }) + + t.Run("fails if auth type is set to federated", func(t *testing.T) { + ctx := context.Background() + provider := getAzdoCiProviderTestHarness() + testConsole := console.NewMockConsole() + + pipelineManagerArgs := PipelineManagerArgs{ + PipelineAuthTypeName: string(AuthTypeFederated), + } + err := provider.preConfigureCheck(ctx, testConsole, pipelineManagerArgs, provisioning.Options{}) + require.Error(t, err) + require.True(t, errors.Is(err, ErrAuthNotSupported)) + }) } func Test_saveEnvironmentConfig(t *testing.T) { @@ -139,6 +173,7 @@ func Test_saveEnvironmentConfig(t *testing.T) { }) } + func getEmptyAzdoScmProviderTestHarness() *AzdoScmProvider { return &AzdoScmProvider{ Env: &environment.Environment{ @@ -161,3 +196,18 @@ func getAzdoScmProviderTestHarness() *AzdoScmProvider { }, } } + +func getAzdoCiProviderTestHarness() *AzdoCiProvider { + return &AzdoCiProvider{ + Env: &environment.Environment{ + Values: map[string]string{ + azdo.AzDoEnvironmentOrgName: "fake_org", + azdo.AzDoEnvironmentProjectName: "project1", + azdo.AzDoEnvironmentProjectIdName: "12345", + azdo.AzDoEnvironmentRepoName: "repo1", + azdo.AzDoEnvironmentRepoIdName: "9876", + azdo.AzDoEnvironmentRepoWebUrl: "https://repo", + }, + }, + } +} diff --git a/cli/azd/pkg/commands/pipeline/github_provider.go b/cli/azd/pkg/commands/pipeline/github_provider.go index 569317d243a..bb5e8c685a2 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider.go +++ b/cli/azd/pkg/commands/pipeline/github_provider.go @@ -48,7 +48,12 @@ func (p *GitHubScmProvider) requiredTools(ctx context.Context) []tools.ExternalT // preConfigureCheck check the current state of external tools and any // other dependency to be as expected for execution. -func (p *GitHubScmProvider) preConfigureCheck(ctx context.Context, console input.Console) error { +func (p *GitHubScmProvider) preConfigureCheck( + ctx context.Context, + console input.Console, + pipelineManagerArgs PipelineManagerArgs, + infraOptions provisioning.Options, +) error { return ensureGitHubLogin(ctx, github.NewGitHubCli(ctx), github.GitHubHostName, console) } @@ -301,8 +306,42 @@ func (p *GitHubCiProvider) requiredTools(ctx context.Context) []tools.ExternalTo // preConfigureCheck validates that current state of tools and GitHub is as expected to // execute. -func (p *GitHubCiProvider) preConfigureCheck(ctx context.Context, console input.Console) error { - return ensureGitHubLogin(ctx, github.NewGitHubCli(ctx), github.GitHubHostName, console) +func (p *GitHubCiProvider) preConfigureCheck( + ctx context.Context, + console input.Console, + pipelineManagerArgs PipelineManagerArgs, + infraOptions provisioning.Options, +) error { + err := ensureGitHubLogin(ctx, github.NewGitHubCli(ctx), github.GitHubHostName, console) + if err != nil { + return err + } + + authType := PipelineAuthType(pipelineManagerArgs.PipelineAuthTypeName) + + // Federated Auth + Terraform is not a supported combination + if infraOptions.Provider == provisioning.Terraform { + // Throw error if Federated auth is explicitly requested + if authType == AuthTypeFederated { + return fmt.Errorf( + //nolint:lll + "Terraform does not support federated authentication. To explicitly use client credentials set the %s flag. %w", + output.WithBackticks("--auth-type client-credentials"), + ErrAuthNotSupported, + ) + } else if authType == "" { + // If not explicitly set, show warning + console.Message( + ctx, + output.WithWarningFormat( + //nolint:lll + "WARNING: Terraform provisioning does not support federated authentication, defaulting to Service Principal with client ID and client secret.\n", + ), + ) + } + } + + return nil } // name returns the name of the provider. @@ -367,29 +406,6 @@ func (p *GitHubCiProvider) configureConnection( authType PipelineAuthType, console input.Console) error { - // Federated Auth + Terraform is not a supported combination - if infraOptions.Provider == provisioning.Terraform { - // Throw error if Federated auth is explicitly requested - if authType == AuthTypeFederated { - return fmt.Errorf( - //nolint:lll - "Terraform does not support federated authentication. Service Principal with client ID and client secret must be used. %w", - ErrAuthNotSupported, - ) - } - - // If not explicitly set, show warning - console.Message( - ctx, - output.WithWarningFormat( - //nolint:lll - "WARNING: Terraform provisioning does not support federated authentication, defaulting to Service Principal with client ID and client secret.\n", - ), - ) - - authType = AuthTypeClientCredentials - } - repoSlug := repoDetails.owner + "/" + repoDetails.repoName console.Message(ctx, fmt.Sprintf("Configuring repository %s.\n", output.WithHighLightFormat(repoSlug))) @@ -398,6 +414,11 @@ func (p *GitHubCiProvider) configureConnection( return fmt.Errorf("ensuring authorization: %w", err) } + // Default auth type to client-credentials for terraform + if infraOptions.Provider == provisioning.Terraform && authType == "" { + authType = AuthTypeClientCredentials + } + var authErr error switch authType { diff --git a/cli/azd/pkg/commands/pipeline/github_provider_test.go b/cli/azd/pkg/commands/pipeline/github_provider_test.go index d012ea513af..8a50b373e96 100644 --- a/cli/azd/pkg/commands/pipeline/github_provider_test.go +++ b/cli/azd/pkg/commands/pipeline/github_provider_test.go @@ -5,8 +5,13 @@ package pipeline import ( "context" + "errors" + "strings" "testing" + "github.com/azure/azure-dev/cli/azd/pkg/exec" + "github.com/azure/azure-dev/cli/azd/pkg/infra/provisioning" + "github.com/azure/azure-dev/cli/azd/test/mocks" "github.com/stretchr/testify/require" ) @@ -35,3 +40,70 @@ func Test_gitHub_provider_getRepoDetails(t *testing.T) { require.EqualValues(t, (*gitRepositoryDetails)(nil), details) }) } + +func Test_gitHub_provider_preConfigure_check(t *testing.T) { + t.Run("success with all default values", func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + setupGithubAuthMock(mockContext) + + provider := &GitHubCiProvider{} + err := provider.preConfigureCheck( + *mockContext.Context, + mockContext.Console, + PipelineManagerArgs{}, + provisioning.Options{}, + ) + require.NoError(t, err) + + // No warnings on console + consoleLog := mockContext.Console.Output() + require.Len(t, consoleLog, 0) + }) + + t.Run("fails with terraform & federated", func(t *testing.T) { + pipelineManagerArgs := PipelineManagerArgs{ + PipelineAuthTypeName: string(AuthTypeFederated), + } + + infraOptions := provisioning.Options{ + Provider: provisioning.Terraform, + } + + mockContext := mocks.NewMockContext(context.Background()) + setupGithubAuthMock(mockContext) + + provider := &GitHubCiProvider{} + err := provider.preConfigureCheck(*mockContext.Context, mockContext.Console, pipelineManagerArgs, infraOptions) + require.Error(t, err) + require.True(t, errors.Is(err, ErrAuthNotSupported)) + }) + + t.Run("warning with terraform & default value", func(t *testing.T) { + pipelineManagerArgs := PipelineManagerArgs{ + PipelineAuthTypeName: "", + } + + infraOptions := provisioning.Options{ + Provider: provisioning.Terraform, + } + + mockContext := mocks.NewMockContext(context.Background()) + setupGithubAuthMock(mockContext) + + provider := &GitHubCiProvider{} + err := provider.preConfigureCheck(*mockContext.Context, mockContext.Console, pipelineManagerArgs, infraOptions) + require.NoError(t, err) + + consoleLog := mockContext.Console.Output() + require.Len(t, consoleLog, 1) + require.Contains(t, consoleLog[0], "WARNING: Terraform provisioning does not support federated authentication") + }) +} + +func setupGithubAuthMock(mockContext *mocks.MockContext) { + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return strings.Contains(command, "gh auth status") + }).RespondFn(func(args exec.RunArgs) (exec.RunResult, error) { + return exec.NewRunResult(0, "", ""), nil + }) +} diff --git a/cli/azd/pkg/commands/pipeline/pipeline.go b/cli/azd/pkg/commands/pipeline/pipeline.go index 536302dadcc..7e85778d9bd 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline.go +++ b/cli/azd/pkg/commands/pipeline/pipeline.go @@ -27,7 +27,12 @@ type subareaProvider interface { // preConfigureCheck validates that the provider's state is ready to be used. // a provider would typically use this method for checking if tools are logged in // of checking if all expected input data is found. - preConfigureCheck(ctx context.Context, console input.Console) error + preConfigureCheck( + ctx context.Context, + console input.Console, + pipelineManagerArgs PipelineManagerArgs, + infraOptions provisioning.Options, + ) error // name returns the name of the provider name() string } diff --git a/cli/azd/pkg/commands/pipeline/pipeline_manager.go b/cli/azd/pkg/commands/pipeline/pipeline_manager.go index ab5f931f9cc..4d0d0638416 100644 --- a/cli/azd/pkg/commands/pipeline/pipeline_manager.go +++ b/cli/azd/pkg/commands/pipeline/pipeline_manager.go @@ -14,6 +14,7 @@ import ( "github.com/azure/azure-dev/cli/azd/internal" "github.com/azure/azure-dev/cli/azd/pkg/environment" "github.com/azure/azure-dev/cli/azd/pkg/environment/azdcontext" + "github.com/azure/azure-dev/cli/azd/pkg/infra/provisioning" "github.com/azure/azure-dev/cli/azd/pkg/input" "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/azure/azure-dev/cli/azd/pkg/project" @@ -72,7 +73,7 @@ func (i *PipelineManager) requiredTools(ctx context.Context) []tools.ExternalToo } // preConfigureCheck invoke the validations from each provider. -func (i *PipelineManager) preConfigureCheck(ctx context.Context) error { +func (i *PipelineManager) preConfigureCheck(ctx context.Context, infraOptions provisioning.Options) error { // Validate the authentication types // auth-type argument must either be an empty string or one of the following values. validAuthTypes := []string{string(AuthTypeFederated), string(AuthTypeClientCredentials)} @@ -86,12 +87,12 @@ func (i *PipelineManager) preConfigureCheck(ctx context.Context) error { } console := input.GetConsole(ctx) - if err := i.ScmProvider.preConfigureCheck(ctx, console); err != nil { - return fmt.Errorf("pre-config check error from %s provider: %w", i.ScmProvider.name(), err) - } - if err := i.CiProvider.preConfigureCheck(ctx, console); err != nil { + if err := i.CiProvider.preConfigureCheck(ctx, console, i.PipelineManagerArgs, infraOptions); err != nil { return fmt.Errorf("pre-config check error from %s provider: %w", i.CiProvider.name(), err) } + if err := i.ScmProvider.preConfigureCheck(ctx, console, i.PipelineManagerArgs, infraOptions); err != nil { + return fmt.Errorf("pre-config check error from %s provider: %w", i.ScmProvider.name(), err) + } return nil } @@ -243,9 +244,15 @@ func (manager *PipelineManager) Configure(ctx context.Context) error { return err } + // Figure out what is the expected provider to use for provisioning + prj, err := project.LoadProjectConfig(manager.AzdCtx.ProjectPath(), manager.Environment) + if err != nil { + return fmt.Errorf("finding provisioning provider: %w", err) + } + // run pre-config validations. manager will check az cli is logged in and // will invoke the per-provider validations. - if errorsFromPreConfig := manager.preConfigureCheck(ctx); errorsFromPreConfig != nil { + if errorsFromPreConfig := manager.preConfigureCheck(ctx, prj.Infra); errorsFromPreConfig != nil { return errorsFromPreConfig } @@ -279,12 +286,6 @@ func (manager *PipelineManager) Configure(ctx context.Context) error { return fmt.Errorf("ensuring git remote: %w", err) } - // Figure out what is the expected provider to use for provisioning - prj, err := project.LoadProjectConfig(manager.AzdCtx.ProjectPath(), manager.Environment) - if err != nil { - return fmt.Errorf("finding provisioning provider: %w", err) - } - err = manager.CiProvider.configureConnection( ctx, manager.Environment, From dfcd92104707348932d2800398380fc33a2da909 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Tue, 8 Nov 2022 13:30:44 -0800 Subject: [PATCH 15/15] Updates test name to fix cspell lint errors --- cli/azd/pkg/commands/pipeline/azdo_provider_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/azd/pkg/commands/pipeline/azdo_provider_test.go b/cli/azd/pkg/commands/pipeline/azdo_provider_test.go index fdb594795c9..a7e992d7629 100644 --- a/cli/azd/pkg/commands/pipeline/azdo_provider_test.go +++ b/cli/azd/pkg/commands/pipeline/azdo_provider_test.go @@ -80,7 +80,7 @@ func Test_azdo_provider_getRepoDetails(t *testing.T) { }) } -func Test_azdo_provider_preConfigureCheck(t *testing.T) { +func Test_azdo_scm_provider_preConfigureCheck(t *testing.T) { t.Run("accepts a PAT via system environment variables", func(t *testing.T) { // arrange testPat := "12345" @@ -119,7 +119,7 @@ func Test_azdo_provider_preConfigureCheck(t *testing.T) { }) } -func Test_azdo_ci_provider_preconfigure_check(t *testing.T) { +func Test_azdo_ci_provider_preConfigureCheck(t *testing.T) { t.Run("success with default options", func(t *testing.T) { ctx := context.Background() provider := getAzdoCiProviderTestHarness()