From cb4a65253b077e7e75d0082f2612249c0b3246c8 Mon Sep 17 00:00:00 2001 From: James Telfer <792299+jamestelfer@users.noreply.github.com> Date: Thu, 23 May 2024 22:16:35 +1000 Subject: [PATCH 1/3] refactor: switch to options pattern for AppsTransport Lays groundwork for adding ClientID support without lots of constructors. --- appsTransport.go | 96 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 81 insertions(+), 15 deletions(-) diff --git a/appsTransport.go b/appsTransport.go index a67510c..524e747 100644 --- a/appsTransport.go +++ b/appsTransport.go @@ -30,11 +30,11 @@ type AppsTransport struct { // NewAppsTransportKeyFromFile returns a AppsTransport using a private key from file. func NewAppsTransportKeyFromFile(tr http.RoundTripper, appID int64, privateKeyFile string) (*AppsTransport, error) { - privateKey, err := ioutil.ReadFile(privateKeyFile) - if err != nil { - return nil, fmt.Errorf("could not read private key: %s", err) - } - return NewAppsTransport(tr, appID, privateKey) + return NewAppsTransportWithAllOptions( + tr, + WithAppID(appID), + WithPrivateKeyFile(privateKeyFile), + ) } // NewAppsTransport returns a AppsTransport using private key. The key is parsed @@ -45,33 +45,59 @@ func NewAppsTransportKeyFromFile(tr http.RoundTripper, appID int64, privateKeyFi // // The returned Transport's RoundTrip method is safe to be used concurrently. func NewAppsTransport(tr http.RoundTripper, appID int64, privateKey []byte) (*AppsTransport, error) { - key, err := jwt.ParseRSAPrivateKeyFromPEM(privateKey) - if err != nil { - return nil, fmt.Errorf("could not parse private key: %s", err) - } - return NewAppsTransportFromPrivateKey(tr, appID, key), nil + return NewAppsTransportWithAllOptions( + tr, + WithAppID(appID), + WithPrivateKeyRaw(privateKey), + ) } // NewAppsTransportFromPrivateKey returns an AppsTransport using a crypto/rsa.(*PrivateKey). func NewAppsTransportFromPrivateKey(tr http.RoundTripper, appID int64, key *rsa.PrivateKey) *AppsTransport { - return &AppsTransport{ + t, _ := NewAppsTransportWithAllOptions( + tr, + WithAppID(appID), + WithPrivateKey(key), + ) + + return t +} + +func NewAppsTransportWithOptions(tr http.RoundTripper, appID int64, opts ...AppsTransportOption) (*AppsTransport, error) { + t := &AppsTransport{ BaseURL: apiBaseURL, Client: &http.Client{Transport: tr}, tr: tr, - signer: NewRSASigner(jwt.SigningMethodRS256, key), appID: appID, } + + for _, fn := range opts { + fn(t) + } + + if t.signer == nil { + return nil, errors.New("no signer provided") + } + + return t, nil } -func NewAppsTransportWithOptions(tr http.RoundTripper, appID int64, opts ...AppsTransportOption) (*AppsTransport, error) { +func NewAppsTransportWithAllOptions(tr http.RoundTripper, opts ...AppsTransportOptionError) (*AppsTransport, error) { t := &AppsTransport{ BaseURL: apiBaseURL, Client: &http.Client{Transport: tr}, tr: tr, - appID: appID, } + for _, fn := range opts { - fn(t) + err := fn(t) + if err != nil { + return nil, err + } + } + + if t.appID == 0 { + return nil, errors.New("no appID provided") } if t.signer == nil { @@ -112,6 +138,46 @@ func (t *AppsTransport) AppID() int64 { } type AppsTransportOption func(*AppsTransport) +type AppsTransportOptionError func(*AppsTransport) error + +func WithAppID(appID int64) AppsTransportOptionError { + return func(at *AppsTransport) error { + at.appID = appID + + return nil + } +} + +func WithPrivateKeyFile(privateKeyFile string) AppsTransportOptionError { + return func(at *AppsTransport) error { + // deprecated function kept for go 1.13 compatibility + privateKey, err := ioutil.ReadFile(privateKeyFile) + if err != nil { + return fmt.Errorf("could not read private key: %w", err) + } + + return WithPrivateKeyRaw(privateKey)(at) + } +} + +func WithPrivateKeyRaw(key []byte) AppsTransportOptionError { + return func(at *AppsTransport) error { + key, err := jwt.ParseRSAPrivateKeyFromPEM(key) + if err != nil { + return fmt.Errorf("could not parse private key: %w", err) + } + + return WithPrivateKey(key)(at) + } +} + +func WithPrivateKey(key *rsa.PrivateKey) AppsTransportOptionError { + return func(at *AppsTransport) error { + at.signer = NewRSASigner(jwt.SigningMethodRS256, key) + + return nil + } +} // WithSigner configures the AppsTransport to use the given Signer for generating JWT tokens. func WithSigner(signer Signer) AppsTransportOption { From 1438121d6a25f4140419204de7c24c3d90f3115d Mon Sep 17 00:00:00 2001 From: James Telfer <792299+jamestelfer@users.noreply.github.com> Date: Thu, 23 May 2024 22:16:49 +1000 Subject: [PATCH 2/3] fix: add ClientID support --- appsTransport.go | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/appsTransport.go b/appsTransport.go index 524e747..8a3c03b 100644 --- a/appsTransport.go +++ b/appsTransport.go @@ -25,7 +25,11 @@ type AppsTransport struct { Client Client // Client to use to refresh tokens, defaults to http.Client with provided transport tr http.RoundTripper // tr is the underlying roundtripper being wrapped signer Signer // signer signs JWT tokens. - appID int64 // appID is the GitHub App's ID + issuer string // issuer is the ClientID (preferred) or AppID (legacy) of the GitHub App + // appID is the GitHub App's ID + // + // deprecated: kept only for backwards compatibility. + appID int64 } // NewAppsTransportKeyFromFile returns a AppsTransport using a private key from file. @@ -63,12 +67,14 @@ func NewAppsTransportFromPrivateKey(tr http.RoundTripper, appID int64, key *rsa. return t } +// deprecated: use NewAppsTransportWithAllOptions instead func NewAppsTransportWithOptions(tr http.RoundTripper, appID int64, opts ...AppsTransportOption) (*AppsTransport, error) { t := &AppsTransport{ BaseURL: apiBaseURL, Client: &http.Client{Transport: tr}, tr: tr, appID: appID, + //fixme } for _, fn := range opts { @@ -96,8 +102,8 @@ func NewAppsTransportWithAllOptions(tr http.RoundTripper, opts ...AppsTransportO } } - if t.appID == 0 { - return nil, errors.New("no appID provided") + if t.issuer == "" { + return nil, errors.New("no appID or clientID provided") } if t.signer == nil { @@ -117,7 +123,7 @@ func (t *AppsTransport) RoundTrip(req *http.Request) (*http.Response, error) { claims := &jwt.RegisteredClaims{ IssuedAt: jwt.NewNumericDate(iss), ExpiresAt: jwt.NewNumericDate(exp), - Issuer: strconv.FormatInt(t.appID, 10), + Issuer: t.issuer, } ss, err := t.signer.Sign(claims) @@ -132,18 +138,41 @@ func (t *AppsTransport) RoundTrip(req *http.Request) (*http.Response, error) { return resp, err } -// AppID returns the appID of the transport +// AppID returns the appID of the transport. This will return 0 if the transport +// is using a ClientID. +// +// Deprecated: use [Issuer()] instead func (t *AppsTransport) AppID() int64 { return t.appID } +// Issuer returns the appID or clientID of the GitHub app used by this +// transport. +func (t *AppsTransport) Issuer() string { + return t.issuer +} + +// deprecated: kept only for backwards compatibility. type AppsTransportOption func(*AppsTransport) type AppsTransportOptionError func(*AppsTransport) error +// Specify the AppID of the GitHub App. Either this or ClientID must be +// specified, but ClientID is now preferred. func WithAppID(appID int64) AppsTransportOptionError { return func(at *AppsTransport) error { + // backwards compatibility to support the AppID() getter at.appID = appID + at.issuer = strconv.FormatInt(appID, 10) + + return nil + } +} +// Specify the ClientID of the GitHub App. Either this or AppID must be +// specified, but ClientID is now preferred. +func WithClientID(clientID string) AppsTransportOptionError { + return func(at *AppsTransport) error { + at.issuer = clientID return nil } } From 52ba4cddc22c614b1ba84df1de015f3bc5f759ae Mon Sep 17 00:00:00 2001 From: James Telfer <792299+jamestelfer@users.noreply.github.com> Date: Mon, 3 Jun 2024 12:35:15 +1000 Subject: [PATCH 3/3] fix: use signerFunc to avoid a separate options type The signerFunc field is used only during configuration to allow the error to occur only when the func is called. --- appsTransport.go | 162 +++++++++++++++++++++++++++-------------------- 1 file changed, 93 insertions(+), 69 deletions(-) diff --git a/appsTransport.go b/appsTransport.go index 7d9eeef..b901841 100644 --- a/appsTransport.go +++ b/appsTransport.go @@ -12,6 +12,8 @@ import ( jwt "github.com/golang-jwt/jwt/v4" ) +type signerFunc = func() (Signer, error) + // AppsTransport provides a http.RoundTripper by wrapping an existing // http.RoundTripper and provides GitHub Apps authentication as a // GitHub App. @@ -21,11 +23,12 @@ import ( // // See https://developer.github.com/apps/building-integrations/setting-up-and-registering-github-apps/about-authentication-options-for-github-apps/ type AppsTransport struct { - BaseURL string // BaseURL is the scheme and host for GitHub API, defaults to https://api.github.com - Client Client // Client to use to refresh tokens, defaults to http.Client with provided transport - tr http.RoundTripper // tr is the underlying roundtripper being wrapped - signer Signer // signer signs JWT tokens. - issuer string // issuer is the ClientID (preferred) or AppID (legacy) of the GitHub App + BaseURL string // BaseURL is the scheme and host for GitHub API, defaults to https://api.github.com + Client Client // Client to use to refresh tokens, defaults to http.Client with provided transport + tr http.RoundTripper // tr is the underlying roundtripper being wrapped + signerFunc signerFunc // used at construction time to create the signer. + signer Signer // signer signs JWT tokens. + issuer string // issuer is the ClientID (preferred) or AppID (legacy) of the GitHub App // appID is the GitHub App's ID // // deprecated: kept only for backwards compatibility. @@ -67,50 +70,48 @@ func NewAppsTransportFromPrivateKey(tr http.RoundTripper, appID int64, key *rsa. return t } -// deprecated: use NewAppsTransportWithAllOptions instead +// deprecated: use [NewAppsTransportWithAllOptions] instead, passing "WithAppID(appID)". func NewAppsTransportWithOptions(tr http.RoundTripper, appID int64, opts ...AppsTransportOption) (*AppsTransport, error) { - t := &AppsTransport{ - BaseURL: apiBaseURL, - Client: &http.Client{Transport: tr}, - tr: tr, - appID: appID, - //fixme - } - - for _, fn := range opts { - fn(t) - } + allOpts := make([]AppsTransportOption, 0, len(opts)) + allOpts = append(allOpts, WithAppID(appID)) + allOpts = append(allOpts, opts...) - if t.signer == nil { - return nil, errors.New("no signer provided") - } - - return t, nil + return NewAppsTransportWithAllOptions( + tr, + allOpts..., + ) } -func NewAppsTransportWithAllOptions(tr http.RoundTripper, opts ...AppsTransportOptionError) (*AppsTransport, error) { - t := &AppsTransport{ +// NewAppsTransportWithAllOptions creates a new AppsTransport instance with the +// provided options. Options allow the issuer and signer to be set as needed; +// see the [With*] option methods for further details. +func NewAppsTransportWithAllOptions(tr http.RoundTripper, opts ...AppsTransportOption) (*AppsTransport, error) { + at := &AppsTransport{ BaseURL: apiBaseURL, Client: &http.Client{Transport: tr}, - tr: tr, } for _, fn := range opts { - err := fn(t) - if err != nil { - return nil, err - } + fn(at) } - if t.issuer == "" { - return nil, errors.New("no appID or clientID provided") + if at.issuer == "" { + return nil, errors.New("appID or clientID must be provided") } - if t.signer == nil { - return nil, errors.New("no signer provided") + if at.signerFunc == nil { + return nil, errors.New("signer (or key) must be provided") + } + + signer, err := at.signerFunc() + if err != nil { + return nil, fmt.Errorf("signer creation failed: %w", err) } - return t, nil + at.signer = signer + at.signerFunc = nil // once it's used, it's no longer needed + + return at, nil } // RoundTrip implements http.RoundTripper interface. @@ -141,7 +142,7 @@ func (t *AppsTransport) RoundTrip(req *http.Request) (*http.Response, error) { // AppID returns the appID of the transport. This will return 0 if the transport // is using a ClientID. // -// Deprecated: use [Issuer()] instead +// Deprecated: use [Issuer()] instead and specify the AppID as a string. func (t *AppsTransport) AppID() int64 { return t.appID } @@ -152,64 +153,87 @@ func (t *AppsTransport) Issuer() string { return t.issuer } -// deprecated: kept only for backwards compatibility. type AppsTransportOption func(*AppsTransport) -type AppsTransportOptionError func(*AppsTransport) error // Specify the AppID of the GitHub App. Either this or ClientID must be // specified, but ClientID is now preferred. -func WithAppID(appID int64) AppsTransportOptionError { - return func(at *AppsTransport) error { - // backwards compatibility to support the AppID() getter +func WithAppID(appID int64) AppsTransportOption { + return func(at *AppsTransport) { at.appID = appID at.issuer = strconv.FormatInt(appID, 10) - - return nil } } -// Specify the ClientID of the GitHub App. Either this or AppID must be -// specified, but ClientID is now preferred. -func WithClientID(clientID string) AppsTransportOptionError { - return func(at *AppsTransport) error { +// WithClientID specifies the ClientID of the GitHub App. Either this or AppID +// must be specified, but ClientID is now preferred. +func WithClientID(clientID string) AppsTransportOption { + return func(at *AppsTransport) { at.issuer = clientID - return nil } } -func WithPrivateKeyFile(privateKeyFile string) AppsTransportOptionError { - return func(at *AppsTransport) error { - privateKey, err := os.ReadFile(privateKeyFile) - if err != nil { - return fmt.Errorf("could not read private key: %s", err) - } +// WithSigner configures the AppsTransport to use the given Signer for +// generating JWT tokens. +func WithSigner(signer Signer) AppsTransportOption { + return func(at *AppsTransport) { + at.signerFunc = func() (Signer, error) { return signer, nil } + } +} - return WithPrivateKeyRaw(privateKey)(at) +// WithPrivateKeyFile directs the transport to use the specified private key +// file for signing requests. The private key is loaded and processed as an RSA +// key in PEM format as provided by GitHub in the application settings. +func WithPrivateKeyFile(privateKeyFile string) AppsTransportOption { + return func(at *AppsTransport) { + at.signerFunc = currySignerFunc(createSignerFromFile, privateKeyFile) } } -func WithPrivateKeyRaw(key []byte) AppsTransportOptionError { - return func(at *AppsTransport) error { - key, err := jwt.ParseRSAPrivateKeyFromPEM(key) - if err != nil { - return fmt.Errorf("could not parse private key: %w", err) - } +// WithPrivateKeyRaw directs the transport to use the specified private key for +// signing requests. The private key is loaded and processed as an RSA key in +// PEM format as provided by GitHub in the application settings. +func WithPrivateKeyRaw(key []byte) AppsTransportOption { + return func(at *AppsTransport) { + at.signerFunc = currySignerFunc(createSignerFromBytes, key) + } +} - return WithPrivateKey(key)(at) +// WithPrivateKeyRaw directs the transport to use the specified private key for +// signing requests. The private key is provided by GitHub in the application +// settings. +func WithPrivateKey(key *rsa.PrivateKey) AppsTransportOption { + return func(at *AppsTransport) { + at.signerFunc = currySignerFunc(createSignerFromKey, key) } } -func WithPrivateKey(key *rsa.PrivateKey) AppsTransportOptionError { - return func(at *AppsTransport) error { - at.signer = NewRSASigner(jwt.SigningMethodRS256, key) +func createSignerFromFile(privateKeyFile string) (Signer, error) { + privateKey, err := os.ReadFile(privateKeyFile) + if err != nil { + return nil, fmt.Errorf("could not read private key: %s", err) + } + + return createSignerFromBytes(privateKey) +} - return nil +func createSignerFromBytes(rawKey []byte) (Signer, error) { + key, err := jwt.ParseRSAPrivateKeyFromPEM(rawKey) + if err != nil { + return nil, fmt.Errorf("could not parse private key: %w", err) } + + return createSignerFromKey(key) } -// WithSigner configures the AppsTransport to use the given Signer for generating JWT tokens. -func WithSigner(signer Signer) AppsTransportOption { - return func(at *AppsTransport) { - at.signer = signer +func createSignerFromKey(key *rsa.PrivateKey) (Signer, error) { + return NewRSASigner(jwt.SigningMethodRS256, key), nil +} + +// currySignerFunc allows the createSigner* functions to be simple, callable +// functions that can be used directly, while still supporting the function +// interface required for config options. +func currySignerFunc[T any](deferred func(T) (Signer, error), arg T) signerFunc { + return func() (Signer, error) { + return deferred(arg) } }