From 3482fd41825674ed5e61892a773fbf8807ee1dbd Mon Sep 17 00:00:00 2001 From: Steve Hipwell Date: Fri, 16 Oct 2026 16:06:37 +0100 Subject: [PATCH 1/3] feat: add token source for generic auth logic Signed-off-by: Steve Hipwell --- README.md | 52 +--- example/basicauth/main.go | 63 ----- example/go.mod | 6 +- example/go.sum | 12 +- example/newfilewithappauth/main.go | 44 ++-- github/doc.go | 74 +++--- github/github-accessors.go | 40 --- github/github-accessors_test.go | 40 --- github/github.go | 313 ++++++++--------------- github/github_test.go | 323 ++++-------------------- go.mod | 1 + go.sum | 2 + otel/go.mod | 1 + otel/go.sum | 2 + test/integration/authorizations_test.go | 8 +- tools/go.mod | 1 + tools/go.sum | 2 + 17 files changed, 242 insertions(+), 742 deletions(-) delete mode 100644 example/basicauth/main.go diff --git a/README.md b/README.md index c2d492a8347..cff8d89d5c6 100644 --- a/README.md +++ b/README.md @@ -123,12 +123,11 @@ include the specified OAuth token. Therefore, authenticated clients should almost never be shared between different users. For API methods that require HTTP Basic Authentication, use the -[`BasicAuthTransport`](https://pkg.go.dev/github.com/google/go-github/v93/github#BasicAuthTransport). +`WithBasicAuth` option. #### As a GitHub App #### -GitHub Apps authentication can be provided by different pkgs like [bradleyfalzon/ghinstallation](https://github.com/bradleyfalzon/ghinstallation) -or [jferrl/go-githubauth](https://github.com/jferrl/go-githubauth). +GitHub Apps authentication can be provided by [jferrl/go-githubauth](https://github.com/jferrl/go-githubauth). > **Note**: Most endpoints (ex. [`GET /rate_limit`]) require access token authentication > while a few others (ex. [`GET /app/hook/deliveries`]) require [JWT] authentication. @@ -137,40 +136,8 @@ or [jferrl/go-githubauth](https://github.com/jferrl/go-githubauth). [`GET /app/hook/deliveries`]: https://docs.github.com/en/rest/apps/webhooks#list-deliveries-for-an-app-webhook [JWT]: https://docs.github.com/en/developers/apps/building-github-apps/authenticating-with-github-apps#authenticating-as-a-github-app -`ghinstallation` provides `Transport`, which implements `http.RoundTripper` to provide authentication as an installation for GitHub Apps. - -Here is an example of how to authenticate as a GitHub App using the `ghinstallation` package: - -```go -import ( - "net/http" - - "github.com/bradleyfalzon/ghinstallation/v2" - "github.com/google/go-github/v93/github" -) - -func main() { - // Wrap the shared transport for use with the integration ID 1 authenticating with installation ID 99. - itr, err := ghinstallation.NewKeyFromFile(http.DefaultTransport, 1, 99, "2016-10-19.private-key.pem") - - // Or for endpoints that require JWT authentication - // itr, err := ghinstallation.NewAppsTransportKeyFromFile(http.DefaultTransport, 1, "2016-10-19.private-key.pem") - - if err != nil { - // Handle error. - } - - // Use installation transport with client. - client, err := github.NewClient(github.WithTransport(itr)) - if err != nil { - // Handle error. - } - - // Use client... -} -``` - -`go-githubauth` implements a set of `oauth2.TokenSource` to be used with `oauth2.Client`. An `oauth2.Client` can be injected into the `github.Client` to authenticate requests. +`go-githubauth` implements a set of functions that can create `oauth2.TokenSource` implementations. +The `oauth2.TokenSource` can be configured for the `github.Client` using the `github.WithTokenSource` option. `NewApplicationTokenSource` accepts either the App's client ID (a `string`, which GitHub now recommends) or its legacy app ID (an `int64`). @@ -208,12 +175,7 @@ func main() { installationTokenSource := githubauth.NewInstallationTokenSource(installationID, appTokenSource) - // oauth2.NewClient uses oauth2.ReuseTokenSource to reuse the token until it expires. - // The token will be automatically refreshed when it expires. - // InstallationTokenSource has the mechanism to refresh the token when it expires. - httpClient := oauth2.NewClient(context.Background(), installationTokenSource) - - client, err := github.NewClient(github.WithHTTPClient(httpClient)) + client, err := github.NewClient(github.WithTokenSource(installationTokenSource)) if err != nil { // Handle error. } @@ -290,8 +252,8 @@ set the client `DisableRateLimitCheck` to `true` so the client doesn't track the If the client is an [OAuth app](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#primary-rate-limit-for-oauth-apps) you can use the apps higher rate limit to request public data by using the -`UnauthenticatedRateLimitedTransport` to make calls as the app instead of as -the user. +`github.WithBasicAuth` option to configure the client to make calls as the +app instead of as the user. ### Accepted Status ### diff --git a/example/basicauth/main.go b/example/basicauth/main.go deleted file mode 100644 index 407fd8575c6..00000000000 --- a/example/basicauth/main.go +++ /dev/null @@ -1,63 +0,0 @@ -// Copyright 2015 The go-github AUTHORS. All rights reserved. -// -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// The basicauth command demonstrates using the github.BasicAuthTransport, -// including handling two-factor authentication. This won't currently work for -// accounts that use SMS to receive one-time passwords. -// -// Deprecation Notice: GitHub will discontinue password authentication to the API. -// You must now authenticate to the GitHub API with an API token, such as an OAuth access token, -// GitHub App installation access token, or personal access token, depending on what you need to do with the token. -// Password authentication to the API will be removed on November 13, 2020. -// See the tokenauth example for details. -package main - -import ( - "bufio" - "context" - "errors" - "fmt" - "log" - "os" - "strings" - - "github.com/google/go-github/v93/github" - "golang.org/x/term" -) - -func main() { - r := bufio.NewReader(os.Stdin) - fmt.Print("GitHub Username: ") - username, _ := r.ReadString('\n') - - fmt.Print("GitHub Password: ") - password, _ := term.ReadPassword(int(os.Stdin.Fd())) - - tp := github.BasicAuthTransport{ - Username: strings.TrimSpace(username), - Password: strings.TrimSpace(string(password)), - } - - client, err := github.NewClient(github.WithHTTPClient(tp.Client())) - if err != nil { - log.Fatalf("error: %v", err) - } - ctx := context.Background() - user, _, err := client.Users.Get(ctx, "") - - // Is this a two-factor auth error? If so, prompt for OTP and try again. - if errors.As(err, new(*github.TwoFactorAuthError)) { - fmt.Print("\nGitHub OTP: ") - otp, _ := r.ReadString('\n') - tp.OTP = strings.TrimSpace(otp) - user, _, err = client.Users.Get(ctx, "") - } - - if err != nil { - log.Fatalf("error: %v", err) - } - - fmt.Printf("\n%v\n", github.Stringify(user)) -} diff --git a/example/go.mod b/example/go.mod index edc61d5dc73..8ef7575c0fa 100644 --- a/example/go.mod +++ b/example/go.mod @@ -4,11 +4,11 @@ go 1.26.0 require ( github.com/ProtonMail/go-crypto v1.5.2 - github.com/bradleyfalzon/ghinstallation/v2 v2.19.0 github.com/gofri/go-github-pagination v1.0.1 github.com/gofri/go-github-ratelimit/v2 v2.0.2 github.com/google/go-github/otel/v93 v93.0.0 github.com/google/go-github/v93 v93.0.0 + github.com/jferrl/go-githubauth v1.9.1 github.com/sigstore/sigstore-go v1.2.2 go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.47.0 go.opentelemetry.io/otel/sdk v1.47.0 @@ -50,10 +50,9 @@ require ( github.com/go-openapi/swag/yamlutils v0.26.1 // indirect github.com/go-openapi/validate v0.26.0 // indirect github.com/go-viper/mapstructure/v2 v2.5.0 // indirect - github.com/golang-jwt/jwt/v4 v4.5.2 // indirect + github.com/golang-jwt/jwt/v5 v5.3.1 // indirect github.com/google/certificate-transparency-go v1.3.3 // indirect github.com/google/go-containerregistry v0.21.7 // indirect - github.com/google/go-github/v88 v88.0.0 // indirect github.com/google/go-querystring v1.2.0 // indirect github.com/google/uuid v1.6.0 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0 // indirect @@ -81,6 +80,7 @@ require ( go.yaml.in/yaml/v3 v3.0.5 // indirect golang.org/x/mod v0.41.0 // indirect golang.org/x/net v0.58.0 // indirect + golang.org/x/oauth2 v0.37.0 // indirect golang.org/x/sync v0.23.0 // indirect golang.org/x/sys v0.48.0 // indirect golang.org/x/text v0.42.0 // indirect diff --git a/example/go.sum b/example/go.sum index 47e08453139..17082559957 100644 --- a/example/go.sum +++ b/example/go.sum @@ -68,8 +68,6 @@ github.com/aws/smithy-go v1.26.0 h1:9ouqbi+NyKP7fV3Te7UElCwdAb6Y8uk7LGwPE5tVe/s= github.com/aws/smithy-go v1.26.0/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= github.com/blang/semver v3.5.1+incompatible h1:cQNTCjp13qL8KC3Nbxr/y2Bqb63oX6wdnnjpJbkM4JQ= github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk= -github.com/bradleyfalzon/ghinstallation/v2 v2.19.0 h1:KQfD+43pRw9NUJhGycGrFr9vF1MubZacksKol1gomFI= -github.com/bradleyfalzon/ghinstallation/v2 v2.19.0/go.mod h1:fe5ECIhCdEnxwLiBlNTxx9CP455wt42BELnlDVMvaAA= github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8= github.com/cenkalti/backoff/v4 v4.3.0/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE= github.com/cenkalti/backoff/v5 v5.0.3 h1:ZN+IMa753KfX5hd8vVaMixjnqRZ3y8CuJKRKj1xcsSM= @@ -162,8 +160,6 @@ github.com/gofri/go-github-pagination v1.0.1 h1:j5uJRx65i/Ta2M0QSgiPcyokY69JnCQg github.com/gofri/go-github-pagination v1.0.1/go.mod h1:Qij55Fb4fNPjam3SB+8cLnqp4pgR8RGMyIspYXcyHX0= github.com/gofri/go-github-ratelimit/v2 v2.0.2 h1:gS8wAS1jTmlWGdTjAM7KIpsLjwY1S0S/gKK5hthfSXM= github.com/gofri/go-github-ratelimit/v2 v2.0.2/go.mod h1:YBQt4gTbdcbMjJFT05YFEaECwH78P5b0IwrnbLiHGdE= -github.com/golang-jwt/jwt/v4 v4.5.2 h1:YtQM7lnr8iZ+j5q71MGKkNw9Mn7AjHM68uc9g5fXeUI= -github.com/golang-jwt/jwt/v4 v4.5.2/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0= github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= @@ -175,8 +171,6 @@ github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/google/go-containerregistry v0.21.7 h1:/vPFuVXDjtFREsVArW+0h1CIl5urnOhzei4X2DMW9IU= github.com/google/go-containerregistry v0.21.7/go.mod h1:kjSbt7/zMsKLWfnHrIvKvhXHUw91jbe9DNjPPJ32gXE= -github.com/google/go-github/v88 v88.0.0 h1:dZA9IKkPK1eXZj4ypngnpRj5FwdpTv4whix2PrQMP7M= -github.com/google/go-github/v88 v88.0.0/go.mod h1:rufTDgn2N45wjhukLTyxmvc9nilSp3mr3Rgtt6b1MPw= github.com/google/go-querystring v1.2.0 h1:yhqkPbu2/OH+V9BfpCVPZkNmUXhb2gBxJArfhIxNtP0= github.com/google/go-querystring v1.2.0/go.mod h1:8IFJqpSRITyJ8QhQ13bmbeMBDfmeEJZD5A0egEOmkqU= github.com/google/s2a-go v0.1.9 h1:LGD7gtMgezd8a/Xak7mEWL0PjoTQFvpRudN895yqKW0= @@ -225,6 +219,8 @@ github.com/jedisct1/go-minisign v0.0.0-20211028175153-1c139d1cc84b h1:ZGiXF8sz7P github.com/jedisct1/go-minisign v0.0.0-20211028175153-1c139d1cc84b/go.mod h1:hQmNrgofl+IY/8L+n20H6E6PWBBTokdsv+q49j0QhsU= github.com/jellydator/ttlcache/v3 v3.4.0 h1:YS4P125qQS0tNhtL6aeYkheEaB/m8HCqdMMP4mnWdTY= github.com/jellydator/ttlcache/v3 v3.4.0/go.mod h1:Hw9EgjymziQD3yGsQdf1FqFdpp7YjFMd4Srg5EJlgD4= +github.com/jferrl/go-githubauth v1.9.1 h1:gDBgNgPxU4gL0yWKf6FOwKCdLb3ICHPVMQrPEq+xq0I= +github.com/jferrl/go-githubauth v1.9.1/go.mod h1:HkuzaWMeDf+S09Dq/5wlBwPeho9ZZm3MixLdD2WTqO0= github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc= github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/letsencrypt/boulder v0.20260309.0 h1:kZynrxK3QfqLGx6hhoz+Rfs3hgltJs1p9Mp+4+VwnY0= @@ -342,8 +338,8 @@ golang.org/x/mod v0.41.0 h1:qJmnOUb4YB+FsEuM3HcWucdZASCPGhsX6uljO6pog0c= golang.org/x/mod v0.41.0/go.mod h1:Ek9pY8RKWXwsWvd3rQiHYtMqkjSUV+s1Rj7j4H5Ur6o= golang.org/x/net v0.58.0 h1:ynWG7rqYi4ccpTEuPZ2QGWHktVEM9DMCj9yzDE0Q7To= golang.org/x/net v0.58.0/go.mod h1:YwCddHnFlT7eLQqVprV19OnhLGtc5xOKgE0RyqgfWAU= -golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= -golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= +golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= +golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.23.0 h1:KameEIfc1IkluZyXWLn39Wd4tURc6GbCiISGiZm2bQk= golang.org/x/sync v0.23.0/go.mod h1:sUUOizhqBxiL6pEWpqNLUiaJn1ShEbZ6BBqskPbjZm0= golang.org/x/sys v0.48.0 h1:bbX/i/6MgT9BVLM9RT1thmxL04yeTAhbEz4SyadbXoo= diff --git a/example/newfilewithappauth/main.go b/example/newfilewithappauth/main.go index 9131d7fbc11..9e9f5ada07e 100644 --- a/example/newfilewithappauth/main.go +++ b/example/newfilewithappauth/main.go @@ -9,35 +9,35 @@ package main import ( + "bufio" "context" + "fmt" "log" - "net/http" "os" - "time" + "strings" - "github.com/bradleyfalzon/ghinstallation/v2" "github.com/google/go-github/v93/github" + "github.com/jferrl/go-githubauth" + "golang.org/x/term" ) func main() { - const gitHost = "https://git.api.com" + gitHost := "https://git.api.com" - privatePem, err := os.ReadFile("path/to/pem") - if err != nil { - log.Fatalf("failed to read pem: %v", err) - } + r := bufio.NewReader(os.Stdin) + fmt.Print("GitHub App Client ID: ") + clientID, _ := r.ReadString('\n') - itr, err := ghinstallation.NewAppsTransport(http.DefaultTransport, 10, privatePem) + fmt.Print("GitHub App Private Key: ") + privateKey, _ := term.ReadPassword(int(os.Stdin.Fd())) + + appTokenSource, err := githubauth.NewApplicationTokenSource(strings.TrimSpace(clientID), privateKey) if err != nil { - log.Fatalf("failed to create app transport: %v", err) + log.Fatalf("failed to create app token source: %v", err) } - itr.BaseURL = gitHost // create git client with app transport - client, err := github.NewClient(github.WithHTTPClient(&http.Client{ - Transport: itr, - Timeout: time.Second * 30, - }), github.WithEnterpriseURLs(gitHost, gitHost)) + client, err := github.NewClient(github.WithTokenSource(appTokenSource), github.WithURLs(&gitHost, &gitHost)) if err != nil { log.Fatalf("failed to create git client for app: %v", err) } @@ -52,18 +52,16 @@ func main() { var installID int64 for _, val := range installations { installID = val.GetID() + break } - token, _, err := client.Apps.CreateInstallationToken( - context.Background(), - installID, - &github.InstallationTokenOptions{}, - ) - if err != nil { - log.Fatalf("failed to create installation token: %v", err) + if installID == 0 { + log.Fatal("no installation found") } - apiClient, err := github.NewClient(github.WithAuthToken(token.GetToken()), github.WithEnterpriseURLs(gitHost, gitHost)) + installationTokenSource := githubauth.NewInstallationTokenSource(installID, appTokenSource, githubauth.WithBaseURL(gitHost)) + + apiClient, err := github.NewClient(github.WithTokenSource(installationTokenSource), github.WithURLs(&gitHost, &gitHost)) if err != nil { log.Fatalf("failed to create new git client with token: %v", err) } diff --git a/github/doc.go b/github/doc.go index 10fef426d4f..9207a9c33cb 100644 --- a/github/doc.go +++ b/github/doc.go @@ -59,53 +59,44 @@ include the specified OAuth token. Therefore, authenticated clients should almost never be shared between different users. For API methods that require HTTP Basic Authentication, use the -[BasicAuthTransport]. +[WithBasicAuth] option. GitHub Apps authentication can be provided by the -https://github.com/bradleyfalzon/ghinstallation package. -It supports both authentication as an installation, using an installation access token, -and as an app, using a JWT. Use the [WithTransport] option to configure your client to use the appropriate transport. - -To authenticate as an installation: - - import "github.com/bradleyfalzon/ghinstallation" - - func main() { - // Wrap the shared transport for use with the integration ID 1 authenticating with installation ID 99. - itr, err := ghinstallation.NewKeyFromFile(http.DefaultTransport, 1, 99, "2016-10-19.private-key.pem") - if err != nil { - // Handle error. - } - - // Use installation transport with client - client, err := github.NewClient(github.WithTransport(itr)) - if err != nil { - // Handle error. - } - - // Use client... +[github.com/jferrl/go-githubauth](https://github.com/jferrl/go-githubauth) package. +Most endpoints (for example, `GET /rate_limit`) require access token +authentication, while a few others (for example, `GET /app/hook/deliveries`) +require [JWT authentication](https://docs.github.com/en/developers/apps/building-github-apps/authenticating-with-github-apps#authenticating-as-a-github-app). + +`go-githubauth` provides functions that create `oauth2.TokenSource` +implementations. Configure one for the client with [WithTokenSource]. +`NewApplicationTokenSource` accepts either the App's client ID (a `string`, +which GitHub now recommends) or its legacy app ID (an `int64`). + +For example, create an application token source and then an installation token +source to authenticate a client for an installation: + + privateKey := []byte(os.Getenv("GITHUB_APP_PRIVATE_KEY")) + clientID := os.Getenv("GITHUB_APP_CLIENT_ID") + installationID, err := strconv.ParseInt(os.Getenv("GITHUB_APP_INSTALLATION_ID"), 10, 64) + if err != nil { + // Handle error. } -To authenticate as an app, using a JWT: - - import "github.com/bradleyfalzon/ghinstallation" - - func main() { - // Wrap the shared transport for use with the application ID 1. - atr, err := ghinstallation.NewAppsTransportKeyFromFile(http.DefaultTransport, 1, "2016-10-19.private-key.pem") - if err != nil { - // Handle error. - } - - // Use app transport with client - client, err := github.NewClient(github.WithTransport(atr)) - if err != nil { - // Handle error. - } + appTokenSource, err := githubauth.NewApplicationTokenSource(clientID, privateKey) + if err != nil { + // Handle error. + } - // Use client... + installationTokenSource := githubauth.NewInstallationTokenSource(installationID, appTokenSource) + client, err := github.NewClient(github.WithTokenSource(installationTokenSource)) + if err != nil { + // Handle error. } +Some APIs, such as writing a file to a repository, require an installation +token. Use the GitHub App's installation ID to generate one, then authenticate +with the OAuth token source as shown above. + # Rate Limiting GitHub imposes a rate limit on all API clients. Unauthenticated clients are @@ -113,8 +104,7 @@ limited to 60 requests per hour, while authenticated clients can make up to 5,000 requests per hour. The Search API has a custom rate limit. Unauthenticated clients are limited to 10 requests per minute, while authenticated clients can make up to 30 requests per minute. To receive the higher rate limit when -making calls that are not issued on behalf of a user, -use [UnauthenticatedRateLimitedTransport]. +making calls that are not issued on behalf of a user, use [github.WithBasicAuth]. The returned [Response].[Rate] value contains the rate limit information from the most recent API call. If a recent enough response isn't diff --git a/github/github-accessors.go b/github/github-accessors.go index 952d3d8fea0..d3feef539ea 100644 --- a/github/github-accessors.go +++ b/github/github-accessors.go @@ -3678,30 +3678,6 @@ func (a *AzureHubConfig) GetName() string { return a.Name } -// GetOTP returns the OTP field. -func (b *BasicAuthTransport) GetOTP() string { - if b == nil { - return "" - } - return b.OTP -} - -// GetPassword returns the Password field. -func (b *BasicAuthTransport) GetPassword() string { - if b == nil { - return "" - } - return b.Password -} - -// GetUsername returns the Username field. -func (b *BasicAuthTransport) GetUsername() string { - if b == nil { - return "" - } - return b.Username -} - // GetID returns the ID field. func (b *BillingCostCenter) GetID() string { if b == nil { @@ -45934,22 +45910,6 @@ func (t *TreeEntry) GetURL() string { return *t.URL } -// GetClientID returns the ClientID field. -func (u *UnauthenticatedRateLimitedTransport) GetClientID() string { - if u == nil { - return "" - } - return u.ClientID -} - -// GetClientSecret returns the ClientSecret field. -func (u *UnauthenticatedRateLimitedTransport) GetClientSecret() string { - if u == nil { - return "" - } - return u.ClientSecret -} - // GetReason returns the Reason field if it's non-nil, zero value otherwise. func (u *UnsuspendUserRequest) GetReason() string { if u == nil || u.Reason == nil { diff --git a/github/github-accessors_test.go b/github/github-accessors_test.go index 7e9ad6671c9..185bc03744d 100644 --- a/github/github-accessors_test.go +++ b/github/github-accessors_test.go @@ -4615,30 +4615,6 @@ func TestAzureHubConfig_GetName(tt *testing.T) { a.GetName() } -func TestBasicAuthTransport_GetOTP(tt *testing.T) { - tt.Parallel() - b := &BasicAuthTransport{} - b.GetOTP() - b = nil - b.GetOTP() -} - -func TestBasicAuthTransport_GetPassword(tt *testing.T) { - tt.Parallel() - b := &BasicAuthTransport{} - b.GetPassword() - b = nil - b.GetPassword() -} - -func TestBasicAuthTransport_GetUsername(tt *testing.T) { - tt.Parallel() - b := &BasicAuthTransport{} - b.GetUsername() - b = nil - b.GetUsername() -} - func TestBillingCostCenter_GetID(tt *testing.T) { tt.Parallel() b := &BillingCostCenter{} @@ -57278,22 +57254,6 @@ func TestTreeEntry_GetURL(tt *testing.T) { t.GetURL() } -func TestUnauthenticatedRateLimitedTransport_GetClientID(tt *testing.T) { - tt.Parallel() - u := &UnauthenticatedRateLimitedTransport{} - u.GetClientID() - u = nil - u.GetClientID() -} - -func TestUnauthenticatedRateLimitedTransport_GetClientSecret(tt *testing.T) { - tt.Parallel() - u := &UnauthenticatedRateLimitedTransport{} - u.GetClientSecret() - u = nil - u.GetClientSecret() -} - func TestUnsuspendUserRequest_GetReason(tt *testing.T) { tt.Parallel() var zeroValue string diff --git a/github/github.go b/github/github.go index a13d0a12e4f..cde396c1d89 100644 --- a/github/github.go +++ b/github/github.go @@ -13,6 +13,7 @@ package github import ( "bytes" "context" + "encoding/base64" "encoding/json" "errors" "fmt" @@ -27,6 +28,7 @@ import ( "time" "github.com/google/go-querystring/query" + "golang.org/x/oauth2" ) const ( @@ -163,14 +165,34 @@ const ( // segment, which could allow path traversal attacks. var ErrPathForbidden = errors.New("path must not contain '..' due to auth vulnerability issue") +// conditionalAuthTransport is an http.RoundTripper that conditionally +// uses an auth transport for URLs that are checked by a given function. +type conditionalAuthTransport struct { + baseTransport http.RoundTripper + authTransport http.RoundTripper + checkURLAuthFunc func(url *url.URL) bool +} + +// RoundTrip implements the http.RoundTripper interface for conditionalAuthTransport. +func (t *conditionalAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) { + // Check if the URL should be authenticated + if t.checkURLAuthFunc(req.URL) { + // Use the auth transport for URLs that match the pattern + return t.authTransport.RoundTrip(req) + } + + // Don't use the auth transport for URLs that don't match the pattern + return t.baseTransport.RoundTrip(req) +} + // A Client manages communication with the GitHub API. type Client struct { - client *http.Client // HTTP client used to communicate with the API. - clientIgnoreRedirects *http.Client // HTTP client used to communicate with the API on endpoints where we don't want to follow redirects. + client *http.Client // HTTP client used to communicate with the API. - // authToken is the token configured by [WithAuthToken], retained so that - // [Client.Clone] can install it against the clone's own origins. - authToken *string + // tokenSource is the token source configured by [WithAuthToken], + // [WithBasicAuth] or [WithTokenSource], used to obtain access tokens for + // authenticated requests. + tokenSource oauth2.TokenSource // baseTransport is the transport the credential wrapper was installed on // top of, so that [Client.Clone] can rebase onto it rather than layering a @@ -266,9 +288,10 @@ type service struct { } // Client returns the http.Client used by this GitHub client. -// The token configured by [WithAuthToken] is attached only to requests to this -// client's configured API and upload origins, so the returned client may also -// be used for requests to other hosts without leaking the token. +// The token source configured by [WithAuthToken], [WithBasicAuth] or +// [WithTokenSource] is attached only to requests to this client's configured +// API and upload origins, so the returned client may also be used for requests +// to other hosts without leaking the token. func (c *Client) Client() *http.Client { clientCopy := *c.client return &clientCopy @@ -369,7 +392,7 @@ type clientOptions struct { apiVersionMax *string userAgent *string envProxy bool - token *string + tokenSource oauth2.TokenSource baseURL *url.URL uploadURL *url.URL disableRateLimitCheck bool @@ -399,6 +422,8 @@ func WithHTTPClient(httpClient *http.Client) ClientOptionsFunc { // WithTransport returns a ClientOptionsFunc that sets the http.RoundTripper // for a Client. This overrides the transport set by [WithHTTPClient]. If not // set and no HTTP client is provided, the default http.RoundTripper will be used. +// Do not use this to authenticate requests; use [WithTokenSource], +// [WithBasicAuth] or [WithAuthToken] instead. func WithTransport(transport http.RoundTripper) ClientOptionsFunc { return func(o *clientOptions) error { if transport == nil { @@ -445,17 +470,44 @@ func WithEnvProxy() ClientOptionsFunc { } } -// WithAuthToken returns a ClientOptionsFunc that sets the authentication token -// for a Client. The token is attached only to requests to the Client's -// configured API and upload origins; a request to any other origin is sent -// without it. If not set, the client will make unauthenticated requests. +// WithAuthToken returns a ClientOptionsFunc that sets the token source +// for a Client to use the provided authentication token. func WithAuthToken(token string) ClientOptionsFunc { return func(o *clientOptions) error { if token == "" { return errors.New("token must not be empty") } - o.token = &token + o.tokenSource = oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token}) + return nil + } +} + +// WithBasicAuth returns a ClientOptionsFunc that sets the token source +// for a Client to use basic authentication with the provided username and password. +func WithBasicAuth(username, password string) ClientOptionsFunc { + return func(o *clientOptions) error { + if username == "" || password == "" { + return errors.New("username and password must not be empty") + } + + creds := base64.StdEncoding.EncodeToString([]byte(username + ":" + password)) + + o.tokenSource = oauth2.StaticTokenSource(&oauth2.Token{TokenType: "Basic", AccessToken: creds}) + return nil + } +} + +// WithTokenSource returns a ClientOptionsFunc that sets the token source +// for a Client. If a token source is not set, the client will operate +// unauthenticated. +func WithTokenSource(tokenSource oauth2.TokenSource) ClientOptionsFunc { + return func(o *clientOptions) error { + if tokenSource == nil { + return errors.New("tokenSource must not be nil") + } + + o.tokenSource = tokenSource return nil } } @@ -559,9 +611,8 @@ func WithMaxSecondaryRateLimitRetryAfterDuration(duration time.Duration) ClientO // For GitHub Enterprise, use [WithEnterpriseURLs] to set the base and upload // URLs. // -// To make authenticated requests, use [WithAuthToken] or [WithHTTPClient] to -// pass in a [http.Client] that performs authentication -// (e.g., using golang.org/x/oauth2). +// To make authenticated requests, use [WithAuthToken], [WithBasicAuth] or +// [WithTokenSource] to provide a token source. // // For production usage it is recommended to provide a timeout using // [WithTimeout] or by providing a custom [http.Client] with an appropriate @@ -616,6 +667,27 @@ func newClient(opts clientOptions) (*Client, error) { c.client.Transport = t2 } + c.baseTransport = c.client.Transport + + if opts.tokenSource != nil { + if c.baseTransport == nil { + c.baseTransport = http.DefaultTransport + } + + c.tokenSource = opts.tokenSource + + t := &conditionalAuthTransport{ + baseTransport: c.baseTransport, + authTransport: &oauth2.Transport{ + Source: c.tokenSource, + Base: c.baseTransport, + }, + checkURLAuthFunc: c.shouldAuthorizeRequest, + } + + c.client.Transport = t + } + if opts.apiVersionMin != nil { c.apiVersionMin = *opts.apiVersionMin } @@ -642,36 +714,6 @@ func newClient(opts clientOptions) (*Client, error) { c.uploadURL, _ = url.Parse(uploadBaseURL) } - // Install the credential wrapper only after the API and upload origins are - // known: the wrapper consults them for every request, so that a request to - // any other origin goes out unauthenticated instead of carrying the token. - c.baseTransport = c.client.Transport - if opts.token != nil { - token := "Bearer " + *opts.token - c.authToken = opts.token - - transport := c.baseTransport - if transport == nil { - transport = http.DefaultTransport - } - - c.client.Transport = roundTripperFunc(func(req *http.Request) (*http.Response, error) { - if c.shouldAuthorizeRequest(req.URL) { - req = req.Clone(req.Context()) - req.Header.Set("Authorization", token) - } - - return transport.RoundTrip(req) - }) - } - - c.clientIgnoreRedirects = &http.Client{ - Transport: c.client.Transport, - Timeout: c.client.Timeout, - Jar: c.client.Jar, - CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, - } - c.disableRateLimitCheck = opts.disableRateLimitCheck if !c.disableRateLimitCheck { @@ -735,15 +777,16 @@ func newClient(opts clientOptions) (*Client, error) { // A request's destination is governed by two rules, and they deliberately differ // because a credential and a payload fail in opposite ways. // -// Credentials — the token configured by [WithAuthToken], and the credentials on -// [BasicAuthTransport] and [UnauthenticatedRateLimitedTransport] — are sent -// only to origins the caller has configured. Any other destination, including -// a redirect hop, goes out with no credentials attached. Such a request is -// never rejected and the credentials are never forwarded: it is simply sent -// unauthenticated, so that a caller cannot leak a token by handing the client a -// URL whose host it does not control. Withholding a credential is safe to do -// silently because the request can still succeed without it — a pre-signed -// download URL handed back by the API is the ordinary case. +// Credentials — the token source configured by [WithAuthToken], [WithBasicAuth] +// or [WithTokenSource], and the credentials on [BasicAuthTransport] and +// [UnauthenticatedRateLimitedTransport] — are sent only to origins the caller +// has configured. Any other destination, including a redirect hop, goes out +// with no credentials attached. Such a request is never rejected and the +// credentials are never forwarded: it is simply sent unauthenticated, so that +// a caller cannot leak a token by handing the client a URL whose host it does +// not control. Withholding a credential is safe to do silently because the +// request can still succeed without it — a pre-signed download URL handed back +// by the API is the ordinary case. // // A payload cannot be withheld that way, because a request whose body is dropped // cannot succeed at all, and the caller would be told a request succeeded when @@ -887,8 +930,8 @@ func (c *Client) UploadURL() string { // updated when the original client's rate limit information is updated. // The returned client is independent of the original client and can be // modified without affecting the original client. Any token configured by -// [WithAuthToken] carries over to the clone, and is re-scoped to the clone's -// own API and upload origins. +// [WithAuthToken], [WithBasicAuth] or [WithTokenSource] carries over to the +// clone, and is re-scoped to the clone's own API and upload origins. func (c *Client) Clone(opts ...ClientOptionsFunc) (*Client, error) { if c.client == nil { return nil, errUninitialized @@ -900,7 +943,7 @@ func (c *Client) Clone(opts ...ClientOptionsFunc) (*Client, error) { userAgent: &c.userAgent, baseURL: new(*c.baseURL), uploadURL: new(*c.uploadURL), - token: c.authToken, + tokenSource: c.tokenSource, disableRateLimitCheck: c.disableRateLimitCheck, rateLimitRedirectionalEndpoints: c.rateLimitRedirectionalEndpoints, maxSecondaryRateLimitRetryAfterDuration: &c.maxSecondaryRateLimitRetryAfterDuration, @@ -921,13 +964,8 @@ func (c *Client) Clone(opts ...ClientOptionsFunc) (*Client, error) { // origins, so it starts from the unwrapped transport: reusing the // wrapped one would stack a second check that still enforces the // original client's origins. - transport := c.client.Transport - if c.authToken != nil { - transport = c.baseTransport - } - o.httpClient = &http.Client{ - Transport: transport, + Transport: c.baseTransport, CheckRedirect: c.client.CheckRedirect, Jar: c.client.Jar, Timeout: c.client.Timeout, @@ -1557,7 +1595,14 @@ func (c *Client) BareDo(req *http.Request) (*Response, error) { // redirection code returned by the API. If a redirection is returned by the api, bareDoIgnoreRedirects // returns a *RedirectionError. func (c *Client) bareDoIgnoreRedirects(req *http.Request) (*Response, error) { - return c.bareDo(c.clientIgnoreRedirects, req) + cc := &http.Client{ + Transport: c.client.Transport, + Timeout: c.client.Timeout, + Jar: c.client.Jar, + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + + return c.bareDo(cc, req) } var errInvalidLocation = errors.New("invalid or empty Location header in redirection response") @@ -2200,142 +2245,6 @@ func (c *Client) RateLimits(ctx context.Context) (*RateLimits, *Response, error) return c.RateLimit.Get(ctx) } -func setCredentialsAsHeaders(req *http.Request, id, secret string) *http.Request { - // To set extra headers, we must make a copy of the Request so - // that we don't modify the Request we were given. This is required by the - // specification of http.RoundTripper. - // - // Since we are going to modify only req.Header here, we only need a deep copy - // of req.Header. - convertedRequest := *req - convertedRequest.Header = make(http.Header, len(req.Header)) - - for k, s := range req.Header { - convertedRequest.Header[k] = append([]string(nil), s...) - } - convertedRequest.SetBasicAuth(id, secret) - return &convertedRequest -} - -/* -UnauthenticatedRateLimitedTransport allows you to make unauthenticated calls -that need to use a higher rate limit associated with your OAuth application. - - t := &github.UnauthenticatedRateLimitedTransport{ - ClientID: "your app's client ID", - ClientSecret: "your app's client secret", - } - client := github.NewClient(t.Client()) - -This will add the client id and secret as a base64-encoded string in the format -ClientID:ClientSecret and apply it as an "Authorization": "Basic" header. - -See https://docs.github.com/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#primary-rate-limit-for-oauth-apps -for more information. -*/ -type UnauthenticatedRateLimitedTransport struct { - // ClientID is the GitHub OAuth client ID of the current application, which - // can be found by selecting its entry in the list at - // https://github.com/settings/applications. - ClientID string - - // ClientSecret is the GitHub OAuth client secret of the current - // application. - ClientSecret string - - // AllowedOrigins limits the origins ClientID and ClientSecret are sent to. - // The credentials are attached only to requests whose origin matches one of - // these; every other request, including a redirect hop, is sent without - // them. - // - // If empty, the GitHub.com API and upload origins are used. An empty - // AllowedOrigins does not mean "any origin": set it explicitly when talking - // to GitHub Enterprise. - AllowedOrigins []*url.URL - - // Transport is the underlying HTTP transport to use when making requests. - // It will default to http.DefaultTransport if nil. - Transport http.RoundTripper -} - -// RoundTrip implements the RoundTripper interface. -func (t *UnauthenticatedRateLimitedTransport) RoundTrip(req *http.Request) (*http.Response, error) { - if t.ClientID == "" { - return nil, errors.New("t.ClientID is empty") - } - if t.ClientSecret == "" { - return nil, errors.New("t.ClientSecret is empty") - } - - if isAllowedOrigin(req.URL, t.AllowedOrigins) { - req = setCredentialsAsHeaders(req, t.ClientID, t.ClientSecret) - } - - // Make the HTTP request. - return t.transport().RoundTrip(req) -} - -// Client returns an *http.Client that makes requests which are subject to the -// rate limit of your OAuth application. -func (t *UnauthenticatedRateLimitedTransport) Client() *http.Client { - return &http.Client{Transport: t} -} - -func (t *UnauthenticatedRateLimitedTransport) transport() http.RoundTripper { - if t.Transport != nil { - return t.Transport - } - return http.DefaultTransport -} - -// BasicAuthTransport is an http.RoundTripper that authenticates all requests -// using HTTP Basic Authentication with the provided username and password. It -// additionally supports users who have two-factor authentication enabled on -// their GitHub account. -type BasicAuthTransport struct { - Username string // GitHub username - Password string // GitHub password - OTP string // one-time password for users with two-factor auth enabled - - // AllowedOrigins limits the origins the credentials above are sent to. They - // are attached only to requests whose origin matches one of these; every - // other request, including a redirect hop, is sent without them. - // - // If empty, the GitHub.com API and upload origins are used. An empty - // AllowedOrigins does not mean "any origin": set it explicitly when talking - // to GitHub Enterprise. - AllowedOrigins []*url.URL - - // Transport is the underlying HTTP transport to use when making requests. - // It will default to http.DefaultTransport if nil. - Transport http.RoundTripper -} - -// RoundTrip implements the RoundTripper interface. -func (t *BasicAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) { - if isAllowedOrigin(req.URL, t.AllowedOrigins) { - req = setCredentialsAsHeaders(req, t.Username, t.Password) - if t.OTP != "" { - req.Header.Set(headerOTP, t.OTP) - } - } - - return t.transport().RoundTrip(req) -} - -// Client returns an *http.Client that makes requests that are authenticated -// using HTTP Basic Authentication. -func (t *BasicAuthTransport) Client() *http.Client { - return &http.Client{Transport: t} -} - -func (t *BasicAuthTransport) transport() http.RoundTripper { - if t.Transport != nil { - return t.Transport - } - return http.DefaultTransport -} - // formatRateReset formats d to look like "[rate reset in 2s]" or // "[rate reset in 87m02s]" for the positive durations. And like "[rate limit was reset 87m02s ago]" // for the negative cases. diff --git a/github/github_test.go b/github/github_test.go index dc902f9ed1c..a5e9d0bf661 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -27,6 +27,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "golang.org/x/oauth2" ) const ( @@ -632,14 +633,62 @@ func TestWithAuthToken(t *testing.T) { t.Fatalf("WithAuthToken errored: %v", err) } - if opts.token == nil || *opts.token != validToken { - t.Errorf("token = %v, want %v", opts.token, validToken) + if opts.tokenSource == nil { + t.Fatal("tokenSource is nil, want non-nil") + } + + gotToken, err := opts.tokenSource.Token() + if err != nil { + t.Fatalf("TokenSource errored: %v", err) + } + if gotToken.AccessToken != validToken { + t.Fatalf("got token %v, want %v", gotToken.AccessToken, validToken) + } + }) +} + +func TestWithTokenSource(t *testing.T) { + t.Parallel() + + t.Run("nil_token_source", func(t *testing.T) { + t.Parallel() + + opts := clientOptions{} + err := WithTokenSource(nil)(&opts) + if err == nil || err.Error() != "tokenSource must not be nil" { + t.Error("expected error for nil token source, got nil") + } + }) + + t.Run("valid_token_source", func(t *testing.T) { + t.Parallel() + + validToken := "ghp_exampletoken1234567890" + source := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: validToken}) + + opts := clientOptions{} + err := WithTokenSource(source)(&opts) + if err != nil { + t.Fatalf("WithTokenSource errored: %v", err) + } + + if opts.tokenSource == nil { + t.Fatal("tokenSource is nil, want non-nil") + } + + gotToken, err := opts.tokenSource.Token() + if err != nil { + t.Fatalf("TokenSource errored: %v", err) + } + if gotToken.AccessToken != validToken { + t.Fatalf("got token %v, want %v", gotToken.AccessToken, validToken) } }) } func TestWithURLs(t *testing.T) { t.Parallel() + for _, tt := range []struct { name string baseURL *string @@ -729,6 +778,7 @@ func TestWithURLs(t *testing.T) { func TestWithEnterpriseURLs(t *testing.T) { t.Parallel() + for _, tt := range []struct { name string baseURL string @@ -1073,29 +1123,13 @@ func Test_newClient(t *testing.T) { if tt.opts.httpClient != nil && c.client != tt.opts.httpClient { t.Error("newClient http.Client should be the same instance as the provided httpClient option") } - if tt.opts.transport != nil && !tt.opts.envProxy && tt.opts.token == nil && c.client.Transport != tt.opts.transport { + if tt.opts.transport != nil && !tt.opts.envProxy && tt.opts.tokenSource == nil && c.client.Transport != tt.opts.transport { t.Error("newClient http.Client.Transport should be the same instance as the provided transport option") } if tt.opts.timeout != nil && c.client.Timeout != *tt.opts.timeout { t.Errorf("newClient http.Client.Timeout is %v, want %v", c.client.Timeout, *tt.opts.timeout) } - if c.clientIgnoreRedirects == nil { - t.Error("newClient http.Client used for redirects is not initialized") - } - if c.clientIgnoreRedirects.Transport != c.client.Transport { - t.Error("newClient http.Client and http.Client used for redirects should share the same Transport instance") - } - if c.clientIgnoreRedirects.Timeout != c.client.Timeout { - t.Errorf("newClient http.Client and http.Client used for redirects should have the same Timeout, got %v and %v", c.client.Timeout, c.clientIgnoreRedirects.Timeout) - } - if c.clientIgnoreRedirects.Jar != c.client.Jar { - t.Error("newClient http.Client and http.Client used for redirects should share the same Jar instance") - } - if c.clientIgnoreRedirects.CheckRedirect == nil { - t.Error("newClient http.Client used for redirects should have a CheckRedirect function") - } - if tt.opts.apiVersionMin != nil && c.apiVersionMin != *tt.opts.apiVersionMin { t.Errorf("newClient apiVersionMin is %v, want %v", c.apiVersionMin, *tt.opts.apiVersionMin) } @@ -2602,15 +2636,7 @@ func TestDo_AcceptedError_LargeBodyTruncated(t *testing.T) { func TestDo_sanitizeURL(t *testing.T) { t.Parallel() baseURL := &url.URL{Scheme: "http", Host: "127.0.0.1:0", Path: "/"} // Use port 0 on purpose to trigger a dial TCP error, expect to get "dial tcp 127.0.0.1:0: connect: can't assign requested address". - tp := &UnauthenticatedRateLimitedTransport{ - ClientID: "id", - ClientSecret: "secret", - // Scope the transport to the origin below so that the credentials really are - // attached to this request; otherwise the test would pass vacuously, with the - // secret never sent in the first place. - AllowedOrigins: []*url.URL{baseURL}, - } - unauthedClient := mustNewClient(t, WithHTTPClient(tp.Client())) + unauthedClient := mustNewClient(t, WithBasicAuth("id", "secret")) unauthedClient.baseURL = baseURL req, err := unauthedClient.NewRequest(t.Context(), "GET", ".", nil) if err != nil { @@ -4677,230 +4703,6 @@ func TestError_Error(t *testing.T) { } } -func TestSetCredentialsAsHeaders(t *testing.T) { - t.Parallel() - req := new(http.Request) - id, secret := "id", "secret" - modifiedRequest := setCredentialsAsHeaders(req, id, secret) - - actualID, actualSecret, ok := modifiedRequest.BasicAuth() - if !ok { - t.Error("request does not contain basic credentials") - } - - if actualID != id { - t.Errorf("id is %v, want %v", actualID, id) - } - - if actualSecret != secret { - t.Errorf("secret is %v, want %v", actualSecret, secret) - } -} - -func TestUnauthenticatedRateLimitedTransport(t *testing.T) { - t.Parallel() - client, mux, _ := setup(t) - - clientID, clientSecret := "id", "secret" - mux.HandleFunc("/", func(_ http.ResponseWriter, r *http.Request) { - id, secret, ok := r.BasicAuth() - if !ok { - t.Error("request does not contain basic auth credentials") - } - if id != clientID { - t.Errorf("request contained basic auth username %q, want %q", id, clientID) - } - if secret != clientSecret { - t.Errorf("request contained basic auth password %q, want %q", secret, clientSecret) - } - }) - - tp := &UnauthenticatedRateLimitedTransport{ - ClientID: clientID, - ClientSecret: clientSecret, - AllowedOrigins: []*url.URL{client.baseURL}, - } - unauthedClient := mustNewClient(t, WithHTTPClient(tp.Client())) - unauthedClient.baseURL = client.baseURL - req, _ := unauthedClient.NewRequest(t.Context(), "GET", ".", nil) - _, err := unauthedClient.Do(req, nil) - assertNilError(t, err) -} - -func TestUnauthenticatedRateLimitedTransport_originScope(t *testing.T) { - t.Parallel() - clientID, clientSecret := "id", "secret" - - // The origin the transport is scoped to receives the credentials... - allowed, allowedMux, _ := setup(t) - allowedMux.HandleFunc("/", func(_ http.ResponseWriter, r *http.Request) { - if _, _, ok := r.BasicAuth(); !ok { - t.Error("request to an allowed origin does not contain basic auth credentials") - } - }) - - // ...and any other origin does not, even when the same transport is used. - foreign, foreignMux, _ := setup(t) - foreignMux.HandleFunc("/", func(_ http.ResponseWriter, r *http.Request) { - if id, secret, ok := r.BasicAuth(); ok { - t.Errorf("request to an unrelated origin contained basic auth credentials %q/%q", id, secret) - } - }) - - tp := &UnauthenticatedRateLimitedTransport{ - ClientID: clientID, - ClientSecret: clientSecret, - AllowedOrigins: []*url.URL{allowed.baseURL}, - } - - for _, test := range []struct { - name string - base *url.URL - }{ - {name: "allowed origin", base: allowed.baseURL}, - {name: "foreign origin", base: foreign.baseURL}, - } { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - c := mustNewClient(t, WithHTTPClient(tp.Client())) - c.baseURL = test.base - req, _ := c.NewRequest(t.Context(), "GET", ".", nil) - _, err := c.Do(req, nil) - assertNilError(t, err) - }) - } -} - -func TestUnauthenticatedRateLimitedTransport_missingFields(t *testing.T) { - t.Parallel() - // missing ClientID - tp := &UnauthenticatedRateLimitedTransport{ - ClientSecret: "secret", - } - _, err := tp.RoundTrip(nil) - if err == nil { - t.Error("Expected error to be returned") - } - - // missing ClientSecret - tp = &UnauthenticatedRateLimitedTransport{ - ClientID: "id", - } - _, err = tp.RoundTrip(nil) - if err == nil { - t.Error("Expected error to be returned") - } -} - -func TestUnauthenticatedRateLimitedTransport_transport(t *testing.T) { - t.Parallel() - // default transport - tp := &UnauthenticatedRateLimitedTransport{ - ClientID: "id", - ClientSecret: "secret", - } - if tp.transport() != http.DefaultTransport { - t.Error("Expected http.DefaultTransport to be used.") - } - - // custom transport - tp = &UnauthenticatedRateLimitedTransport{ - ClientID: "id", - ClientSecret: "secret", - Transport: &http.Transport{}, - } - if tp.transport() == http.DefaultTransport { - t.Error("Expected custom transport to be used.") - } -} - -func TestBasicAuthTransport(t *testing.T) { - t.Parallel() - client, mux, _ := setup(t) - - username, password, otp := "u", "p", "123456" - - mux.HandleFunc("/", func(_ http.ResponseWriter, r *http.Request) { - u, p, ok := r.BasicAuth() - if !ok { - t.Error("request does not contain basic auth credentials") - } - if u != username { - t.Errorf("request contained basic auth username %q, want %q", u, username) - } - if p != password { - t.Errorf("request contained basic auth password %q, want %q", p, password) - } - if got, want := r.Header.Get(headerOTP), otp; got != want { - t.Errorf("request contained OTP %q, want %q", got, want) - } - }) - - tp := &BasicAuthTransport{ - Username: username, - Password: password, - OTP: otp, - AllowedOrigins: []*url.URL{client.baseURL}, - } - basicAuthClient := mustNewClient(t, WithHTTPClient(tp.Client())) - basicAuthClient.baseURL = client.baseURL - req, _ := basicAuthClient.NewRequest(t.Context(), "GET", ".", nil) - _, err := basicAuthClient.Do(req, nil) - assertNilError(t, err) -} - -func TestBasicAuthTransport_originScope(t *testing.T) { - t.Parallel() - username, password, otp := "u", "p", "123456" - - // The origin the transport is scoped to receives both credentials and the OTP... - allowed, allowedMux, _ := setup(t) - allowedMux.HandleFunc("/", func(_ http.ResponseWriter, r *http.Request) { - if u, p, ok := r.BasicAuth(); !ok || u != username || p != password { - t.Error("request to an allowed origin does not contain the expected basic auth credentials") - } - if got := r.Header.Get(headerOTP); got != otp { - t.Errorf("request to an allowed origin contained OTP %q, want %q", got, otp) - } - }) - - // ...and any other origin receives neither. The OTP is a second factor, so a - // foreign origin must not see it even though it is not itself a "credential". - foreign, foreignMux, _ := setup(t) - foreignMux.HandleFunc("/", func(_ http.ResponseWriter, r *http.Request) { - if u, p, ok := r.BasicAuth(); ok { - t.Errorf("request to an unrelated origin contained basic auth credentials %q/%q", u, p) - } - if got := r.Header.Get(headerOTP); got != "" { - t.Errorf("request to an unrelated origin contained OTP %q, want none", got) - } - }) - - tp := &BasicAuthTransport{ - Username: username, - Password: password, - OTP: otp, - AllowedOrigins: []*url.URL{allowed.baseURL}, - } - - for _, test := range []struct { - name string - base *url.URL - }{ - {name: "allowed origin", base: allowed.baseURL}, - {name: "foreign origin", base: foreign.baseURL}, - } { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - c := mustNewClient(t, WithHTTPClient(tp.Client())) - c.baseURL = test.base - req, _ := c.NewRequest(t.Context(), "GET", ".", nil) - _, err := c.Do(req, nil) - assertNilError(t, err) - }) - } -} - // authRecorderServer returns a server that reports the Authorization header of // each request it serves on the returned channel. The channel is buffered and // keeps only the first value, so a test cannot block the server, and a test that @@ -5150,23 +4952,6 @@ func TestClient_CloneReScopesToken(t *testing.T) { assertRecordedAuthHeader(t, firstAuth, "") } -func TestBasicAuthTransport_transport(t *testing.T) { - t.Parallel() - // default transport - tp := &BasicAuthTransport{} - if tp.transport() != http.DefaultTransport { - t.Error("Expected http.DefaultTransport to be used.") - } - - // custom transport - tp = &BasicAuthTransport{ - Transport: &http.Transport{}, - } - if tp.transport() == http.DefaultTransport { - t.Error("Expected custom transport to be used.") - } -} - func TestFormatRateReset(t *testing.T) { t.Parallel() d := 120*time.Minute + 12*time.Second diff --git a/go.mod b/go.mod index f6bd23874e6..402aa870dbf 100644 --- a/go.mod +++ b/go.mod @@ -5,4 +5,5 @@ go 1.26.0 require ( github.com/google/go-cmp v0.7.0 github.com/google/go-querystring v1.2.0 + golang.org/x/oauth2 v0.37.0 ) diff --git a/go.sum b/go.sum index 68bb006efb3..51b34e84bc6 100644 --- a/go.sum +++ b/go.sum @@ -3,3 +3,5 @@ github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/google/go-querystring v1.2.0 h1:yhqkPbu2/OH+V9BfpCVPZkNmUXhb2gBxJArfhIxNtP0= github.com/google/go-querystring v1.2.0/go.mod h1:8IFJqpSRITyJ8QhQ13bmbeMBDfmeEJZD5A0egEOmkqU= +golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= +golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= diff --git a/otel/go.mod b/otel/go.mod index 620ecaf8b45..53c10e94ce4 100644 --- a/otel/go.mod +++ b/otel/go.mod @@ -18,6 +18,7 @@ require ( github.com/google/uuid v1.6.0 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/otel/log v1.47.0 // indirect + golang.org/x/oauth2 v0.37.0 // indirect golang.org/x/sys v0.48.0 // indirect ) diff --git a/otel/go.sum b/otel/go.sum index d6b97f31672..12d454a2c3a 100644 --- a/otel/go.sum +++ b/otel/go.sum @@ -32,5 +32,7 @@ go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= +golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= +golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sys v0.48.0 h1:bbX/i/6MgT9BVLM9RT1thmxL04yeTAhbEz4SyadbXoo= golang.org/x/sys v0.48.0/go.mod h1:hNLxWAXmnKAxqDtdwIYC4bM9oQPEecfsnNMuSxOs3og= diff --git a/test/integration/authorizations_test.go b/test/integration/authorizations_test.go index aadc48e001c..a0c8d6ab5d6 100644 --- a/test/integration/authorizations_test.go +++ b/test/integration/authorizations_test.go @@ -9,7 +9,6 @@ package integration import ( "os" - "strings" "testing" "time" @@ -137,12 +136,7 @@ func getOAuthAppClient(t *testing.T) *github.Client { t.Skipf(msgEnvMissing, envKeyClientSecret) } - tp := github.BasicAuthTransport{ - Username: strings.TrimSpace(username), - Password: strings.TrimSpace(password), - } - - c, err := github.NewClient(github.WithHTTPClient(tp.Client())) + c, err := github.NewClient(github.WithBasicAuth(username, password)) if err != nil { t.Fatal(err) } diff --git a/tools/go.mod b/tools/go.mod index 939fe9e380b..81784ddb4d1 100644 --- a/tools/go.mod +++ b/tools/go.mod @@ -19,6 +19,7 @@ require ( github.com/oasdiff/yaml v0.1.1 // indirect github.com/oasdiff/yaml3 v0.0.14 // indirect github.com/santhosh-tekuri/jsonschema/v6 v6.0.3 // indirect + golang.org/x/oauth2 v0.37.0 // indirect golang.org/x/text v0.39.0 // indirect ) diff --git a/tools/go.sum b/tools/go.sum index 0b499f5bbdd..e0e45bf686d 100644 --- a/tools/go.sum +++ b/tools/go.sum @@ -43,6 +43,8 @@ github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsT github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= +golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= +golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= golang.org/x/sync v0.24.0/go.mod h1:sUUOizhqBxiL6pEWpqNLUiaJn1ShEbZ6BBqskPbjZm0= golang.org/x/text v0.39.0 h1:UbZz4pLOvn600D6Oh6GGEI6VAmndrEBLv8/6BEXzyus= From 395869a8ada795c337763737d7817d12b7956882 Mon Sep 17 00:00:00 2001 From: Steve Hipwell Date: Fri, 16 Oct 2026 20:15:19 +0100 Subject: [PATCH 2/3] fix: remove withhttpclient Signed-off-by: Steve Hipwell --- README.md | 17 ++- example/newfilewithappauth/main.go | 9 +- example/ratelimit/main.go | 4 +- github/github.go | 46 +------- github/github_benchmark_test.go | 4 +- github/github_test.go | 169 ++--------------------------- 6 files changed, 32 insertions(+), 217 deletions(-) diff --git a/README.md b/README.md index cff8d89d5c6..0ffbe827017 100644 --- a/README.md +++ b/README.md @@ -101,9 +101,9 @@ For more sample code snippets, head over to the ### Authentication ### -Use the `github.WithAuthToken` options method to configure your client to authenticate using an -OAuth token (for example, a [personal access token][]). This is what is needed -for a majority of use cases aside from GitHub Apps. +Use the `github.WithAuthToken` options method to configure your client to authenticate +using an OAuth token (for example, a [personal access token][]). +This is what is needed for a majority of use cases aside from GitHub Apps. ```go client, err := github.NewClient(github.WithAuthToken("... your access token ...")) @@ -112,18 +112,15 @@ if err != nil { } ``` -To support more advanced use cases; you can use the `github.WithTransport` option to provide a -custom `http.RoundTripper` that handles authentication for you, or the `github.WithHTTPClient` -option to provide a custom `http.Client`. As an example; you can use the `oauth2.Transport` -from the [golang.org/x/oauth2](https://pkg.go.dev/golang.org/x/oauth2) package to handle OAuth -token refreshing for you. +To support more advanced use cases; you can use the `github.WithTokenSource` option to provide a +`oauth2.TokenSource` that handles authentication for you. Note that when using an authenticated Client, all calls made by the client will include the specified OAuth token. Therefore, authenticated clients should almost never be shared between different users. For API methods that require HTTP Basic Authentication, use the -`WithBasicAuth` option. +`github.WithBasicAuth` option. #### As a GitHub App #### @@ -290,7 +287,7 @@ import ( _ "github.com/bartventer/httpcache/store/memcache" // Register the in-memory backend ) -client, err := github.NewClient(github.WithHTTPClient(httpcache.NewClient("memcache://")), github.WithAuthToken(os.Getenv("GITHUB_TOKEN"))) +client, err := github.NewClient(github.WithTransport(httpcache.NewTransport("memcache://")), github.WithAuthToken(os.Getenv("GITHUB_TOKEN"))) if err != nil { // Handle error. } diff --git a/example/newfilewithappauth/main.go b/example/newfilewithappauth/main.go index 9e9f5ada07e..6108fd6295e 100644 --- a/example/newfilewithappauth/main.go +++ b/example/newfilewithappauth/main.go @@ -18,7 +18,6 @@ import ( "github.com/google/go-github/v93/github" "github.com/jferrl/go-githubauth" - "golang.org/x/term" ) func main() { @@ -28,8 +27,12 @@ func main() { fmt.Print("GitHub App Client ID: ") clientID, _ := r.ReadString('\n') - fmt.Print("GitHub App Private Key: ") - privateKey, _ := term.ReadPassword(int(os.Stdin.Fd())) + fmt.Print("Path to GitHub App Private Key (.pem): ") + pemPath, _ := r.ReadString('\n') + privateKey, err := os.ReadFile(strings.TrimSpace(pemPath)) + if err != nil { + log.Fatalf("failed to read private key: %v", err) + } appTokenSource, err := githubauth.NewApplicationTokenSource(strings.TrimSpace(clientID), privateKey) if err != nil { diff --git a/example/ratelimit/main.go b/example/ratelimit/main.go index a5d7ce1e570..34b8447f20a 100644 --- a/example/ratelimit/main.go +++ b/example/ratelimit/main.go @@ -35,10 +35,10 @@ func main() { }), ) - paginator := githubpagination.NewClient(rateLimiter, + paginator := githubpagination.New(rateLimiter, githubpagination.WithPerPage(100), // default to 100 results per page ) - client, err := github.NewClient(github.WithHTTPClient(paginator)) + client, err := github.NewClient(github.WithTransport(paginator)) if err != nil { log.Fatalf("Error creating GitHub client: %v", err) } diff --git a/github/github.go b/github/github.go index cde396c1d89..ad4e9e6a263 100644 --- a/github/github.go +++ b/github/github.go @@ -385,7 +385,6 @@ var errUninitialized = errors.New("client is not initialized") // clientOptions holds the configuration options for a Client. type clientOptions struct { - httpClient *http.Client transport http.RoundTripper timeout *time.Duration apiVersionMin *string @@ -405,23 +404,8 @@ type clientOptions struct { // to a Client. type ClientOptionsFunc func(*clientOptions) error -// WithHTTPClient returns a ClientOptionsFunc that sets the http.Client -// for a Client. If not set, a default http.Client will be used. -func WithHTTPClient(httpClient *http.Client) ClientOptionsFunc { - return func(o *clientOptions) error { - if httpClient == nil { - return errors.New("http client must not be nil") - } - - httpClient := *httpClient - o.httpClient = &httpClient - return nil - } -} - // WithTransport returns a ClientOptionsFunc that sets the http.RoundTripper -// for a Client. This overrides the transport set by [WithHTTPClient]. If not -// set and no HTTP client is provided, the default http.RoundTripper will be used. +// for a Client. If not set, the default http.RoundTripper will be used. // Do not use this to authenticate requests; use [WithTokenSource], // [WithBasicAuth] or [WithAuthToken] instead. func WithTransport(transport http.RoundTripper) ClientOptionsFunc { @@ -436,8 +420,7 @@ func WithTransport(transport http.RoundTripper) ClientOptionsFunc { } // WithTimeout returns a ClientOptionsFunc that sets the timeout for a Client. -// This overrides the timeout set by [WithHTTPClient]. If not set and no HTTP -// client is provided, the default http.Client with no timeout will be used. +// If not set, the default http.Client with no timeout will be used. // It is recommended to provide a timeout for production environments. func WithTimeout(timeout time.Duration) ClientOptionsFunc { return func(o *clientOptions) error { @@ -615,8 +598,7 @@ func WithMaxSecondaryRateLimitRetryAfterDuration(duration time.Duration) ClientO // [WithTokenSource] to provide a token source. // // For production usage it is recommended to provide a timeout using -// [WithTimeout] or by providing a custom [http.Client] with an appropriate -// timeout using [WithHTTPClient]. +// [WithTimeout]. func NewClient(opts ...ClientOptionsFunc) (*Client, error) { o := clientOptions{} for _, opt := range opts { @@ -635,12 +617,7 @@ func newClient(opts clientOptions) (*Client, error) { apiVersionDefault: api20221128, apiVersionMin: api20221128, apiVersionMax: api20260310, - } - - if opts.httpClient != nil { - c.client = opts.httpClient - } else { - c.client = &http.Client{} + client: &http.Client{}, } if opts.transport != nil { @@ -938,6 +915,8 @@ func (c *Client) Clone(opts ...ClientOptionsFunc) (*Client, error) { } o := clientOptions{ + transport: c.baseTransport, + timeout: &c.client.Timeout, apiVersionMin: &c.apiVersionMin, apiVersionMax: &c.apiVersionMax, userAgent: &c.userAgent, @@ -959,19 +938,6 @@ func (c *Client) Clone(opts ...ClientOptionsFunc) (*Client, error) { } } - if o.httpClient == nil { - // A clone that carries a token installs the credential against its own - // origins, so it starts from the unwrapped transport: reusing the - // wrapped one would stack a second check that still enforces the - // original client's origins. - o.httpClient = &http.Client{ - Transport: c.baseTransport, - CheckRedirect: c.client.CheckRedirect, - Jar: c.client.Jar, - Timeout: c.client.Timeout, - } - } - clone, err := newClient(o) if err != nil { return nil, err diff --git a/github/github_benchmark_test.go b/github/github_benchmark_test.go index 47a61fe7239..2a5f7022616 100644 --- a/github/github_benchmark_test.go +++ b/github/github_benchmark_test.go @@ -37,9 +37,7 @@ func BenchmarkDo(b *testing.B) { if err != nil { b.Fatalf("json.Marshal returned error: %v", err) } - client, err := NewClient(WithHTTPClient(&http.Client{ - Transport: &fixedResponseTransport{payload: payload}, - })) + client, err := NewClient(WithTransport(&fixedResponseTransport{payload: payload})) if err != nil { b.Fatalf("NewClient returned error: %v", err) } diff --git a/github/github_test.go b/github/github_test.go index a5e9d0bf661..3a0187c02ae 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -118,14 +118,9 @@ func setup(t *testing.T) (client *Client, mux *http.ServeMux, serverURL string) IdleConnTimeout: 20 * time.Second, } - // Create HTTP client with the isolated transport - httpClient := &http.Client{ - Transport: transport, - Timeout: 30 * time.Second, - } // client is the GitHub client being tested and is // configured to use test server. - client = mustNewClient(t, WithHTTPClient(httpClient)) + client = mustNewClient(t, WithTransport(transport), WithTimeout(30*time.Second)) url, _ := url.Parse(server.URL + baseURLPath + "/") client.baseURL = url @@ -450,43 +445,6 @@ func assertWrite(t *testing.T, w io.Writer, data []byte) { assertNilError(t, err) } -func TestWithHTTPClient(t *testing.T) { - t.Parallel() - - t.Run("nil_client", func(t *testing.T) { - t.Parallel() - - opts := clientOptions{} - err := WithHTTPClient(nil)(&opts) - if err == nil || err.Error() != "http client must not be nil" { - t.Errorf("WithHTTPClient errored: %v", err) - } - }) - - t.Run("non_nil_client", func(t *testing.T) { - t.Parallel() - - customClient := &http.Client{Timeout: 10 * time.Second} - opts := clientOptions{} - err := WithHTTPClient(customClient)(&opts) - if err != nil { - t.Fatalf("WithHTTPClient errored: %v", err) - } - - if opts.httpClient == nil { - t.Fatal("httpClient is nil") - } - - if opts.httpClient == customClient { - t.Error("httpClient should be a shallow copy of the provided client, but is the same instance") - } - - if opts.httpClient.Timeout != customClient.Timeout { - t.Errorf("httpClient Timeout = %v, want %v", opts.httpClient.Timeout, customClient.Timeout) - } - }) -} - func TestWithTransport(t *testing.T) { t.Parallel() @@ -991,7 +949,7 @@ func TestNewClient(t *testing.T) { { name: "with_options", opts: []ClientOptionsFunc{ - WithHTTPClient(&http.Client{Timeout: 10 * time.Second}), + WithTimeout(10 * time.Second), }, wantErr: "", }, @@ -1042,13 +1000,6 @@ func Test_newClient(t *testing.T) { opts: clientOptions{}, wantErr: "", }, - { - name: "with_http_client", - opts: clientOptions{ - httpClient: &http.Client{Transport: &http.Transport{IdleConnTimeout: 5 * time.Second}}, - }, - wantErr: "", - }, { name: "with_transport", opts: clientOptions{ @@ -1059,7 +1010,6 @@ func Test_newClient(t *testing.T) { { name: "with_all_options", opts: clientOptions{ - httpClient: &http.Client{Transport: &http.Transport{IdleConnTimeout: 5 * time.Second}}, transport: &http.Transport{IdleConnTimeout: 10 * time.Second}, timeout: new(15 * time.Second), apiVersionMin: new(api20221128), @@ -1120,9 +1070,6 @@ func Test_newClient(t *testing.T) { if c.client == nil { t.Error("newClient http.Client is not initialized") } - if tt.opts.httpClient != nil && c.client != tt.opts.httpClient { - t.Error("newClient http.Client should be the same instance as the provided httpClient option") - } if tt.opts.transport != nil && !tt.opts.envProxy && tt.opts.tokenSource == nil && c.client.Transport != tt.opts.transport { t.Error("newClient http.Client.Transport should be the same instance as the provided transport option") } @@ -1326,6 +1273,7 @@ func TestClient_Clone(t *testing.T) { t.Parallel() c := mustNewClient(t) + c.baseTransport = &http.Transport{IdleConnTimeout: 10 * time.Second} c.apiVersionMin = api20221128 c.apiVersionMax = api20221128 c.userAgent = "CustomUserAgent/1.0" @@ -1335,7 +1283,7 @@ func TestClient_Clone(t *testing.T) { c.rateLimitRedirectionalEndpoints = true c.maxSecondaryRateLimitRetryAfterDuration = 2 * time.Minute c.Marketplace.Stubbed = true - c.client.Transport = &http.Transport{IdleConnTimeout: 10 * time.Second} + c.client.Transport = c.baseTransport c.client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return nil } c.client.Timeout = 15 * time.Second c.rateLimits[CoreCategory].Remaining = 100 @@ -1352,12 +1300,6 @@ func TestClient_Clone(t *testing.T) { if cloned.client.Transport != c.client.Transport { t.Error("Cloned Client http.Client.Transport is not the same instance as original") } - if cloned.client.CheckRedirect == nil || fmt.Sprintf("%p", cloned.client.CheckRedirect) != fmt.Sprintf("%p", c.client.CheckRedirect) { - t.Error("Cloned Client http.Client.CheckRedirect is not the same function instance as original") - } - if cloned.client.Jar != c.client.Jar { - t.Error("Cloned Client http.Client.Jar is not the same instance as original") - } if cloned.client.Timeout != c.client.Timeout { t.Errorf("Cloned Client http.Client.Timeout is %v, want %v", cloned.client.Timeout, c.client.Timeout) } @@ -1394,12 +1336,13 @@ func TestClient_Clone(t *testing.T) { t.Parallel() c := mustNewClient(t) + c.baseTransport = &http.Transport{IdleConnTimeout: 10 * time.Second} c.userAgent = "CustomUserAgent/1.0" c.baseURL.Path = "/custom/" c.uploadURL.Path = "/custom-upload/" c.disableRateLimitCheck = true c.Marketplace.Stubbed = true - c.client.Transport = &http.Transport{IdleConnTimeout: 10 * time.Second} + c.client.Transport = c.baseTransport c.client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return nil } c.client.Timeout = 15 * time.Second c.rateLimits[CoreCategory].Remaining = 100 @@ -1416,12 +1359,6 @@ func TestClient_Clone(t *testing.T) { if cloned.client.Transport != c.client.Transport { t.Error("Cloned Client http.Client.Transport is not the same instance as original") } - if cloned.client.CheckRedirect == nil || fmt.Sprintf("%p", cloned.client.CheckRedirect) != fmt.Sprintf("%p", c.client.CheckRedirect) { - t.Error("Cloned Client http.Client.CheckRedirect is not the same function instance as original") - } - if cloned.client.Jar != c.client.Jar { - t.Error("Cloned Client http.Client.Jar is not the same instance as original") - } if cloned.client.Timeout != c.client.Timeout { t.Errorf("Cloned Client http.Client.Timeout is %v, want %v", cloned.client.Timeout, c.client.Timeout) } @@ -1442,47 +1379,12 @@ func TestClient_Clone(t *testing.T) { } }) - t.Run("initialized_client_with_http_client", func(t *testing.T) { - t.Parallel() - - c := mustNewClient(t) - c.client.Transport = &http.Transport{IdleConnTimeout: 10 * time.Second} - c.client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return nil } - c.client.Timeout = 15 * time.Second - - h := &http.Client{ - Transport: &http.Transport{IdleConnTimeout: 20 * time.Second}, - CheckRedirect: func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse }, - Timeout: 25 * time.Second, - } - - cloned, err := c.Clone(WithHTTPClient(h)) - if err != nil { - t.Fatalf("Client.Clone returned error: %v", err) - } - - if cloned.client == h { - t.Error("Cloned Client has same http.Client instance as provided in WithHTTPClient, but should be a different instance") - } - if cloned.client.Transport != h.Transport { - t.Error("Cloned Client http.Client.Transport is not the same instance as original") - } - if cloned.client.CheckRedirect == nil || fmt.Sprintf("%p", cloned.client.CheckRedirect) != fmt.Sprintf("%p", h.CheckRedirect) { - t.Error("Cloned Client http.Client.CheckRedirect is not the same function instance as original") - } - if cloned.client.Jar != h.Jar { - t.Error("Cloned Client http.Client.Jar is not the same instance as original") - } - if cloned.client.Timeout != h.Timeout { - t.Errorf("Cloned Client http.Client.Timeout is %v, want %v", cloned.client.Timeout, h.Timeout) - } - }) - t.Run("initialized_client_with_transport", func(t *testing.T) { t.Parallel() c := mustNewClient(t) - c.client.Transport = &http.Transport{IdleConnTimeout: 10 * time.Second} + c.baseTransport = &http.Transport{IdleConnTimeout: 10 * time.Second} + c.client.Transport = c.baseTransport c.client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return nil } c.client.Timeout = 15 * time.Second @@ -1499,12 +1401,6 @@ func TestClient_Clone(t *testing.T) { if cloned.client.Transport != tr { t.Error("Cloned Client http.Client.Transport is not the same instance as original") } - if cloned.client.CheckRedirect == nil || fmt.Sprintf("%p", cloned.client.CheckRedirect) != fmt.Sprintf("%p", c.client.CheckRedirect) { - t.Error("Cloned Client http.Client.CheckRedirect is not the same function instance as original") - } - if cloned.client.Jar != c.client.Jar { - t.Error("Cloned Client http.Client.Jar is not the same instance as original") - } if cloned.client.Timeout != c.client.Timeout { t.Errorf("Cloned Client http.Client.Timeout is %v, want %v", cloned.client.Timeout, c.client.Timeout) } @@ -1514,7 +1410,8 @@ func TestClient_Clone(t *testing.T) { t.Parallel() c := mustNewClient(t) - c.client.Transport = &http.Transport{IdleConnTimeout: 10 * time.Second} + c.baseTransport = &http.Transport{IdleConnTimeout: 10 * time.Second} + c.client.Transport = c.baseTransport c.client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return nil } c.client.Timeout = 15 * time.Second @@ -1531,52 +1428,6 @@ func TestClient_Clone(t *testing.T) { if cloned.client.Transport != c.client.Transport { t.Error("Cloned Client http.Client.Transport is not the same instance as original") } - if cloned.client.CheckRedirect == nil || fmt.Sprintf("%p", cloned.client.CheckRedirect) != fmt.Sprintf("%p", c.client.CheckRedirect) { - t.Error("Cloned Client http.Client.CheckRedirect is not the same function instance as original") - } - if cloned.client.Jar != c.client.Jar { - t.Error("Cloned Client http.Client.Jar is not the same instance as original") - } - if cloned.client.Timeout != timeout { - t.Errorf("Cloned Client http.Client.Timeout is %v, want %v", cloned.client.Timeout, timeout) - } - }) - - t.Run("initialized_client_with_http_client_transport_timeout", func(t *testing.T) { - t.Parallel() - - c := mustNewClient(t) - c.client.Transport = &http.Transport{IdleConnTimeout: 10 * time.Second} - c.client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return nil } - c.client.Timeout = 15 * time.Second - - h := &http.Client{ - Transport: &http.Transport{IdleConnTimeout: 20 * time.Second}, - CheckRedirect: func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse }, - Timeout: 25 * time.Second, - } - - tr := &http.Transport{IdleConnTimeout: 30 * time.Second} - - timeout := 45 * time.Second - - cloned, err := c.Clone(WithHTTPClient(h), WithTransport(tr), WithTimeout(timeout)) - if err != nil { - t.Fatalf("Client.Clone returned error: %v", err) - } - - if cloned.client == h { - t.Error("Cloned Client has same http.Client instance as provided in WithHTTPClient, but should be a different instance") - } - if cloned.client.Transport != tr { - t.Error("Cloned Client http.Client.Transport is not the same instance as original") - } - if cloned.client.CheckRedirect == nil || fmt.Sprintf("%p", cloned.client.CheckRedirect) != fmt.Sprintf("%p", h.CheckRedirect) { - t.Error("Cloned Client http.Client.CheckRedirect is not the same function instance as original") - } - if cloned.client.Jar != h.Jar { - t.Error("Cloned Client http.Client.Jar is not the same instance as original") - } if cloned.client.Timeout != timeout { t.Errorf("Cloned Client http.Client.Timeout is %v, want %v", cloned.client.Timeout, timeout) } From 44f72f5e2577f2a2d0d0977999eec66641aa15e9 Mon Sep 17 00:00:00 2001 From: Steve Hipwell Date: Fri, 16 Oct 2026 21:07:59 +0100 Subject: [PATCH 3/3] fix: add missing test Signed-off-by: Steve Hipwell --- github/github_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/github/github_test.go b/github/github_test.go index 3a0187c02ae..7dc21eb2108 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -605,6 +605,49 @@ func TestWithAuthToken(t *testing.T) { }) } +func TestWithBasicAuth(t *testing.T) { + t.Parallel() + + t.Run("empty_username", func(t *testing.T) { + t.Parallel() + + opts := clientOptions{} + err := WithBasicAuth("", "bar")(&opts) + if err == nil || err.Error() != "username and password must not be empty" { + t.Error("expected error for empty username, got nil") + } + }) + + t.Run("empty_password", func(t *testing.T) { + t.Parallel() + + opts := clientOptions{} + err := WithBasicAuth("foo", "")(&opts) + if err == nil || err.Error() != "username and password must not be empty" { + t.Error("expected error for empty password, got nil") + } + }) + + t.Run("valid_credentials", func(t *testing.T) { + t.Parallel() + + opts := clientOptions{} + err := WithBasicAuth("foo", "bar")(&opts) + if err != nil { + t.Fatalf("WithBasicAuth errored: %v", err) + } + + if opts.tokenSource == nil { + t.Fatal("tokenSource is nil, want non-nil") + } + + _, err = opts.tokenSource.Token() + if err != nil { + t.Fatalf("TokenSource errored: %v", err) + } + }) +} + func TestWithTokenSource(t *testing.T) { t.Parallel()