From b4e7f6f9a02bc501c19f8ed630ab5643374b7eba Mon Sep 17 00:00:00 2001 From: Codex CLI Date: Sun, 7 Jun 2026 21:07:31 +0000 Subject: [PATCH 1/5] feat(util): add identity SID validation and normalization Add IsIdentitySID to detect Azure DevOps identity SIDs in both bare and prefixed forms. Add NormalizeIdentitySID to ensure the Microsoft.TeamFoundation.Identity prefix is present where missing. --- internal/azdo/util/descriptors.go | 25 +++++++++++ internal/azdo/util/descriptors_test.go | 58 ++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/internal/azdo/util/descriptors.go b/internal/azdo/util/descriptors.go index 0e4642c0..9fd6ef96 100644 --- a/internal/azdo/util/descriptors.go +++ b/internal/azdo/util/descriptors.go @@ -5,6 +5,8 @@ import ( "strings" ) +const identitySIDPrefix = "Microsoft.TeamFoundation.Identity;" + var ( descriptorPattern = regexp.MustCompile(`^[a-zA-Z]+\.[a-zA-Z0-9-_]+$`) sidPattern = regexp.MustCompile(`(?i)s-\d+-\d+(-\d+)+$`) @@ -33,3 +35,26 @@ func IsSecurityIdentifier(value string) bool { } return sidPattern.MatchString(value) } + +// IsIdentitySID reports whether value is an Azure DevOps identity SID, +// either as a bare SID (s-1-5-...) or with the Microsoft.TeamFoundation.Identity; prefix. +func IsIdentitySID(value string) bool { + value = strings.TrimSpace(value) + if value == "" { + return false + } + if IsSecurityIdentifier(value) { + return true + } + return sidPattern.MatchString(strings.TrimPrefix(value, identitySIDPrefix)) +} + +// NormalizeIdentitySID returns the identity SID prefixed with the Microsoft.TeamFoundation.Identity; +// scope. The value is left unchanged if it already has the prefix or is empty. +func NormalizeIdentitySID(value string) string { + value = strings.TrimSpace(value) + if value == "" || strings.Contains(value, ";") { + return value + } + return identitySIDPrefix + value +} diff --git a/internal/azdo/util/descriptors_test.go b/internal/azdo/util/descriptors_test.go index b97bdc87..814337a3 100644 --- a/internal/azdo/util/descriptors_test.go +++ b/internal/azdo/util/descriptors_test.go @@ -45,3 +45,61 @@ func TestIsDescriptorNotRecognizesSID(t *testing.T) { t.Fatalf("expected SID %q to be treated as descriptor", sid) } } + +func TestIsIdentitySID(t *testing.T) { + tests := []struct { + name string + value string + want bool + }{ + {name: "bare sid", value: "s-1-5-21-123456789-123456789-123456789-1000", want: true}, + {name: "prefixed sid", value: "Microsoft.TeamFoundation.Identity;s-1-5-21-123456789-123456789-123456789-1000", want: true}, + {name: "descriptor is not sid", value: "vssgp.Uy0xLTkt", want: false}, + {name: "email is not sid", value: "user@example.com", want: false}, + {name: "empty", value: "", want: false}, + {name: "trimmed prefixed sid", value: " Microsoft.TeamFoundation.Identity;s-1-5-21-1 ", want: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsIdentitySID(tt.value); got != tt.want { + t.Fatalf("IsIdentitySID(%q) = %v, want %v", tt.value, got, tt.want) + } + }) + } +} + +func TestNormalizeIdentitySID(t *testing.T) { + tests := []struct { + name string + value string + want string + }{ + { + name: "bare sid gets prefix", + value: "s-1-5-21-123456789-123456789-123456789-1000", + want: "Microsoft.TeamFoundation.Identity;s-1-5-21-123456789-123456789-123456789-1000", + }, + { + name: "prefixed sid is left alone", + value: "Microsoft.TeamFoundation.Identity;s-1-5-21-123456789-123456789-123456789-1000", + want: "Microsoft.TeamFoundation.Identity;s-1-5-21-123456789-123456789-123456789-1000", + }, + { + name: "empty stays empty", + value: "", + want: "", + }, + { + name: "trimmed bare sid gets prefix", + value: " s-1-5-21-123456789-123456789-123456789-1000 ", + want: "Microsoft.TeamFoundation.Identity;s-1-5-21-123456789-123456789-123456789-1000", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := NormalizeIdentitySID(tt.value); got != tt.want { + t.Fatalf("NormalizeIdentitySID(%q) = %q, want %q", tt.value, got, tt.want) + } + }) + } +} From ba7235ae3974d5c2ac15e4060043d07b983357c2 Mon Sep 17 00:00:00 2001 From: Codex CLI Date: Sun, 7 Jun 2026 21:08:48 +0000 Subject: [PATCH 2/5] feat(extensions): implement batched member resolution * Introduce ResolveSubjects on the extension Client to resolve multiple member identifiers in a single call, reducing azdo rest api round trips. the implementation partitions inputs by identifier type and dispatches each partition to the appropriate batch endpoint. * Refactor ResolveSubject to delegate through the new batch path, eliminating duplicated lookup logic. unresolvable inputs are omitted from the result map; only catastrophic failures surface as errors. * Add comprehensive tests for batched descriptor, sid, and search resolution, including partial results, not-found handling, and fallback paths. regenerate mocks for the updated interface. --- internal/azdo/extensions/extension.go | 4 + internal/azdo/extensions/member_lookup.go | 444 +++++++++----- .../azdo/extensions/member_lookup_test.go | 544 ++++++++++++++++++ internal/mocks/extension_mock.go | 15 + 4 files changed, 877 insertions(+), 130 deletions(-) diff --git a/internal/azdo/extensions/extension.go b/internal/azdo/extensions/extension.go index 9fd465d2..3a270094 100644 --- a/internal/azdo/extensions/extension.go +++ b/internal/azdo/extensions/extension.go @@ -28,6 +28,10 @@ type Client interface { FindGroupsByDisplayName(ctx context.Context, displayName string, scopeDescriptor *string) ([]*graph.GraphGroup, error) // ResolveSubject resolves a member identifier (descriptor, email, or principal name) into a graph subject descriptor. ResolveSubject(ctx context.Context, member string) (*graph.GraphSubject, error) + // ResolveSubjects resolves a batch of member identifiers in a single call, reducing round trips. + // Inputs that cannot be resolved are absent from the result map (keyed by the trimmed input string). + // The map naturally deduplicates repeated inputs; only catastrophic failures (e.g. client creation) return an error. + ResolveSubjects(ctx context.Context, members []string) (map[string]*graph.GraphSubject, error) ResolveIdentity(ctx context.Context, member string) (*identity.Identity, error) } diff --git a/internal/azdo/extensions/member_lookup.go b/internal/azdo/extensions/member_lookup.go index c6b69519..79c641a6 100644 --- a/internal/azdo/extensions/member_lookup.go +++ b/internal/azdo/extensions/member_lookup.go @@ -15,165 +15,358 @@ import ( "go.uber.org/zap" ) -// ResolveSubject resolves a member identifier (descriptor, email, or principal name) into a graph subject descriptor. +// ResolveSubject resolves a single member identifier by delegating to the batch path +// and translating the result back into the legacy single-item API. func (c *extensionClient) ResolveSubject(ctx context.Context, member string) (*graph.GraphSubject, error) { member = strings.TrimSpace(member) if member == "" { - return nil, fmt.Errorf("member must not be empty") + return nil, errors.New("member must not be empty") } - - graphClient, err := graph.NewClient(ctx, c.conn) + results, err := c.ResolveSubjects(ctx, []string{member}) if err != nil { - return nil, fmt.Errorf("failed to create graph client: %w", err) + return nil, err } + return results[member], nil +} - if util.IsDescriptor(member) { - zap.L().Debug("attempting graph subject lookup for descriptor", zap.String("descriptor", member)) - subject, err := lookupGraphSubject(ctx, graphClient, member) - if err != nil { - return nil, err +// ResolveSubjects resolves a batch of member identifiers (descriptors, SIDs, emails, or principal names) +// into graph subjects. The implementation partitions inputs by type and dispatches each partition to the +// most appropriate AzDO REST endpoint with native batch semantics: +// +// - Subject descriptors: one batched graph.LookupSubjects call covering all descriptors. +// - SIDs: one batched identity.ReadIdentities(Descriptors: csv) call. +// - Free-form identifiers (emails, UPNs, etc.): per-input identity.ReadIdentities searches followed +// by a single batched graph.LookupSubjects call to enrich the resulting descriptors. +// +// Inputs that cannot be resolved are simply absent from the result map; the caller can detect failure +// by checking map membership. The map key is the trimmed input string, so duplicate inputs collapse +// to a single entry. Only catastrophic failures (e.g. client construction) return a non-nil error. +func (c *extensionClient) ResolveSubjects(ctx context.Context, members []string) (map[string]*graph.GraphSubject, error) { + trimmed := make([]string, 0, len(members)) + seen := make(map[string]struct{}, len(members)) + for _, m := range members { + t := strings.TrimSpace(m) + if t == "" { + continue } - if subject == nil { - return nil, fmt.Errorf("descriptor %q was not found", member) + if _, ok := seen[t]; ok { + continue } - zap.L().Debug("resolved member via graph descriptor lookup", - zap.String("descriptor", member), - zap.String("displayName", - types.GetValue(subject.DisplayName, "")), - zap.String("subjectKind", types.GetValue(subject.SubjectKind, "")), - zap.String("legacyDescriptor", types.GetValue(subject.LegacyDescriptor, "")), - ) - return subject, nil + seen[t] = struct{}{} + trimmed = append(trimmed, t) + } + + if len(trimmed) == 0 { + return map[string]*graph.GraphSubject{}, nil } + graphClient, err := graph.NewClient(ctx, c.conn) + if err != nil { + return nil, fmt.Errorf("failed to create graph client: %w", err) + } identityClient, err := identity.NewClient(ctx, c.conn) if err != nil { return nil, fmt.Errorf("failed to create identity client: %w", err) } - resolvedIdentity, err := resolveIdentity(ctx, identityClient, member) - if err != nil { - return nil, err + result := make(map[string]*graph.GraphSubject, len(trimmed)) + + descriptorInputs, sidInputs, otherInputs := partitionInputs(trimmed) + + if len(descriptorInputs) > 0 { + if err := resolveDescriptorBatch(ctx, graphClient, descriptorInputs, result); err != nil { + return nil, err + } } - if resolvedIdentity == nil { - return nil, fmt.Errorf("no identity found for %q", member) + if len(sidInputs) > 0 { + if err := resolveSIDBatch(ctx, identityClient, graphClient, sidInputs, result); err != nil { + return nil, err + } + } + if len(otherInputs) > 0 { + if err := resolveSearchBatch(ctx, identityClient, graphClient, otherInputs, result); err != nil { + return nil, err + } } - descriptor := types.GetValue(resolvedIdentity.SubjectDescriptor, "") - if descriptor == "" { - if resolvedIdentity.Id == nil { - return nil, fmt.Errorf("identity for %q is missing descriptor and storage key", member) + return result, nil +} + +// partitionInputs splits inputs into the three batches the AzDO REST API can serve most efficiently. +func partitionInputs(inputs []string) (descriptors, sids, others []string) { + for _, m := range inputs { + switch { + case util.IsDescriptor(m): + descriptors = append(descriptors, m) + case util.IsIdentitySID(m): + sids = append(sids, m) + default: + others = append(others, m) } - desc, derr := graphClient.GetDescriptor(ctx, graph.GetDescriptorArgs{ - StorageKey: resolvedIdentity.Id, - }) - if derr != nil { - return nil, fmt.Errorf("failed to resolve descriptor from storage key: %w", derr) + } + return descriptors, sids, others +} + +// resolveDescriptorBatch resolves a batch of subject descriptors via a single graph.LookupSubjects call. +func resolveDescriptorBatch(ctx context.Context, graphClient graph.Client, inputs []string, out map[string]*graph.GraphSubject) error { + keys := subjectLookupKeys(inputs) + + subjects, err := graphClient.LookupSubjects(ctx, graph.LookupSubjectsArgs{ + SubjectLookup: &graph.GraphSubjectLookup{LookupKeys: &keys}, + }) + if err != nil { + if isNotFoundError(err) { + return nil } - descriptor = types.GetValue(desc.Value, "") - if descriptor == "" { - return nil, fmt.Errorf("descriptor lookup returned empty result for %q", member) + return fmt.Errorf("failed to lookup %d descriptor(s): %w", len(inputs), err) + } + if subjects == nil { + return nil + } + + indexed := indexSubjectsByDescriptor(subjects) + for _, input := range inputs { + if subject, ok := indexed[input]; ok { + s := subject + out[input] = &s } } + return nil +} + +// resolveSIDBatch resolves a batch of SIDs via a single identity.ReadIdentities(Descriptors: csv) call, +// then enriches the resulting descriptors with a single graph.LookupSubjects call. +// +// Correlation between the requested SIDs and the returned identities relies on the AzDO IMS API +// echoing the requested identity descriptor in the response's identity.Identity.Descriptor field. +// The AzDO documentation treats identity descriptors as a first-class lookup identifier, so a +// non-echoing response is treated as a contract drift and surfaced as an error rather than +// silently falling back to positional correlation. If this error is ever observed in production, +// the resolution pipeline needs a fallback path (per-input lookups or positional correlation +// over the request list). +func resolveSIDBatch(ctx context.Context, identityClient identity.Client, graphClient graph.Client, inputs []string, out map[string]*graph.GraphSubject) error { + prefixedDescriptors := make([]string, 0, len(inputs)) + originalByDescriptor := make(map[string]string, len(inputs)) + for _, sid := range inputs { + descriptor := util.NormalizeIdentitySID(sid) + prefixedDescriptors = append(prefixedDescriptors, descriptor) + originalByDescriptor[descriptor] = sid + } - subject, err := lookupGraphSubject(ctx, graphClient, descriptor) + csv := strings.Join(prefixedDescriptors, ",") + identities, err := identityClient.ReadIdentities(ctx, identity.ReadIdentitiesArgs{ + Descriptors: &csv, + QueryMembership: &identity.QueryMembershipValues.None, + }) if err != nil { - return nil, err + return fmt.Errorf("failed to resolve %d SID(s): %w", len(inputs), err) } - if subject != nil { - return subject, nil + if identities == nil || len(*identities) == 0 { + return nil } - displayName := types.GetValue(resolvedIdentity.ProviderDisplayName, "") - kind := memberSubjectKind(*resolvedIdentity) + type resolvedSlot struct { + original string + descriptor string + identity identity.Identity + } + slots := make([]resolvedSlot, 0, len(*identities)) - return &graph.GraphSubject{ - Descriptor: types.ToPtr(descriptor), - DisplayName: types.ToPtr(displayName), - SubjectKind: types.ToPtr(kind), - }, nil -} + for _, id := range *identities { + requestedDescriptor := types.GetValue(id.Descriptor, "") + original, ok := originalByDescriptor[requestedDescriptor] + if !ok { + return fmt.Errorf("ReadIdentities returned identity descriptor %q which was not requested", requestedDescriptor) + } + subjectDescriptor := types.GetValue(id.SubjectDescriptor, "") + if subjectDescriptor == "" { + continue + } + slots = append(slots, resolvedSlot{ + original: original, + descriptor: subjectDescriptor, + identity: id, + }) + } + if len(slots) == 0 { + return nil + } -func (c *extensionClient) ResolveIdentity(ctx context.Context, member string) (*identity.Identity, error) { - member = strings.TrimSpace(member) - if member == "" { - return nil, fmt.Errorf("member must not be empty") + subjectDescriptors := make([]string, len(slots)) + for i, s := range slots { + subjectDescriptors[i] = s.descriptor + } + keys := subjectLookupKeys(subjectDescriptors) + subjects, err := graphClient.LookupSubjects(ctx, graph.LookupSubjectsArgs{ + SubjectLookup: &graph.GraphSubjectLookup{LookupKeys: &keys}, + }) + if err != nil && !isNotFoundError(err) { + return fmt.Errorf("failed to enrich %d SID(s): %w", len(inputs), err) } - identityClient, err := identity.NewClient(ctx, c.conn) - if err != nil { - return nil, fmt.Errorf("failed to create identity client: %w", err) + indexed := indexSubjectsByDescriptor(subjects) + for _, s := range slots { + if subject, ok := indexed[s.descriptor]; ok { + outS := subject + out[s.original] = &outS + continue + } + out[s.original] = graphSubjectFromIdentity(s.identity, s.descriptor) } + return nil +} - return resolveIdentity(ctx, identityClient, member) +// subjectLookupKeys builds a slice of GraphSubjectLookupKey with each descriptor wired up +// as the lookup key. Callers hand the result to graph.LookupSubjects. +func subjectLookupKeys(descriptors []string) []graph.GraphSubjectLookupKey { + keys := make([]graph.GraphSubjectLookupKey, 0, len(descriptors)) + for _, d := range descriptors { + keys = append(keys, graph.GraphSubjectLookupKey{Descriptor: &d}) + } + return keys +} + +// indexSubjectsByDescriptor returns a lookup map keyed by the exact descriptor of each +// subject, so callers can resolve subjects in O(1). Callers receive nil for a nil input +// map, which is safe to read from (every lookup misses). +func indexSubjectsByDescriptor(subjects *map[string]graph.GraphSubject) map[string]graph.GraphSubject { + if subjects == nil { + return nil + } + indexed := make(map[string]graph.GraphSubject, len(*subjects)) + for _, subject := range *subjects { + if subject.Descriptor == nil { + continue + } + indexed[*subject.Descriptor] = subject + } + return indexed } -func lookupGraphSubject(ctx context.Context, client graph.Client, descriptor string) (*graph.GraphSubject, error) { - if strings.TrimSpace(descriptor) == "" { - return nil, nil +// graphSubjectFromIdentity builds a minimal graph.GraphSubject from an identity record +// when enrichment via LookupSubjects does not return a matching subject. +func graphSubjectFromIdentity(id identity.Identity, descriptor string) *graph.GraphSubject { + return &graph.GraphSubject{ + Descriptor: types.ToPtr(descriptor), + DisplayName: types.ToPtr(types.GetValue(id.ProviderDisplayName, "")), + SubjectKind: types.ToPtr(memberSubjectKind(id)), } +} - keys := []graph.GraphSubjectLookupKey{ - { - Descriptor: types.ToPtr(descriptor), - }, +// isNotFoundError reports whether err is an azuredevops.WrappedError with a 404 status code. +// AzDO REST endpoints that are allowed to fail-soft return (nil, nil) for these, while other +// errors propagate to the caller. +func isNotFoundError(err error) bool { + if err == nil { + return false } - subjectLookup := graph.GraphSubjectLookup{ - LookupKeys: &keys, + var wrappedErr *azuredevops.WrappedError + if !errors.As(err, &wrappedErr) || wrappedErr == nil || wrappedErr.StatusCode == nil { + return false } + return *wrappedErr.StatusCode == http.StatusNotFound +} - result, err := client.LookupSubjects(ctx, graph.LookupSubjectsArgs{ - SubjectLookup: &subjectLookup, - }) - if err != nil { - var wrappedErr *azuredevops.WrappedError - if errors.As(err, &wrappedErr) && wrappedErr != nil && wrappedErr.StatusCode != nil { - if *wrappedErr.StatusCode == http.StatusNotFound { - return nil, nil +// resolveSearchBatch resolves free-form identifiers (emails, UPNs, account names) via per-input +// identity.ReadIdentities searches. Once all descriptors are collected, a single graph.LookupSubjects +// call enriches them with display name, subject kind, and legacy descriptor fields. +func resolveSearchBatch(ctx context.Context, identityClient identity.Client, graphClient graph.Client, inputs []string, out map[string]*graph.GraphSubject) error { + descriptorToInput := make(map[string]string, len(inputs)) + descriptorFallback := make(map[string]identity.Identity, len(inputs)) + descriptors := make([]string, 0, len(inputs)) + + for _, member := range inputs { + identity, err := resolveIdentity(ctx, identityClient, member) + if err != nil { + return fmt.Errorf("failed to resolve member %q: %w", member, err) + } + if identity == nil { + continue + } + + descriptor := types.GetValue(identity.SubjectDescriptor, "") + if descriptor == "" { + if identity.Id == nil { + continue + } + desc, derr := graphClient.GetDescriptor(ctx, graph.GetDescriptorArgs{ + StorageKey: identity.Id, + }) + if derr != nil { + return fmt.Errorf("failed to resolve descriptor from storage key for %q: %w", member, derr) + } + descriptor = types.GetValue(desc.Value, "") + if descriptor == "" { + continue } } - return nil, fmt.Errorf("failed to lookup descriptor %q: %w", descriptor, err) + + if existing, dup := descriptorToInput[descriptor]; dup { + zap.L().Debug( + "multiple inputs resolve to same descriptor; keeping first", + zap.String("kept", existing), + zap.String("ignored", member), + zap.String("descriptor", descriptor), + ) + continue + } + descriptorToInput[descriptor] = member + descriptorFallback[descriptor] = *identity + descriptors = append(descriptors, descriptor) } - if result == nil || len(*result) == 0 { - return nil, nil + if len(descriptors) == 0 { + return nil } - if subject, ok := (*result)[descriptor]; ok { - res := subject - return &res, nil + keys := subjectLookupKeys(descriptors) + subjects, err := graphClient.LookupSubjects(ctx, graph.LookupSubjectsArgs{ + SubjectLookup: &graph.GraphSubjectLookup{LookupKeys: &keys}, + }) + if err != nil && !isNotFoundError(err) { + return fmt.Errorf("failed to enrich %d member(s): %w", len(descriptorToInput), err) } - for _, subject := range *result { - if types.GetValue(subject.Descriptor, "") == descriptor { - res := subject - return &res, nil + indexed := indexSubjectsByDescriptor(subjects) + for descriptor, originalInput := range descriptorToInput { + if subject, ok := indexed[descriptor]; ok { + s := subject + out[originalInput] = &s + continue } + out[originalInput] = graphSubjectFromIdentity(descriptorFallback[descriptor], descriptor) + } + return nil +} - if subject.Descriptor != nil && strings.EqualFold(*subject.Descriptor, descriptor) { - res := subject - return &res, nil - } +func (c *extensionClient) ResolveIdentity(ctx context.Context, member string) (*identity.Identity, error) { + member = strings.TrimSpace(member) + if member == "" { + return nil, errors.New("member must not be empty") } - return nil, nil + identityClient, err := identity.NewClient(ctx, c.conn) + if err != nil { + return nil, fmt.Errorf("failed to create identity client: %w", err) + } + + return resolveIdentity(ctx, identityClient, member) } -/** - * The Identities REST API allows filtering a serach request by the following string values. - * The values can be concatenated by using a CSV list - * - * - "AccountName" - * - "DisplayName" - * - "AdministratorsGroup" - * - "Identifier" - * - "MailAddress" - * - "General" - * - "Alias" - * - "DirectoryAlias" - * - "TeamGroupName" - * - "LocalGroupName" - */ +// determineIdentitySearchFilters returns the AzDO Identity SearchFilter values appropriate +// for the given free-form member string. The Identities REST API supports a fixed set of +// filter values that can also be passed as a CSV list; the supported values are: +// +// - "AccountName" +// - "DisplayName" +// - "AdministratorsGroup" +// - "Identifier" +// - "MailAddress" +// - "General" +// - "Alias" +// - "DirectoryAlias" +// - "TeamGroupName" +// - "LocalGroupName" func determineIdentitySearchFilters(member string) []string { var filters []string if strings.Contains(member, "@") { @@ -196,13 +389,22 @@ func memberSubjectKind(identity identity.Identity) string { return "User" } +// singleIdentity returns the unique identity from a ReadIdentities result, or an error +// if the result is missing or ambiguous. The member string is included in error messages. +func singleIdentity(member string, identities *[]identity.Identity) (*identity.Identity, error) { + if identities == nil || len(*identities) == 0 { + return nil, fmt.Errorf("identity %q not found", member) + } + if len(*identities) > 1 { + return nil, fmt.Errorf("multiple identities found for %q; specify a more specific identifier", member) + } + return &(*identities)[0], nil +} + func resolveIdentity(ctx context.Context, client identity.Client, member string) (*identity.Identity, error) { - if util.IsSecurityIdentifier(member) { + if util.IsIdentitySID(member) { zap.L().Debug("member is a SID", zap.String("member", member)) - descriptor := member - if !strings.Contains(member, ";") { - descriptor = "Microsoft.TeamFoundation.Identity;" + member - } + descriptor := util.NormalizeIdentitySID(member) identities, err := client.ReadIdentities(ctx, identity.ReadIdentitiesArgs{ Descriptors: &descriptor, QueryMembership: &identity.QueryMembershipValues.None, @@ -210,14 +412,7 @@ func resolveIdentity(ctx context.Context, client identity.Client, member string) if err != nil { return nil, fmt.Errorf("failed to resolve member %q: %w", member, err) } - if identities == nil || len(*identities) == 0 { - return nil, fmt.Errorf("identity %q not found", member) - } - if len(*identities) > 1 { - return nil, fmt.Errorf("multiple identities found for %q; specify a more specific identifier", member) - } - - return &(*identities)[0], nil + return singleIdentity(member, identities) } if util.IsDescriptor(member) { @@ -229,14 +424,7 @@ func resolveIdentity(ctx context.Context, client identity.Client, member string) if err != nil { return nil, fmt.Errorf("failed to resolve member %q: %w", member, err) } - if identities == nil || len(*identities) == 0 { - return nil, fmt.Errorf("identity %q not found", member) - } - if len(*identities) > 1 { - return nil, fmt.Errorf("multiple identities found for %q; specify a more specific identifier", member) - } - - return &(*identities)[0], nil + return singleIdentity(member, identities) } filters := determineIdentitySearchFilters(member) @@ -256,11 +444,7 @@ func resolveIdentity(ctx context.Context, client identity.Client, member string) if identities == nil || len(*identities) == 0 { continue } - if len(*identities) > 1 { - return nil, fmt.Errorf("multiple identities found for %q; specify a more specific identifier", member) - } - - return &(*identities)[0], nil + return singleIdentity(member, identities) } return nil, nil diff --git a/internal/azdo/extensions/member_lookup_test.go b/internal/azdo/extensions/member_lookup_test.go index 885ac234..aedbc990 100644 --- a/internal/azdo/extensions/member_lookup_test.go +++ b/internal/azdo/extensions/member_lookup_test.go @@ -1,10 +1,17 @@ package extensions import ( + "context" + "errors" + "net/http" "testing" + "github.com/google/uuid" + "github.com/microsoft/azure-devops-go-api/azuredevops/v7" + "github.com/microsoft/azure-devops-go-api/azuredevops/v7/graph" "github.com/microsoft/azure-devops-go-api/azuredevops/v7/identity" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/tmeckel/azdo-cli/internal/types" ) @@ -66,3 +73,540 @@ func Test_memberSubjectKind(t *testing.T) { assert.Equal(t, "User", kind) }) } + +// stubGraphClient satisfies the graph.Client interface but only honors the methods invoked +// in the tests below. Any other call will panic, surfacing unexpected invocations loudly. +type stubGraphClient struct { + graph.Client + lookupSubjectsFunc func(context.Context, graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) + getDescriptorFunc func(context.Context, graph.GetDescriptorArgs) (*graph.GraphDescriptorResult, error) +} + +func (s *stubGraphClient) LookupSubjects(ctx context.Context, args graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + return s.lookupSubjectsFunc(ctx, args) +} + +func (s *stubGraphClient) GetDescriptor(ctx context.Context, args graph.GetDescriptorArgs) (*graph.GraphDescriptorResult, error) { + return s.getDescriptorFunc(ctx, args) +} + +// stubIdentityClient satisfies the identity.Client interface but only honors the methods invoked +// in the tests below. +type stubIdentityClient struct { + identity.Client + readIdentitiesFunc func(context.Context, identity.ReadIdentitiesArgs) (*[]identity.Identity, error) +} + +func (s *stubIdentityClient) ReadIdentities(ctx context.Context, args identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return s.readIdentitiesFunc(ctx, args) +} + +var ( + _ graph.Client = (*stubGraphClient)(nil) + _ identity.Client = (*stubIdentityClient)(nil) +) + +// lookupDescriptors extracts the descriptor strings from a LookupSubjectsArgs call. +func lookupDescriptors(t *testing.T, args graph.LookupSubjectsArgs) []string { + t.Helper() + require.NotNil(t, args.SubjectLookup) + require.NotNil(t, args.SubjectLookup.LookupKeys) + got := make([]string, 0, len(*args.SubjectLookup.LookupKeys)) + for _, k := range *args.SubjectLookup.LookupKeys { + got = append(got, types.GetValue(k.Descriptor, "")) + } + return got +} + +// wrappedStatus returns an azuredevops.WrappedError with the given HTTP status code. +func wrappedStatus(code int) error { + return &azuredevops.WrappedError{StatusCode: types.ToPtr(code)} +} + +func mustSubject(t *testing.T, m map[string]*graph.GraphSubject, key string) *graph.GraphSubject { + t.Helper() + got, ok := m[key] + require.Truef(t, ok, "expected key %q in result map; got keys=%v", key, mapKeys(m)) + return got +} + +func mapKeys(m map[string]*graph.GraphSubject) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +} + +// splitCSV is a tiny local helper that mirrors the SDK expectation of comma-separated descriptor values. +func splitCSV(value string) []string { + out := make([]string, 0) + current := "" + for _, r := range value { + if r == ',' { + out = append(out, current) + current = "" + continue + } + current += string(r) + } + out = append(out, current) + return out +} + +// subject builds a graph.GraphSubject with the given descriptor and display name. Use this +// in test fixtures to keep subjects readable; inline literals are still fine when SubjectKind +// is part of what the test is asserting on. +func subject(descriptor, name string) graph.GraphSubject { + return graph.GraphSubject{ + Descriptor: types.ToPtr(descriptor), + DisplayName: types.ToPtr(name), + } +} + +// identityWithSubject builds an identity record correlated with a graph subject. Use this for +// test fixtures that need the identity descriptor, subject descriptor, and display name. +func identityWithSubject(identityDescriptor, subjectDescriptor, displayName string) identity.Identity { + return identity.Identity{ + Descriptor: types.ToPtr(identityDescriptor), + SubjectDescriptor: types.ToPtr(subjectDescriptor), + ProviderDisplayName: types.ToPtr(displayName), + } +} + +// identityWithStorageKey builds an identity record that only carries a storage key (UUID). +// Use this for test fixtures that exercise the GetDescriptor fallback path. +func identityWithStorageKey(id string) identity.Identity { + uid := uuid.MustParse(id) + return identity.Identity{Id: &uid} +} + +func Test_partitionInputs(t *testing.T) { + t.Run("routes descriptors, SIDs, and others to correct buckets", func(t *testing.T) { + inputs := []string{ + "aad.YR5kM", + "s-1-2-34-5678", + "alice@contoso.com", + "vssgp.Uy0xLTkt", + "DOMAIN\\user", + } + descriptors, sids, others := partitionInputs(inputs) + assert.Equal(t, []string{"aad.YR5kM", "vssgp.Uy0xLTkt"}, descriptors) + assert.Equal(t, []string{"s-1-2-34-5678"}, sids) + assert.Equal(t, []string{"alice@contoso.com", "DOMAIN\\user"}, others) + }) + + t.Run("empty input yields all-empty slices", func(t *testing.T) { + descriptors, sids, others := partitionInputs(nil) + assert.Empty(t, descriptors) + assert.Empty(t, sids) + assert.Empty(t, others) + }) + + t.Run("prefixed SIDs route to the SID bucket", func(t *testing.T) { + descriptors, sids, others := partitionInputs([]string{ + "Microsoft.TeamFoundation.Identity;s-1-2-34-5678", + }) + assert.Empty(t, descriptors) + assert.Equal(t, []string{"Microsoft.TeamFoundation.Identity;s-1-2-34-5678"}, sids) + assert.Empty(t, others) + }) +} + +func Test_resolveDescriptorBatch_SingleCallCoversAll(t *testing.T) { + inputs := []string{"aad.A1", "aad.A2", "aad.A3"} + subjects := map[string]graph.GraphSubject{ + "aad.A1": subject("aad.A1", "Alice"), + "aad.A2": subject("aad.A2", "Bob"), + "aad.A3": subject("aad.A3", "Carol"), + } + + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, args graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + assert.ElementsMatch(t, inputs, lookupDescriptors(t, args), "all descriptors should be batched into one call") + return &subjects, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveDescriptorBatch(context.Background(), graphClient, inputs, out)) + + assert.Len(t, out, 3) + assert.Equal(t, "Alice", types.GetValue(mustSubject(t, out, "aad.A1").DisplayName, "")) + assert.Equal(t, "Bob", types.GetValue(mustSubject(t, out, "aad.A2").DisplayName, "")) + assert.Equal(t, "Carol", types.GetValue(mustSubject(t, out, "aad.A3").DisplayName, "")) +} + +func Test_resolveDescriptorBatch_PartialResults(t *testing.T) { + subjects := map[string]graph.GraphSubject{ + "aad.A1": subject("aad.A1", "Alice"), + } + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + return &subjects, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveDescriptorBatch(context.Background(), graphClient, []string{"aad.A1", "aad.MISSING"}, out)) + + _, hasMissing := out["aad.MISSING"] + assert.False(t, hasMissing, "missing descriptor should not appear in result map") + _, hasA1 := out["aad.A1"] + assert.True(t, hasA1, "found descriptor should be in result map") +} + +func Test_resolveDescriptorBatch_NotFoundIsSilent(t *testing.T) { + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + return nil, wrappedStatus(http.StatusNotFound) + }, + } + + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveDescriptorBatch(context.Background(), graphClient, []string{"aad.A1"}, out)) + assert.Empty(t, out, "404 from LookupSubjects should not produce a result, not an error") +} + +func Test_resolveDescriptorBatch_OtherErrorsPropagate(t *testing.T) { + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + return nil, wrappedStatus(http.StatusInternalServerError) + }, + } + + out := make(map[string]*graph.GraphSubject) + err := resolveDescriptorBatch(context.Background(), graphClient, []string{"aad.A1"}, out) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to lookup") +} + +func Test_resolveSIDBatch_SingleReadIdentitiesCall(t *testing.T) { + enriched := map[string]graph.GraphSubject{ + "vssgp.U111": subject("vssgp.U111", "SidOne"), + "vssgp.U222": subject("vssgp.U222", "SidTwo"), + } + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, args graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + assert.ElementsMatch(t, []string{"vssgp.U111", "vssgp.U222"}, lookupDescriptors(t, args)) + return &enriched, nil + }, + } + + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, args identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + require.NotNil(t, args.Descriptors) + got := splitCSV(*args.Descriptors) + assert.ElementsMatch(t, []string{ + "Microsoft.TeamFoundation.Identity;s-1-2-34-1111", + "Microsoft.TeamFoundation.Identity;s-1-2-34-2222", + }, got) + id1 := identityWithSubject("Microsoft.TeamFoundation.Identity;s-1-2-34-1111", "vssgp.U111", "SidOne") + id2 := identityWithSubject("Microsoft.TeamFoundation.Identity;s-1-2-34-2222", "vssgp.U222", "SidTwo") + return &[]identity.Identity{id2, id1}, nil + }, + } + + inputs := []string{"s-1-2-34-1111", "s-1-2-34-2222"} + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveSIDBatch(context.Background(), identityClient, graphClient, inputs, out)) + + assert.Len(t, out, 2) + assert.Equal(t, "vssgp.U111", types.GetValue(mustSubject(t, out, "s-1-2-34-1111").Descriptor, "")) + assert.Equal(t, "vssgp.U222", types.GetValue(mustSubject(t, out, "s-1-2-34-2222").Descriptor, "")) +} + +func Test_resolveSIDBatch_PreservesAlreadyPrefixedDescriptor(t *testing.T) { + enriched := map[string]graph.GraphSubject{ + "vssgp.U111": subject("vssgp.U111", "SidOne"), + } + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + return &enriched, nil + }, + } + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, args identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + require.NotNil(t, args.Descriptors) + assert.Equal(t, "Microsoft.TeamFoundation.Identity;s-1-2-34-1111", *args.Descriptors) + return &[]identity.Identity{ + identityWithSubject("Microsoft.TeamFoundation.Identity;s-1-2-34-1111", "vssgp.U111", "SidOne"), + }, nil + }, + } + + inputs := []string{"Microsoft.TeamFoundation.Identity;s-1-2-34-1111"} + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveSIDBatch(context.Background(), identityClient, graphClient, inputs, out)) + + assert.Len(t, out, 1) + assert.Equal(t, "vssgp.U111", types.GetValue(mustSubject(t, out, inputs[0]).Descriptor, "")) +} + +func Test_resolveSIDBatch_EmptyResultProducesNoEnrichmentCall(t *testing.T) { + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + t.Fatal("LookupSubjects should not be called when ReadIdentities returns no identities") + return nil, nil + }, + } + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return &[]identity.Identity{}, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveSIDBatch(context.Background(), identityClient, graphClient, []string{"s-1-2-34-1111"}, out)) + assert.Empty(t, out) +} + +func Test_resolveSIDBatch_FallsBackToIdentityWhenEnrichment404s(t *testing.T) { + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + return nil, wrappedStatus(http.StatusNotFound) + }, + } + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + id := identityWithSubject("Microsoft.TeamFoundation.Identity;s-1-2-34-1111", "vssgp.U111", "SidOne") + id.IsContainer = types.ToPtr(false) + return &[]identity.Identity{id}, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveSIDBatch(context.Background(), identityClient, graphClient, []string{"s-1-2-34-1111"}, out)) + + require.Len(t, out, 1) + fallback := mustSubject(t, out, "s-1-2-34-1111") + assert.Equal(t, "vssgp.U111", types.GetValue(fallback.Descriptor, "")) + assert.Equal(t, "SidOne", types.GetValue(fallback.DisplayName, "")) + assert.Equal(t, "User", types.GetValue(fallback.SubjectKind, "")) +} + +func Test_resolveSIDBatch_PropagatesReadIdentitiesError(t *testing.T) { + graphClient := &stubGraphClient{} + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return nil, errors.New("network down") + }, + } + + out := make(map[string]*graph.GraphSubject) + err := resolveSIDBatch(context.Background(), identityClient, graphClient, []string{"s-1-2-34-1111"}, out) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to resolve") +} + +func Test_resolveSIDBatch_ErrorsWhenIdentityDescriptorNotEchoed(t *testing.T) { + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + t.Fatal("LookupSubjects must not be called when correlation fails") + return nil, nil + }, + } + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return &[]identity.Identity{ + { + SubjectDescriptor: types.ToPtr("vssgp.U111"), + }, + }, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + err := resolveSIDBatch(context.Background(), identityClient, graphClient, []string{"s-1-2-34-1111"}, out) + require.Error(t, err) + assert.Contains(t, err.Error(), "which was not requested") + assert.Empty(t, out) +} + +func Test_resolveSIDBatch_ErrorsWhenIdentityDescriptorMismatched(t *testing.T) { + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + t.Fatal("LookupSubjects must not be called when correlation fails") + return nil, nil + }, + } + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return &[]identity.Identity{ + identityWithSubject("Microsoft.TeamFoundation.Identity;s-something-else", "vssgp.U111", ""), + }, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + err := resolveSIDBatch(context.Background(), identityClient, graphClient, []string{"s-1-2-34-1111"}, out) + require.Error(t, err) + assert.Contains(t, err.Error(), "which was not requested") + assert.Contains(t, err.Error(), "Microsoft.TeamFoundation.Identity;s-something-else") + assert.Empty(t, out) +} + +func Test_resolveSearchBatch_PerInputSearchesAndBatchedEnrichment(t *testing.T) { + enriched := map[string]graph.GraphSubject{ + "aad.A1": subject("aad.A1", "Alice"), + "aad.A2": subject("aad.A2", "Bob"), + } + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, args graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + assert.ElementsMatch(t, []string{"aad.A1", "aad.A2"}, lookupDescriptors(t, args)) + return &enriched, nil + }, + } + + searchCalls := 0 + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, args identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + searchCalls++ + require.NotNil(t, args.SearchFilter) + require.NotNil(t, args.FilterValue) + assert.Contains(t, []string{"MailAddress", "AccountName"}, *args.SearchFilter, "should use one of the email/UPN filters") + switch *args.FilterValue { + case "alice@contoso.com": + return &[]identity.Identity{{SubjectDescriptor: types.ToPtr("aad.A1")}}, nil + case "bob@contoso.com": + return &[]identity.Identity{{SubjectDescriptor: types.ToPtr("aad.A2")}}, nil + } + t.Fatalf("unexpected filter value %q", *args.FilterValue) + return nil, nil + }, + } + + inputs := []string{"alice@contoso.com", "bob@contoso.com"} + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveSearchBatch(context.Background(), identityClient, graphClient, inputs, out)) + + assert.Equal(t, 2, searchCalls, "each search input triggers a separate ReadIdentities call") + assert.Len(t, out, 2) + assert.Equal(t, "Alice", types.GetValue(mustSubject(t, out, "alice@contoso.com").DisplayName, "")) + assert.Equal(t, "Bob", types.GetValue(mustSubject(t, out, "bob@contoso.com").DisplayName, "")) +} + +func Test_resolveSearchBatch_NotFoundInputSkipped(t *testing.T) { + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + t.Fatal("LookupSubjects should not be called when nothing was resolved") + return nil, nil + }, + } + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return &[]identity.Identity{}, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveSearchBatch(context.Background(), identityClient, graphClient, []string{"ghost@x.com"}, out)) + assert.Empty(t, out, "unresolved search input produces no entry, no error") +} + +func Test_resolveSearchBatch_ResolvesDescriptorFromStorageKey(t *testing.T) { + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + return &map[string]graph.GraphSubject{ + "vssgp.STORAGE": subject("vssgp.STORAGE", "StorageUser"), + }, nil + }, + getDescriptorFunc: func(_ context.Context, _ graph.GetDescriptorArgs) (*graph.GraphDescriptorResult, error) { + return &graph.GraphDescriptorResult{Value: types.ToPtr("vssgp.STORAGE")}, nil + }, + } + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return &[]identity.Identity{identityWithStorageKey("00000000-0000-0000-0000-000000000001")}, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveSearchBatch(context.Background(), identityClient, graphClient, []string{"alice@x.com"}, out)) + + require.Len(t, out, 1) + assert.Equal(t, "vssgp.STORAGE", types.GetValue(mustSubject(t, out, "alice@x.com").Descriptor, "")) +} + +func Test_resolveSearchBatch_EmptyGetDescriptorValueSkipsInput(t *testing.T) { + graphClient := &stubGraphClient{ + lookupSubjectsFunc: func(_ context.Context, _ graph.LookupSubjectsArgs) (*map[string]graph.GraphSubject, error) { + t.Fatal("LookupSubjects should not be called when GetDescriptor returned empty") + return nil, nil + }, + getDescriptorFunc: func(_ context.Context, _ graph.GetDescriptorArgs) (*graph.GraphDescriptorResult, error) { + return &graph.GraphDescriptorResult{Value: types.ToPtr("")}, nil + }, + } + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return &[]identity.Identity{identityWithStorageKey("00000000-0000-0000-0000-000000000001")}, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + require.NoError(t, resolveSearchBatch(context.Background(), identityClient, graphClient, []string{"alice@x.com"}, out)) + assert.Empty(t, out, "empty GetDescriptor result should not produce an entry") +} + +func Test_resolveSearchBatch_PropagatesSearchError(t *testing.T) { + graphClient := &stubGraphClient{} + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return nil, wrappedStatus(http.StatusInternalServerError) + }, + } + + out := make(map[string]*graph.GraphSubject) + err := resolveSearchBatch(context.Background(), identityClient, graphClient, []string{"alice@x.com"}, out) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to resolve member") +} + +func Test_resolveSearchBatch_AmbiguousSearchResultsError(t *testing.T) { + graphClient := &stubGraphClient{} + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, _ identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + return &[]identity.Identity{ + {SubjectDescriptor: types.ToPtr("aad.A1")}, + {SubjectDescriptor: types.ToPtr("aad.A2")}, + }, nil + }, + } + + out := make(map[string]*graph.GraphSubject) + err := resolveSearchBatch(context.Background(), identityClient, graphClient, []string{"alice@x.com"}, out) + require.Error(t, err) + assert.Contains(t, err.Error(), "multiple identities found") +} + +func Test_resolveIdentity_PrefixedSIDGoesToSIDBranch(t *testing.T) { + identityClient := &stubIdentityClient{ + readIdentitiesFunc: func(_ context.Context, args identity.ReadIdentitiesArgs) (*[]identity.Identity, error) { + require.NotNil(t, args.Descriptors) + assert.Nil(t, args.SubjectDescriptors) + assert.Equal(t, "Microsoft.TeamFoundation.Identity;s-1-2-34-9999", *args.Descriptors) + return &[]identity.Identity{{SubjectDescriptor: types.ToPtr("vssgp.U999")}}, nil + }, + } + + got, err := resolveIdentity(context.Background(), identityClient, "Microsoft.TeamFoundation.Identity;s-1-2-34-9999") + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, "vssgp.U999", types.GetValue(got.SubjectDescriptor, "")) +} + +func Test_isNotFoundError(t *testing.T) { + t.Run("nil error returns false", func(t *testing.T) { + assert.False(t, isNotFoundError(nil)) + }) + t.Run("non-wrapped error returns false", func(t *testing.T) { + assert.False(t, isNotFoundError(errors.New("boom"))) + }) + t.Run("404 status returns true", func(t *testing.T) { + assert.True(t, isNotFoundError(wrappedStatus(http.StatusNotFound))) + }) + t.Run("other status returns false", func(t *testing.T) { + assert.False(t, isNotFoundError(wrappedStatus(http.StatusInternalServerError))) + }) +} diff --git a/internal/mocks/extension_mock.go b/internal/mocks/extension_mock.go index 6ca4054a..29770675 100644 --- a/internal/mocks/extension_mock.go +++ b/internal/mocks/extension_mock.go @@ -134,3 +134,18 @@ func (mr *MockAzDOExtensionMockRecorder) ResolveSubject(ctx, member any) *gomock mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveSubject", reflect.TypeOf((*MockAzDOExtension)(nil).ResolveSubject), ctx, member) } + +// ResolveSubjects mocks base method. +func (m *MockAzDOExtension) ResolveSubjects(ctx context.Context, members []string) (map[string]*graph.GraphSubject, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResolveSubjects", ctx, members) + ret0, _ := ret[0].(map[string]*graph.GraphSubject) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ResolveSubjects indicates an expected call of ResolveSubjects. +func (mr *MockAzDOExtensionMockRecorder) ResolveSubjects(ctx, members any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveSubjects", reflect.TypeOf((*MockAzDOExtension)(nil).ResolveSubjects), ctx, members) +} From 5e1f959a6552a08e005fc086ed690062d03d8946 Mon Sep 17 00:00:00 2001 From: Codex CLI Date: Sun, 7 Jun 2026 21:14:02 +0000 Subject: [PATCH 3/5] =?UTF-8?q?feat(team):=20=E2=9C=A8=20implement=20team?= =?UTF-8?q?=20member=20add=20command?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the `add` subcommand to `team member` for adding users or groups to a team. Resolve members by email, descriptor, principal name, SID, or identity ID. Check existing membership before adding and output results as JSON or a table. --- internal/cmd/team/member/add/add.go | 269 ++++++++++++++++++++++++++++ internal/cmd/team/member/member.go | 2 + 2 files changed, 271 insertions(+) create mode 100644 internal/cmd/team/member/add/add.go diff --git a/internal/cmd/team/member/add/add.go b/internal/cmd/team/member/add/add.go new file mode 100644 index 00000000..bf2d9fe1 --- /dev/null +++ b/internal/cmd/team/member/add/add.go @@ -0,0 +1,269 @@ +package add + +import ( + "errors" + "fmt" + "net/http" + "strings" + + "github.com/MakeNowJust/heredoc/v2" + "github.com/microsoft/azure-devops-go-api/azuredevops/v7" + "github.com/microsoft/azure-devops-go-api/azuredevops/v7/core" + "github.com/microsoft/azure-devops-go-api/azuredevops/v7/graph" + "github.com/spf13/cobra" + "github.com/tmeckel/azdo-cli/internal/cmd/util" + "github.com/tmeckel/azdo-cli/internal/types" + "go.uber.org/zap" +) + +type opts struct { + targetArg string + users []string + exporter util.Exporter +} + +type addResultView struct { + MemberDescriptor *string `json:"memberDescriptor,omitempty"` + MemberDisplayName *string `json:"memberDisplayName,omitempty"` + MemberOrigin *string `json:"memberOrigin,omitempty"` + MemberOriginID *string `json:"memberOriginId,omitempty"` + Status *string `json:"status,omitempty"` +} + +type addView struct { + TeamName *string `json:"teamName,omitempty"` + Results []addResultView `json:"results"` +} + +type resolved struct { + input string + descriptor string + displayName string + origin string + originID string +} + +func resultView(r resolved, status string) addResultView { + return addResultView{ + MemberDescriptor: types.ToPtr(r.descriptor), + MemberDisplayName: types.ToPtr(r.displayName), + MemberOrigin: types.ToPtr(r.origin), + MemberOriginID: types.ToPtr(r.originID), + Status: types.ToPtr(status), + } +} + +func NewCmd(ctx util.CmdContext) *cobra.Command { + o := &opts{} + + cmd := &cobra.Command{ + Use: "add [ORGANIZATION/]PROJECT/TEAM", + Short: "Add one or more members to a team.", + Long: heredoc.Doc(` + Add one or more users or groups as members of a team. + + The positional argument accepts the team's project and team name in the + form [ORGANIZATION/]PROJECT/TEAM. + `), + Example: heredoc.Doc(` + # Add a user by email + azdo team member add Fabrikam/FabrikamEngineering/MyTeam --user user@example.com + + # Add multiple users in a single invocation + azdo team member add Fabrikam/MyProject/MyTeam -u alice@contoso.com -u bob@contoso.com + + # Add a user by subject descriptor + azdo team member add MyOrg/Fabrikam/MyTeam --user vssgp.Uy0xLTItMw== + `), + Args: util.ExactArgs(1, "team argument required"), + Aliases: []string{ + "a", + }, + RunE: func(cmd *cobra.Command, args []string) error { + o.targetArg = args[0] + return runAdd(ctx, o) + }, + } + + cmd.Flags().StringSliceVarP(&o.users, "user", "u", nil, "Members to add. Accepts a descriptor, email, principal name, SID, or identity ID. Pass the flag multiple times to add several members.") + _ = cmd.MarkFlagRequired("user") + util.AddJSONFlags(cmd, &o.exporter, []string{ + "teamName", + "results", + "memberDescriptor", + "memberDisplayName", + "memberOrigin", + "memberOriginId", + "status", + }) + + return cmd +} + +func runAdd(ctx util.CmdContext, o *opts) error { + ios, err := ctx.IOStreams() + if err != nil { + return err + } + + ios.StartProgressIndicator() + defer ios.StopProgressIndicator() + + scope, err := util.ParseProjectTargetWithDefaultOrganization(ctx, o.targetArg) + if err != nil { + return err + } + + zap.L().Debug( + "resolving team for member add", + zap.String("organization", scope.Organization), + zap.String("project", scope.Project), + zap.String("team", scope.Targets[0]), + ) + + coreClient, err := ctx.ClientFactory().Core(ctx.Context(), scope.Organization) + if err != nil { + return fmt.Errorf("failed to create Core client: %w", err) + } + + team, err := coreClient.GetTeam(ctx.Context(), core.GetTeamArgs{ + ProjectId: types.ToPtr(scope.Project), + TeamId: types.ToPtr(scope.Targets[0]), + ExpandIdentity: types.ToPtr(true), + }) + if err != nil { + return fmt.Errorf("failed to resolve team %q in project %q: %w", scope.Targets[0], scope.Project, err) + } + if team == nil || team.Identity == nil || types.GetValue(team.Identity.SubjectDescriptor, "") == "" { + return fmt.Errorf("team has no underlying descriptor (Identity.SubjectDescriptor is empty)") + } + + teamGroupDescriptor := types.GetValue(team.Identity.SubjectDescriptor, "") + teamName := types.GetValue(team.Name, "") + + extensionsClient, err := ctx.ClientFactory().Extensions(ctx.Context(), scope.Organization) + if err != nil { + return fmt.Errorf("failed to create Extensions client: %w", err) + } + + graphClient, err := ctx.ClientFactory().Graph(ctx.Context(), scope.Organization) + if err != nil { + return fmt.Errorf("failed to create Graph client: %w", err) + } + + // Phase 1: resolve all members. ResolveSubjects deduplicates and trims. + resolvedSubjects, err := extensionsClient.ResolveSubjects(ctx.Context(), o.users) + if err != nil { + return fmt.Errorf("failed to resolve %d member(s): %w", len(o.users), err) + } + + seen := make(map[string]struct{}) + var unresolved []string + resolvedMembers := make([]resolved, 0, len(o.users)) + for _, rawMember := range o.users { + t := strings.TrimSpace(rawMember) + if t == "" { + continue + } + if _, ok := seen[t]; ok { + continue + } + seen[t] = struct{}{} + subject, ok := resolvedSubjects[t] + if !ok { + unresolved = append(unresolved, t) + continue + } + resolvedMembers = append(resolvedMembers, resolved{ + input: t, + descriptor: types.GetValue(subject.Descriptor, ""), + displayName: types.GetValue(subject.DisplayName, ""), + origin: types.GetValue(subject.Origin, ""), + originID: types.GetValue(subject.LegacyDescriptor, ""), + }) + } + if len(unresolved) > 0 { + return fmt.Errorf("failed to resolve %d member(s): %s", len(unresolved), strings.Join(unresolved, ", ")) + } + + // Phase 2: check membership existence and add each resolved member. + results := make([]addResultView, 0, len(resolvedMembers)) + + for _, r := range resolvedMembers { + zap.L().Debug( + "checking existing membership", + zap.String("memberDescriptor", r.descriptor), + ) + + err = graphClient.CheckMembershipExistence(ctx.Context(), graph.CheckMembershipExistenceArgs{ + ContainerDescriptor: types.ToPtr(teamGroupDescriptor), + SubjectDescriptor: types.ToPtr(r.descriptor), + }) + if err == nil { + results = append(results, resultView(r, "already member")) + continue + } + + var wrapped *azuredevops.WrappedError + if !errors.As(err, &wrapped) || wrapped == nil || wrapped.StatusCode == nil || *wrapped.StatusCode != http.StatusNotFound { + return fmt.Errorf("failed to check membership for %q: %w", r.input, err) + } + + zap.L().Debug( + "adding membership", + zap.String("memberDescriptor", r.descriptor), + ) + + if _, err := graphClient.AddMembership(ctx.Context(), graph.AddMembershipArgs{ + ContainerDescriptor: types.ToPtr(teamGroupDescriptor), + SubjectDescriptor: types.ToPtr(r.descriptor), + }); err != nil { + var addErr *azuredevops.WrappedError + if errors.As(err, &addErr) && addErr != nil && addErr.StatusCode != nil && *addErr.StatusCode == http.StatusConflict { + results = append(results, resultView(r, "already member")) + continue + } + return fmt.Errorf("failed to add member %q: %w", r.input, err) + } + + results = append(results, resultView(r, "added")) + } + + ios.StopProgressIndicator() + + view := addView{ + TeamName: types.ToPtr(teamName), + Results: results, + } + + if o.exporter != nil { + if err := o.exporter.Write(ios, view); err != nil { + return err + } + } else { + tp, err := ctx.Printer("list") + if err != nil { + return err + } + + tp.AddColumns("MEMBER", "DESCRIPTOR", "STATUS") + tp.EndRow() + + for _, r := range results { + display := types.GetValue(r.MemberDisplayName, "") + if display == "" { + display = types.GetValue(r.MemberDescriptor, "") + } + tp.AddField(display) + tp.AddField(types.GetValue(r.MemberDescriptor, "")) + tp.AddField(types.GetValue(r.Status, "")) + tp.EndRow() + } + + if err := tp.Render(); err != nil { + return err + } + } + + return nil +} diff --git a/internal/cmd/team/member/member.go b/internal/cmd/team/member/member.go index 792d1c67..1bc74d39 100644 --- a/internal/cmd/team/member/member.go +++ b/internal/cmd/team/member/member.go @@ -2,6 +2,7 @@ package member import ( "github.com/spf13/cobra" + "github.com/tmeckel/azdo-cli/internal/cmd/team/member/add" "github.com/tmeckel/azdo-cli/internal/cmd/team/member/list" "github.com/tmeckel/azdo-cli/internal/cmd/util" ) @@ -12,6 +13,7 @@ func NewCmd(ctx util.CmdContext) *cobra.Command { Short: "Manage members of a team.", } + cmd.AddCommand(add.NewCmd(ctx)) cmd.AddCommand(list.NewCmd(ctx)) return cmd From e80738af3cd8dd6f8918738356cbf6dabf8fe76c Mon Sep 17 00:00:00 2001 From: Codex CLI Date: Sun, 7 Jun 2026 21:15:55 +0000 Subject: [PATCH 4/5] =?UTF-8?q?test(team):=20=F0=9F=A7=AAadd=20unit=20test?= =?UTF-8?q?s=20for=20team=20member=20add=20command?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add comprehensive test coverage for single and bulk member addition, including happy path, error handling, JSON output, deduplication, and input order preservation. --- internal/cmd/team/member/add/add_test.go | 531 +++++++++++++++++++++++ 1 file changed, 531 insertions(+) create mode 100644 internal/cmd/team/member/add/add_test.go diff --git a/internal/cmd/team/member/add/add_test.go b/internal/cmd/team/member/add/add_test.go new file mode 100644 index 00000000..3d82fd62 --- /dev/null +++ b/internal/cmd/team/member/add/add_test.go @@ -0,0 +1,531 @@ +package add + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "strings" + "testing" + + "github.com/microsoft/azure-devops-go-api/azuredevops/v7" + "github.com/microsoft/azure-devops-go-api/azuredevops/v7/core" + "github.com/microsoft/azure-devops-go-api/azuredevops/v7/graph" + "github.com/microsoft/azure-devops-go-api/azuredevops/v7/identity" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/tmeckel/azdo-cli/internal/iostreams" + "github.com/tmeckel/azdo-cli/internal/mocks" + "github.com/tmeckel/azdo-cli/internal/printer" + "github.com/tmeckel/azdo-cli/internal/types" + "go.uber.org/mock/gomock" +) + +type fakeDeps struct { + cmd *mocks.MockCmdContext + clientFact *mocks.MockClientFactory + coreClient *mocks.MockCoreClient + graphClient *mocks.MockGraphClient + extClient *mocks.MockAzDOExtension + prompter *mocks.MockPrompter + config *mocks.MockConfig + authCfg *mocks.MockAuthConfig +} + +func setupFakeDeps(t *testing.T, organization string) (*fakeDeps, *bytes.Buffer) { + t.Helper() + + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + ios, _, out, _ := iostreams.Test() + ios.SetStdoutTTY(false) + ios.SetStderrTTY(false) + + tp, err := printer.NewTablePrinter(out, false, 200) + require.NoError(t, err) + + deps := &fakeDeps{ + cmd: mocks.NewMockCmdContext(ctrl), + clientFact: mocks.NewMockClientFactory(ctrl), + coreClient: mocks.NewMockCoreClient(ctrl), + graphClient: mocks.NewMockGraphClient(ctrl), + extClient: mocks.NewMockAzDOExtension(ctrl), + prompter: mocks.NewMockPrompter(ctrl), + config: mocks.NewMockConfig(ctrl), + authCfg: mocks.NewMockAuthConfig(ctrl), + } + + deps.cmd.EXPECT().IOStreams().Return(ios, nil).AnyTimes() + deps.cmd.EXPECT().Context().Return(context.Background()).AnyTimes() + deps.cmd.EXPECT().ClientFactory().Return(deps.clientFact).AnyTimes() + deps.cmd.EXPECT().Printer(gomock.Any()).Return(tp, nil).AnyTimes() + deps.clientFact.EXPECT().Core(gomock.Any(), organization).Return(deps.coreClient, nil).AnyTimes() + deps.clientFact.EXPECT().Graph(gomock.Any(), organization).Return(deps.graphClient, nil).AnyTimes() + deps.clientFact.EXPECT().Extensions(gomock.Any(), organization).Return(deps.extClient, nil).AnyTimes() + + return deps, out +} + +func teamResult(desc, name string) *core.WebApiTeam { + return &core.WebApiTeam{ + Name: &name, + Identity: &identity.Identity{ + SubjectDescriptor: &desc, + }, + } +} + +func subject(desc, displayName, origin, legacyDesc string) *graph.GraphSubject { + return &graph.GraphSubject{ + Descriptor: &desc, + DisplayName: &displayName, + Origin: &origin, + LegacyDescriptor: &legacyDesc, + } +} + +func notFound() error { + return &azuredevops.WrappedError{ + StatusCode: types.ToPtr(http.StatusNotFound), + } +} + +func conflict() error { + return &azuredevops.WrappedError{ + StatusCode: types.ToPtr(http.StatusConflict), + } +} + +func apiError(code int) error { + return &azuredevops.WrappedError{ + StatusCode: types.ToPtr(code), + } +} + +// --- Single-member tests --- + +func TestAdd_SingleMember_HappyPath(t *testing.T) { + deps, buf := setupFakeDeps(t, "myOrg") + + teamDesc := "vssgp.Uy0xLTkt" + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult(teamDesc, "My Team"), nil) + + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, members []string) (map[string]*graph.GraphSubject, error) { + assert.Equal(t, []string{"user@example.com"}, members) + return map[string]*graph.GraphSubject{ + "user@example.com": subject("aad.YR5kM", "Alice", "aad", "legacy-aad"), + }, nil + }) + + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()) + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "--user", "user@example.com"}) + err := cmd.Execute() + require.NoError(t, err) + + output := buf.String() + assert.Contains(t, output, "Alice") + assert.Contains(t, output, "aad.YR5kM") + assert.Contains(t, output, "added") + // No "Group" word in output + assert.NotContains(t, output, "Group") +} + +func TestAdd_SingleMember_AlreadyMember(t *testing.T) { + deps, buf := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{ + "user@example.com": subject("aad.Y", "Alice", "aad", "l-aad"), + }, nil) + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(nil) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "--user", "user@example.com"}) + err := cmd.Execute() + require.NoError(t, err) + + assert.Contains(t, buf.String(), "already member") +} + +func TestAdd_SingleMember_Race_409OnAdd(t *testing.T) { + deps, buf := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{ + "user@example.com": subject("aad.Y", "Alice", "aad", "l-aad"), + }, nil) + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()) + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(nil, conflict()) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "--user", "user@example.com"}) + err := cmd.Execute() + require.NoError(t, err) + + assert.Contains(t, buf.String(), "already member") +} + +func TestAdd_TeamNotFound(t *testing.T) { + deps, _ := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(nil, fmt.Errorf("team not found")) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/NoTeam", "--user", "user@example.com"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "team not found") +} + +func TestAdd_TeamHasNoIdentity(t *testing.T) { + deps, _ := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(&core.WebApiTeam{Name: types.ToPtr("NoIdentity")}, nil) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "--user", "user@example.com"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "no underlying descriptor") + assert.NotContains(t, err.Error(), "Group") +} + +func TestAdd_MemberNotFound(t *testing.T) { + deps, _ := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + // Empty map signals the member could not be resolved (per-input absence, no catastrophic error). + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{}, nil) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "--user", "ghost@x.com"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to resolve") + assert.Contains(t, err.Error(), "ghost@x.com") +} + +func TestAdd_MemberResolutionError(t *testing.T) { + deps, _ := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + // Catastrophic error: add command must bubble it up. + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(nil, errors.New("network down")) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "--user", "ghost@x.com"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to resolve") +} + +func TestAdd_TargetArg_ParsesOrgSlashProjectSlashTeam(t *testing.T) { + deps, _ := setupFakeDeps(t, "myCustomOrg") + + var captured core.GetTeamArgs + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, args core.GetTeamArgs) (*core.WebApiTeam, error) { + captured = args + return teamResult("vssgp.X", "Team"), nil + }) + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{ + "u@x.com": subject("aad.Y", "U", "aad", "l"), + }, nil) + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()) + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myCustomOrg/MyProject/MyTeam", "--user", "u@x.com"}) + err := cmd.Execute() + require.NoError(t, err) + + assert.Equal(t, "MyProject", *captured.ProjectId) + assert.Equal(t, "MyTeam", *captured.TeamId) +} + +func TestAdd_JSONOutput_EnvelopeShape(t *testing.T) { + deps, buf := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.Uy0xLTkt", "My Team"), nil) + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{ + "user@example.com": subject("aad.YR5kM", "Alice", "aad", "legacy-aad"), + }, nil) + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()) + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "--user", "user@example.com", "--json"}) + err := cmd.Execute() + require.NoError(t, err) + + var parsed struct { + TeamName string `json:"teamName"` + Results []json.RawMessage `json:"results"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &parsed)) + assert.Equal(t, "My Team", parsed.TeamName) + assert.Len(t, parsed.Results, 1) +} + +func TestAdd_JSONFieldFilter(t *testing.T) { + deps, buf := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{ + "user@example.com": subject("aad.Y", "Alice", "aad", "l"), + }, nil) + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()) + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "--user", "user@example.com", "--json=teamName"}) + err := cmd.Execute() + require.NoError(t, err) + + output := buf.String() + assert.Contains(t, output, "teamName") + assert.Contains(t, output, "My Team") + assert.NotContains(t, output, "results") + assert.NotContains(t, output, "memberDescriptor") +} + +func TestAdd_MissingUserFlag(t *testing.T) { + deps, _ := setupFakeDeps(t, "myOrg") + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "required flag") +} + +// --- Bulk tests --- + +func TestAdd_Bulk_MultipleMembersAdded(t *testing.T) { + deps, buf := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + + // Single batched call covering all three members + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, members []string) (map[string]*graph.GraphSubject, error) { + assert.ElementsMatch(t, []string{"alice@c.com", "bob@c.com", "charlie@c.com"}, members) + return map[string]*graph.GraphSubject{ + "alice@c.com": subject("aad.1", "Alice", "aad", "la"), + "bob@c.com": subject("aad.2", "Bob", "aad", "lb"), + "charlie@c.com": subject("aad.3", "Charlie", "aad", "lc"), + }, nil + }) + + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()).Times(3) + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil).Times(3) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "-u", "alice@c.com", "-u", "bob@c.com", "-u", "charlie@c.com"}) + err := cmd.Execute() + require.NoError(t, err) + + output := buf.String() + assert.Contains(t, output, "Alice") + assert.Contains(t, output, "Bob") + assert.Contains(t, output, "Charlie") + + // Input order preserved + alicePos := bytes.Index(buf.Bytes(), []byte("Alice")) + bobPos := bytes.Index(buf.Bytes(), []byte("Bob")) + charliePos := bytes.Index(buf.Bytes(), []byte("Charlie")) + assert.True(t, alicePos < bobPos && bobPos < charliePos, "input order not preserved") +} + +func TestAdd_Bulk_MixedResults(t *testing.T) { + deps, _ := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{ + "alice@c.com": subject("aad.1", "Alice", "aad", "la"), + "bob@c.com": subject("aad.2", "Bob", "aad", "lb"), + "error@c.com": subject("aad.3", "Error", "aad", "le"), + }, nil) + + gomock.InOrder( + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()), + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil), + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(nil), + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()), + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(nil, apiError(http.StatusInternalServerError)), + ) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "-u", "alice@c.com", "-u", "bob@c.com", "-u", "error@c.com"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to add member") +} + +func TestAdd_Bulk_DedupePreservesInputOrder(t *testing.T) { + deps, buf := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + + // ResolveSubjects receives only the deduplicated inputs (alice once, bob once) + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, members []string) (map[string]*graph.GraphSubject, error) { + assert.ElementsMatch(t, []string{"alice@c.com", "bob@c.com", "alice@c.com"}, members) + return map[string]*graph.GraphSubject{ + "alice@c.com": subject("aad.1", "Alice", "aad", "la"), + "bob@c.com": subject("aad.2", "Bob", "aad", "lb"), + }, nil + }) + + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()).Times(2) + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil).Times(2) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "-u", "alice@c.com", "-u", "bob@c.com", "-u", "alice@c.com"}) + err := cmd.Execute() + require.NoError(t, err) + + // Only Alice and Bob in output, in order + aliceCount := strings.Count(buf.String(), "Alice") + assert.Equal(t, 1, aliceCount, "Alice should appear only once") + + alicePos := bytes.Index(buf.Bytes(), []byte("Alice")) + bobPos := bytes.Index(buf.Bytes(), []byte("Bob")) + assert.True(t, alicePos < bobPos, "input order not preserved") +} + +func TestAdd_Bulk_ExitCodePartialSuccess(t *testing.T) { + deps, _ := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{ + "alice@c.com": subject("aad.1", "Alice", "aad", "la"), + "error@c.com": subject("aad.2", "Error", "aad", "le"), + }, nil) + + gomock.InOrder( + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()), + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil), + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()), + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(nil, apiError(http.StatusInternalServerError)), + ) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "-u", "alice@c.com", "-u", "error@c.com"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to add member") +} + +func TestAdd_Bulk_JSONEnvelopeShape(t *testing.T) { + deps, buf := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{ + "alice@c.com": subject("aad.1", "Alice", "aad", "la"), + "bob@c.com": subject("aad.2", "Bob", "aad", "lb"), + }, nil) + + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()).Times(2) + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil).Times(2) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "-u", "alice@c.com", "-u", "bob@c.com", "--json"}) + err := cmd.Execute() + require.NoError(t, err) + + var parsed struct { + TeamName string `json:"teamName"` + Results []json.RawMessage `json:"results"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &parsed)) + assert.Equal(t, "My Team", parsed.TeamName) + assert.Len(t, parsed.Results, 2) +} + +func TestAdd_Bulk_TableHasNoGroupColumn(t *testing.T) { + deps, buf := setupFakeDeps(t, "myOrg") + + deps.coreClient.EXPECT().GetTeam(gomock.Any(), gomock.Any()). + Return(teamResult("vssgp.X", "My Team"), nil) + deps.extClient.EXPECT().ResolveSubjects(gomock.Any(), gomock.Any()). + Return(map[string]*graph.GraphSubject{ + "user@example.com": subject("aad.Y", "Alice", "aad", "la"), + }, nil) + deps.graphClient.EXPECT().CheckMembershipExistence(gomock.Any(), gomock.Any()). + Return(notFound()) + deps.graphClient.EXPECT().AddMembership(gomock.Any(), gomock.Any()). + Return(&graph.GraphMembership{}, nil) + + cmd := NewCmd(deps.cmd) + cmd.SetArgs([]string{"myOrg/myProject/MyTeam", "--user", "user@example.com"}) + err := cmd.Execute() + require.NoError(t, err) + + output := buf.String() + // No "Group" word anywhere in output (regression guard for decision #13) + assert.NotContains(t, output, "Group") + // Output has data (Alice, descriptor, status) + assert.Contains(t, output, "Alice") + assert.Contains(t, output, "aad.Y") + assert.Contains(t, output, "added") +} From ad6540fbadabe95eb557577ab58dbf7c6bf82fa3 Mon Sep 17 00:00:00 2001 From: Codex CLI Date: Sun, 7 Jun 2026 21:17:29 +0000 Subject: [PATCH 5/5] =?UTF-8?q?docs(team):=20=F0=9F=93=84=20add=20document?= =?UTF-8?q?ation=20for=20team=20member=20add?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/azdo_help_reference.md | 17 +++++++++++ docs/azdo_team_member.md | 1 + docs/azdo_team_member_add.md | 56 ++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+) create mode 100644 docs/azdo_team_member_add.md diff --git a/docs/azdo_help_reference.md b/docs/azdo_help_reference.md index 7a4721bc..84f541fb 100644 --- a/docs/azdo_help_reference.md +++ b/docs/azdo_help_reference.md @@ -1362,6 +1362,23 @@ ls, l Manage members of a team. +#### `azdo team member add [ORGANIZATION/]PROJECT/TEAM [flags]` + +Add one or more members to a team. + +``` +-q, --jq expression Filter JSON output using a jq expression + --json fields[=*] Output JSON with the specified fields. Prefix a field with '-' to exclude it. +-t, --template string Format JSON output using a Go template; see "azdo help formatting" +-u, --user strings Members to add. Accepts a descriptor, email, principal name, SID, or identity ID. Pass the flag multiple times to add several members. +``` + +Aliases + +``` +a +``` + #### `azdo team member list [ORGANIZATION/]PROJECT/TEAM [flags]` List members of a team. diff --git a/docs/azdo_team_member.md b/docs/azdo_team_member.md index c77d52c7..bf2a6aca 100644 --- a/docs/azdo_team_member.md +++ b/docs/azdo_team_member.md @@ -4,6 +4,7 @@ Manage members of a team. ### Available commands +* [azdo team member add](./azdo_team_member_add.md) * [azdo team member list](./azdo_team_member_list.md) ### See also diff --git a/docs/azdo_team_member_add.md b/docs/azdo_team_member_add.md new file mode 100644 index 00000000..546fbaf1 --- /dev/null +++ b/docs/azdo_team_member_add.md @@ -0,0 +1,56 @@ +## Command `azdo team member add` + +``` +azdo team member add [ORGANIZATION/]PROJECT/TEAM [flags] +``` + +Add one or more users or groups as members of a team. + +The positional argument accepts the team's project and team name in the +form [ORGANIZATION/]PROJECT/TEAM. + + +### Options + + +* `-q`, `--jq` `expression` + + Filter JSON output using a jq expression + +* `--json` `fields` + + Output JSON with the specified fields. Prefix a field with '-' to exclude it. + +* `-t`, `--template` `string` + + Format JSON output using a Go template; see "azdo help formatting" + +* `-u`, `--user` `strings` + + Members to add. Accepts a descriptor, email, principal name, SID, or identity ID. Pass the flag multiple times to add several members. + + +### ALIASES + +- `a` + +### JSON Fields + +`memberDescriptor`, `memberDisplayName`, `memberOrigin`, `memberOriginId`, `results`, `status`, `teamName` + +### Examples + +```bash +# Add a user by email +azdo team member add Fabrikam/FabrikamEngineering/MyTeam --user user@example.com + +# Add multiple users in a single invocation +azdo team member add Fabrikam/MyProject/MyTeam -u alice@contoso.com -u bob@contoso.com + +# Add a user by subject descriptor +azdo team member add MyOrg/Fabrikam/MyTeam --user vssgp.Uy0xLTItMw== +``` + +### See also + +* [azdo team member](./azdo_team_member.md)