diff options
-rw-r--r-- | Makefile.build.mk | 1 | ||||
-rw-r--r-- | go.sum | 1 | ||||
-rw-r--r-- | internal/auth/auth_test.go | 44 | ||||
-rw-r--r-- | internal/httpfs/http_fs.go | 48 | ||||
-rw-r--r-- | internal/httpfs/http_fs_test.go | 32 | ||||
-rw-r--r-- | internal/mocks/source.go | 50 | ||||
-rw-r--r-- | internal/serving/disk/symlink/path_test.go | 10 | ||||
-rw-r--r-- | internal/source/source_mock.go | 38 | ||||
-rw-r--r-- | internal/vfs/local/root_test.go | 8 | ||||
-rw-r--r-- | internal/vfs/local/vfs.go | 11 | ||||
-rw-r--r-- | internal/vfs/zip/vfs.go | 3 | ||||
-rw-r--r-- | test/acceptance/acceptance_test.go | 4 | ||||
-rw-r--r-- | test/acceptance/helpers_test.go | 4 |
13 files changed, 116 insertions, 138 deletions
diff --git a/Makefile.build.mk b/Makefile.build.mk index 3d13f47a..c3294ffe 100644 --- a/Makefile.build.mk +++ b/Makefile.build.mk @@ -11,6 +11,7 @@ setup: .GOPATH/.ok generate-mocks: .GOPATH/.ok $Q bin/mockgen -source=internal/interface.go -destination=internal/mocks/mocks.go -package=mocks + $Q bin/mockgen -source=internal/source/source.go -destination=internal/mocks/source.go -package=mocks build: .GOPATH/.ok $Q GOBIN=$(CURDIR)/bin go install $(if $V,-v) $(VERSION_FLAGS) -tags "${GO_BUILD_TAGS}" -buildmode exe $(IMPORT_PATH) @@ -262,7 +262,6 @@ github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb6 github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= github.com/spf13/viper v1.3.2/go.mod h1:ZiWeW+zYFKm7srdB9IoDzzZXaJaI5eL9QjNiN/DMA2s= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= -github.com/stretchr/objx v0.1.1 h1:2vfRuCMp5sSVIDSqO8oNnWJq7mPa6KVP3iPIwFBuy8A= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 77ea0b7a..6df0afcc 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -10,11 +10,12 @@ import ( "strings" "testing" + "github.com/golang/mock/gomock" "github.com/gorilla/sessions" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/mocks" "gitlab.com/gitlab-org/gitlab-pages/internal/request" - "gitlab.com/gitlab-org/gitlab-pages/internal/source" ) func createTestAuth(t *testing.T, internalServer string, publicServer string) *Auth { @@ -82,7 +83,11 @@ func TestTryAuthenticate(t *testing.T) { reqURL.Scheme = request.SchemeHTTPS r := &http.Request{URL: reqURL} - require.Equal(t, false, auth.TryAuthenticate(result, r, source.NewMockSource())) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockSource := mocks.NewMockSource(mockCtrl) + require.False(t, auth.TryAuthenticate(result, r, mockSource)) } func TestTryAuthenticateWithError(t *testing.T) { @@ -95,7 +100,11 @@ func TestTryAuthenticateWithError(t *testing.T) { reqURL.Scheme = request.SchemeHTTPS r := &http.Request{URL: reqURL} - require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewMockSource())) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockSource := mocks.NewMockSource(mockCtrl) + require.True(t, auth.TryAuthenticate(result, r, mockSource)) require.Equal(t, http.StatusUnauthorized, result.Code) } @@ -114,7 +123,11 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) { session.Values["state"] = "state" session.Save(r, result) - require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewMockSource())) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockSource := mocks.NewMockSource(mockCtrl) + require.True(t, auth.TryAuthenticate(result, r, mockSource)) require.Equal(t, http.StatusUnauthorized, result.Code) } @@ -136,7 +149,11 @@ func TestTryAuthenticateRemoveTokenFromRedirect(t *testing.T) { session.Values["proxy_auth_domain"] = "https://domain.com" session.Save(r, result) - require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewMockSource())) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockSource := mocks.NewMockSource(mockCtrl) + require.True(t, auth.TryAuthenticate(result, r, mockSource)) require.Equal(t, http.StatusFound, result.Code) redirect, err := url.Parse(result.Header().Get("Location")) @@ -152,7 +169,11 @@ func TestTryAuthenticateWithDomainAndState(t *testing.T) { require.NoError(t, err) r := &http.Request{URL: reqURL} - require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewMockSource())) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockSource := mocks.NewMockSource(mockCtrl) + require.True(t, auth.TryAuthenticate(result, r, mockSource)) require.Equal(t, http.StatusFound, result.Code) redirect, err := url.Parse(result.Header().Get("Location")) require.NoError(t, err) @@ -208,7 +229,12 @@ func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) { }) result := httptest.NewRecorder() - require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewMockSource())) + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockSource := mocks.NewMockSource(mockCtrl) + require.True(t, auth.TryAuthenticate(result, r, mockSource)) res := result.Result() defer res.Body.Close() @@ -474,7 +500,7 @@ func TestCheckResponseForInvalidTokenWhenInvalidToken(t *testing.T) { resp := &http.Response{StatusCode: http.StatusUnauthorized, Body: io.NopCloser(bytes.NewReader([]byte("{\"error\":\"invalid_token\"}")))} defer resp.Body.Close() - require.Equal(t, true, auth.CheckResponseForInvalidToken(result, r, resp)) + require.True(t, auth.CheckResponseForInvalidToken(result, r, resp)) res := result.Result() defer res.Body.Close() require.Equal(t, http.StatusFound, res.StatusCode) @@ -491,5 +517,5 @@ func TestCheckResponseForInvalidTokenWhenNotInvalidToken(t *testing.T) { resp := &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(bytes.NewReader([]byte("ok")))} - require.Equal(t, false, auth.CheckResponseForInvalidToken(result, r, resp)) + require.False(t, auth.CheckResponseForInvalidToken(result, r, resp)) } diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go index 3578352f..58cc9e74 100644 --- a/internal/httpfs/http_fs.go +++ b/internal/httpfs/http_fs.go @@ -8,7 +8,6 @@ package httpfs import ( "errors" - "fmt" "net/http" "os" "path" @@ -25,27 +24,15 @@ var ( // fileSystemPaths implements the http.FileSystem interface type fileSystemPaths struct { allowedPaths []string - // workaround for https://gitlab.com/gitlab-org/gitlab/-/issues/326117#note_546346101 - // where daemon-inplace-chroot=true fails to serve zip archives when - // pages_serve_with_zip_file_protocol is enabled - // TODO: evaluate if we need to remove this field when we remove - // chroot https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 - chrootPath string } // NewFileSystemPath creates a new fileSystemPaths that can be used to register // a file:// protocol with an http.Transport. -// When the daemon runs inside a chroot we need to strip chrootPath out of each -// of the allowedPaths so that we are able to find the file correctly inside -// the chroot. When Open is called, the same chrootPath will be stripped out of -// the full filepath. -func NewFileSystemPath(allowedPaths []string, chrootPath string) (http.FileSystem, error) { +func NewFileSystemPath(allowedPaths []string) (http.FileSystem, error) { for k, allowedPath := range allowedPaths { - strippedPath, err := stripChrootPath(ensureEndingSlash(allowedPath), chrootPath) - if err != nil { - return nil, err - } + strippedPath := ensureEndingSlash(allowedPath) + var err error allowedPaths[k], err = filepath.Abs(strippedPath) if err != nil { return nil, err @@ -54,7 +41,6 @@ func NewFileSystemPath(allowedPaths []string, chrootPath string) (http.FileSyste return &fileSystemPaths{ allowedPaths: allowedPaths, - chrootPath: chrootPath, }, nil } @@ -67,18 +53,7 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) { cleanedPath := filepath.FromSlash(path.Clean("/" + name)) - // since deamon can run in a chroot, we allow to define a chroot path that will be stripped from - // the FS location - // TODO: evaluate if we need to remove this check when we remove chroot - // https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 - strippedPath, err := stripChrootPath(cleanedPath, p.chrootPath) - if err != nil { - log.WithError(err).Error(os.ErrPermission) - - return nil, os.ErrPermission - } - - absPath, err := filepath.Abs(strippedPath) + absPath, err := filepath.Abs(cleanedPath) if err != nil { return nil, err } @@ -104,18 +79,3 @@ func ensureEndingSlash(path string) string { return path + "/" } - -func stripChrootPath(path, chrootPath string) (string, error) { - if chrootPath == "" { - return path, nil - } - - if !strings.HasPrefix(path, chrootPath+"/") { - return "", fmt.Errorf("allowed path %q is not in chroot path %q", path, chrootPath) - } - - // path will contain a leading `/` - path = path[len(chrootPath):] - - return path, nil -} diff --git a/internal/httpfs/http_fs_test.go b/internal/httpfs/http_fs_test.go index cbdbc0e1..ce404a11 100644 --- a/internal/httpfs/http_fs_test.go +++ b/internal/httpfs/http_fs_test.go @@ -19,7 +19,6 @@ func TestFSOpen(t *testing.T) { tests := map[string]struct { allowedPaths []string - chrootPath string fileName string expectedContent string expectedErrMsg string @@ -59,23 +58,11 @@ func TestFSOpen(t *testing.T) { fileName: wd + "/../httpfs/testdata/file1.txt", expectedErrMsg: os.ErrPermission.Error(), }, - "chroot_path_not_found_when_not_in_real_chroot": { - allowedPaths: []string{wd + "/testdata"}, - fileName: wd + "/testdata/file1.txt", - expectedErrMsg: "no such file or directory", - chrootPath: wd + "/testdata", - }, - "chroot_path_empty_finds_file": { - allowedPaths: []string{wd + "/testdata"}, - fileName: wd + "/testdata/file1.txt", - expectedContent: "file1.txt\n", - chrootPath: "", - }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - p, err := NewFileSystemPath(test.allowedPaths, test.chrootPath) + p, err := NewFileSystemPath(test.allowedPaths) require.NoError(t, err) got, err := p.Open(test.fileName) @@ -101,7 +88,6 @@ func TestFileSystemPathCanServeHTTP(t *testing.T) { tests := map[string]struct { path string - chrootPath string fileName string escapeURL bool expectedStatusCode int @@ -150,26 +136,12 @@ func TestFileSystemPathCanServeHTTP(t *testing.T) { expectedStatusCode: http.StatusForbidden, expectedContent: "403 Forbidden\n", }, - "chroot_path_fails_in_unit_test_not_found_when_not_in_real_chroot": { - path: wd + "/testdata", - fileName: "file1.txt", - chrootPath: wd + "/testdata", - expectedStatusCode: http.StatusNotFound, - expectedContent: "404 page not found\n", - }, - "chroot_path_empty_in_unit_test_file_found": { - path: wd + "/testdata", - fileName: "file1.txt", - chrootPath: "", - expectedStatusCode: http.StatusOK, - expectedContent: "file1.txt\n", - }, } for name, test := range tests { t.Run(name, func(t *testing.T) { transport := httptransport.NewTransport() - fs, err := NewFileSystemPath([]string{test.path}, test.chrootPath) + fs, err := NewFileSystemPath([]string{test.path}) require.NoError(t, err) transport.RegisterProtocol("file", http.NewFileTransport(fs)) diff --git a/internal/mocks/source.go b/internal/mocks/source.go new file mode 100644 index 00000000..c6cc6216 --- /dev/null +++ b/internal/mocks/source.go @@ -0,0 +1,50 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: internal/source/source.go + +// Package mocks is a generated GoMock package. +package mocks + +import ( + context "context" + gomock "github.com/golang/mock/gomock" + domain "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + reflect "reflect" +) + +// MockSource is a mock of Source interface +type MockSource struct { + ctrl *gomock.Controller + recorder *MockSourceMockRecorder +} + +// MockSourceMockRecorder is the mock recorder for MockSource +type MockSourceMockRecorder struct { + mock *MockSource +} + +// NewMockSource creates a new mock instance +func NewMockSource(ctrl *gomock.Controller) *MockSource { + mock := &MockSource{ctrl: ctrl} + mock.recorder = &MockSourceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockSource) EXPECT() *MockSourceMockRecorder { + return m.recorder +} + +// GetDomain mocks base method +func (m *MockSource) GetDomain(arg0 context.Context, arg1 string) (*domain.Domain, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetDomain", arg0, arg1) + ret0, _ := ret[0].(*domain.Domain) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetDomain indicates an expected call of GetDomain +func (mr *MockSourceMockRecorder) GetDomain(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDomain", reflect.TypeOf((*MockSource)(nil).GetDomain), arg0, arg1) +} diff --git a/internal/serving/disk/symlink/path_test.go b/internal/serving/disk/symlink/path_test.go index 400432c2..21880efa 100644 --- a/internal/serving/disk/symlink/path_test.go +++ b/internal/serving/disk/symlink/path_test.go @@ -6,6 +6,8 @@ package symlink_test import ( "context" + "errors" + "io/fs" "os" "path/filepath" "runtime" @@ -20,7 +22,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local" ) -var fs = vfs.Instrumented(&local.VFS{}) +var localFs = vfs.Instrumented(&local.VFS{}) type EvalSymlinksTest struct { // If dest is empty, the path is created; otherwise the dest is symlinked to the path. @@ -70,7 +72,7 @@ func simpleJoin(path ...string) string { } func testEvalSymlinks(t *testing.T, wd, path, want string) { - root, err := fs.Root(context.Background(), wd) + root, err := localFs.Root(context.Background(), wd) require.NoError(t, err) have, err := symlink.EvalSymlinks(context.Background(), root, path) @@ -125,7 +127,7 @@ func TestEvalSymlinksIsNotExist(t *testing.T) { root, _ := testhelpers.TmpDir(t, "symlink_tests") _, err := symlink.EvalSymlinks(context.Background(), root, "notexist") - if !os.IsNotExist(err) { + if !errors.Is(err, fs.ErrNotExist) { t.Errorf("expected the file is not found, got %v\n", err) } @@ -136,7 +138,7 @@ func TestEvalSymlinksIsNotExist(t *testing.T) { defer os.Remove("link") _, err = symlink.EvalSymlinks(context.Background(), root, "link") - if !os.IsNotExist(err) { + if !errors.Is(err, fs.ErrNotExist) { t.Errorf("expected the file is not found, got %v\n", err) } } diff --git a/internal/source/source_mock.go b/internal/source/source_mock.go deleted file mode 100644 index 67e80833..00000000 --- a/internal/source/source_mock.go +++ /dev/null @@ -1,38 +0,0 @@ -package source - -import ( - "context" - - "github.com/stretchr/testify/mock" - - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" -) - -// MockSource can be used for testing -type MockSource struct { - mock.Mock -} - -// GetDomain is a mocked function -func (m *MockSource) GetDomain(ctx context.Context, name string) (*domain.Domain, error) { - args := m.Called(name) - err := args.Error(1) - - d, ok := args.Get(0).(*domain.Domain) - if !ok { - return nil, err - } - - return d, err -} - -func (m *MockSource) IsReady() bool { - args := m.Called() - - return args.Get(0).(bool) -} - -// NewMockSource returns a new Source mock for testing -func NewMockSource() *MockSource { - return &MockSource{} -} diff --git a/internal/vfs/local/root_test.go b/internal/vfs/local/root_test.go index 1cf505bf..a835bae3 100644 --- a/internal/vfs/local/root_test.go +++ b/internal/vfs/local/root_test.go @@ -2,7 +2,9 @@ package local import ( "context" + "errors" "io" + "io/fs" "os" "path/filepath" "testing" @@ -95,7 +97,7 @@ func TestReadlink(t *testing.T) { target, err := root.Readlink(ctx, test.path) if test.expectedIsNotExist { - require.Equal(t, test.expectedIsNotExist, os.IsNotExist(err), "IsNotExist") + require.Equal(t, test.expectedIsNotExist, errors.Is(err, fs.ErrNotExist), "IsNotExist") return } @@ -211,7 +213,7 @@ func TestLstat(t *testing.T) { fi, err := root.Lstat(ctx, test.path) if test.expectedIsNotExist { - require.Equal(t, test.expectedIsNotExist, os.IsNotExist(err), "IsNotExist") + require.Equal(t, test.expectedIsNotExist, errors.Is(err, fs.ErrNotExist), "IsNotExist") return } @@ -271,7 +273,7 @@ func TestOpen(t *testing.T) { } if test.expectedIsNotExist { - require.Equal(t, test.expectedIsNotExist, os.IsNotExist(err), "IsNotExist") + require.Equal(t, test.expectedIsNotExist, errors.Is(err, fs.ErrNotExist), "IsNotExist") return } diff --git a/internal/vfs/local/vfs.go b/internal/vfs/local/vfs.go index ea54e8e8..5cca94fd 100644 --- a/internal/vfs/local/vfs.go +++ b/internal/vfs/local/vfs.go @@ -3,6 +3,7 @@ package local import ( "context" "errors" + "io/fs" "os" "path/filepath" @@ -14,21 +15,21 @@ var errNotDirectory = errors.New("path needs to be a directory") type VFS struct{} -func (fs VFS) Root(ctx context.Context, path string) (vfs.Root, error) { +func (localFs VFS) Root(ctx context.Context, path string) (vfs.Root, error) { rootPath, err := filepath.Abs(path) if err != nil { return nil, err } rootPath, err = filepath.EvalSymlinks(rootPath) - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { return nil, &vfs.ErrNotExist{Inner: err} } else if err != nil { return nil, err } fi, err := os.Lstat(rootPath) - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { return nil, &vfs.ErrNotExist{Inner: err} } else if err != nil { return nil, err @@ -41,11 +42,11 @@ func (fs VFS) Root(ctx context.Context, path string) (vfs.Root, error) { return &Root{rootPath: rootPath}, nil } -func (fs *VFS) Name() string { +func (localFs *VFS) Name() string { return "local" } -func (fs *VFS) Reconfigure(*config.Config) error { +func (localFs *VFS) Reconfigure(*config.Config) error { // noop return nil } diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index 4118dd4a..c90d7614 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -123,8 +123,7 @@ func (fs *zipVFS) Reconfigure(cfg *config.Config) error { } func (fs *zipVFS) reconfigureTransport(cfg *config.Config) error { - // TODO: remove chrootPath from httpsfs package: https://gitlab.com/gitlab-org/gitlab-pages/-/issues/598 - fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths, "") + fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths) if err != nil { return err } diff --git a/test/acceptance/acceptance_test.go b/test/acceptance/acceptance_test.go index 9d0c0cd2..b36bd63d 100644 --- a/test/acceptance/acceptance_test.go +++ b/test/acceptance/acceptance_test.go @@ -1,8 +1,10 @@ package acceptance_test import ( + "errors" "flag" "fmt" + "io/fs" "log" "os" "testing" @@ -58,7 +60,7 @@ func TestMain(m *testing.M) { os.Exit(0) } - if _, err := os.Stat(*pagesBinary); os.IsNotExist(err) { + if _, err := os.Stat(*pagesBinary); errors.Is(err, fs.ErrNotExist) { log.Fatalf("Couldn't find gitlab-pages binary at %s\n", *pagesBinary) } diff --git a/test/acceptance/helpers_test.go b/test/acceptance/helpers_test.go index 33a19127..d9f2f14c 100644 --- a/test/acceptance/helpers_test.go +++ b/test/acceptance/helpers_test.go @@ -6,8 +6,10 @@ import ( "crypto/tls" "crypto/x509" "encoding/json" + "errors" "fmt" "io" + "io/fs" "net" "net/http" "net/http/httptest" @@ -554,7 +556,7 @@ func (o *stubOpts) getAPICalled() bool { func lookupFromFile(t *testing.T, domain string, w http.ResponseWriter) { fixture, err := os.Open("../../shared/lookups/" + domain + ".json") - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { w.WriteHeader(http.StatusNoContent) t.Logf("GitLab domain %s source stub served 204", domain) |