Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 13 additions & 1 deletion auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,10 @@ func WithHTTPClient(client *http.Client) InstallationTokenSourceOpt {
// only the Transport is swapped to inject GitHub App authentication.
authClient := *client
authClient.Transport = &oauth2.Transport{
Source: i.src,
// Reuse the JWT across requests exactly as the default transport
// does; otherwise supplying a custom client silently re-signs on
// every installation-token request.
Source: oauth2.ReuseTokenSource(nil, i.src),
Base: client.Transport,
}

Expand Down Expand Up @@ -400,6 +403,9 @@ func (t *installationTokenSource) setConfigErr(err error) {
// NewInstallationTokenSource creates a GitHub App installation token source.
// Requires installation ID and a GitHub App JWT token source for authentication.
//
// A non-positive installation ID is a configuration error, reported by the
// first call to Token() rather than sent to GitHub as a request that can only 404.
//
// The returned token source is wrapped in ReuseTokenSourceWithSkew so cached
// tokens are refreshed DefaultExpirySkew before their expiry, eliminating
// in-flight 401s when a request starts close to exp and reaches GitHub after.
Expand All @@ -423,6 +429,12 @@ func NewInstallationTokenSource(id int64, src oauth2.TokenSource, opts ...Instal
skew: DefaultExpirySkew,
}

// A non-positive ID can only produce /app/installations/0/access_tokens and
// a 404. Fail like a misconfiguration rather than like a GitHub outage.
if id <= 0 {
i.setConfigErr(errors.New("installation identifier is required"))
}

for _, opt := range opts {
opt(i)
}
Expand Down
89 changes: 89 additions & 0 deletions auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"reflect"
"strings"
"sync"
"sync/atomic"
"testing"
"time"

Expand Down Expand Up @@ -1234,3 +1235,91 @@ func TestWithExpirySkew_Wiring(t *testing.T) {
})
}
}

// countingTokenSource records how many times the application JWT was minted.
type countingTokenSource struct {
calls atomic.Int32
}

func (c *countingTokenSource) Token() (*oauth2.Token, error) {
c.calls.Add(1)
return &oauth2.Token{
AccessToken: "app-jwt",
TokenType: bearerTokenType,
Expiry: time.Now().Add(time.Hour),
}, nil
}

// Test_WithHTTPClient_CachesApplicationToken proves WithHTTPClient caches the
// application JWT the same way the default transport does. NewInstallationTokenSource
// wraps the source in oauth2.ReuseTokenSource, but WithHTTPClient wires the raw
// source, so supplying a custom client silently re-signs a JWT on every
// installation-token request.
func Test_WithHTTPClient_CachesApplicationToken(t *testing.T) {
src := &countingTokenSource{}

// expires_at inside the skew window, so the installation-token cache
// refetches on every call and each one reaches the transport.
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusCreated)
_ = json.NewEncoder(w).Encode(map[string]any{
"token": "ghs_installation",
"expires_at": time.Now().Add(time.Second).UTC().Format(time.RFC3339),
})
}))
defer server.Close()

ts := NewInstallationTokenSource(42, src,
WithBaseURL(server.URL),
WithHTTPClient(&http.Client{}),
)

const fetches = 3
for i := range fetches {
if _, err := ts.Token(); err != nil {
t.Fatalf("Token() #%d err = %v", i+1, err)
}
}

if got := src.calls.Load(); got != 1 {
t.Errorf("application token source consulted %d times across %d fetches, want 1; "+
"WithHTTPClient must reuse the JWT like the default transport", got, fetches)
}
}

// Test_NewInstallationTokenSource_RejectsZeroID proves a zero installation ID is
// refused as a configuration error instead of being sent to GitHub as
// /app/installations/0/access_tokens, which can only 404. A zero App ID is
// already rejected at construction; this is the same mistake one layer down.
func Test_NewInstallationTokenSource_RejectsZeroID(t *testing.T) {
var hits atomic.Int32

server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
hits.Add(1)
w.WriteHeader(http.StatusNotFound)
_, _ = w.Write([]byte(`{"message":"Not Found"}`))
}))
defer server.Close()

for _, id := range []int64{0, -1} {
t.Run(fmt.Sprintf("id=%d", id), func(t *testing.T) {
hits.Store(0)

ts := NewInstallationTokenSource(id, oauth2StaticSource{accessToken: "jwt"},
WithBaseURL(server.URL),
)

_, err := ts.Token()
if err == nil {
t.Fatalf("Token() err = nil, want a configuration error for installation ID %d", id)
}
if got := hits.Load(); got != 0 {
t.Errorf("server received %d request(s), want 0; installation ID %d must fail before any network call", got, id)
}
if !strings.Contains(err.Error(), "installation") {
t.Errorf("err = %q, want it to name the installation identifier", err)
}
})
}
}
15 changes: 1 addition & 14 deletions github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,27 +299,14 @@ func Test_githubClient_createInstallationToken_ErrorCases(t *testing.T) {
if tt.wantErr && tt.errorSubstring != "" {
if err == nil {
t.Errorf("expected error containing %q, got nil", tt.errorSubstring)
} else if !contains(err.Error(), tt.errorSubstring) {
} else if !strings.Contains(err.Error(), tt.errorSubstring) {
t.Errorf("expected error containing %q, got %q", tt.errorSubstring, err.Error())
}
}
})
}
}

func contains(s, substr string) bool {
return len(s) >= len(substr) && (s == substr || len(substr) == 0 || (len(s) > 0 && len(substr) > 0 && hasSubstring(s, substr)))
}

func hasSubstring(s, substr string) bool {
for i := 0; i <= len(s)-len(substr); i++ {
if s[i:i+len(substr)] == substr {
return true
}
}
return false
}

func Test_Ptr(t *testing.T) {
t.Run("string pointer", func(t *testing.T) {
s := "test"
Expand Down
Loading