- Notifications
You must be signed in to change notification settings - Fork838
Description
I was recently investigating an issue that manifested as Git operations failing to complete while API access continued to function when accessing a popular Git provider. The issue was caused by SSO enforcement on the credentials being used: the provider was configured to require an SSO check every 24h for Git operations.
Unfortunately, the only messaging I was getting out of this library was "authorization failed" (coming fromhttps://github.com/go-git/go-git/blob/master/plumbing/transport/common.go#L35).
Digging into this a bit, I found thetransport/http
error handling, which discards the response body when the HTTP code maps to a known error type. This hides the message from the Git provider which is akin to "account configured for SSO" or some such.
I think a better way to handle these errors would be to wrap the common error kind usingfmt.Errorf("%w: %s", err, reason)
. This is compatible with the current error output if folks are usingerrors.Is(err, target) bool
to check error equality (which folks should be doing).
With regards to being able to pass the reason string back to the caller: I don't expect that any Git provider would generate output for these errors that would be problematic to pass back to a consumer of the library.
eg:
// instead of thisswitch r.StatusCode {case http.StatusUnauthorized:return transport.ErrAuthenticationRequiredcase http.StatusForbidden:return transport.ErrAuthorizationFailedcase http.StatusNotFound:return transport.ErrRepositoryNotFound}// do thisswitch r.StatusCode {case http.StatusUnauthorized:return fmt.Errorf("%w: %s", transport.ErrAuthenticationRequired, reason)case http.StatusForbidden:return fmt.Errorf("%w: %s", transport.ErrAuthorizationFailed, reason)case http.StatusNotFound:return fmt.Errorf("%w: %s", transport.ErrRepositoryNotFound, reason)}