diff --git a/handlers.go b/handlers.go index b2fbe16..d8a98bf 100644 --- a/handlers.go +++ b/handlers.go @@ -4,7 +4,7 @@ import ( "encoding/json" "io" "net/http" - "net/url" + "strings" "github.com/jamestelfer/chinmina-bridge/internal/credentialhandler" "github.com/jamestelfer/chinmina-bridge/internal/jwt" @@ -59,18 +59,14 @@ func handlePostGitCredentials(tokenVendor vendor.PipelineTokenVendor) http.Handl return } - u, _ := url.Parse("https://github.com") - if protocol, ok := requestedRepo.Lookup("protocol"); ok { - u.Scheme = protocol - } - if host, ok := requestedRepo.Lookup("host"); ok { - u.Host = host - } - if path, ok := requestedRepo.Lookup("path"); ok { - u.Path = path + requestedRepoURL, err := credentialhandler.ConstructRepositoryURL(requestedRepo) + if err != nil { + log.Info().Msgf("invalid request parameters %v\n", err) + requestError(w, http.StatusBadRequest) + return } - tokenResponse, err := tokenVendor(r.Context(), claims, u.String()) + tokenResponse, err := tokenVendor(r.Context(), claims, requestedRepoURL) if err != nil { log.Info().Msgf("token creation failed %v\n", err) requestError(w, http.StatusInternalServerError) @@ -79,10 +75,13 @@ func handlePostGitCredentials(tokenVendor vendor.PipelineTokenVendor) http.Handl w.Header().Set("Content-Type", "text/plain") - // repo mismatch: empty return + // Given repository doesn't match the pipeline: empty return this means + // that we understand the request but cannot fulfil it: this is a + // successful case for a credential helper, so we successfully return + // but don't offer credentials. if tokenResponse == nil { - w.WriteHeader(http.StatusOK) w.Header().Add("Content-Length", "0") + w.WriteHeader(http.StatusOK) return } @@ -98,7 +97,7 @@ func handlePostGitCredentials(tokenVendor vendor.PipelineTokenVendor) http.Handl props := credentialhandler.NewMap(6) props.Set("protocol", tokenURL.Scheme) props.Set("host", tokenURL.Host) - props.Set("path", tokenURL.Path) + props.Set("path", strings.TrimPrefix(tokenURL.Path, "/")) props.Set("username", "x-access-token") props.Set("password", tokenResponse.Token) props.Set("password_expiry_utc", tokenResponse.ExpiryUnix()) diff --git a/handlers_test.go b/handlers_test.go index 03adae6..fb8fb79 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/auth0/go-jwt-middleware/v2/validator" + "github.com/jamestelfer/chinmina-bridge/internal/credentialhandler" "github.com/jamestelfer/chinmina-bridge/internal/jwt" "github.com/jamestelfer/chinmina-bridge/internal/vendor" "github.com/stretchr/testify/assert" @@ -54,16 +55,7 @@ func TestHandlers_RequireClaims(t *testing.T) { func TestHandlePostToken_ReturnsTokenOnSuccess(t *testing.T) { tokenVendor := tv("expected-token-value") - ctx := context.Background() - ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{ - RegisteredClaims: validator.RegisteredClaims{ - Issuer: "https://buildkite.com", - }, - CustomClaims: &jwt.BuildkiteClaims{ - OrganizationSlug: "organization-slug", - PipelineSlug: "pipeline-slug", - }, - }) + ctx := claimsContext() req, err := http.NewRequest("POST", "/token", nil) require.NoError(t, err) @@ -93,16 +85,7 @@ func TestHandlePostToken_ReturnsTokenOnSuccess(t *testing.T) { func TestHandlePostToken_ReturnsFailureOnVendorFailure(t *testing.T) { tokenVendor := tvFails(errors.New("vendor failure")) - ctx := context.Background() - ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{ - RegisteredClaims: validator.RegisteredClaims{ - Issuer: "https://buildkite.com", - }, - CustomClaims: &jwt.BuildkiteClaims{ - OrganizationSlug: "organization-slug", - PipelineSlug: "pipeline-slug", - }, - }) + ctx := claimsContext() req, err := http.NewRequest("POST", "/token", nil) require.NoError(t, err) @@ -123,18 +106,15 @@ func TestHandlePostToken_ReturnsFailureOnVendorFailure(t *testing.T) { func TestHandlePostGitCredentials_ReturnsTokenOnSuccess(t *testing.T) { tokenVendor := tv("expected-token-value") - ctx := context.Background() - ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{ - RegisteredClaims: validator.RegisteredClaims{ - Issuer: "https://buildkite.com", - }, - CustomClaims: &jwt.BuildkiteClaims{ - OrganizationSlug: "organization-slug", - PipelineSlug: "pipeline-slug", - }, - }) + ctx := claimsContext() + + m := credentialhandler.NewMap(10) + m.Set("protocol", "https") + m.Set("host", "github.com") + m.Set("path", "org/repo") - body := bytes.NewBufferString("\n\n\n\nuseless-content") + body := &bytes.Buffer{} + credentialhandler.WriteProperties(m, body) req, err := http.NewRequest("POST", "/git-credentials", body) require.NoError(t, err) @@ -150,24 +130,109 @@ func TestHandlePostGitCredentials_ReturnsTokenOnSuccess(t *testing.T) { assert.Equal(t, "text/plain", rr.Header().Get("Content-Type")) respBody := rr.Body.String() - assert.Equal(t, "protocol=https\nhost=github.com\npath=\nusername=x-access-token\npassword=expected-token-value\npassword_expiry_utc=1715104776\n\n", respBody) + assert.Equal(t, "protocol=https\nhost=github.com\npath=org/repo\nusername=x-access-token\npassword=expected-token-value\npassword_expiry_utc=1715104776\n\n", respBody) +} + +func TestHandlePostGitCredentials_ReturnsEmptySuccessWhenNoToken(t *testing.T) { + tokenVendor := vendor.PipelineTokenVendor(func(_ context.Context, claims jwt.BuildkiteClaims, repoUrl string) (*vendor.PipelineRepositoryToken, error) { + return nil, nil + }) + + ctx := claimsContext() + + m := credentialhandler.NewMap(10) + m.Set("protocol", "https") + m.Set("host", "github.com") + m.Set("path", "org/repo") + + body := &bytes.Buffer{} + credentialhandler.WriteProperties(m, body) + req, err := http.NewRequest("POST", "/git-credentials", body) + require.NoError(t, err) + + req = req.WithContext(ctx) + rr := httptest.NewRecorder() + + // act + handler := handlePostGitCredentials(tokenVendor) + handler.ServeHTTP(rr, req) + + // assert + r := rr.Result() + assert.Equal(t, http.StatusOK, r.StatusCode) + assert.Equal(t, "text/plain", r.Header.Get("Content-Type")) + assert.Equal(t, "0", r.Header.Get("Content-Length")) + assert.Equal(t, int64(0), r.ContentLength) + + respBody := rr.Body.String() + assert.Equal(t, "", respBody) +} +func TestHandlePostGitCredentials_ReturnsFailureOnInvalidRequest(t *testing.T) { + tokenVendor := tv("expected-token-value") + + ctx := claimsContext() + + req, err := http.NewRequest("POST", "/git-credentials", nil) + require.NoError(t, err) + + req = req.WithContext(ctx) + rr := httptest.NewRecorder() + + // act + handler := handlePostGitCredentials(tokenVendor) + handler.ServeHTTP(rr, req) + + // assert + assert.Equal(t, http.StatusBadRequest, rr.Code) + // important to know that internal details aren't part of the error response + assert.Equal(t, "Bad Request\n", rr.Body.String()) +} + +func TestHandlePostGitCredentials_ReturnsFailureOnReadFailure(t *testing.T) { + tokenVendor := tv("expected-token-value") + + ctx := claimsContext() + + m := credentialhandler.NewMap(10) + m.Set("protocol", "https") + m.Set("host", "github.com") + m.Set("path", "org/repo") + + body := &bytes.Buffer{} + credentialhandler.WriteProperties(m, body) + + req, err := http.NewRequest("POST", "/git-credentials", body) + require.NoError(t, err) + + req = req.WithContext(ctx) + rr := httptest.NewRecorder() + + // act + handler := maxRequestSize(1)( + // use the request size limit to force an error in the credentials handler + handlePostGitCredentials(tokenVendor), + ) + handler.ServeHTTP(rr, req) + + // assert + assert.Equal(t, http.StatusInternalServerError, rr.Code) + // important to know that internal details aren't part of the error response + assert.Equal(t, "Internal Server Error\n", rr.Body.String()) } func TestHandlePostGitCredentials_ReturnsFailureOnVendorFailure(t *testing.T) { tokenVendor := tvFails(errors.New("vendor failure")) - ctx := context.Background() - ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{ - RegisteredClaims: validator.RegisteredClaims{ - Issuer: "https://buildkite.com", - }, - CustomClaims: &jwt.BuildkiteClaims{ - OrganizationSlug: "organization-slug", - PipelineSlug: "pipeline-slug", - }, - }) + ctx := claimsContext() - req, err := http.NewRequest("POST", "/token", nil) + m := credentialhandler.NewMap(10) + m.Set("protocol", "https") + m.Set("host", "github.com") + m.Set("path", "org/repo") + + body := &bytes.Buffer{} + credentialhandler.WriteProperties(m, body) + req, err := http.NewRequest("POST", "/git-credentials", body) require.NoError(t, err) req = req.WithContext(ctx) @@ -201,6 +266,22 @@ func tvFails(err error) vendor.PipelineTokenVendor { }) } +func claimsContext() context.Context { + ctx := context.Background() + + ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{ + RegisteredClaims: validator.RegisteredClaims{ + Issuer: "https://buildkite.com", + }, + CustomClaims: &jwt.BuildkiteClaims{ + OrganizationSlug: "organization-slug", + PipelineSlug: "pipeline-slug", + }, + }) + + return ctx +} + func TestMaxRequestSizeMiddleware(t *testing.T) { mw := maxRequestSize(10) diff --git a/internal/credentialhandler/marshal.go b/internal/credentialhandler/marshal.go index 882fe89..4cad480 100644 --- a/internal/credentialhandler/marshal.go +++ b/internal/credentialhandler/marshal.go @@ -5,6 +5,7 @@ import ( "bytes" "errors" "io" + "net/url" "strings" ) @@ -74,3 +75,28 @@ func WriteProperties(props *ArrayMap, w io.Writer) error { return nil } + +func ConstructRepositoryURL(props *ArrayMap) (string, error) { + u := &url.URL{} + + protocol, ok := props.Lookup("protocol") + if !ok { + return "", errors.New("protocol/scheme must be present") + } + + host, ok := props.Lookup("host") + if !ok { + return "", errors.New("host must be present") + } + + path, ok := props.Lookup("path") + if !ok { + return "", errors.New("path must be present") + } + + u.Scheme = protocol + u.Host = host + u.Path = path + + return u.String(), nil +} diff --git a/internal/credentialhandler/marshal_test.go b/internal/credentialhandler/marshal_test.go index 96d3c76..606c9eb 100644 --- a/internal/credentialhandler/marshal_test.go +++ b/internal/credentialhandler/marshal_test.go @@ -211,3 +211,108 @@ func TestWriteProperties(t *testing.T) { }) } } + +func TestConstructURL(t *testing.T) { + cases := []struct { + name string + input [][]string + expected string + failed bool + failure string + }{ + { + name: "fails on empty", + input: [][]string{}, + expected: "", + failed: true, + failure: "protocol/scheme must be present", + }, + { + name: "fails without scheme", + input: [][]string{ + {"host", "github.com"}, + {"path", "org/repo.git"}, + }, + expected: "", + failed: true, + failure: "protocol/scheme must be present", + }, + { + name: "fails without host", + input: [][]string{ + {"protocol", "https"}, + {"path", "org/repo.git"}, + }, + expected: "", + failed: true, + failure: "host must be present", + }, + { + name: "fails without path", + input: [][]string{ + {"protocol", "https"}, + {"host", "github.com"}, + }, + expected: "", + failed: true, + failure: "path must be present", + }, + { + // validating the correct host is handled elsewhere, outside the + // responsibility of this function + name: "succeeds with non-Github", + input: [][]string{ + {"protocol", "https"}, + {"host", "gitlubber.com"}, + {"path", "org/repo.git"}, + }, + expected: "https://gitlubber.com/org/repo.git", + }, + { + // the URL may not be correct, but that's OK: the pipeline won't + // match and the application can't create a token for it. + name: "succeeds with incorrectly formed path", + input: [][]string{ + {"protocol", "https"}, + {"host", "github.com"}, + {"path", "org/much-too-long/repo.git"}, + }, + expected: "https://github.com/org/much-too-long/repo.git", + }, + { + name: "succeeds when correctly formed", + input: [][]string{ + {"protocol", "https"}, + {"host", "github.com"}, + {"path", "org/repo.git"}, + }, + expected: "https://github.com/org/repo.git", + }, + { + name: "succeeds when correctly formed with unknown keys", + input: [][]string{ + {"protocol", "https"}, + {"host", "github.com"}, + {"path", "org/repo.git"}, + {"x-host", "gitgrub.com"}, + {"hostlike", "unhostly.com"}, + {"pathymcpathface", "how many paths must we walk down"}, + }, + expected: "https://github.com/org/repo.git", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + input := credentialhandler.NewMapFromArray(c.input) + url, err := credentialhandler.ConstructRepositoryURL(input) + require.Equal(t, err != nil, c.failed, "%v", err) + + if c.failed { + assert.ErrorContains(t, err, c.failure) + } + + assert.Equal(t, c.expected, url) + }) + } +}