Repository navigation
Add owner-based identity selection - #14923
Draft
williammartin wants to merge 2 commits into
Draft
williammartin wants to merge 2 commits into
williammartin wants to merge 2 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 965836ef-19ac-4473-ba1f-e326ce0f106b
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 965836ef-19ac-4473-ba1f-e326ce0f106b
2 of 6 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved identity-resolution defects break scope precedence and destination-specific Git credentials, while GraphQL handling rejects valid block strings and ignores owner selection from file or stdin input.
4 open issues
What changed in this PR
This draft adds owner-based identity selection to GitHub CLI’s multi-account prototype, extending the preceding directory-binding layer with an owner fallback below repository and directory choices. It remains explicitly marked DO NOT MERGE.
Changes:
- Add owner binding storage, command flags, cleanup reporting, and native Git include conditions.
- Pass API request context and Git credential paths into account selection.
- Extend tests and add an offline replay for owner precedence and ambiguity.
| File | Description |
|---|---|
| script/multiaccount-review/04-owners.sh | Replay owner precedence and conflicting-owner scenarios. |
| pkg/cmd/repo/create/identity_test.go | Verify directory identity during ownerless repository creation. |
| pkg/cmd/auth/use/use.go | Add owner binding and inspection options. |
| pkg/cmd/auth/use/use_test.go | Test scope storage and flag validation. |
| pkg/cmd/auth/identity/delete.go | Include owner bindings in deletion descriptions. |
| pkg/cmd/auth/identity/delete_test.go | Cover owner cleanup and deletion warnings. |
| pkg/cmd/auth/gitcredential/identity_test.go | Exercise owner-based HTTPS credentials. |
| pkg/cmd/auth/gitcredential/helper.go | Forward repository paths to credential resolution. |
| pkg/cmd/auth/gitcredential/helper_test.go | Adapt the credential test double. |
| internal/identity/store.go | Store and resolve owner bindings; generate Git conditions. |
| internal/identity/store_test.go | Cover precedence, capitalization, and owner conflicts. |
| internal/identity/ssh_test.go | Exercise owner-based SSH routing. |
| internal/identity/request.go | Extract owner context from REST and GraphQL requests. |
| internal/identity/request_test.go | Test request-based selection and environment-token refusal. |
| internal/identity/hardening_test.go | Cover stale-scope removal and Git version requirements. |
| internal/identity/credentials.go | Pass HTTP requests into token resolution. |
| internal/identity/account_test.go | Test owner-specific SSH routes for account overrides. |
| internal/gh/gh.go | Extend authentication interfaces with request context. |
| internal/config/config.go | Use request owners and credential paths for selection. |
| internal/config/active_account_test.go | Update credential-resolution calls. |
| api/identity_test.go | Test owner selection for API and clone lookups. |
| api/identity_gateway_test.go | Test canonical-host credentials through the identity resolver. |
| api/http_client.go | Forward requests to token resolvers. |
| api/auth_resolver_test.go | Adapt tests to request-aware resolver signatures. |
Review effort: Balanced
Comment on lines
+42
to
+43
| if owner != "" && value != "" && !strings.EqualFold(value, owner) { | ||
| return errors.New("GraphQL request has multiple owners; split the request by owner") |
| } | ||
| var lex scanner.Scanner | ||
| lex.Init(strings.NewReader(query.Query)) | ||
| lex.Mode = scanner.ScanIdents | scanner.ScanStrings |
Comment on lines
+621
to
+622
| condition := "hasconfig:remote.*.url:" + prefix + ownerPattern(binding.Owner) + "/**" | ||
| fmt.Fprintf(&config, "[includeIf %s]\n\tpath = %s\n", gitQuote(condition), gitQuote(path)) |
Comment on lines
+25
to
+26
| if !strings.HasSuffix(req.URL.Path, "/graphql") || req.GetBody == nil { | ||
| return "", nil |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow on from #14922
A repository owner can be a more useful account-selection rule than where the repository happens to be cloned. This PR adds owner bindings as a fallback behind repository and directory choices.
Layer 4 of an eight-part prototype review stack. This is a way to review the design and implementation in smaller pieces, not a proposal to merge the current stack as-is.
How did you test this change?
I inspected both pages of the actual owner binding file and checked the selected real account. I exercised owner fallback, stronger directory and repository choices, and refusal when remotes imply conflicting bound owners.
An owner binding matches repository remote URLs
multiaccount-layer-4-01-Wiring---Layer-4---Owner-binding-file.mp4
A repository owner provides a fallback, independent of the selected account login
multiaccount-layer-4-02-A-repository-owner-provides-a-fallback-independent-of-the-selected-account-login.mp4
Conflicting remote-owner choices refuse to guess; an explicit repository binding resolves the ambiguity
multiaccount-layer-4-03-Conflicting-remote-owner-choices-refuse-to-guess-an-explicit-repository-binding-resolves-the-ambiguity.mp4
Directory and repository choices outrank owner fallback, and removal reveals the next surviving choice
multiaccount-layer-4-04-Directory-and-repository-choices-outrank-owner-fallback-and-removal-reveals-the-next-surviving-choice.mp4
Key points
Native
includeIfconditions usehasconfig:remote.*.urlto load the identity for matching remotes. Separate patterns cover HTTPS and SSH URL forms without rewriting the saved remote.Character classes such as
[gG]and[wW]are intentional. Git matches these URL globs case-sensitively, but GitHub hostnames and owner names are case-insensitive. The generated patterns therefore match mixed-case remotes without changing their spelling.Notes for reviewers
Start with owner extraction and precedence, then compare API request context with the native Git conditions. Requests involving competing owner choices fail rather than guess. Owner selection does not override an explicit repository or directory choice.
The recordings use the product code at this layer, not a later stack endpoint. The included offline replay scripts are supplementary and do not reproduce the live-account recordings.
Offline replay on macOS
Build this layer, then run its synthetic scenario with an absolute executable path. This is a replay instruction, not a claim that the command below was used to make the videos.
Authorship and follow-up
Who wrote this:
Who answers review comments:
Acceptance Criteria
Prototype behaviour under review:
Given an owner is bound to an identity and no stronger scope applies.
When a repository remote or supported API request identifies that owner.
Then the owner binding selects the identity without changing the remote URL.
DO NOT MERGE