iframe-proxy

williammartin · 2024-09-13T12:43:39Z

Description

When targeting the API of tenants we should be using the subdomain rather than the path prefix.

Implementation Choices

Right now, I took the straightest path to implementing this, which included copying functions from the auth package. It was an intentional choice not to DRY this out because I want to derisk this work and then review it with confidence that the black box behaviour is correct.

How To Test

The easiest way to black box validate these changes is to use the PR in cli/cli that pins to the commit sha.

Alternatively, replace the dependency in cli/cli like so:

go mod edit -replace github.com/cli/go-gh/v2=/path/to/go-gh

Then build and run the CLI with commands that use these clients such as:

GH_DEBUG=api GH_HOST=tenant.ghe.com ./bin/gh secret list --repo whatever/whatever # REST
GH_DEBUG=api GH_HOST=tenant.ghe.com ./bin/gh repo list # GQL

In both cases the HTTP requests should demonstrate the API subdomain being used instead of the path prefix.

andyfeller

I'm torn regarding some of the duplication of this logic within cli/go-gh as well as cli/cli. My knee jerk would be cli/go-gh is the source of truth for this logic and cli/cli leveraging the logic for its needs, which I realize is outside of the scope of the original issue and likely would be deferred.

Beyond that, I wonder how we go about testing these changes in an extension other than gh to ensure they work as expected. Perhaps this is a great opportunity to pair on testing this with gh copilot?

andyfeller · 2024-09-13T13:47:08Z

 func isEnterprise(host string) bool {
-	return host != github && host != localhost
+	return host != github && host != localhost && !isTenancy(host)
+}
+
+// tenancyHost is the domain name of a tenancy GitHub instance.
+const tenancyHost = "ghe.com"
+
+func isTenancy(host string) bool {
+	return strings.HasSuffix(host, "."+tenancyHost)


Are there any concerns about this being seemingly redundant to the logic in auth?

I did a double take when I first saw this because I thought the enterprise check was already taking tenancy into account, when I realized I was thinking about:

go-gh/pkg/auth/auth.go

Lines 152 to 167 in 25db6b9

// TenancyHost is the domain name of a tenancy GitHub instance.

const tenancyHost = "ghe.com"

// IsEnterprise determines if a provided host is a GitHub Enterprise Server instance,

// rather than GitHub.com or a tenancy GitHub instance.

func IsEnterprise(host string) bool {

normalizedHost := normalizeHostname(host)

return normalizedHost != github && normalizedHost != localhost && !IsTenancy(normalizedHost)

}

// IsTenancy determines if a provided host is a tenancy GitHub instance,

// rather than GitHub.com or a GitHub Enterprise Server instance.

func IsTenancy(host string) bool {

normalizedHost := normalizeHostname(host)

return strings.HasSuffix(normalizedHost, "."+tenancyHost)

}

My concern is managing 2 sets of similar / overlapping functionality in 2 places and introducing drift.

See "implementation choices" section in PR description.

I think we should do work here or in a follow up to address this but I'm not going through tearing things up until we know this is works as expected and covers the right use cases.

andyfeller · 2024-09-13T13:49:17Z

+	// This has been copied over from the cli/cli NormalizeHostname function
+	// to ensure compatible behaviour but we don't fully understand when or
+	// why it would be useful here. We can't see what harm will come of
+	// duplicating the logic.
+	if before, found := strings.CutSuffix(hostname, "."+tenancyHost); found {
+		idx := strings.LastIndex(before, ".")
+		return fmt.Sprintf("%s.%s", before[idx+1:], tenancyHost)
+	}


Similar to my comment above, it seems we now have 3 copies of this logic across cli/cli and 2 places within cli/go-gh and that makes me a little concerned:

go-gh/pkg/auth/auth.go

Lines 169 to 186 in 25db6b9

func normalizeHostname(host string) string {

hostname := strings.ToLower(host)

if strings.HasSuffix(hostname, "."+github) {

return github

}

if strings.HasSuffix(hostname, "."+localhost) {

return localhost

}

// This has been copied over from the cli/cli NormalizeHostname function

// to ensure compatible behaviour but we don't fully understand when or

// why it would be useful here. We can't see what harm will come of

// duplicating the logic.

if before, found := cutSuffix(hostname, "."+tenancyHost); found {

idx := strings.LastIndex(before, ".")

return fmt.Sprintf("%s.%s", before[idx+1:], tenancyHost)

}

return hostname

}

Same as my response above.

williammartin · 2024-09-13T14:02:47Z

andyfeller

LGTM

Use api subdomain for REST and GQL clients when host is tenant

af31cb3

williammartin mentioned this pull request Sep 13, 2024

Use api subdomains for tenant hosts cli/cli#9618

Merged

andyfeller reviewed Sep 13, 2024

View reviewed changes

andyfeller approved these changes Sep 16, 2024

View reviewed changes

andyfeller merged commit 267e1ca into trunk Sep 16, 2024

andyfeller deleted the wm/tenant-api branch September 16, 2024 11:55

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use api subdomain for REST and GQL clients when host is tenant#172

Use api subdomain for REST and GQL clients when host is tenant#172
andyfeller merged 1 commit into
trunkfrom
wm/tenant-api

williammartin commented Sep 13, 2024 •

edited

Loading

Uh oh!

andyfeller left a comment

Uh oh!

andyfeller Sep 13, 2024

Uh oh!

williammartin Sep 13, 2024

Uh oh!

andyfeller Sep 13, 2024

Uh oh!

williammartin Sep 13, 2024

Uh oh!

williammartin commented Sep 13, 2024 •

edited

Loading

Uh oh!

andyfeller left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// TenancyHost is the domain name of a tenancy GitHub instance.
	const tenancyHost = "ghe.com"

	// IsEnterprise determines if a provided host is a GitHub Enterprise Server instance,
	// rather than GitHub.com or a tenancy GitHub instance.
	func IsEnterprise(host string) bool {
	normalizedHost := normalizeHostname(host)
	return normalizedHost != github && normalizedHost != localhost && !IsTenancy(normalizedHost)
	}

	// IsTenancy determines if a provided host is a tenancy GitHub instance,
	// rather than GitHub.com or a GitHub Enterprise Server instance.
	func IsTenancy(host string) bool {
	normalizedHost := normalizeHostname(host)
	return strings.HasSuffix(normalizedHost, "."+tenancyHost)
	}

	func normalizeHostname(host string) string {
	hostname := strings.ToLower(host)
	if strings.HasSuffix(hostname, "."+github) {
	return github
	}
	if strings.HasSuffix(hostname, "."+localhost) {
	return localhost
	}
	// This has been copied over from the cli/cli NormalizeHostname function
	// to ensure compatible behaviour but we don't fully understand when or
	// why it would be useful here. We can't see what harm will come of
	// duplicating the logic.
	if before, found := cutSuffix(hostname, "."+tenancyHost); found {
	idx := strings.LastIndex(before, ".")
	return fmt.Sprintf("%s.%s", before[idx+1:], tenancyHost)
	}
	return hostname
	}

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

williammartin commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Implementation Choices

How To Test

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

andyfeller Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

williammartin Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

andyfeller Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

williammartin Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

williammartin commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

williammartin commented Sep 13, 2024 •

edited

Loading

williammartin commented Sep 13, 2024 •

edited

Loading