From 93b479823a741f675e9e8139232a18cae07307a3 Mon Sep 17 00:00:00 2001 From: trangevi Date: Tue, 27 Oct 2026 11:55:20 -0700 Subject: [PATCH 1/2] fix(foundry): share access preparation across agent and toolbox writes Complete layer 3 of the Foundry access stack: migrate agent predeploy and diagnostics to shared helpers, cover remaining toolbox write paths, and preserve local validation and injected diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- cli/azd/extensions/azure.ai.agents/README.md | 29 ++ .../init_foundry_resources_helpers_test.go | 23 +- .../internal/cmd/listen_test.go | 8 +- .../internal/exterrors/codes.go | 1 + .../internal/project/agent_identity_rbac.go | 104 +------ .../project/agent_identity_rbac_test.go | 96 ------- .../internal/project/developer_rbac_check.go | 261 ++++++------------ .../project/developer_rbac_check_test.go | 131 ++++++++- .../internal/project/developer_rbac_query.go | 3 +- .../tier2/2.00-setup-deploy-shared-agent.yaml | 1 + .../extensions/azure.ai.toolboxes/AGENTS.md | 10 +- .../extensions/azure.ai.toolboxes/README.md | 9 +- cli/azd/extensions/azure.ai.toolboxes/go.mod | 2 +- .../internal/cmd/context.go | 2 +- .../azure.ai.toolboxes/internal/cmd/root.go | 3 +- .../cmd/testdata/TestAIHelp-root.snap | 3 +- .../internal/cmd/toolbox_access_test.go | 109 +++++++- .../internal/cmd/toolbox_commands_test.go | 4 + .../internal/cmd/toolbox_connection_add.go | 127 +++++---- .../internal/cmd/toolbox_connection_remove.go | 8 +- .../internal/cmd/toolbox_publish.go | 9 +- .../internal/cmd/toolbox_skill_add.go | 29 +- .../internal/cmd/toolbox_skill_remove.go | 8 +- .../internal/cmd/toolbox_skill_verbs_test.go | 2 + .../internal/foundry/credential.go | 32 --- docs/architecture/extension-framework.md | 22 ++ docs/guides/creating-an-extension.md | 15 + 27 files changed, 536 insertions(+), 515 deletions(-) delete mode 100644 cli/azd/extensions/azure.ai.toolboxes/internal/foundry/credential.go diff --git a/cli/azd/extensions/azure.ai.agents/README.md b/cli/azd/extensions/azure.ai.agents/README.md index 685d1c9a807..5de2d0a6e87 100644 --- a/cli/azd/extensions/azure.ai.agents/README.md +++ b/cli/azd/extensions/azure.ai.agents/README.md @@ -263,6 +263,35 @@ The optional [live preview scenario](tests/cli-interactive-tester-scenarios/tier is driven through `foundry-extension-scenario-orchestrator`; its setup incurs Azure cost and is not run automatically. +## Developer Foundry access + +Agent predeploy uses the shared core `pkg/foundry` access preparation, also used +by toolbox writes. A non-mutating agent-list probe checks effective data-plane +access first, so working custom or externally managed permissions do not +require role-assignment writes. On a confirmed authorization denial, it checks +inherited Foundry roles and attempts a project-scoped Foundry User assignment +only when needed, then waits for bounded propagation before deployment. +Optional role-inspection or assignment failures warn and let deployment report +the service result. Cancellation or an exhausted post-grant readiness check +stops predeploy; wait and retry if propagation takes longer. + +Preparation requires a matching `AZURE_AI_PROJECT_ID` and +`FOUNDRY_PROJECT_ENDPOINT` (falling back to `AZURE_AI_PROJECT_ENDPOINT`), with no +conflicting `AZURE_SUBSCRIPTION_ID`. Guest users and service principals use the +host's user-access tenant for credentials and resource-tenant principal for +assignments. Missing or inconsistent project context warns without granting. + +`AZD_AGENT_SKIP_ROLE_ASSIGNMENTS=true` disables developer access preparation +and hosted-agent permission checks. A non-empty active azd environment value +takes precedence over the process environment; absent or empty values fall +back to the process. Hosted agents retain their separate role-assignment-write +and ACR classic/ABAC checks. Provisioning and read-only diagnostics do not +automatically grant developer roles. + +This shared API must be released in core before updating the independently +published extension's azd SDK dependency. Local cross-module builds can use a +Go workspace; do not commit local module replacements. + ## Extension telemetry API Requires azd 1.37.0 or later. Telemetry, deployment preview, and developer RBAC diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_foundry_resources_helpers_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_foundry_resources_helpers_test.go index bb8800aaa4b..052fc4e7c0e 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_foundry_resources_helpers_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_foundry_resources_helpers_test.go @@ -409,12 +409,13 @@ func TestCreateNewEnvironment_ReusesExistingEnvironmentAfterAlreadyExistsError(t type testEnvironmentServiceServer struct { azdext.UnimplementedEnvironmentServiceServer - environments map[string]*azdext.Environment - current *azdext.Environment - values map[string]map[string]string // envName -> key -> value - setKeys []string - getCurrentCalls int - getValuesCalls int + environments map[string]*azdext.Environment + current *azdext.Environment + values map[string]map[string]string // envName -> key -> value + setKeys []string + getCurrentCalls int + getValuesCalls int + getActiveValuesCalls int } func (s *testEnvironmentServiceServer) GetCurrent(context.Context, *azdext.EmptyRequest) (*azdext.EnvironmentResponse, error) { @@ -468,7 +469,15 @@ func (s *testEnvironmentServiceServer) GetValues( _ context.Context, req *azdext.GetEnvironmentRequest, ) (*azdext.KeyValueListResponse, error) { s.getValuesCalls++ - values := s.values[req.Name] + name := req.GetName() + if name == "" { + s.getActiveValuesCalls++ + if s.current == nil { + return nil, status.Error(codes.NotFound, "current environment not found") + } + name = s.current.GetName() + } + values := s.values[name] keyValues := make([]*azdext.KeyValue, 0, len(values)) for key, value := range values { keyValues = append(keyValues, &azdext.KeyValue{Key: key, Value: value}) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go index 421c39ed42c..c2f1203972e 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go @@ -1288,10 +1288,10 @@ func TestPredeployAgentRBACRunsPerService(t *testing.T) { client := newTestAzdClient(t, envServer, &testWorkflowServiceServer{}) require.NoError(t, predeployHandler(t.Context(), client, &azdext.ServiceEventArgs{Project: proj, Service: prompt})) - firstCalls := envServer.getCurrentCalls - require.GreaterOrEqual(t, firstCalls, 2) + firstCalls := envServer.getActiveValuesCalls + require.Equal(t, 1, firstCalls) require.NoError(t, predeployHandler(t.Context(), client, &azdext.ServiceEventArgs{Project: proj, Service: hosted})) - require.GreaterOrEqual(t, envServer.getCurrentCalls, firstCalls+2) + require.Equal(t, firstCalls+1, envServer.getActiveValuesCalls) } func TestPredeployVoiceRunsDeveloperRBAC(t *testing.T) { @@ -1318,7 +1318,7 @@ func TestPredeployVoiceRunsDeveloperRBAC(t *testing.T) { require.NoError(t, predeployHandler(t.Context(), client, &azdext.ServiceEventArgs{ Project: proj, Service: svc, })) - require.GreaterOrEqual(t, envServer.getCurrentCalls, 2) + require.Equal(t, 1, envServer.getActiveValuesCalls) }) } } diff --git a/cli/azd/extensions/azure.ai.agents/internal/exterrors/codes.go b/cli/azd/extensions/azure.ai.agents/internal/exterrors/codes.go index abe5478ab9d..ae185900f9b 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/exterrors/codes.go +++ b/cli/azd/extensions/azure.ai.agents/internal/exterrors/codes.go @@ -218,6 +218,7 @@ const ( OpCreateAgent = "create_agent" OpCreateConversation = "create_conversation" OpCreateTelephonyBinding = "create_telephony_binding" + OpCheckProjectAccess = "check_project_access" OpGetAgent = "get_agent" OpGetTelephonyBinding = "get_telephony_binding" OpUpdateAgent = "update_agent" diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac.go b/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac.go index 9a16c64bdaa..9e33d221aff 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac.go @@ -4,16 +4,13 @@ package project import ( - "context" "fmt" "strings" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" - "github.com/google/uuid" + "github.com/azure/azure-dev/cli/azd/pkg/foundry" ) -const roleAzureAIUser = "53ca6127-db72-4b80-b1b0-d745d6d5456d" +const roleAzureAIUser = foundry.FoundryUserRole // agentIdentityInfo holds the parsed project information needed for agent identity RBAC. type agentIdentityInfo struct { @@ -69,100 +66,3 @@ func parseAgentIdentityInfo(projectResourceID string) (*agentIdentityInfo, error return info, nil } - -// assignRoleToIdentity assigns a single RBAC role to a principal at the given scope. -// It is idempotent: existing assignments are detected and skipped. -// Returns true if a new assignment was created, false if it already existed. -func assignRoleToIdentity( - ctx context.Context, - cred *azidentity.AzureDeveloperCLICredential, - principalID string, - roleID string, - roleName string, - scope string, - principalType armauthorization.PrincipalType, -) (bool, error) { - subscriptionID := extractSubscriptionID(scope) - if subscriptionID == "" { - return false, fmt.Errorf("could not extract subscription ID from scope %s for role %s", scope, roleName) - } - - client, err := armauthorization.NewRoleAssignmentsClient(subscriptionID, cred, nil) - if err != nil { - return false, fmt.Errorf("failed to create role assignments client for role %s: %w", roleName, err) - } - return assignRoleToIdentityWithClient(ctx, client, principalID, roleID, roleName, scope, principalType) -} - -func assignRoleToIdentityWithClient( - ctx context.Context, - client *armauthorization.RoleAssignmentsClient, - principalID, roleID, roleName, scope string, - principalType armauthorization.PrincipalType, -) (bool, error) { - roleDefinitionSuffix := fmt.Sprintf("/roleDefinitions/%s", roleID) - - // Check for existing assignment - pager := client.NewListForScopePager(scope, &armauthorization.RoleAssignmentsClientListForScopeOptions{ - Filter: new("atScope()"), - }) - - alreadyExists := false - for pager.More() { - page, err := pager.NextPage(ctx) - if err != nil { - return false, fmt.Errorf("failed to list role assignments for role %s: %w", roleName, err) - } - for _, assignment := range page.Value { - if assignment.Properties != nil && - assignment.Properties.PrincipalID != nil && - assignment.Properties.RoleDefinitionID != nil && - *assignment.Properties.PrincipalID == principalID && - strings.HasSuffix(*assignment.Properties.RoleDefinitionID, roleDefinitionSuffix) { - alreadyExists = true - break - } - } - if alreadyExists { - break - } - } - - if alreadyExists { - return false, nil - } - - // Create assignment - fullRoleDefinitionID := fmt.Sprintf( - "/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", extractSubscriptionID(scope), roleID) - roleAssignmentName := uuid.New().String() - parameters := armauthorization.RoleAssignmentCreateParameters{ - Properties: &armauthorization.RoleAssignmentProperties{ - RoleDefinitionID: new(fullRoleDefinitionID), - PrincipalID: new(principalID), - PrincipalType: &principalType, - }, - } - - _, err := client.Create(ctx, scope, roleAssignmentName, parameters, nil) - if err != nil { - if strings.Contains(err.Error(), "RoleAssignmentExists") || - strings.Contains(err.Error(), "409") { - return false, nil - } - return false, fmt.Errorf("failed to create role assignment for role %s: %w", roleName, err) - } - - return true, nil -} - -// extractSubscriptionID pulls the subscription ID from an Azure resource ID. -func extractSubscriptionID(resourceID string) string { - parts := strings.Split(resourceID, "/") - for i, part := range parts { - if part == "subscriptions" && i+1 < len(parts) { - return parts[i+1] - } - } - return "" -} diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac_test.go b/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac_test.go index 960e481ffd8..fddee905d52 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac_test.go @@ -4,71 +4,12 @@ package project import ( - "encoding/json" - "io" - "net/http" - "strings" "testing" - "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" - "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestAssignRoleToIdentityRequest(t *testing.T) { - const scope = "/subscriptions/sub-123/resourceGroups/rg/providers/" + - "Microsoft.CognitiveServices/accounts/account/projects/project" - const principalID = "11111111-2222-3333-4444-555555555555" - for _, principalType := range []armauthorization.PrincipalType{ - armauthorization.PrincipalTypeUser, armauthorization.PrincipalTypeServicePrincipal, - } { - t.Run(string(principalType), func(t *testing.T) { - var methods []string - transport := storageTestTransport(func(request *http.Request) (*http.Response, error) { - methods = append(methods, request.Method) - body := `{"value":[]}` - switch request.Method { - case http.MethodGet: - require.Equal(t, scope+"/providers/Microsoft.Authorization/roleAssignments", request.URL.Path) - require.Equal(t, "atScope()", request.URL.Query().Get("$filter")) - case http.MethodPut: - require.True(t, strings.HasPrefix(request.URL.Path, - scope+"/providers/Microsoft.Authorization/roleAssignments/")) - var parameters armauthorization.RoleAssignmentCreateParameters - require.NoError(t, json.NewDecoder(request.Body).Decode(¶meters)) - require.NotNil(t, parameters.Properties) - require.Equal(t, new("/subscriptions/sub-123/providers/Microsoft.Authorization/roleDefinitions/"+ - roleAzureAIUser), parameters.Properties.RoleDefinitionID) - require.Equal(t, new(principalID), parameters.Properties.PrincipalID) - require.Equal(t, new(principalType), parameters.Properties.PrincipalType) - body = `{"properties":{}}` - default: - t.Fatalf("unexpected request method %s", request.Method) - } - return &http.Response{ - StatusCode: http.StatusOK, Header: http.Header{"Content-Type": {"application/json"}}, - Body: io.NopCloser(strings.NewReader(body)), Request: request, - }, nil - }) - client, err := armauthorization.NewRoleAssignmentsClient("sub-123", storageTestCredential{}, &arm.ClientOptions{ - ClientOptions: azcore.ClientOptions{Transport: transport, Retry: policy.RetryOptions{MaxRetries: -1}}, - }) - require.NoError(t, err) - - created, err := assignRoleToIdentityWithClient( - t.Context(), client, principalID, roleAzureAIUser, "Foundry User", scope, principalType, - ) - - require.NoError(t, err) - require.True(t, created) - require.Equal(t, []string{http.MethodGet, http.MethodPut}, methods) - }) - } -} - func TestParseAgentIdentityInfo(t *testing.T) { tests := []struct { name string @@ -146,43 +87,6 @@ func TestParseAgentIdentityInfo(t *testing.T) { } } -func TestExtractSubscriptionID(t *testing.T) { - tests := []struct { - name string - resourceID string - want string - }{ - { - name: "valid resource ID", - resourceID: "/subscriptions/sub-123/resourceGroups/rg-test/providers/" + - "Microsoft.CognitiveServices/accounts/my-account", - want: "sub-123", - }, - { - name: "no subscription segment", - resourceID: "/resourceGroups/rg-test/providers/Microsoft.CognitiveServices/accounts/my-account", - want: "", - }, - { - name: "empty string", - resourceID: "", - want: "", - }, - { - name: "subscription at end with no value", - resourceID: "/subscriptions/", - want: "", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := extractSubscriptionID(tt.resourceID) - assert.Equal(t, tt.want, got) - }) - } -} - func TestConstants(t *testing.T) { assert.Equal(t, "53ca6127-db72-4b80-b1b0-d745d6d5456d", roleAzureAIUser) } diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check.go b/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check.go index 13d74131969..f38dd1c5027 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check.go @@ -7,17 +7,17 @@ import ( "context" "errors" "fmt" - "net/http" "os" - "strconv" "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/containerregistry/armcontainerregistry" "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/azure/azure-dev/cli/azd/pkg/foundry" "github.com/azure/azure-dev/cli/azd/pkg/output" + + "azureaiagent/internal/exterrors" + "azureaiagent/internal/pkg/agents/agent_api" ) const ( @@ -30,7 +30,7 @@ const ( // block Microsoft.Authorization/*/Write. roleUserAccessAdministrator = "18d7d88d-d35e-4fb5-a5c3-7773c20a72d9" roleRBACAdministrator = "f58310d9-a9f6-439a-9e8d-f62e7b41a168" - roleAzureAIProjectManager = "eadc314b-1a2d-4efa-be10-5d325db5065e" + roleAzureAIProjectManager = foundry.FoundryProjectManagerRole roleAzureAIAccountOwner = "e47c6f54-e4a2-4754-9501-8e0985b135e1" // Classic ACR roles that grant push/build access. @@ -48,12 +48,12 @@ const ( // Cognitive Services User: backward-compat alias for Foundry User. // Assigned by older Bicep provisions (AZURE_PRINCIPAL_ID flow); has the same // Microsoft.CognitiveServices/* dataActions as Foundry User and is accepted as equivalent. - roleCognitiveServicesUser = "a97b65f3-24c7-4388-baec-2e87135dc908" + roleCognitiveServicesUser = foundry.CognitiveServicesUserRole // Foundry Owner: full Foundry project management + Microsoft.CognitiveServices/* dataActions. // The code comment on sufficientAIUserRoles already recommends this role; include it so users // with Foundry Owner are not falsely flagged as missing the data-plane role. - roleFoundryOwner = "c883944f-8b7b-4483-af10-35834be79c4a" + roleFoundryOwner = foundry.FoundryOwnerRole ) // sufficientACRRoles lists every role that grants enough ACR access to build @@ -83,12 +83,7 @@ var sufficientACRAbacRoles = []string{ // - roleAzureAIDeveloper: dataActions cover only OpenAI/Speech/ContentSafety/MaaS, // NOT the AIServices.* namespace used by Foundry agents; Azure docs state // "For Foundry project access, use the Foundry User or Foundry Owner roles instead" -var sufficientAIUserRoles = []string{ - roleAzureAIUser, // 53ca6127-... Foundry User: dataActions Microsoft.CognitiveServices/* - roleCognitiveServicesUser, // a97b65f3-... backward compat: same dataActions, assigned by older Bicep - roleAzureAIProjectManager, // eadc314b-... Foundry Project Manager: dataActions Microsoft.CognitiveServices/* - roleFoundryOwner, // c883944f-... Foundry Owner: dataActions Microsoft.CognitiveServices/* -} +var sufficientAIUserRoles = foundry.ProjectAccessRoles() // sufficientRoleAssignWriteRoles lists every role that grants // Microsoft.Authorization/roleAssignments/write on Azure Resource Manager. @@ -105,110 +100,94 @@ var sufficientRoleAssignWriteRoles = []string{ } // CheckDeveloperRBAC checks project access and hosted-only permissions when needed. -// Missing roles are warnings so deployment can proceed when access is managed externally. +// Optional ARM failures are warnings; newly granted data-plane access is checked before deployment. func CheckDeveloperRBAC(ctx context.Context, azdClient *azdext.AzdClient, hasHosted bool) error { - envClient := azdClient.Environment() - envResp, err := envClient.GetCurrent(ctx, &azdext.EmptyRequest{}) + values, err := foundry.LoadProjectEnvironment(ctx, azdClient.Environment()) if err != nil { - return fmt.Errorf("failed to get current environment: %w", err) - } - envName := envResp.Environment.Name - - skipValue := "" - skipResp, err := envClient.GetValue(ctx, &azdext.GetEnvRequest{ - EnvName: envName, - Key: "AZD_AGENT_SKIP_ROLE_ASSIGNMENTS", - }) - if err == nil { - skipValue = skipResp.Value - } - if skipValue == "" { - skipValue = os.Getenv("AZD_AGENT_SKIP_ROLE_ASSIGNMENTS") + return fmt.Errorf("failed to get environment values for RBAC check: %w", err) } - if skip, parseErr := strconv.ParseBool(skipValue); parseErr == nil && skip { + if foundry.RoleAssignmentsSkipped( + values["AZD_AGENT_SKIP_ROLE_ASSIGNMENTS"], os.Getenv("AZD_AGENT_SKIP_ROLE_ASSIGNMENTS")) { fmt.Println(" (-) Skipping developer RBAC pre-flight check (AZD_AGENT_SKIP_ROLE_ASSIGNMENTS is set)") return nil } - projectResp, err := envClient.GetValue(ctx, &azdext.GetEnvRequest{ - EnvName: envName, - Key: "AZURE_AI_PROJECT_ID", - }) - if err != nil || projectResp.Value == "" { - // Can't check RBAC without the project ID; deployment will fail later with a clearer message. + warn := func(message string) { fmt.Println(output.WithWarningFormat(" Warning: %s", message)) } + endpoint := values["FOUNDRY_PROJECT_ENDPOINT"] + if endpoint == "" { + endpoint = values["AZURE_AI_PROJECT_ENDPOINT"] + } + resource, err := foundry.ProjectScopeFromEnvironment(endpoint, values) + if err != nil || resource == nil { + warn("No matching Foundry project ID and endpoint are available; skipping developer RBAC preparation.") return nil } - - info, err := parseAgentIdentityInfo(projectResp.Value) - if err != nil { - return nil // Non-critical: let deployment handle parse errors. + info := &agentIdentityInfo{ + SubscriptionID: resource.SubscriptionID, ProjectScope: resource.String(), + AccountName: resource.Parent.Name, ProjectName: resource.Name, } - tenantResponse, err := azdClient.Account().LookupTenant(ctx, &azdext.LookupTenantRequest{ - SubscriptionId: info.SubscriptionID, - }) - if err != nil { - return nil // Non-critical: can't resolve tenant for pre-flight check. + tenantID, err := foundry.LookupProjectTenant(ctx, azdClient.Account(), info.SubscriptionID) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr } - - cred, err := azidentity.NewAzureDeveloperCLICredential(&azidentity.AzureDeveloperCLICredentialOptions{ - TenantID: tenantResponse.TenantId, - AdditionallyAllowedTenants: []string{"*"}, - }) if err != nil { - return nil // Non-critical: auth issues will surface during deploy. + warn("Could not resolve the project user-access tenant; skipping developer RBAC preparation.") + return nil } - fmt.Println() - fmt.Println("Developer RBAC pre-flight check") - - principalID, principalType, err := developerRBACPrincipal(ctx, azdClient.Account(), info.SubscriptionID) + cred, err := foundry.NewCredential(tenantID) if err != nil { - fmt.Printf(" ⚠ Could not resolve current principal: %s — skipping RBAC pre-flight check\n", err) + warn("Could not create the project credential; skipping developer RBAC preparation.") return nil } - fmt.Printf(" Developer: %s (%s)\n", principalID, principalType) + fmt.Println() + fmt.Println("Developer RBAC pre-flight check") - // Check 1: Foundry User (or a superset) is sufficient to use the agent data plane. - hasAIAccess, err := hasAnyRoleAssignment(ctx, cred, principalID, sufficientAIUserRoles, info.ProjectScope) - if err != nil { - fmt.Printf(" ⚠ Could not check AI User role: %s\n", err) - } else if !hasAIAccess { - fmt.Println(" Foundry User role not found — attempting to auto-assign...") - if _, assignErr := assignRoleToIdentity( - ctx, cred, principalID, roleAzureAIUser, - "Foundry User → Foundry Project", info.ProjectScope, - principalType, - ); assignErr != nil { - if respErr, ok := errors.AsType[*azcore.ResponseError](assignErr); ok && - respErr.StatusCode == http.StatusForbidden { - fmt.Printf("%s\n", output.WithWarningFormat( - "Your identity (%s) does not have the 'Foundry User' role on the Foundry Project %s/%s "+ - "and auto-assign was denied.\n"+ - " Ask a subscription Owner or User Access Administrator to assign the role:\n"+ - " az role assignment create --assignee %s --role "+roleAzureAIUser+ - " --scope %q # Foundry User (formerly Azure AI User)", - principalID, info.AccountName, info.ProjectName, - principalID, info.ProjectScope, - )) - } else { - fmt.Printf(" ⚠ Foundry User auto-assign failed (non-auth error): %s — continuing\n", assignErr) - } - } else { - fmt.Println(" ✓ Foundry User auto-assigned to developer identity") + agentClient := agent_api.NewAgentClient(strings.TrimRight(endpoint, "/"), cred) + if err := (foundry.AccessPreparation{ + Account: azdClient.Account(), Credential: cred, Scope: info.ProjectScope, + Probe: func(ctx context.Context) error { + _, err := agentClient.ListAgents(ctx, &agent_api.ListAgentQueryParameters{Limit: new(int32(1))}, + agent_api.AgentEndpointAPIVersion) + return err + }, + Warn: warn, + Progress: func(message string) { + fmt.Println(" " + message) + }, + }).Ensure(ctx); err != nil { + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr } - } else { - fmt.Println(" ✓ Foundry User on Foundry Project") + serviceErr := exterrors.ServiceFromAzure(err, exterrors.OpCheckProjectAccess) + if structured, ok := errors.AsType[*azdext.ServiceError](serviceErr); ok { + structured.Suggestion = "Foundry User was assigned but access has not propagated. Wait and retry; " + + "if access is still denied, ask an administrator to verify project access." + } + return serviceErr } if !hasHosted { return nil } + principalID, principalType, err := foundry.ResolvePrincipal(ctx, azdClient.Account(), info.SubscriptionID) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } + if err != nil { + warn("Could not resolve the current principal; skipping hosted-agent permission checks.") + return nil + } + fmt.Printf(" Developer: %s (%s)\n", principalID, principalType) // Check 2: roleAssignments/write capability on Foundry Project scope. // Required for the postdeploy hook to assign Foundry User to agent service principals. // Note: Contributor cannot write role assignments (it is excluded from sufficientRoleAssignWriteRoles). - hasRoleWrite, err := hasAnyRoleAssignment(ctx, cred, principalID, sufficientRoleAssignWriteRoles, info.ProjectScope) + hasRoleWrite, err := foundry.HasAnyRoleAssignment(ctx, cred, principalID, sufficientRoleAssignWriteRoles, info.ProjectScope) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } if err != nil { fmt.Printf(" ⚠ Could not check role-assignment-write capability: %s\n", err) } else if !hasRoleWrite { @@ -234,11 +213,8 @@ func CheckDeveloperRBAC(ctx context.Context, azdClient *azdext.AzdClient, hasHos } // Check 3: ACR role — must branch on registry mode (ABAC vs classic). - acrResp, err := envClient.GetValue(ctx, &azdext.GetEnvRequest{ - EnvName: envName, - Key: "AZURE_CONTAINER_REGISTRY_ENDPOINT", - }) - if err != nil || acrResp.Value == "" { + acrEndpoint := values["AZURE_CONTAINER_REGISTRY_ENDPOINT"] + if acrEndpoint == "" { fmt.Println(" ⚠ AZURE_CONTAINER_REGISTRY_ENDPOINT not set — skipping ACR role check") return nil } @@ -249,14 +225,13 @@ func CheckDeveloperRBAC(ctx context.Context, azdClient *azdext.AzdClient, hasHos acrResourceID string isAbac bool ) - acrRidResp, _ := envClient.GetValue(ctx, &azdext.GetEnvRequest{ - EnvName: envName, - Key: "AZURE_CONTAINER_REGISTRY_RESOURCE_ID", - }) - if acrRidResp != nil && acrRidResp.Value != "" { - acrResourceID, isAbac, err = resolveACRInfoByResourceID(ctx, cred, acrRidResp.Value) + if resourceID := values["AZURE_CONTAINER_REGISTRY_RESOURCE_ID"]; resourceID != "" { + acrResourceID, isAbac, err = resolveACRInfoByResourceID(ctx, cred, resourceID) } else { - acrResourceID, isAbac, err = resolveACRInfo(ctx, cred, info.SubscriptionID, acrResp.Value) + acrResourceID, isAbac, err = resolveACRInfo(ctx, cred, info.SubscriptionID, acrEndpoint) + } + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr } if err != nil { fmt.Printf(" ⚠ Could not resolve ACR resource info: %s — skipping ACR role check\n", err) @@ -268,14 +243,17 @@ func CheckDeveloperRBAC(ctx context.Context, azdClient *azdext.AzdClient, hasHos acrRolesToCheck = sufficientACRAbacRoles } - hasACRAccess, err := hasAnyRoleAssignment(ctx, cred, principalID, acrRolesToCheck, acrResourceID) + hasACRAccess, err := foundry.HasAnyRoleAssignment(ctx, cred, principalID, acrRolesToCheck, acrResourceID) + if cancelErr := foundry.AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } if err != nil { fmt.Printf(" ⚠ Could not check ACR role: %s\n", err) return nil } if !hasACRAccess { - acrName := strings.TrimSuffix(normalizeLoginServer(acrResp.Value), ".azurecr.io") + acrName := strings.TrimSuffix(normalizeLoginServer(acrEndpoint), ".azurecr.io") if isAbac { fmt.Printf("%s\n", output.WithWarningFormat( "Your identity (%s) does not have the required role on the ABAC-mode Container Registry '%s' "+ @@ -315,81 +293,6 @@ func CheckDeveloperRBAC(ctx context.Context, azdClient *azdext.AzdClient, hasHos return nil } -func developerRBACPrincipal( - ctx context.Context, - account azdext.AccountServiceClient, - subscriptionID string, -) (string, armauthorization.PrincipalType, error) { - // The host resolves object IDs in the resource tenant for both user and app logins. - principal, err := account.GetCurrentPrincipal(ctx, &azdext.GetCurrentPrincipalRequest{SubscriptionId: subscriptionID}) - if err != nil { - return "", "", err - } - if strings.TrimSpace(principal.GetObjectId()) == "" { - return "", "", fmt.Errorf("the azd host returned an empty current principal object ID") - } - switch principal.GetPrincipalType() { - case azdext.PrincipalType_PRINCIPAL_TYPE_USER: - return principal.GetObjectId(), armauthorization.PrincipalTypeUser, nil - case azdext.PrincipalType_PRINCIPAL_TYPE_SERVICE_PRINCIPAL: - return principal.GetObjectId(), armauthorization.PrincipalTypeServicePrincipal, nil - default: - return "", "", fmt.Errorf("the azd host returned an unsupported current principal type: %v", principal.GetPrincipalType()) - } -} - -// hasAnyRoleAssignment checks whether the given principal has any of the specified roles -// at the given scope (including inherited assignments from parent scopes). -// Uses server-side filtering by principal to reduce API load on large subscriptions. -func hasAnyRoleAssignment( - ctx context.Context, - cred *azidentity.AzureDeveloperCLICredential, - principalID string, - roleIDs []string, - scope string, -) (bool, error) { - subscriptionID := extractSubscriptionID(scope) - if subscriptionID == "" { - return false, fmt.Errorf("could not extract subscription ID from scope: %s", scope) - } - - client, err := armauthorization.NewRoleAssignmentsClient(subscriptionID, cred, nil) - if err != nil { - return false, fmt.Errorf("failed to create role assignments client: %w", err) - } - - // Build a set of suffixes to match against. - suffixes := make(map[string]bool, len(roleIDs)) - for _, id := range roleIDs { - suffixes[fmt.Sprintf("/roleDefinitions/%s", id)] = true - } - - // Use server-side assignedTo filter to only return assignments for this principal. - filter := fmt.Sprintf("assignedTo('%s')", principalID) - pager := client.NewListForScopePager(scope, &armauthorization.RoleAssignmentsClientListForScopeOptions{ - Filter: &filter, - }) - for pager.More() { - page, err := pager.NextPage(ctx) - if err != nil { - return false, fmt.Errorf("failed to list role assignments: %w", err) - } - for _, assignment := range page.Value { - if assignment.Properties == nil || assignment.Properties.RoleDefinitionID == nil { - continue - } - roleDefID := *assignment.Properties.RoleDefinitionID - for suffix := range suffixes { - if strings.HasSuffix(roleDefID, suffix) { - return true, nil - } - } - } - } - - return false, nil -} - // normalizeLoginServer strips protocol prefixes, trailing slashes, and lowercases // an ACR login server endpoint for consistent comparison. func normalizeLoginServer(loginServer string) string { @@ -409,7 +312,7 @@ func normalizeLoginServer(loginServer string) string { // /subscriptions/{sub}/resourceGroups/{rg}/providers/Microsoft.ContainerRegistry/registries/{name} func resolveACRInfoByResourceID( ctx context.Context, - cred *azidentity.AzureDeveloperCLICredential, + cred azcore.TokenCredential, resourceID string, ) (string, bool, error) { // Parse resource group and registry name from the ARM resource ID. @@ -461,7 +364,7 @@ func resolveACRInfoByResourceID( // isAbac is true when the registry uses roleAssignmentMode == AbacRepositoryPermissions. func resolveACRInfo( ctx context.Context, - cred *azidentity.AzureDeveloperCLICredential, + cred azcore.TokenCredential, subscriptionID string, loginServer string, ) (resourceID string, isAbac bool, err error) { diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check_test.go b/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check_test.go index bcc14d50e53..c98e08e3722 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check_test.go @@ -8,8 +8,8 @@ import ( "net" "testing" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/azure/azure-dev/cli/azd/pkg/foundry" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" @@ -52,13 +52,13 @@ func TestDeveloperRBACPrincipalUsesStableRPC(t *testing.T) { require.NoError(t, err) t.Cleanup(client.Close) - id, principalType, err := developerRBACPrincipal(t.Context(), client.Account(), "selected-subscription") + id, principalType, err := foundry.ResolvePrincipal(t.Context(), client.Account(), "selected-subscription") require.NoError(t, err) assert.Equal(t, "resource-tenant-object-id", id) - assert.Equal(t, armauthorization.PrincipalTypeUser, principalType) + assert.Equal(t, "User", string(principalType)) assert.Equal(t, "/azd.extensions.v1.AccountService/GetCurrentPrincipal", <-methods) - _, _, err = developerRBACPrincipal(t.Context(), client.Account(), "") + _, _, err = foundry.ResolvePrincipal(t.Context(), client.Account(), "") require.Equal(t, codes.InvalidArgument, status.Code(err)) assert.Equal(t, "/azd.extensions.v1.AccountService/GetCurrentPrincipal", <-methods) } @@ -83,7 +83,7 @@ func TestDeveloperRBACPrincipal(t *testing.T) { name string response *azdext.GetCurrentPrincipalResponse err error - wantType armauthorization.PrincipalType + wantType string wantErr string }{ { @@ -91,14 +91,14 @@ func TestDeveloperRBACPrincipal(t *testing.T) { response: &azdext.GetCurrentPrincipalResponse{ ObjectId: "resource-tenant-object-id", PrincipalType: azdext.PrincipalType_PRINCIPAL_TYPE_USER, }, - wantType: armauthorization.PrincipalTypeUser, + wantType: "User", }, { name: "service principal", response: &azdext.GetCurrentPrincipalResponse{ ObjectId: "service-principal-object-id", PrincipalType: azdext.PrincipalType_PRINCIPAL_TYPE_SERVICE_PRINCIPAL, }, - wantType: armauthorization.PrincipalTypeServicePrincipal, + wantType: "ServicePrincipal", }, {name: "lookup failure", err: lookupErr, wantErr: "subscription unavailable"}, {name: "empty ID", response: &azdext.GetCurrentPrincipalResponse{}, wantErr: "empty current principal object ID"}, @@ -110,7 +110,7 @@ func TestDeveloperRBACPrincipal(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { account := &developerRBACAccountClient{response: tt.response, err: tt.err} - id, principalType, err := developerRBACPrincipal(t.Context(), account, "selected-subscription") + id, principalType, err := foundry.ResolvePrincipal(t.Context(), account, "selected-subscription") assert.Equal(t, "selected-subscription", account.subscriptionID) if tt.wantErr != "" { require.ErrorContains(t, err, tt.wantErr) @@ -123,7 +123,7 @@ func TestDeveloperRBACPrincipal(t *testing.T) { } require.NoError(t, err) assert.Equal(t, tt.response.ObjectId, id) - assert.Equal(t, tt.wantType, principalType) + assert.Equal(t, tt.wantType, string(principalType)) }) } } @@ -209,3 +209,116 @@ func TestSufficientRoleLists(t *testing.T) { assert.Contains(t, sufficientACRAbacRoles, roleContainerRegistryRepositoryContributor) assert.NotContains(t, sufficientACRAbacRoles, roleAcrPush) } + +type developerAccessEnvironmentServer struct { + azdext.UnimplementedEnvironmentServiceServer + values map[string]string + err error +} + +func (s *developerAccessEnvironmentServer) GetValues( + _ context.Context, req *azdext.GetEnvironmentRequest, +) (*azdext.KeyValueListResponse, error) { + if req.GetName() != "" { + return nil, status.Error(codes.InvalidArgument, "wrong environment") + } + if s.err != nil { + return nil, s.err + } + response := &azdext.KeyValueListResponse{} + for key, value := range s.values { + response.KeyValues = append(response.KeyValues, &azdext.KeyValue{Key: key, Value: value}) + } + return response, nil +} + +type developerAccessAccountServer struct { + azdext.UnimplementedAccountServiceServer + requests chan string + err error +} + +func (s *developerAccessAccountServer) LookupTenant( + _ context.Context, req *azdext.LookupTenantRequest, +) (*azdext.LookupTenantResponse, error) { + s.requests <- req.GetSubscriptionId() + return &azdext.LookupTenantResponse{}, s.err +} + +func TestCheckDeveloperRBACScopeAndSkip(t *testing.T) { + const scope = "/subscriptions/sub/resourceGroups/rg/providers/" + + "Microsoft.CognitiveServices/accounts/acct/projects/proj" + const endpoint = "https://acct.services.ai.azure.com/api/projects/proj" + for _, tt := range []struct { + name string + change func(map[string]string) + processSkip string + envErr error + tenantErr error + wantLookup bool + wantCode codes.Code + }{ + {name: "empty tenant stops before credential or role writes", wantLookup: true}, + {name: "legacy endpoint", change: func(v map[string]string) { + delete(v, "FOUNDRY_PROJECT_ENDPOINT") + v["AZURE_AI_PROJECT_ENDPOINT"] = endpoint + }, wantLookup: true}, + {name: "Foundry endpoint wins", change: func(v map[string]string) { + v["AZURE_AI_PROJECT_ENDPOINT"] = "https://other.services.ai.azure.com/api/projects/other" + }, wantLookup: true}, + {name: "missing project", change: func(v map[string]string) { delete(v, "AZURE_AI_PROJECT_ID") }}, + {name: "missing endpoint", change: func(v map[string]string) { delete(v, "FOUNDRY_PROJECT_ENDPOINT") }}, + {name: "unrelated endpoint", change: func(v map[string]string) { + v["FOUNDRY_PROJECT_ENDPOINT"] = "https://other.services.ai.azure.com/api/projects/proj" + }}, + {name: "child resource", change: func(v map[string]string) { v["AZURE_AI_PROJECT_ID"] += "/agents/agent" }}, + {name: "subscription conflict", change: func(v map[string]string) { v["AZURE_SUBSCRIPTION_ID"] = "other" }}, + {name: "explicit opt out", change: func(v map[string]string) { v["AZD_AGENT_SKIP_ROLE_ASSIGNMENTS"] = "true" }}, + {name: "process opt out", processSkip: "true"}, + {name: "persisted false wins", processSkip: "true", change: func(v map[string]string) { + v["AZD_AGENT_SKIP_ROLE_ASSIGNMENTS"] = "false" + }, wantLookup: true}, + {name: "invalid persisted value does not skip", processSkip: "true", change: func(v map[string]string) { + v["AZD_AGENT_SKIP_ROLE_ASSIGNMENTS"] = "invalid" + }, wantLookup: true}, + {name: "environment cancellation", envErr: status.Error(codes.Canceled, "cancelled"), wantCode: codes.Canceled}, + {name: "tenant cancellation", tenantErr: status.Error(codes.Canceled, "cancelled"), + wantLookup: true, wantCode: codes.Canceled}, + {name: "tenant deadline", tenantErr: status.Error(codes.DeadlineExceeded, "timeout"), + wantLookup: true, wantCode: codes.DeadlineExceeded}, + {name: "tenant lookup unavailable", tenantErr: status.Error(codes.Unavailable, "unavailable"), wantLookup: true}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("AZD_AGENT_SKIP_ROLE_ASSIGNMENTS", tt.processSkip) + values := map[string]string{"AZURE_AI_PROJECT_ID": scope, "FOUNDRY_PROJECT_ENDPOINT": endpoint} + if tt.change != nil { + tt.change(values) + } + account := &developerAccessAccountServer{requests: make(chan string, 2), err: tt.tenantErr} + server := grpc.NewServer() + azdext.RegisterEnvironmentServiceServer(server, &developerAccessEnvironmentServer{values: values, err: tt.envErr}) + azdext.RegisterAccountServiceServer(server, account) + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + go func() { _ = server.Serve(listener) }() + t.Cleanup(func() { + server.Stop() + _ = listener.Close() + }) + client, err := azdext.NewAzdClient(azdext.WithAddress(listener.Addr().String())) + require.NoError(t, err) + t.Cleanup(client.Close) + + for _, hosted := range []bool{false, true} { + err := CheckDeveloperRBAC(t.Context(), client, hosted) + require.Equal(t, tt.wantCode, status.Code(err)) + if tt.wantLookup { + require.Len(t, account.requests, 1) + assert.Equal(t, "sub", <-account.requests) + } else { + assert.Empty(t, account.requests, "invalid or skipped input must not reach authentication") + } + } + }) + } +} diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_query.go b/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_query.go index 2ce36ab756d..3e6a4bf17f7 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_query.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_query.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/azure/azure-dev/cli/azd/pkg/foundry" "github.com/azure/azure-dev/cli/azd/pkg/graphsdk" ) @@ -143,7 +144,7 @@ func QueryDeveloperRBAC( return nil, fmt.Errorf("retrieve user profile: %w", err) } - hasRole, err := hasAnyRoleAssignment( + hasRole, err := foundry.HasAnyRoleAssignment( ctx, cred, userProfile.Id, sufficientAIUserRoles, info.ProjectScope) if err != nil { return nil, fmt.Errorf("list role assignments: %w", err) diff --git a/cli/azd/extensions/azure.ai.agents/tests/cli-interactive-tester-scenarios/tier2/2.00-setup-deploy-shared-agent.yaml b/cli/azd/extensions/azure.ai.agents/tests/cli-interactive-tester-scenarios/tier2/2.00-setup-deploy-shared-agent.yaml index 69743c0cae7..fb2d36f2e68 100644 --- a/cli/azd/extensions/azure.ai.agents/tests/cli-interactive-tester-scenarios/tier2/2.00-setup-deploy-shared-agent.yaml +++ b/cli/azd/extensions/azure.ai.agents/tests/cli-interactive-tester-scenarios/tier2/2.00-setup-deploy-shared-agent.yaml @@ -59,6 +59,7 @@ goals: - "Before provisioning, run the shared managed-deployment override helper at '{fixtures_dir}/scripts/override-model-deployment.sh' from inside the '{shared_agent_name}' subdirectory with model '{model}', optional version '{model_version}', and SKU '{model_sku}'. Require a successful exit before continuing. The helper must resolve and preserve the existing deployment name, perform at most one replacement, and verify the command's JSON result; when either model or SKU is empty, it must skip without changing the scaffold." - "Run 'azd provision' (from inside the '{shared_agent_name}' subdirectory) and wait for it to succeed. This provisions the Azure infrastructure (Foundry project/account, model deployment, etc.) but does NOT yet deploy the agent." - "After provision succeeds, run 'azd deploy' (from the same subdirectory) and wait for it to succeed. This deploys the agent to the provisioned infrastructure — it is a required, separate step from provision before the agent can be shown or invoked." + - "During deploy, confirm developer Foundry access preparation does not prompt for role assignment or require a manual grant before continuing. If an automatic grant has not propagated within the bounded readiness checks, the command must exit nonzero with 'Foundry User was assigned but access has not propagated' and wait-and-retry guidance, rather than hanging or claiming success. Record that result as a failed deployment; do not retry, alter roles, or set AZD_AGENT_SKIP_ROLE_ASSIGNMENTS to bypass it." - "After deploy, run 'azd ai agent show' and note the agent name and endpoint URL — record these for the targeted scenarios." - "Take a screenshot of the successful provision, deploy, and 'show' output." - "Report a finding if init, the model SKU replacement, provision, or deploy fails, hangs, or produces a confusing error. Do NOT run 'azd down' here — teardown is a separate scenario." diff --git a/cli/azd/extensions/azure.ai.toolboxes/AGENTS.md b/cli/azd/extensions/azure.ai.toolboxes/AGENTS.md index 6a7af499f11..7c85961551e 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/AGENTS.md +++ b/cli/azd/extensions/azure.ai.toolboxes/AGENTS.md @@ -16,8 +16,8 @@ Useful places to start: - `internal/cmd/`: Cobra commands, verb implementations, and `connection` subgroup - `internal/pkg/azure/`: `FoundryToolboxClient` (toolbox CRUD data-plane client) -- `internal/foundry/`: Foundry primitives shared with sibling extensions - (project-endpoint cascade, credential factory, single-connection lookup) +- `internal/foundry/`: project-endpoint cascade and single-connection lookup. + Credentials and developer-access policy use core's shared `pkg/foundry`. - `internal/exterrors/`: structured error factories and toolbox-specific codes ## Build and test @@ -53,14 +53,16 @@ present. `internal/foundry/` holds Foundry data-plane primitives that are currently duplicated from `azure.ai.agents` and intended to be lifted into a shared module -later. Two rules keep that future lift mechanical: +later. Credentials and developer-access preparation already live in core's +`pkg/foundry`; reuse those helpers rather than adding local copies. +Two rules keep the remaining lift mechanical: 1. **One-way import contract.** Files under `internal/foundry/` MUST NOT import anything under `internal/cmd/`. Files under `internal/cmd/` may import `internal/foundry/...` freely. 2. **No toolbox-specific logic inside `internal/foundry/`.** Keep that package to primitives any sibling extension would also need (project-endpoint - resolution, single-connection lookup, credential factory). Toolbox-specific + resolution and single-connection lookup). Toolbox-specific concepts (`projectConnection`, `connectionResolver`, the `FoundryToolboxClient`) live in `internal/cmd/` and `internal/pkg/azure/`. diff --git a/cli/azd/extensions/azure.ai.toolboxes/README.md b/cli/azd/extensions/azure.ai.toolboxes/README.md index 0e2004e39ad..67a711c2d23 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/README.md +++ b/cli/azd/extensions/azure.ai.toolboxes/README.md @@ -72,9 +72,10 @@ 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 +## Developer access -Managed toolbox deployment and `azd ai toolbox create` prepare Foundry access +Managed toolbox deployment, `create`, `connection add/remove`, `skill add/remove`, +and `publish` 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 @@ -106,8 +107,8 @@ 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. +on stdout. Read/list/show, toolbox deletion, 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 diff --git a/cli/azd/extensions/azure.ai.toolboxes/go.mod b/cli/azd/extensions/azure.ai.toolboxes/go.mod index 2ac9a223241..b21e29c960b 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/go.mod +++ b/cli/azd/extensions/azure.ai.toolboxes/go.mod @@ -4,7 +4,6 @@ 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 @@ -19,6 +18,7 @@ require ( require ( dario.cat/mergo v1.0.2 // indirect github.com/AlecAivazis/survey/v2 v2.3.7 // indirect + github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.15.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/internal v1.13.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/appservice/armappservice/v2 v2.3.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/keyvault/armkeyvault v1.5.0 // indirect 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 3e228e35f55..8e79fc5580b 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/context.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/context.go @@ -38,7 +38,7 @@ var toolboxNamePattern = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) const maxToolboxNameLength = 63 func validateToolboxWriteFlags(cmd *cobra.Command) error { - for _, name := range []string{"project-endpoint", "from-file"} { + for _, name := range []string{"project-endpoint", "from-file", "from-version"} { if !cmd.Flags().Changed(name) { continue } 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 8e80380117d..24a660ba127 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/root.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/root.go @@ -27,7 +27,8 @@ 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 +Managed deployment, 'create', 'connection add/remove', 'skill add/remove', and +'publish' 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.`, }) 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 f291177d5e1..28f088538f4 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,7 +6,8 @@ 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 +Managed deployment, 'create', 'connection add/remove', 'skill add/remove', and +'publish' 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. 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 08c82a093ae..eb4bde091eb 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 @@ -450,9 +450,21 @@ func TestToolboxCreateValidatesLocalInputBeforeAccess(t *testing.T) { } func TestToolboxCreateRejectsExplicitEmptyFlags(t *testing.T) { + stubToolboxEndpointEnv(t) + t.Setenv("AZD_SERVER", "") for _, args := range [][]string{ {"create", "research", "--from-file="}, {"create", "research", "--from-file=tools.yaml", "--project-endpoint="}, + {"publish", "research", "1", "--project-endpoint="}, + {"connection", "add", "research", "mcp", "--from-file="}, + {"connection", "add", "research", "mcp", "--index="}, + {"skill", "add", "research", "skill", "--from-file="}, + {"connection", "add", "research", "--from-file=tools.yaml", "--index="}, + {"connection", "add", "research", "--from-file=tools.yaml", "--instance-name="}, + {"connection", "add", "research", "mcp", "--from-version="}, + {"connection", "remove", "research", "mcp", "--from-version="}, + {"skill", "add", "research", "skill", "--from-version="}, + {"skill", "remove", "research", "skill", "--from-version="}, } { t.Run(strings.Join(args, " "), func(t *testing.T) { root := NewRootCommand() @@ -469,19 +481,23 @@ func TestToolboxCreateRejectsExplicitEmptyFlags(t *testing.T) { type toolboxAccessEnvironmentServer struct { azdext.UnimplementedEnvironmentServiceServer reads atomic.Int32 + err error } func (s *toolboxAccessEnvironmentServer) GetValues( context.Context, *azdext.GetEnvironmentRequest, ) (*azdext.KeyValueListResponse, error) { s.reads.Add(1) + if s.err != nil { + return nil, s.err + } 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) { +func TestToolboxWriteCommandsReachAccessPreparation(t *testing.T) { server := grpc.NewServer() environment := &toolboxAccessEnvironmentServer{} azdext.RegisterEnvironmentServiceServer(server, environment) @@ -493,14 +509,89 @@ func TestToolboxCreateCommandReachesAccessPreparation(t *testing.T) { }) 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()) + for _, args := range [][]string{ + {"create", "research", "--from-file=" + file}, + {"publish", "research", "1"}, + {"connection", "add", "research", "mcp"}, + {"connection", "remove", "research", "mcp", "--force"}, + {"skill", "add", "research", "skill"}, + {"skill", "remove", "research", "skill", "--force"}, + } { + t.Run(strings.Join(args[:2], " "), func(t *testing.T) { + root := NewRootCommand() + root.SetOut(io.Discard) + root.SetErr(io.Discard) + root.SetArgs(append(args, "--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(6), environment.reads.Load(), "every write command must reach scope validation") +} + +func TestToolboxBroaderWritesValidateBeforeAccess(t *testing.T) { + t.Setenv("AZD_SERVER", "") + for _, args := range [][]string{ + {"connection", "add", "research", "mcp", "--from-file=missing.yaml"}, + {"connection", "add", "research", "--from-file=missing.yaml"}, + {"connection", "add", "research", "mcp", "--index=x", "--instance-name=y"}, + {"skill", "add", "research", "skill@"}, + {"skill", "add", "research", "--from-file=" + writeTempFile(t, ".yaml", "skills:\n - name: INVALID\n")}, + {"skill", "add", "research", "--from-file=" + writeTempFile(t, ".yaml", + "skills:\n - name: skill\n - name: skill\n")}, + } { + t.Run(strings.Join(args, " "), func(t *testing.T) { + root := NewRootCommand() + root.SetOut(io.Discard) + root.SetErr(io.Discard) + root.SetArgs(append(args, "--project-endpoint="+toolboxAccessTestEndpoint)) + err := root.ExecuteContext(t.Context()) + require.Error(t, err) + assert.NotContains(t, err.Error(), "endpoint") + assert.NotContains(t, err.Error(), "credential") + }) + } +} + +type accessWarningWriter struct { + bytes.Buffer + cancel context.CancelFunc +} + +func (w *accessWarningWriter) Write(p []byte) (int, error) { + n, err := w.Buffer.Write(p) + w.cancel() + return n, err +} + +func TestToolboxBroaderWriteWarningsUseInjectedWriter(t *testing.T) { + server := grpc.NewServer() + azdext.RegisterEnvironmentServiceServer(server, &toolboxAccessEnvironmentServer{err: errors.New("unavailable")}) + 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()) + for _, args := range [][]string{ + {"publish", "research", "1"}, + {"connection", "add", "research", "mcp"}, + {"connection", "remove", "research", "mcp", "--force"}, + {"skill", "add", "research", "skill"}, + {"skill", "remove", "research", "skill", "--force"}, + } { + t.Run(strings.Join(args[:2], " "), func(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + diagnostics := &accessWarningWriter{cancel: cancel} + root := NewRootCommand() + root.SetOut(io.Discard) + root.SetErr(diagnostics) + root.SetArgs(append(args, "--project-endpoint="+toolboxAccessTestEndpoint)) + require.ErrorIs(t, root.ExecuteContext(ctx), context.Canceled) + assert.Contains(t, diagnostics.String(), "Foundry roles will not be assigned automatically.") + }) + } } func TestToolboxCreateRejectsMixedDuplicateNamesBeforeAccess(t *testing.T) { diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_commands_test.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_commands_test.go index 26b455a844a..b02e61c8019 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_commands_test.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_commands_test.go @@ -5,6 +5,7 @@ package cmd import ( "errors" + "io" "os" "testing" @@ -302,6 +303,7 @@ func TestRunConnectionAdd_RejectsEmptyConnectionWithoutFromFile(t *testing.T) { connectionAddFlags{}, toolboxFlags{output: "table"}, newStubConnectionResolver(), + io.Discard, ) requireLocalError(t, err, exterrors.CodeInvalidPositionalArg) } @@ -416,6 +418,7 @@ func TestRunToolboxPublish_WhitespaceVersion(t *testing.T) { err := runToolboxPublish( t.Context(), "tb", " ", toolboxFlags{output: "table"}, + io.Discard, ) requireLocalError(t, err, exterrors.CodeMissingUpdateField) } @@ -877,6 +880,7 @@ func TestRunConnectionRemove_NoPromptWithoutForce(t *testing.T) { connectionRemoveFlags{force: false}, toolboxFlags{output: "table", noPrompt: true}, newStubConnectionResolver(), + io.Discard, ) requireLocalError(t, err, exterrors.CodeMissingForceFlag) } diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_connection_add.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_connection_add.go index 1b6d522cce1..ec290212264 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_connection_add.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_connection_add.go @@ -6,9 +6,11 @@ package cmd import ( "context" "fmt" + "io" "slices" "strings" + "azure.ai.toolboxes/internal/definition" "azure.ai.toolboxes/internal/exterrors" "azure.ai.toolboxes/internal/pkg/azure" @@ -22,6 +24,8 @@ type connectionAddFlags struct { instanceName string fromFile string fromVersion string + indexSet bool + instanceSet bool } // newToolboxConnectionAddCommand returns the `connection add` command. @@ -84,6 +88,11 @@ At least one connection must be provided.`, return nil }, RunE: func(cmd *cobra.Command, args []string) error { + if err := validateToolboxWriteFlags(cmd); err != nil { + return err + } + flags.indexSet = cmd.Flags().Changed("index") + flags.instanceSet = cmd.Flags().Changed("instance-name") connName := "" if len(args) > 1 { connName = args[1] @@ -92,6 +101,7 @@ At least one connection must be provided.`, cmd.Context(), args[0], connName, *flags, readToolboxFlags(cmd, extCtx), defaultConnectionResolver{}, + cmd.ErrOrStderr(), ) }, } @@ -118,6 +128,7 @@ func runConnectionAdd( ctx context.Context, toolboxName, connName string, verb connectionAddFlags, parent toolboxFlags, resolver connectionResolver, + writer io.Writer, ) error { if err := validateToolboxName(toolboxName); err != nil { return err @@ -125,21 +136,18 @@ func runConnectionAdd( if err := validateOutputFormat(parent.output); err != nil { return err } - if strings.TrimSpace(verb.fromFile) == "" && strings.TrimSpace(connName) == "" { - return exterrors.Validation( - exterrors.CodeInvalidPositionalArg, - " must not be empty when --from-file is not set", - "pass a connection name or use --from-file", - ) + specs, err := collectConnectionSpecs(connName, verb) + if err != nil { + return err } - client, resolved, err := resolveToolboxAndClient(ctx, parent) + client, resolved, err := resolveToolboxWriteAndClient(ctx, parent, toolboxName, writer) if err != nil { return err } logResolvedEndpoint("toolbox connection add", resolved) - return runConnectionAddWith(ctx, client, resolver, resolved.Endpoint, toolboxName, connName, verb, parent) + return addToolboxConnections(ctx, client, resolver, resolved.Endpoint, toolboxName, specs, verb, parent) } // runConnectionAddWith is the testable core. @@ -150,6 +158,53 @@ func runConnectionAddWith( endpoint, toolboxName, connName string, verb connectionAddFlags, parent toolboxFlags, +) error { + specs, err := collectConnectionSpecs(connName, verb) + if err != nil { + return err + } + return addToolboxConnections(ctx, client, resolver, endpoint, toolboxName, specs, verb, parent) +} + +func collectConnectionSpecs(connName string, verb connectionAddFlags) ([]toolboxConnectionSpec, error) { + if strings.TrimSpace(verb.fromFile) == "" && strings.TrimSpace(connName) == "" { + return nil, exterrors.Validation(exterrors.CodeInvalidPositionalArg, + " must not be empty when --from-file is not set", "pass a connection name or use --from-file") + } + specs := []toolboxConnectionSpec{{Name: connName, Index: verb.index, InstanceName: verb.instanceName}} + if strings.TrimSpace(verb.fromFile) != "" { + if strings.TrimSpace(connName) != "" { + return nil, exterrors.Validation(exterrors.CodeInvalidPositionalArg, + "do not pass when --from-file is set", + "either pass a single connection positional or use --from-file") + } + if verb.indexSet || verb.index != "" { + return nil, exterrors.Validation(exterrors.CodeUnsupportedIndexFlag, + "--index cannot be used together with --from-file", "set connection indexes in the file") + } + if verb.instanceSet || verb.instanceName != "" { + return nil, exterrors.Validation(exterrors.CodeUnsupportedInstanceNameFlag, + "--instance-name cannot be used together with --from-file", "set connection instance names in the file") + } + var input toolboxToolsFile + if err := parseToolboxFile(verb.fromFile, &input); err != nil { + return nil, err + } + specs = input.Connections + } else if (verb.indexSet && strings.TrimSpace(verb.index) == "") || + (verb.instanceSet && strings.TrimSpace(verb.instanceName) == "") { + return nil, exterrors.Validation(exterrors.CodeInvalidParameter, + "--index and --instance-name must not be empty when supplied", "provide a non-empty value or omit the flag") + } + if err := validateToolboxDefinition(&definition.Definition{Connections: specs}); err != nil { + return nil, err + } + return specs, nil +} + +func addToolboxConnections( + ctx context.Context, client toolboxClient, resolver connectionResolver, + endpoint, toolboxName string, specs []toolboxConnectionSpec, verb connectionAddFlags, parent toolboxFlags, ) error { tb, err := client.GetToolbox(ctx, toolboxName) if err != nil { @@ -172,55 +227,13 @@ func runConnectionAddWith( return exterrors.ServiceFromAzure(err, exterrors.OpGetToolboxVersion) } - newEntries := []map[string]any{} - addedConnectionNames := []string{} - - if strings.TrimSpace(verb.fromFile) != "" { - if strings.TrimSpace(connName) != "" { - return exterrors.Validation( - exterrors.CodeInvalidPositionalArg, - "do not pass when --from-file is set", - "either pass a single connection positional or use --from-file", - ) - } - if verb.index != "" { - return exterrors.Validation( - exterrors.CodeUnsupportedIndexFlag, - "--index cannot be used together with --from-file", - "set connection indexes in the file under connections[].index", - ) - } - if verb.instanceName != "" { - return exterrors.Validation( - exterrors.CodeUnsupportedInstanceNameFlag, - "--instance-name cannot be used together with --from-file", - "set connection instance names in the file under connections[].instanceName", - ) - } - - var input toolboxToolsFile - if err := parseToolboxFile(verb.fromFile, &input); err != nil { - return err - } - resolvedEntries, err := resolveConnectionSpecs(ctx, resolver, endpoint, input.Connections) - if err != nil { - return err - } - for _, c := range input.Connections { - addedConnectionNames = append(addedConnectionNames, c.Name) - } - newEntries = append(newEntries, resolvedEntries...) - } else { - conn, err := resolver.resolveConnection(ctx, endpoint, connName) - if err != nil { - return err - } - entry, err := buildToolEntry(conn, verb.index, verb.instanceName) - if err != nil { - return err - } - newEntries = append(newEntries, entry) - addedConnectionNames = append(addedConnectionNames, conn.Name) + newEntries, err := resolveConnectionSpecs(ctx, resolver, endpoint, specs) + if err != nil { + return err + } + addedConnectionNames := make([]string, 0, len(specs)) + for _, spec := range specs { + addedConnectionNames = append(addedConnectionNames, spec.Name) } if len(newEntries) == 0 { diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_connection_remove.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_connection_remove.go index 60ef07c90fd..c2a85d1036a 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_connection_remove.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_connection_remove.go @@ -6,6 +6,7 @@ package cmd import ( "context" "fmt" + "io" "slices" "strings" @@ -44,11 +45,15 @@ Refuses to leave the toolbox with zero tools (use 'azd ai toolbox delete' instea `, Args: cobra.MinimumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { + if err := validateToolboxWriteFlags(cmd); err != nil { + return err + } return runConnectionRemove( cmd.Context(), args[0], args[1:], *flags, readToolboxFlags(cmd, extCtx), defaultConnectionResolver{}, + cmd.ErrOrStderr(), ) }, } @@ -65,6 +70,7 @@ func runConnectionRemove( ctx context.Context, toolboxName string, connNames []string, verb connectionRemoveFlags, parent toolboxFlags, resolver connectionResolver, + writer io.Writer, ) error { if err := validateToolboxName(toolboxName); err != nil { return err @@ -96,7 +102,7 @@ func runConnectionRemove( ) } - client, resolved, err := resolveToolboxAndClient(ctx, parent) + client, resolved, err := resolveToolboxWriteAndClient(ctx, parent, toolboxName, writer) if err != nil { return err } diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_publish.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_publish.go index 57fe93128f2..86525a52ffa 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_publish.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_publish.go @@ -6,6 +6,7 @@ package cmd import ( "context" "fmt" + "io" "strings" "azure.ai.toolboxes/internal/exterrors" @@ -30,7 +31,10 @@ use 'connection add', 'connection remove', 'skill add', or 'skill remove'.`, azd ai toolbox publish research 2`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - return runToolboxPublish(cmd.Context(), args[0], args[1], readToolboxFlags(cmd, extCtx)) + if err := validateToolboxWriteFlags(cmd); err != nil { + return err + } + return runToolboxPublish(cmd.Context(), args[0], args[1], readToolboxFlags(cmd, extCtx), cmd.ErrOrStderr()) }, } @@ -41,6 +45,7 @@ use 'connection add', 'connection remove', 'skill add', or 'skill remove'.`, func runToolboxPublish( ctx context.Context, name string, version string, parent toolboxFlags, + writer io.Writer, ) error { if err := validateToolboxName(name); err != nil { return err @@ -57,7 +62,7 @@ func runToolboxPublish( ) } - client, resolved, err := resolveToolboxAndClient(ctx, parent) + client, resolved, err := resolveToolboxWriteAndClient(ctx, parent, name, writer) if err != nil { return err } diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_add.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_add.go index f36e3a3768c..9aad24961ce 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_add.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_add.go @@ -6,6 +6,7 @@ package cmd import ( "context" "fmt" + "io" "slices" "strings" @@ -62,11 +63,14 @@ version at read time.`, return nil }, RunE: func(cmd *cobra.Command, args []string) error { + if err := validateToolboxWriteFlags(cmd); err != nil { + return err + } rawSkill := "" if len(args) > 1 { rawSkill = args[1] } - return runSkillAdd(cmd.Context(), args[0], rawSkill, *flags, readToolboxFlags(cmd, extCtx)) + return runSkillAdd(cmd.Context(), args[0], rawSkill, *flags, readToolboxFlags(cmd, extCtx), cmd.ErrOrStderr()) }, } cmd.Flags().StringVar( @@ -81,6 +85,7 @@ version at read time.`, func runSkillAdd( ctx context.Context, toolboxName, rawSkill string, verb skillAddFlags, parent toolboxFlags, + writer io.Writer, ) error { if err := validateToolboxName(toolboxName); err != nil { return err @@ -105,13 +110,17 @@ func runSkillAdd( ) } - client, resolved, err := resolveToolboxAndClient(ctx, parent) + specs, err := collectSkillSpecs(rawSkill, verb) + if err != nil { + return err + } + client, resolved, err := resolveToolboxWriteAndClient(ctx, parent, toolboxName, writer) if err != nil { return err } logResolvedEndpoint("toolbox skill add", resolved) - return runSkillAddWith(ctx, client, toolboxName, rawSkill, verb, parent) + return addToolboxSkills(ctx, client, toolboxName, specs, verb, parent) } // runSkillAddWith is the testable core. @@ -125,6 +134,13 @@ func runSkillAddWith( return err } + return addToolboxSkills(ctx, client, toolboxName, specs, verb, parent) +} + +func addToolboxSkills( + ctx context.Context, client toolboxClient, toolboxName string, + specs []skillSpec, verb skillAddFlags, parent toolboxFlags, +) error { tb, err := client.GetToolbox(ctx, toolboxName) if err != nil { return toolboxNotFoundOrService(err, toolboxName, exterrors.OpGetToolbox) @@ -202,10 +218,17 @@ func collectSkillSpecs(rawSkill string, verb skillAddFlags) ([]skillSpec, error) ) } specs := make([]skillSpec, 0, len(input.Skills)) + seen := map[string]bool{} for _, s := range input.Skills { if err := validateSkillName(s.Name); err != nil { return nil, err } + name := strings.TrimSpace(s.Name) + if seen[name] { + return nil, exterrors.Validation(exterrors.CodeDuplicateSkill, + "skill is duplicated in the input", "remove duplicate skills[] entries from the input file") + } + seen[name] = true specs = append(specs, skillSpec{ Name: strings.TrimSpace(s.Name), Version: strings.TrimSpace(s.Version), diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_remove.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_remove.go index b34bb355b44..8a16be1ae32 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_remove.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_remove.go @@ -6,6 +6,7 @@ package cmd import ( "context" "fmt" + "io" "slices" "strings" @@ -44,8 +45,12 @@ Removing the last skill is allowed.`, `, Args: cobra.MinimumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { + if err := validateToolboxWriteFlags(cmd); err != nil { + return err + } return runSkillRemove( cmd.Context(), args[0], args[1:], *flags, readToolboxFlags(cmd, extCtx), + cmd.ErrOrStderr(), ) }, } @@ -61,6 +66,7 @@ Removing the last skill is allowed.`, func runSkillRemove( ctx context.Context, toolboxName string, skillNames []string, verb skillRemoveFlags, parent toolboxFlags, + writer io.Writer, ) error { if err := validateToolboxName(toolboxName); err != nil { return err @@ -88,7 +94,7 @@ func runSkillRemove( ) } - client, resolved, err := resolveToolboxAndClient(ctx, parent) + client, resolved, err := resolveToolboxWriteAndClient(ctx, parent, toolboxName, writer) if err != nil { return err } diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_verbs_test.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_verbs_test.go index f307c0111b7..c31102f54b5 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_verbs_test.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_skill_verbs_test.go @@ -4,6 +4,7 @@ package cmd import ( + "io" "os" "testing" @@ -237,6 +238,7 @@ func TestRunSkillRemove_NoPromptWithoutForce(t *testing.T) { t.Context(), "tb", []string{"any-skill"}, skillRemoveFlags{force: false}, toolboxFlags{output: "table", noPrompt: true}, + io.Discard, ) requireLocalError(t, err, exterrors.CodeMissingForceFlag) } diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/foundry/credential.go b/cli/azd/extensions/azure.ai.toolboxes/internal/foundry/credential.go deleted file mode 100644 index d4217f62246..00000000000 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/foundry/credential.go +++ /dev/null @@ -1,32 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -// Package foundry holds Foundry data-plane primitives that this extension -// duplicates from azure.ai.agents. The shared package is decoupled from -// toolbox-specific concepts so that a future lift-out into a shared module -// is mechanical. See cli/azd/docs/design/azure-ai-toolbox-direct-commands.md -// § 3 for the duplication contract. -package foundry - -import ( - "fmt" - - "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" -) - -// NewCredential returns the credential used by Foundry data-plane clients. -// -// This is the toolboxes-extension copy of azure.ai.agents' newAgentCredential. -func NewCredential(tenantID string) (azcore.TokenCredential, error) { - options := &azidentity.AzureDeveloperCLICredentialOptions{} - if tenantID != "" { - options.TenantID = tenantID - options.AdditionallyAllowedTenants = []string{"*"} - } - cred, err := azidentity.NewAzureDeveloperCLICredential(options) - if err != nil { - return nil, fmt.Errorf("failed to create Azure credential: %w", err) - } - return cred, nil -} diff --git a/docs/architecture/extension-framework.md b/docs/architecture/extension-framework.md index 9be0e41e746..b424c61231d 100644 --- a/docs/architecture/extension-framework.md +++ b/docs/architecture/extension-framework.md @@ -72,6 +72,28 @@ is separate from the [gRPC service and invocation store](../../cli/azd/internal/ See the [SDK reference](../../cli/azd/docs/extensions/extension-sdk-reference.md#project-lifecycle-follow-up) for the API, host compatibility, and contribution ordering rules. +## Shared Foundry access preparation + +The Go [`pkg/foundry`](../../cli/azd/pkg/foundry/access.go) package owns the +common access policy used by agent predeploy and toolbox write operations: +probe effective data-plane access, resolve the resource-tenant principal, +check inherited built-in roles, conditionally assign project-scoped Foundry +User, and wait for propagation. Successful probes never require role-assignment +writes. Optional ARM failures produce warnings; cancellation and exhausted +post-grant readiness checks stop the operation. + +Core also shares active-environment loading, endpoint/scope consistency checks, +skip-setting precedence, user-access tenant lookup, and credential construction. +Extensions own endpoint selection, non-mutating service-specific probes, and +warning/error rendering. The ARM role client is created only after a probe +confirms missing access. Provisioning, read/delete commands, and read-only +diagnostics do not invoke automatic grants. +Hosted-agent role-assignment-write and ACR checks remain agent-specific. + +New shared APIs must ship in a core SDK release before independently published +extension modules can consume them. Validate cross-module development with a +local Go workspace or temporary replacements; do not merge local replacements. + ## Capabilities Extensions declare their capabilities in `extension.yaml`: diff --git a/docs/guides/creating-an-extension.md b/docs/guides/creating-an-extension.md index 9e809830f0c..14b13f393c0 100644 --- a/docs/guides/creating-an-extension.md +++ b/docs/guides/creating-an-extension.md @@ -53,6 +53,21 @@ If your Go extension creates role assignments, use the stable [`Account().GetCur Go extensions that resolve local Foundry configuration `$ref` files should use [`foundry.ResolveFileRefs`](../../cli/azd/pkg/foundry/includes.go). Each referenced file must contain exactly one YAML or JSON object; additional documents, trailing content, arrays, and scalars are rejected. Resolution retains YAML value types, aliases, and sibling overlays. Pass only the selected configuration when unrelated references should remain unopened. Extensions must consume an SDK release containing this validation before their binaries enforce it. +For Foundry developer access, reuse +[`foundry.AccessPreparation`](../../cli/azd/pkg/foundry/access.go) rather than +duplicating RBAC policy. Validate the selected endpoint against its ARM project +ID with `foundry.ProjectAccessScope` (or `ProjectScopeFromEnvironment` when +using an active environment). Reuse `LoadProjectEnvironment`, +`RoleAssignmentsSkipped`, `LookupProjectTenant`, and `NewCredential` for +common setup, then supply the credential, a non-mutating +data-plane `Probe`, an injected `Warn` callback, and optionally `Progress` for +grant attempts, propagation waits, and confirmed readiness. Keep both callbacks +on the command's injected writer. Call `Ensure` only before +operations intended to prepare access, never from read/delete or diagnostic +paths. It skips grants when effective access already works and waits after a +new grant. Consume a core SDK release containing these APIs before publishing +the extension; use a local Go workspace for cross-module development. + The default scaffolds and SDK `EventManager` use the stable `v1` contract. Go extensions can opt into beta structured messages for service `predeploy`/`postdeploy` handlers with From 50823ce3da433e1572291cbdeb40b140efb78ab7 Mon Sep 17 00:00:00 2001 From: hund030 Date: Wed, 28 Oct 2026 16:02:07 +0800 Subject: [PATCH 2/2] fix(agents): emit project access confirmation in RBAC pre-flight check --- .../internal/project/developer_rbac_check.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check.go b/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check.go index f38dd1c5027..47c82215816 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/developer_rbac_check.go @@ -145,6 +145,7 @@ func CheckDeveloperRBAC(ctx context.Context, azdClient *azdext.AzdClient, hasHos fmt.Println() fmt.Println("Developer RBAC pre-flight check") + accessWarned := false agentClient := agent_api.NewAgentClient(strings.TrimRight(endpoint, "/"), cred) if err := (foundry.AccessPreparation{ Account: azdClient.Account(), Credential: cred, Scope: info.ProjectScope, @@ -153,7 +154,10 @@ func CheckDeveloperRBAC(ctx context.Context, azdClient *azdext.AzdClient, hasHos agent_api.AgentEndpointAPIVersion) return err }, - Warn: warn, + Warn: func(message string) { + accessWarned = true + warn(message) + }, Progress: func(message string) { fmt.Println(" " + message) }, @@ -168,7 +172,11 @@ func CheckDeveloperRBAC(ctx context.Context, azdClient *azdext.AzdClient, hasHos } return serviceErr } + if !accessWarned { + fmt.Println(" ✓ Foundry User on Foundry Project") + } if !hasHosted { + fmt.Println() return nil } principalID, principalType, err := foundry.ResolvePrincipal(ctx, azdClient.Account(), info.SubscriptionID)