diff --git a/cli/azd/extensions/azure.ai.agents/README.md b/cli/azd/extensions/azure.ai.agents/README.md index e7763ee1a63..47a8fade202 100644 --- a/cli/azd/extensions/azure.ai.agents/README.md +++ b/cli/azd/extensions/azure.ai.agents/README.md @@ -261,6 +261,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..d8e8b93b4af 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,91 @@ 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, + }).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 +210,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 +222,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 +240,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 +290,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 +309,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 +361,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 ffe8e6f93ee..518e34f2779 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/README.md +++ b/cli/azd/extensions/azure.ai.toolboxes/README.md @@ -67,6 +67,63 @@ 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 + +Toolbox operations require Foundry data-plane access. Subscription **Owner** or +**Contributor** alone does not grant that access; **Foundry User** or an +equivalent role is required. + +Before deploying a managed toolbox, or running `create`, `connection add/remove`, +`skill add/remove`, or `publish`, the extension checks access independently of +any agent predeploy hook. It validates local inputs first, then probes toolbox +read access before resolving remote connections or writing a toolbox version. +If the probe succeeds, no role assignment is attempted, including when access +comes from a custom or externally managed role. + +Automatic grants are limited to a verified active-environment project: +`AZURE_AI_PROJECT_ID` must identify the project selected by +`FOUNDRY_PROJECT_ENDPOINT` (or its `AZURE_AI_PROJECT_ENDPOINT` fallback), and the +selected operation endpoint must match. If `AZURE_SUBSCRIPTION_ID` is set, it +must agree with the resource ID. An explicit `--project-endpoint` matching that +project participates in the check; an unrelated endpoint does not borrow its +resource ID or tenant. Endpoint-only/global-context usage without matching +project metadata continues without automatic grants. + +If the probe confirms an authorization denial, the extension checks inherited +and equivalent roles, then attempts a project-scoped **Foundry User** assignment +only when no sufficient assignment exists. Credentials use the subscription's +user-access tenant, and the principal is resolved in its resource tenant, for +both guest users and service-principal logins. A new assignment is followed by +bounded readiness checks to allow role propagation. If propagation takes longer, +wait and retry the command. + +When role inspection or assignment is unavailable or denied, the extension warns +and lets the requested toolbox operation determine the final result. If it still +returns 403, ask a subscription Owner or User Access Administrator to grant +Foundry User on the target project. The assignment-denied warning includes an +`az role assignment create` command for that scope. + +To disable automatic developer role checks and grants for toolbox writes, set +`AZD_AGENT_SKIP_ROLE_ASSIGNMENTS=true` in the active azd environment or process +environment. A non-empty azd environment value takes precedence; an absent or +empty value falls back to the process environment. The value is parsed as a +boolean; invalid values do not enable skipping, and the default is not to skip. +This setting does not grant access: externally managed permissions must already +allow the requested operation. + +Read/list/show commands, toolbox deletion, local `add connection`/`add skill` +authoring, and publication of a bring-your-own toolbox endpoint do not grant +roles. Provisioning remains independent of developer role-assignment permission. +Access-check warnings go to stderr, preserving JSON results on stdout. + +Developer access preparation is implemented by core's shared +`pkg/foundry.AccessPreparation`; this extension retains endpoint selection, +toolbox probes, and output/error integration. Environment access, scope checks, +skip precedence, and tenant credentials also use shared core helpers. The agents +extension uses the same role and propagation policy. Release the core helper +before updating the independently published extensions' azd SDK dependencies; +use a local Go workspace to validate both sides before that release. + ## 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..b21e29c960b 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/go.mod +++ b/cli/azd/extensions/azure.ai.toolboxes/go.mod @@ -4,7 +4,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 @@ -18,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/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..1259a26d29e 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/context.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/context.go @@ -10,16 +10,17 @@ import ( "fmt" "log" "net/http" + "os" "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", "from-version"} { + 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,31 @@ func resolveToolboxAndClient( return client, resolved, nil } +// resolveToolboxWriteAndClient is deliberately separate from read/delete resolution. +func resolveToolboxWriteAndClient( + ctx context.Context, flags toolboxFlags, name string, +) (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(os.Stderr, "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) + 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..994ef11ac58 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,12 @@ 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. + +Create/update commands check Foundry access and may assign Foundry User when +the target matches the active environment's project resource ID and endpoint. +Read and delete 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..1a396c97f8d 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,7 +194,19 @@ 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 } @@ -206,9 +219,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 +349,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 +473,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 +507,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-root.snap b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/testdata/TestAIHelp-root.snap index 7c258ffd46f..a2e968204d4 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,11 @@ 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. +Create/update commands check Foundry access and may assign Foundry User when +the target matches the active environment's project resource ID and endpoint. +Read and delete 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..f44ddaee2ee --- /dev/null +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access.go @@ -0,0 +1,140 @@ +// 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 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 isAzureNotFound(err) { + return nil + } + return err + }, + Warn: a.warn, + } + 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 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..5cdf51fa3a1 --- /dev/null +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_access_test.go @@ -0,0 +1,479 @@ +// 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" +) + +func toolboxAccessTestValues() map[string]string { + return map[string]string{ + "AZURE_AI_PROJECT_ID": toolboxAccessTestID, "FOUNDRY_PROJECT_ENDPOINT": toolboxAccessTestEndpoint, + "AZURE_SUBSCRIPTION_ID": "sub", + } +} + +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 +} + +func (c *accessToolboxClient) GetToolbox(ctx context.Context, name string) (*azure.ToolboxObject, error) { + *c.events = append(*c.events, "read") + 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: toolboxAccessTestValues(), + }, + 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 TestToolboxWritePreparesAccessBeforeCreate(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) + 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) + resolver := accessConnectionResolver{events: &events, roles: client.roles} + err = createToolboxFromDefinition(t.Context(), prepared, resolver, 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) + }) + } +} + +func TestToolboxDeployPreparesAccessWithoutAgentHook(t *testing.T) { + stubToolboxEndpointEnv(t) + 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 }) + 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": "mcp"}}, + }) + 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) + assert.Equal(t, []string{"read", "roles", "grant", "read", "connection", "create"}, events) +} + +func TestToolboxWriteAccessDoesNotGrantForUnscopedOrSkippedOperations(t *testing.T) { + for _, mode := range []string{"unrelated endpoint", "missing scope", "skip", "managed access"} { + t.Run(mode, func(t *testing.T) { + var events []string + access, client, _ := accessTestSetup(t, &events, azdext.PrincipalType_PRINCIPAL_TYPE_USER) + environment := access.environment.(*accessEnvironment) + 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 + } + _, 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") + }) + } +} + +func TestToolboxAccessWarningsDoNotDiscloseCredentials(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{"password", "sig=", "secret", "fragment"} { + assert.NotContains(t, diagnostics.String(), secret) + } + _, err = client.GetToolbox(t.Context(), "research") + structured := exterrors.ServiceFromAzure(err, exterrors.OpGetToolbox) + var serviceErr *azdext.ServiceError + require.ErrorAs(t, structured, &serviceErr) + assert.Equal(t, http.StatusForbidden, serviceErr.StatusCode) + assert.Contains(t, serviceErr.Suggestion, "Foundry User") +} + +func TestToolboxWriteValidatesLocalInputBeforeAccess(t *testing.T) { + t.Setenv("AZD_SERVER", "") + t.Setenv("FOUNDRY_PROJECT_ENDPOINT", "") + t.Setenv("AZURE_AI_PROJECT_ENDPOINT", "") + for _, run := range []struct { + name string + run func() error + }{ + {name: "create missing file", run: func() error { + return runToolboxCreate(t.Context(), "research", toolboxCreateFlags{fromFile: "nonexistent.yaml"}, + toolboxFlags{}, nil) + }}, + {name: "create invalid tools", run: func() error { + file := writeTempFile(t, ".yaml", "tools:\n - name: missing-type\n") + return runToolboxCreate(t.Context(), "research", toolboxCreateFlags{fromFile: file}, toolboxFlags{}, nil) + }}, + {name: "connection file conflict", run: func() error { + return runConnectionAdd(t.Context(), "research", "mcp", connectionAddFlags{fromFile: "file.yaml"}, + toolboxFlags{}, nil) + }}, + {name: "connection missing file", run: func() error { + return runConnectionAdd(t.Context(), "research", "", connectionAddFlags{fromFile: "nonexistent.yaml"}, + toolboxFlags{}, nil) + }}, + {name: "connection conflicting options", run: func() error { + return runConnectionAdd(t.Context(), "research", "connection", + connectionAddFlags{index: "index", instanceName: "instance"}, toolboxFlags{}, nil) + }}, + {name: "skill invalid spec", run: func() error { + return runSkillAdd(t.Context(), "research", "skill@", skillAddFlags{}, toolboxFlags{}) + }}, + {name: "skill invalid file", run: func() error { + file := writeTempFile(t, ".yaml", "skills:\n - name: INVALID\n") + return runSkillAdd(t.Context(), "research", "", skillAddFlags{fromFile: file}, toolboxFlags{}) + }}, + } { + t.Run(run.name, func(t *testing.T) { + err := run.run() + require.Error(t, err) + assert.NotContains(t, err.Error(), "endpoint") + assert.NotContains(t, err.Error(), "credential") + }) + } +} + +func TestToolboxWriteRejectsExplicitEmptyIgnoredFlags(t *testing.T) { + 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() + 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 TestToolboxWriteCommandsReachAccessPreparation(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() + _ = listener.Close() + }) + t.Setenv("AZD_SERVER", listener.Addr().String()) + file := writeTempFile(t, ".yaml", "tools:\n - type: web_search\n") + 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 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{}, // Any attempt to resolve scope or credentials would fail. + 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) { + old := projectctx.ReadAzdHostedSourcesFunc + projectctx.ReadAzdHostedSourcesFunc = func(context.Context) (projectctx.AzdHostedSources, error) { + return projectctx.AzdHostedSources{EnvValue: toolboxAccessTestEndpoint}, nil + } + t.Cleanup(func() { projectctx.ReadAzdHostedSourcesFunc = old }) + 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 TestToolboxAccessOptionalFailures(t *testing.T) { + for _, failure := range []string{"environment", "credential", "tenant", "empty tenant", "cancelled"} { + 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": + access.newCredential = func(string) (azcore.TokenCredential, error) { + return nil, errors.New("credential unavailable") + } + case "tenant": + account.err = errors.New("tenant unavailable") + case "empty tenant": + account.tenantID = "" + case "cancelled": + access.environment.(*accessEnvironment).err = context.Canceled + } + _, err := access.writeClient(t.Context(), toolboxAccessTestEndpoint, "research", + func(string, string) (toolboxClient, error) { return client, nil }) + switch failure { + case "tenant", "empty tenant": + requireLocalError(t, err, exterrors.CodeTenantLookupFailed) + case "cancelled": + require.ErrorIs(t, err, context.Canceled) + 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_connection_add.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_connection_add.go index 1b6d522cce1..3d3f72590c4 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 @@ -9,6 +9,7 @@ import ( "slices" "strings" + "azure.ai.toolboxes/internal/definition" "azure.ai.toolboxes/internal/exterrors" "azure.ai.toolboxes/internal/pkg/azure" @@ -22,6 +23,8 @@ type connectionAddFlags struct { instanceName string fromFile string fromVersion string + indexSet bool + instanceSet bool } // newToolboxConnectionAddCommand returns the `connection add` command. @@ -84,6 +87,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] @@ -125,21 +133,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) 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 +155,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 +224,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..d93e4d7a54e 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 @@ -44,6 +44,9 @@ 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, @@ -96,7 +99,7 @@ func runConnectionRemove( ) } - client, resolved, err := resolveToolboxAndClient(ctx, parent) + client, resolved, err := resolveToolboxWriteAndClient(ctx, parent, toolboxName) if err != nil { return err } 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..bd4dd946692 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 @@ -11,7 +11,6 @@ import ( "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" @@ -51,6 +50,9 @@ 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{}, @@ -81,18 +83,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) 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 +105,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 +128,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 +147,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 +231,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/cmd/toolbox_publish.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/toolbox_publish.go index 57fe93128f2..1959d5453ca 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 @@ -30,6 +30,9 @@ 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 { + if err := validateToolboxWriteFlags(cmd); err != nil { + return err + } return runToolboxPublish(cmd.Context(), args[0], args[1], readToolboxFlags(cmd, extCtx)) }, } @@ -57,7 +60,7 @@ func runToolboxPublish( ) } - client, resolved, err := resolveToolboxAndClient(ctx, parent) + client, resolved, err := resolveToolboxWriteAndClient(ctx, parent, name) 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..1840e87127e 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 @@ -62,6 +62,9 @@ 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] @@ -105,13 +108,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) 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 +132,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 +216,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..76a3470c7f2 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 @@ -44,6 +44,9 @@ 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), ) @@ -88,7 +91,7 @@ func runSkillRemove( ) } - client, resolved, err := resolveToolboxAndClient(ctx, parent) + client, resolved, err := resolveToolboxWriteAndClient(ctx, parent, toolboxName) if err != nil { return err } 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)) diff --git a/cli/azd/pkg/foundry/access.go b/cli/azd/pkg/foundry/access.go new file mode 100644 index 00000000000..3ed463d3862 --- /dev/null +++ b/cli/azd/pkg/foundry/access.go @@ -0,0 +1,419 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +// cspell:ignore permissiondenied authorizationfailed authorizationpermissionmismatch aiservices managementgroups +package foundry + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/url" + "slices" + "strconv" + "strings" + "time" + + "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/resourcemanager/authorization/armauthorization/v2" + "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/google/uuid" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// FoundryUserRole is the built-in Foundry User role definition. +const FoundryUserRole = "53ca6127-db72-4b80-b1b0-d745d6d5456d" + +// CognitiveServicesUserRole grants equivalent Foundry data-plane access for legacy assignments. +const CognitiveServicesUserRole = "a97b65f3-24c7-4388-baec-2e87135dc908" + +// FoundryProjectManagerRole grants project management and Foundry data-plane access. +const FoundryProjectManagerRole = "eadc314b-1a2d-4efa-be10-5d325db5065e" + +// FoundryOwnerRole grants Foundry management and data-plane access. +const FoundryOwnerRole = "c883944f-8b7b-4483-af10-35834be79c4a" + +var projectAccessRoles = []string{ + FoundryUserRole, + CognitiveServicesUserRole, + FoundryProjectManagerRole, + FoundryOwnerRole, +} + +// ProjectAccessRoles returns the built-in roles sufficient for Foundry agent data-plane access. +// Azure Owner and Contributor are intentionally excluded: neither grants data actions. +func ProjectAccessRoles() []string { + return slices.Clone(projectAccessRoles) +} + +// ProjectAccessScope validates that an ARM project ID identifies the endpoint being probed. +// Errors deliberately omit the endpoint, which may contain authored credentials. +func ProjectAccessScope(projectID, endpoint string) (*arm.ResourceID, error) { + id, err := arm.ParseResourceID(strings.TrimSpace(projectID)) + if err != nil || !strings.EqualFold(id.ResourceType.String(), "Microsoft.CognitiveServices/accounts/projects") || + id.SubscriptionID == "" || id.ResourceGroupName == "" || id.Name == "" || id.Parent == nil || + id.Parent.Name == "" || len(strings.Split(id.String(), "/")) != 11 { + return nil, fmt.Errorf("project ID is not a valid Foundry project resource ID") + } + u, err := url.Parse(strings.TrimRight(endpoint, "/")) + if err != nil || !strings.EqualFold(u.Scheme, "https") || u.User != nil || + u.RawQuery != "" || u.Fragment != "" || u.Port() != "" || + !strings.EqualFold(u.Hostname(), id.Parent.Name+".services.ai.azure.com") || + !strings.EqualFold(u.Path, "/api/projects/"+id.Name) { + return nil, fmt.Errorf("project ID does not identify the active Foundry project endpoint") + } + return id, nil +} + +// ProjectScopeFromEnvironment returns a scope only for the selected environment's endpoint. +// An unrelated explicit endpoint never borrows the active environment's project or tenant. +func ProjectScopeFromEnvironment(endpoint string, values map[string]string) (*arm.ResourceID, error) { + configured := values["FOUNDRY_PROJECT_ENDPOINT"] + if configured == "" { + configured = values["AZURE_AI_PROJECT_ENDPOINT"] + } + if configured == "" || strings.TrimSpace(values["AZURE_AI_PROJECT_ID"]) == "" { + return nil, nil + } + normalized := projectEndpointIdentity(configured) + if normalized == "" || normalized != projectEndpointIdentity(endpoint) { + return nil, nil + } + resource, err := ProjectAccessScope(values["AZURE_AI_PROJECT_ID"], normalized) + if err != nil { + return nil, fmt.Errorf("AZURE_AI_PROJECT_ID: %w", err) + } + if subscription := strings.TrimSpace(values["AZURE_SUBSCRIPTION_ID"]); subscription != "" && + !strings.EqualFold(subscription, resource.SubscriptionID) { + return nil, fmt.Errorf("AZURE_SUBSCRIPTION_ID conflicts with AZURE_AI_PROJECT_ID") + } + return resource, nil +} + +func projectEndpointIdentity(endpoint string) string { + u, err := url.Parse(strings.TrimSpace(endpoint)) + if err != nil || !strings.EqualFold(u.Scheme, "https") || u.Port() != "" || + u.User != nil || u.RawQuery != "" || u.Fragment != "" || + !strings.HasSuffix(strings.ToLower(u.Hostname()), ".services.ai.azure.com") { + return "" + } + return "https://" + strings.ToLower(u.Hostname()) + strings.TrimRight(u.EscapedPath(), "/") +} + +// RoleAssignmentsSkipped applies the shared environment-over-process boolean precedence. +func RoleAssignmentsSkipped(environmentValue, processValue string) bool { + if environmentValue == "" { + environmentValue = processValue + } + skip, _ := strconv.ParseBool(environmentValue) + return skip +} + +// ProjectEnvironment reads the selected azd environment without spawning another CLI process. +type ProjectEnvironment interface { + GetValues(context.Context, *azdext.GetEnvironmentRequest, ...grpc.CallOption) (*azdext.KeyValueListResponse, error) +} + +// LoadProjectEnvironment reads the active environment and its values through the azd host. +func LoadProjectEnvironment(ctx context.Context, environment ProjectEnvironment) (map[string]string, error) { + // An empty name asks the host for the active environment. + response, err := environment.GetValues(ctx, &azdext.GetEnvironmentRequest{}) + if err != nil { + return nil, fmt.Errorf("read the active azd environment: %w", err) + } + values := make(map[string]string, len(response.GetKeyValues())) + for _, entry := range response.GetKeyValues() { + values[entry.GetKey()] = entry.GetValue() + } + return values, nil +} + +// TenantResolver resolves the user-access tenant for a subscription. +type TenantResolver interface { + LookupTenant(context.Context, *azdext.LookupTenantRequest, ...grpc.CallOption) (*azdext.LookupTenantResponse, error) +} + +// LookupProjectTenant resolves a non-empty user-access tenant, including for guest users. +func LookupProjectTenant(ctx context.Context, account TenantResolver, subscriptionID string) (string, error) { + response, err := account.LookupTenant(ctx, &azdext.LookupTenantRequest{SubscriptionId: subscriptionID}) + if err != nil { + return "", err + } + tenantID := strings.TrimSpace(response.GetTenantId()) + if tenantID == "" { + return "", fmt.Errorf("azd returned an empty user-access tenant") + } + return tenantID, nil +} + +// PrincipalResolver resolves the current identity in the project's resource tenant. +type PrincipalResolver interface { + GetCurrentPrincipal(context.Context, *azdext.GetCurrentPrincipalRequest, ...grpc.CallOption) ( + *azdext.GetCurrentPrincipalResponse, error) +} + +// ProjectRoles checks inherited access and assigns a project-scoped role. +type ProjectRoles interface { + HasAccess(context.Context, string, string) (bool, error) + Assign(context.Context, string, string, armauthorization.PrincipalType) error +} + +type projectRoles struct { + client *armauthorization.RoleAssignmentsClient +} + +func newProjectRoles(subscriptionID string, credential azcore.TokenCredential) (ProjectRoles, error) { + client, err := armauthorization.NewRoleAssignmentsClient(subscriptionID, credential, nil) + if err != nil { + return nil, err + } + return &projectRoles{client: client}, nil +} + +func (r *projectRoles) HasAccess(ctx context.Context, scope, principalID string) (bool, error) { + return r.hasRoles(ctx, scope, principalID, projectAccessRoles) +} + +// HasAnyRoleAssignment checks the specified roles at the target scope, including inherited assignments. +func HasAnyRoleAssignment( + ctx context.Context, credential azcore.TokenCredential, principalID string, roleIDs []string, scope string, +) (bool, error) { + resource, err := arm.ParseResourceID(scope) + if err != nil || resource.SubscriptionID == "" { + return false, fmt.Errorf("a subscription-scoped resource ID is required for role inspection") + } + client, err := armauthorization.NewRoleAssignmentsClient(resource.SubscriptionID, credential, nil) + if err != nil { + return false, err + } + return (&projectRoles{client: client}).hasRoles(ctx, scope, principalID, roleIDs) +} + +func (r *projectRoles) hasRoles(ctx context.Context, scope, principalID string, roleIDs []string) (bool, error) { + pager := r.client.NewListForScopePager(scope, &armauthorization.RoleAssignmentsClientListForScopeOptions{ + Filter: new(fmt.Sprintf("assignedTo('%s')", principalID)), + }) + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + return false, err + } + for _, assignment := range page.Value { + if assignment.Properties == nil || assignment.Properties.RoleDefinitionID == nil || + assignment.Properties.Scope == nil { + continue + } + assignmentScope := strings.ToLower(strings.TrimRight(*assignment.Properties.Scope, "/")) + targetScope := strings.ToLower(strings.TrimRight(scope, "/")) + // ARM can also return inherited management-group roles, whose IDs are not path ancestors. + const managementGroupPrefix = "/providers/microsoft.management/managementgroups/" + isManagementGroup := strings.HasPrefix(assignmentScope, managementGroupPrefix) && + len(strings.Split(strings.TrimPrefix(assignmentScope, managementGroupPrefix), "/")) == 1 + if assignmentScope == "" || (assignmentScope != targetScope && + !strings.HasPrefix(targetScope, assignmentScope+"/") && !isManagementGroup) { + continue + } + for _, role := range roleIDs { + if strings.EqualFold(roleDefinitionSuffix(*assignment.Properties.RoleDefinitionID), role) { + return true, nil + } + } + } + } + return false, nil +} + +func roleDefinitionSuffix(id string) string { + return id[strings.LastIndex(id, "/")+1:] +} + +func (r *projectRoles) Assign( + ctx context.Context, scope, principalID string, principalType armauthorization.PrincipalType, +) error { + // A stable name makes simultaneous preparations for the same project/principal idempotent. + name := uuid.NewSHA1(uuid.NameSpaceURL, []byte(strings.ToLower(scope+"/"+principalID+"/"+FoundryUserRole))).String() + resource, err := arm.ParseResourceID(scope) + if err != nil || resource.SubscriptionID == "" { + return fmt.Errorf("a subscription-scoped project resource ID is required for role assignment") + } + subscriptionID := resource.SubscriptionID + _, err = r.client.Create(ctx, scope, name, armauthorization.RoleAssignmentCreateParameters{ + Properties: &armauthorization.RoleAssignmentProperties{ + RoleDefinitionID: new("/subscriptions/" + subscriptionID + + "/providers/Microsoft.Authorization/roleDefinitions/" + FoundryUserRole), + PrincipalID: new(principalID), + PrincipalType: new(principalType), + }, + }, nil) + if response, ok := errors.AsType[*azcore.ResponseError](err); ok && + response.StatusCode == http.StatusConflict && response.ErrorCode == "RoleAssignmentExists" { + return nil + } + return err +} + +// AccessPreparation prepares access only after a data-plane probe establishes an authorization denial. +// Probe must be a non-mutating check of the operation's required Foundry read access. +type AccessPreparation struct { + Account PrincipalResolver + Credential azcore.TokenCredential + Scope string + Probe func(context.Context) error + Warn func(string) + // Roles optionally replaces the ARM client; by default it is created only after a denied probe. + Roles ProjectRoles + // RetryDelay is the interval for a newly granted role to reach the data plane. + RetryDelay time.Duration + Attempts int +} + +// ResolvePrincipal resolves the resource-tenant object ID and ARM principal type through the azd host. +func ResolvePrincipal( + ctx context.Context, account PrincipalResolver, subscriptionID string, +) (string, armauthorization.PrincipalType, error) { + 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()) + } +} + +// IsProjectAccessDenied distinguishes Foundry authorization errors from unrelated HTTP 403 responses. +func IsProjectAccessDenied(err error) bool { + response, ok := errors.AsType[*azcore.ResponseError](err) + if !ok || response.StatusCode != http.StatusForbidden { + return false + } + switch strings.ToLower(response.ErrorCode) { + case "permissiondenied", "authorizationfailed", "authorizationpermissionmismatch": + return true + default: + return strings.Contains(strings.ToLower(response.Error()), "microsoft.cognitiveservices/accounts/aiservices/agents/") + } +} + +// Ensure checks existing access, conditionally grants Foundry User, and waits for propagation. +// Optional ARM failures are warnings; the caller's actual operation remains authoritative. +func (p AccessPreparation) Ensure(ctx context.Context) error { + if err := ctx.Err(); err != nil { + return err + } + probeErr := p.Probe(ctx) + if err := AccessCancellation(ctx, probeErr); err != nil { + return err + } + if probeErr == nil { + return nil + } + if !IsProjectAccessDenied(probeErr) { + p.Warn("Could not establish missing Foundry access; continuing without assigning a role.") + return nil + } + resource, err := arm.ParseResourceID(p.Scope) + if err != nil || resource.SubscriptionID == "" { + return fmt.Errorf("a subscription-scoped project resource ID is required for access preparation") + } + principalID, principalType, err := ResolvePrincipal(ctx, p.Account, resource.SubscriptionID) + if cancelErr := AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } + if err != nil { + p.Warn("Could not resolve the current principal; continuing without assigning a role.") + return nil + } + roles := p.Roles + if roles == nil { + roles, err = newProjectRoles(resource.SubscriptionID, p.Credential) + if err != nil { + p.Warn("Could not create the Foundry role-assignment client; continuing without assigning a role.") + return nil + } + } + hasAccess, err := roles.HasAccess(ctx, p.Scope, principalID) + if cancelErr := AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } + if err != nil { + p.Warn("Could not inspect inherited Foundry roles; continuing without assigning a role.") + return nil + } + if hasAccess { + p.Warn("A sufficient Foundry role is already assigned; " + + "if access is still denied, verify role propagation or policy.") + return nil + } + if err := roles.Assign(ctx, p.Scope, principalID, principalType); err != nil { + if cancelErr := AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } + p.Warn(fmt.Sprintf( + "Could not assign Foundry User. Ask an administrator to run: "+ + "az role assignment create --assignee %q --role %s --scope %q. Continuing.", + principalID, FoundryUserRole, p.Scope)) + return nil + } + attempts := p.Attempts + if attempts <= 0 { + attempts = 12 + } + delay := p.RetryDelay + if delay <= 0 { + delay = 5 * time.Second + } + for attempt := range attempts { + err := p.Probe(ctx) + if cancelErr := AccessCancellation(ctx, err); cancelErr != nil { + return cancelErr + } + if err == nil { + return nil + } + if !IsProjectAccessDenied(err) { + p.Warn("Foundry access readiness could not be confirmed; continuing with the operation.") + return nil + } + if attempt == attempts-1 { + return fmt.Errorf("Foundry User was assigned but access has not propagated; wait and retry: %w", err) + } + timer := time.NewTimer(delay) + select { + case <-ctx.Done(): + timer.Stop() + return ctx.Err() + case <-timer.C: + } + } + return nil +} + +// AccessCancellation preserves local and transported cancellation/deadline errors during access preparation. +func AccessCancellation(ctx context.Context, err error) error { + if ctx.Err() != nil { + return ctx.Err() + } + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return err + } + switch status.Code(err) { + case codes.Canceled, codes.DeadlineExceeded: + return err + } + return nil +} diff --git a/cli/azd/pkg/foundry/access_test.go b/cli/azd/pkg/foundry/access_test.go new file mode 100644 index 00000000000..6cc26dd9237 --- /dev/null +++ b/cli/azd/pkg/foundry/access_test.go @@ -0,0 +1,562 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package foundry + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "strings" + "testing" + "time" + + "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/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/grpc/codes" + "google.golang.org/grpc/status" +) + +const accessTestScope = "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.CognitiveServices/" + + "accounts/acct/projects/proj" + +func TestProjectAccessScope(t *testing.T) { + const endpoint = "https://acct.services.ai.azure.com/api/projects/proj" + for _, tt := range []struct { + name string + id string + endpoint string + valid bool + }{ + {name: "matching project", id: accessTestScope, endpoint: endpoint, valid: true}, + {name: "trailing slash", id: accessTestScope, endpoint: endpoint + "/", valid: true}, + {name: "case insensitive ARM ID", id: strings.ToUpper(accessTestScope), endpoint: endpoint, valid: true}, + {name: "invalid ID", id: "/subscriptions/sub", endpoint: endpoint}, + {name: "missing ID", endpoint: endpoint}, + {name: "child resource", id: accessTestScope + "/agents/agent", endpoint: endpoint}, + {name: "missing endpoint", id: accessTestScope}, + {name: "different account", id: accessTestScope, endpoint: strings.Replace(endpoint, "acct.", "other.", 1)}, + {name: "different project", id: accessTestScope, endpoint: endpoint + "-other"}, + {name: "different host", id: accessTestScope, endpoint: "https://unrelated.example/api/projects/proj"}, + {name: "insecure endpoint", id: accessTestScope, endpoint: strings.Replace(endpoint, "https:", "http:", 1)}, + {name: "credentials", id: accessTestScope, + endpoint: strings.Replace(endpoint, "https://", "https://user:secret@", 1)}, + {name: "query credentials", id: accessTestScope, endpoint: endpoint + "?sig=secret"}, + {name: "fragment credentials", id: accessTestScope, endpoint: endpoint + "#secret"}, + {name: "custom port", id: accessTestScope, endpoint: strings.Replace(endpoint, ".com/", ".com:8080/", 1)}, + {name: "malformed URL", id: accessTestScope, endpoint: endpoint + "%secret"}, + } { + t.Run(tt.name, func(t *testing.T) { + resource, err := ProjectAccessScope(tt.id, tt.endpoint) + if !tt.valid { + require.Error(t, err) + assert.Nil(t, resource) + assert.NotContains(t, err.Error(), "secret") + if tt.endpoint != "" { + assert.NotContains(t, err.Error(), tt.endpoint, "errors must not disclose authored URLs") + } + return + } + require.NoError(t, err) + assert.True(t, strings.EqualFold(accessTestScope, resource.String())) + }) + } +} + +func TestProjectAccessRolesAreIndependent(t *testing.T) { + roles := ProjectAccessRoles() + require.Len(t, roles, 4) + roles[0] = "changed" + assert.Equal(t, FoundryUserRole, ProjectAccessRoles()[0]) +} + +func TestProjectScopeFromEnvironment(t *testing.T) { + const endpoint = "https://acct.services.ai.azure.com/api/projects/proj" + for _, tt := range []struct { + name string + endpoint string + change func(map[string]string) + wantNil bool + wantErr string + }{ + {name: "active project"}, + {name: "legacy endpoint key", change: func(v map[string]string) { + v["AZURE_AI_PROJECT_ENDPOINT"] = v["FOUNDRY_PROJECT_ENDPOINT"] + delete(v, "FOUNDRY_PROJECT_ENDPOINT") + }}, + {name: "Foundry key wins", change: func(v map[string]string) { + v["AZURE_AI_PROJECT_ENDPOINT"] = "https://other.services.ai.azure.com/api/projects/proj" + }}, + {name: "unrelated explicit endpoint", + endpoint: "https://other.services.ai.azure.com/api/projects/proj", wantNil: true}, + {name: "explicit endpoint overrides invalid default", change: func(v map[string]string) { + v["FOUNDRY_PROJECT_ENDPOINT"] = "invalid-default" + }, wantNil: true}, + {name: "missing project ID", change: func(v map[string]string) { delete(v, "AZURE_AI_PROJECT_ID") }, wantNil: true}, + {name: "missing environment endpoint", change: func(v map[string]string) { + delete(v, "FOUNDRY_PROJECT_ENDPOINT") + }, wantNil: true}, + {name: "malformed project ID", change: func(v map[string]string) { + v["AZURE_AI_PROJECT_ID"] = "/subscriptions/sub" + }, wantErr: "not a valid Foundry project"}, + {name: "wrong resource type", change: func(v map[string]string) { + v["AZURE_AI_PROJECT_ID"] = strings.ReplaceAll(accessTestScope, "/projects/", "/connections/") + }, wantErr: "not a valid Foundry project"}, + {name: "contradictory subscription", change: func(v map[string]string) { + v["AZURE_SUBSCRIPTION_ID"] = "other" + }, wantErr: "conflicts"}, + {name: "contradictory project", change: func(v map[string]string) { + v["AZURE_AI_PROJECT_ID"] = accessTestScope + "-other" + }, wantErr: "does not identify"}, + {name: "contradictory account", change: func(v map[string]string) { + v["AZURE_AI_PROJECT_ID"] = strings.ReplaceAll(accessTestScope, "/acct/", "/other/") + }, wantErr: "does not identify"}, + {name: "case-insensitive ARM identity", change: func(v map[string]string) { + v["AZURE_AI_PROJECT_ID"] = strings.ToUpper(accessTestScope) + }}, + {name: "normalized endpoint", endpoint: " https://ACCT.services.ai.azure.com/api/projects/proj/ "}, + {name: "credential-bearing default", change: func(v map[string]string) { + v["FOUNDRY_PROJECT_ENDPOINT"] = "https://user:secret@acct.services.ai.azure.com/api/projects/proj?sig=secret" + }, wantNil: true}, + } { + t.Run(tt.name, func(t *testing.T) { + values := map[string]string{ + "AZURE_AI_PROJECT_ID": accessTestScope, "FOUNDRY_PROJECT_ENDPOINT": endpoint, "AZURE_SUBSCRIPTION_ID": "sub", + } + if tt.change != nil { + tt.change(values) + } + selected := tt.endpoint + if selected == "" { + selected = endpoint + } + scope, err := ProjectScopeFromEnvironment(selected, values) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + assert.Nil(t, scope) + } else { + require.NoError(t, err) + assert.Equal(t, tt.wantNil, scope == nil) + } + }) + } +} + +func TestRoleAssignmentsSkipped(t *testing.T) { + for _, tt := range []struct { + name, env, process string + want bool + }{ + {name: "default"}, + {name: "environment true", env: "true", want: true}, + {name: "process true", process: "true", want: true}, + {name: "environment false overrides process", env: "false", process: "true"}, + {name: "invalid is not enabled", env: "invalid", process: "true"}, + } { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, RoleAssignmentsSkipped(tt.env, tt.process)) + }) + } +} + +type accessTestEnvironment struct { + err error + request *azdext.GetEnvironmentRequest +} + +func (e *accessTestEnvironment) GetValues( + _ context.Context, req *azdext.GetEnvironmentRequest, _ ...grpc.CallOption, +) (*azdext.KeyValueListResponse, error) { + e.request = req + return &azdext.KeyValueListResponse{KeyValues: []*azdext.KeyValue{{Key: "SETTING", Value: "value"}}}, e.err +} + +func TestLoadProjectEnvironment(t *testing.T) { + for _, tt := range []struct { + name string + err error + }{ + {name: "active environment"}, + {name: "no active environment", err: errors.New("no active environment")}, + {name: "lookup failure", err: errors.New("unavailable")}, + {name: "cancellation", err: context.Canceled}, + } { + t.Run(tt.name, func(t *testing.T) { + environment := &accessTestEnvironment{err: tt.err} + values, err := LoadProjectEnvironment(t.Context(), environment) + require.NotNil(t, environment.request) + assert.Empty(t, environment.request.GetName(), "the host must resolve the active environment") + if tt.err != nil { + require.ErrorContains(t, err, "read the active azd environment") + assert.ErrorIs(t, err, tt.err) + assert.Nil(t, values) + } else { + require.NoError(t, err) + assert.Equal(t, map[string]string{"SETTING": "value"}, values) + } + }) + } +} + +type accessTestTenant struct { + tenantID string + err error + request *azdext.LookupTenantRequest +} + +func (a *accessTestTenant) LookupTenant( + _ context.Context, req *azdext.LookupTenantRequest, _ ...grpc.CallOption, +) (*azdext.LookupTenantResponse, error) { + a.request = req + return &azdext.LookupTenantResponse{TenantId: a.tenantID}, a.err +} + +func TestLookupProjectTenant(t *testing.T) { + for _, tt := range []struct { + name, tenant string + err error + wantErr string + }{ + {name: "user-access tenant", tenant: " user-access-tenant "}, + {name: "empty tenant", wantErr: "empty user-access tenant"}, + {name: "lookup failure", err: errors.New("unavailable"), wantErr: "unavailable"}, + {name: "cancellation", err: context.Canceled, wantErr: "canceled"}, + } { + t.Run(tt.name, func(t *testing.T) { + account := &accessTestTenant{tenantID: tt.tenant, err: tt.err} + tenantID, err := LookupProjectTenant(t.Context(), account, "selected-subscription") + require.NotNil(t, account.request) + assert.Equal(t, "selected-subscription", account.request.GetSubscriptionId()) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + assert.Empty(t, tenantID) + if tt.err != nil { + assert.ErrorIs(t, err, tt.err) + } + } else { + require.NoError(t, err) + assert.Equal(t, "user-access-tenant", tenantID) + } + }) + } +} + +type accessTestAccount struct { + response *azdext.GetCurrentPrincipalResponse + err error + calls int +} + +func (a *accessTestAccount) GetCurrentPrincipal( + _ context.Context, req *azdext.GetCurrentPrincipalRequest, _ ...grpc.CallOption, +) (*azdext.GetCurrentPrincipalResponse, error) { + a.calls++ + if req.SubscriptionId != "sub" { + return nil, errors.New("wrong subscription") + } + return a.response, a.err +} + +type accessTestRoles struct { + hasAccess bool + checkErr error + assignErr error + checks int + assigns int + principalID string + principal armauthorization.PrincipalType +} + +func (r *accessTestRoles) HasAccess(_ context.Context, _, principalID string) (bool, error) { + r.checks++ + r.principalID = principalID + return r.hasAccess, r.checkErr +} + +func (r *accessTestRoles) Assign( + _ context.Context, _, principalID string, principal armauthorization.PrincipalType, +) error { + r.assigns++ + r.principalID, r.principal = principalID, principal + return r.assignErr +} + +func accessDenied() error { + return &azcore.ResponseError{StatusCode: http.StatusForbidden, ErrorCode: "PermissionDenied"} +} + +func TestAccessPreparation(t *testing.T) { + for _, tt := range []struct { + name string + probeErr error + principal azdext.PrincipalType + principalID string + principalErr error + hasAccess bool + checkErr error + assignErr error + wantChecks int + wantAssigns int + wantProbes int + wantWarning bool + wantErr error + }{ + {name: "externally managed custom access", wantProbes: 1}, + {name: "non-authorization probe failure", probeErr: errors.New("network failure"), wantProbes: 1, wantWarning: true}, + {name: "unrelated forbidden", probeErr: &azcore.ResponseError{ + StatusCode: 403, ErrorCode: "FirewallDenied", + }, wantProbes: 1, wantWarning: true}, + {name: "inherited Foundry User", probeErr: accessDenied(), principal: azdext.PrincipalType_PRINCIPAL_TYPE_USER, + principalID: "guest-object-id", hasAccess: true, wantChecks: 1, wantProbes: 1, wantWarning: true}, + {name: "owner without data access", probeErr: accessDenied(), principal: azdext.PrincipalType_PRINCIPAL_TYPE_USER, + principalID: "guest-object-id", wantChecks: 1, wantAssigns: 1, wantProbes: 2}, + {name: "service principal", probeErr: accessDenied(), + principal: azdext.PrincipalType_PRINCIPAL_TYPE_SERVICE_PRINCIPAL, principalID: "sp-object-id", + wantChecks: 1, wantAssigns: 1, wantProbes: 2}, + {name: "principal unavailable", probeErr: accessDenied(), principalErr: errors.New("lookup failed"), + wantProbes: 1, wantWarning: true}, + {name: "principal empty", probeErr: accessDenied(), principal: azdext.PrincipalType_PRINCIPAL_TYPE_USER, + wantProbes: 1, wantWarning: true}, + {name: "unsupported principal", probeErr: accessDenied(), principalID: "object-id", + wantProbes: 1, wantWarning: true}, + {name: "role visibility denied", probeErr: accessDenied(), principal: azdext.PrincipalType_PRINCIPAL_TYPE_USER, + principalID: "object-id", checkErr: accessDenied(), wantChecks: 1, wantProbes: 1, wantWarning: true}, + {name: "grant denied", probeErr: accessDenied(), principal: azdext.PrincipalType_PRINCIPAL_TYPE_USER, + principalID: "object-id", assignErr: accessDenied(), wantChecks: 1, wantAssigns: 1, + wantProbes: 1, wantWarning: true}, + {name: "probe cancellation", probeErr: context.Canceled, wantProbes: 1, wantErr: context.Canceled}, + {name: "principal cancellation", probeErr: accessDenied(), principalErr: context.DeadlineExceeded, + wantProbes: 1, wantErr: context.DeadlineExceeded}, + {name: "role check cancellation", probeErr: accessDenied(), principal: azdext.PrincipalType_PRINCIPAL_TYPE_USER, + principalID: "object-id", checkErr: context.Canceled, wantChecks: 1, wantProbes: 1, wantErr: context.Canceled}, + {name: "grant cancellation", probeErr: accessDenied(), principal: azdext.PrincipalType_PRINCIPAL_TYPE_USER, + principalID: "object-id", assignErr: context.Canceled, wantChecks: 1, wantAssigns: 1, + wantProbes: 1, wantErr: context.Canceled}, + } { + t.Run(tt.name, func(t *testing.T) { + account := &accessTestAccount{response: &azdext.GetCurrentPrincipalResponse{ + ObjectId: tt.principalID, PrincipalType: tt.principal, + }, err: tt.principalErr} + roles := &accessTestRoles{hasAccess: tt.hasAccess, checkErr: tt.checkErr, assignErr: tt.assignErr} + probes := 0 + var warnings []string + err := (AccessPreparation{ + Account: account, Roles: roles, Scope: accessTestScope, + Probe: func(context.Context) error { + probes++ + if probes == 1 { + return tt.probeErr + } + require.Equal(t, 1, roles.assigns, "probe may only be retried after a grant") + return nil + }, + Warn: func(message string) { warnings = append(warnings, message) }, + }).Ensure(t.Context()) + require.ErrorIs(t, err, tt.wantErr) + assert.Equal(t, tt.wantChecks, roles.checks) + assert.Equal(t, tt.wantAssigns, roles.assigns) + assert.Equal(t, tt.wantProbes, probes) + assert.Equal(t, tt.wantWarning, len(warnings) > 0) + if roles.assigns > 0 { + assert.Equal(t, tt.principalID, roles.principalID) + if tt.principal == azdext.PrincipalType_PRINCIPAL_TYPE_SERVICE_PRINCIPAL { + assert.Equal(t, armauthorization.PrincipalTypeServicePrincipal, roles.principal) + } else { + assert.Equal(t, armauthorization.PrincipalTypeUser, roles.principal) + } + } + }) + } +} + +func TestAccessPreparationPropagation(t *testing.T) { + for _, tt := range []struct { + name string + readyAt int + }{ + {name: "eventual readiness", readyAt: 4}, + {name: "bounded exhaustion"}, + } { + t.Run(tt.name, func(t *testing.T) { + probes := 0 + roles := &accessTestRoles{} + err := (AccessPreparation{ + Account: &accessTestAccount{response: &azdext.GetCurrentPrincipalResponse{ + ObjectId: "object-id", PrincipalType: azdext.PrincipalType_PRINCIPAL_TYPE_USER, + }}, + Roles: roles, Scope: accessTestScope, + Probe: func(context.Context) error { + probes++ + if probes == tt.readyAt { + return nil + } + return accessDenied() + }, + Warn: func(message string) { t.Errorf("unexpected warning: %s", message) }, + Attempts: 3, RetryDelay: time.Nanosecond, + }).Ensure(t.Context()) + if tt.readyAt > 0 { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, "access has not propagated") + assert.True(t, IsProjectAccessDenied(err)) + } + assert.Equal(t, 4, probes) + assert.Equal(t, 1, roles.assigns) + }) + } +} + +func TestAccessPreparationCancellationDuringPropagation(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + probes := 0 + err := (AccessPreparation{ + Account: &accessTestAccount{response: &azdext.GetCurrentPrincipalResponse{ + ObjectId: "object-id", PrincipalType: azdext.PrincipalType_PRINCIPAL_TYPE_USER, + }}, + Roles: &accessTestRoles{}, Scope: accessTestScope, + Probe: func(context.Context) error { + probes++ + if probes == 2 { + cancel() + } + return accessDenied() + }, + Warn: func(string) {}, + }).Ensure(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 2, probes) +} + +func TestAccessPreparationGRPCCancellation(t *testing.T) { + err := (AccessPreparation{ + Probe: func(context.Context) error { return status.Error(codes.Canceled, "cancelled") }, + }).Ensure(t.Context()) + require.Equal(t, codes.Canceled, status.Code(err)) +} + +type accessTestCredential struct{} + +func (accessTestCredential) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) { + return azcore.AccessToken{Token: "test-token", ExpiresOn: time.Now().Add(time.Hour)}, nil +} + +type accessTestTransport func(*http.Request) (*http.Response, error) + +func (f accessTestTransport) Do(req *http.Request) (*http.Response, error) { return f(req) } + +func newTestProjectRoles(t *testing.T, transport accessTestTransport) *projectRoles { + t.Helper() + client, err := armauthorization.NewRoleAssignmentsClient("sub", accessTestCredential{}, &arm.ClientOptions{ + ClientOptions: policy.ClientOptions{Transport: transport, Retry: policy.RetryOptions{MaxRetries: -1}}, + }) + require.NoError(t, err) + return &projectRoles{client: client} +} + +func accessResponse(req *http.Request, code int, body string) *http.Response { + return &http.Response{StatusCode: code, Request: req, Header: http.Header{"Content-Type": {"application/json"}}, + Body: io.NopCloser(strings.NewReader(body))} +} + +func TestProjectRolesInheritedAndEquivalentAccess(t *testing.T) { + for _, tt := range []struct { + name string + role string + scope string + want bool + }{ + {name: "project Foundry User", role: FoundryUserRole, scope: accessTestScope, want: true}, + {name: "inherited Foundry User", role: FoundryUserRole, scope: "/subscriptions/sub", want: true}, + {name: "management group inheritance", role: FoundryUserRole, + scope: "/providers/Microsoft.Management/managementGroups/group", want: true}, + {name: "legacy Cognitive Services User", role: projectAccessRoles[1], scope: accessTestScope, want: true}, + {name: "Foundry Project Manager", role: projectAccessRoles[2], scope: accessTestScope, want: true}, + {name: "Foundry Owner", role: projectAccessRoles[3], scope: accessTestScope, want: true}, + {name: "Owner has no data actions", role: "8e3af657-a8ff-443c-a75c-2fe8c4bcb635", scope: accessTestScope}, + {name: "Contributor has no data actions", role: "b24988ac-6180-42a0-ab88-20f7382dd24c", scope: accessTestScope}, + {name: "sibling assignment", role: FoundryUserRole, scope: accessTestScope + "-other"}, + {name: "child assignment", role: FoundryUserRole, scope: accessTestScope + "/agents/agent"}, + } { + t.Run(tt.name, func(t *testing.T) { + roles := newTestProjectRoles(t, func(req *http.Request) (*http.Response, error) { + assert.Contains(t, req.URL.Query().Get("$filter"), "assignedTo('principal')") + return accessResponse(req, 200, `{"value":[{"properties":{"scope":"`+tt.scope+ + `","roleDefinitionId":"/providers/Microsoft.Authorization/roleDefinitions/`+tt.role+`"}}]}`), nil + }) + hasAccess, err := roles.HasAccess(t.Context(), accessTestScope, "principal") + require.NoError(t, err) + assert.Equal(t, tt.want, hasAccess) + }) + } +} + +func TestProjectRolesAssignmentConflicts(t *testing.T) { + for _, tt := range []struct { + code string + wantErr bool + }{ + {code: "RoleAssignmentExists"}, + {code: "RoleAssignmentUpdateNotPermitted", wantErr: true}, + } { + t.Run(tt.code, func(t *testing.T) { + var paths []string + roles := newTestProjectRoles(t, func(req *http.Request) (*http.Response, error) { + paths = append(paths, req.URL.Path) + body, err := io.ReadAll(req.Body) + require.NoError(t, err) + assert.Contains(t, string(body), `"principalType":"ServicePrincipal"`) + assert.Contains(t, string(body), FoundryUserRole) + return accessResponse(req, 409, `{"error":{"code":"`+tt.code+`","message":"conflict"}}`), nil + }) + for range 2 { + err := roles.Assign( + t.Context(), accessTestScope, "principal", armauthorization.PrincipalTypeServicePrincipal) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + } + require.Len(t, paths, 2) + assert.Equal(t, paths[0], paths[1], "the same project/principal must use the same assignment name") + }) + } +} + +func TestProjectRolesAssignmentRequest(t *testing.T) { + const principalID = "11111111-2222-3333-4444-555555555555" + for _, principalType := range []armauthorization.PrincipalType{ + armauthorization.PrincipalTypeUser, armauthorization.PrincipalTypeServicePrincipal, + } { + t.Run(string(principalType), func(t *testing.T) { + calls := 0 + roles := newTestProjectRoles(t, func(request *http.Request) (*http.Response, error) { + calls++ + require.Equal(t, http.MethodPut, request.Method) + require.True(t, strings.HasPrefix(request.URL.Path, + accessTestScope+"/providers/Microsoft.Authorization/roleAssignments/")) + var parameters armauthorization.RoleAssignmentCreateParameters + require.NoError(t, json.NewDecoder(request.Body).Decode(¶meters)) + require.NotNil(t, parameters.Properties) + assert.Equal(t, new("/subscriptions/sub/providers/Microsoft.Authorization/roleDefinitions/"+ + FoundryUserRole), parameters.Properties.RoleDefinitionID) + assert.Equal(t, new(principalID), parameters.Properties.PrincipalID) + assert.Equal(t, new(principalType), parameters.Properties.PrincipalType) + return accessResponse(request, http.StatusCreated, `{"properties":{}}`), nil + }) + require.NoError(t, roles.Assign(t.Context(), accessTestScope, principalID, principalType)) + assert.Equal(t, 1, calls, "the preparation's role check makes a second pre-check unnecessary") + }) + } +} diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/foundry/credential.go b/cli/azd/pkg/foundry/credential.go similarity index 56% rename from cli/azd/extensions/azure.ai.toolboxes/internal/foundry/credential.go rename to cli/azd/pkg/foundry/credential.go index d4217f62246..4eb553bd9a6 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/foundry/credential.go +++ b/cli/azd/pkg/foundry/credential.go @@ -1,11 +1,6 @@ // 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 ( @@ -15,9 +10,8 @@ import ( "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. +// NewCredential returns a Foundry credential bound to the project's user-access tenant. +// An empty tenant ID retains default CLI credential behavior for unscoped operations. func NewCredential(tenantID string) (azcore.TokenCredential, error) { options := &azidentity.AzureDeveloperCLICredentialOptions{} if tenantID != "" { 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..9615c34df34 100644 --- a/docs/guides/creating-an-extension.md +++ b/docs/guides/creating-an-extension.md @@ -53,6 +53,19 @@ 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`, and an injected `Warn` callback. 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