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) 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/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) + } + }) + } +} 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/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") +} 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 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) +}