From 5b4ecd38ca66b4b103238d5be0b0a56d97f42e75 Mon Sep 17 00:00:00 2001 From: trangevi Date: Tue, 27 Oct 2026 11:39:55 -0700 Subject: [PATCH 1/4] fix(toolbox): prepare Foundry access before deploy and create Limit access preparation to managed deployment and standalone create after local validation and before remote connection resolution. Reuse shared probe-first policy with verified project scope, user-tenant credentials, cancellation, and stderr progress. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../extensions/azure.ai.toolboxes/README.md | 43 ++ cli/azd/extensions/azure.ai.toolboxes/go.mod | 1 + cli/azd/extensions/azure.ai.toolboxes/go.sum | 2 + .../internal/cmd/context.go | 59 +- .../azure.ai.toolboxes/internal/cmd/root.go | 6 +- .../internal/cmd/service_target.go | 92 +-- .../cmd/testdata/TestAIHelp-create.snap | 4 + .../cmd/testdata/TestAIHelp-root.snap | 4 + .../internal/cmd/toolbox_access.go | 152 +++++ .../internal/cmd/toolbox_access_test.go | 526 ++++++++++++++++++ .../internal/cmd/toolbox_create.go | 122 +++- .../internal/exterrors/errors.go | 11 +- 12 files changed, 950 insertions(+), 72 deletions(-) create mode 100644 cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go create mode 100644 cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go diff --git a/cli/azd/extensions/azure.ai.toolboxes/README.md b/cli/azd/extensions/azure.ai.toolboxes/README.md index ffe8e6f93ee..df4f9f13d2f 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/README.md +++ b/cli/azd/extensions/azure.ai.toolboxes/README.md @@ -67,6 +67,49 @@ For direct resource operations: Removing a service from `azure.yaml` stops managing the toolbox but does not delete the remote resource; use `azd ai toolbox delete` to delete it. +## Developer access for deployment and create + +Managed toolbox deployment and `azd ai toolbox create` prepare Foundry access +independently of any agent predeploy hook. Subscription **Owner** or +**Contributor** alone does not grant data-plane access. After local input +validation, the extension probes toolbox read access before resolving remote +connections or creating a version. A successful read or a toolbox-not-found +response does not trigger a role assignment. + +Automatic grants require `AZURE_AI_PROJECT_ID` to identify the project selected +by `FOUNDRY_PROJECT_ENDPOINT` (or its `AZURE_AI_PROJECT_ENDPOINT` fallback). +The operation endpoint must match that project, including when explicitly +selected with `--project-endpoint`. If `AZURE_SUBSCRIPTION_ID` is set, it must +agree with the project ID. Unrelated endpoints and endpoint-only usage do not +borrow the active project's scope or tenant and do not receive automatic grants. +Managed deployment retains its existing subscription/user-tenant credential +fallback when matching project metadata is unavailable. + +Only a confirmed authorization denial triggers inherited/equivalent role +inspection and, if needed, a project-scoped **Foundry User** assignment. +Credentials use the subscription's user-access tenant; principal lookup uses +the project's resource tenant, including for guest users and service principals. +New grants are followed by bounded readiness checks. If propagation takes longer, +wait and retry. Inspection or assignment failures produce a warning and let the +requested operation determine the final result. An assignment-denied warning +includes an administrator command for the verified project scope. + +Set `AZD_AGENT_SKIP_ROLE_ASSIGNMENTS=true` in the active azd environment or +process environment to disable these automatic checks and grants. A non-empty +azd environment value takes precedence; an absent or empty value falls back to +the process environment. Values are parsed as booleans; invalid values do not +enable skipping. Existing externally managed access is still required. + +Warnings and grant/propagation progress go to stderr, preserving JSON results +on stdout. Read/list/show, delete, connection and skill mutations, publish, +local authoring, and bring-your-own endpoint reuse do not prepare access. +Provisioning remains independent of developer role-assignment permission. + +The adapter uses core `pkg/foundry.AccessPreparation`. Release the core helper +and bump this extension's azd dependency before merging/releasing the extension; +an external Go workspace can validate both modules before that release without +committing a local `replace` or inventing a dependency version. + ## Extension telemetry API Extension code can report best-effort usage events through the shared diff --git a/cli/azd/extensions/azure.ai.toolboxes/go.mod b/cli/azd/extensions/azure.ai.toolboxes/go.mod index a9055bba52d..2ac9a223241 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/go.mod +++ b/cli/azd/extensions/azure.ai.toolboxes/go.mod @@ -5,6 +5,7 @@ go 1.26.4 require ( github.com/Azure/azure-sdk-for-go/sdk/azcore v1.23.3 github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.15.0 + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2 v2.2.0 github.com/azure/azure-dev/cli/azd v1.37.0 github.com/fatih/color v1.19.0 github.com/spf13/cobra v1.10.2 diff --git a/cli/azd/extensions/azure.ai.toolboxes/go.sum b/cli/azd/extensions/azure.ai.toolboxes/go.sum index 586d90f7326..42c3a1b2d99 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/go.sum +++ b/cli/azd/extensions/azure.ai.toolboxes/go.sum @@ -13,6 +13,8 @@ github.com/Azure/azure-sdk-for-go/sdk/internal v1.13.0 h1:Terj1CMW4+NY5CpWjSRkqL github.com/Azure/azure-sdk-for-go/sdk/internal v1.13.0/go.mod h1:7Ut2Qw9vcwpLW5Q4MBy43uSxXS7tK2yjs94OJ6j5thE= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/appservice/armappservice/v2 v2.3.0 h1:JI8PcWOImyvIUEZ0Bbmfe05FOlWkMi2KhjG+cAKaUms= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/appservice/armappservice/v2 v2.3.0/go.mod h1:nJLFPGJkyKfDDyJiPuHIXsCi/gpJkm07EvRgiX7SGlI= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2 v2.2.0 h1:Hp+EScFOu9HeCbeW8WU2yQPJd4gGwhMgKxWe+G6jNzw= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2 v2.2.0/go.mod h1:/pz8dyNQe+Ey3yBp/XuYz7oqX8YDNWVpPB0hH3XWfbc= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0 h1:PTFGRSlMKCQelWwxUyYVEUqseBJVemLyqWJjvMyt0do= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0/go.mod h1:LRr2FzBTQlONPPa5HREE5+RjSCTXl7BwOvYOaWTqCaI= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.0 h1:2qsIIvxVT+uE6yrNldntJKlLRgxGbZ85kgtz5SNBhMw= diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/context.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/context.go index 4fdb0af69d3..3e228e35f55 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/context.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/context.go @@ -8,18 +8,19 @@ import ( "context" "errors" "fmt" + "io" "log" "net/http" "regexp" "strings" "azure.ai.toolboxes/internal/exterrors" - "azure.ai.toolboxes/internal/foundry" "azure.ai.toolboxes/internal/foundry/projectctx" "azure.ai.toolboxes/internal/pkg/azure" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/azure/azure-dev/cli/azd/pkg/foundry" "github.com/spf13/cobra" ) @@ -36,6 +37,24 @@ var toolboxNamePattern = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) // maxToolboxNameLength caps positional names. const maxToolboxNameLength = 63 +func validateToolboxWriteFlags(cmd *cobra.Command) error { + for _, name := range []string{"project-endpoint", "from-file"} { + if !cmd.Flags().Changed(name) { + continue + } + value, err := cmd.Flags().GetString(name) + if err != nil { + return err + } + if strings.TrimSpace(value) == "" { + return exterrors.Validation(exterrors.CodeInvalidParameter, + fmt.Sprintf("--%s must not be empty when supplied", name), + "provide a non-empty value, or omit the flag if it is optional") + } + } + return nil +} + // readToolboxFlags extracts the persistent flag values from a subcommand. The // reserved azd globals `--output` and `--no-prompt` come from extCtx. `output` // is normalized to lowercase so downstream branches can compare with `== "json"`. @@ -120,6 +139,18 @@ func newToolboxClientForTenant(endpoint, tenantID string) (*azure.FoundryToolbox return azure.NewFoundryToolboxClient(endpoint, cred), nil } +func toolboxTenantID(ctx context.Context, account foundry.TenantResolver, subscriptionID string) (string, error) { + tenantID, err := foundry.LookupProjectTenant(ctx, account, subscriptionID) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return "", cancelErr + } + if err != nil { + return "", exterrors.Auth(exterrors.CodeTenantLookupFailed, + "could not resolve the Foundry project's user-access tenant", "run 'azd auth login' and retry") + } + return tenantID, nil +} + // resolveToolboxAndClient walks the endpoint cascade, validates flags, and // returns a toolbox client bound to the resolved endpoint. func resolveToolboxAndClient( @@ -139,6 +170,32 @@ func resolveToolboxAndClient( return client, resolved, nil } +// resolveToolboxWriteAndClient is deliberately separate from read/delete resolution. +func resolveToolboxWriteAndClient( + ctx context.Context, flags toolboxFlags, name string, writer io.Writer, +) (toolboxClient, *projectctx.Resolved, error) { + if err := validateOutputFormat(flags.output); err != nil { + return nil, nil, err + } + resolved, err := projectctx.Resolve(ctx, projectctx.ResolveOpts{FlagValue: flags.projectEndpoint}) + if err != nil { + return nil, nil, err + } + host, err := azdext.NewAzdClient() + if err != nil { + fmt.Fprintln(writer, "Warning: No azd host is available; Foundry roles will not be assigned automatically.") + client, clientErr := newToolboxClient(resolved.Endpoint) + return client, resolved, clientErr + } + defer host.Close() + access := newToolboxAccess(host) + access.writer = writer + client, err := access.writeClient(ctx, resolved.Endpoint, name, func(endpoint, tenantID string) (toolboxClient, error) { + return newToolboxClientForTenant(endpoint, tenantID) + }) + return client, resolved, err +} + // isAzureNotFound reports whether err originates from an Azure response with HTTP 404. func isAzureNotFound(err error) bool { if err == nil { diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/root.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/root.go index f09ee4fe32c..8e80380117d 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/root.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/root.go @@ -25,7 +25,11 @@ A toolbox is a versioned, named collection of connection-backed tools that agents reference at run time. Each version is immutable: mutations (connection add/remove, skill add/remove) create a new version but never change which version is the default. Use 'azd ai toolbox publish ' -to promote a version.`, +to promote a version. + +Managed deployment and 'create' check Foundry access and may assign Foundry User +when the target matches the active environment's project resource ID and endpoint. +Other commands do not assign roles. Provisioning does not assign developer access.`, }) rootCmd.SilenceUsage = true diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go index c3c8aeb6be5..d58cc9bc562 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "fmt" + "maps" "strings" "azure.ai.toolboxes/internal/definition" @@ -52,10 +53,11 @@ type toolboxServiceConfig struct { // artifact. type toolboxServiceTarget struct { environmentClient environmentReader - accountClient accountTenantLookup + accountClient foundry.TenantResolver projectClient serviceConfigReader resolver connectionResolver newClient func(endpoint, tenantID string) (toolboxClient, error) + access *toolboxAccess } // newToolboxServiceTarget creates the azure.ai.toolbox service-target provider. @@ -67,6 +69,7 @@ func newToolboxServiceTarget( accountClient: azdClient.Account(), projectClient: azdClient.Project(), resolver: defaultConnectionResolver{}, + access: newToolboxAccess(azdClient), newClient: func(endpoint, tenantID string) (toolboxClient, error) { return newToolboxClientForTenant(endpoint, tenantID) }, @@ -167,25 +170,23 @@ func (p *toolboxServiceTarget) Deploy( if err != nil { return nil, err } - rawTools, err := p.buildToolEntries( - ctx, - endpoint, - cfg.Tools, - environment, - ) - if err != nil { + if err := validateToolboxName(name); err != nil { return nil, err } - request, err := buildToolboxVersionRequest(ctx, p.resolver, endpoint, &definition.Definition{ + tools := make([]map[string]any, 0, len(cfg.Tools)) + for _, tool := range cfg.Tools { + tools = append(tools, expandToolboxValue(tool, environment).(map[string]any)) + } + input := &definition.Definition{ Name: name, Description: cfg.Description, Connections: cfg.Connections, Skills: cfg.Skills, - Tools: rawTools, + Tools: tools, Policies: cfg.Policies, Metadata: cfg.Metadata, - }) - if err != nil { + } + if err := validateToolboxDefinition(input); err != nil { return nil, err } @@ -193,11 +194,26 @@ func (p *toolboxServiceTarget) Deploy( progress(fmt.Sprintf("Upserting toolbox %q", name)) } - tenantID, err := p.credentialTenantID(ctx) + var scope *toolboxAccessScope + if p.access != nil { + scope, err = p.access.resolve(ctx, endpoint) + if err != nil { + return nil, err + } + } + var tenantID string + if scope != nil { + tenantID, err = toolboxTenantID(ctx, p.accountClient, scope.resource.SubscriptionID) + } else { + tenantID, err = p.credentialTenantID(ctx) + } if err != nil { return nil, err } client, err := p.newClient(endpoint, tenantID) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return nil, cancelErr + } if err != nil { return nil, exterrors.Auth( exterrors.CodeCredentialCreationFailed, @@ -206,9 +222,23 @@ func (p *toolboxServiceTarget) Deploy( ) } + if p.access != nil { + if err := p.access.prepare(ctx, scope, tenantID, client, name); err != nil { + return nil, err + } + } + input.Tools, err = p.buildToolEntries(ctx, endpoint, tools, nil) + if err != nil { + return nil, err + } + request, err := buildToolboxVersionRequest(ctx, p.resolver, endpoint, input) + if err != nil { + return nil, err + } + created, err := client.CreateToolboxVersion(ctx, name, request) if err != nil { - return nil, fmt.Errorf("upserting toolbox %q: %w", name, err) + return nil, exterrors.ServiceFromAzure(err, exterrors.OpCreateToolboxVersion) } // Surface the toolbox's MCP endpoint to agents (and the developer) without re-running. @@ -322,9 +352,9 @@ func (p *toolboxServiceTarget) buildToolEntries( ) ([]map[string]any, error) { out := make([]map[string]any, 0, len(tools)) for _, tool := range tools { - entry, ok := expandToolboxValue(tool, env).(map[string]any) - if !ok { - continue + entry := maps.Clone(tool) + if env != nil { + entry = expandToolboxValue(tool, env).(map[string]any) } if connName, isString := entry["connection"].(string); isString && connName != "" { conn, err := p.resolver.resolveConnection(ctx, endpoint, connName) @@ -446,14 +476,6 @@ type environmentReader interface { ) (*azdext.KeyValueResponse, error) } -type accountTenantLookup interface { - LookupTenant( - ctx context.Context, - in *azdext.LookupTenantRequest, - opts ...grpc.CallOption, - ) (*azdext.LookupTenantResponse, error) -} - func (p *toolboxServiceTarget) credentialTenantID(ctx context.Context) (string, error) { current, err := p.environmentClient.GetCurrent(ctx, &azdext.EmptyRequest{}) if err != nil { @@ -488,25 +510,7 @@ func (p *toolboxServiceTarget) credentialTenantID(ctx context.Context) (string, "set AZURE_SUBSCRIPTION_ID in the active azd environment and retry", ) } - tenantResponse, err := p.accountClient.LookupTenant(ctx, &azdext.LookupTenantRequest{ - SubscriptionId: subscriptionID, - }) - if err != nil { - return "", exterrors.Auth( - exterrors.CodeTenantLookupFailed, - fmt.Sprintf("failed to get tenant ID for subscription %s: %s", subscriptionID, err), - "verify your Azure login with 'azd auth login' and that you have access to this subscription", - ) - } - tenantID := strings.TrimSpace(tenantResponse.GetTenantId()) - if tenantID == "" { - return "", exterrors.Auth( - exterrors.CodeTenantLookupFailed, - fmt.Sprintf("azd returned an empty tenant ID for subscription %s", subscriptionID), - "verify your Azure login with 'azd auth login' and that you have access to this subscription", - ) - } - return tenantID, nil + return toolboxTenantID(ctx, p.accountClient, subscriptionID) } func serviceEnvDeclared( diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/testdata/TestAIHelp-create.snap b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/testdata/TestAIHelp-create.snap index 85fec5ef541..8fef0620ae7 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/testdata/TestAIHelp-create.snap +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/testdata/TestAIHelp-create.snap @@ -92,6 +92,10 @@ environment under the TOOLBOX__MCP_ENDPOINT variable (the same key agents consume), where is the toolbox name uppercased with non-alphanumeric character runs replaced by underscores. +After local validation, checks Foundry access before resolving connections. +May assign Foundry User when the target matches the active environment's +project resource ID and endpoint. Access warnings and progress go to stderr. + Usage azd ai toolbox create --from-file [flags] diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/testdata/TestAIHelp-root.snap b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/testdata/TestAIHelp-root.snap index 7c258ffd46f..f291177d5e1 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/testdata/TestAIHelp-root.snap +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/testdata/TestAIHelp-root.snap @@ -6,6 +6,10 @@ add/remove, skill add/remove) create a new version but never change which version is the default. Use 'azd ai toolbox publish ' to promote a version. +Managed deployment and 'create' check Foundry access and may assign Foundry User +when the target matches the active environment's project resource ID and endpoint. +Other commands do not assign roles. Provisioning does not assign developer access. + Usage azd ai toolbox [command] diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go new file mode 100644 index 00000000000..3764dfcaffb --- /dev/null +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go @@ -0,0 +1,152 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package cmd + +import ( + "context" + "errors" + "fmt" + "io" + "os" + + "azure.ai.toolboxes/internal/exterrors" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/azure/azure-dev/cli/azd/pkg/foundry" +) + +type toolboxAccessAccount interface { + foundry.TenantResolver + foundry.PrincipalResolver +} + +type toolboxAccess struct { + environment foundry.ProjectEnvironment + account toolboxAccessAccount + newCredential func(string) (azcore.TokenCredential, error) + roles foundry.ProjectRoles + writer io.Writer +} + +type toolboxAccessScope struct { + resource *arm.ResourceID + skip bool +} + +func newToolboxAccess(client *azdext.AzdClient) *toolboxAccess { + return &toolboxAccess{ + environment: client.Environment(), + account: client.Account(), + newCredential: foundry.NewCredential, + writer: os.Stderr, + } +} + +func (a *toolboxAccess) warn(message string) { + fmt.Fprintln(a.writer, "Warning: "+message) +} + +func (a *toolboxAccess) resolve(ctx context.Context, endpoint string) (*toolboxAccessScope, error) { + values, err := foundry.LoadProjectEnvironment(ctx, a.environment) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return nil, cancelErr + } + if err != nil { + a.warn("Could not read the active project scope; Foundry roles will not be assigned automatically.") + return nil, nil + } + resource, err := foundry.ProjectScopeFromEnvironment(endpoint, values) + if err != nil { + return nil, exterrors.Validation(exterrors.CodeInvalidParameter, err.Error(), + "run 'azd provision' or correct the active environment's Foundry project ID, endpoint, and subscription") + } + if resource == nil { + a.warn("No matching active project resource ID is available; Foundry roles will not be assigned automatically.") + return nil, nil + } + return &toolboxAccessScope{ + resource: resource, + skip: foundry.RoleAssignmentsSkipped( + values["AZD_AGENT_SKIP_ROLE_ASSIGNMENTS"], os.Getenv("AZD_AGENT_SKIP_ROLE_ASSIGNMENTS")), + }, nil +} + +func (a *toolboxAccess) prepare( + ctx context.Context, scope *toolboxAccessScope, tenantID string, client toolboxClient, name string, +) error { + if scope == nil || scope.skip { + return nil + } + credential, err := a.newCredential(tenantID) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } + if err != nil { + a.warn("Could not create the Foundry role-check credential; continuing without assigning a role.") + return nil + } + prepare := foundry.AccessPreparation{ + Account: a.account, + Credential: credential, + Roles: a.roles, + Scope: scope.resource.String(), + Probe: func(ctx context.Context) error { + _, err := client.GetToolbox(ctx, name) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } + if isAzureNotFound(err) { + return nil + } + return err + }, + Warn: a.warn, + Progress: func(message string) { + fmt.Fprintln(a.writer, message) + }, + } + if err := prepare.Ensure(ctx); err != nil { + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } + serviceErr := exterrors.ServiceFromAzure(err, exterrors.OpGetToolbox) + if structured, ok := errors.AsType[*azdext.ServiceError](serviceErr); ok { + structured.Suggestion = "Foundry User was assigned on the project but access has not propagated. " + + "Wait and retry; if access is still denied, ask an administrator to verify project access." + } + return serviceErr + } + return nil +} + +func (a *toolboxAccess) writeClient( + ctx context.Context, endpoint, name string, + newClient func(string, string) (toolboxClient, error), +) (toolboxClient, error) { + scope, err := a.resolve(ctx, endpoint) + if err != nil { + return nil, err + } + var tenantID string + if scope != nil { + tenantID, err = toolboxTenantID(ctx, a.account, scope.resource.SubscriptionID) + if err != nil { + return nil, err + } + } + client, err := newClient(endpoint, tenantID) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return nil, cancelErr + } + if err != nil { + return nil, exterrors.Auth(exterrors.CodeCredentialCreationFailed, + "could not create the Foundry credential", "run 'azd auth login' and retry") + } + if err := a.prepare(ctx, scope, tenantID, client, name); err != nil { + return nil, err + } + return client, nil +} diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go new file mode 100644 index 00000000000..526eb28231d --- /dev/null +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go @@ -0,0 +1,526 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package cmd + +import ( + "bytes" + "context" + "errors" + "io" + "net" + "net/http" + "strings" + "sync/atomic" + "testing" + + "azure.ai.toolboxes/internal/definition" + "azure.ai.toolboxes/internal/exterrors" + "azure.ai.toolboxes/internal/foundry/projectctx" + "azure.ai.toolboxes/internal/pkg/azure" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2" + "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/protobuf/types/known/structpb" +) + +const ( + toolboxAccessTestEndpoint = "https://acct.services.ai.azure.com/api/projects/proj" + toolboxAccessTestID = "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.CognitiveServices/" + + "accounts/acct/projects/proj" +) + +type accessEnvironment struct { + stubEnvironmentReader + values map[string]string + err error +} + +func (e *accessEnvironment) GetValues( + context.Context, *azdext.GetEnvironmentRequest, ...grpc.CallOption, +) (*azdext.KeyValueListResponse, error) { + if e.err != nil { + return nil, e.err + } + var entries []*azdext.KeyValue + for key, value := range e.values { + entries = append(entries, &azdext.KeyValue{Key: key, Value: value}) + } + return &azdext.KeyValueListResponse{KeyValues: entries}, nil +} + +type accessAccount struct { + stubAccountTenantLookup + principalType azdext.PrincipalType + principalID string + principalSub string +} + +func (a *accessAccount) GetCurrentPrincipal( + _ context.Context, req *azdext.GetCurrentPrincipalRequest, _ ...grpc.CallOption, +) (*azdext.GetCurrentPrincipalResponse, error) { + a.principalSub = req.GetSubscriptionId() + return &azdext.GetCurrentPrincipalResponse{ + ObjectId: a.principalID, PrincipalType: a.principalType, + }, nil +} + +type accessRoles struct { + events *[]string + assigned bool + assignErr error + principalID string + principal armauthorization.PrincipalType +} + +func (r *accessRoles) HasAccess(context.Context, string, string) (bool, error) { + *r.events = append(*r.events, "roles") + return false, nil +} + +func (r *accessRoles) Assign(_ context.Context, scope, id string, kind armauthorization.PrincipalType) error { + if scope != toolboxAccessTestID { + return errors.New("wrong scope") + } + *r.events = append(*r.events, "grant") + r.assigned = r.assignErr == nil + r.principalID, r.principal = id, kind + return r.assignErr +} + +type accessToolboxClient struct { + *mockToolboxClient + roles *accessRoles + events *[]string + probeErr error +} + +func (c *accessToolboxClient) GetToolbox(ctx context.Context, name string) (*azure.ToolboxObject, error) { + *c.events = append(*c.events, "read") + if c.probeErr != nil { + return nil, c.probeErr + } + if !c.roles.assigned { + return nil, &azcore.ResponseError{StatusCode: 403, ErrorCode: "PermissionDenied"} + } + return c.mockToolboxClient.GetToolbox(ctx, name) +} + +func (c *accessToolboxClient) CreateToolboxVersion( + ctx context.Context, name string, req *azure.CreateToolboxVersionRequest, +) (*azure.ToolboxVersionObject, error) { + *c.events = append(*c.events, "create") + if !c.roles.assigned { + return nil, &azcore.ResponseError{StatusCode: 403, ErrorCode: "PermissionDenied"} + } + return c.mockToolboxClient.CreateToolboxVersion(ctx, name, req) +} + +type accessConnectionResolver struct { + events *[]string + roles *accessRoles +} + +func (r accessConnectionResolver) resolveConnection( + _ context.Context, _, name string, +) (*projectConnection, error) { + *r.events = append(*r.events, "connection") + if !r.roles.assigned { + return nil, errors.New("connection data-plane access before developer grant") + } + return &projectConnection{ID: "/connections/" + name, Name: name, Category: "RemoteTool", Target: "https://mcp.test"}, nil +} + +func accessTestSetup( + t *testing.T, events *[]string, kind azdext.PrincipalType, +) (*toolboxAccess, *accessToolboxClient, *accessAccount) { + t.Helper() + t.Setenv("AZD_AGENT_SKIP_ROLE_ASSIGNMENTS", "") + account := &accessAccount{ + stubAccountTenantLookup: stubAccountTenantLookup{tenantID: "user-access-tenant"}, + principalType: kind, principalID: "resource-tenant-object-id", + } + roles := &accessRoles{events: events} + client := &accessToolboxClient{ + mockToolboxClient: newMockToolboxClient(toolboxAccessTestEndpoint), roles: roles, events: events, + } + access := &toolboxAccess{ + environment: &accessEnvironment{ + stubEnvironmentReader: stubEnvironmentReader{name: "test", subscriptionID: "sub"}, + values: map[string]string{ + "AZURE_AI_PROJECT_ID": toolboxAccessTestID, "FOUNDRY_PROJECT_ENDPOINT": toolboxAccessTestEndpoint, + "AZURE_SUBSCRIPTION_ID": "sub", + }, + }, + account: account, + newCredential: func(tenantID string) (azcore.TokenCredential, error) { + assert.Equal(t, "user-access-tenant", tenantID) + return nil, nil + }, + roles: roles, writer: io.Discard, + } + return access, client, account +} + +func TestToolboxCreatePreparesAccessBeforeConnections(t *testing.T) { + for _, kind := range []azdext.PrincipalType{ + azdext.PrincipalType_PRINCIPAL_TYPE_USER, azdext.PrincipalType_PRINCIPAL_TYPE_SERVICE_PRINCIPAL, + } { + t.Run(kind.String(), func(t *testing.T) { + stubToolboxEndpointEnv(t) + var events []string + access, client, account := accessTestSetup(t, &events, kind) + var diagnostics bytes.Buffer + access.writer = &diagnostics + prepared, err := access.writeClient(t.Context(), toolboxAccessTestEndpoint, "research", + func(endpoint, tenantID string) (toolboxClient, error) { + assert.Equal(t, toolboxAccessTestEndpoint, endpoint) + assert.Equal(t, "user-access-tenant", tenantID) + return client, nil + }) + require.NoError(t, err) + assert.Equal(t, "sub", account.request.GetSubscriptionId()) + assert.Equal(t, "sub", account.principalSub) + assert.Equal(t, "resource-tenant-object-id", client.roles.principalID) + expectedKind := armauthorization.PrincipalTypeUser + if kind == azdext.PrincipalType_PRINCIPAL_TYPE_SERVICE_PRINCIPAL { + expectedKind = armauthorization.PrincipalTypeServicePrincipal + } + assert.Equal(t, expectedKind, client.roles.principal) + err = createToolboxFromDefinition(t.Context(), prepared, + accessConnectionResolver{events: &events, roles: client.roles}, + toolboxAccessTestEndpoint, "research", + &definition.Definition{Connections: []definition.ConnectionReference{{Name: "mcp"}}}, + toolboxFlags{output: "json"}) + require.NoError(t, err) + assert.Equal(t, []string{"read", "roles", "grant", "read", "read", "connection", "create"}, events) + assert.Equal(t, "Assigning Foundry User to the current principal.\n"+ + "Waiting for Foundry access to propagate.\nFoundry access is ready.\n", diagnostics.String()) + }) + } +} + +func toolboxDeployEndpoint(t *testing.T) { + t.Helper() + old := projectctx.ReadAzdHostedSourcesFunc + projectctx.ReadAzdHostedSourcesFunc = func(context.Context) (projectctx.AzdHostedSources, error) { + return projectctx.AzdHostedSources{EnvValue: toolboxAccessTestEndpoint, EnvName: "test"}, nil + } + t.Cleanup(func() { projectctx.ReadAzdHostedSourcesFunc = old }) +} + +func TestToolboxDeployPreparesAccessWithoutAgentHook(t *testing.T) { + stubToolboxEndpointEnv(t) + toolboxDeployEndpoint(t) + var events []string + access, client, account := accessTestSetup(t, &events, azdext.PrincipalType_PRINCIPAL_TYPE_USER) + environment, ok := access.environment.(environmentReader) + require.True(t, ok) + target := &toolboxServiceTarget{ + environmentClient: environment, accountClient: account, access: access, + resolver: accessConnectionResolver{events: &events, roles: client.roles}, + newClient: func(endpoint, tenantID string) (toolboxClient, error) { + assert.Equal(t, toolboxAccessTestEndpoint, endpoint) + assert.Equal(t, "user-access-tenant", tenantID) + return client, nil + }, + } + properties, err := structpb.NewStruct(map[string]any{ + "tools": []any{map[string]any{"type": "mcp", "connection": "${CONNECTION}"}}, + }) + require.NoError(t, err) + _, err = target.Deploy(t.Context(), &azdext.ServiceConfig{ + Name: "research", AdditionalProperties: properties, Environment: map[string]string{"CONNECTION": "mcp"}, + }, nil, nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{"read", "roles", "grant", "read", "connection", "create"}, events) + require.Len(t, client.createVersionCalls, 1) + assert.NotContains(t, client.createVersionCalls[0].req.Tools[0], "connection") +} + +func TestToolboxAccessDoesNotGrantForUnscopedSkippedOrAuthorizedOperations(t *testing.T) { + for _, mode := range []string{"unrelated endpoint", "missing scope", "skip", "managed access", "404", "unknown 403"} { + t.Run(mode, func(t *testing.T) { + var events []string + access, client, _ := accessTestSetup(t, &events, azdext.PrincipalType_PRINCIPAL_TYPE_USER) + environment := access.environment.(*accessEnvironment) + var diagnostics bytes.Buffer + access.writer = &diagnostics + switch mode { + case "unrelated endpoint": + environment.values["FOUNDRY_PROJECT_ENDPOINT"] = "https://other.services.ai.azure.com/api/projects/proj" + case "missing scope": + delete(environment.values, "AZURE_AI_PROJECT_ID") + case "skip": + environment.values["AZD_AGENT_SKIP_ROLE_ASSIGNMENTS"] = "true" + case "managed access": + client.roles.assigned = true + case "404": + client.probeErr = &azcore.ResponseError{StatusCode: http.StatusNotFound} + case "unknown 403": + client.probeErr = &azcore.ResponseError{StatusCode: http.StatusForbidden, ErrorCode: "PolicyDenied"} + } + _, err := access.writeClient(t.Context(), toolboxAccessTestEndpoint, "research", + func(_, tenantID string) (toolboxClient, error) { + if mode == "unrelated endpoint" || mode == "missing scope" { + assert.Empty(t, tenantID, "do not borrow an unrelated environment's tenant") + } + return client, nil + }) + require.NoError(t, err) + assert.NotContains(t, events, "grant") + assert.NotContains(t, events, "roles") + assert.NotContains(t, diagnostics.String(), "Foundry access is ready.") + }) + } +} + +func TestToolboxAccessWarningsUseInjectedWriterWithoutCredentials(t *testing.T) { + var events []string + access, client, _ := accessTestSetup(t, &events, azdext.PrincipalType_PRINCIPAL_TYPE_USER) + var diagnostics bytes.Buffer + access.writer = &diagnostics + client.roles.assignErr = errors.New("https://user:password@acct.services.ai.azure.com?sig=secret#fragment") + _, err := access.writeClient(t.Context(), toolboxAccessTestEndpoint, "research", + func(string, string) (toolboxClient, error) { return client, nil }) + require.NoError(t, err) + assert.Contains(t, diagnostics.String(), "az role assignment create") + assert.Contains(t, diagnostics.String(), toolboxAccessTestID) + for _, secret := range []string{"user:password", "password", "sig=", "secret", "fragment"} { + assert.NotContains(t, diagnostics.String(), secret) + } + assert.NotContains(t, diagnostics.String(), "Waiting for Foundry access") + assert.NotContains(t, diagnostics.String(), "Foundry access is ready.") + _, err = client.GetToolbox(t.Context(), "research") + serviceErr, ok := errors.AsType[*azdext.ServiceError](exterrors.ServiceFromAzure(err, exterrors.OpGetToolbox)) + require.True(t, ok) + assert.Equal(t, http.StatusForbidden, serviceErr.StatusCode) + assert.Contains(t, serviceErr.Suggestion, "Foundry User") +} + +func TestToolboxCreateAccessWarningUsesInjectedWriter(t *testing.T) { + t.Setenv("AZD_SERVER", "") + var diagnostics bytes.Buffer + client, resolved, err := resolveToolboxWriteAndClient(t.Context(), + toolboxFlags{projectEndpoint: toolboxAccessTestEndpoint, output: "json"}, "research", &diagnostics) + require.NoError(t, err) + assert.Equal(t, toolboxAccessTestEndpoint, client.Endpoint()) + assert.Equal(t, toolboxAccessTestEndpoint, resolved.Endpoint) + assert.Contains(t, diagnostics.String(), "Warning:") + assert.Contains(t, diagnostics.String(), "Foundry roles will not be assigned automatically.") +} + +func TestToolboxDeployRetainsFallbackTenantWithoutMatchingScope(t *testing.T) { + for _, mode := range []string{"missing scope", "unrelated endpoint"} { + t.Run(mode, func(t *testing.T) { + stubToolboxEndpointEnv(t) + toolboxDeployEndpoint(t) + var events []string + access, client, account := accessTestSetup(t, &events, azdext.PrincipalType_PRINCIPAL_TYPE_USER) + environment := access.environment.(*accessEnvironment) + environment.subscriptionID = "fallback-subscription" + if mode == "missing scope" { + delete(environment.values, "AZURE_AI_PROJECT_ID") + } else { + environment.values["FOUNDRY_PROJECT_ENDPOINT"] = "https://other.services.ai.azure.com/api/projects/proj" + } + target := &toolboxServiceTarget{ + environmentClient: environment, accountClient: account, access: access, + resolver: newStubConnectionResolver(), + newClient: func(endpoint, tenantID string) (toolboxClient, error) { + assert.Equal(t, toolboxAccessTestEndpoint, endpoint) + assert.Equal(t, "user-access-tenant", tenantID) + return client.mockToolboxClient, nil + }, + } + properties, err := structpb.NewStruct(map[string]any{ + "tools": []any{map[string]any{"type": "web_search"}}, + }) + require.NoError(t, err) + _, err = target.Deploy(t.Context(), &azdext.ServiceConfig{ + Name: "research", AdditionalProperties: properties, Environment: map[string]string{"SETTING": "isolated"}, + }, nil, nil, nil) + require.NoError(t, err) + require.NotNil(t, account.request) + assert.Equal(t, "fallback-subscription", account.request.GetSubscriptionId()) + assert.Empty(t, events) + assert.Empty(t, account.principalSub) + }) + } +} + +func TestToolboxAccessRejectsMismatchedMetadataBeforeClientCreation(t *testing.T) { + for _, key := range []string{"AZURE_AI_PROJECT_ID", "AZURE_SUBSCRIPTION_ID"} { + t.Run(key, func(t *testing.T) { + var events []string + access, _, _ := accessTestSetup(t, &events, azdext.PrincipalType_PRINCIPAL_TYPE_USER) + access.environment.(*accessEnvironment).values[key] = "invalid" + _, err := access.writeClient(t.Context(), toolboxAccessTestEndpoint, "research", + func(string, string) (toolboxClient, error) { + t.Fatal("invalid metadata must fail before credential creation") + return nil, nil + }) + requireLocalError(t, err, exterrors.CodeInvalidParameter) + assert.Empty(t, events) + }) + } +} + +func TestToolboxCreateValidatesLocalInputBeforeAccess(t *testing.T) { + t.Setenv("AZD_SERVER", "") + t.Setenv("FOUNDRY_PROJECT_ENDPOINT", "") + t.Setenv("AZURE_AI_PROJECT_ENDPOINT", "") + for _, input := range []string{ + "nonexistent.yaml", + writeTempFile(t, ".yaml", "tools:\n - name: missing-type\n"), + writeTempFile(t, ".yaml", "connections:\n - name: mcp\n index: x\n instanceName: y\n"), + writeTempFile(t, ".yaml", "skills:\n - name: INVALID\n"), + } { + err := runToolboxCreate(t.Context(), "research", toolboxCreateFlags{fromFile: input}, + toolboxFlags{}, nil, io.Discard) + require.Error(t, err) + assert.NotContains(t, err.Error(), "endpoint") + assert.NotContains(t, err.Error(), "credential") + } +} + +func TestToolboxCreateRejectsExplicitEmptyFlags(t *testing.T) { + for _, args := range [][]string{ + {"create", "research", "--from-file="}, + {"create", "research", "--from-file=tools.yaml", "--project-endpoint="}, + } { + t.Run(strings.Join(args, " "), func(t *testing.T) { + root := NewRootCommand() + root.SetOut(io.Discard) + root.SetErr(io.Discard) + root.SetArgs(args) + err := root.ExecuteContext(t.Context()) + require.Error(t, err) + assert.Contains(t, err.Error(), "--") + }) + } +} + +type toolboxAccessEnvironmentServer struct { + azdext.UnimplementedEnvironmentServiceServer + reads atomic.Int32 +} + +func (s *toolboxAccessEnvironmentServer) GetValues( + context.Context, *azdext.GetEnvironmentRequest, +) (*azdext.KeyValueListResponse, error) { + s.reads.Add(1) + return &azdext.KeyValueListResponse{KeyValues: []*azdext.KeyValue{ + {Key: "FOUNDRY_PROJECT_ENDPOINT", Value: toolboxAccessTestEndpoint}, + {Key: "AZURE_AI_PROJECT_ID", Value: "invalid-project-id"}, + }}, nil +} + +func TestToolboxCreateCommandReachesAccessPreparation(t *testing.T) { + server := grpc.NewServer() + environment := &toolboxAccessEnvironmentServer{} + azdext.RegisterEnvironmentServiceServer(server, environment) + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + go func() { _ = server.Serve(listener) }() + t.Cleanup(func() { + server.Stop() + }) + t.Setenv("AZD_SERVER", listener.Addr().String()) + file := writeTempFile(t, ".yaml", "tools:\n - type: web_search\n") + root := NewRootCommand() + root.SetOut(io.Discard) + root.SetErr(io.Discard) + root.SetArgs([]string{"create", "research", "--from-file=" + file, "--project-endpoint=" + toolboxAccessTestEndpoint}) + err = root.ExecuteContext(t.Context()) + local := requireLocalError(t, err, exterrors.CodeInvalidParameter) + assert.Contains(t, local.Message, "AZURE_AI_PROJECT_ID") + assert.Equal(t, int32(1), environment.reads.Load()) +} + +func TestToolboxReadAndReuseDoNotPrepareAccess(t *testing.T) { + t.Setenv("AZD_SERVER", "") + client, _, err := resolveToolboxAndClient(t.Context(), toolboxFlags{projectEndpoint: toolboxAccessTestEndpoint}) + require.NoError(t, err) + assert.Equal(t, toolboxAccessTestEndpoint, client.Endpoint()) + + stubToolboxEndpointEnv(t) + properties, err := structpb.NewStruct(map[string]any{"endpoint": "https://mcp.test/toolboxes/reused"}) + require.NoError(t, err) + target := &toolboxServiceTarget{ + access: &toolboxAccess{}, + newClient: func(string, string) (toolboxClient, error) { + t.Fatal("reuse must not create a toolbox client") + return nil, nil + }, + } + _, err = target.Deploy(t.Context(), &azdext.ServiceConfig{ + Name: "reused", AdditionalProperties: properties, Environment: map[string]string{"SETTING": "isolated"}, + }, nil, nil, nil) + require.NoError(t, err) +} + +func TestToolboxDeployRejectsInvalidInputBeforeAccess(t *testing.T) { + toolboxDeployEndpoint(t) + properties, err := structpb.NewStruct(map[string]any{ + "connections": []any{map[string]any{"name": "mcp"}}, + "tools": []any{map[string]any{"name": "missing-type"}}, + }) + require.NoError(t, err) + target := &toolboxServiceTarget{access: &toolboxAccess{}} + _, err = target.Deploy(t.Context(), &azdext.ServiceConfig{ + Name: "research", AdditionalProperties: properties, Environment: map[string]string{"SETTING": "isolated"}, + }, nil, nil, nil) + requireLocalError(t, err, exterrors.CodeMissingToolType) +} + +func TestToolboxAccessOptionalFailuresAndCancellation(t *testing.T) { + for _, failure := range []string{ + "environment", "credential", "tenant", "empty tenant", "cancelled environment", + "cancelled credential", "cancelled probe", "cancelled not found", + } { + t.Run(failure, func(t *testing.T) { + var events []string + access, client, account := accessTestSetup(t, &events, azdext.PrincipalType_PRINCIPAL_TYPE_USER) + var diagnostics bytes.Buffer + access.writer = &diagnostics + switch failure { + case "environment": + access.environment.(*accessEnvironment).err = errors.New("environment unavailable") + case "credential", "cancelled credential": + access.newCredential = func(string) (azcore.TokenCredential, error) { + if failure == "cancelled credential" { + return nil, context.Canceled + } + return nil, errors.New("credential unavailable") + } + case "tenant": + account.err = errors.New("tenant unavailable") + case "empty tenant": + account.tenantID = "" + case "cancelled environment": + access.environment.(*accessEnvironment).err = context.Canceled + case "cancelled probe": + client.probeErr = context.Canceled + case "cancelled not found": + client.probeErr = errors.Join(context.Canceled, &azcore.ResponseError{StatusCode: http.StatusNotFound}) + } + _, err := access.writeClient(t.Context(), toolboxAccessTestEndpoint, "research", + func(string, string) (toolboxClient, error) { return client, nil }) + switch { + case strings.HasPrefix(failure, "cancelled"): + require.ErrorIs(t, err, context.Canceled) + case failure == "tenant" || failure == "empty tenant": + requireLocalError(t, err, exterrors.CodeTenantLookupFailed) + default: + require.NoError(t, err) + assert.Contains(t, diagnostics.String(), "Warning:") + } + assert.NotContains(t, events, "grant") + }) + } +} diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create.go index b3d945cab9f..c4ffd18620e 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create.go @@ -6,12 +6,12 @@ package cmd import ( "context" "fmt" + "io" "slices" "strings" "azure.ai.toolboxes/internal/definition" "azure.ai.toolboxes/internal/exterrors" - "azure.ai.toolboxes/internal/foundry/projectctx" "azure.ai.toolboxes/internal/pkg/azure" "github.com/azure/azure-dev/cli/azd/pkg/azdext" @@ -42,7 +42,11 @@ tool entry, so 'create' takes its inputs from a JSON or YAML file via On success the toolbox's runtime MCP endpoint is written to the active azd environment under the TOOLBOX__MCP_ENDPOINT variable (the same key agents consume), where is the toolbox name uppercased with -non-alphanumeric character runs replaced by underscores.`, +non-alphanumeric character runs replaced by underscores. + +After local validation, checks Foundry access before resolving connections. +May assign Foundry User when the target matches the active environment's +project resource ID and endpoint. Access warnings and progress go to stderr.`, Example: ` # Create a toolbox from a JSON definition azd ai toolbox create research --from-file ./tools.json @@ -51,9 +55,13 @@ non-alphanumeric character runs replaced by underscores.`, `, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if err := validateToolboxWriteFlags(cmd); err != nil { + return err + } return runToolboxCreate( cmd.Context(), args[0], *flags, readToolboxFlags(cmd, extCtx), defaultConnectionResolver{}, + cmd.ErrOrStderr(), ) }, } @@ -72,7 +80,7 @@ non-alphanumeric character runs replaced by underscores.`, func runToolboxCreate( ctx context.Context, name string, verb toolboxCreateFlags, - parent toolboxFlags, resolver connectionResolver, + parent toolboxFlags, resolver connectionResolver, writer io.Writer, ) error { if err := validateToolboxName(name); err != nil { return err @@ -81,18 +89,17 @@ func runToolboxCreate( return err } - resolved, err := projectctx.Resolve(ctx, projectctx.ResolveOpts{FlagValue: parent.projectEndpoint}) + input, err := loadCreateDefinition(name, verb) if err != nil { return err } - logResolvedEndpoint("toolbox create", resolved) - - client, err := newToolboxClient(resolved.Endpoint) + client, resolved, err := resolveToolboxWriteAndClient(ctx, parent, name, writer) if err != nil { return err } + logResolvedEndpoint("toolbox create", resolved) - return runToolboxCreateWith(ctx, client, resolver, resolved.Endpoint, name, verb, parent) + return createToolboxFromDefinition(ctx, client, resolver, resolved.Endpoint, name, input, parent) } // runToolboxCreateWith is the testable core. It accepts injected client and @@ -104,6 +111,17 @@ func runToolboxCreateWith( endpoint, name string, verb toolboxCreateFlags, parent toolboxFlags, +) error { + input, err := loadCreateDefinition(name, verb) + if err != nil { + return err + } + return createToolboxFromDefinition(ctx, client, resolver, endpoint, name, input, parent) +} + +func createToolboxFromDefinition( + ctx context.Context, client toolboxClient, resolver connectionResolver, + endpoint, name string, input *definition.Definition, parent toolboxFlags, ) error { if _, err := client.GetToolbox(ctx, name); err == nil { return exterrors.Validation( @@ -116,21 +134,6 @@ func runToolboxCreateWith( return exterrors.ServiceFromAzure(err, exterrors.OpGetToolbox) } - input := &definition.Definition{} - if strings.TrimSpace(verb.fromFile) != "" { - loaded, err := loadLocalToolboxDefinition(verb.fromFile) - if err != nil { - return err - } - input = loaded - } - if input.Name != "" && input.Name != name { - return exterrors.Validation( - exterrors.CodeInvalidToolboxName, - fmt.Sprintf("toolbox name %q in %q does not match command name %q", input.Name, verb.fromFile, name), - "use the same toolbox name in the definition and command, or remove 'name' from the definition", - ) - } req, err := buildToolboxVersionRequest(ctx, resolver, endpoint, input) if err != nil { return err @@ -150,12 +153,37 @@ func runToolboxCreateWith( return emitCreateResult(name, created.Version, parent.output, mcpURL) } +func loadCreateDefinition(name string, verb toolboxCreateFlags) (*definition.Definition, error) { + input := &definition.Definition{} + if strings.TrimSpace(verb.fromFile) != "" { + loaded, err := loadLocalToolboxDefinition(verb.fromFile) + if err != nil { + return nil, err + } + input = loaded + } + if input.Name != "" && input.Name != name { + return nil, exterrors.Validation( + exterrors.CodeInvalidToolboxName, + fmt.Sprintf("toolbox name %q in %q does not match command name %q", input.Name, verb.fromFile, name), + "use the same toolbox name in the definition and command, or remove 'name' from the definition", + ) + } + if err := validateToolboxDefinition(input); err != nil { + return nil, err + } + return input, nil +} + func buildToolboxVersionRequest( ctx context.Context, resolver connectionResolver, endpoint string, input *definition.Definition, ) (*azure.CreateToolboxVersionRequest, error) { + if err := validateToolboxDefinition(input); err != nil { + return nil, err + } entries, err := resolveConnectionSpecs(ctx, resolver, endpoint, input.Connections) if err != nil { return nil, err @@ -209,6 +237,54 @@ func buildToolboxVersionRequest( }, nil } +// validateToolboxDefinition checks authored input without resolving remote connections. +func validateToolboxDefinition(input *definition.Definition) error { + if len(input.Connections)+len(input.Skills)+len(input.Tools) == 0 { + return exterrors.Validation(exterrors.CodeInvalidToolboxName, + "toolbox definition requires at least one tool or skill entry", + "add a connection, skill, or raw tool to the toolbox definition") + } + seen := map[string]bool{} + for _, connection := range input.Connections { + name := strings.TrimSpace(connection.Name) + if name == "" { + return exterrors.Validation(exterrors.CodeInvalidParameter, + "connection name must not be empty", "set connections[].name in the input file") + } + if seen[name] { + return exterrors.Validation(exterrors.CodeDuplicateConnection, + "connection is duplicated in the input", "remove duplicate connection entries") + } + if connection.Index != "" && connection.InstanceName != "" { + return exterrors.Validation(exterrors.CodeConflictingArguments, + "a connection cannot set both index and instanceName", + "use index for CognitiveSearch or instanceName for GroundingWithCustomSearch, not both") + } + seen[name] = true + } + skills := make([]map[string]any, 0, len(input.Skills)) + for _, skill := range input.Skills { + if err := validateSkillName(skill.Name); err != nil { + return err + } + skills = append(skills, buildSkillEntry(skillSpec{Name: strings.TrimSpace(skill.Name), Version: skill.Version})) + } + if err := validateNoDuplicateSkills(skills); err != nil { + return err + } + if _, err := validateRawToolEntries(input.Tools); err != nil { + return err + } + if err := validateNoDuplicateToolNames(input.Tools); err != nil { + return err + } + if err := validateNoDuplicateConnectionIDs(input.Tools); err != nil { + return err + } + _, err := buildToolboxPolicies(input.Policies) + return err +} + func emitCreateResult(name, version, output, mcpURL string) error { if output == "json" { return emitJSON(map[string]any{ diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/exterrors/errors.go b/cli/azd/extensions/azure.ai.toolboxes/internal/exterrors/errors.go index 4e3c02b6731..6dbb17da5fe 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/exterrors/errors.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/exterrors/errors.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "net/http" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/azure/azure-dev/cli/azd/pkg/azdext" @@ -90,8 +91,7 @@ func Cancelled(message string) error { // with operation context. If the error is not an azcore.ResponseError, it // returns a generic internal [azdext.LocalError]. func ServiceFromAzure(err error, operation string) error { - var respErr *azcore.ResponseError - if errors.As(err, &respErr) { + if respErr, ok := errors.AsType[*azcore.ResponseError](err); ok { serviceName := "" if respErr.RawResponse != nil && respErr.RawResponse.Request != nil { serviceName = respErr.RawResponse.Request.Host @@ -100,12 +100,17 @@ func ServiceFromAzure(err error, operation string) error { if code == "" { code = fmt.Sprintf("%d", respErr.StatusCode) } - return &azdext.ServiceError{ + serviceErr := &azdext.ServiceError{ Message: fmt.Sprintf("%s: %s", operation, respErr.Error()), ErrorCode: fmt.Sprintf("%s.%s", operation, code), StatusCode: respErr.StatusCode, ServiceName: serviceName, } + if respErr.StatusCode == http.StatusForbidden { + serviceErr.Suggestion = "Verify that your identity has Foundry User or equivalent access on the target " + + "Foundry project. Subscription Owner or Contributor alone does not grant data-plane access." + } + return serviceErr } if IsCancellation(err) { return Cancelled(fmt.Sprintf("%s was cancelled", operation)) From 244641aafe1f28352aa55ef45e9c55d22a0e159c Mon Sep 17 00:00:00 2001 From: hund030 Date: Wed, 28 Oct 2026 15:28:18 +0800 Subject: [PATCH 2/4] fix(toolbox): reuse access probe result and guard tool expansion - Cache the verified GetToolbox probe response for one-time reuse by the immediate write operation. - Guard expandToolboxValue map type assertions when a tool entry is nil. --- .../internal/cmd/service_target.go | 8 +-- .../internal/cmd/toolbox_access.go | 51 ++++++++++++++----- .../internal/cmd/toolbox_access_test.go | 41 +++++++++++---- 3 files changed, 73 insertions(+), 27 deletions(-) diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go index d58cc9bc562..539f69f8cc4 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go @@ -175,7 +175,8 @@ func (p *toolboxServiceTarget) Deploy( } tools := make([]map[string]any, 0, len(cfg.Tools)) for _, tool := range cfg.Tools { - tools = append(tools, expandToolboxValue(tool, environment).(map[string]any)) + expanded, _ := expandToolboxValue(tool, environment).(map[string]any) + tools = append(tools, expanded) } input := &definition.Definition{ Name: name, @@ -223,7 +224,8 @@ func (p *toolboxServiceTarget) Deploy( } if p.access != nil { - if err := p.access.prepare(ctx, scope, tenantID, client, name); err != nil { + client, err = p.access.prepare(ctx, scope, tenantID, client, name) + if err != nil { return nil, err } } @@ -354,7 +356,7 @@ func (p *toolboxServiceTarget) buildToolEntries( for _, tool := range tools { entry := maps.Clone(tool) if env != nil { - entry = expandToolboxValue(tool, env).(map[string]any) + entry, _ = expandToolboxValue(tool, env).(map[string]any) } if connName, isString := entry["connection"].(string); isString && connName != "" { conn, err := p.resolver.resolveConnection(ctx, endpoint, connName) diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go index 3764dfcaffb..0d66958c727 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go @@ -11,6 +11,7 @@ import ( "os" "azure.ai.toolboxes/internal/exterrors" + "azure.ai.toolboxes/internal/pkg/azure" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -76,29 +77,37 @@ func (a *toolboxAccess) resolve(ctx context.Context, endpoint string) (*toolboxA func (a *toolboxAccess) prepare( ctx context.Context, scope *toolboxAccessScope, tenantID string, client toolboxClient, name string, -) error { +) (toolboxClient, error) { if scope == nil || scope.skip { - return nil + return client, nil } credential, err := a.newCredential(tenantID) if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { - return cancelErr + return nil, cancelErr } if err != nil { a.warn("Could not create the Foundry role-check credential; continuing without assigning a role.") - return nil + return client, nil } + var cached *probedToolboxClient prepare := foundry.AccessPreparation{ Account: a.account, Credential: credential, Roles: a.roles, Scope: scope.resource.String(), Probe: func(ctx context.Context) error { - _, err := client.GetToolbox(ctx, name) + cached = nil + tb, err := client.GetToolbox(ctx, name) if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { return cancelErr } - if isAzureNotFound(err) { + if err == nil || isAzureNotFound(err) { + cached = &probedToolboxClient{ + toolboxClient: client, + probedName: name, + probedObj: tb, + probedErr: err, + } return nil } return err @@ -110,16 +119,35 @@ func (a *toolboxAccess) prepare( } if err := prepare.Ensure(ctx); err != nil { if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { - return cancelErr + return nil, cancelErr } serviceErr := exterrors.ServiceFromAzure(err, exterrors.OpGetToolbox) if structured, ok := errors.AsType[*azdext.ServiceError](serviceErr); ok { structured.Suggestion = "Foundry User was assigned on the project but access has not propagated. " + "Wait and retry; if access is still denied, ask an administrator to verify project access." } - return serviceErr + return nil, serviceErr + } + if cached != nil { + return cached, nil + } + return client, nil +} + +type probedToolboxClient struct { + toolboxClient + probedName string + probedObj *azure.ToolboxObject + probedErr error + consumed bool +} + +func (c *probedToolboxClient) GetToolbox(ctx context.Context, name string) (*azure.ToolboxObject, error) { + if !c.consumed && name == c.probedName { + c.consumed = true + return c.probedObj, c.probedErr } - return nil + return c.toolboxClient.GetToolbox(ctx, name) } func (a *toolboxAccess) writeClient( @@ -145,8 +173,5 @@ func (a *toolboxAccess) writeClient( return nil, exterrors.Auth(exterrors.CodeCredentialCreationFailed, "could not create the Foundry credential", "run 'azd auth login' and retry") } - if err := a.prepare(ctx, scope, tenantID, client, name); err != nil { - return nil, err - } - return client, nil + return a.prepare(ctx, scope, tenantID, client, name) } diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go index 526eb28231d..2b6872feff2 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go @@ -197,7 +197,7 @@ func TestToolboxCreatePreparesAccessBeforeConnections(t *testing.T) { &definition.Definition{Connections: []definition.ConnectionReference{{Name: "mcp"}}}, toolboxFlags{output: "json"}) require.NoError(t, err) - assert.Equal(t, []string{"read", "roles", "grant", "read", "read", "connection", "create"}, events) + assert.Equal(t, []string{"read", "roles", "grant", "read", "connection", "create"}, events) assert.Equal(t, "Assigning Foundry User to the current principal.\n"+ "Waiting for Foundry access to propagate.\nFoundry access is ready.\n", diagnostics.String()) }) @@ -466,16 +466,35 @@ func TestToolboxReadAndReuseDoNotPrepareAccess(t *testing.T) { func TestToolboxDeployRejectsInvalidInputBeforeAccess(t *testing.T) { toolboxDeployEndpoint(t) - properties, err := structpb.NewStruct(map[string]any{ - "connections": []any{map[string]any{"name": "mcp"}}, - "tools": []any{map[string]any{"name": "missing-type"}}, - }) - require.NoError(t, err) - target := &toolboxServiceTarget{access: &toolboxAccess{}} - _, err = target.Deploy(t.Context(), &azdext.ServiceConfig{ - Name: "research", AdditionalProperties: properties, Environment: map[string]string{"SETTING": "isolated"}, - }, nil, nil, nil) - requireLocalError(t, err, exterrors.CodeMissingToolType) + for _, tc := range []struct { + name string + tools []any + wantCode string + }{ + { + name: "missing tool type", + tools: []any{map[string]any{"name": "missing-type"}}, + wantCode: exterrors.CodeMissingToolType, + }, + { + name: "null tool entry", + tools: []any{nil}, + wantCode: exterrors.CodeInvalidParameter, + }, + } { + t.Run(tc.name, func(t *testing.T) { + properties, err := structpb.NewStruct(map[string]any{ + "connections": []any{map[string]any{"name": "mcp"}}, + "tools": tc.tools, + }) + require.NoError(t, err) + target := &toolboxServiceTarget{access: &toolboxAccess{}} + _, err = target.Deploy(t.Context(), &azdext.ServiceConfig{ + Name: "research", AdditionalProperties: properties, Environment: map[string]string{"SETTING": "isolated"}, + }, nil, nil, nil) + requireLocalError(t, err, tc.wantCode) + }) + } } func TestToolboxAccessOptionalFailuresAndCancellation(t *testing.T) { From 1047bf21bb68e3fd5ee92959581ef812c657d5d7 Mon Sep 17 00:00:00 2001 From: hund030 Date: Wed, 28 Oct 2026 18:22:13 +0800 Subject: [PATCH 3/4] fix(toolbox): distinguish project 404 from toolbox 404 in access probe --- .../internal/cmd/toolbox_access.go | 5 ++++- .../internal/cmd/toolbox_access_test.go | 14 +++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go index 0d66958c727..23d41947f2d 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "os" + "time" "azure.ai.toolboxes/internal/exterrors" "azure.ai.toolboxes/internal/pkg/azure" @@ -29,6 +30,7 @@ type toolboxAccess struct { account toolboxAccessAccount newCredential func(string) (azcore.TokenCredential, error) roles foundry.ProjectRoles + retryDelay time.Duration writer io.Writer } @@ -94,6 +96,7 @@ func (a *toolboxAccess) prepare( Account: a.account, Credential: credential, Roles: a.roles, + RetryDelay: a.retryDelay, Scope: scope.resource.String(), Probe: func(ctx context.Context) error { cached = nil @@ -101,7 +104,7 @@ func (a *toolboxAccess) prepare( if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { return cancelErr } - if err == nil || isAzureNotFound(err) { + if err == nil || (isAzureNotFound(err) && !foundry.IsProjectNotFound(err)) { cached = &probedToolboxClient{ toolboxClient: client, probedName: name, diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go index 2b6872feff2..855972d463c 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go @@ -13,6 +13,7 @@ import ( "strings" "sync/atomic" "testing" + "time" "azure.ai.toolboxes/internal/definition" "azure.ai.toolboxes/internal/exterrors" @@ -79,7 +80,7 @@ type accessRoles struct { func (r *accessRoles) HasAccess(context.Context, string, string) (bool, error) { *r.events = append(*r.events, "roles") - return false, nil + return r.assigned, nil } func (r *accessRoles) Assign(_ context.Context, scope, id string, kind armauthorization.PrincipalType) error { @@ -161,7 +162,7 @@ func accessTestSetup( assert.Equal(t, "user-access-tenant", tenantID) return nil, nil }, - roles: roles, writer: io.Discard, + roles: roles, retryDelay: time.Nanosecond, writer: io.Discard, } return access, client, account } @@ -197,7 +198,7 @@ func TestToolboxCreatePreparesAccessBeforeConnections(t *testing.T) { &definition.Definition{Connections: []definition.ConnectionReference{{Name: "mcp"}}}, toolboxFlags{output: "json"}) require.NoError(t, err) - assert.Equal(t, []string{"read", "roles", "grant", "read", "connection", "create"}, events) + assert.Equal(t, []string{"roles", "grant", "read", "connection", "create"}, events) assert.Equal(t, "Assigning Foundry User to the current principal.\n"+ "Waiting for Foundry access to propagate.\nFoundry access is ready.\n", diagnostics.String()) }) @@ -237,7 +238,7 @@ func TestToolboxDeployPreparesAccessWithoutAgentHook(t *testing.T) { Name: "research", AdditionalProperties: properties, Environment: map[string]string{"CONNECTION": "mcp"}, }, nil, nil, nil) require.NoError(t, err) - assert.Equal(t, []string{"read", "roles", "grant", "read", "connection", "create"}, events) + assert.Equal(t, []string{"roles", "grant", "read", "connection", "create"}, events) require.Len(t, client.createVersionCalls, 1) assert.NotContains(t, client.createVersionCalls[0].req.Tools[0], "connection") } @@ -260,8 +261,10 @@ func TestToolboxAccessDoesNotGrantForUnscopedSkippedOrAuthorizedOperations(t *te case "managed access": client.roles.assigned = true case "404": + client.roles.assigned = true client.probeErr = &azcore.ResponseError{StatusCode: http.StatusNotFound} case "unknown 403": + client.roles.assigned = true client.probeErr = &azcore.ResponseError{StatusCode: http.StatusForbidden, ErrorCode: "PolicyDenied"} } _, err := access.writeClient(t.Context(), toolboxAccessTestEndpoint, "research", @@ -273,7 +276,6 @@ func TestToolboxAccessDoesNotGrantForUnscopedSkippedOrAuthorizedOperations(t *te }) require.NoError(t, err) assert.NotContains(t, events, "grant") - assert.NotContains(t, events, "roles") assert.NotContains(t, diagnostics.String(), "Foundry access is ready.") }) } @@ -524,8 +526,10 @@ func TestToolboxAccessOptionalFailuresAndCancellation(t *testing.T) { case "cancelled environment": access.environment.(*accessEnvironment).err = context.Canceled case "cancelled probe": + client.roles.assigned = true client.probeErr = context.Canceled case "cancelled not found": + client.roles.assigned = true client.probeErr = errors.Join(context.Canceled, &azcore.ResponseError{StatusCode: http.StatusNotFound}) } _, err := access.writeClient(t.Context(), toolboxAccessTestEndpoint, "research", From 45c279f674a17bb0e2aa982d70ed0164398cefb7 Mon Sep 17 00:00:00 2001 From: huimiu Date: Wed, 28 Oct 2026 17:07:17 +0800 Subject: [PATCH 4/4] fix: reject mixed toolbox tool names before access checks --- .../extensions/azure.ai.toolboxes/README.md | 5 + .../internal/cmd/service_target.go | 12 +- .../internal/cmd/toolbox_access_test.go | 104 ++++++++++++++++++ .../internal/cmd/toolbox_create.go | 6 +- .../internal/cmd/toolbox_create_test.go | 74 +++++++++++++ 5 files changed, 194 insertions(+), 7 deletions(-) diff --git a/cli/azd/extensions/azure.ai.toolboxes/README.md b/cli/azd/extensions/azure.ai.toolboxes/README.md index df4f9f13d2f..0e2004e39ad 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/README.md +++ b/cli/azd/extensions/azure.ai.toolboxes/README.md @@ -44,6 +44,11 @@ Toolbox-owned authoring properties use camelCase, including pass-through payloads and retain the field names and discriminator values defined by the corresponding service contract. +Connection-backed tools use the connection's name as the tool name. Names must +be unique across `connections[].name` and explicit `tools[].name` values. +Create and managed deployment reject duplicates before endpoint resolution or +access preparation. + Configure a Foundry project endpoint before deploying. If the project or referenced Connections are also managed as services, declare those dependencies with `uses`. Run `azd provision` for the project, then `azd deploy --all` for diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go index 539f69f8cc4..7dc48ede76e 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go @@ -160,12 +160,6 @@ func (p *toolboxServiceTarget) Deploy( return p.deployReuse(ctx, name, cfg, serviceConfig, progress) } - resolved, err := projectctx.Resolve(ctx, projectctx.ResolveOpts{}) - if err != nil { - return nil, err - } - endpoint := resolved.Endpoint - environment, err := p.environmentValues(ctx, serviceConfig) if err != nil { return nil, err @@ -191,6 +185,12 @@ func (p *toolboxServiceTarget) Deploy( return nil, err } + resolved, err := projectctx.Resolve(ctx, projectctx.ResolveOpts{}) + if err != nil { + return nil, err + } + endpoint := resolved.Endpoint + if progress != nil { progress(fmt.Sprintf("Upserting toolbox %q", name)) } diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go index 855972d463c..08c82a093ae 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go @@ -39,11 +39,13 @@ type accessEnvironment struct { stubEnvironmentReader values map[string]string err error + reads int } func (e *accessEnvironment) GetValues( context.Context, *azdext.GetEnvironmentRequest, ...grpc.CallOption, ) (*azdext.KeyValueListResponse, error) { + e.reads++ if e.err != nil { return nil, e.err } @@ -243,6 +245,63 @@ func TestToolboxDeployPreparesAccessWithoutAgentHook(t *testing.T) { assert.NotContains(t, client.createVersionCalls[0].req.Tools[0], "connection") } +func TestToolboxDeployValidatesMixedToolNamesBeforeAccess(t *testing.T) { + for _, rawName := range []string{"mcp", "web"} { + t.Run(rawName, func(t *testing.T) { + envWrites := stubToolboxEndpointEnv(t) + var events []string + previous := projectctx.ReadAzdHostedSourcesFunc + projectctx.ReadAzdHostedSourcesFunc = func(context.Context) (projectctx.AzdHostedSources, error) { + events = append(events, "endpoint") + return projectctx.AzdHostedSources{EnvValue: toolboxAccessTestEndpoint, EnvName: "test"}, nil + } + t.Cleanup(func() { projectctx.ReadAzdHostedSourcesFunc = previous }) + access, client, account := accessTestSetup(t, &events, azdext.PrincipalType_PRINCIPAL_TYPE_USER) + environment, ok := access.environment.(*accessEnvironment) + require.True(t, ok) + target := &toolboxServiceTarget{ + environmentClient: environment, accountClient: account, access: access, + resolver: accessConnectionResolver{events: &events, roles: client.roles}, + newClient: func(endpoint, tenantID string) (toolboxClient, error) { + assert.Equal(t, toolboxAccessTestEndpoint, endpoint) + assert.Equal(t, "user-access-tenant", tenantID) + return client, nil + }, + } + properties, err := structpb.NewStruct(map[string]any{ + "connections": []any{map[string]any{"name": "mcp"}}, + "tools": []any{map[string]any{"type": "web_search", "name": "${TOOL_NAME}"}}, + }) + require.NoError(t, err) + result, err := target.Deploy(t.Context(), &azdext.ServiceConfig{ + Name: "research", AdditionalProperties: properties, Environment: map[string]string{"TOOL_NAME": rawName}, + }, nil, nil, nil) + if rawName == "mcp" { + local := requireLocalError(t, err, exterrors.CodeDuplicateToolName) + assert.Contains(t, local.Message, `"mcp"`) + assert.Nil(t, result) + assert.Empty(t, events) + assert.Zero(t, environment.reads) + assert.Nil(t, account.request) + assert.Empty(t, account.principalSub) + assert.False(t, client.roles.assigned) + assert.Empty(t, client.createVersionCalls) + assert.Empty(t, *envWrites) + return + } + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, []string{"endpoint", "roles", "grant", "read", "connection", "create"}, events) + require.Len(t, client.createVersionCalls, 1) + tools := client.createVersionCalls[0].req.Tools + require.Len(t, tools, 2) + assert.Equal(t, "mcp", tools[0]["name"]) + assert.Equal(t, "web", tools[1]["name"]) + assert.Len(t, *envWrites, 1) + }) + } +} + func TestToolboxAccessDoesNotGrantForUnscopedSkippedOrAuthorizedOperations(t *testing.T) { for _, mode := range []string{"unrelated endpoint", "missing scope", "skip", "managed access", "404", "unknown 403"} { t.Run(mode, func(t *testing.T) { @@ -444,6 +503,51 @@ func TestToolboxCreateCommandReachesAccessPreparation(t *testing.T) { assert.Equal(t, int32(1), environment.reads.Load()) } +func TestToolboxCreateRejectsMixedDuplicateNamesBeforeAccess(t *testing.T) { + server := grpc.NewServer() + environment := &toolboxAccessEnvironmentServer{} + azdext.RegisterEnvironmentServiceServer(server, environment) + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + go func() { _ = server.Serve(listener) }() + t.Cleanup(server.Stop) + t.Setenv("AZD_SERVER", listener.Addr().String()) + + endpointReads := 0 + previous := projectctx.ReadAzdHostedSourcesFunc + projectctx.ReadAzdHostedSourcesFunc = func(context.Context) (projectctx.AzdHostedSources, error) { + endpointReads++ + return projectctx.AzdHostedSources{EnvValue: toolboxAccessTestEndpoint, EnvName: "test"}, nil + } + t.Cleanup(func() { projectctx.ReadAzdHostedSourcesFunc = previous }) + envWrites := stubToolboxEndpointEnv(t) + + for _, tc := range []struct { + extension string + content string + }{ + {".yaml", "connections:\n - name: mcp\ntools:\n - type: web_search\n name: mcp\n"}, + {".json", `{"connections":[{"name":"mcp"}],"tools":[{"type":"web_search","name":"mcp"}]}`}, + } { + t.Run(tc.extension, func(t *testing.T) { + file := writeTempFile(t, tc.extension, tc.content) + var diagnostics bytes.Buffer + root := NewRootCommand() + root.SetOut(io.Discard) + root.SetErr(&diagnostics) + root.SetArgs([]string{"create", "research", "--from-file=" + file}) + + err := root.ExecuteContext(t.Context()) + local := requireLocalError(t, err, exterrors.CodeDuplicateToolName) + assert.Contains(t, local.Message, `"mcp"`) + assert.Zero(t, endpointReads) + assert.Zero(t, environment.reads.Load()) + assert.Empty(t, diagnostics.String()) + assert.Empty(t, *envWrites) + }) + } +} + func TestToolboxReadAndReuseDoNotPrepareAccess(t *testing.T) { t.Setenv("AZD_SERVER", "") client, _, err := resolveToolboxAndClient(t.Context(), toolboxFlags{projectEndpoint: toolboxAccessTestEndpoint}) diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create.go index c4ffd18620e..fa8a943fff4 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create.go @@ -275,7 +275,11 @@ func validateToolboxDefinition(input *definition.Definition) error { if _, err := validateRawToolEntries(input.Tools); err != nil { return err } - if err := validateNoDuplicateToolNames(input.Tools); err != nil { + tools := slices.Clone(input.Tools) + for name := range seen { + tools = append(tools, map[string]any{"name": name}) + } + if err := validateNoDuplicateToolNames(tools); err != nil { return err } if err := validateNoDuplicateConnectionIDs(input.Tools); err != nil { diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create_test.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create_test.go index feec1cbb374..e0a81ad624d 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create_test.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_create_test.go @@ -38,3 +38,77 @@ func TestCreateRejectsDefinitionNameMismatch(t *testing.T) { ) requireLocalError(t, err, exterrors.CodeInvalidToolboxName) } + +func TestValidateToolboxDefinitionToolNames(t *testing.T) { + for _, tc := range []struct { + name string + connections []definition.ConnectionReference + tools []map[string]any + wantCode string + }{ + { + name: "mixed duplicate names", + connections: []definition.ConnectionReference{{Name: "mcp"}}, + tools: []map[string]any{{"type": "web_search", "name": "mcp"}}, + wantCode: exterrors.CodeDuplicateToolName, + }, + { + name: "trimmed connection name", + connections: []definition.ConnectionReference{{Name: " mcp "}}, + tools: []map[string]any{{"type": "web_search", "name": "mcp"}}, + wantCode: exterrors.CodeDuplicateToolName, + }, + { + name: "distinct names", + connections: []definition.ConnectionReference{{Name: "mcp"}}, + tools: []map[string]any{{"type": "web_search", "name": "web"}}, + }, + { + name: "unnamed raw tool", + connections: []definition.ConnectionReference{{Name: "mcp"}}, + tools: []map[string]any{{"type": "web_search"}}, + }, + { + name: "case sensitive names", + connections: []definition.ConnectionReference{{Name: "mcp"}}, + tools: []map[string]any{{"type": "web_search", "name": "MCP"}}, + }, + { + name: "duplicate raw names", + tools: []map[string]any{ + {"type": "web_search", "name": "web"}, + {"type": "file_search", "name": "web"}, + }, + wantCode: exterrors.CodeDuplicateToolName, + }, + { + name: "duplicate connections", + connections: []definition.ConnectionReference{{Name: "mcp"}, {Name: "mcp"}}, + wantCode: exterrors.CodeDuplicateConnection, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := validateToolboxDefinition(&definition.Definition{Connections: tc.connections, Tools: tc.tools}) + if tc.wantCode == "" { + require.NoError(t, err) + return + } + requireLocalError(t, err, tc.wantCode) + }) + } +} + +func TestBuildToolboxVersionRequestRejectsResolvedDuplicateToolNames(t *testing.T) { + resolver := newStubConnectionResolver() + resolver.byName["alias"] = &projectConnection{ + ID: "/connections/mcp", Category: "RemoteTool", Name: "mcp", Target: "https://mcp.test", + } + + request, err := buildToolboxVersionRequest(t.Context(), resolver, "https://example.test", + &definition.Definition{ + Connections: []definition.ConnectionReference{{Name: "alias"}}, + Tools: []map[string]any{{"type": "web_search", "name": "mcp"}}, + }) + requireLocalError(t, err, exterrors.CodeDuplicateToolName) + assert.Nil(t, request) +}